On 03/11/2015 03:53 PM, Petr Spacek wrote:
On 11.3.2015 14:27, Martin Babinsky wrote:
Actually, now that I think about it, I will try to address some of your 

+        except krbV.Krb5Error, e:
except ... , ... syntax is not going to work in Python 3. Maybe 'as' would be

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.

Ah I have forgotten about our Coding Style page completely! Ok 'except ... as e' it is then.
diff --git a/ipa-client/ipa-install/ipa-client-install
--- 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
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
       basic_group.add_option("-f", "--force", dest="force",
                         default=False, help="force setting of LDAP/Kerberos
+    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).

Ok I will check it.

  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
--- 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

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
--- a/ipa-client/ipa-install/ipa-client-automount
+++ b/ipa-client/ipa-install/ipa-client-automount
@@ -425,10 +425,11 @@ def main():
-            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 :-)

This situation (making temporary ccache and then cleaning it up) also occurs in ipa-client-install, renew-ca/ra-cert, dogtag-ipa-ca-renew-agent-submit (what a name!), and ipa_certupdate.

Martin^3 Babinsky

Manage your subscription for the Freeipa-devel mailing list:
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

Reply via email to