Updated patch attached; comments inline below.

On Mon, Apr 25, 2016 at 07:55:46AM +0200, Jan Cholasta wrote:
> I think it would be better to merge the `client` and `client_servicename`
> into a single `client_principal` argument, as both of the arguments are used
> only to specify the principal name of the client.
> 
Done.

> Also I would prefer if the keyfile and keytab arguments were required,
> because it's better if you can explicitly see what values are used at the
> call site.
> 
Done.

> Why is init_creds() now called from __init__()? Why is it still called from
> _auth_header()?
> 
I invoke it from __init__ for more eager failure if there's a
problem (e.g. service is not in keytab), giving better error
locality.

It remains necessary to invoke it from _auth_header in case of
short-lived credentials.  I added some comments to the source.

> Why is ldap_uri now passed to IPAKEMKeys()?
> 
It tries to use LDAPI by default, so ldap_uri needs to be passed
through if process owner cannot access LDAPI socket.  I added
commentary to source.

Regarding your suggestion to make a base class and override class
variables, I prefer to pass values around.  There are very few
instantiations of CustodiaClient so it is easy enough to follow.

Thanks,
Fraser
From ba34a8f5c9b3e31a511e01266e3d3fedd53bcca6 Mon Sep 17 00:00:00 2001
From: Fraser Tweedale <ftwee...@redhat.com>
Date: Fri, 8 Apr 2016 15:21:19 +1000
Subject: [PATCH] Allow CustodiaClient to be used by arbitrary principals

Currently CustodiaClient assumes that the client is the host
principal, and it is hard-coded to read the host keytab and server
keys.

For the Lightweight CAs feature, Dogtag on CA replicas will use
CustodiaClient to retrieve signing keys from the originating
replica.  Because this process runs as 'pkiuser', the host keys
cannot be used; instead, each Dogtag replica will have a service
principal to use for Custodia authentication.

Update CustodiaClient to require specifying the client keytab and
Custodia keyfile to use, and change the client argument to be a full
GSS service name (instead of hard-coding host service) to load from
the keytab.  Update call sites accordingly.

Also pass the given 'ldap_uri' argument through to IPAKEMKeys
because without it, the client tries to use LDAPI, but may not have
access.

Part of: https://fedorahosted.org/freeipa/ticket/4559
---
 ipapython/secrets/client.py           | 20 +++++++++++++-------
 ipaserver/install/custodiainstance.py | 14 +++++++++++---
 2 files changed, 24 insertions(+), 10 deletions(-)

diff --git a/ipapython/secrets/client.py b/ipapython/secrets/client.py
index 
5b671988ddc66eedd9ae1cd4ddec0e1308bc5a93..56ed6f7944c46393ed225cde1b5e0bb80fe6bef0
 100644
--- a/ipapython/secrets/client.py
+++ b/ipapython/secrets/client.py
@@ -41,16 +41,22 @@ class CustodiaClient(object):
 
         return iSecStore(config)
 
-    def __init__(self, client, server, realm, ldap_uri=None, auth_type=None):
-        self.client = client
-        self.creds = None
+    def __init__(
+            self, client_service, keyfile, keytab, server, realm,
+            ldap_uri=None, auth_type=None):
+        self.client_service = client_service
+        self.keytab = keytab
+
+        # Init creds immediately to make sure they are valid.  Creds
+        # can also be re-inited by _auth_header to avoid expiry.
+        #
+        self.creds = self.init_creds()
 
         self.service_name = gssapi.Name('HTTP@%s' % (server,),
                                         gssapi.NameType.hostbased_service)
         self.server = server
 
-        keyfile = os.path.join(paths.IPA_CUSTODIA_CONF_DIR, 'server.keys')
-        self.ikk = IPAKEMKeys({'server_keys': keyfile})
+        self.ikk = IPAKEMKeys({'server_keys': keyfile, 'ldap_uri': ldap_uri})
 
         self.kemcli = KEMClient(self._server_keys(server, realm),
                                 self._client_keys())
@@ -61,9 +67,9 @@ class CustodiaClient(object):
         requests.packages.urllib3.disable_warnings()
 
     def init_creds(self):
-        name = gssapi.Name('host@%s' % (self.client,),
+        name = gssapi.Name(self.client_service,
                            gssapi.NameType.hostbased_service)
-        store = {'client_keytab': paths.KRB5_KEYTAB,
+        store = {'client_keytab': self.keytab,
                  'ccache': 'MEMORY:Custodia_%s' % b64encode(os.urandom(8))}
         return gssapi.Credentials(name=name, store=store, usage='initiate')
 
diff --git a/ipaserver/install/custodiainstance.py 
b/ipaserver/install/custodiainstance.py
index 
d5c5bf738752ab4cf84f98285a37b820c80fa3be..be3f661caa4291bba91a90d5adaf17d63f903d30
 100644
--- a/ipaserver/install/custodiainstance.py
+++ b/ipaserver/install/custodiainstance.py
@@ -12,6 +12,7 @@ from ipaserver.install import ldapupdate
 from ipaserver.install import sysupgrade
 from base64 import b64encode, b64decode
 from jwcrypto.common import json_decode
+import functools
 import shutil
 import os
 import tempfile
@@ -28,6 +29,13 @@ class CustodiaInstance(SimpleServiceInstance):
         self.fqdn = host_name
         self.realm = realm
         self.ca_is_configured = ca_is_configured
+        self.__CustodiaClient = functools.partial(
+            CustodiaClient,
+            client_service='host@' + self.fqdn,
+            keyfile=self.server_keys,
+            keytab=paths.KRB5_KEYTAB,
+            realm=realm,
+        )
 
     def __config_file(self):
         template_file = os.path.basename(self.config_file) + '.template'
@@ -94,11 +102,11 @@ class CustodiaInstance(SimpleServiceInstance):
         updater.update([os.path.join(paths.UPDATES_DIR, '73-custodia.update')])
 
     def __import_ra_key(self):
-        cli = CustodiaClient(self.fqdn, self.master_host_name, self.realm)
+        cli = self.__CustodiaClient(server=self.master_host_name)
         cli.fetch_key('ra/ipaCert')
 
     def import_dm_password(self, master_host_name):
-        cli = CustodiaClient(self.fqdn, master_host_name, self.realm)
+        cli = self.__CustodiaClient(server=master_host_name)
         cli.fetch_key('dm/DMHash')
 
     def __get_keys(self, ca_host, cacerts_file, cacerts_pwd, data):
@@ -108,7 +116,7 @@ class CustodiaInstance(SimpleServiceInstance):
         prefix = data['prefix']
         certlist = data['list']
 
-        cli = CustodiaClient(self.fqdn, ca_host, self.realm)
+        cli = self.__CustodiaClient(server=ca_host)
 
         # Temporary nssdb
         tmpnssdir = tempfile.mkdtemp(dir=paths.TMP)
-- 
2.5.5

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