On Fri, Oct 17, 2014 at 01:29:42PM -0400, Simo Sorce wrote:
On Fri, 17 Oct 2014 18:47:28 +0200
Jakub Hrozek <jhrozek(a)redhat.com> wrote:
> On Thu, Oct 16, 2014 at 10:25:12AM +0200, Jakub Hrozek wrote:
> > On Wed, Oct 15, 2014 at 06:19:49PM -0400, Simo Sorce wrote:
> > > On Wed, 15 Oct 2014 22:24:04 +0200
> > > Jakub Hrozek <jhrozek(a)redhat.com> wrote:
> > >
> > > > Attached are patches that perform changes in the monitor
> > > > process and the low-level sbus and sysdb code required to run
> > > > the NSS responder as a non-privileged user. Some of the patches
> > > > call chmod/chown on files owned by the SSSD, so I'd like to
> > > > request a very careful review.
> > >
> > > Aside from the points raised in the emails already sent the rest
> > > looks good to me.
> >
> > Thank you very much for the review. I'll send out updated patches.
>
> Attached are patches that implement the corrections Simo and Pavel
> asked for with the exception of confdb being read-only for sssd
> processes, Michal is still investigating that one.
>
> I also merged some more Michal's patches that help spawn the PAM
> privileged pipe before dropping privileges and also dropping
> privileges in the other responders.
>
> With this patchset, all responders are able to run as the SSSD user.
Great!
Well, I lied. InfoPipe still runs as root, but that effort is tracked
separately with:
https://fedorahosted.org/sssd/ticket/2395
> I have one question:
> is it wise to also set the permissions of directories we create when
> we "install -d" them? Or is this something typically done by the
> downstream? Previously, we would just rely on the default system umask
> when running "make install", maybe we should tighten the permissions
> in Makefile.am as part of this effort?
+1
I filed:
https://fedorahosted.org/sssd/ticket/2467
> And one comment:
> One of Michal's patches moves creating the socket into a separate
> function. This function also changes the umask, which I don't think
> belongs into a utility function. I realize this is how the code worked
> even before Michal's change, but I think it would be better to move
> the umask setting into function like sss_process_init().
+1
And here:
https://fedorahosted.org/sssd/ticket/2468
Because both issues were already present in the code, just discovered
when changing the SSSD to run unprivileged.
> Thanks again for the review.
FYI the commit message for patch 2 still mentions the group option you
just removed :-)
Same for patch 4
The rest looks good.
I found some other minor issues when I actually made the leap myself and
started running rootless SSSD on my laptop. I will fix those along with
your and Pavel's comments and re-send.