[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