[Freeipa-devel] [PATCH 149] IPA KDB: allow case in-sensitive realm in AS request
Sumit Bose
sbose at redhat.com
Tue Jul 28 13:02:56 UTC 2015
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.
bye,
Sumit
-------------- next part --------------
From da05e8e800aed9eb00536c359510272ad04e42ba Mon Sep 17 00:00:00 2001
From: Sumit Bose <sbose at redhat.com>
Date: Tue, 28 Jul 2015 11:00:41 +0200
Subject: [PATCH 149/150] 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) is transformed into upper-case to match the IPA
convention for realm names.
Resolves https://fedorahosted.org/freeipa/ticket/4844
---
daemons/ipa-kdb/ipa_kdb.h | 2 +-
daemons/ipa-kdb/ipa_kdb_common.c | 43 ++++++++++++++++++++++++++-
daemons/ipa-kdb/ipa_kdb_principals.c | 3 +-
daemons/ipa-kdb/ipa_kdb_pwdpolicy.c | 2 +-
daemons/ipa-kdb/tests/ipa_kdb_tests.c | 55 +++++++++++++++++++++++++++++++++++
5 files changed, 101 insertions(+), 4 deletions(-)
diff --git a/daemons/ipa-kdb/ipa_kdb.h b/daemons/ipa-kdb/ipa_kdb.h
index 4abb7335d356f976eb5dc777c94b35c81655ad79..a9d36fe259b60fdc7d500c889b18c6a2e57a3f47 100644
--- a/daemons/ipa-kdb/ipa_kdb.h
+++ b/daemons/ipa-kdb/ipa_kdb.h
@@ -133,7 +133,7 @@ struct ipadb_context *ipadb_get_context(krb5_context kcontext);
int ipadb_get_connection(struct ipadb_context *ipactx);
/* COMMON LDAP FUNCTIONS */
-char *ipadb_filter_escape(const char *input, bool star);
+char *ipadb_filter_escape(const char *input, bool star, bool unify);
krb5_error_code ipadb_simple_search(struct ipadb_context *ipactx,
char *basedn, int scope,
char *filter, char **attrs,
diff --git a/daemons/ipa-kdb/ipa_kdb_common.c b/daemons/ipa-kdb/ipa_kdb_common.c
index 112086b57c9f83895589538b5494ae81fb14a948..80afa23f06efcc12201189ff9d13cbfb9cb489ff 100644
--- a/daemons/ipa-kdb/ipa_kdb_common.c
+++ b/daemons/ipa-kdb/ipa_kdb_common.c
@@ -25,9 +25,41 @@
static struct timeval std_timeout = {300, 0};
-char *ipadb_filter_escape(const char *input, bool star)
+#define PRINC_BUF_SIZE 512
+
+static char *canon_princ(const char *princ)
+{
+ int ret;
+ char *p;
+ uint8_t *uc_realm;
+ char *canon_princ;
+ uint8_t buf[PRINC_BUF_SIZE] = { 0 };
+ size_t size = PRINC_BUF_SIZE;
+
+ p = strrchr(princ, '@');
+ if (p == NULL) {
+ return NULL;
+ }
+
+ /* Assume the worst-case. */
+ uc_realm = u8_toupper((const uint8_t *)( p + 1), size, NULL, NULL, buf,
+ &size);
+ if (uc_realm == NULL) {
+ return NULL;
+ }
+
+ ret = asprintf(&canon_princ, "%.*s@%s", (p - princ), princ, uc_realm);
+ if (ret == -1) {
+ return NULL;
+ }
+
+ return canon_princ;
+}
+
+char *ipadb_filter_escape(const char *input, bool star, bool canon)
{
char *output;
+ char *canonicalized;
size_t i = 0;
size_t j = 0;
@@ -75,6 +107,15 @@ char *ipadb_filter_escape(const char *input, bool star)
}
output[j] = '\0';
+ if (canon) {
+ canonicalized = canon_princ(output);
+ /* return output in case of an error */
+ if (canonicalized != NULL) {
+ free(output);
+ output = canonicalized;
+ }
+ }
+
return output;
}
diff --git a/daemons/ipa-kdb/ipa_kdb_principals.c b/daemons/ipa-kdb/ipa_kdb_principals.c
index b3f8b1ad7784f55f55b4d6edd05f778a9389de27..5fb280d6217c2957b9a554cc5fd3a027ddd729fa 100644
--- a/daemons/ipa-kdb/ipa_kdb_principals.c
+++ b/daemons/ipa-kdb/ipa_kdb_principals.c
@@ -788,7 +788,8 @@ static krb5_error_code ipadb_fetch_principals(struct ipadb_context *ipactx,
/* escape filter but do not touch '*' as this function accepts
* wildcards in names */
- esc_original_princ = ipadb_filter_escape(principal, false);
+ esc_original_princ = ipadb_filter_escape(principal, false,
+ (flags & KRB5_KDB_FLAG_ALIAS_OK));
if (!esc_original_princ) {
kerr = KRB5_KDB_INTERNAL_ERROR;
goto done;
diff --git a/daemons/ipa-kdb/ipa_kdb_pwdpolicy.c b/daemons/ipa-kdb/ipa_kdb_pwdpolicy.c
index 076314a12840881a340763ab5693131aaccafec6..875960c5d77984c111b197ba03ad7ba79705d6e7 100644
--- a/daemons/ipa-kdb/ipa_kdb_pwdpolicy.c
+++ b/daemons/ipa-kdb/ipa_kdb_pwdpolicy.c
@@ -151,7 +151,7 @@ krb5_error_code ipadb_get_pwd_policy(krb5_context kcontext, char *name,
return KRB5_KDB_DBNOTINITED;
}
- esc_name = ipadb_filter_escape(name, true);
+ esc_name = ipadb_filter_escape(name, true, false);
if (!esc_name) {
return ENOMEM;
}
diff --git a/daemons/ipa-kdb/tests/ipa_kdb_tests.c b/daemons/ipa-kdb/tests/ipa_kdb_tests.c
index edd4ae0975628d6b3abe9bab2852c990c9a8c590..3f8a0bfd0bc2ac01ab84b44ee3942473bcc01d96 100644
--- a/daemons/ipa-kdb/tests/ipa_kdb_tests.c
+++ b/daemons/ipa-kdb/tests/ipa_kdb_tests.c
@@ -465,6 +465,60 @@ void test_dom_sid_string(void **state)
str_sid = dom_sid_string(test_ctx, &test_sid);
}
+void test_ipadb_filter_escape(void **state)
+{
+ char *out;
+ size_t c;
+
+ struct test_data {
+ const char *in;
+ bool star;
+ bool canon;
+ const char *exp_out;
+ } test_data[] = {
+ {"abc", false, false, "abc"},
+ {"abc", false, true, "abc"},
+ {"abc", true, true, "abc"},
+ {"abc", true, false, "abc"},
+ {"abc at def", false, false, "abc at def"},
+ {"abc at def", false, true, "abc at DEF"},
+ {"abc at def", true, true, "abc at DEF"},
+ {"abc at def", true, false, "abc at def"},
+ {"abc at DEF", false, false, "abc at DEF"},
+ {"abc at DEF", false, true, "abc at DEF"},
+ {"abc at DEF", true, true, "abc at DEF"},
+ {"abc at DEF", true, false, "abc at DEF"},
+ {"ab*c at def", false, false, "ab*c at def"},
+ {"ab*c at def", false, true, "ab*c at DEF"},
+ {"ab*c at def", true, true, "ab\\2ac at DEF"},
+ {"ab*c at def", true, false, "ab\\2ac at def"},
+ {"\\a(b)c at def", false, false, "\\5ca\\28b\\29c at def"},
+ {"\\a(b)c at def", false, true, "\\5ca\\28b\\29c at DEF"},
+ {"\\a(b)c at def", true, true, "\\5ca\\28b\\29c at DEF"},
+ {"\\a(b)c at def", true, false, "\\5ca\\28b\\29c at def"},
+ {"abc at de*f", false, false, "abc at de*f"},
+ {"abc at de*f", false, true, "abc at DE*F"},
+ {"abc at de*f", true, true, "abc at DE\\2AF"},
+ {"abc at de*f", true, false, "abc at de\\2af"},
+ /* Special characters must be UTF-8 encoded, don't change encoding */
+ {"abc@???", false, false, "abc@???"},
+ {"abc@???", false, true, "abc@???"},
+ {"abc@???", true, true, "abc@???"},
+ {"abc@???", true, false, "abc@???"},
+ {NULL, false, false, NULL}
+ };
+
+ out = ipadb_filter_escape(NULL, false, false);
+ assert_null(out);
+
+ for (c = 0; test_data[c]. in != NULL; c++) {
+ out = ipadb_filter_escape(test_data[c].in, test_data[c].star,
+ test_data[c].canon);
+ assert_string_equal(out, test_data[c].exp_out);
+ free(out);
+ }
+}
+
int main(int argc, const char *argv[])
{
@@ -473,6 +527,7 @@ int main(int argc, const char *argv[])
unit_test_setup_teardown(test_filter_logon_info, setup, teardown),
unit_test(test_string_to_sid),
unit_test_setup_teardown(test_dom_sid_string, setup, teardown),
+ unit_test(test_ipadb_filter_escape),
};
return run_tests(tests);
--
2.4.3
-------------- next part --------------
From 067b284d041994442b825aac8326d4624789d652 Mon Sep 17 00:00:00 2001
From: Sumit Bose <sbose at redhat.com>
Date: Tue, 28 Jul 2015 10:56:26 +0200
Subject: [PATCH 150/150] IPA KDB: use empty profile to init krb5 context in
tests
If the systems /etc/krb5.conf contains some unexpected or broken
configuration the test might fail. With this patch the tests are run
with an empty configuration.
---
daemons/ipa-kdb/tests/ipa_kdb_tests.c | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/daemons/ipa-kdb/tests/ipa_kdb_tests.c b/daemons/ipa-kdb/tests/ipa_kdb_tests.c
index 3f8a0bfd0bc2ac01ab84b44ee3942473bcc01d96..5f0dda6e8a59e5a76c296bfa209801bfd20c40ab 100644
--- a/daemons/ipa-kdb/tests/ipa_kdb_tests.c
+++ b/daemons/ipa-kdb/tests/ipa_kdb_tests.c
@@ -27,6 +27,7 @@
#include <stdint.h>
#include <setjmp.h>
#include <cmocka.h>
+#include <profile.h>
#include <talloc.h>
@@ -79,8 +80,13 @@ void setup(void **state)
krb5_error_code kerr;
struct ipadb_context *ipa_ctx;
struct test_ctx *test_ctx;
+ struct _profile_t *profile;
+ long perr;
- kerr = krb5_init_context(&krb5_ctx);
+ perr = profile_init(NULL, &profile);
+ assert_int_equal(perr, 0);
+
+ kerr = krb5_init_context_profile(profile, 0, &krb5_ctx);
assert_int_equal(kerr, 0);
kerr = krb5_db_setup_lib_handle(krb5_ctx);
assert_int_equal(kerr, 0);
--
2.4.3
More information about the Freeipa-devel
mailing list