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

Reply via email to