kafka-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From maniku...@apache.org
Subject [kafka] branch trunk updated: KAFKA-8860: Let SslPrincipalMapper split SSL principal mapping rules
Date Mon, 02 Sep 2019 18:03:31 GMT
This is an automated email from the ASF dual-hosted git repository.

manikumar pushed a commit to branch trunk
in repository https://gitbox.apache.org/repos/asf/kafka.git


The following commit(s) were added to refs/heads/trunk by this push:
     new 88d1b6d  KAFKA-8860: Let SslPrincipalMapper split SSL principal mapping rules
88d1b6d is described below

commit 88d1b6de1f0e5b5228743f4f54ac01a2a153adf9
Author: teebee <tb@teebee.de>
AuthorDate: Mon Sep 2 23:32:49 2019 +0530

    KAFKA-8860: Let SslPrincipalMapper split SSL principal mapping rules
    
    Author: teebee <tb@teebee.de>
    
    Reviewers: Reviewers: Manikumar Reddy <manikumar.reddy@gmail.com>
    
    Closes #7140 from teebee/teebee/ssl-principal-mapping-rules-handling
---
 .../config/internals/BrokerSecurityConfigs.java    |   2 +-
 .../kafka/common/network/SslChannelBuilder.java    |   4 +-
 .../common/security/ssl/SslPrincipalMapper.java    |  62 +++++------
 .../auth/DefaultKafkaPrincipalBuilderTest.java     |   4 +-
 .../security/ssl/SslPrincipalMapperTest.java       | 114 ++++++++++++---------
 core/src/main/scala/kafka/server/KafkaConfig.scala |   2 +-
 6 files changed, 98 insertions(+), 90 deletions(-)

diff --git a/clients/src/main/java/org/apache/kafka/common/config/internals/BrokerSecurityConfigs.java
b/clients/src/main/java/org/apache/kafka/common/config/internals/BrokerSecurityConfigs.java
index 98f6467..de54f16 100644
--- a/clients/src/main/java/org/apache/kafka/common/config/internals/BrokerSecurityConfigs.java
+++ b/clients/src/main/java/org/apache/kafka/common/config/internals/BrokerSecurityConfigs.java
@@ -55,7 +55,7 @@ public class BrokerSecurityConfigs {
             " see <a href=\"#security_authz\"> security authorization and acls</a>.
Note that this configuration is ignored" +
             " if an extension of KafkaPrincipalBuilder is provided by the <code>" +
PRINCIPAL_BUILDER_CLASS_CONFIG + "</code>" +
            " configuration.";
-    public static final List<String> DEFAULT_SSL_PRINCIPAL_MAPPING_RULES = Collections.singletonList("DEFAULT");
+    public static final String DEFAULT_SSL_PRINCIPAL_MAPPING_RULES = "DEFAULT";
 
     public static final String SASL_KERBEROS_PRINCIPAL_TO_LOCAL_RULES_DOC = "A list of rules
for mapping from principal " +
             "names to short names (typically operating system usernames). The rules are evaluated
in order and the " +
diff --git a/clients/src/main/java/org/apache/kafka/common/network/SslChannelBuilder.java
b/clients/src/main/java/org/apache/kafka/common/network/SslChannelBuilder.java
index 5008199..aade37c 100644
--- a/clients/src/main/java/org/apache/kafka/common/network/SslChannelBuilder.java
+++ b/clients/src/main/java/org/apache/kafka/common/network/SslChannelBuilder.java
@@ -35,7 +35,6 @@ import java.net.InetAddress;
 import java.net.InetSocketAddress;
 import java.nio.channels.SelectionKey;
 import java.nio.channels.SocketChannel;
-import java.util.List;
 import java.util.Map;
 import java.util.Set;
 import java.util.function.Supplier;
@@ -63,8 +62,7 @@ public class SslChannelBuilder implements ChannelBuilder, ListenerReconfigurable
     public void configure(Map<String, ?> configs) throws KafkaException {
         try {
             this.configs = configs;
-            @SuppressWarnings("unchecked")
-            List<String> sslPrincipalMappingRules = (List<String>) configs.get(BrokerSecurityConfigs.SSL_PRINCIPAL_MAPPING_RULES_CONFIG);
+            String sslPrincipalMappingRules = (String) configs.get(BrokerSecurityConfigs.SSL_PRINCIPAL_MAPPING_RULES_CONFIG);
             if (sslPrincipalMappingRules != null)
                 sslPrincipalMapper = SslPrincipalMapper.fromRules(sslPrincipalMappingRules);
             this.sslFactory = new SslFactory(mode, null, isInterBrokerListener);
diff --git a/clients/src/main/java/org/apache/kafka/common/security/ssl/SslPrincipalMapper.java
b/clients/src/main/java/org/apache/kafka/common/security/ssl/SslPrincipalMapper.java
index 3b95e1a..977fdc3 100644
--- a/clients/src/main/java/org/apache/kafka/common/security/ssl/SslPrincipalMapper.java
+++ b/clients/src/main/java/org/apache/kafka/common/security/ssl/SslPrincipalMapper.java
@@ -18,53 +18,44 @@ package org.apache.kafka.common.security.ssl;
 
 import java.io.IOException;
 import java.util.List;
-import java.util.Collections;
 import java.util.ArrayList;
 import java.util.Locale;
 import java.util.regex.Matcher;
 import java.util.regex.Pattern;
 
+import static org.apache.kafka.common.config.internals.BrokerSecurityConfigs.DEFAULT_SSL_PRINCIPAL_MAPPING_RULES;
+
 public class SslPrincipalMapper {
 
-    private static final Pattern RULE_PARSER = Pattern.compile("((DEFAULT)|(RULE:(([^/]*)/([^/]*))/([LU])?))");
+    private static final String RULE_PATTERN = "(DEFAULT)|RULE:((\\\\.|[^\\\\/])*)/((\\\\.|[^\\\\/])*)/([LU]?).*?|(.*?)";
+    private static final Pattern RULE_SPLITTER = Pattern.compile("\\s*(" + RULE_PATTERN +
")\\s*(,\\s*|$)");
+    private static final Pattern RULE_PARSER = Pattern.compile(RULE_PATTERN);
 
     private final List<Rule> rules;
 
-    public SslPrincipalMapper(List<Rule> sslPrincipalMappingRules) {
-        this.rules = sslPrincipalMappingRules;
+    public SslPrincipalMapper(String sslPrincipalMappingRules) {
+        this.rules = parseRules(splitRules(sslPrincipalMappingRules));
     }
 
-    public static SslPrincipalMapper fromRules(List<String> sslPrincipalMappingRules)
{
-        List<String> rules = sslPrincipalMappingRules == null ? Collections.singletonList("DEFAULT")
: sslPrincipalMappingRules;
-        return new SslPrincipalMapper(parseRules(rules));
+    public static SslPrincipalMapper fromRules(String sslPrincipalMappingRules) {
+        return new SslPrincipalMapper(sslPrincipalMappingRules);
     }
 
-    private static List<String> joinSplitRules(List<String> rules) {
-        String rule = "RULE:";
-        String defaultRule = "DEFAULT";
-        List<String> retVal = new ArrayList<>();
-        StringBuilder currentRule = new StringBuilder();
-        for (String r : rules) {
-            if (currentRule.length() > 0) {
-                if (r.startsWith(rule) || r.equals(defaultRule)) {
-                    retVal.add(currentRule.toString());
-                    currentRule.setLength(0);
-                    currentRule.append(r);
-                } else {
-                    currentRule.append(String.format(",%s", r));
-                }
-            } else {
-                currentRule.append(r);
-            }
+    private static List<String> splitRules(String sslPrincipalMappingRules) {
+        if (sslPrincipalMappingRules == null) {
+            sslPrincipalMappingRules = DEFAULT_SSL_PRINCIPAL_MAPPING_RULES;
         }
-        if (currentRule.length() > 0) {
-            retVal.add(currentRule.toString());
+
+        List<String> result = new ArrayList<>();
+        Matcher matcher = RULE_SPLITTER.matcher(sslPrincipalMappingRules.trim());
+        while (matcher.find()) {
+            result.add(matcher.group(1));
         }
-        return retVal;
+
+        return result;
     }
 
     private static List<Rule> parseRules(List<String> rules) {
-        rules = joinSplitRules(rules);
         List<Rule> result = new ArrayList<>();
         for (String rule : rules) {
             Matcher matcher = RULE_PARSER.matcher(rule);
@@ -74,15 +65,18 @@ public class SslPrincipalMapper {
             if (rule.length() != matcher.end()) {
                 throw new IllegalArgumentException("Invalid rule: `" + rule + "`, unmatched
substring: `" + rule.substring(matcher.end()) + "`");
             }
-            if (matcher.group(2) != null) {
+
+            // empty rules are ignored
+            if (matcher.group(1) != null) {
                 result.add(new Rule());
-            } else {
-                result.add(new Rule(matcher.group(5),
-                                    matcher.group(6),
-                                    "L".equals(matcher.group(7)),
-                                    "U".equals(matcher.group(7))));
+            } else if (matcher.group(2) != null) {
+                result.add(new Rule(matcher.group(2),
+                                    matcher.group(4),
+                                    "L".equals(matcher.group(6)),
+                                    "U".equals(matcher.group(6))));
             }
         }
+
         return result;
     }
 
diff --git a/clients/src/test/java/org/apache/kafka/common/security/auth/DefaultKafkaPrincipalBuilderTest.java
b/clients/src/test/java/org/apache/kafka/common/security/auth/DefaultKafkaPrincipalBuilderTest.java
index dd5087a..dc1d622 100644
--- a/clients/src/test/java/org/apache/kafka/common/security/auth/DefaultKafkaPrincipalBuilderTest.java
+++ b/clients/src/test/java/org/apache/kafka/common/security/auth/DefaultKafkaPrincipalBuilderTest.java
@@ -30,8 +30,6 @@ import javax.net.ssl.SSLSession;
 import javax.security.sasl.SaslServer;
 import java.net.InetAddress;
 import java.security.Principal;
-import java.util.Arrays;
-import java.util.List;
 
 import static org.junit.Assert.assertEquals;
 import static org.mockito.ArgumentMatchers.any;
@@ -143,7 +141,7 @@ public class DefaultKafkaPrincipalBuilderTest {
                                         .thenReturn(new X500Principal("CN=duke, OU=JavaSoft,
O=Sun Microsystems"))
                                         .thenReturn(new X500Principal("OU=JavaSoft, O=Sun
Microsystems, C=US"));
 
-        List<String> rules = Arrays.asList(
+        String rules = String.join(", ",
             "RULE:^CN=(.*),OU=ServiceUsers.*$/$1/L",
             "RULE:^CN=(.*),OU=(.*),O=(.*),L=(.*),ST=(.*),C=(.*)$/$1@$2/L",
             "RULE:^.*[Cc][Nn]=([a-zA-Z0-9.]*).*$/$1/U",
diff --git a/clients/src/test/java/org/apache/kafka/common/security/ssl/SslPrincipalMapperTest.java
b/clients/src/test/java/org/apache/kafka/common/security/ssl/SslPrincipalMapperTest.java
index 56ef977..7b7b67b 100644
--- a/clients/src/test/java/org/apache/kafka/common/security/ssl/SslPrincipalMapperTest.java
+++ b/clients/src/test/java/org/apache/kafka/common/security/ssl/SslPrincipalMapperTest.java
@@ -18,9 +18,6 @@ package org.apache.kafka.common.security.ssl;
 
 import org.junit.Test;
 
-import java.util.Arrays;
-import java.util.List;
-
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.fail;
 
@@ -28,59 +25,37 @@ public class SslPrincipalMapperTest {
 
     @Test
     public void testValidRules() {
-        testValidRule(Arrays.asList("DEFAULT"));
-        testValidRule(Arrays.asList("RULE:^CN=(.*?),OU=ServiceUsers.*$/$1/"));
-        testValidRule(Arrays.asList("RULE:^CN=(.*?),OU=ServiceUsers.*$/$1/L", "DEFAULT"));
-        testValidRule(Arrays.asList("RULE:^CN=(.*?),OU=(.*?),O=(.*?),L=(.*?),ST=(.*?),C=(.*?)$/$1@$2/"));
-        testValidRule(Arrays.asList("RULE:^.*[Cc][Nn]=([a-zA-Z0-9.]*).*$/$1/L"));
-        testValidRule(Arrays.asList("RULE:^cn=(.?),ou=(.?),dc=(.?),dc=(.?)$/$1@$2/U"));
+        testValidRule("DEFAULT");
+        testValidRule("RULE:^CN=(.*?),OU=ServiceUsers.*$/$1/");
+        testValidRule("RULE:^CN=(.*?),OU=ServiceUsers.*$/$1/L, DEFAULT");
+        testValidRule("RULE:^CN=(.*?),OU=(.*?),O=(.*?),L=(.*?),ST=(.*?),C=(.*?)$/$1@$2/");
+        testValidRule("RULE:^.*[Cc][Nn]=([a-zA-Z0-9.]*).*$/$1/L");
+        testValidRule("RULE:^cn=(.?),ou=(.?),dc=(.?),dc=(.?)$/$1@$2/U");
+
+        testValidRule("RULE:^CN=([^,ADEFLTU,]+)(,.*|$)/$1/");
+        testValidRule("RULE:^CN=([^,DEFAULT,]+)(,.*|$)/$1/");
     }
 
-    @Test
-    public void testValidSplitRules() {
-        testValidRule(Arrays.asList("DEFAULT"));
-        testValidRule(Arrays.asList("RULE:^CN=(.*?)", "OU=ServiceUsers.*$/$1/"));
-        testValidRule(Arrays.asList("RULE:^CN=(.*?)", "OU=ServiceUsers.*$/$1/L", "DEFAULT"));
-        testValidRule(Arrays.asList("RULE:^CN=(.*?)", "OU=(.*?),O=(.*?),L=(.*?)", "ST=(.*?)",
"C=(.*?)$/$1@$2/"));
-        testValidRule(Arrays.asList("RULE:^.*[Cc][Nn]=([a-zA-Z0-9.]*).*$/$1/L"));
-        testValidRule(Arrays.asList("RULE:^cn=(.?)", "ou=(.?)", "dc=(.?)", "dc=(.?)$/$1@$2/U"));
-    }
-
-    private void testValidRule(List<String> rules) {
+    private void testValidRule(String rules) {
         SslPrincipalMapper.fromRules(rules);
     }
 
     @Test
     public void testInvalidRules() {
-        testInvalidRule(Arrays.asList("default"));
-        testInvalidRule(Arrays.asList("DEFAUL"));
-        testInvalidRule(Arrays.asList("DEFAULT/L"));
-        testInvalidRule(Arrays.asList("DEFAULT/U"));
-
-        testInvalidRule(Arrays.asList("RULE:CN=(.*?),OU=ServiceUsers.*/$1"));
-        testInvalidRule(Arrays.asList("rule:^CN=(.*?),OU=ServiceUsers.*$/$1/"));
-        testInvalidRule(Arrays.asList("RULE:^CN=(.*?),OU=ServiceUsers.*$/$1/L/U"));
-        testInvalidRule(Arrays.asList("RULE:^CN=(.*?),OU=ServiceUsers.*$/L"));
-        testInvalidRule(Arrays.asList("RULE:^CN=(.*?),OU=ServiceUsers.*$/U"));
-        testInvalidRule(Arrays.asList("RULE:^CN=(.*?),OU=ServiceUsers.*$/LU"));
+        testInvalidRule("default");
+        testInvalidRule("DEFAUL");
+        testInvalidRule("DEFAULT/L");
+        testInvalidRule("DEFAULT/U");
+
+        testInvalidRule("RULE:CN=(.*?),OU=ServiceUsers.*/$1");
+        testInvalidRule("rule:^CN=(.*?),OU=ServiceUsers.*$/$1/");
+        testInvalidRule("RULE:^CN=(.*?),OU=ServiceUsers.*$/$1/L/U");
+        testInvalidRule("RULE:^CN=(.*?),OU=ServiceUsers.*$/L");
+        testInvalidRule("RULE:^CN=(.*?),OU=ServiceUsers.*$/U");
+        testInvalidRule("RULE:^CN=(.*?),OU=ServiceUsers.*$/LU");
     }
 
-    @Test
-    public void testInvalidSplitRules() {
-        testInvalidRule(Arrays.asList("default"));
-        testInvalidRule(Arrays.asList("DEFAUL"));
-        testInvalidRule(Arrays.asList("DEFAULT/L"));
-        testInvalidRule(Arrays.asList("DEFAULT/U"));
-
-        testInvalidRule(Arrays.asList("RULE:CN=(.*?)", "OU=ServiceUsers.*/$1"));
-        testInvalidRule(Arrays.asList("rule:^CN=(.*?)", "OU=ServiceUsers.*$/$1/"));
-        testInvalidRule(Arrays.asList("RULE:^CN=(.*?)", "OU=ServiceUsers.*$/$1/L/U"));
-        testInvalidRule(Arrays.asList("RULE:^CN=(.*?)", "OU=ServiceUsers.*$/L"));
-        testInvalidRule(Arrays.asList("RULE:^CN=(.*?)", "OU=ServiceUsers.*$/U"));
-        testInvalidRule(Arrays.asList("RULE:^CN=(.*?)", "OU=ServiceUsers.*$/LU"));
-    }
-
-    private void testInvalidRule(List<String> rules) {
+    private void testInvalidRule(String rules) {
         try {
             System.out.println(SslPrincipalMapper.fromRules(rules));
             fail("should have thrown IllegalArgumentException");
@@ -90,7 +65,7 @@ public class SslPrincipalMapperTest {
 
     @Test
     public void testSslPrincipalMapper() throws Exception {
-        List<String> rules = Arrays.asList(
+        String rules = String.join(", ",
             "RULE:^CN=(.*?),OU=ServiceUsers.*$/$1/L",
             "RULE:^CN=(.*?),OU=(.*?),O=(.*?),L=(.*?),ST=(.*?),C=(.*?)$/$1@$2/L",
             "RULE:^cn=(.*?),ou=(.*?),dc=(.*?),dc=(.*?)$/$1@$2/U",
@@ -107,4 +82,47 @@ public class SslPrincipalMapperTest {
         assertEquals("OU=JavaSoft,O=Sun Microsystems,C=US", mapper.getName("OU=JavaSoft,O=Sun
Microsystems,C=US"));
     }
 
+    private void testRulesSplitting(String expected, String rules) {
+        SslPrincipalMapper mapper = SslPrincipalMapper.fromRules(rules);
+        assertEquals(String.format("SslPrincipalMapper(rules = %s)", expected), mapper.toString());
+    }
+
+    @Test
+    public void testRulesSplitting() {
+        // seeing is believing
+        testRulesSplitting("[]", "");
+        testRulesSplitting("[DEFAULT]", "DEFAULT");
+        testRulesSplitting("[RULE:/]", "RULE://");
+        testRulesSplitting("[RULE:/.*]", "RULE:/.*/");
+        testRulesSplitting("[RULE:/.*/L]", "RULE:/.*/L");
+        testRulesSplitting("[RULE:/, DEFAULT]", "RULE://,DEFAULT");
+        testRulesSplitting("[RULE:/, DEFAULT]", "  RULE:// ,  DEFAULT  ");
+        testRulesSplitting("[RULE:   /     , DEFAULT]", "  RULE:   /     / ,  DEFAULT  ");
+        testRulesSplitting("[RULE:  /     /U, DEFAULT]", "  RULE:  /     /U   ,DEFAULT  ");
+        testRulesSplitting("[RULE:([A-Z]*)/$1/U, RULE:([a-z]+)/$1, DEFAULT]", "  RULE:([A-Z]*)/$1/U
  ,RULE:([a-z]+)/$1/,   DEFAULT  ");
+
+        // empty rules are ignored
+        testRulesSplitting("[]", ",   , , ,      , , ,   ");
+        testRulesSplitting("[RULE:/, DEFAULT]", ",,RULE://,,,DEFAULT,,");
+        testRulesSplitting("[RULE: /   , DEFAULT]", ",  , RULE: /   /    ,,,   DEFAULT, ,
  ");
+        testRulesSplitting("[RULE:   /  /U, DEFAULT]", "     ,  , RULE:   /  /U    ,,  ,DEFAULT,
,");
+
+        // escape sequences
+        testRulesSplitting("[RULE:\\/\\\\\\(\\)\\n\\t/\\/\\/]", "RULE:\\/\\\\\\(\\)\\n\\t/\\/\\//");
+        testRulesSplitting("[RULE:\\**\\/+/*/L, RULE:\\/*\\**/**]", "RULE:\\**\\/+/*/L,RULE:\\/*\\**/**/");
+
+        // rules rule
+        testRulesSplitting(
+            "[RULE:,RULE:,/,RULE:,\\//U, RULE:,/RULE:,, RULE:,RULE:,/L,RULE:,/L, RULE:, DEFAULT,
/DEFAULT, DEFAULT]",
+            "RULE:,RULE:,/,RULE:,\\//U,RULE:,/RULE:,/,RULE:,RULE:,/L,RULE:,/L,RULE:, DEFAULT,
/DEFAULT/,DEFAULT"
+        );
+    }
+
+    @Test
+    public void testCommaWithWhitespace() throws Exception {
+        String rules = "RULE:^CN=((\\\\, *|\\w)+)(,.*|$)/$1/,DEFAULT";
+
+        SslPrincipalMapper mapper = SslPrincipalMapper.fromRules(rules);
+        assertEquals("Tkac\\, Adam", mapper.getName("CN=Tkac\\, Adam,OU=ITZ,DC=geodis,DC=cz"));
+    }
 }
diff --git a/core/src/main/scala/kafka/server/KafkaConfig.scala b/core/src/main/scala/kafka/server/KafkaConfig.scala
index 97f894c..4d6a65d 100755
--- a/core/src/main/scala/kafka/server/KafkaConfig.scala
+++ b/core/src/main/scala/kafka/server/KafkaConfig.scala
@@ -1059,7 +1059,7 @@ object KafkaConfig {
       .define(SslSecureRandomImplementationProp, STRING, null, LOW, SslSecureRandomImplementationDoc)
       .define(SslClientAuthProp, STRING, Defaults.SslClientAuthentication, in(Defaults.SslClientAuthenticationValidValues:_*),
MEDIUM, SslClientAuthDoc)
       .define(SslCipherSuitesProp, LIST, Collections.emptyList(), MEDIUM, SslCipherSuitesDoc)
-      .define(SslPrincipalMappingRulesProp, LIST, Defaults.SslPrincipalMappingRules, LOW,
SslPrincipalMappingRulesDoc)
+      .define(SslPrincipalMappingRulesProp, STRING, Defaults.SslPrincipalMappingRules, LOW,
SslPrincipalMappingRulesDoc)
 
       /** ********* Sasl Configuration ****************/
       .define(SaslMechanismInterBrokerProtocolProp, STRING, Defaults.SaslMechanismInterBrokerProtocol,
MEDIUM, SaslMechanismInterBrokerProtocolDoc)


Mime
View raw message