[SSSD] [PATCH] possible alternative for #2071

Jakub Hrozek jhrozek at redhat.com
Tue Sep 24 10:52:16 UTC 2013


On Fri, Sep 20, 2013 at 01:09:34PM -0400, Simo Sorce wrote:
> On Mon, 2013-09-09 at 11:22 -0400, Simo Sorce wrote:
> > On Mon, 2013-09-09 at 13:41 +0200, Jakub Hrozek wrote:
> > > On Mon, Sep 09, 2013 at 01:25:30PM +0200, Sumit Bose wrote:
> > > > On Tue, Sep 03, 2013 at 10:52:14PM -0400, Simo Sorce wrote:
> > > > > Attached an untested possible alternative for #2071
> > > > > 
> > > > > The other option is the last patch of my previous patchset in the thread
> > > > > named: "[SSSD] [PATCHES] Simplify credential krb5 cache manipulation".
> > > > > 
> > > > > Let's discuss if any of these  approaches is ok, or if a third way needs
> > > > > to be found.
> > > > > 
> > > > > Simo.
> > > > > 
> > > > > -- 
> > > > > Simo Sorce * Red Hat, Inc * New York
> > > > 
> > > > (a short summary of the two patches in discussion)
> > > > The difference between the two approaches is that the patch in this
> > > > tread just sets the permission on all newly created directories to
> > > > "new_dir_mode = 0700" making them private by default. The patch in the
> > > > other thread tries to improve the current scheme where SSSD tries to
> > > > figure out which directory which must be created is a public or a
> > > > private one and sets the mode accordingly.
> > > > 
> > > > I agree with 'Setting up public directories is the job of the admin' and
> > > > like the fact that the patch is making the SSSD code much simpler. But I
> > > > assume that there are SSSD setups in use which depend on the feature of
> > > > creating the directories with the "right" permissions. I can imagine
> > > > automatic installations e.g. with kickstart which does not create the
> > > > needed directories but rely on SSSD to create them or installations
> > > > where the credential caches are stored into some RAM based file system
> > > > where directories must be recreated on every restart.  I do not want to
> > > > discuss the correctness of those approaches or if the paths used to
> > > > store the credential caches are chosen wisely my point is just that a
> > > > change might break existing installations.
> > > > 
> > > > For this reason I would prefer to just improve the existing behaviour.
> > > > But if a majority says that it is time for a change I will not argue
> > > > against it.
> > > > 
> > > > bye,
> > > > Sumit
> > > 
> > > That's a good point.
> > > 
> > > Could we get away with keeping the existing behaviour and printing a log
> > > message whenever we create a public directory saying that this feature
> > > might go away in a later release?
> > 
> > I do not really think it is a good point.
> > A) I really do not think there is any user fo this feature, the only 2
> > places where people put stuff today are /tmp (FILE or DIR) and /run/user
> > (DIR).
> > 
> > /tmp is already set up correctly (if not many other things will break
> > and SSSD should *not* mask the mess by creating the dir and further
> > confusing the admin.
> > /run/user is a private directory so making it public would be wrong.
> > 
> > I think a change of behavior is in the best interest of our community
> > even if it ends up breaking 1 rare install that happened to know of this
> > new behavior and intentionally used it.
> 
> Bump and attached find the 2 patches rebased both on top of my latest
> patchset sent to the list earlier.
> 
> 0004 tries to maintain behavior, just saner.
> 0001 is the option to remove it completely.

As agreed yesterday on a call, we're going to remove the option
completely. The patch no longer applies cleanly, not even with a 3-way
merge. I had to apply it with patch(1), so can you please re-send? One
minor comment inline:

> -static errno_t check_parent_stat(bool private_path, struct stat *parent_stat,
> +static errno_t check_parent_stat(struct stat *parent_stat,
>                                   uid_t uid, gid_t gid)
>  {
> -    if (private_path) {
> -        if (!((parent_stat->st_uid == 0 && parent_stat->st_gid == 0) ||
> -               parent_stat->st_uid == uid)) {
> -            DEBUG(1, ("Private directory can only be created below a "
> -                      "directory belonging to root or to "
> -                      "[%"SPRIuid"][%"SPRIgid"].\n", uid, gid));
> -            return EINVAL;
> -        }
> +    if (!((parent_stat->st_uid == 0 && parent_stat->st_gid == 0) ||
> +           parent_stat->st_uid == uid)) {
> +        DEBUG(1, ("Private directory can only be created below a "
> +                  "directory belonging to root or to ",
                                                       ^^^^^
                                                you need to drop this
                                                trailing comma, it's
                                                causing a complilation
                                                warning.

                            Also please change the debug level to SSSDBG
> +                  "[%"SPRIuid"][%"SPRIgid"].\n", uid, gid));
> +        return EINVAL;
> +    }

Other than that, I think it's OK. Both online and offline auth with
dircache work fine for me.



More information about the sssd-devel mailing list