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

Fri Aug 7 20:56:19 UTC 2015

On Tue, 28 Jul 2015, Sumit Bose wrote:
>On Tue, Jul 28, 2015 at 01:42:29PM +0200, Sumit Bose wrote:
>> On Tue, Jul 28, 2015 at 02:26:34PM +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.
>> >
>> > >>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 didn't want to use 'canonical' because the result will not be the
>> canonical name in the general case but only a name we use for searching.
>> I was thinking about 'normalized' bit this has a special meaning with
>> unicode. So I came up with 'unify'. But if you prefer 'canon' I can
>> change it.
>>
>> >
>> > >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.
>>
>> ok, I will change this.
>
>new version with changed name and 1/2 KiB buffer attached. No changes to
>the 2nd patch.
Thanks.

Patches look good to me. I, perhaps, would have added 
  char *canon_princ = NULL;

in the definition of canon_princ but as you never access it in case
asprintf() failed, that's fine.

Simo?

-- 
/ Alexander Bokovoy