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

[Martin Babinsky]

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.

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