[Freeipa-devel] [PATCHES 0015-0017] consolidation of various Kerberos auth methods in FreeIPA code
Petr Spacek
pspacek at redhat.com
Wed Mar 11 14:53:03 UTC 2015
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 <mbabinsk at 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
More information about the Freeipa-devel
mailing list