KAFKA-5764: Add toLowerCase support to sasl.kerberos.principal.to.local rule #3800

omkreddy · 2017-09-06T07:35:58Z

Update the tests and docs

Committer Checklist (excluded from commit message)

Verify design and implementation
Verify test coverage and CI build status
Verify documentation (including upgrade notes)

omkreddy · 2017-09-14T07:14:09Z

retest this please

omkreddy · 2017-09-14T07:15:31Z

cc @hachikuji @ijuma pinging for review

hachikuji · 2017-09-14T18:07:37Z

@omkreddy Thanks for the patch. Looks like a good improvement, but I think we might need a short KIP since it changes a public API.

omkreddy · 2017-11-29T10:04:02Z

https://cwiki.apache.org/confluence/display/KAFKA/KIP-203%3A+Add+toLowerCase+support+to+sasl.kerberos.principal.to.local+rule

@hachikuji @ijuma Can you take a look at this minor KIP-203? KIP vote thread is waiting for couple of more votes.

omkreddy · 2017-11-29T10:07:31Z

retest this please

hachikuji · 2017-11-29T23:38:48Z

@omkreddy Voted. I'll see if I can get one more vote for you as well.

omkreddy · 2017-12-07T16:26:27Z

@hachikuji whenever you get a chance, Please review the PR

hachikuji

Thanks for the patch. Left a few comments/questions.

hachikuji · 2017-12-07T17:17:27Z

clients/src/main/java/org/apache/kafka/common/security/kerberos/KerberosShortNamer.java

@@ -33,7 +33,7 @@
    /**
     * A pattern for parsing a auth_to_local rule.
     */
-    private static final Pattern RULE_PARSER = Pattern.compile("((DEFAULT)|(RULE:\\[(\\d*):([^\\]]*)](\\(([^)]*)\\))?(s/([^/]*)/([^/]*)/(g)?)?))");
+    private static final Pattern RULE_PARSER = Pattern.compile("((DEFAULT)|(RULE:\\[(\\d*):([^\\]]*)](\\(([^)]*)\\))?(s/([^/]*)/([^/]*)/(g)?)?))/?(L)?");


Seems like "DEFAULT/L" is a valid input. Is that intentional?

it is not intentional, but i was not using "/L" in this case. Updated the regex to treat "DEFAULT/L" as invalid input.

hachikuji · 2017-12-07T17:22:14Z

docs/security.html

+        RULE:[n:string](regexp)s/pattern/replacement/
+        RULE:[n:string](regexp)s/pattern/replacement/g
+        RULE:[n:string](regexp)s/pattern/replacement//L
+        RULE:[n:string](regexp)s/pattern/replacement/g/L</pre>


nit: move the end tag to the next line.

hachikuji · 2017-12-07T17:25:07Z

docs/security.html

-    The format of <code>sasl.kerberos.principal.to.local.rules</code> is a list where each rule works in the same way as the auth_to_local in <a href="http://web.mit.edu/Kerberos/krb5-latest/doc/admin/conf_files/krb5_conf.html">Kerberos configuration file (krb5.conf)</a>. Each rules starts with RULE: and contains an expression in the format [n:string](regexp)s/pattern/replacement/g. See the kerberos documentation for more details. An example of adding a rule to properly translate user@MYDOMAIN.COM to user while also keeping the default rule in place is:
+    By default, the SASL user name will be the primary part of the Kerberos principal. One can change that by setting <code>sasl.kerberos.principal.to.local.rules</code> to a customized rule in broker.properties.
+    The format of <code>sasl.kerberos.principal.to.local.rules</code> is a list where each rule works in the same way as the auth_to_local in <a href="http://web.mit.edu/Kerberos/krb5-latest/doc/admin/conf_files/krb5_conf.html">Kerberos configuration file (krb5.conf)</a>. This also support additional lowercase rule, to force the translated result to be all lower case. This is done by adding a "/L" to the end of the rule. check below formats for syntax.
+    Each rules starts with RULE: and contains an expression as the following formats. See the kerberos documentation for more details.


Great to have some examples in our docs 👍 . Do you think it's worth explaining each case in a little more detail?

There are few articles available on web explaining these rules. so I felt not to repeat in our docs.

hachikuji · 2017-12-07T17:29:10Z

clients/src/main/java/org/apache/kafka/common/security/kerberos/KerberosRule.java

@@ -182,6 +189,9 @@ String apply(String[] params) throws IOException {
        if (result != null && NON_SIMPLE_PATTERN.matcher(result).find()) {
            throw new NoMatchingRule("Non-simple name " + result + " after auth_to_local rule " + this);
        }
+        if (toLowerCase && result != null) {
+            result = result.toLowerCase(Locale.ENGLISH);


Do you know the range of characters permitted in kerberos names?

I am not sure about the character range. I felt it is safe to set EN locale. Hadoop implementation is also using EN locale.

@rajinisivaram any thoughts here?

…al rules

omkreddy · 2017-12-11T16:35:30Z

retest this please

omkreddy · 2017-12-12T15:40:00Z

@hachikuji pinging for review

hachikuji

Thanks for the update. One comment on testing. The rest looks good.

hachikuji · 2017-12-12T16:49:53Z

clients/src/test/java/org/apache/kafka/common/security/kerberos/KerberosNameTest.java

@@ -55,4 +55,33 @@ public void testParse() throws IOException {
        assertEquals("REALM.COM", name.realm());
        assertEquals("user", shortNamer.shortName(name));
    }
+
+    @Test
+    public void testToLowerCase() throws Exception {


Do we have any test cases which validate the behavior for invalid rules?

@hachikuji Thanks for the review. Added a simple test case for invalid rules. Pls take a look.

hachikuji

LGTM. Thanks for the KIP and the patch! Will merge assuming no issues with the builds.

…al rule (KIP-203) Author: Manikumar Reddy <manikumar.reddy@gmail.com> Reviewers: Jason Gustafson <jason@confluent.io> Closes apache#3800 from omkreddy/KAFKA-5764-REGEX

umaanbazhagan · 2018-06-26T23:30:37Z

This is useful, can we backport this fix to Kafka 1.0? If yes, I have a PR for this: #5260

omkreddy · 2018-06-27T14:39:16Z

@umaanbazhagan Currently Kafka 1.0.2 Release vote is going on. I am not sure if we can include this now. You can post your query on vote thread.

omkreddy force-pushed the KAFKA-5764-REGEX branch from b155f3f to fa2666b Compare September 14, 2017 11:15

omkreddy force-pushed the KAFKA-5764-REGEX branch from fa2666b to 9a3e2c6 Compare September 30, 2017 11:27

hachikuji reviewed Dec 7, 2017

View reviewed changes

omkreddy added 2 commits December 8, 2017 14:50

KAFKA-5764: Add toLowerCase support to sasl.kerberos.principal.to.loc…

0cc0afa

…al rules

Address review comments

2ac957f

omkreddy force-pushed the KAFKA-5764-REGEX branch from 9a3e2c6 to 2ac957f Compare December 8, 2017 11:32

hachikuji reviewed Dec 12, 2017

View reviewed changes

Address review comments

4d34056

hachikuji approved these changes Dec 12, 2017

View reviewed changes

asfgit closed this in b088822 Dec 12, 2017

umaanbazhagan mentioned this pull request Jun 20, 2018

KAFKA-5764; Add toLowerCase support to sasl.kerberos.principal.to.loc… #5260

Closed

3 tasks

omkreddy deleted the KAFKA-5764-REGEX branch July 3, 2018 15:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

KAFKA-5764: Add toLowerCase support to sasl.kerberos.principal.to.local rule #3800

KAFKA-5764: Add toLowerCase support to sasl.kerberos.principal.to.local rule #3800

omkreddy commented Sep 6, 2017 •

edited

Loading

omkreddy commented Sep 14, 2017

omkreddy commented Sep 14, 2017

hachikuji commented Sep 14, 2017

omkreddy commented Nov 29, 2017

omkreddy commented Nov 29, 2017

hachikuji commented Nov 29, 2017

omkreddy commented Dec 7, 2017

hachikuji left a comment

hachikuji Dec 7, 2017

omkreddy Dec 8, 2017

hachikuji Dec 7, 2017

hachikuji Dec 7, 2017

omkreddy Dec 8, 2017

hachikuji Dec 7, 2017

omkreddy Dec 8, 2017

omkreddy commented Dec 11, 2017

omkreddy commented Dec 12, 2017

hachikuji left a comment

hachikuji Dec 12, 2017

omkreddy Dec 12, 2017

hachikuji left a comment

umaanbazhagan commented Jun 26, 2018

omkreddy commented Jun 27, 2018

KAFKA-5764: Add toLowerCase support to sasl.kerberos.principal.to.local rule #3800

KAFKA-5764: Add toLowerCase support to sasl.kerberos.principal.to.local rule #3800

Conversation

omkreddy commented Sep 6, 2017 • edited Loading

Committer Checklist (excluded from commit message)

omkreddy commented Sep 14, 2017

omkreddy commented Sep 14, 2017

hachikuji commented Sep 14, 2017

omkreddy commented Nov 29, 2017

omkreddy commented Nov 29, 2017

hachikuji commented Nov 29, 2017

omkreddy commented Dec 7, 2017

hachikuji left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

omkreddy commented Dec 11, 2017

omkreddy commented Dec 12, 2017

hachikuji left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hachikuji left a comment

Choose a reason for hiding this comment

umaanbazhagan commented Jun 26, 2018

omkreddy commented Jun 27, 2018

omkreddy commented Sep 6, 2017 •

edited

Loading