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

[Simo Sorce]

On Mon, 22 Sep 2014 17:25:27 +0200
Martin Kosek <mkosek at redhat.com> wrote:

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

This is what I was proposing, renaming existing ones only if they are
"touched".

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

The only issue seem to be the change to krbCanonicalNAme, but of course
this will take time, this patch should not hold a release, though we
are getting closer, we shouldn't just defer it forever.

Simo.

-- 
Simo Sorce * Red Hat, Inc * New York