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

Thu Mar 3 12:37:39 UTC 2016

On 08/10/2015 11:37 PM, Simo Sorce wrote:
> On Mon, 2015-08-10 at 23:15 +0300, Alexander Bokovoy wrote:
>> On Mon, 10 Aug 2015, Alexander Bokovoy wrote:
>>> On Sun, 09 Aug 2015, Simo Sorce wrote:
>>>> On Fri, 2015-08-07 at 23:56 +0300, Alexander Bokovoy wrote:
>>>>> 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?
>>>>>
>>>>
>>>> LGTM.
>>> Hold on. I think I've found a bug -- when krbPrincipalName values match
>>> insensitively but krbCanonicalName value is missing, we do not set
>>> principal to the matched value. This breaks canonicalization for case
>>> when there is only one krbPrincipalName as you don't need to have
>>> krbCanonicalName in this case.
>>>
>>> I have a prototype which still misses checks.
>> ... and I think we miss checks in few other places. I'm getting
>> canonicalization working randomly -- sometimes one or two times in a row
>> I get 'Client principal is not found' for canonicalization case:
>>
>> Aug 10 20:01:21 m1.example.com krb5kdc[18758](Error): searched for
>> admin at example.com, found admin at EXAMPLE.COM, result is 1, index is 0,
>> next val is (nil)
>>
>> Aug 10 20:01:21 m1.example.com krb5kdc[18758](Error): searched for
>> krbtgt/example.com at example.com, found krbtgt/EXAMPLE.COM at EXAMPLE.COM,
>> result is 1, index is 0, next val is (nil)
>>
>> Aug 10 20:01:21 m1.example.com krb5kdc[18758](info): AS_REQ (6 etypes
>> {18 17 16 23 25 26}) 192.168.122.99: NEEDED_PREAUTH: admin at example.com
>> for krbtgt/example.com at example.com, Additional pre-authentication
>> required
>>
>> Aug 10 20:01:24 m1.example.com krb5kdc[18758](Error): searched for
>> admin at example.com, found admin at EXAMPLE.COM, result is 1, index is 0,
>> next val is (nil)
>>
>> Aug 10 20:01:24 m1.example.com krb5kdc[18758](Error): searched for
>> krbtgt/example.com at example.com, found krbtgt/EXAMPLE.COM at EXAMPLE.COM,
>> result is 1, index is 0, next val is (nil)
>>
>> Aug 10 20:01:24 m1.example.com krb5kdc[18758](info): AS_REQ (6 etypes
>> {18 17 16 23 25 26}) 192.168.122.99: ISSUE: authtime 1439236884, etypes
>> {rep=18 tkt=18 ses=18}, admin at example.com for
>> krbtgt/example.com at example.com
>>
>> Aug 10 20:01:58 m1.example.com krb5kdc[18758](info): AS_REQ (6 etypes
>> {18 17 16 23 25 26}) 192.168.122.99: CLIENT_NOT_FOUND: admin at example.com
>> for krbtgt/example.com at example.com, Client not found in Kerberos
>> database
>>
>> These are logs with debugging I've added.
>> --
>> / Alexander Bokovoy
>>
>
> After looking carefully at this with Alexander I think there are too
> many things to fix and check, and given the looming deadline for Fedora
> we should just postpone. These patches are not critical for us but it
> would be bad if they'd go in and not work as expected.
>
> I can take a better look at them when back from flock.
>
> Simo.
>

Resurrecting this thread so the patches can get into FreeIPA 4.4.
-- 
Petr Vobornik