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 :-) So I agree with removing
the variable..
_______________________________________________
sssd-devel mailing list
[email protected]
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
