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" <sb...@redhat.com>
> >>>>> > >>> > To: "freeipa-devel" <freeipa-devel@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@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
> ad...@example.com, found ad...@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....@example.com, found krbtgt/example....@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: ad...@example.com
> for krbtgt/example....@example.com, Additional pre-authentication
> required
> 
> Aug 10 20:01:24 m1.example.com krb5kdc[18758](Error): searched for
> ad...@example.com, found ad...@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....@example.com, found krbtgt/example....@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}, ad...@example.com for
> krbtgt/example....@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: ad...@example.com
> for krbtgt/example....@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.

-- 
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

Reply via email to