[Freeipa-devel] [PATCH] 0015-16 Allow multiple krbprincipalnames + test

[Martin Kosek]

On 09/22/2014 04:16 PM, Simo Sorce wrote:
> On Mon, 22 Sep 2014 15:36:01 +0200
> David Kupka <dkupka at redhat.com> wrote:
> 
>> On 09/18/2014 09:42 PM, Martin Kosek wrote:
>>> On 09/18/2014 09:11 PM, Simo Sorce wrote:
>>>> On Thu, 18 Sep 2014 14:57:45 -0400
>>>> Rob Crittenden <rcritten at redhat.com> wrote:
>>>>
>>>>> Martin Kosek wrote:
>>>>>> On 09/18/2014 04:06 PM, David Kupka wrote:
>>>>>>> On 09/18/2014 03:44 PM, Rob Crittenden wrote:
>>>>>>>> David Kupka wrote:
>>>>>>>>> https://fedorahosted.org/freeipa/ticket/4421
>>>>>>>>
>>>>>>>> You are removing an ACI in this patch. It is always possible it
>>>>>>>> is no longer needed. Did you test all the client enrollment
>>>>>>>> scenarios?
>>>>>>>>
>>>>>>>> rob
>>>>>>>>
>>>>>>>
>>>>>>> As far as I'm aware I'm not removing any ACI. I'm modifying ACI
>>>>>>> so it is possible to add krbPrincipalName to host even when
>>>>>>> there is already one (or more). And adding one ACI to allow
>>>>>>> writing krbCanonicalName to host. But I'm still not really
>>>>>>> familiar with ACI so please correct me if I'm wrong.
>>>>>>>
>>>>>>
>>>>>> What refers to is probably the update in ACI.txt - the ACI
>>>>>> alternative to API.txt. David updated an ACI, not removed it.
>>>>>>
>>>>>> On that note, what is the reason for this permission change:
>>>>>>
>>>>>> -            'ipapermtargetfilter': [
>>>>>> -                '(objectclass=ipahost)',
>>>>>> -                '(!(krbprincipalname=*))',
>>>>>> -            ],
>>>>>>
>>>>>> ?
>>>>>
>>>>> Yes, this is exactly the change I was referring to. Permission
>>>>> changes within a plugin now translate automatically to ACI
>>>>> changes. Sorry I wasn't clearer.
>>>>>
>>>>> This ACI gets replaced with a much simpler one and I'm not 100%
>>>>> sure it will work with all enrollments:
>>>>>
>>>>> -aci: (targetattr = "krbprincipalname")(targetfilter =
>>>>> "(&(!(krbprincipalname=*))(objectclass=ipahost))")(version 3.0;acl
>>>>> "permission:System: Add krbPrincipalName to a Host";allow (write)
>>>>> groupdn = "ldap:///cn=System: Add krbPrincipalName to a
>>>>> Host,cn=permissions,cn=pbac,dc=ipa,dc=example";)
>>>>>
>>>>> +aci: (targetattr = "krbprincipalname")(targetfilter =
>>>>> "(objectclass=ipahost)")(version 3.0;acl "permission:System: Add
>>>>> krbPrincipalName to a Host";allow (write) groupdn =
>>>>> "ldap:///cn=System: Add krbPrincipalName to a
>>>>> Host,cn=permissions,cn=pbac,dc=ipa,dc=example";)
>>>>>
>>>>> The first one restricts writing the attribute only if it isn't
>>>>> already set. The second lets it be changed unconditionally.
>>>>
>>>> Yeah this is wrong indeed, the point of the ACI is to allow
>>>> setting the principal only when it is not already set, which is
>>>> the OTP enrollment case. But if krbprincipal is set then this
>>>> specific permission should not grant rights to change it.
>>>>
>>>> At least this was my understanding.
>>>>
>>>> Simo.
>>>
>>> Right. It seems to me we should add keep this permission intact and
>>> add a new permission allowing adding krbPrincipalName aliases. This
>>> would allow writing both krbPrincipalName and krbCanonicalName.
>>>
>>> Martin
>>>
>>
>> Thank you all for explanation and help. I rewrote the patch so it
>> should work as requested now. Also I added tests to reassure the
>> behavior is correct.
> 
> 
> Sorry for not catching this earlier, but should we also add new IPA
> MODRDN configuration ?
> We currently change krbPrincipalName if the user uid changes.
> 
> However perhaps what we should do is instead to allows aliases only for
> service/host principals but not for ipa users, at least for now.

Right. But this patch does not allow aliases for users, so we do not do any
changes on that side.

> Should we also change permissions so that host/service entries
> *cannot* be renamed ? Otherwise we'd need to add again IAP MODRDN
> configuration so that if a service/host krbprincipalname is changed
> then krbcanonicalname is too.

Hmm, I think only admin is allowed to rename hosts right now. But you have a
good point about the renames though.

> Last but not least, and here I regret we may have a BIG issue, I just
> realize we use krbprincipalname in the Services DN, if we add
> additional values there does it mean we end up with a multivalued RDN ?

Oh, this one is a major issue then. It would mean that, yes. But is that a
problem? I think DS is OK with just one attribute from multi-valued attribute
to be in the DN, right? Though it may become awkward to manipulate in the
framework.

Another issue, are we handling the services correctly? They contain check for
the host existence & given they krbPrincipalName they are also bound to one of
the aliases actually. What if somebody creates a service for
alias.ipa.server.test and then deletes that alias? We will need to address it too.

> That may be a problem, in such case should we rename services to use
> krbcanonicalname as the rnd instead ? We can do this in a compatible
> manner I think by renaming on the fly old entries if the still use
> krbprincipalname and changing our code to start always setting
> krbcanonicalname on new entries and set both canmonical and principal
> name on every new entry.
> 
> Opinions ?

I am a bit afraid of renaming all services, there may be a *lot* of them.
However, with new ACIs there should not be a problem having objects with
different RDNs.

I am thinking host plugin could change RDN to krbCanonicalName when a second
krbPrincipalName is added.

Hmm, I am thinking there may be too many issues related to this patch to make
us consider deferring for now...

Martin