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 <mbabi...@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. > + 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? > + 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 <mbabi...@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. > + > + 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. > + 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. > 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. > +.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 <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. > +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. > + 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. > + 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 :-) -- Petr^2 Spacek -- 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