[SSSD] [PATCH] possible alternative for #2071

Pavel Březina pbrezina at redhat.com
Fri Oct 4 11:16:02 UTC 2013


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?



-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-remove-unused-variable-warning.patch
Type: text/x-patch
Size: 825 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20131004/9870eb42/attachment.bin>


More information about the sssd-devel mailing list