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 -- Manage your subscription for the Freeipa-devel mailing list: https://www.redhat.com/mailman/listinfo/freeipa-devel Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code