Re: [SSSD] [PATCH] Move settig the SELinux context into a privileged child process

[Jakub Hrozek]

On Fri, Oct 31, 2014 at 10:48:09PM +0100, Michal Židek wrote:
> On 10/31/2014 05:59 PM, Jakub Hrozek wrote:
> > On Fri, Oct 24, 2014 at 10:57:02PM +0200, Jakub Hrozek wrote:
> > > > Hi,
> > > >
> > > > this patchset moves setting the SELinux context from the sssd_be process
> > > > to a privileges child process in preparation for making the sssd_be process
> > > > running as the sssd user.
> > > >
> > > > The first two patches just reduce code duplication between the child
> > > > processes we already have, the last one implements setting the context.
> > > >
> > > > Please note that the Fedora and RHEL SELinux policy must be tweaked, so
> > > > currently the patches must be tested with SELinux set to Permissive.
> > > >
> > > > Additionally, I wasn't able to run Coverity scan on these patches, because
> > > > our Coverity server was returning errors again for me. I'll run the tests
> > > > later, or, if the reviewer would find any issues, I'll be happy to fix them.
> >
> > Hi,
> >
> > attached patches change the packaging a bit -- make distcheck didn't
> > pass previously and I forgot to package selinux_child in the RPM.
> >
> > 0001-UTIL-Remove-code-duplication-of-struct-io.patch
>
> Ack.
> > 0002-UTIL-Remove-more-code-duplication-setting-up-child-p.patch
>
> Ack.
> > 0003-IPA-Move-setting-the-SELinux-context-to-a-child-proc.patch
>
> The chgrp and chmod in Makefile.am should probably be called
> only if the semanage is available:
> --- a/Makefile.am
> +++ b/Makefile.am
> @@ -2852,10 +2852,12 @@ endif
>
> if SSSD_USER
>    chgrp $(SSSD_USER) $(sssdlibexecdir)/ldap_child
>   chgrp $(SSSD_USER) $(sssdlibexecdir)/selinux_child
>   chmod 4750 $(sssdlibexecdir)/ldap_child
>
> +if BUILD_SEMANAGE
>   chgrp $(SSSD_USER) $(sssdlibexecdir)/selinux_child
>   chmod 4750 $(sssdlibexecdir)/selinux_child
>
> endif
> +endif

Fixed
> In src/providers/ipa/ipa_selinux.c, the line 405 is too long:
>  405     /* Update the SELinux context ...
>  406      * unprivileged
>  407      */

Fixed
> Also in src/providers/ipa/ipa_selinux.c, the indentation
> on line 872 should be aligned with the line above:
>  871     if (sci->seuser == NULL || sci->mls_range == NULL
>  872             || sci->username == NULL) {
>
> Should be:
>  871     if (sci->seuser == NULL || sci->mls_range == NULL
>  872         || sci->username == NULL) {
> (Or at least I am doing it like this, and it is style that
> was proposed IIRC by Stephen in some old discussion).

Fixed. I thought the leading tabs were also proposed in that thread..
> Otherwise the patch looks good to me and the selinux
> context is retrieved properly.
>
> Michal

Thank you for the review, new patches are attached.