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

[Martin Babinsky]

On 03/23/2015 03:13 PM, Simo Sorce wrote:
> On Mon, 2015-03-23 at 14:22 +0100, Petr Spacek wrote:
>> On 23.3.2015 14:08, Simo Sorce wrote:
>>> On Mon, 2015-03-23 at 12:48 +0100, Martin Babinsky wrote:
>>>> On 03/17/2015 06:00 PM, Simo Sorce wrote:
>>>>> On Mon, 2015-03-16 at 13:30 +0100, Martin Babinsky wrote:
>>>>>> 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?
>>>>>
>>>>> Are you retrying on any error ?
>>>>> Please do *not* do that, if you retry many times on an error that
>>>>> indicates the password is wrong you may end up locking an administrative
>>>>> account. If you want to retry you should do it only for very specific
>>>>> timeout errors.
>>>>>
>>>>> Simo.
>>>>>
>>>>>
>>>> I have taken a look at the logs attached to the original BZ
>>>> (https://bugzilla.redhat.com/show_bug.cgi?id=1161722).
>>>>
>>>> In ipaclient-install.log the kinit error is:
>>>>
>>>> "Cannot contact any KDC for realm 'ITW.USPTO.GOV' while getting initial
>>>> credentials"
>>>>
>>>> which can be translated to krbV.KRB5_KDC_UNREACH error. However,
>>>> krb5kdc.log (http://pastebin.test.redhat.com/271394) reports errors
>>>> which are seemingly unrelated to the root cause (kinit timing out on
>>>> getting host TGT).
>>>>
>>>> Thus I'm not quite sure which errors should we chceck against in this
>>>> case, anyone care to advise? These are potential candidates:
>>>>
>>>> KRB5KDC_ERR_SVC_UNAVAILABLE, "A service is not available that is
>>>> required to process the request"
>>>> KRB5KRB_ERR_RESPONSE_TOO_BIG,    "Response too big for UDP, retry with TCP"
>>>> KRB5_REALM_UNKNOWN,      "Cannot find KDC for requested realm"
>>>> KRB5_KDC_UNREACH,        "Cannot contact any KDC for requested realm"
>>>>
>>>
>>> The only ones that you should retry on, at first glance are
>>> KRB5_KDC_UNREACH, KRB5KDC_ERR_SVC_UNAVAILABLE.
>>>
>>> You should never see KRB5KRB_ERR_RESPONSE_TOO_BIG in the script as it
>>> should be handled automatically by the library, and if you get
>>> KRB5_REALM_UNKNOWN I do not think that retrying will make any
>>> difference.
>>
>> I might be wrong but I was under the impression that this feature was also for
>> workarounding replication delay - service is not available / key is not
>> present / something like that.
>>
>> (This could happen if host/principal was added to one server but then the
>> client connected to another server or so.)
>
> If we have that problem we should instead use a temporary krb5.conf file
> that lists explicitly only the server we are joining.
>
> Simo.
>

This is already done since ipa-3-0: by default only one server/KDC is 
used during client install so there are actually no problems with 
replication delay, only with KDC timeouts.

Anyway I'm sending updated patches.

-- 
Martin^3 Babinsky
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-mbabinsk-0015-6-ipautil-new-functions-kinit_keytab-and-kinit_passwor.patch
Type: text/x-patch
Size: 4471 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20150325/8721a58b/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-mbabinsk-0016-5-ipa-client-install-try-to-get-host-TGT-several-times.patch
Type: text/x-patch
Size: 8794 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20150325/8721a58b/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/20150325/8721a58b/attachment-0002.bin>