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, 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!

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.

Blind attempt: https://bugzilla.redhat.com/show_bug.cgi?id=1593185

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.

Please take another look.

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

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

test-builds are still in https://copr.fedorainfracloud.org/coprs/praiskup/test-copr-selinux/

Ping for review.

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.

+1

I have rebased and pushed the change proposed in comment-54559.

https://infrastructure.fedoraproject.org/cgit/ansible.git/commit/?id=ada5904d81746a318607f6bcf0de7708378f4071

@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).