On Tue, Oct 01, 2013 at 09:45:37PM +0200, Jakub Hrozek wrote:
> On Thu, Sep 26, 2013 at 08:25:12PM +0200, Jakub Hrozek wrote:
> > On Wed, Sep 25, 2013 at 05:50:16PM -0400, Simo Sorce wrote:
> > > On Wed, 2013-09-25 at 17:37 -0400, Simo Sorce wrote:
> > > > On Tue, 2013-09-24 at 12:52 +0200, Jakub Hrozek wrote:
> > > > > 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:
> > > >
> > > > It doesn't apply because, it depends on the patch from the other
> > > > patchset that you did not apply ("krb5: Improve ccache name checks")
> > >
> > > Ok fixed the issues and rebase before the other patch, should apply
> > > cleanly on master now.
> > >
> > > Simo.
> > >
> > > --
> > > Simo Sorce * Red Hat, Inc * New York
> >
> > There still must be some patch missing in between, I can't compile this
> > version:
> >
> > CC src/providers/krb5/krb5_init_shared.lo
> > /home/remote/jhrozek/devel/sssd/src/providers/krb5/krb5_auth.c:295:17:
> > warning: unused variable 'realm' [-Wunused-variable]
> > const char *realm;
> > ^
> > 2 warnings generated.
> > /home/remote/jhrozek/devel/sssd/src/tests/krb5_child-test.c:249:57: error:
> > too many arguments to function call, expected 5, have 6
> > true, true, &private);
> > ^~~~~~~~
> > /home/remote/jhrozek/devel/sssd/src/providers/krb5/krb5_utils.h:48:1: note:
> > 'expand_ccname_template' declared here
> > char *expand_ccname_template(TALLOC_CTX *mem_ctx, struct krb5child_req *kr,
> > ^
> > /home/remote/jhrozek/devel/sssd/src/tests/krb5_child-test.c:265:55: error:
> > too many arguments to function call, expected 4, have 5
> > kr->uid, kr->gid, private);
> > ^~~~~~~
>
> Ugh, I must have misapplied the patch. Now everything seems to have
> applied and built correctly. My initial testing also passed.
>
> ACK
I will just squash in the following change before pushing:
diff --git a/src/providers/krb5/krb5_auth.c b/src/providers/krb5/krb5_auth.c
index a16b539..a4183dc 100644
--- a/src/providers/krb5/krb5_auth.c
+++ b/src/providers/krb5/krb5_auth.c
@@ -292,7 +292,6 @@ static errno_t krb5_auth_prepare_ccache_name(struct
krb5child_req *kr,
struct be_ctx *be_ctx)
{
const char *ccname_template;
- const char *realm;
errno_t ret;
if (!kr->is_offline) {
_______________________________________________
sssd-devel mailing list
[email protected]
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
