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


Thank you for plenty of suggestions Petr. I will try to update the patches accordingly.

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