[SSSD] [PATCH] possible alternative for #2071

Jakub Hrozek jhrozek at redhat.com
Mon Oct 7 09:00:35 UTC 2013


On Fri, Oct 04, 2013 at 02:27:40PM +0200, Pavel Březina wrote:
> On 10/04/2013 02:16 PM, Jakub Hrozek wrote:
> >On Fri, Oct 04, 2013 at 01:16:02PM +0200, Pavel Březina wrote:
> >>On 10/04/2013 09:48 AM, Jakub Hrozek wrote:
> >>>On Tue, Oct 01, 2013 at 10:10:57PM +0200, Jakub Hrozek wrote:
> >>>>On Tue, Oct 01, 2013 at 09:50:07PM +0200, Jakub Hrozek wrote:
> >>>>>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) {
> >>>>
> >>>>Sorry for talking to myself, but I have to nack the patch after all, the
> >>>>unit tests are not passing.
> >>>
> >>>I'd like to apply the attached patch on top of yours.
> >>
> >>Hi,
> >>ack to the unit tests. Nack to the Simo's patch - it contains unused
> >>variable. Can you squash attached patch in?
> >>
> >>
> >>
> >
> >Yes, I mentioned the unused variable in a previous e-mail in the long
> >thread where I've been talking to myself :-)
> 
> Ah, sorry. I actually read your monologue, but not today so I
> forgotten that you already have a patch :-)
> 
>  So I agree with removing
> >the variable..

Pushed Simo's patch amended to suppress the compilation warning and the
patch to fix unit tests to master and sssd-1-11



More information about the sssd-devel mailing list