#301 [dist-git][keygen][selinux] move selinux rules to copr-selinux
Merged 2 years ago by msuchy. Opened 2 years ago by praiskup.
copr/ praiskup/copr selinux-movement  into  master

@@ -71,6 +71,7 @@ 

  Requires:   prunerepo

  Requires:   libappstream-glib-builder

  Requires:   rpm-sign

+ Requires:   (copr-selinux if selinux-policy-targeted)

  

  Requires(post): systemd

  Requires(preun): systemd

file modified
+1 -3
@@ -31,6 +31,7 @@ 

  Requires: python3-rpkg

  Requires: python3-munch

  Requires: findutils

+ Requires: (copr-selinux if selinux-policy-targeted)

  

  %{?fedora:Requires(post): policycoreutils-python-utils}

  %{?rhel:Requires(post): policycoreutils-python}
@@ -85,9 +86,6 @@ 

  ./run_tests.sh

  

  %post

- # change context to be readable by cgit

- semanage fcontext -a -t httpd_sys_content_t '/var/lib/copr-dist-git(/.*)?'

- restorecon -rv /var/lib/copr-dist-git

  %systemd_post copr-dist-git.service

  

  %preun

@@ -144,6 +144,7 @@ 

  Requires: js-html5shiv

  Requires: js-respond

  Requires: python3-copr-common

+ Requires: (copr-selinux if selinux-policy-targeted)

  

  Provides: bundled(bootstrap-combobox) = 1.1.6

  Provides: bundled(bootstrap-select) = 1.5.4

file modified
-6
@@ -127,12 +127,6 @@ 

  

  

  %post

- semanage fcontext -a -t httpd_log_t '/var/log/copr-keygen(/.*)?'

- restorecon -rv /var/log/copr-keygen/

- 

- semanage fcontext -a -t httpd_var_lib_t '/var/lib/copr-keygen(/.*)?'

- restorecon -rv /var/lib/copr-keygen/

- 

  service httpd condrestart &>/dev/null || :

  

  

file modified
+30 -41
@@ -4,9 +4,10 @@ 

  

  %global moduletype apps

  %global modulename copr

- %{!?_selinux_policy_version: %global _selinux_policy_version %(sed -e 's,.*selinux-policy-\\([^/]*\\)/.*,\\1,' %{_datadir}/selinux/devel/policyhelp 2>/dev/null)}

- %global file_context_file %{_sysconfdir}/selinux/targeted/contexts/files/file_contexts

- %global file_context_file_pre %{_localstatedir}/lib/rpm-state/file_contexts.pre

+ %global selinuxbooleans httpd_enable_cgi=1 httpd_can_network_connect=1 httpd_can_sendmail=1 nis_enabled=1

+ # We can build 'mls' too, once this is resolved:

+ # https://github.com/fedora-selinux/selinux-policy-macros/pull/4

+ %global selinuxvariants targeted

  

  Name:       {{{ git_dir_name }}}

  Version:    {{{ git_dir_version lead=1 }}}
@@ -25,20 +26,10 @@ 

  BuildArch:  noarch

  BuildRequires: asciidoc

  BuildRequires: libxslt

- BuildRequires:  checkpolicy, selinux-policy-devel

- BuildRequires:  policycoreutils

  BuildRequires:  perl

- Requires(post): policycoreutils, libselinux-utils

- %if 0%{?rhel} && 0%{?rhel} <= 7

- Requires(post): policycoreutils-python

- %else

- Requires(post): policycoreutils-python-utils

- %endif

- Requires(post): selinux-policy-targeted

- Requires(postun): policycoreutils

- %if "%{_selinux_policy_version}" != ""

- Requires:      selinux-policy >= %{_selinux_policy_version}

- %endif

+ 

+ BuildRequires: selinux-policy-devel

+ %{?selinux_requires}

  

  

  %description
@@ -57,15 +48,16 @@ 

  a2x -d manpage -f manpage man/copr-selinux-relabel.8.asciidoc

  

  perl -i -pe 'BEGIN { $VER = join ".", grep /^\d+$/, split /\./, "%{version}.%{release}"; } s!\@\@VERSION\@\@!$VER!g;' %{modulename}.te

- for selinuxvariant in targeted mls; do

+ for selinuxvariant in %selinuxvariants; do

      make NAME=${selinuxvariant} -f %{_datadir}/selinux/devel/Makefile

      bzip2 -9 %{modulename}.pp

      mv %{modulename}.pp.bz2 %{modulename}.pp.bz2.${selinuxvariant}

      make NAME=${selinuxvariant} -f %{_datadir}/selinux/devel/Makefile clean

  done

  

+ 

  %install

- for selinuxvariant in targeted mls; do

+ for selinuxvariant in %selinuxvariants; do

      install -d %{buildroot}%{_datadir}/selinux/${selinuxvariant}

      install -p -m 644 %{modulename}.pp.bz2.${selinuxvariant} \

             %{buildroot}%{_datadir}/selinux/${selinuxvariant}/%{modulename}.pp.bz2
@@ -74,41 +66,39 @@ 

  install -d %{buildroot}%{_datadir}/selinux/devel/include/%{moduletype}

  install -p -m 644 %{modulename}.if \

    %{buildroot}%{_datadir}/selinux/devel/include/%{moduletype}/%{modulename}.if

- # Install copr-selinux-enable which will be called in %%posttrans

  install -d %{buildroot}%{_sbindir}

  install -p -m 755 %{name}-enable %{buildroot}%{_sbindir}/%{name}-enable

  install -p -m 755 %{name}-relabel %{buildroot}%{_sbindir}/%{name}-relabel

- 

  install -d %{buildroot}%{_mandir}/man8

  install -p -m 644 man/%{name}-enable.8 %{buildroot}/%{_mandir}/man8/

  install -p -m 644 man/%{name}-relabel.8 %{buildroot}/%{_mandir}/man8/

  

+ 

  %pre

- if /usr/sbin/selinuxenabled ; then

-    [ -f %{file_context_file_pre} ] || cp -f %{file_context_file} %{file_context_file_pre}

- fi

+ for selinuxvariant in %selinuxvariants; do

+   %selinux_relabel_pre -s $selinuxvariant

+ done

+ 

  

  %post

- if /usr/sbin/selinuxenabled ; then

-    %{_sbindir}/%{name}-enable

- fi

+ for selinuxvariant in %selinuxvariants; do

+   %selinux_modules_install -s $selinuxvariant %{_datadir}/selinux/${selinuxvariant}/%{modulename}.pp.bz2

+   %selinux_set_booleans    -s $selinuxvariant %{selinuxbooleans}

+ done

  

- %posttrans

- if /usr/sbin/selinuxenabled ; then

-    if [ -f %{file_context_file_pre} ]; then

-      /usr/sbin/fixfiles -C %{file_context_file_pre} restore

-      rm -f %{file_context_file_pre}

-    fi

- fi

  

  %postun

- # Clean up after package removal

- if [ $1 -eq 0 ]; then

-   for selinuxvariant in targeted mls; do

-       /usr/sbin/semodule -s ${selinuxvariant} -l > /dev/null 2>&1 \

-         && /usr/sbin/semodule -s ${selinuxvariant} -r %{modulename} || :

-     done

- fi

+ for selinuxvariant in %selinuxvariants; do

+   %selinux_modules_uninstall -s $selinuxvariant %{modulename}

+   %selinux_unset_booleans    -s $selinuxvariant %{selinuxbooleans}

+ done

+ 

+ 

+ %posttrans

+ for selinuxvariant in %selinuxvariants; do

+   %selinux_relabel_post -s $selinuxvariant

+ done

+ 

  

  %files

  %license LICENSE
@@ -119,7 +109,6 @@ 

  %{_sbindir}/%{name}-relabel

  %{_mandir}/man8/%{name}-enable.8*

  %{_mandir}/man8/%{name}-relabel.8*

- %dir %{_datadir}/selinux/mls

  

  %changelog

  {{{ git_dir_changelog since_tag=copr-selinux-1.49-1 }}}

file modified
+9 -7
@@ -1,7 +1,9 @@ 

- 

- /var/lib/copr(/.*)? gen_context(system_u:object_r:copr_data_t,s0)

- /var/log/copr-backend(/.*)? gen_context(system_u:object_r:copr_httpd_log_t,s0)

- /var/log/copr-frontend(/.*)? gen_context(system_u:object_r:copr_httpd_log_t,s0)

- /var/lib/dist-git/git/rpms(/.*)? gen_context(system_u:object_r:git_user_content_t,s0)

- /usr/libexec/git-core/git-http-backend gen_context(system_u:object_r:git_script_exec_t,s0)

- /var/lib/copr-keygen/gnupg/trustdb.gpg gen_context(system_u:object_r:httpd_var_lib_t,s0)

+ /var/lib/copr(/.*)?                     gen_context(system_u:object_r:copr_data_t,s0)

+ /var/log/copr-backend(/.*)?             gen_context(system_u:object_r:copr_httpd_log_t,s0)

+ /var/log/copr-frontend(/.*)?            gen_context(system_u:object_r:copr_httpd_log_t,s0)

+ /var/lib/dist-git/git/rpms(/.*)?        gen_context(system_u:object_r:git_user_content_t,s0)

+ /usr/libexec/git-core/git-http-backend  gen_context(system_u:object_r:git_script_exec_t,s0)

+ /var/lib/copr-dist-git(/.*)?            gen_context(system_u:object_r:httpd_sys_content_t,s0)

+ /var/lib/copr-keygen(/.*)?              gen_context(system_u:object_r:httpd_var_lib_t,s0)

+ /var/lib/copr-keygen/gnupg/trustdb.gpg  gen_context(system_u:object_r:httpd_var_lib_t,s0)

+ /var/log/copr-keygen(/.*)?              gen_context(system_u:object_r:httpd_log_t,s0)

... mainly to avoid using 'semanage/restorecon' in rpm scriptlets;
it's somewhat risky because the tools can fail and the rest of the
calling scriptlet would be skipped (shell's set -e feature).

LGTM. We will need to take care of installing copr-selinux on distgit and keygen machine though, because it is not specified in Requires.

Can someone try this patch for fedora infra playbooks? Then we could merge it as well as this PR.

Can someone try this patch for fedora infra playbooks? Then we could merge it as well as this PR.

Good question. Looking at the patch, I did not know that the copr-selinux isn't installed at all :-( which brings some risks to this patch actually.

rebased onto bf202b9b9c03702c8b499600c5c7ab81ca40da9b

2 years ago

Rebased, adding trello card for today.

You need to alter copr-selinux-relabel as well.

On Tuesday, June 19, 2018 4:57:00 PM CEST Miroslav Such=FD wrote:

You need to alter copr-selinux-relabel as well.

Hm. That file isn't called in scriptlets, as otherwise claimed in
manpage:

    DESCRIPTION
       The copr-selinux-relabel program will call restorecon(8) on all Copr
       files. This effectively restores the SELinux context.

       The program is called by an rpm post script and it is not usually
       required to run it manually.

But, after the meeting yesterday, I think you are running this script
somewhere in Fedora Copr (probably from ansible playbooks), and I'm
curious whether it actually makes some sense. WDYT?

Btw., this changed after b257132 (I still
fail to read that commit, documentation would be awesome). I believe it
did not behave correctly (bug?) -> the "pre" contexts are restored in
"posttrans". I'm trying to remove this, please discuss.

Naturally, running 'restorecon' recursively on several TB of
backend/dist-git data will take some time, and I don't think it is needed.
Once copr-selinux is installed, RPM respects the policy and relabels
packaged files appropriately, and the following files created underneath
just inherit the context from parents.

The only possible problem is at the moment when there's running copr stack
without copr-selinux, and the selinux package is installed later. At that
point, I'd expect admin to run recursive restorecon manually :-/.

Which brings me to the idea that we should use 'Recommends: copr-selinux'
in copr-{backend,frontend,dist-git,key-gen}.spec. That will guarantee
(note we don't have loops in deps) that tsort will first install
copr-selinux (and enable the selinux module in %post) and then the
dependant packages.

Btw., I tried to test it with quick-package that Recommends: copr-selinux=
, and
it worked like that:

$ sudo dnf install quick-package
=2E..
Installing:
 quick-package ...
Installing weak dependencies:
 copr-selinux ...
Transaction Summary
Install  2 Packages
=2E..
Running transaction
  Preparing        :
  Installing       : copr-selinux-1.48.git.185.7f89ea59.wtree.zkuecf-1.fc28=
=2Enoarch
  Running scriptlet: copr-selinux-1.48.git.185.7f89ea59.wtree.zkuecf-1.fc28=
=2Enoarch
=46ailed to resolve roletype statement at /var/lib/selinux/mls/tmp/modules/=
400/copr/cil:2
/usr/sbin/semodule:  Failed!
  Installing       : quick-package-20180620_0943-0.fc28.x86_64
  Running scriptlet: quick-package-20180620_0943-0.fc28.x86_64
  Verifying        : quick-package-20180620_0943-0.fc28.x86_64
  Verifying        : copr-selinux-1.48.git.185.7f89ea59.wtree.zkuecf-1.fc28=
=2Enoarch

Installed:
  quick-package.x86_64 20180620_0943-0.fc28

Complete!

And really, the files from quick-package were labeled according to rules =
from
just now installed copr-selinux package.

Also note the selinux failures is something I'd love to fix in different PR:

=46ailed to resolve roletype statement at /var/lib/selinux/mls/tmp/modules/=
400/copr/cil:2
/usr/sbin/semodule:  Failed!
libsemanage.semanage_direct_remove_key: Removing last copr module (no other=
 copr module exists at another priority).
libsemanage.semanage_direct_remove_key: Unable to remove module copr at pri=
ority 400. (No such file or directory).
/usr/sbin/semodule:  Failed!

rebased onto ef8c59c86034ce30930b78c500a5995db67e8242

2 years ago

Hms, after trying this concept in:
https://copr.fedorainfracloud.org/coprs/praiskup/test-copr-selinux/package/copr-selinux/

Turns out the Recommends: copr-selinux doesn't affect the transaction that much
so the copr-selinux would be installed first :-(. Usually I'd do Requires(pre): copr-selinux, but it's not an option as long as we consider copr-selinux to be optional package.

On Tuesday, June 19, 2018 4:57:00 PM CEST Miroslav Such=FD wrote:

You need to alter copr-selinux-relabel as well.

Hm. That file isn't called in scriptlets, as otherwise claimed in
manpage:
DESCRIPTION
The copr-selinux-relabel program will call restorecon(8) on all Copr
files. This effectively restores the SELinux context.

   The program is called by an rpm post script and it is not usually
   required to run it manually.

But, after the meeting yesterday, I think you are running this script
somewhere in Fedora Copr (probably from ansible playbooks), and I'm
curious whether it actually makes some sense. WDYT?
Btw., this changed after b257132 (I still
fail to read that commit, documentation would be awesome). I believe it
did not behave correctly (bug?) -> the "pre" contexts are restored in
"posttrans". I'm trying to remove this, please discuss.
Naturally, running 'restorecon' recursively on several TB of
backend/dist-git data will take some time, and I don't think it is needed.
Once copr-selinux is installed, RPM respects the policy and relabels
packaged files appropriately, and the following files created underneath
just inherit the context from parents.

I agree that the "auto-"relabeling should be just dropped from the package itself.

The only possible problem is at the moment when there's running copr stack
without copr-selinux, and the selinux package is installed later. At that
point, I'd expect admin to run recursive restorecon manually :-/.
Which brings me to the idea that we should use 'Recommends: copr-selinux'
in copr-{backend,frontend,dist-git,key-gen}.spec. That will guarantee
(note we don't have loops in deps) that tsort will first install
copr-selinux (and enable the selinux module in %post) and then the
dependant packages.
Btw., I tried to test it with quick-package that Recommends: copr-selinux=
, and
it worked like that:
$ sudo dnf install quick-package
=2E..
Installing:
quick-package ...
Installing weak dependencies:
copr-selinux ...
Transaction Summary
Install 2 Packages
=2E..
Running transaction
Preparing :
Installing : copr-selinux-1.48.git.185.7f89ea59.wtree.zkuecf-1.fc28=
=2Enoarch
Running scriptlet: copr-selinux-1.48.git.185.7f89ea59.wtree.zkuecf-1.fc28=
=2Enoarch
=46ailed to resolve roletype statement at /var/lib/selinux/mls/tmp/modules/=
400/copr/cil:2
/usr/sbin/semodule: Failed!
Installing : quick-package-20180620_0943-0.fc28.x86_64
Running scriptlet: quick-package-20180620_0943-0.fc28.x86_64
Verifying : quick-package-20180620_0943-0.fc28.x86_64
Verifying : copr-selinux-1.48.git.185.7f89ea59.wtree.zkuecf-1.fc28=
=2Enoarch

Installed:
quick-package.x86_64 20180620_0943-0.fc28

Complete!

And really, the files from quick-package were labeled according to rules =
from
just now installed copr-selinux package.

I suggest copr-selinux is made as an non-optional package if we move everything there. If the environment doesn't support SELinux, it's installation still should not hurt.

Also note the selinux failures is something I'd love to fix in different PR:
=46ailed to resolve roletype statement at /var/lib/selinux/mls/tmp/modules/=
400/copr/cil:2
/usr/sbin/semodule: Failed!

That would be cool if we could fix it.

libsemanage.semanage_direct_remove_key: Removing last copr module (no other=
copr module exists at another priority).
libsemanage.semanage_direct_remove_key: Unable to remove module copr at pri=
ority 400. (No such file or directory).
/usr/sbin/semodule: Failed!

Hm. That file isn't called in scriptlets, as otherwise claimed in
manpage

True. It is not called in the scriptlet. And yes, man page is lying. Yet, the script exists and we should update it as well. Or delete. Thou I vote for just updating it.

I just checked fedora-infra.git and the script is not even called from playbook.

The automatic relable is done in %posttrans using /usr/sbin/fixfiles -C %{file_context_file_pre} restore which should run only on files/directories which actually got changed policy. So it should skip repos if there were no change in selinux policy.

True. It is not called in the scriptlet. And yes, man page is lying. Yet, the script exists and we should update it as well. Or delete. Thou I vote for just updating it.

I'm OK to fix that, what would be the fix? Using fixfiles, right?

The automatic relable is done in %posttrans using /usr/sbin/fixfiles -C %{file_context_file_pre} restore which should run only on files/directories which actually got changed policy. So it should skip repos if there were no change in selinux policy.

Meh, I interpreted it from the man page like "restore" the file contexts based on the PREVIOUS_FILECONTEXT. I have to check; why do we need that? Isn't actually RPM doing this automatically?

I have to check; why do we need that? Isn't actually RPM doing this automatically?

Automatic relabeling of files whose context changed? No, rpm does not do that automatically.

And why we need copr-selinux-relabel script? Because when you terminate copr machine, reprovision it to new fedora, attach volumes, then you should relabel the files on those volumes.

I have to check; why do we need that? Isn't actually RPM doing this automatically?

Automatic relabeling of files whose context changed? No, rpm does not do that automatically.
And why we need copr-selinux-relabel script? Because when you terminate copr machine, reprovision it to new fedora, attach volumes, then you should relabel the files on those volumes.

I don't think we need it. As far as I know, SELInux labels are kept after reattaching and remounting the volume so we don't need to restore them in any way. Maybe this applied for some earlier versions of nfs that SELInux labels were not correctly transferred?

If the labels get messed up for some reason (e.g. by user unintentionally), they can be always fixed by running restorecon or fixfiles manually from command-line.

From practice, I always need to kill the fixfiles process during the playbook execution when copr-selinux is being installed, so I would welcome if this wasn't a part of the installation.

Mirek is right, if we (theoretically) changed some label, we need explicit relabel (the same thing is done by selinux-policy-targeted, e.g.).

From practice, I always need to kill the fixfiles process during the playbook execution when copr-selinux is being installed, so I would welcome if this wasn't a part of the installation.

I guess the problem is only on initial package installation (not package update). I'll try to optimize this case, but please allow me some time to better analyze (e.g. I'm not convinced that %posttrans is a correct place to execute this, see https://pagure.io/packaging-committee/issue/726).

Mirek is right, if we (theoretically) changed some label, we need explicit relabel (the same thing is done by selinux-policy-targeted, e.g.).

The only two labels we are introducing are copr_data_t, copr_httpd_log_t and the chance that we change any of them is pretty much none. Even if we "theoretically" changed them, then we would fix it by a one-shot action in the package accompanied by major version bump.

I really don't see why this code is needed so I vote for removing it.

It's not only about introducing new types, it's also about labeling concrete file paths (at least we now have some semanage calls, and adding such rule really is a reason for relabel).

Otherwise I agree it's not that burning issue; but at least from the guidelines perspective, we are expected to do it. For me it is at least good thing for research because we plan to do similar things for PostgreSQL packaging (maybe SCLs).

Definitely, we need to make the process of relabeling optimal - so among others, the point is to fix this pull request so you don't have to kill fixfiles process anymore (I guess that's what you worry about).

It's not only about introducing new types

Adding new types is not a problem as well, only renaming the existing ones

it's also about labeling concrete file paths (at least we now have some semanage calls, and adding such rule really is a reason for relabel).

Not sure what you mean. Can you give an example?

For example if updated copr-selinux labeled /var/log/foo as copr_data_t. If I understand it correctly, it's not guaranteed that updated copr-selinux is installed before updated package that (newly) installs /var/log/foo.

For example if updated copr-selinux labeled /var/log/foo as copr_data_t. If I understand it correctly, it's not guaranteed that updated copr-selinux is installed before updated package that (newly) installs /var/log/foo.

Well, as I see it at the moment, copr-selinux should only define the global shared types copr_data_t and copr_httpd_t and the policy ("allow") rules for them. Everything else like concrete application of these rules to specific paths should be distributed to individual packages. Also the Requires(pre): copr-selinux should be used as suggested here before so that we make sure the needed types exists when they are needed. Feel free to tweak this as you think is correct.

rebased onto 210aaed

2 years ago

Please take another look.

rebased onto a91f89c9a8ca26d08427b50b828f5c143811df2d

2 years ago

rebased onto 555627d94c5973deafaa29867807a1cd3a84490d

2 years ago

Rebased once more (I don't want to remove the copr-selinux-relabel script, and I also squashed the commits).

rebased onto af2f4fd44525aa77f98f0e1f83c7bd211baf5645

2 years ago

rebased onto b7bb558

2 years ago

Updated once more. Doing relabel through %pre and %posttrans is really safe.

2 new commits added

  • [selinux] do the relabel in %posttrans
  • [selinux][backend][dist-git][frontend] fix SELinux
2 years ago

I'm starting to be brave enough to merge this without review before next meeting. It's not that huge risk TBH, and I'd fix remaining problems if something goes nuts.

Pull-Request has been merged by msuchy

2 years ago

@praiskup For future reference, OrderWithRequires: copr-selinux in copr-backend would ensure that it is installed before copr-backend if it's in the same transaction without adding a hard dependency on the package. Combined with the rich Recommends, it'd have the behavior you expect.

@ngompa, thanks, good to know. But Recommends should now work as expected (should reorder the transaction similarly to Requires).