[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