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

[Martin Babinsky]

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.

-- 
Martin^3 Babinsky
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-mbabinsk-0015-8-ipautil-new-functions-kinit_keytab-and-kinit_passwor.patch
Type: text/x-patch
Size: 4613 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20150415/09fe5341/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-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/20150415/09fe5341/attachment-0001.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-mbabinsk-0017-6-Adopted-kinit_keytab-and-kinit_password-for-kerberos.patch
Type: text/x-patch
Size: 12309 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20150415/09fe5341/attachment-0002.bin>