Support Matcher group index for extracting ACL entity from SAN in X509 auth provider (#56)

narendly · web-flow · commit 042f094f31a0 · 2021-08-12T10:12:43.000-04:00
In order to allow for greater flexibility in extracting a part of the string given in the SAN in the client certificate in X509AuthenticationProvider, this commit adds another JVM parameter for specifying Java regex Matcher Group index. X509AuthTest still has coverage of this logic.

Also, 1) internal enum names have been renamed for better readability (spaced with underscores), and 2) the test case has been enhanced with a more complicated regex with a Matcher group index.
diff --git a/zookeeper-server/src/main/java/org/apache/zookeeper/server/auth/X509AuthenticationProvider.java b/zookeeper-server/src/main/java/org/apache/zookeeper/server/auth/X509AuthenticationProvider.java
@@ -31,7 +31,6 @@
 import javax.security.auth.x500.X500Principal;
 import org.apache.zookeeper.KeeperException;
 import org.apache.zookeeper.common.ClientX509Util;
-import org.apache.zookeeper.common.X509Exception;
 import org.apache.zookeeper.common.X509Exception.KeyManagerException;
 import org.apache.zookeeper.common.X509Exception.TrustManagerException;
 import org.apache.zookeeper.common.X509Util;
@@ -61,12 +60,14 @@ public class X509AuthenticationProvider implements AuthenticationProvider {
      * The following System Property keys are used to extract clientId from the client cert.
      * @see #getClientId(X509Certificate)
      */
-    public static final String ZOOKEEPER_X509AUTHENTICATIONPROVIDER_CLIENTCERTIDTYPE = "zookeeper.X509AuthenticationProvider.clientCertIdType";
-    public static final String ZOOKEEPER_X509AUTHENTICATIONPROVIDER_CLIENTCERTIDSANMATCHTYPE = "zookeeper.X509AuthenticationProvider.clientCertIdSanMatchType";
+    public static final String ZOOKEEPER_X509AUTHENTICATIONPROVIDER_CLIENT_CERT_ID_TYPE = "zookeeper.X509AuthenticationProvider.clientCertIdType";
+    public static final String ZOOKEEPER_X509AUTHENTICATIONPROVIDER_CLIENT_CERT_ID_SAN_MATCH_TYPE = "zookeeper.X509AuthenticationProvider.clientCertIdSanMatchType";
     // Match Regex is used to choose which entry to use
-    public static final String ZOOKEEPER_X509AUTHENTICATIONPROVIDER_CLIENTCERTIDSANMATCHREGEX = "zookeeper.X509AuthenticationProvider.clientCertIdSanMatchRegex";
+    public static final String ZOOKEEPER_X509AUTHENTICATIONPROVIDER_CLIENT_CERT_ID_SAN_MATCH_REGEX = "zookeeper.X509AuthenticationProvider.clientCertIdSanMatchRegex";
     // Extract Regex is used to construct a client ID (acl entity) to return
-    public static final String ZOOKEEPER_X509AUTHENTICATIONPROVIDER_CLIENTCERTIDSANEXTRACTREGEX = "zookeeper.X509AuthenticationProvider.clientCertIdSanExtractRegex";
+    public static final String ZOOKEEPER_X509AUTHENTICATIONPROVIDER_CLIENT_CERT_ID_SAN_EXTRACT_REGEX = "zookeeper.X509AuthenticationProvider.clientCertIdSanExtractRegex";
+    // Specifies match group index for the extract regex (i in Matcher.group(i))
+    public static final String ZOOKEEPER_X509AUTHENTICATIONPROVIDER_CLIENT_CERT_ID_SAN_EXTRACT_MATCHER_GROUP_INDEX = "zookeeper.X509AuthenticationProvider.clientCertIdSanExtractMatcherGroupIndex";
 
     static final Logger LOG = LoggerFactory.getLogger(X509AuthenticationProvider.class);
     private final X509TrustManager trustManager;
@@ -196,11 +197,11 @@ public KeeperException.Code handleAuthentication(ServerCnxn cnxn, byte[] authDat
      */
     protected String getClientId(X509Certificate clientCert) {
         String clientCertIdType =
-            System.getProperty(ZOOKEEPER_X509AUTHENTICATIONPROVIDER_CLIENTCERTIDTYPE);
+            System.getProperty(ZOOKEEPER_X509AUTHENTICATIONPROVIDER_CLIENT_CERT_ID_TYPE);
         if (clientCertIdType != null && clientCertIdType.equalsIgnoreCase("SAN")) {
             try {
                 return matchAndExtractSAN(clientCert);
-            } catch (CertificateException | IllegalArgumentException ce) {
+            } catch (Exception ce) {
                 LOG.warn("X509AuthenticationProvider::getClientId(): failed to match and extract a"
                     + " client ID from SAN! Using Subject DN instead...", ce);
             }
@@ -212,14 +213,18 @@ protected String getClientId(X509Certificate clientCert) {
     private String matchAndExtractSAN(X509Certificate clientCert)
         throws CertificateParsingException {
         Integer matchType =
-            Integer.getInteger(ZOOKEEPER_X509AUTHENTICATIONPROVIDER_CLIENTCERTIDSANMATCHTYPE);
+            Integer.getInteger(ZOOKEEPER_X509AUTHENTICATIONPROVIDER_CLIENT_CERT_ID_SAN_MATCH_TYPE);
         String matchRegex =
-            System.getProperty(ZOOKEEPER_X509AUTHENTICATIONPROVIDER_CLIENTCERTIDSANMATCHREGEX);
+            System.getProperty(ZOOKEEPER_X509AUTHENTICATIONPROVIDER_CLIENT_CERT_ID_SAN_MATCH_REGEX);
         String extractRegex =
-            System.getProperty(ZOOKEEPER_X509AUTHENTICATIONPROVIDER_CLIENTCERTIDSANEXTRACTREGEX);
+            System.getProperty(
+                ZOOKEEPER_X509AUTHENTICATIONPROVIDER_CLIENT_CERT_ID_SAN_EXTRACT_REGEX);
+        Integer extractMatcherGroupIndex = Integer.getInteger(
+            ZOOKEEPER_X509AUTHENTICATIONPROVIDER_CLIENT_CERT_ID_SAN_EXTRACT_MATCHER_GROUP_INDEX);
         LOG.info("X509AuthenticationProvider::matchAndExtractSAN(): Using SAN in the client cert "
-                + "for client ID! matchType: {}, matchRegex: {}, extractRegex: {}", matchType,
-            matchRegex, extractRegex);
+                + "for client ID! matchType: {}, matchRegex: {}, extractRegex: {}, "
+                + "extractMatcherGroupIndex: {}", matchType, matchRegex, extractRegex,
+            extractMatcherGroupIndex);
         if (matchType == null || matchRegex == null || extractRegex == null || matchType < 0
             || matchType > 8) {
             // SAN extension must be in the range of [0, 8].
@@ -259,9 +264,12 @@ private String matchAndExtractSAN(X509Certificate clientCert)
         Pattern extractPattern = Pattern.compile(extractRegex);
         Matcher matcher = extractPattern.matcher(matched.iterator().next().get(1).toString());
         if (matcher.find()) {
-            String result = matcher.group();
+            // If extractMatcherGroupIndex is not given, return the 1st index by default
+            extractMatcherGroupIndex =
+                extractMatcherGroupIndex == null ? 1 : extractMatcherGroupIndex;
+            String result = matcher.group(extractMatcherGroupIndex);
             LOG.info("X509AuthenticationProvider::matchAndExtractSAN(): returning extracted "
-                + "client ID: {}", result);
+                + "client ID: {} using Matcher group index: {}", result, extractMatcherGroupIndex);
             return result;
         }
         String errStr = "X509AuthenticationProvider::matchAndExtractSAN(): failed to find an "
diff --git a/zookeeper-server/src/test/java/org/apache/zookeeper/test/X509AuthTest.java b/zookeeper-server/src/test/java/org/apache/zookeeper/test/X509AuthTest.java
@@ -43,6 +43,8 @@
 import javax.net.ssl.X509KeyManager;
 import javax.net.ssl.X509TrustManager;
 import javax.security.auth.x500.X500Principal;
+
+import com.google.common.annotations.VisibleForTesting;
 import org.apache.zookeeper.KeeperException;
 import org.apache.zookeeper.ZKTestCase;
 import org.apache.zookeeper.server.MockServerCnxn;
@@ -94,35 +96,52 @@ public void testUntrustedAuth() {
 
     @Test
     public void testSANBasedAuth() {
+        String clientCertIdType = "SAN";
+        String clientCertIdSANMatchType = "6";
+        // The following clientCertIdSANMatchRegex matches the entire SAN String
+        String clientCertIdSANMatchRegex = ".*";
+        // TEST_SAN_STR = "a:b:c(d;e;f)" in the test. The following clientCertIdSANExtractRegex
+        // extracts the first element in the parentheses excluding "a:b:c(" and trailing ";*"
+        String clientCertIdSANExtractRegex = "^a:b:c\\((.+);.+;.+\\)$";
+        // The following clientCertIdSANExtractMatcherGroupIndex specifies the first index in the
+        // Matcher group, which is "d"
+        String clientCertIdSANExtractMatcherGroupIndex = "1";
+        String expectedClientIdFromSANExtraction = "d";
+
         // Set JVM properties to enable SAN-based client id extraction
         System.setProperty(
-            X509AuthenticationProvider.ZOOKEEPER_X509AUTHENTICATIONPROVIDER_CLIENTCERTIDTYPE,
-            "SAN");
+            X509AuthenticationProvider.ZOOKEEPER_X509AUTHENTICATIONPROVIDER_CLIENT_CERT_ID_TYPE,
+            clientCertIdType);
         System.setProperty(
-            X509AuthenticationProvider.ZOOKEEPER_X509AUTHENTICATIONPROVIDER_CLIENTCERTIDSANMATCHTYPE,
-            "6");
+            X509AuthenticationProvider.ZOOKEEPER_X509AUTHENTICATIONPROVIDER_CLIENT_CERT_ID_SAN_MATCH_TYPE,
+            clientCertIdSANMatchType);
         System.setProperty(
-            X509AuthenticationProvider.ZOOKEEPER_X509AUTHENTICATIONPROVIDER_CLIENTCERTIDSANMATCHREGEX,
-            TestCertificate.TEST_SAN_STR);
+            X509AuthenticationProvider.ZOOKEEPER_X509AUTHENTICATIONPROVIDER_CLIENT_CERT_ID_SAN_MATCH_REGEX,
+            clientCertIdSANMatchRegex);
         System.setProperty(
-            X509AuthenticationProvider.ZOOKEEPER_X509AUTHENTICATIONPROVIDER_CLIENTCERTIDSANEXTRACTREGEX,
-            ".*");
+            X509AuthenticationProvider.ZOOKEEPER_X509AUTHENTICATIONPROVIDER_CLIENT_CERT_ID_SAN_EXTRACT_REGEX,
+            clientCertIdSANExtractRegex);
+        System.setProperty(
+            X509AuthenticationProvider.ZOOKEEPER_X509AUTHENTICATIONPROVIDER_CLIENT_CERT_ID_SAN_EXTRACT_MATCHER_GROUP_INDEX,
+            clientCertIdSANExtractMatcherGroupIndex);
 
         X509AuthenticationProvider provider = createProvider(clientCert);
         MockServerCnxn cnxn = new MockServerCnxn();
         cnxn.clientChain = new X509Certificate[]{clientCert};
         assertEquals(KeeperException.Code.OK, provider.handleAuthentication(cnxn, null));
-        assertEquals(TestCertificate.TEST_SAN_STR, cnxn.getAuthInfo().get(0).getId());
+        assertEquals(expectedClientIdFromSANExtraction, cnxn.getAuthInfo().get(0).getId());
 
-        // Remove JVM properties
+        // Remove JVM properties so they don't interfere with other tests
+        System.clearProperty(
+            X509AuthenticationProvider.ZOOKEEPER_X509AUTHENTICATIONPROVIDER_CLIENT_CERT_ID_TYPE);
         System.clearProperty(
-            X509AuthenticationProvider.ZOOKEEPER_X509AUTHENTICATIONPROVIDER_CLIENTCERTIDTYPE);
+            X509AuthenticationProvider.ZOOKEEPER_X509AUTHENTICATIONPROVIDER_CLIENT_CERT_ID_SAN_MATCH_TYPE);
         System.clearProperty(
-            X509AuthenticationProvider.ZOOKEEPER_X509AUTHENTICATIONPROVIDER_CLIENTCERTIDSANMATCHTYPE);
+            X509AuthenticationProvider.ZOOKEEPER_X509AUTHENTICATIONPROVIDER_CLIENT_CERT_ID_SAN_MATCH_REGEX);
         System.clearProperty(
-            X509AuthenticationProvider.ZOOKEEPER_X509AUTHENTICATIONPROVIDER_CLIENTCERTIDSANMATCHREGEX);
+            X509AuthenticationProvider.ZOOKEEPER_X509AUTHENTICATIONPROVIDER_CLIENT_CERT_ID_SAN_EXTRACT_REGEX);
         System.clearProperty(
-            X509AuthenticationProvider.ZOOKEEPER_X509AUTHENTICATIONPROVIDER_CLIENTCERTIDSANEXTRACTREGEX);
+            X509AuthenticationProvider.ZOOKEEPER_X509AUTHENTICATIONPROVIDER_CLIENT_CERT_ID_SAN_EXTRACT_MATCHER_GROUP_INDEX);
     }
 
     private static class TestPublicKey implements PublicKey {
@@ -144,7 +163,8 @@ public byte[] getEncoded() {
     }
 
     private static class TestCertificate extends X509Certificate {
-        static final String TEST_SAN_STR = "test_san_userPrincipal";
+        @VisibleForTesting
+        static final String TEST_SAN_STR = "a:b:c(d;e;f)";
         private byte[] encoded;
         private X500Principal principal;
         private PublicKey publicKey;
