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.
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?
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().
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.
Simo Sorce * Red Hat, Inc * New York