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..