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

[Martin Babinsky]

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.

-- 
Martin^3 Babinsky
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-4-1-mbabinsk-0015-8-ipautil-new-functions-kinit_keytab-and-kinit_passwor.patch
Type: text/x-patch
Size: 4612 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20150420/c2a5eb8d/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-4-1-mbabinsk-0016-7-ipa-client-install-try-to-get-host-TGT-several-times.patch
Type: text/x-patch
Size: 9218 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20150420/c2a5eb8d/attachment-0001.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-4-1-mbabinsk-0017-6-Adopted-kinit_keytab-and-kinit_password-for-kerberos.patch
Type: text/x-patch
Size: 12113 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20150420/c2a5eb8d/attachment-0002.bin>