Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ipa: Removal of umask(0) in selinux_child #457

Open
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
9 participants
@amitkumar50
Copy link
Contributor

commented Nov 22, 2017

Code for calling umask(0) was added to address bugzilla
https://bugzilla.redhat.com/show_bug.cgi?id=1186422.
Now this bugzilla is fixed, removing the workaround.

Resolves: https://pagure.io/SSSD/sssd/issue/3583

@centos-ci

This comment has been minimized.

Copy link
Collaborator

commented Nov 22, 2017

Can one of the admins verify this patch?

1 similar comment
@centos-ci

This comment has been minimized.

Copy link
Collaborator

commented Nov 22, 2017

Can one of the admins verify this patch?

@sumit-bose

This comment has been minimized.

Copy link
Contributor

commented Nov 22, 2017

ok to test

@fidencio

This comment has been minimized.

Copy link
Contributor

commented Nov 22, 2017

@amitkumar50, @jhrozek: Do you know in which Fedora/RHEL versions the fix on libsemanage are included?

Seems it's fixed on RHEL-7.3+ ... So, I guess it's safe to have it merged.

ipa: Removal of umask(0) in selinux_child
Code for calling umask(0) was added to address bugzilla
https://bugzilla.redhat.com/show_bug.cgi?id=1186422.
Now this bugzilla is fixed, removing the workaround.

Resolves: https://pagure.io/SSSD/sssd/issue/3583
@jhrozek

This comment has been minimized.

Copy link
Contributor

commented Nov 22, 2017

What about RHEL-6? We still support that release..

@amitkumar50 I really appreciate your help, but in this case, I wonder if it was better to ask on sssd-devel or #sssd what the good approach is, because I'm not sure we can merge this patch unless all supported distributions carry the fixed selinux policy version..

@fidencio

This comment has been minimized.

Copy link
Contributor

commented Nov 22, 2017

We're fine about RHEL-6 as long as we don't explicitly backport it there.

I'm a little bit worried about openSUSE/SLES as they may use SELinux. Debian, AFAIR, doesn't use SELinux (but I'd like to have Timo's confirmation here).

@simo5

This comment has been minimized.

Copy link
Contributor

commented Nov 22, 2017

I see nothing in the commit message that explains why this is needed.
for a thing that changes a core security control I would really like a long and detailed explanation!

@simo5

This comment has been minimized.

Copy link
Contributor

commented Nov 22, 2017

Also if it works only with a specific version of a dependency we need a depndency check/enforcement.

@amitkumar50

This comment has been minimized.

Copy link
Contributor Author

commented Nov 23, 2017

I believe this change would go into rhel-7.1 and above since mentioned bugzilla is fixed there.
But yes this code should not go to rhel-6/Debian etc where bugzilla is not fixed.

But what we do for these workaround-codes when ever We have an working-permanent fix?

@lslebodn

This comment has been minimized.

Copy link
Contributor

commented Nov 23, 2017

@bachradsusi

This comment has been minimized.

Copy link

commented Nov 23, 2017

I guess it's related to the change of SElinux module store location in /var/lib/selinux which happened in Release 2015-02-02 aka libsemanage-2.4.

@lslebodn

This comment has been minimized.

Copy link
Contributor

commented Nov 24, 2017

@amitkumar50

This comment has been minimized.

Copy link
Contributor Author

commented Nov 27, 2017

if libsemanage-2.6 does not need umask(0), if it is so. are we good to go on PR, or any additional changes are required.

@simo5

This comment has been minimized.

Copy link
Contributor

commented Nov 27, 2017

You need to detect we have the correct libsemanage and ifdef those out. In a few years (when RHEL6 goes away) we can remove the code entirely.
Also add minimum required libsemanage version in sssd spec file in contrib if not there yet.

@bachradsusi

This comment has been minimized.

Copy link

commented Jan 15, 2018

I need to do more tests but it looks like the issue reported in the https://bugzilla.redhat.com/show_bug.cgi?id=1186422 is not fixed yet. The test used for the verification checks probably a wrong path. I'll update this PR when I have more information.

@lslebodn

This comment has been minimized.

Copy link
Contributor

commented Jan 15, 2018

I need to do more tests but it looks like the issue reported in the https://bugzilla.redhat.com/show_bug.cgi?id=1186422 was actually fixed. The test used for the verification checks probably a wrong path. I'll update this PR when I have more information.

I tested sssd-master + this patch on CentOS6 (6.9) CentOS7 (7.4) and I could not see any problem.
I ran full IPA related tests + test-case from https://bugzilla.redhat.com/show_bug.cgi?id=1184982#c3

Other distros:

  • debian >= stable have libsemanage >= 2.6
  • debian oldstable has libsemanage-2.3
  • ubuntu trusty (12.04) : libsemanage-2.2
  • ubuntu xenial (16.04) : libsemanage-2.3
  • ubuntu >= 17.04 (zesty, artful, bionic) have libsemanage >= 2.6
  • opensuse does not build sssd with libsemanage.

I think we are safe on all fronts and PR can be merged without any buildtime/runtime checks/workarounds.

@bachradsusi, Do you agree?

@bachradsusi

This comment has been minimized.

Copy link

commented Jan 15, 2018

The problem with the test case is that it uses old path. /etc/selinux/targeted/modules/active changed to /etc/selinux/targeted/active/modules/

[root@centos7 targeted]# rpm -q libsemanage
libsemanage-2.5-8.el7.x86_64
[root@centos7 targeted]# ls -ld /etc/selinux/targeted/active/ /etc/selinux/targeted/active/modules/       
drwx------. 3 root root 206 Jan 15 17:24 /etc/selinux/targeted/active/                                    
drwx------. 4 root root  33 Jan 15 17:24 /etc/selinux/targeted/active/modules/                            
[root@centos7 targeted]# umask 777                   
[root@centos7 targeted]# semodule -B                 
[root@centos7 targeted]# ls -ld /etc/selinux/targeted/active/ /etc/selinux/targeted/active/modules/
d---------. 3 root root 206 Jan 15 17:27 /etc/selinux/targeted/active/
d---------. 4 root root  33 Jan 15 17:27 /etc/selinux/targeted/active/modules/
@lslebodn

This comment has been minimized.

Copy link
Contributor

commented Jan 16, 2018

@vmojzis

This comment has been minimized.

Copy link

commented Sep 12, 2018

The issue is fixed in libsemanage-2.5-13.el7. The fix it is upstream and there is a pull-request waiting for inclusion in Fedora (fedora-selinux/selinux#53).

@sumit-bose

This comment has been minimized.

Copy link
Contributor

commented May 22, 2019

Hi,

I'd like to revive this PR with a different view on the topic.

I tend to prefer to keep the umask() calls, not as a workaround, but as a precaution. Since we expect the libsemange calls to create files or directories and change the umask of the process saving and resetting 'our' umask would protect us from potential issue in the future where libsemange sets the umask() but for whatever reasons it is not set back to its original value.

If we can agree on this view I wonder if @amitkumar50 would like to update that patch so that only the comment is replaced with a new one explaining why umask() is used?

bye,
Sumit

@jhrozek

This comment has been minimized.

Copy link
Contributor

commented May 23, 2019

I agree all the configure checks and the other complexity is not worth the work and the umask can stay in the end.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.