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.