[Freeipa-devel] [PATCHES 0015-0017] consolidation of various Kerberos auth methods in FreeIPA code

[Jan Cholasta]

Dne 20.4.2015 v 10:06 Martin Babinsky napsal(a):
> On 04/20/2015 09:48 AM, Jan Cholasta wrote:
>> Dne 15.4.2015 v 15:17 Martin Babinsky napsal(a):
>>> On 04/13/2015 02:16 PM, Martin Babinsky wrote:
>>>> On 04/09/2015 03:38 PM, Jan Cholasta wrote:
>>>>
>>>>>
>>>>> Some comments:
>>>>>
>>>>> Patch 15:
>>>>>
>>>>> 1) The functions should be as similar as possible:
>>>>>
>>>>>      a) kinit_password() should have a 'ccache_path' argument
>>>>> instead of
>>>>> passing the path in KRB5CCNAME in the 'env' argument.
>>>>>
>>>>>      b) I don't think kinit_password() should have the 'env'
>>>>> argument at
>>>>> all. You can always call kinit with LC_ALL=C and set other
>>>>> variables in
>>>>> os.environ if you want.
>>>>>
>>>>>      c) The arguments should have the same ordering.
>>>>>
>>>>>      d) Either set KRB5CCNAME in both kinit_keytab() and
>>>>> kinit_password() or in none of them.
>>>>>
>>>>>      e) Either rename armor_ccache to armor_ccache_path or ccache_path
>>>>> to ccache.
>>>>>
>>>> I have done some reordering of parameters in both functions so they are
>>>> very similar now and the parameter ordering should make more sense (at
>>>> least to me).
>>>>
>>>> Neither of them sets KRB5CCNAME env. variable since I think that it is
>>>> not a very good practice and the developer should be responsible for
>>>> pointing to correct CCache path. Jan agrees with this and the other
>>>> patches are updated accordingly.
>>>>>
>>>>> 2) Space before comma in docstring:
>>>>>
>>>>> +    Given a ccache_path , keytab file and a principal kinit as that
>>>>> user.
>>>>>
>>>>>
>>>>> 3) I would prefer if the default value of 'armor_ccache' in
>>>>> kinit_password() was None.
>>>>>
>>>> Fixed.
>>>>>
>>>>> Patch 16:
>>>>>
>>>>> 1) The callback should not be named 'validate_kinit_attempts_option',
>>>>> but rather 'kinit_attempts_callback', as it doesn't just validate the
>>>>> value.
>>>>>
>>>> Fixed.
>>>>>
>>>>> 2) Why is there the sys.maxint upper bound on --kinit-attempts
>>>>> again? A
>>>>> comment with explanation would be nice.
>>>>>
>>>> It actually doesn't make much sense to have such upper bound, so I have
>>>> removed it from the check and updated the error message accordingly.
>>>>>
>>>>> Patch 17:
>>>>>
>>>>> 1) Is there a reason for the ccache filename changes in DNSSEC code?
>>>>>
>>>> That was Petr Spacek's request since a sane naming of persistent
>>>> Ccaches
>>>> makes debugging of Kerberos-related errors a bit easier for him.
>>>>
>>>> Attaching updated patches.
>>>>
>>>>
>>>>
>>>
>>> Jan had some further suggestions so I am attaching updated patches which
>>> should reflect them.
>>>
>>> He also recommended to split the naming changes of DNSSEC daemon
>>> credential caches to a separate patch, so I will submit them later when
>>> this patchset is pushed.
>>>
>>
>> ACK. The patches need to be rebased on top of ipa-4-1 though.
>>
>
> Right, attaching rebased patches.
>

Thanks.

Pushed to:
master: 3d2feac0e416c66ba37eee53ef5b3833c2c3e414
ipa-4-1: 0ca8254959f3566c322eb7b20c6d6522814d78d1

-- 
Jan Cholasta