[Freeipa-devel] [PATCHES 0015-0017] consolidation of various Kerberos auth methods in FreeIPA code
Simo Sorce
simo at redhat.com
Thu Apr 9 14:15:27 UTC 2015
On Thu, 2015-04-09 at 15:38 +0200, Jan Cholasta wrote:
> Dne 9.4.2015 v 14:41 Simo Sorce napsal(a):
> > On Wed, 2015-03-25 at 11:52 +0100, Martin Babinsky wrote:
> >> 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.
> >
> > LGTM!
> >
> > Simo.
> >
> >
>
> 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.
>
>
> 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.
>
>
> 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.
>
>
> 2) Why is there the sys.maxint upper bound on --kinit-attempts again? A
> comment with explanation would be nice.
>
>
> Patch 17:
>
> 1) Is there a reason for the ccache filename changes in DNSSEC code?
>
Good catches, although I think they are mostly nitpicks, it would be
nice to get these changed as you ask before pushing.
Simo.
--
Simo Sorce * Red Hat, Inc * New York
More information about the Freeipa-devel
mailing list