[Freeipa-devel] [PATCHES 0015-0017] consolidation of various Kerberos auth methods in FreeIPA code
Martin Babinsky
mbabinsk at redhat.com
Wed Mar 11 13:27:10 UTC 2015
Actually, now that I think about it, I will try to address some of your
comments:
On 03/11/2015 12:42 PM, Petr Spacek wrote:
> Hello Martin^3,
>
> good work, we are almost there! Please see my nitpicks in-line.
>
> On 9.3.2015 13:06, Martin Babinsky wrote:
>> On 03/06/2015 01:05 PM, Martin Babinsky wrote:
>>> This series of patches for the master/4.1 branch attempts to implement
>>> some of the Rob's and Petr Vobornik's ideas which originated from a
>>> discussion on this list regarding my original patch fixing
>>> https://fedorahosted.org/freeipa/ticket/4808.
>>>
>>> I suppose that these patches are just a first iteration, we may further
>>> discuss if this is the right thing to do.
>>>
>>> Below is a quote from the original discussion just to get the context:
>>>
>>>
>>>
>>
>> The next iteration of patches is attached below. Thanks to jcholast and
>> pvoborni for the comments and insights.
>>
>> --
>> Martin^3 Babinsky
>>
>> freeipa-mbabinsk-0015-2-ipautil-new-functions-kinit_keytab-and-kinit_passwor.patch
>>
>>
>> From 97e4eed332391bab7a12dc593152e369f347fd3c Mon Sep 17 00:00:00 2001
>> From: Martin Babinsky <mbabinsk at redhat.com>
>> Date: Mon, 9 Mar 2015 12:53:10 +0100
>> Subject: [PATCH 1/3] ipautil: new functions kinit_keytab and kinit_password
>>
>> kinit_keytab replaces kinit_hostprincipal and performs Kerberos auth using
>> keytab file. Function is also able to repeat authentication multiple times
>> before giving up and raising StandardError.
>>
>> kinit_password wraps kinit auth using password and also supports FAST
>> authentication using httpd armor ccache.
>> ---
>> ipapython/ipautil.py | 60 ++++++++++++++++++++++++++++++++++++++++------------
>> 1 file changed, 46 insertions(+), 14 deletions(-)
>>
>> diff --git a/ipapython/ipautil.py b/ipapython/ipautil.py
>> index 4116d974e620341119b56fad3cff1bda48af3bab..4547165ccf24ff6edf5c65e756aa321aa34b9e61 100644
>> --- a/ipapython/ipautil.py
>> +++ b/ipapython/ipautil.py
>> @@ -1175,27 +1175,59 @@ def wait_for_open_socket(socket_name, timeout=0):
>> else:
>> raise e
>>
>> -def kinit_hostprincipal(keytab, ccachedir, principal):
>> +
>> +def kinit_keytab(keytab, ccache_path, principal, attempts=1):
>> """
>> - Given a ccache directory and a principal kinit as that user.
>> + Given a ccache_path , keytab file and a principal kinit as that user.
>> +
>> + The optional parameter 'attempts' specifies how many times the credential
>> + initialization should be attempted before giving up and raising
>> + StandardError.
>>
>> This blindly overwrites the current CCNAME so if you need to save
>> it do so before calling this function.
>>
>> Thus far this is used to kinit as the local host.
>> """
>> - try:
>> - ccache_file = 'FILE:%s/ccache' % ccachedir
>> - krbcontext = krbV.default_context()
>> - ktab = krbV.Keytab(name=keytab, context=krbcontext)
>> - princ = krbV.Principal(name=principal, context=krbcontext)
>> - os.environ['KRB5CCNAME'] = ccache_file
>> - ccache = krbV.CCache(name=ccache_file, context=krbcontext, primary_principal=princ)
>> - ccache.init(princ)
>> - ccache.init_creds_keytab(keytab=ktab, principal=princ)
>> - return ccache_file
>> - except krbV.Krb5Error, e:
>> - raise StandardError('Error initializing principal %s in %s: %s' % (principal, keytab, str(e)))
>> + root_logger.debug("Initializing principal %s using keytab %s"
>> + % (principal, keytab))
>> + for attempt in xrange(attempts):
> I would like to see new code compatible with Python 3. Here I'm not sure what
> is the generic solution for xrange but in this particular case I would
> recommend you to use just range. Attempts variable should have small values so
> the x/range differences do not matter here.
>
>> + try:
>> + krbcontext = krbV.default_context()
>> + ktab = krbV.Keytab(name=keytab, context=krbcontext)
>> + princ = krbV.Principal(name=principal, context=krbcontext)
>> + os.environ['KRB5CCNAME'] = ccache_path
> This is a bit scary, especially when it comes to multi-threaded execution. If
> it is really necessary please add comment that this method is not thread-safe.
>
So far this function is used in various installers/uninstallers/restart
scripts, so I'm not quite sure if there is even some multithreaded
thingy going on there (I have never done multithreaded programming so I
don't know, someone with more experience can comment on this). I will,
however, add the warning to the docstring.
>> + ccache = krbV.CCache(name=ccache_path, context=krbcontext,
>> + primary_principal=princ)
>> + ccache.init(princ)
>> + ccache.init_creds_keytab(keytab=ktab, principal=princ)
>> + root_logger.debug("Attempt %d/%d: success"
>> + % (attempt + 1, attempts))
> What about adding + 1 to range boundaries instead of + 1 here and there?
>
>> + return
>> + 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.
>> + root_logger.debug("Attempt %d/%d: failed"
>> + % (attempt + 1, attempts))
>> + time.sleep(1)
>> +
>> + root_logger.debug("Maximum number of attempts (%d) reached"
>> + % attempts)
>> + raise StandardError("Error initializing principal %s: %s"
>> + % (principal, str(e)))
>> +
>> +
>> +def kinit_password(principal, password, env={}, armor_ccache=""):
>> + """perform interactive kinit as principal using password. Additional
>> + enviroment variables can be specified using env. If using FAST for
>> + web-based authentication, use armor_ccache to specify http service ccache.
>> + """
>> + root_logger.debug("Initializing principal %s using password" % principal)
>> + args = [paths.KINIT, principal]
>> + if armor_ccache:
>> + root_logger.debug("Using armor ccache %s for FAST webauth"
>> + % armor_ccache)
>> + args.extend(['-T', armor_ccache])
>> + run(args, env=env, stdin=password)
>> +
>>
>> def dn_attribute_property(private_name):
>> '''
>> -- 2.1.0
>>
>>
>> freeipa-mbabinsk-0016-2-ipa-client-install-try-to-get-host-TGT-several-times.patch
>>
>>
>> From e438d8a0711b4271d24d7d24e98395503912a1c4 Mon Sep 17 00:00:00 2001
>> From: Martin Babinsky <mbabinsk at redhat.com>
>> Date: Mon, 9 Mar 2015 12:53:57 +0100
>> Subject: [PATCH 2/3] ipa-client-install: try to get host TGT several times
>> before giving up
>>
>> New option '--kinit-attempts' enables the host to make multiple attempts to
>> obtain TGT from KDC before giving up and aborting client installation.
>>
>> In addition, all kinit attempts were replaced by calls to
>> 'ipautil.kinit_keytab' and 'ipautil.kinit_password'.
>>
>> https://fedorahosted.org/freeipa/ticket/4808
>> ---
>> ipa-client/ipa-install/ipa-client-install | 65 +++++++++++++++++--------------
>> ipa-client/man/ipa-client-install.1 | 5 +++
>> 2 files changed, 41 insertions(+), 29 deletions(-)
>>
>> 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.
>> +
>> + 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?
>> + callback=validate_kinit_attempts_option,
>> + help=("number of attempts to obtain host TGT"
>> + " (defaults to %default)."))
>> basic_group.add_option("-d", "--debug", dest="debug", action="store_true",
>> default=False, help="print debugging information")
>> basic_group.add_option("-U", "--unattended", dest="unattended",
>> @@ -2351,6 +2363,7 @@ def install(options, env, fstore, statestore):
>> root_logger.debug(
>> "will use principal provided as option: %s", options.principal)
>>
>> + host_principal = 'host/%s@%s' % (hostname, cli_realm)
>> if not options.on_master:
>> nolog = tuple()
>> # First test out the kerberos configuration
>> @@ -2371,7 +2384,6 @@ def install(options, env, fstore, statestore):
>> env['KRB5_CONFIG'] = krb_name
>> (ccache_fd, ccache_name) = tempfile.mkstemp()
>> os.close(ccache_fd)
>> - env['KRB5CCNAME'] = os.environ['KRB5CCNAME'] = ccache_name
>> join_args = [paths.SBIN_IPA_JOIN,
>> "-s", cli_server[0],
>> "-b", str(realm_to_suffix(cli_realm)),
>> @@ -2409,29 +2421,24 @@ def install(options, env, fstore, statestore):
>> else:
>> stdin = sys.stdin.readline()
>>
>> - (stderr, stdout, returncode) = run(["kinit", principal],
>> - raiseonerr=False,
>> - stdin=stdin,
>> - env=env)
>> - if returncode != 0:
>> + try:
>> + ipautil.kinit_password(principal, stdin, env)
>> + except CalledProcessError, e:
>> print_port_conf_info()
>> - root_logger.error("Kerberos authentication failed")
>> - root_logger.info("%s", stdout)
>> + root_logger.error("Kerberos authentication failed: %s"
>> + % str(e))
> Isn't str() redundant? IMHO %s calls str() implicitly.
>
Indeed it does, I will fix it (also in other places).
>> return CLIENT_INSTALL_ERROR
>> elif options.keytab:
>> join_args.append("-f")
>> if os.path.exists(options.keytab):
>> - (stderr, stdout, returncode) = run(
>> - [paths.KINIT,'-k', '-t', options.keytab,
>> - 'host/%s@%s' % (hostname, cli_realm)],
>> - env=env,
>> - raiseonerr=False)
>> -
>> - if returncode != 0:
>> + try:
>> + ipautil.kinit_keytab(options.keytab, ccache_name,
>> + host_principal,
>> + attempts=options.kinit_attempts)
>> + except StandardError, e:
>> print_port_conf_info()
>> - root_logger.error("Kerberos authentication failed "
>> - "using keytab: %s", options.keytab)
>> - root_logger.info("%s", stdout)
>> + root_logger.error("Kerberos authentication failed: %s"
>> + % str(e))
> Again str().
>
>> return CLIENT_INSTALL_ERROR
>> else:
>> root_logger.error("Keytab file could not be found: %s"
>> @@ -2501,12 +2508,13 @@ def install(options, env, fstore, statestore):
>> # only the KDC we're installing under is contacted.
>> # Other KDCs might not have replicated the principal yet.
>> # Once we have the TGT, it's usable on any server.
>> - env['KRB5CCNAME'] = os.environ['KRB5CCNAME'] = CCACHE_FILE
>> try:
>> - run([paths.KINIT, '-k', '-t', paths.KRB5_KEYTAB,
>> - 'host/%s@%s' % (hostname, cli_realm)], env=env)
>> - except CalledProcessError, e:
>> - root_logger.error("Failed to obtain host TGT.")
>> + ipautil.kinit_keytab(paths.KRB5_KEYTAB, CCACHE_FILE,
>> + host_principal,
>> + attempts=options.kinit_attempts)
>> + except StandardError, e:
>> + print_port_conf_info()
>> + root_logger.error("Failed to obtain host TGT: %s" % str(e))
> str()?
>
>> # failure to get ticket makes it impossible to login and bind
>> # from sssd to LDAP, abort installation and rollback changes
>> return CLIENT_INSTALL_ERROR
>> @@ -2543,16 +2551,15 @@ def install(options, env, fstore, statestore):
>> return CLIENT_INSTALL_ERROR
>> root_logger.info("Configured /etc/sssd/sssd.conf")
>>
>> - host_principal = 'host/%s@%s' % (hostname, cli_realm)
>> if options.on_master:
>> # If on master assume kerberos is already configured properly.
>> # Get the host TGT.
>> - os.environ['KRB5CCNAME'] = CCACHE_FILE
>> try:
>> - run([paths.KINIT, '-k', '-t', paths.KRB5_KEYTAB,
>> - host_principal])
>> - except CalledProcessError, e:
>> - root_logger.error("Failed to obtain host TGT.")
>> + ipautil.kinit_keytab(paths.KRB5_KEYTAB, CCACHE_FILE,
>> + host_principal,
>> + attempts=options.kinit_attempts)
>> + except StandardError, e:
>> + root_logger.error("Failed to obtain host TGT: %s" % str(e))
> str()? I will not mention it again but please look for it.
>
>> return CLIENT_INSTALL_ERROR
>> else:
>> # Configure krb5.conf
>> diff --git a/ipa-client/man/ipa-client-install.1 b/ipa-client/man/ipa-client-install.1
>> index 726a6c133132dd2e3ba2fde43d8a2ec0549bfcef..56ed899a25e626b8ae61714f77f3588059fa86f9 100644
>> --- a/ipa-client/man/ipa-client-install.1
>> +++ b/ipa-client/man/ipa-client-install.1
>> @@ -152,6 +152,11 @@ Do not use Authconfig to modify the nsswitch.conf and PAM configuration.
>> \fB\-f\fR, \fB\-\-force\fR
>> Force the settings even if errors occur
>> .TP
>> +\fB\-\-kinit\-attempts\fR=\fIKINIT_ATTEMPTS\fR
>> +Number of unsuccessful attempts to obtain host TGT that will be performed
>> +before aborting client installation. \fIKINIT_ATTEMPTS\fR should be a number
>> +greater than zero. By default 5 attempts to get TGT are performed.
> It would be nice to add a rationale to the text. Current text is somehow
> confusing for users not familiar with replication and related problems.
>
> My hope is descriptive manual will prevent users from creating cargo-cults
> based on copy&pastings texts they do not understand.
>
I will try to reformulate this so it makes more sense.
>> +.TP
>> \fB\-d\fR, \fB\-\-debug\fR
>> Print debugging information to stdout
>> .TP
>> -- 2.1.0
>>
>>
>> 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).
>> +ipautil.kinit_keytab(paths.IPA_DNSKEYSYNCD_KEYTAB, ccache_filename,
>> + PRINCIPAL)
>> log.debug('Got TGT')
>>
>> # LDAP initialization
>> ldap = ipalib.api.Backend[ldap2]
>> # fixme
>> log.debug('Connecting to LDAP')
>> -ldap.connect(ccache="%s/ccache" % WORKDIR)
>> +ldap.connect(ccache=ccache_filename)
>> log.debug('Connected')
>>
>>
>> diff --git a/daemons/dnssec/ipa-dnskeysyncd b/daemons/dnssec/ipa-dnskeysyncd
>> index 54a08a1e6307e89b3f52e78bddbe28cda8ac1345..4e64596c7f8ccd6cff47df4772c875917c71606a 100755
>> --- a/daemons/dnssec/ipa-dnskeysyncd
>> +++ b/daemons/dnssec/ipa-dnskeysyncd
>> @@ -65,7 +65,7 @@ log = root_logger
>> # Kerberos initialization
>> PRINCIPAL = str('%s/%s' % (DAEMONNAME, api.env.host))
>> log.debug('Kerberos principal: %s', PRINCIPAL)
>> -ipautil.kinit_hostprincipal(KEYTAB_FB, WORKDIR, PRINCIPAL)
>> +ipautil.kinit_keytab(KEYTAB_FB, os.path.join(WORKDIR, 'ccache'), PRINCIPAL)
> 'ipa-dnskeysyncd.ccache'?
>
>> # LDAP initialization
>> basedn = DN(api.env.container_dns, api.env.basedn)
>> diff --git a/daemons/dnssec/ipa-ods-exporter b/daemons/dnssec/ipa-ods-exporter
>> index dc1851d3a34bb09c1a87c86d101b11afe35e49fe..0cf825950338cdce330a15b3ea22150f6e02588a 100755
>> --- a/daemons/dnssec/ipa-ods-exporter
>> +++ b/daemons/dnssec/ipa-ods-exporter
>> @@ -399,7 +399,8 @@ ipalib.api.finalize()
>> # Kerberos initialization
>> PRINCIPAL = str('%s/%s' % (DAEMONNAME, ipalib.api.env.host))
>> log.debug('Kerberos principal: %s', PRINCIPAL)
>> -ipautil.kinit_hostprincipal(paths.IPA_ODS_EXPORTER_KEYTAB, WORKDIR, PRINCIPAL)
>> +ccache_name = os.path.join(WORKDIR, 'ccache')
> 'ipa-ods-exporter.ccache'?
>
>> +ipautil.kinit_keytab(paths.IPA_ODS_EXPORTER_KEYTAB, ccache_name, PRINCIPAL)
>> log.debug('Got TGT')
>>
>> # LDAP initialization
>> @@ -407,7 +408,7 @@ dns_dn = DN(ipalib.api.env.container_dns, ipalib.api.env.basedn)
>> ldap = ipalib.api.Backend[ldap2]
>> # fixme
>> log.debug('Connecting to LDAP')
>> -ldap.connect(ccache="%s/ccache" % WORKDIR)
>> +ldap.connect(ccache=ccache_name)
>> log.debug('Connected')
>>
>>
>> diff --git a/install/certmonger/dogtag-ipa-ca-renew-agent-submit b/install/certmonger/dogtag-ipa-ca-renew-agent-submit
>> index 7b91fc61148912c77d0ae962b3847d73e8d0720e..13d2c2a912d2fcf84053d36da5e07fc834f9cf25 100755
>> --- a/install/certmonger/dogtag-ipa-ca-renew-agent-submit
>> +++ b/install/certmonger/dogtag-ipa-ca-renew-agent-submit
>> @@ -440,7 +440,8 @@ def main():
>> certs.renewal_lock.acquire()
>> try:
>> principal = str('host/%s@%s' % (api.env.host, api.env.realm))
>> - ipautil.kinit_hostprincipal(paths.KRB5_KEYTAB, tmpdir, principal)
>> + ipautil.kinit_keytab(paths.KRB5_KEYTAB, os.path.join(tmpdir, 'ccache'),
> 'dogtag-ipa-ca-renew-agent-submit.ccache'?
>
>> + principal)
>>
>> profile = os.environ.get('CERTMONGER_CA_PROFILE')
>> if profile:
>> diff --git a/install/restart_scripts/renew_ca_cert b/install/restart_scripts/renew_ca_cert
>> index c7bd5d74c5b4659b3ad66d630653ff6419868d99..67156122bb75f00a4c3f612697092e5bab3723fb 100644
>> --- a/install/restart_scripts/renew_ca_cert
>> +++ b/install/restart_scripts/renew_ca_cert
>> @@ -73,8 +73,9 @@ def _main():
>> tmpdir = tempfile.mkdtemp(prefix="tmp-")
>> try:
>> principal = str('host/%s@%s' % (api.env.host, api.env.realm))
>> - ccache = ipautil.kinit_hostprincipal(paths.KRB5_KEYTAB, tmpdir,
>> - principal)
>> + ccache_filename = '%s/ccache' % tmpdir
> 'renew_ca_cert.ccache'?
>
>> + ipautil.kinit_keytab(paths.KRB5_KEYTAB, ccache_filename,
>> + principal)
>>
>> ca = cainstance.CAInstance(host_name=api.env.host, ldapi=False)
>> ca.update_cert_config(nickname, cert, configured_constants)
>> @@ -139,7 +140,7 @@ def _main():
>> conn = None
>> try:
>> conn = ldap2(shared_instance=False, ldap_uri=api.env.ldap_uri)
>> - conn.connect(ccache=ccache)
>> + conn.connect(ccache=ccache_filename)
>> except Exception, e:
>> syslog.syslog(
>> syslog.LOG_ERR, "Failed to connect to LDAP: %s" % e)
>> diff --git a/install/restart_scripts/renew_ra_cert b/install/restart_scripts/renew_ra_cert
>> index 7dae3562380e919b2cc5f53825820291fc93fdc5..6276de78e4528dc1caa39be6628094a9d00e5988 100644
>> --- a/install/restart_scripts/renew_ra_cert
>> +++ b/install/restart_scripts/renew_ra_cert
>> @@ -42,8 +42,8 @@ def _main():
>> tmpdir = tempfile.mkdtemp(prefix="tmp-")
>> try:
>> principal = str('host/%s@%s' % (api.env.host, api.env.realm))
>> - ccache = ipautil.kinit_hostprincipal(paths.KRB5_KEYTAB, tmpdir,
>> - principal)
>> + ipautil.kinit_keytab(paths.KRB5_KEYTAB, '%s/ccache' % tmpdir,
> 'renew_ra_cert.ccache'?
>
>> + principal)
>>
>> ca = cainstance.CAInstance(host_name=api.env.host, ldapi=False)
>> if ca.is_renewal_master():
>> 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.
>> + host_princ)
>> + except StandardError, e:
>> + sys.exit("Failed to obtain host TGT: %s" % e)
>> # Now we have a TGT, connect to IPA
>> try:
>> api.Backend.rpcclient.connect()
>> diff --git a/ipa-client/ipaclient/ipa_certupdate.py b/ipa-client/ipaclient/ipa_certupdate.py
>> index 031a34c3a54a02d43978eedcb794678a1550702b..d6f7bbb3daff3ae4dced5d69f83a0516003235ab 100644
>> --- a/ipa-client/ipaclient/ipa_certupdate.py
>> +++ b/ipa-client/ipaclient/ipa_certupdate.py
>> @@ -57,7 +57,8 @@ class CertUpdate(admintool.AdminTool):
>> tmpdir = tempfile.mkdtemp(prefix="tmp-")
>> try:
>> principal = str('host/%s@%s' % (api.env.host, api.env.realm))
>> - ipautil.kinit_hostprincipal(paths.KRB5_KEYTAB, tmpdir, principal)
>> + ipautil.kinit_keytab(paths.KRB5_KEYTAB,
>> + os.path.join(tmpdir, 'ccache'), principal)
> 'ipa_certupdate.ccache'?
>
>>
>> api.Backend.rpcclient.connect()
>> try:
>> diff --git a/ipaserver/rpcserver.py b/ipaserver/rpcserver.py
>> index d6bc955b9d9910a24eec5df1def579310eb54786..36f16908ac8477d9982bfee613b77576853054eb 100644
>> --- a/ipaserver/rpcserver.py
>> +++ b/ipaserver/rpcserver.py
>> @@ -958,8 +958,8 @@ class login_password(Backend, KerberosSession, HTTP_Status):
>>
>> def kinit(self, user, realm, password, ccache_name):
>> # get http service ccache as an armor for FAST to enable OTP authentication
>> - armor_principal = krb5_format_service_principal_name(
>> - 'HTTP', self.api.env.host, realm)
>> + armor_principal = str(krb5_format_service_principal_name(
>> + 'HTTP', self.api.env.host, realm))
>> keytab = paths.IPA_KEYTAB
>> armor_name = "%sA_%s" % (krbccache_prefix, user)
>> armor_path = os.path.join(krbccache_dir, armor_name)
>> @@ -967,34 +967,33 @@ class login_password(Backend, KerberosSession, HTTP_Status):
>> self.debug('Obtaining armor ccache: principal=%s keytab=%s ccache=%s',
>> armor_principal, keytab, armor_path)
>>
>> - (stdout, stderr, returncode) = ipautil.run(
>> - [paths.KINIT, '-kt', keytab, armor_principal],
>> - env={'KRB5CCNAME': armor_path}, raiseonerr=False)
>> -
>> - if returncode != 0:
>> - raise CCacheError()
>> + try:
>> + ipautil.kinit_keytab(paths.IPA_KEYTAB, armor_path,
>> + armor_principal)
>> + except StandardError, e:
>> + raise CCacheError(str(e))
>>
>> # Format the user as a kerberos principal
>> principal = krb5_format_principal_name(user, realm)
>>
>> - (stdout, stderr, returncode) = ipautil.run(
>> - [paths.KINIT, principal, '-T', armor_path],
>> - env={'KRB5CCNAME': ccache_name, 'LC_ALL': 'C'},
>> - stdin=password, raiseonerr=False)
>> + try:
>> + ipautil.kinit_password(principal, password,
>> + env={'KRB5CCNAME': ccache_name,
>> + 'LC_ALL': 'C'},
>> + armor_ccache=armor_path)
>>
>> - self.debug('kinit: principal=%s returncode=%s, stderr="%s"',
>> - principal, returncode, stderr)
>> -
>> - self.debug('Cleanup the armor ccache')
>> - ipautil.run(
>> - [paths.KDESTROY, '-A', '-c', armor_path],
>> - env={'KRB5CCNAME': armor_path},
>> - raiseonerr=False)
>> -
>> - if returncode != 0:
>> - if stderr.strip() == 'kinit: Cannot read password while getting initial credentials':
>> - raise PasswordExpired(principal=principal, message=unicode(stderr))
>> - raise InvalidSessionPassword(principal=principal, message=unicode(stderr))
>> + self.debug('Cleanup the armor ccache')
>> + ipautil.run(
>> + [paths.KDESTROY, '-A', '-c', armor_path],
>> + env={'KRB5CCNAME': armor_path},
>> + raiseonerr=False)
>> + except ipautil.CalledProcessError, e:
>> + if ('kinit: Cannot read password while '
>> + 'getting initial credentials') in e.output:
> I know it is not your code but please make sure it will work with non-English
> LANG or LC_MESSAGE.
>
Ah yes I did not realize it, I will try to fix it.
>> + raise PasswordExpired(principal=principal,
>> + message=unicode(e.output))
>> + raise InvalidSessionPassword(principal=principal,
>> + message=unicode(e.output))
>>
>> class change_password(Backend, HTTP_Status):
>
> Hopefully this review is not too annoying :-)
>
Not at all :).
--
Martin^3 Babinsky
More information about the Freeipa-devel
mailing list