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

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

Ok I will check it.
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 :-)

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:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

Reply via email to