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.

bye,
Sumit
From da05e8e800aed9eb00536c359510272ad04e42ba Mon Sep 17 00:00:00 2001
From: Sumit Bose <sb...@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@def", false, false, "abc@def"},
+        {"abc@def", false, true, "abc@DEF"},
+        {"abc@def", true, true, "abc@DEF"},
+        {"abc@def", true, false, "abc@def"},
+        {"abc@DEF", false, false, "abc@DEF"},
+        {"abc@DEF", false, true, "abc@DEF"},
+        {"abc@DEF", true, true, "abc@DEF"},
+        {"abc@DEF", true, false, "abc@DEF"},
+        {"ab*c@def", false, false, "ab*c@def"},
+        {"ab*c@def", false, true, "ab*c@DEF"},
+        {"ab*c@def", true, true, "ab\\2ac@DEF"},
+        {"ab*c@def", true, false, "ab\\2ac@def"},
+        {"\\a(b)c@def", false, false, "\\5ca\\28b\\29c@def"},
+        {"\\a(b)c@def", false, true, "\\5ca\\28b\\29c@DEF"},
+        {"\\a(b)c@def", true, true, "\\5ca\\28b\\29c@DEF"},
+        {"\\a(b)c@def", true, false, "\\5ca\\28b\\29c@def"},
+        {"abc@de*f", false, false, "abc@de*f"},
+        {"abc@de*f", false, true, "abc@DE*F"},
+        {"abc@de*f", true, true, "abc@DE\\2AF"},
+        {"abc@de*f", true, false, "abc@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

From 067b284d041994442b825aac8326d4624789d652 Mon Sep 17 00:00:00 2001
From: Sumit Bose <sb...@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

-- 
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