[Freeipa-devel] [PATCHES 0015-0017] consolidation of various Kerberos auth methods in FreeIPA code

Wed Mar 11 15:19:10 UTC 2015

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