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