[Freeipa-devel] [PATCH 149] IPA KDB: allow case in-sensitive realm in AS request

[Simo Sorce]

On Tue, 2015-07-28 at 14:26 +0300, Alexander Bokovoy wrote:
> On Tue, 28 Jul 2015, Simo Sorce wrote:
> >On Tue, 2015-07-28 at 12:15 +0200, Sumit Bose wrote:
> >> On Wed, Jul 22, 2015 at 09:41:51AM -0400, Simo Sorce wrote:
> >> > ----- Original Message -----
> >> > > From: "Sumit Bose" <sbose at redhat.com>
> >> > > To: "freeipa-devel" <freeipa-devel at redhat.com>
> >> > > Sent: Tuesday, July 21, 2015 7:41:14 AM
> >> > > Subject: [Freeipa-devel] [PATCH 149] IPA KDB: allow case in-sensitive realm	in AS request
> >> > >
> >> > > Hi,
> >> > >
> >> > > this patch is my suggestion to solve
> >> > > https://fedorahosted.org/freeipa/ticket/4844 .
> >> > >
> >> > > The original issue in the ticket has two part. One is a loop in libkrb5
> >> > > which is already fixed. The other is to handle canonicalization better.
> >> >
> >> > Sorry Sumit,
> >> > I see several issues with this patck.
> >> >
> >> > first of all you should really not change ipadb_get_principal(), that's the
> >> > wrong place to apply your logic.
> >> >
> >> > To support searching for the realm name case-insensitively all we should do
> >> > is to always forcibly upper case the realm name at the same time we build the
> >> > filter (in ipadb_fetch_principals(), if canonicalization was requested.
> >> > Because we will never store (code to prevent that should probably be dded with
> >> > this patch) a realm name that is not all caps.
> >> > Then the post search matches should be done straight within ipadb_find_principal().
> >> >
> >> > > The general way to allow canonicalization on a principal is to add the
> >> > > attributes 'krbcanonicalname'[1] and 'ipakrbprincipalalias' together
> >> > > with the objectclass 'ipaKrbPrincipal' to the user object.
> >> >
> >> > We have already a ticket open since long to remove krbprincipalalias, it was
> >> > a mistake to add it and any patch that depends on it will be nacked by me.
> >> > We need to use krbPrincipalName and krbCanonicalName.
> >> >
> >> > > Then the IPA
> >> > > KDB backend will use 'ipakrbprincipalalias' for case in-sensitive
> >> > > matches and  the principal from 'krbcanonicalname' will be the canonical
> >> > > principal used further on. The 'krbPrincipalName' is not suitable for
> >> > > either because it has caseExact* matching rules and is a multivalue
> >> > > attribute [2].
> >> >
> >> > Case-exact match is a problem only if we do not canonicalize names when storing
> >> > them, otherwise all you need to do is store a "search form" in krbPrincipalName
> >> > and always change searches to that form (forcibly upper case realm, forcibly
> >> > lowercase components) when canonicalization is requested.
> >> >
> >> > Additionally in the patch you are using stcasecmp(), that function is not
> >> > acceptable, look at ipadb_find_principal() and you'll see we use ulc_casecmp()
> >> > there.
> >> > Also modyfing the principal before searching is done wrong (you use strchr()
> >> > to find the @ sign, but you could find an @ in the components this way, you
> >> > should use strrchr() at the very least), and is dangerous if done outside of
> >> > the inner functions because then we never have a way to know the original
> >> > form should it be needed. In any case as said above realm should be forcibly
> >> > uppercase, given a flag in the escape function instead.
> >>
> >> Thank for for the review and the comments.
> >>
> >> I changed the patch as you suggested to upper-case the realm in the
> >> escape function if the flag is set.
> >>
> >> I didn't add any checks to make sure that the realm of newly added
> >> principal attributes is always upper case. Since the attributes can be
> >> added via various ways I think the check should happen on the DS level
> >
> >We should indeed intercept add/modify operations and see if they try to
> >set krbPrincipalName/krbCanonicalName and then validate the name.
> >Return unwilling to perform if the case of the realm is different (or
> >fix it on the fly, up for discussion) from the default case as
> >configured in the server.
> Will break trusts -- ipasam does add these principals for krbtgt/IPA at AD.

You misunderstood, we compare case-insenstively and adjust the case (or
simply always uppercase).
We do not refuse to add realm names that are completely different, I
know about the cross realm principals :)

> >> but I see this more in the context of full canonicalization fix covered
> >> by https://fedorahosted.org/freeipa/ticket/3864 . If you think this is a
> >> requirement for the patch attached I would suggest to drop
> >> https://fedorahosted.org/freeipa/ticket/4844 and solve it together with
> >> #3864.
> >
> >We should clsoe 4844 as fixed upstream (there *was* a bug in libkrb5).
> >I commented on #3864 about what we can do, and we can also avoid
> >changing the schema.
> Yep.
> 
> >So on the new patches, what does "unify" means ? I do not get what it
> >means (so probably it is a poor name), I guess you may want to call it
> >"canonicalization" ? (or even 'canon' to shorten it a bit).
> I have same question. I tried to understand why it is called unify and
> failed.
> 
> >I think the worst case for a utf8 string is more then length*2, probably
> >more like length*6, unless there is some guarantee around case changes
> >that I am not aware of, that said we could probably just allocate on the
> >stack a fixed size string of a KiB or so, the longest DNS name is 256
> >chars IIRC and a service name can't be that much longer, also usernames
> >can't be arbitrarily long. So 1/2 KiB should probably be fine for a full
> >principal name. (avoids a malloc too which is good).
> Yes, sounds good. A hostname label can be up to 63 characters and full
> domain name including dots would be 253 characters. At the same time, a
> a component of the principal may be of arbitrary length. From practical
> perspective it would probably be enough to go with a static buffer of
> 1/2 KiB for the quickest case and fall back to malloc() if the size is
> bigger than that one.

Just fail if the size is bigger, I do not see any legitimate case for >
1000 characters principal names for now.

Simo.