Re: [Freeipa-devel] [PATCH 149] IPA KDB: allow case in-sensitive realm in AS request
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" To: "freeipa-devel" 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
Re: [Freeipa-devel] [PATCH 149] IPA KDB: allow case in-sensitive realm in AS request
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" > > > >>> > To: "freeipa-devel" > > > >>> > 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
Re: [Freeipa-devel] [PATCH 149] IPA KDB: allow case in-sensitive realm in AS request
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" > >>> > To: "freeipa-devel" > >>> > 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
Re: [Freeipa-devel] [PATCH 149] IPA KDB: allow case in-sensitive realm in AS request
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" >> > >>> > To: "freeipa-devel" >> > >>> > 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. >> > >
Re: [Freeipa-devel] [PATCH 149] IPA KDB: allow case in-sensitive realm in AS request
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" > >> > >>> > To: "freeipa-devel" > >> > >>> > Sent: Tuesday, July 21, 2015 7:41:14 AM > >> > >>> > Subject: [Freeipa-devel] [PATCH 149] IPA KDB: allow case > >> > >>> > in-sensitive realmin 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://fedo
Re: [Freeipa-devel] [PATCH 149] IPA KDB: allow case in-sensitive realm in AS request
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" > >>> > To: "freeipa-devel" > >>> > 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
Re: [Freeipa-devel] [PATCH 149] IPA KDB: allow case in-sensitive realm in AS request
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" > > >>> > To: "freeipa-devel" > > >>> > 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
Re: [Freeipa-devel] [PATCH 149] IPA KDB: allow case in-sensitive realm in AS request
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" > >> > > To: "freeipa-devel" > >> > > Sent: Tuesday, July 21, 2015 7:41:14 AM > >> > > Subject: [Freeipa-devel] [PATCH 149] IPA KDB: allow case in-sensitive > >> > > realmin 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. 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 > >mean
Re: [Freeipa-devel] [PATCH 149] IPA KDB: allow case in-sensitive realm in AS request
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" > >>> > To: "freeipa-devel" > >>> > 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
Re: [Freeipa-devel] [PATCH 149] IPA KDB: allow case in-sensitive realm in AS request
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" > > To: "freeipa-devel" > > 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 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 wou
Re: [Freeipa-devel] [PATCH 149] IPA KDB: allow case in-sensitive realm in AS request
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" > > > To: "freeipa-devel" > > > 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 t
Re: [Freeipa-devel] [PATCH 149] IPA KDB: allow case in-sensitive realm in AS request
On Wed, Jul 22, 2015 at 09:41:51AM -0400, Simo Sorce wrote: > - Original Message - > > From: "Sumit Bose" > > To: "freeipa-devel" > > 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 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. 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. bye, Sumit > > > What I got from the comments in the ticket and the related bugzilla > > ticket is that it should be possible to get a TGT for a user even if the > > realm is given in lower-case if canonicalization is enabled. Please note > > that the client can only send such request because we have > > 'dns_lookup_kdc = true' in krb.conf and DNS is case in-sensitive. If you > > set 'dns_lookup_kdc = false' the client will fail immediately without > > sending a request at all, because it is not able to find a KDC for the > > lower-case realm. > > > > On the server-side the request is processed because of > > http://k5wiki.kerberos.org/wiki/Projects/Aliases which made parts of > > processing case in-sensitive. > > > > With the attached patch a second lookup is done if the lookup with the > > original input returned no result, canonicalization is > > enabled and the realm from the original input matches the IPA realm case > > in-sensitive. For the second lookup the realm is replace with the IPA > > realm. This approach adds a bit redundant code but does not add extra > > processing requests which would be successful before. > > > > Without the patch > > kinitipauser@IPA.REALM -> success > > kinit -C ipauser@IPA.REALM -> success > > kinitipauser@ipa.realm -> failure > > kinit -C ipauser@ipa.realm -> failure > > > > With the
Re: [Freeipa-devel] [PATCH 149] IPA KDB: allow case in-sensitive realm in AS request
- Original Message - > From: "Sumit Bose" > To: "freeipa-devel" > 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. > What I got from the comments in the ticket and the related bugzilla > ticket is that it should be possible to get a TGT for a user even if the > realm is given in lower-case if canonicalization is enabled. Please note > that the client can only send such request because we have > 'dns_lookup_kdc = true' in krb.conf and DNS is case in-sensitive. If you > set 'dns_lookup_kdc = false' the client will fail immediately without > sending a request at all, because it is not able to find a KDC for the > lower-case realm. > > On the server-side the request is processed because of > http://k5wiki.kerberos.org/wiki/Projects/Aliases which made parts of > processing case in-sensitive. > > With the attached patch a second lookup is done if the lookup with the > original input returned no result, canonicalization is > enabled and the realm from the original input matches the IPA realm case > in-sensitive. For the second lookup the realm is replace with the IPA > realm. This approach adds a bit redundant code but does not add extra > processing requests which would be successful before. > > Without the patch > kinitipauser@IPA.REALM -> success > kinit -C ipauser@IPA.REALM -> success > kinitipauser@ipa.realm -> failure > kinit -C ipauser@ipa.realm -> failure > > With the patch > kinitipauser@IPA.REALM -> success > kinit -C ipauser@IPA.REALM -> success > kinitipauser@ipa.realm -> success > kinit -C ipauser@ipa.realm -> success > > where 'ipa.realm' can be replace by mixed case version like 'iPa.ReAlM' > as well. > > bye, > Sumit > > [1] I was not able to add 'krbcanonicalname' as admin user because of an > ACI denial. I wonder if this is expected or if the ACI rules should be > extended here? Yes, we need to fix this, it's a bug that admins can't set the canonical name. > [2] We might to skip the requirement that 'krbcanonicalname' must exists > if 'ipaKrbPrincipal' only has a single value but canonicalization will > fail immediately if someone adds a second value so I guess it would be > more safe to keep it as it is. If someone adds a second value we must have code to set krbCanonicalName anyway or we will not know anymore what is the canonical name. So this also needs fixing in this patchset probably, by adding checks to the add/modify principal functions
Re: [Freeipa-devel] [PATCH 149] IPA KDB: allow case in-sensitive realm in AS request
On Tue, 21 Jul 2015, Sumit Bose wrote:
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.
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. 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].
Yes. Right now we only have alias support built in into services, not
users.
What I got from the comments in the ticket and the related bugzilla
ticket is that it should be possible to get a TGT for a user even if the
realm is given in lower-case if canonicalization is enabled. Please note
that the client can only send such request because we have
'dns_lookup_kdc = true' in krb.conf and DNS is case in-sensitive. If you
set 'dns_lookup_kdc = false' the client will fail immediately without
sending a request at all, because it is not able to find a KDC for the
lower-case realm.
On the server-side the request is processed because of
http://k5wiki.kerberos.org/wiki/Projects/Aliases which made parts of
processing case in-sensitive.
With the attached patch a second lookup is done if the lookup with the
original input returned no result, canonicalization is
enabled and the realm from the original input matches the IPA realm case
in-sensitive. For the second lookup the realm is replace with the IPA
realm. This approach adds a bit redundant code but does not add extra
processing requests which would be successful before.
Without the patch
kinitipauser@IPA.REALM -> success
kinit -C ipauser@IPA.REALM -> success
kinitipauser@ipa.realm -> failure
kinit -C ipauser@ipa.realm -> failure
With the patch
kinitipauser@IPA.REALM -> success
kinit -C ipauser@IPA.REALM -> success
kinitipauser@ipa.realm -> success
Failure here (as you wrote in the other email).
kinit -C ipauser@ipa.realm -> success
where 'ipa.realm' can be replace by mixed case version like 'iPa.ReAlM'
as well.
bye,
Sumit
[1] I was not able to add 'krbcanonicalname' as admin user because of an
ACI denial. I wonder if this is expected or if the ACI rules should be
extended here?
ACIs need to be extended to allow setting the attribute, yes.
[2] We might to skip the requirement that 'krbcanonicalname' must exists
if 'ipaKrbPrincipal' only has a single value but canonicalization will
fail immediately if someone adds a second value so I guess it would be
more safe to keep it as it is.
yep.
From 39744160f1779cf8e5ff00531b432c88fc53200b Mon Sep 17 00:00:00 2001
From: Sumit Bose
Date: Tue, 21 Jul 2015 12:12:56 +0200
Subject: [PATCH] IPA KDB: allow case in-sensitive realm in AS request
If the canonicalization flag is set the realm of the client principal in
an AS request (kinit) may only match case in-sensitive.
Resolves https://fedorahosted.org/freeipa/ticket/4844
---
daemons/ipa-kdb/ipa_kdb_principals.c | 31 ++-
1 file changed, 30 insertions(+), 1 deletion(-)
diff --git a/daemons/ipa-kdb/ipa_kdb_principals.c
b/daemons/ipa-kdb/ipa_kdb_principals.c
index
b3f8b1ad7784f55f55b4d6edd05f778a9389de27..344d44fb9b20f74615993a561b8b39f34f57cf25
100644
--- a/daemons/ipa-kdb/ipa_kdb_principals.c
+++ b/daemons/ipa-kdb/ipa_kdb_principals.c
@@ -1026,6 +1026,8 @@ krb5_error_code ipadb_get_principal(krb5_context kcontext,
LDAPMessage *res = NULL;
LDAPMessage *lentry;
uint32_t pol;
+size_t realm_len;
+char *p;
ipactx = ipadb_get_context(kcontext);
if (!ipactx) {
@@ -1044,7 +1046,34 @@ krb5_error_code ipadb_get_principal(krb5_context
kcontext,
kerr = ipadb_find_principal(kcontext, flags, res, &principal, &lentry);
if (kerr != 0) {
-goto done;
+realm_len = strlen(ipactx->realm);
+
+/* If canonicalization is enabled and the realm only differs in case
+ * from the IPA realm retry with the correct case. */
+if (kerr == KRB5_KDB_NOENTRY
+&& (flags & KRB5_KDB_FLAG_ALIAS_OK) != 0
+&& krb5_princ_realm(kcontext, search_for)->length == realm_len
+&& strncasecmp(krb5_princ_realm(kcontext,search_for)->data,
+ ipactx->realm, realm_len) == 0) {
It would probably be better to use ulc_casecmp() here like in
ipadb_get_principal().
+p = strchr(principal, '@');
Should it be strrchr()?
+if (p == NULL || *(++p + realm_len) != '\0') {
+goto done;
+}
+memcpy(p, ipactx->realm, realm_len);
+
+kerr = ipadb_fetch_principals(ipactx, flags, principal,
Re: [Freeipa-devel] [PATCH 149] IPA KDB: allow case in-sensitive realm in AS request
On Tue, Jul 21, 2015 at 01:41:14PM +0200, Sumit Bose wrote: > 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. > > 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. 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]. > > What I got from the comments in the ticket and the related bugzilla > ticket is that it should be possible to get a TGT for a user even if the > realm is given in lower-case if canonicalization is enabled. Please note > that the client can only send such request because we have > 'dns_lookup_kdc = true' in krb.conf and DNS is case in-sensitive. If you > set 'dns_lookup_kdc = false' the client will fail immediately without > sending a request at all, because it is not able to find a KDC for the > lower-case realm. > > On the server-side the request is processed because of > http://k5wiki.kerberos.org/wiki/Projects/Aliases which made parts of > processing case in-sensitive. > > With the attached patch a second lookup is done if the lookup with the > original input returned no result, canonicalization is > enabled and the realm from the original input matches the IPA realm case > in-sensitive. For the second lookup the realm is replace with the IPA > realm. This approach adds a bit redundant code but does not add extra > processing requests which would be successful before. > > Without the patch > kinitipauser@IPA.REALM -> success > kinit -C ipauser@IPA.REALM -> success > kinitipauser@ipa.realm -> failure > kinit -C ipauser@ipa.realm -> failure > > With the patch > kinitipauser@IPA.REALM -> success > kinit -C ipauser@IPA.REALM -> success > kinitipauser@ipa.realm -> success ah, sorry, copy-and-paste error, this will of course still fail. Even if we would automatically canonicalize it on the server the client wouldn't accept the changed principal without the -C option. bye, Sumit > kinit -C ipauser@ipa.realm -> success > > where 'ipa.realm' can be replace by mixed case version like 'iPa.ReAlM' > as well. > > bye, > Sumit > > [1] I was not able to add 'krbcanonicalname' as admin user because of an > ACI denial. I wonder if this is expected or if the ACI rules should be > extended here? > > [2] We might to skip the requirement that 'krbcanonicalname' must exists > if 'ipaKrbPrincipal' only has a single value but canonicalization will > fail immediately if someone adds a second value so I guess it would be > more safe to keep it as it is. > -- 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
