On 11.3.2015 14:27, Martin Babinsky wrote: > Actually, now that I think about it, I will try to address some of your > comments:
>>> + except krbV.Krb5Error, e: >> except ... , ... syntax is not going to work in Python 3. Maybe 'as' would be >> better? >> > AFAIK except ... as ... syntax was added in Python 2.6. Using this syntax can > break older versions of Python. If this is not a concern for us, I will fix > this and use this syntax also in my later patches. Please see http://www.freeipa.org/page/Python_Coding_Style :-) Python 2.7 is required anyway. >>> diff --git a/ipa-client/ipa-install/ipa-client-install >>> b/ipa-client/ipa-install/ipa-client-install >>> index >>> ccaab5536e83b4b6ac60b81132c3455c0af19ae1..c817f9e86dbaa6a2cca7d1a463f53d491fa7badb >>> 100755 >>> --- a/ipa-client/ipa-install/ipa-client-install >>> +++ b/ipa-client/ipa-install/ipa-client-install >>> @@ -91,6 +91,13 @@ def parse_options(): >>> >>> parser.values.ca_cert_file = value >>> >>> + def validate_kinit_attempts_option(option, opt, value, parser): >>> + if value < 1 or value > sys.maxint: >>> + raise OptionValueError( >>> + "%s option has invalid value %d" % (opt, value)) >> It would be nice if the error message said what is the expected value. >> ("Expected integer in range <1,%s>" % sys.maxint) >> >> BTW is it possible to do this using existing option parser? I would expect >> some generic support for type=uint or something similar. >> > OptionParser supports 'type' keywords when adding options, which perform the > neccessary conversions (int(), etc) and validation (see > https://docs.python.org/2/library/optparse.html#optparse-standard-option-types). > However, in this case you still have to manually check for values less that 1 > which do not make sense. AFAIK OptionParser has no built-in way to do this. Okay then. >>> + >>> + parser.values.kinit_attempts = value >>> + >>> parser = IPAOptionParser(version=version.VERSION) >>> >>> basic_group = OptionGroup(parser, "basic options") >>> @@ -144,6 +151,11 @@ def parse_options(): >>> help="do not modify the nsswitch.conf and PAM >>> configuration") >>> basic_group.add_option("-f", "--force", dest="force", >>> action="store_true", >>> default=False, help="force setting of LDAP/Kerberos >>> conf") >>> + basic_group.add_option('--kinit-attempts', dest='kinit_attempts', >>> + action='callback', type='int', default=5, >> >> It would be good to check lockout numbers in default configuration to make >> sure that replication delay will not lock the principal. >> > I'm not sure that I follow, could you be more specific what you mean by this? KDC and DS will lock account after n failed attempts. See $ ipa pwpolicy-find to find out the number in your installation (keytab == password). >>> freeipa-mbabinsk-0017-2-Adopted-kinit_keytab-and-kinit_password-for-kerberos.patch >>> >>> >>> >>> From 912113529138e5b1bd8357ae6a17376cb5d32759 Mon Sep 17 00:00:00 2001 >>> From: Martin Babinsky <mbabi...@redhat.com> >>> Date: Mon, 9 Mar 2015 12:54:36 +0100 >>> Subject: [PATCH 3/3] Adopted kinit_keytab and kinit_password for kerberos >>> auth >>> >>> --- >>> daemons/dnssec/ipa-dnskeysync-replica | 6 ++- >>> daemons/dnssec/ipa-dnskeysyncd | 2 +- >>> daemons/dnssec/ipa-ods-exporter | 5 ++- >>> .../certmonger/dogtag-ipa-ca-renew-agent-submit | 3 +- >>> install/restart_scripts/renew_ca_cert | 7 ++-- >>> install/restart_scripts/renew_ra_cert | 4 +- >>> ipa-client/ipa-install/ipa-client-automount | 9 ++-- >>> ipa-client/ipaclient/ipa_certupdate.py | 3 +- >>> ipaserver/rpcserver.py | 49 >>> +++++++++++----------- >>> 9 files changed, 47 insertions(+), 41 deletions(-) >>> >>> diff --git a/daemons/dnssec/ipa-dnskeysync-replica >>> b/daemons/dnssec/ipa-dnskeysync-replica >>> index >>> d04f360e04ee018dcdd1ba9b2ca42b1844617af9..e9cae519202203a10678b7384e5acf748f256427 >>> 100755 >>> --- a/daemons/dnssec/ipa-dnskeysync-replica >>> +++ b/daemons/dnssec/ipa-dnskeysync-replica >>> @@ -139,14 +139,16 @@ log.setLevel(level=logging.DEBUG) >>> # Kerberos initialization >>> PRINCIPAL = str('%s/%s' % (DAEMONNAME, ipalib.api.env.host)) >>> log.debug('Kerberos principal: %s', PRINCIPAL) >>> -ipautil.kinit_hostprincipal(paths.IPA_DNSKEYSYNCD_KEYTAB, WORKDIR, >>> PRINCIPAL) >>> +ccache_filename = os.path.join(WORKDIR, 'ccache') >> BTW I really appreciate this patch set! We finally can use more descriptive >> names like 'ipa-dnskeysync-replica.ccache' which sometimes make debugging >> easier. >> > Named ccaches seems to be a good idea. I will fix this in all places where the > ccache is somehow persistent (and not deleted after installation). Thank you! >>> diff --git a/ipa-client/ipa-install/ipa-client-automount >>> b/ipa-client/ipa-install/ipa-client-automount >>> index >>> 7b9e701dead5f50a033a455eb62e30df78cc0249..19197d34ca580062742b3d7363e5dfb2dad0e4de >>> 100755 >>> --- a/ipa-client/ipa-install/ipa-client-automount >>> +++ b/ipa-client/ipa-install/ipa-client-automount >>> @@ -425,10 +425,11 @@ def main(): >>> os.close(ccache_fd) >>> try: >>> try: >>> - os.environ['KRB5CCNAME'] = ccache_name >>> - ipautil.run([paths.KINIT, '-k', '-t', paths.KRB5_KEYTAB, >>> 'host/%s@%s' % (api.env.host, api.env.realm)]) >>> - except ipautil.CalledProcessError, e: >>> - sys.exit("Failed to obtain host TGT.") >>> + host_princ = str('host/%s@%s' % (api.env.host, api.env.realm)) >>> + ipautil.kinit_keytab(paths.KRB5_KEYTAB, ccache_name, >> I'm not sure what is ccache_name here but it should be something descriptive. >> > In this case ccache_name points to a temporary file made by tempfile.mkstemp() > which is cleaned up in a later finally: block (so you will not get to it even > if the whole thing comes crashing). I'm not sure if there's a point in > renaming it. Okay, that is exactly where I wasn't sure :-) -- Petr^2 Spacek -- 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
