Principal extraction from nested username claim was broken (#194)

* Principal extraction from nested username claim was broken Signed-off-by: Marko Strukelj <[email protected]> * Add proper support for nested claims Signed-off-by: Marko Strukelj <[email protected]> * Address PR comments and suggestions Signed-off-by: Marko Strukelj <[email protected]> * Remove testsuite run using Kafka 3.3.2 from Travis build Signed-off-by: Marko Strukelj <[email protected]> * Replace custom parsing with JsonPath already used for groups extraction Signed-off-by: Marko Strukelj <[email protected]> * Fix javadoc issue Signed-off-by: Marko Strukelj <[email protected]> --------- Signed-off-by: Marko Strukelj <[email protected]>
strimzi · Jul 7, 2023 · 47e76b1 · 47e76b1
1 parent a0c4c34
commit 47e76b1
Show file tree

Hide file tree

Showing 7 changed files with 283 additions and 29 deletions.
diff --git a/.travis/build.sh b/.travis/build.sh
@@ -69,14 +69,14 @@ elif [[ "$arch" != 'ppc64le' ]]; then
   EXIT=$?
   exitIfError
 
-  clearDockerEnv
-  mvn -e -V -B clean install -f testsuite -Pkafka-3_3_2
-  EXIT=$?
-  exitIfError
-
   # Excluded by default to not exceed Travis job timeout
   if [ "$SKIP_DISABLED" == "false" ]; then
 
+    clearDockerEnv
+    mvn -e -V -B clean install -f testsuite -Pkafka-3_3_2
+    EXIT=$?
+    exitIfError
+
     clearDockerEnv
     mvn -e -V -B clean install -f testsuite -Pkafka-3_2_3 -DfailIfNoTests=false -Dtest=\!KeycloakKRaftAuthorizationTests
     EXIT=$?

diff --git a/README.md b/README.md
@@ -316,12 +316,12 @@ If the configured `oauth.client.id` is `kafka`, the following are valid examples
 
 JWT tokens contain unique user identification in `sub` claim. However, this is often a long number or a UUID, but we usually prefer to use human-readable usernames, which may also be present in JWT token.
 Use `oauth.username.claim` to map the claim (attribute) where the value you want to use as user id is stored:
-- `oauth.username.claim` (e.g.: "preferred_username")
+- `oauth.username.claim` (e.g.: "preferred_username", for nested attributes use `[topAttrKey].[subAttrKey]`. Claim names can also be single quoted: `['topAttrKey'].['subAttrKey']`)
 
 If `oauth.username.claim` is specified the value of that claim is used instead, but if not set, the automatic fallback claim is the `sub` claim.
 
 You can specify the secondary claim to fall back to, which allows you to map multiple account types into the same principal namespace: 
-- `oauth.fallback.username.claim` (e.g.: "client_id")
+- `oauth.fallback.username.claim` (e.g.: "client_id", for nested attributes use `[topAttrKey].[subAttrKey]`. Claim names can also be single quoted: `['topAttrKey'].['subAttrKey']`)
 - `oauth.fallback.username.prefix` (e.g.: "client-account-")
 
 If `oauth.username.claim` is specified but value does not exist in the token, then `oauth.fallback.username.claim` is used. If value for that doesn't exist either, the exception is thrown.
@@ -400,10 +400,10 @@ Introspection Endpoint may or may not return identifying information which we co
 If the information is available we attempt to extract the user id from Introspection Endpoint response.
 
 Use `oauth.username.claim` to map the attribute where the user id is stored:
-- `oauth.username.claim` (e.g.: "preferred_username")
+- `oauth.username.claim` (e.g.: "preferred_username", for nested attributes use `[topAttrKey].[subAttrKey]`. Claim names can also be single quoted: `['topAttrKey'].['subAttrKey']`)
 
 You can fall back to a secondary attribute, which allows you to map multiple account types into the same user id namespace: 
-- `oauth.fallback.username.claim` (e.g.: "client_id")
+- `oauth.fallback.username.claim` (e.g.: "client_id", for nested attributes use `[topAttrKey].[subAttrKey]`. Claim names can also be single quoted: `['topAttrKey'].['subAttrKey']`)
 - `oauth.fallback.username.prefix` (e.g.: "client-account-")
 
 If `oauth.username.claim` is specified but value does not exist in the Introspection Endpoint response, then `oauth.fallback.username.claim` is used. If value for that doesn't exist either, the exception is thrown.
@@ -985,7 +985,7 @@ Audience is sent to the Token Endpoint when obtaining the access token.
 
 For debug purposes you may want to properly configure which JWT token attribute contains the user id of the account used to obtain the access token:
 
-- `oauth.username.claim` (e.g.: "preferred_username")
+- `oauth.username.claim` (e.g.: "preferred_username", for nested attributes use `[topAttrKey].[subAttrKey]`. Claim names can also be single quoted: `['topAttrKey'].['subAttrKey']`)
 
 This does not affect how Kafka client is presented to the Kafka Broker.
 The broker performs user id extraction from the token once again or it uses the Introspection Endpoint or the User Info Endpoint to get the user id.

diff --git a/oauth-common/src/main/java/io/strimzi/kafka/oauth/common/JSONUtil.java b/oauth-common/src/main/java/io/strimzi/kafka/oauth/common/JSONUtil.java
@@ -112,6 +112,9 @@ public static String getClaimFromJWT(String claim, Object token) {
      * @return Value of the specific claim as String or null if claim not present
      */
     public static String getClaimFromJWT(JsonNode node, String... path) {
+        if (path.length == 0) {
+            return null;
+        }
         for (String p: path) {
             node = node.get(p);
             if (node == null) {

diff --git a/oauth-common/src/main/java/io/strimzi/kafka/oauth/common/PrincipalExtractor.java b/oauth-common/src/main/java/io/strimzi/kafka/oauth/common/PrincipalExtractor.java
@@ -5,37 +5,67 @@
 package io.strimzi.kafka.oauth.common;
 
 import com.fasterxml.jackson.databind.JsonNode;
+import io.strimzi.kafka.oauth.jsonpath.JsonPathQuery;
 
 import static io.strimzi.kafka.oauth.common.JSONUtil.getClaimFromJWT;
 
 /**
  * An object with logic for extracting a principal name (i.e. a user id) from a JWT token.
- *
+ * <p>
  * First a claim configured as <code>usernameClaim</code> is looked up.
  * If not found the claim configured as <code>fallbackUsernameClaim</code> is looked up. If that one is found and if
  * the <code>fallbackUsernamePrefix</code> is configured prefix the found value with the prefix, otherwise not.
+ * <p>
+ * The claim specification uses the following rules:
+ * <ul>
+ * <li>If the claim specification starts with an opening square bracket '[', it is interpreted as a JsonPath query, and allows
+ * targeting a nested attribute. </li>
+ * <li>Otherwise, it is interpreted as a top level attribute name.</li>
+ * </ul>
+ * <p>
+ * A JsonPath query is resolved relative to JSON object containing info to identify user
+ * (a JWT payload, a response from Introspection Endpoint or a response from User Info Endpoint).
+ * <p>
+ * For more on JsonPath syntax see https://github.com/json-path/JsonPath.
+ * <p>
+ * Examples of claim specification:
+ * <pre>
+ *     userId                    ... use top level attribute named 'userId'
+ *     user.id                   ... use top level attribute named 'user.id'
+ *     $userid                   ... use top level attribute named '$userid'
+ *     ['userInfo']['id']        ... use nested attribute 'id' under 'userInfo' top level attribute
+ *     ['userInfo'].id           ... use nested attribute 'id' under 'userInfo' top level attribute (second segment not using brackets)
+ *     ['user.info']['user.id']  ... use nested attribute 'user.id' under 'user.info' top level attribute
+ *     ['user.info'].['user.id'] ... use nested attribute 'user.id' under 'user.info' top level attribute (optional dot)
+ * </pre>
+ *
+ * See PrincipalExtractorTest.java for more working and non-working examples of claim specification.
  */
 public class PrincipalExtractor {
 
-    private String usernameClaim;
-    private String fallbackUsernameClaim;
-    private String fallbackUsernamePrefix;
+    private final Extractor usernameExtractor;
+    private final Extractor fallbackUsernameExtractor;
+    private final String fallbackUsernamePrefix;
 
     /**
      * Create a new instance
      */
-    public PrincipalExtractor() {}
+    public PrincipalExtractor() {
+        usernameExtractor = null;
+        fallbackUsernameExtractor = null;
+        fallbackUsernamePrefix = null;
+    }
 
     /**
      * Create a new instance
      *
-     * @param usernameClaim Attribute name for an attribute containing the user id to lookup first
+     * @param usernameClaim Attribute name for an attribute containing the user id to lookup first.
      * @param fallbackUsernameClaim Attribute name for an attribute containg the user id to lookup as a fallback
      * @param fallbackUsernamePrefix A prefix to prepend to the value of the fallback attribute value if set
      */
     public PrincipalExtractor(String usernameClaim, String fallbackUsernameClaim, String fallbackUsernamePrefix) {
-        this.usernameClaim = usernameClaim;
-        this.fallbackUsernameClaim = fallbackUsernameClaim;
+        this.usernameExtractor = parseClaimSpec(usernameClaim);
+        this.fallbackUsernameExtractor = parseClaimSpec(fallbackUsernameClaim);
         this.fallbackUsernamePrefix = fallbackUsernamePrefix;
     }
 
@@ -48,23 +78,38 @@ public PrincipalExtractor(String usernameClaim, String fallbackUsernameClaim, St
     public String getPrincipal(JsonNode json) {
         String result;
 
-        if (usernameClaim != null) {
-            result = getClaimFromJWT(json, usernameClaim);
+        if (usernameExtractor != null) {
+            result = extractUsername(usernameExtractor, json);
             if (result != null) {
                 return result;
             }
-
-            if (fallbackUsernameClaim != null) {
-                result = getClaimFromJWT(json, fallbackUsernameClaim);
+            if (fallbackUsernameExtractor != null) {
+                result = extractUsername(fallbackUsernameExtractor, json);
                 if (result != null) {
-                    return fallbackUsernamePrefix == null ? result : fallbackUsernamePrefix + result;
+                    return result;
                 }
             }
         }
 
         return null;
     }
 
+    private String extractUsername(Extractor extractor, JsonNode json) {
+        if (extractor.getAttributeName() != null) {
+            String result = getClaimFromJWT(json, extractor.getAttributeName());
+            if (result != null && !result.isEmpty()) {
+                return result;
+            }
+        } else {
+            JsonNode queryResult = extractor.getJSONPathQuery().apply(json);
+            String result = queryResult == null ? null : queryResult.asText().trim();
+            if (result != null && !result.isEmpty()) {
+                return result;
+            }
+        }
+        return null;
+    }
+
     /**
      * Get the value of <code>sub</code> claim
      *
@@ -77,7 +122,7 @@ public String getSub(JsonNode json) {
 
     @Override
     public String toString() {
-        return "PrincipalExtractor {usernameClaim: " + usernameClaim  + ", fallbackUsernameClaim: " + fallbackUsernameClaim + ", fallbackUsernamePrefix: " + fallbackUsernamePrefix + "}";
+        return "PrincipalExtractor {usernameClaim: " + usernameExtractor + ", fallbackUsernameClaim: " + fallbackUsernameExtractor + ", fallbackUsernamePrefix: " + fallbackUsernamePrefix + "}";
     }
 
     /**
@@ -86,6 +131,66 @@ public String toString() {
      * @return True if any of the constructor parameters is set
      */
     public boolean isConfigured() {
-        return usernameClaim != null || fallbackUsernameClaim != null || fallbackUsernamePrefix != null;
+        return usernameExtractor != null || fallbackUsernameExtractor != null || fallbackUsernamePrefix != null;
+    }
+
+    /**
+     * The claim specification uses the following rules:
+     * <ul>
+     * <li>If the claim specification starts with an opening square bracket '[', it is interpreted as a JsonPath query, and allows
+     * targeting a nested attribute. </li>
+     * <li>Otherwise, it is interpreted as a top level attribute name.</li>
+     * </ul>
+     * For more on JsonPath syntax see https://github.com/json-path/JsonPath.
+     * <p>
+     * Examples of claim specification:
+     * <pre>
+     *     userId                    ... use top level attribute named 'userId'
+     *     user.id                   ... use top level attribute named 'user.id'
+     *     $userid                   ... use top level attribute named '$userid'
+     *     ['userInfo']['id']        ... use nested attribute 'id' under 'userInfo' top level attribute
+     *     ['userInfo'].id           ... use nested attribute 'id' under 'userInfo' top level attribute (second segment not using brackets)
+     *     ['user.info']['user.id']  ... use nested attribute 'user.id' under 'user.info' top level attribute
+     *     ['user.info'].['user.id'] ... use nested attribute 'user.id' under 'user.info' top level attribute (optional dot)
+     * </pre>
+     *
+     * @param spec Claim specification
+     * @return Result containing either a claim with top level attribute name or a JsonPathQuery object
+     */
+    private static Extractor parseClaimSpec(String spec) {
+        spec = spec == null ? null : spec.trim();
+        if (spec == null || spec.isEmpty()) {
+            return null;
+        }
+
+        if (!spec.startsWith("[")) {
+            return new Extractor(spec);
+        }
+
+        return new Extractor(JsonPathQuery.parse(spec));
+    }
+
+    static class Extractor {
+
+        private final String attributeName;
+        private final JsonPathQuery query;
+
+        private Extractor(JsonPathQuery query) {
+            this.query = query;
+            this.attributeName = null;
+        }
+
+        private Extractor(String attributeName) {
+            this.attributeName = attributeName;
+            this.query = null;
+        }
+
+        String getAttributeName() {
+            return attributeName;
+        }
+
+        JsonPathQuery getJSONPathQuery() {
+            return query;
+        }
     }
 }
diff --git a/oauth-common/src/main/java/io/strimzi/kafka/oauth/jsonpath/JsonPathQuery.java b/oauth-common/src/main/java/io/strimzi/kafka/oauth/jsonpath/JsonPathQuery.java
@@ -90,7 +90,7 @@ private JsonPathQuery(String query) {
         try {
             this.matcher = new Matcher(ctx, query, false);
         } catch (JsonPathException e) {
-            throw new JsonPathQueryException("Failed to parse filter query: \"" + query + "\"", e);
+            throw new JsonPathQueryException("Failed to parse JsonPath query: \"" + query + "\"", e);
         }
     }