[sssd PR#457][opened] ipa: Removal of umask(0) in selinux_child

[amitkumar50]

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

PR body:
"""
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
"""

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

[sssd PR#457][comment] ipa: Removal of umask(0) in selinux_child

[centos-ci]

  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

[sssd PR#457][comment] ipa: Removal of umask(0) in selinux_child

[centos-ci]

  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-346255167

[sssd PR#457][comment] ipa: Removal of umask(0) in selinux_child

[sumit-bose]

  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

[sssd PR#457][comment] ipa: Removal of umask(0) in selinux_child

[fidencio]

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

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

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

[sssd PR#457][comment] ipa: Removal of umask(0) in selinux_child

[jhrozek]

  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

[sssd PR#457][comment] ipa: Removal of umask(0) in selinux_child

[fidencio]

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

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

"""

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

[sssd PR#457][comment] ipa: Removal of umask(0) in selinux_child

[simo5]

  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

[sssd PR#457][comment] ipa: Removal of umask(0) in selinux_child

[simo5]

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

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

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

[sssd PR#457][comment] ipa: Removal of umask(0) in selinux_child

[amitkumar50]

  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

[sssd PR#457][comment] ipa: Removal of umask(0) in selinux_child

[lslebodn]

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

lslebodn commented:
"""
On (22/11/17 12:23), fidencio wrote:
> 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).
>  
Actually situation is different.

*SUSE use apparmor and SSSD is compiled without SELinux and libsemanage.
But I do not thik many debian users use SELinux + libsemanage.
BTW debian stable has libsemanage 2.6 https://packages.debian.org/stretch/libsemanage1

@bachradsusi , do you know which upstream version of libsemanage does not
requires workaround with `umask(0)`?

LS

"""

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

[sssd PR#457][comment] ipa: Removal of umask(0) in selinux_child

[bachradsusi]

  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

[sssd PR#457][comment] ipa: Removal of umask(0) in selinux_child

[lslebodn]

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

lslebodn commented:
"""
On (23/11/17 12:45), Petr Lautrbach wrote:
> 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.
>  
Do I understand it correctly that `umask(0)` is not required for
libsemanage < 2.4 and libsemanage >= 2.6 ?

+el7 has backported patch.

LS

"""

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

[sssd PR#457][comment] ipa: Removal of umask(0) in selinux_child

[amitkumar50]

  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

[sssd PR#457][comment] ipa: Removal of umask(0) in selinux_child

[simo5]

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

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

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

[sssd PR#457][comment] ipa: Removal of umask(0) in selinux_child

[bachradsusi]

  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

[sssd PR#457][comment] ipa: Removal of umask(0) in selinux_child

[bachradsusi]

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

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

[sssd PR#457][comment] ipa: Removal of umask(0) in selinux_child

[lslebodn]

  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

[sssd PR#457][comment] ipa: Removal of umask(0) in selinux_child

[bachradsusi]

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

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

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

[sssd PR#457][comment] ipa: Removal of umask(0) in selinux_child

[lslebodn]

  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

[sssd PR#457][comment] ipa: Removal of umask(0) in selinux_child

[vmojzis]

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

vmojzis commented:
"""
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
(https://github.com/fedora-selinux/selinux/pull/53).
"""

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

[sssd PR#457][comment] ipa: Removal of umask(0) in selinux_child

[sumit-bose]

  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

[sssd PR#457][comment] ipa: Removal of umask(0) in selinux_child

[jhrozek]

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

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

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

[sssd PR#457][comment] ipa: Removal of umask(0) in selinux_child

[pbrezina]

  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

[sssd PR#457][+superseded] ipa: Removal of umask(0) in selinux_child

[alexey-tikhonov]

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

Label: +superseded

[sssd PR#457][comment] ipa: Removal of umask(0) in selinux_child

[alexey-tikhonov]

  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

[sssd PR#457][comment] ipa: Removal of umask(0) in selinux_child

[alexey-tikhonov]

  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 #920
"""

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

[sssd PR#457][closed] ipa: Removal of umask(0) in selinux_child

[pbrezina]

   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