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

[Jan Cholasta]

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.

-- 
Jan Cholasta