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

[Martin Babinsky]

On 03/16/2015 01:35 PM, Jan Cholasta wrote:
> Dne 16.3.2015 v 13:30 Martin Babinsky napsal(a):
>> On 03/16/2015 12:15 PM, Martin Kosek wrote:
>>> On 03/13/2015 05:37 PM, Martin Babinsky wrote:
>>>> Attaching the next iteration of patches.
>>>>
>>>> I have tried my best to reword the ipa-client-install man page bit
>>>> about the
>>>> new option. Any suggestions to further improve it are welcome.
>>>>
>>>> I have also slightly modified the 'kinit_keytab' function so that in
>>>> Kerberos
>>>> errors are reported for each attempt and the text of the last error
>>>> is retained
>>>> when finally raising exception.
>>>
>>> The approach looks very good. I think that my only concern with this
>>> patch is
>>> this part:
>>>
>>> +            ccache.init_creds_keytab(keytab=ktab, principal=princ)
>>> ...
>>> +        except krbV.Krb5Error as e:
>>> +            last_exc = str(e)
>>> +            root_logger.debug("Attempt %d/%d: failed: %s"
>>> +                              % (attempt, attempts, last_exc))
>>> +            time.sleep(1)
>>> +
>>> +    root_logger.debug("Maximum number of attempts (%d) reached"
>>> +                      % attempts)
>>> +    raise StandardError("Error initializing principal %s: %s"
>>> +                        % (principal, last_exc))
>>>
>>> The problem here is that this function will raise the super-generic
>>> StandardError instead of the proper with all the context and
>>> information about
>>> the error that the caller can then process.
>>>
>>> I think that
>>>
>>>      except krbV.Krb5Error as e:
>>>          if attempt == max_attempts:
>>>              log something
>>>              raise
>>>
>>> would be better.
>>>
>>
>> Yes that seems reasonable. I'm just thinking whether we should re-raise
>> Krb5Error or raise ipalib.errors.KerberosError? the latter options makes
>> more sense to me as we would not have to additionally import Krb5Error
>> everywhere and it would also make the resulting errors more consistent.
>>
>> I am thinking about someting like this:
>>
>>      except krbV.Krb5Error as e:
>>         if attempt == attempts:
>>             # log that we have reaches maximum number of attempts
>>             raise KerberosError(minor=str(e))
>>
>> What do you think?
>>
>
> NACK, don't use ipalib from ipapython in new code, we are trying to get
> rid of this circular dependency. Krb5Error is OK in this case.
>

Ok attaching updated patches.

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