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

[Simo Sorce]

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.

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

> I added a second patch which makes the unit test a bit more robust if
> the krb5.conf on the system running the tests is broken.

Ok.

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

On the tests, realms can't use unicode afaik.

The change to use an empty profile is probably ok.

HTH,
Simo.