Re: [Freeipa-devel] [PATCH] 0051 Allow CustodiaClient to be used by arbitrary principals

[Jan Cholasta]

On 6.6.2016 15:25, Fraser Tweedale wrote:

On Wed, Jun 01, 2016 at 02:49:06PM +1000, Fraser Tweedale wrote:

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.


It may be easy to follow, but what you are currently doing is basically 
emulating subclassing with functools.partial, which is both unidiomatic 
and less readable than actual subclassing.


But we are past the nitpicking phase, so I'm going to let that slide :-)




Self-NACK on 0051-4; rebased and updated patch attached.

Only one substantive change, in custodiainstance.py:

-client_service='host@' + self.fqdn,
+client_service='host@%s' % self.fqdn,

First version broke uninstall because CustodiaInstance gets
initialised with 'host_name = None' and '+' doesn't like None.


This will give "host@None" on uninstall, but I guess that's OK, since 
it's currently not used during uninstall.


ACK.

Pushed to master: f94ccca6761f7dbe3f99855d181fe2cec380d476

--
Jan Cholasta

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

Re: [Freeipa-devel] [PATCH] 0051 Allow CustodiaClient to be used by arbitrary principals

[Fraser Tweedale]

On Wed, Jun 01, 2016 at 02:49:06PM +1000, Fraser Tweedale wrote:
> 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.
> 
Self-NACK on 0051-4; rebased and updated patch attached.

Only one substantive change, in custodiainstance.py:

-client_service='host@' + self.fqdn,
+client_service='host@%s' % self.fqdn,

First version broke uninstall because CustodiaInstance gets
initialised with 'host_name = None' and '+' doesn't like None.
From a3387063510cf3b99fe3bfc0fe695671393f2f79 Mon Sep 17 00:00:00 2001
From: Fraser Tweedale 
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 

Re: [Freeipa-devel] [PATCH] 0051 Allow CustodiaClient to be used by arbitrary principals

[Fraser Tweedale]

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

Re: [Freeipa-devel] [PATCH] 0051 Allow CustodiaClient to be used by arbitrary principals

[Jan Cholasta]

On 25.4.2016 07:55, Jan Cholasta wrote:

Hi,

On 20.4.2016 08:22, Fraser Tweedale wrote:

On Mon, Apr 18, 2016 at 03:44:08PM -0400, Simo Sorce wrote:

On Thu, 2016-04-14 at 16:33 +1000, Fraser Tweedale wrote:

On Wed, Apr 13, 2016 at 11:15:50AM +1000, Fraser Tweedale wrote:

On Tue, Apr 12, 2016 at 09:31:30AM -0400, Simo Sorce wrote:

On Sat, 2016-04-09 at 10:11 +1000, Fraser Tweedale wrote:

On Fri, Apr 08, 2016 at 10:47:19AM -0400, Simo Sorce wrote:

On Sat, 2016-04-09 at 00:23 +1000, Fraser Tweedale wrote:

-name = gssapi.Name('host@%s' % (self.client,),

-   gssapi.NameType.hostbased_service)


If you remove this then on a serve that has nfs keys in the
keytab you
may end up acquiring the wrong credentials.
You need to pass down what credentials you want to use to
initialize the
cred store, we canot rely on ordering in the system keytab case.

Simo.


Thanks Simo; updated patch attached.


Except the ACI the rest looks good to me.
For ACI please add a separate patch that follows the naming scheme
for
subCA keys.


The ACI here targets the Custodia server public keys, so the client
can search and read them.  It should just read:

add:aci: (target = "ldap:///cn=*,cn=custodia,cn=ipa,cn=etc,$SUFFIX;)
(targetattr = "ipaPublicKey || ipaKeyUsage || memberPrincipal")
(version 3.0; acl "Anyone can search Custodia public keys";
allow(read, search, compare) userdn = "ldap:///all;;)

I don't mind putting the ACI in a separate patch, but it is
necessary to restrict read access on the public keys to only the
dogtag-ipa-custodia service principals.


Updated patches attached.  ACI was split into new patch and
simplified (removed ($dn) macro).


Ack on the custodia patch.
However do we really need to allow *anyone* to look up these keys ?
I know they are "public" keys, but still  ... I think I would prefer a
stricter ACI.


OK, I rescind the ACI patch (0052) and will include a more specific
ACI in a new version of the patch that adds the principal.

0051 is good to merge now :)


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.

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.


Actually these would better be class attributes, like:


class BaseCustodiaClient(object):
client_service_name = None
keyfile = None
keytab = None

# CustodiaClient code
...


class CustodiaClient(BaseCustodiaClient):
@property
def client_service_name(self):
return 'host@%s' % (self.client,)

keyfile = os.path.join(paths.IPA_CUSTODIA_CONF_DIR, 'server.keys')
keytab = paths.KRB5_KEYTAB


Later you can inherit DogtagCustodiaClient from BaseCustodiaClient and 
override the attributes as appropriate.




Why is init_creds() now called from __init__()? Why is it still called
from _auth_header()?

Why is ldap_uri now passed to IPAKEMKeys()?



Thanks for the review, Simo.
Fraser







--
Jan Cholasta

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

Re: [Freeipa-devel] [PATCH] 0051 Allow CustodiaClient to be used by arbitrary principals

[Jan Cholasta]

Hi,

On 20.4.2016 08:22, Fraser Tweedale wrote:

On Mon, Apr 18, 2016 at 03:44:08PM -0400, Simo Sorce wrote:

On Thu, 2016-04-14 at 16:33 +1000, Fraser Tweedale wrote:

On Wed, Apr 13, 2016 at 11:15:50AM +1000, Fraser Tweedale wrote:

On Tue, Apr 12, 2016 at 09:31:30AM -0400, Simo Sorce wrote:

On Sat, 2016-04-09 at 10:11 +1000, Fraser Tweedale wrote:

On Fri, Apr 08, 2016 at 10:47:19AM -0400, Simo Sorce wrote:

On Sat, 2016-04-09 at 00:23 +1000, Fraser Tweedale wrote:

-name = gssapi.Name('host@%s' % (self.client,),

-   gssapi.NameType.hostbased_service)


If you remove this then on a serve that has nfs keys in the keytab you
may end up acquiring the wrong credentials.
You need to pass down what credentials you want to use to initialize the
cred store, we canot rely on ordering in the system keytab case.

Simo.


Thanks Simo; updated patch attached.


Except the ACI the rest looks good to me.
For ACI please add a separate patch that follows the naming scheme for
subCA keys.


The ACI here targets the Custodia server public keys, so the client
can search and read them.  It should just read:

add:aci: (target = "ldap:///cn=*,cn=custodia,cn=ipa,cn=etc,$SUFFIX;)
(targetattr = "ipaPublicKey || ipaKeyUsage || memberPrincipal")
(version 3.0; acl "Anyone can search Custodia public keys";
allow(read, search, compare) userdn = "ldap:///all;;)

I don't mind putting the ACI in a separate patch, but it is
necessary to restrict read access on the public keys to only the
dogtag-ipa-custodia service principals.


Updated patches attached.  ACI was split into new patch and
simplified (removed ($dn) macro).


Ack on the custodia patch.
However do we really need to allow *anyone* to look up these keys ?
I know they are "public" keys, but still  ... I think I would prefer a
stricter ACI.


OK, I rescind the ACI patch (0052) and will include a more specific
ACI in a new version of the patch that adds the principal.

0051 is good to merge now :)


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.


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.


Why is init_creds() now called from __init__()? Why is it still called 
from _auth_header()?


Why is ldap_uri now passed to IPAKEMKeys()?



Thanks for the review, Simo.
Fraser




--
Jan Cholasta

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

Re: [Freeipa-devel] [PATCH] 0051 Allow CustodiaClient to be used by arbitrary principals

[Fraser Tweedale]

On Mon, Apr 18, 2016 at 03:44:08PM -0400, Simo Sorce wrote:
> On Thu, 2016-04-14 at 16:33 +1000, Fraser Tweedale wrote:
> > On Wed, Apr 13, 2016 at 11:15:50AM +1000, Fraser Tweedale wrote:
> > > On Tue, Apr 12, 2016 at 09:31:30AM -0400, Simo Sorce wrote:
> > > > On Sat, 2016-04-09 at 10:11 +1000, Fraser Tweedale wrote:
> > > > > On Fri, Apr 08, 2016 at 10:47:19AM -0400, Simo Sorce wrote:
> > > > > > On Sat, 2016-04-09 at 00:23 +1000, Fraser Tweedale wrote:
> > > > > > > -name = gssapi.Name('host@%s' % (self.client,),
> > > > > > > 
> > > > > > > -   gssapi.NameType.hostbased_service)
> > > > > > 
> > > > > > If you remove this then on a serve that has nfs keys in the keytab 
> > > > > > you
> > > > > > may end up acquiring the wrong credentials.
> > > > > > You need to pass down what credentials you want to use to 
> > > > > > initialize the
> > > > > > cred store, we canot rely on ordering in the system keytab case.
> > > > > > 
> > > > > > Simo.
> > > > > > 
> > > > > Thanks Simo; updated patch attached.
> > > > 
> > > > Except the ACI the rest looks good to me.
> > > > For ACI please add a separate patch that follows the naming scheme for
> > > > subCA keys.
> > > > 
> > > The ACI here targets the Custodia server public keys, so the client
> > > can search and read them.  It should just read:
> > > 
> > > add:aci: (target = "ldap:///cn=*,cn=custodia,cn=ipa,cn=etc,$SUFFIX;)
> > > (targetattr = "ipaPublicKey || ipaKeyUsage || memberPrincipal")
> > > (version 3.0; acl "Anyone can search Custodia public keys";
> > > allow(read, search, compare) userdn = "ldap:///all;;)
> > > 
> > > I don't mind putting the ACI in a separate patch, but it is
> > > necessary to restrict read access on the public keys to only the
> > > dogtag-ipa-custodia service principals.
> > > 
> > Updated patches attached.  ACI was split into new patch and
> > simplified (removed ($dn) macro).
> 
> Ack on the custodia patch.
> However do we really need to allow *anyone* to look up these keys ?
> I know they are "public" keys, but still  ... I think I would prefer a
> stricter ACI.
> 
OK, I rescind the ACI patch (0052) and will include a more specific
ACI in a new version of the patch that adds the principal.

0051 is good to merge now :)

Thanks for the review, Simo.
Fraser

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

Re: [Freeipa-devel] [PATCH] 0051 Allow CustodiaClient to be used by arbitrary principals

[Simo Sorce]

On Thu, 2016-04-14 at 16:33 +1000, Fraser Tweedale wrote:
> On Wed, Apr 13, 2016 at 11:15:50AM +1000, Fraser Tweedale wrote:
> > On Tue, Apr 12, 2016 at 09:31:30AM -0400, Simo Sorce wrote:
> > > On Sat, 2016-04-09 at 10:11 +1000, Fraser Tweedale wrote:
> > > > On Fri, Apr 08, 2016 at 10:47:19AM -0400, Simo Sorce wrote:
> > > > > On Sat, 2016-04-09 at 00:23 +1000, Fraser Tweedale wrote:
> > > > > > -name = gssapi.Name('host@%s' % (self.client,),
> > > > > > 
> > > > > > -   gssapi.NameType.hostbased_service)
> > > > > 
> > > > > If you remove this then on a serve that has nfs keys in the keytab you
> > > > > may end up acquiring the wrong credentials.
> > > > > You need to pass down what credentials you want to use to initialize 
> > > > > the
> > > > > cred store, we canot rely on ordering in the system keytab case.
> > > > > 
> > > > > Simo.
> > > > > 
> > > > Thanks Simo; updated patch attached.
> > > 
> > > Except the ACI the rest looks good to me.
> > > For ACI please add a separate patch that follows the naming scheme for
> > > subCA keys.
> > > 
> > The ACI here targets the Custodia server public keys, so the client
> > can search and read them.  It should just read:
> > 
> > add:aci: (target = "ldap:///cn=*,cn=custodia,cn=ipa,cn=etc,$SUFFIX;)
> > (targetattr = "ipaPublicKey || ipaKeyUsage || memberPrincipal")
> > (version 3.0; acl "Anyone can search Custodia public keys";
> > allow(read, search, compare) userdn = "ldap:///all;;)
> > 
> > I don't mind putting the ACI in a separate patch, but it is
> > necessary to restrict read access on the public keys to only the
> > dogtag-ipa-custodia service principals.
> > 
> Updated patches attached.  ACI was split into new patch and
> simplified (removed ($dn) macro).

Ack on the custodia patch.
However do we really need to allow *anyone* to look up these keys ?
I know they are "public" keys, but still  ... I think I would prefer a
stricter ACI.

Simo.

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

Re: [Freeipa-devel] [PATCH] 0051 Allow CustodiaClient to be used by arbitrary principals

[Fraser Tweedale]

On Wed, Apr 13, 2016 at 11:15:50AM +1000, Fraser Tweedale wrote:
> On Tue, Apr 12, 2016 at 09:31:30AM -0400, Simo Sorce wrote:
> > On Sat, 2016-04-09 at 10:11 +1000, Fraser Tweedale wrote:
> > > On Fri, Apr 08, 2016 at 10:47:19AM -0400, Simo Sorce wrote:
> > > > On Sat, 2016-04-09 at 00:23 +1000, Fraser Tweedale wrote:
> > > > > -name = gssapi.Name('host@%s' % (self.client,),
> > > > > 
> > > > > -   gssapi.NameType.hostbased_service)
> > > > 
> > > > If you remove this then on a serve that has nfs keys in the keytab you
> > > > may end up acquiring the wrong credentials.
> > > > You need to pass down what credentials you want to use to initialize the
> > > > cred store, we canot rely on ordering in the system keytab case.
> > > > 
> > > > Simo.
> > > > 
> > > Thanks Simo; updated patch attached.
> > 
> > Except the ACI the rest looks good to me.
> > For ACI please add a separate patch that follows the naming scheme for
> > subCA keys.
> > 
> The ACI here targets the Custodia server public keys, so the client
> can search and read them.  It should just read:
> 
> add:aci: (target = "ldap:///cn=*,cn=custodia,cn=ipa,cn=etc,$SUFFIX;)
> (targetattr = "ipaPublicKey || ipaKeyUsage || memberPrincipal")
> (version 3.0; acl "Anyone can search Custodia public keys";
> allow(read, search, compare) userdn = "ldap:///all;;)
> 
> I don't mind putting the ACI in a separate patch, but it is
> necessary to restrict read access on the public keys to only the
> dogtag-ipa-custodia service principals.
> 
Updated patches attached.  ACI was split into new patch and
simplified (removed ($dn) macro).

Cheers,
Fraser
From 1f1193b3a4e786c63bde4fa0abe3640c16481633 Mon Sep 17 00:00:00 2001
From: Fraser Tweedale 
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 allow specifying the keytab and Custodia
keyfile to use.  Avoid hard-coding the service name to find in the
keytab.

Part of: https://fedorahosted.org/freeipa/ticket/4559
---
 ipapython/secrets/client.py | 17 ++---
 1 file changed, 10 insertions(+), 7 deletions(-)

diff --git a/ipapython/secrets/client.py b/ipapython/secrets/client.py
index 
5b671988ddc66eedd9ae1cd4ddec0e1308bc5a93..a15057ae67c377a782db3642d14384e0bf11b5a2
 100644
--- a/ipapython/secrets/client.py
+++ b/ipapython/secrets/client.py
@@ -41,16 +41,19 @@ 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, server, realm, ldap_uri=None, auth_type=None,
+client_servicename='host', keyfile=None, keytab=None):
+self.client_service = '%s@%s' % (client_servicename, client)
+self.keytab = keytab or paths.KRB5_KEYTAB
+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})
+if keyfile is None:
+keyfile = os.path.join(paths.IPA_CUSTODIA_CONF_DIR, 'server.keys')
+self.ikk = IPAKEMKeys({'server_keys': keyfile, 'ldap_uri': ldap_uri})
 
 self.kemcli = KEMClient(self._server_keys(server, realm),
 self._client_keys())
@@ -61,9 +64,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')
 
-- 
2.5.5

From 234e6334dafb27224b89fed86968df3016b27474 Mon Sep 17 00:00:00 2001
From: Fraser Tweedale 
Date: Wed, 13 Apr 2016 14:51:16 +1000
Subject: [PATCH] Allow all principals to read Custodia keys

Add an ACI to allow all authenticated principals to read and search
for Custodia server public keys.

Part of: https://fedorahosted.org/freeipa/ticket/4559
---
 install/updates/20-aci.update | 3 

Re: [Freeipa-devel] [PATCH] 0051 Allow CustodiaClient to be used by arbitrary principals

[Fraser Tweedale]

On Tue, Apr 12, 2016 at 09:31:30AM -0400, Simo Sorce wrote:
> On Sat, 2016-04-09 at 10:11 +1000, Fraser Tweedale wrote:
> > On Fri, Apr 08, 2016 at 10:47:19AM -0400, Simo Sorce wrote:
> > > On Sat, 2016-04-09 at 00:23 +1000, Fraser Tweedale wrote:
> > > > -name = gssapi.Name('host@%s' % (self.client,),
> > > > 
> > > > -   gssapi.NameType.hostbased_service)
> > > 
> > > If you remove this then on a serve that has nfs keys in the keytab you
> > > may end up acquiring the wrong credentials.
> > > You need to pass down what credentials you want to use to initialize the
> > > cred store, we canot rely on ordering in the system keytab case.
> > > 
> > > Simo.
> > > 
> > Thanks Simo; updated patch attached.
> 
> Except the ACI the rest looks good to me.
> For ACI please add a separate patch that follows the naming scheme for
> subCA keys.
> 
The ACI here targets the Custodia server public keys, so the client
can search and read them.  It should just read:

add:aci: (target = "ldap:///cn=*,cn=custodia,cn=ipa,cn=etc,$SUFFIX;)
(targetattr = "ipaPublicKey || ipaKeyUsage || memberPrincipal")
(version 3.0; acl "Anyone can search Custodia public keys";
allow(read, search, compare) userdn = "ldap:///all;;)

I don't mind putting the ACI in a separate patch, but it is
necessary to restrict read access on the public keys to only the
dogtag-ipa-custodia service principals.

Cheers,
Fraser

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

Re: [Freeipa-devel] [PATCH] 0051 Allow CustodiaClient to be used by arbitrary principals

[Simo Sorce]

On Sat, 2016-04-09 at 10:11 +1000, Fraser Tweedale wrote:
> On Fri, Apr 08, 2016 at 10:47:19AM -0400, Simo Sorce wrote:
> > On Sat, 2016-04-09 at 00:23 +1000, Fraser Tweedale wrote:
> > > -name = gssapi.Name('host@%s' % (self.client,),
> > > 
> > > -   gssapi.NameType.hostbased_service)
> > 
> > If you remove this then on a serve that has nfs keys in the keytab you
> > may end up acquiring the wrong credentials.
> > You need to pass down what credentials you want to use to initialize the
> > cred store, we canot rely on ordering in the system keytab case.
> > 
> > Simo.
> > 
> Thanks Simo; updated patch attached.

Except the ACI the rest looks good to me.
For ACI please add a separate patch that follows the naming scheme for
subCA keys.

Simo.

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

Re: [Freeipa-devel] [PATCH] 0051 Allow CustodiaClient to be used by arbitrary principals

[Simo Sorce]

On Sat, 2016-04-09 at 00:23 +1000, Fraser Tweedale wrote:
> -name = gssapi.Name('host@%s' % (self.client,),
> 
> -   gssapi.NameType.hostbased_service)

If you remove this then on a serve that has nfs keys in the keytab you
may end up acquiring the wrong credentials.
You need to pass down what credentials you want to use to initialize the
cred store, we canot rely on ordering in the system keytab case.

Simo.

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