URL: https://github.com/SSSD/sssd/pull/457 Title: #457: ipa: Removal of umask(0) in selinux_child

centos-ci commented: """ Can one of the admins verify this patch? """

See the full comment at https://github.com/SSSD/sssd/pull/457#issuecomment-346255166

URL: https://github.com/SSSD/sssd/pull/457 Title: #457: ipa: Removal of umask(0) in selinux_child

sumit-bose commented: """ ok to test """

See the full comment at https://github.com/SSSD/sssd/pull/457#issuecomment-346262982

URL: https://github.com/SSSD/sssd/pull/457 Title: #457: ipa: Removal of umask(0) in selinux_child

jhrozek commented: """ 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.. """

See the full comment at https://github.com/SSSD/sssd/pull/457#issuecomment-346334073

URL: https://github.com/SSSD/sssd/pull/457 Title: #457: ipa: Removal of umask(0) in selinux_child

simo5 commented: """ 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! """

See the full comment at https://github.com/SSSD/sssd/pull/457#issuecomment-346438652

URL: https://github.com/SSSD/sssd/pull/457 Title: #457: ipa: Removal of umask(0) in selinux_child

amitkumar50 commented: """ 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? """

See the full comment at https://github.com/SSSD/sssd/pull/457#issuecomment-346591909

URL: https://github.com/SSSD/sssd/pull/457 Title: #457: ipa: Removal of umask(0) in selinux_child

bachradsusi commented: """ 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. """

See the full comment at https://github.com/SSSD/sssd/pull/457#issuecomment-346609381

URL: https://github.com/SSSD/sssd/pull/457 Title: #457: ipa: Removal of umask(0) in selinux_child

amitkumar50 commented: """ 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. """

See the full comment at https://github.com/SSSD/sssd/pull/457#issuecomment-347078142

URL: https://github.com/SSSD/sssd/pull/457 Title: #457: ipa: Removal of umask(0) in selinux_child

bachradsusi commented: """ 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. """

See the full comment at https://github.com/SSSD/sssd/pull/457#issuecomment-357676434

URL: https://github.com/SSSD/sssd/pull/457 Title: #457: ipa: Removal of umask(0) in selinux_child

lslebodn commented: """

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? """

See the full comment at https://github.com/SSSD/sssd/pull/457#issuecomment-357733669

URL: https://github.com/SSSD/sssd/pull/457 Title: #457: ipa: Removal of umask(0) in selinux_child

lslebodn commented: """ On (15/01/18 09:28), Petr Lautrbach wrote:

[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/

I thought that upstream libsemanage-2.6 is safe but it isn't

``` [root@f26 ~]# ls -ld /var/lib/selinux/targeted/active/ /var/lib/selinux/targeted/active/modules/ drwx------. 3 root root 228 Jan 16 07:17 /var/lib/selinux/targeted/active/ drwx------. 6 root root 55 Jan 16 07:17 /var/lib/selinux/targeted/active/modules/ [root@f26 ~]# umask 777 [root@f26 ~]# semodule -B [root@f26 ~]# ls -ld /var/lib/selinux/targeted/active/ /var/lib/selinux/targeted/active/modules/ d---------. 3 root root 228 Jan 16 07:18 /var/lib/selinux/targeted/active/ d---------. 6 root root 55 Jan 16 07:18 /var/lib/selinux/targeted/active/modules/ [root@f26 ~]# rpm -q libsemanage libsemanage-2.6-4.fc26.x86_64 ```

and the same behaviour is also in rawhide ``` [root@rawhide ~]# rpm -q libsemanage libsemanage-2.7-5.fc28.x86_64 [root@rawhide ~]# ls -ld /var/lib/selinux/targeted/active/ drwx------. 3 root root 4096 Jan 15 12:18 /var/lib/selinux/targeted/active/ [root@rawhide ~]# ls -ld /var/lib/selinux/targeted/active/ /var/lib/selinux/targeted/active/modules/ drwx------. 3 root root 4096 Jan 15 12:18 /var/lib/selinux/targeted/active/ drwx------. 5 root root 4096 Jan 15 12:18 /var/lib/selinux/targeted/active/modules/ [root@ibm-x3650m4-01-vm-06 ~]# umask 777 [root@ibm-x3650m4-01-vm-06 ~]# semodule -B [root@ibm-x3650m4-01-vm-06 ~]# ls -ld /var/lib/selinux/targeted/active/ /var/lib/selinux/targeted/active/modules/ d---------. 3 root root 4096 Jan 16 07:24 /var/lib/selinux/targeted/active/ d---------. 5 root root 4096 Jan 16 07:24 /var/lib/selinux/targeted/active/modules/ ```

@bachradsusi, so we should not merge that without fixed version of libsemanage (ideal upstream) and then we can do some build time checks.

LS

"""

See the full comment at https://github.com/SSSD/sssd/pull/457#issuecomment-357945879

URL: https://github.com/SSSD/sssd/pull/457 Title: #457: ipa: Removal of umask(0) in selinux_child

sumit-bose commented: """ 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 """

See the full comment at https://github.com/SSSD/sssd/pull/457#issuecomment-494749209

URL: https://github.com/SSSD/sssd/pull/457 Title: #457: ipa: Removal of umask(0) in selinux_child

pbrezina commented: """ @amitkumar50 Would you mind just changing the comment so we can merge this patch? """

See the full comment at https://github.com/SSSD/sssd/pull/457#issuecomment-522981230

URL: https://github.com/SSSD/sssd/pull/457 Title: #457: ipa: Removal of umask(0) in selinux_child

alexey-tikhonov commented: """ This PR is superseded in favor of PR920 """

See the full comment at https://github.com/SSSD/sssd/pull/457#issuecomment-547874086

URL: https://github.com/SSSD/sssd/pull/457 Author: amitkumar50 Title: #457: ipa: Removal of umask(0) in selinux_child Action: closed

To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/457/head:pr457 git checkout pr457