From faa1b0b4b4b8304d4d34e65635ac94b61944c0f5 Mon Sep 17 00:00:00 2001 From: Craig Perkins Date: Wed, 21 May 2025 11:48:32 -0400 Subject: [PATCH 1/7] Handle roles in nested claim for JWT auth backends Signed-off-by: Craig Perkins --- .../JwtAuthenticationNestedClaimsTests.java | 174 ++++++++++++++++++ .../security/http/JwtAuthenticationTests.java | 3 +- .../JwtAuthenticationWithUrlParamTests.java | 2 +- .../http/JwtAuthorizationHeaderFactory.java | 32 +++- .../test/framework/JwtConfigBuilder.java | 9 +- .../jwt/AbstractHTTPJwtAuthenticator.java | 20 +- .../auth/http/jwt/HTTPJwtAuthenticator.java | 73 +++++--- 7 files changed, 272 insertions(+), 41 deletions(-) create mode 100644 src/integrationTest/java/org/opensearch/security/http/JwtAuthenticationNestedClaimsTests.java diff --git a/src/integrationTest/java/org/opensearch/security/http/JwtAuthenticationNestedClaimsTests.java b/src/integrationTest/java/org/opensearch/security/http/JwtAuthenticationNestedClaimsTests.java new file mode 100644 index 0000000000..525eec37a4 --- /dev/null +++ b/src/integrationTest/java/org/opensearch/security/http/JwtAuthenticationNestedClaimsTests.java @@ -0,0 +1,174 @@ +/* +* Copyright OpenSearch Contributors +* SPDX-License-Identifier: Apache-2.0 +* +* The OpenSearch Contributors require contributions made to +* this file be licensed under the Apache-2.0 license or a +* compatible open source license. +* +*/ +package org.opensearch.security.http; + +import java.security.KeyPair; +import java.util.Arrays; +import java.util.Base64; +import java.util.HashMap; +import java.util.List; +import java.util.Map; + +import com.carrotsearch.randomizedtesting.annotations.ThreadLeakScope; +import org.apache.hc.core5.http.Header; +import org.junit.BeforeClass; +import org.junit.ClassRule; +import org.junit.Rule; +import org.junit.Test; +import org.junit.runner.RunWith; + +import org.opensearch.test.framework.JwtConfigBuilder; +import org.opensearch.test.framework.TestSecurityConfig; +import org.opensearch.test.framework.TestSecurityConfig.Role; +import org.opensearch.test.framework.cluster.ClusterManager; +import org.opensearch.test.framework.cluster.LocalCluster; +import org.opensearch.test.framework.cluster.TestRestClient; +import org.opensearch.test.framework.cluster.TestRestClient.HttpResponse; +import org.opensearch.test.framework.log.LogsRule; +import org.opensearch.transport.client.Client; + +import io.jsonwebtoken.SignatureAlgorithm; +import io.jsonwebtoken.security.Keys; + +import static java.nio.charset.StandardCharsets.US_ASCII; +import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.containsInAnyOrder; +import static org.hamcrest.Matchers.equalTo; +import static org.hamcrest.Matchers.hasSize; +import static org.opensearch.action.support.WriteRequest.RefreshPolicy.IMMEDIATE; +import static org.opensearch.security.Song.SONGS; +import static org.opensearch.security.http.JwtAuthenticationTests.POINTER_BACKEND_ROLES; +import static org.opensearch.security.http.JwtAuthenticationTests.POINTER_USERNAME; +import static org.opensearch.test.framework.TestSecurityConfig.AuthcDomain.AUTHC_HTTPBASIC_INTERNAL; +import static org.opensearch.test.framework.TestSecurityConfig.AuthcDomain.BASIC_AUTH_DOMAIN_ORDER; +import static org.opensearch.test.framework.TestSecurityConfig.Role.ALL_ACCESS; + +@RunWith(com.carrotsearch.randomizedtesting.RandomizedRunner.class) +@ThreadLeakScope(ThreadLeakScope.Scope.NONE) +public class JwtAuthenticationNestedClaimsTests { + + public static final String CLAIM_USERNAME = "preferred-username"; + public static final List CLAIM_ROLES = List.of("attributes", "roles"); + + public static final String USER_SUPERHERO = "superhero"; + public static final String ROLE_VP = "role_vp"; + + public static final String QA_DEPARTMENT = "qa-department"; + + public static final String CLAIM_DEPARTMENT = "department"; + + public static final String DEPARTMENT_SONG_INDEX_PATTERN = String.format("song_lyrics_${attr.jwt.%s}", CLAIM_DEPARTMENT); + + public static final String QA_SONG_INDEX_NAME = String.format("song_lyrics_%s", QA_DEPARTMENT); + + private static final KeyPair KEY_PAIR1 = Keys.keyPairFor(SignatureAlgorithm.RS256); + private static final String PUBLIC_KEY1 = new String(Base64.getEncoder().encode(KEY_PAIR1.getPublic().getEncoded()), US_ASCII); + + private static final KeyPair KEY_PAIR2 = Keys.keyPairFor(SignatureAlgorithm.RS256); + private static final String PUBLIC_KEY2 = new String(Base64.getEncoder().encode(KEY_PAIR2.getPublic().getEncoded()), US_ASCII); + + static final TestSecurityConfig.User ADMIN_USER = new TestSecurityConfig.User("admin").roles(ALL_ACCESS); + + private static final String JWT_AUTH_HEADER = "jwt-auth"; + + private static final JwtAuthorizationHeaderFactory tokenFactory1 = new JwtAuthorizationHeaderFactory( + KEY_PAIR1.getPrivate(), + CLAIM_USERNAME, + CLAIM_ROLES, + JWT_AUTH_HEADER + ); + public static final TestSecurityConfig.AuthcDomain JWT_AUTH_DOMAIN = new TestSecurityConfig.AuthcDomain( + "jwt", + BASIC_AUTH_DOMAIN_ORDER - 1 + ).jwtHttpAuthenticator( + new JwtConfigBuilder().jwtHeader(JWT_AUTH_HEADER) + .signingKey(List.of(PUBLIC_KEY1, PUBLIC_KEY2)) + .subjectKey(CLAIM_USERNAME) + .rolesKey(CLAIM_ROLES) + ).backend("noop"); + public static final String SONG_ID_1 = "song-id-01"; + + public static final Role DEPARTMENT_SONG_LISTENER_ROLE = new Role("department-song-listener-role").indexPermissions( + "indices:data/read/search" + ).on(DEPARTMENT_SONG_INDEX_PATTERN); + + @ClassRule + public static final LocalCluster cluster = new LocalCluster.Builder().clusterManager(ClusterManager.SINGLENODE) + .anonymousAuth(false) + .nodeSettings( + Map.of("plugins.security.restapi.roles_enabled", List.of("user_" + ADMIN_USER.getName() + "__" + ALL_ACCESS.getName())) + ) + .authc(AUTHC_HTTPBASIC_INTERNAL) + .users(ADMIN_USER) + .roles(DEPARTMENT_SONG_LISTENER_ROLE) + .authc(JWT_AUTH_DOMAIN) + .build(); + + @Rule + public LogsRule logsRule = new LogsRule("org.opensearch.security.auth.http.jwt.HTTPJwtAuthenticator"); + + @BeforeClass + public static void createTestData() { + try (Client client = cluster.getInternalNodeClient()) { + client.prepareIndex(QA_SONG_INDEX_NAME).setId(SONG_ID_1).setRefreshPolicy(IMMEDIATE).setSource(SONGS[0].asMap()).get(); + } + try (TestRestClient client = cluster.getRestClient(ADMIN_USER)) { + client.createRoleMapping(ROLE_VP, DEPARTMENT_SONG_LISTENER_ROLE.getName()); + } + } + + // TODO write tests for scenarios where roles are in nested claim. i.e. rolesKey: ['attributes', 'roles'] + @Test + public void shouldAuthenticateWithNestedRolesClaim() { + // Create nested claims structure + Map attributes = new HashMap<>(); + List rolesClaim = Arrays.asList("all_access", "securitymanager"); + attributes.put("roles", rolesClaim); + + Map nestedClaims = new HashMap<>(); + nestedClaims.put("attributes", attributes); + + // Generate token with nested claims + Header header = tokenFactory1.generateValidTokenWithCustomClaims(USER_SUPERHERO, null, nestedClaims); + + try (TestRestClient client = cluster.getRestClient(header)) { + HttpResponse response = client.getAuthInfo(); + + response.assertStatusCode(200); + String username = response.getTextFromJsonBody(POINTER_USERNAME); + assertThat(username, equalTo(USER_SUPERHERO)); + List roles = response.getTextArrayFromJsonBody(POINTER_BACKEND_ROLES); + assertThat(roles, hasSize(2)); + assertThat(roles, containsInAnyOrder("all_access", "securitymanager")); + } + } + + @Test + public void shouldHandleMissingNestedRolesClaim() { + // Create invalid nested claims structure + Map attributes = new HashMap<>(); + attributes.put("wrong", "missing"); // Invalid format - should be a list + + Map nestedClaims = new HashMap<>(); + nestedClaims.put("attributes", attributes); + + Header header = tokenFactory1.generateValidTokenWithCustomClaims(USER_SUPERHERO, null, nestedClaims); + + try (TestRestClient client = cluster.getRestClient(header)) { + HttpResponse response = client.getAuthInfo(); + + response.assertStatusCode(200); + String username = response.getTextFromJsonBody(POINTER_USERNAME); + assertThat(username, equalTo(USER_SUPERHERO)); + List roles = response.getTextArrayFromJsonBody(POINTER_BACKEND_ROLES); + assertThat(roles, hasSize(0)); + } + } +} diff --git a/src/integrationTest/java/org/opensearch/security/http/JwtAuthenticationTests.java b/src/integrationTest/java/org/opensearch/security/http/JwtAuthenticationTests.java index afd3e52534..6173dd7c55 100644 --- a/src/integrationTest/java/org/opensearch/security/http/JwtAuthenticationTests.java +++ b/src/integrationTest/java/org/opensearch/security/http/JwtAuthenticationTests.java @@ -69,7 +69,7 @@ public class JwtAuthenticationTests { public static final String CLAIM_USERNAME = "preferred-username"; - public static final String CLAIM_ROLES = "backend-user-roles"; + public static final List CLAIM_ROLES = List.of("backend-user-roles"); public static final String USER_SUPERHERO = "superhero"; public static final String USERNAME_ROOT = "root"; @@ -305,5 +305,4 @@ public void secondKeypairShouldAuthenticateWithJwtToken_positiveWithAnotherUsern assertThat(username, equalTo(USERNAME_ROOT)); } } - } diff --git a/src/integrationTest/java/org/opensearch/security/http/JwtAuthenticationWithUrlParamTests.java b/src/integrationTest/java/org/opensearch/security/http/JwtAuthenticationWithUrlParamTests.java index d48e64bcda..788abb1432 100644 --- a/src/integrationTest/java/org/opensearch/security/http/JwtAuthenticationWithUrlParamTests.java +++ b/src/integrationTest/java/org/opensearch/security/http/JwtAuthenticationWithUrlParamTests.java @@ -51,7 +51,7 @@ public class JwtAuthenticationWithUrlParamTests { public static final String CLAIM_USERNAME = "preferred-username"; - public static final String CLAIM_ROLES = "backend-user-roles"; + public static final List CLAIM_ROLES = List.of("backend-user-roles"); public static final String POINTER_USERNAME = "/user_name"; private static final KeyPair KEY_PAIR = Keys.keyPairFor(SignatureAlgorithm.RS256); diff --git a/src/integrationTest/java/org/opensearch/security/http/JwtAuthorizationHeaderFactory.java b/src/integrationTest/java/org/opensearch/security/http/JwtAuthorizationHeaderFactory.java index b19900186c..b5d22747d6 100644 --- a/src/integrationTest/java/org/opensearch/security/http/JwtAuthorizationHeaderFactory.java +++ b/src/integrationTest/java/org/opensearch/security/http/JwtAuthorizationHeaderFactory.java @@ -10,11 +10,10 @@ package org.opensearch.security.http; import java.security.PrivateKey; -import java.util.Arrays; import java.util.Date; import java.util.HashMap; +import java.util.List; import java.util.Map; -import java.util.stream.Collectors; import com.google.common.collect.ImmutableMap; import org.apache.commons.lang3.StringUtils; @@ -33,11 +32,11 @@ class JwtAuthorizationHeaderFactory { private final String usernameClaimName; - private final String rolesClaimName; + private final List rolesClaimName; private final String headerName; - public JwtAuthorizationHeaderFactory(PrivateKey privateKey, String usernameClaimName, String rolesClaimName, String headerName) { + public JwtAuthorizationHeaderFactory(PrivateKey privateKey, String usernameClaimName, List rolesClaimName, String headerName) { this.privateKey = requireNonNull(privateKey, "Private key is required"); this.usernameClaimName = requireNonNull(usernameClaimName, "Username claim name is required"); this.rolesClaimName = requireNonNull(rolesClaimName, "Roles claim name is required."); @@ -56,6 +55,7 @@ Header generateValidToken(String username, String... roles) { .setExpiration(new Date(now.getTime() + 3600 * 1000)) .signWith(privateKey, RS256) .compact(); + System.out.println("Generated token: " + token); return toHeader(token); } @@ -64,8 +64,28 @@ private Map customClaimsMap(String username, String[] roles) { if (StringUtils.isNoneEmpty(username)) { builder.put(usernameClaimName, username); } - if ((roles != null) && (roles.length > 0)) { - builder.put(rolesClaimName, Arrays.stream(roles).collect(Collectors.joining(","))); + if (roles != null && roles.length > 0) { + if (rolesClaimName.size() == 1) { + // Simple case - no nesting + builder.put(rolesClaimName.get(0), String.join(",", roles)); + } else { + // Handle nested claims + Map nestedMap = new HashMap<>(); + Map currentMap = nestedMap; + + // Build the nested structure + for (int i = 0; i < rolesClaimName.size() - 1; i++) { + Map nextMap = new HashMap<>(); + currentMap.put(rolesClaimName.get(i), nextMap); + currentMap = nextMap; + } + + // Add the roles array at the deepest level + currentMap.put(rolesClaimName.get(rolesClaimName.size() - 1), String.join(",", roles)); + + // Add the entire nested structure to the builder + builder.putAll(nestedMap); + } } return builder.build(); } diff --git a/src/integrationTest/java/org/opensearch/test/framework/JwtConfigBuilder.java b/src/integrationTest/java/org/opensearch/test/framework/JwtConfigBuilder.java index f5984bad8f..871419d2bf 100644 --- a/src/integrationTest/java/org/opensearch/test/framework/JwtConfigBuilder.java +++ b/src/integrationTest/java/org/opensearch/test/framework/JwtConfigBuilder.java @@ -22,7 +22,7 @@ public class JwtConfigBuilder { private String jwtUrlParameter; private List signingKeys; private String subjectKey; - private String rolesKey; + private List rolesKey; public JwtConfigBuilder jwtHeader(String jwtHeader) { this.jwtHeader = jwtHeader; @@ -45,6 +45,11 @@ public JwtConfigBuilder subjectKey(String subjectKey) { } public JwtConfigBuilder rolesKey(String rolesKey) { + this.rolesKey = List.of(rolesKey); + return this; + } + + public JwtConfigBuilder rolesKey(List rolesKey) { this.rolesKey = rolesKey; return this; } @@ -64,7 +69,7 @@ public Map build() { if (isNoneBlank(subjectKey)) { builder.put("subject_key", subjectKey); } - if (isNoneBlank(rolesKey)) { + if (rolesKey != null && !rolesKey.isEmpty()) { builder.put("roles_key", rolesKey); } return builder.build(); diff --git a/src/main/java/org/opensearch/security/auth/http/jwt/AbstractHTTPJwtAuthenticator.java b/src/main/java/org/opensearch/security/auth/http/jwt/AbstractHTTPJwtAuthenticator.java index 6a793edcfb..f79f61d3e8 100644 --- a/src/main/java/org/opensearch/security/auth/http/jwt/AbstractHTTPJwtAuthenticator.java +++ b/src/main/java/org/opensearch/security/auth/http/jwt/AbstractHTTPJwtAuthenticator.java @@ -61,7 +61,7 @@ public abstract class AbstractHTTPJwtAuthenticator implements HTTPAuthenticator private final boolean isDefaultAuthHeader; private final String jwtUrlParameter; private final String subjectKey; - private final String rolesKey; + private final List rolesKey; private final List requiredAudience; private final String requiredIssuer; @@ -72,7 +72,7 @@ public AbstractHTTPJwtAuthenticator(Settings settings, Path configPath) { jwtUrlParameter = settings.get("jwt_url_parameter"); jwtHeaderName = settings.get("jwt_header", AUTHORIZATION); isDefaultAuthHeader = AUTHORIZATION.equalsIgnoreCase(jwtHeaderName); - rolesKey = settings.get("roles_key"); + rolesKey = settings.getAsList("roles_key"); subjectKey = settings.get("subject_key"); clockSkewToleranceSeconds = settings.getAsInt("jwt_clock_skew_tolerance_seconds", DEFAULT_CLOCK_SKEW_TOLERANCE_SECONDS); requiredAudience = settings.getAsList("required_audience"); @@ -219,7 +219,21 @@ public String[] extractRoles(JWTClaimsSet claims) { return new String[0]; } - Object rolesObject = claims.getClaim(rolesKey); + Object rolesObject = null; + Map claimsMap = claims.getClaims(); + for (int i = 0; i < rolesKey.size(); i++) { + if (i == rolesKey.size() - 1) { + rolesObject = claimsMap.get(rolesKey.get(i)); + } else if (claimsMap.get(rolesKey.get(i)) instanceof Map) { + claimsMap = (Map) claimsMap.get(rolesKey.get(i)); + } else { + log.warn( + "Failed to get roles from JWT claims with roles_key '{}'. Check if this key is correct and available in the JWT payload.", + rolesKey + ); + return new String[0]; + } + } if (rolesObject == null) { log.warn( diff --git a/src/main/java/org/opensearch/security/auth/http/jwt/HTTPJwtAuthenticator.java b/src/main/java/org/opensearch/security/auth/http/jwt/HTTPJwtAuthenticator.java index 2142d2e0ec..177a85b780 100644 --- a/src/main/java/org/opensearch/security/auth/http/jwt/HTTPJwtAuthenticator.java +++ b/src/main/java/org/opensearch/security/auth/http/jwt/HTTPJwtAuthenticator.java @@ -15,11 +15,14 @@ import java.security.AccessController; import java.security.PrivilegedAction; import java.util.ArrayList; +import java.util.Arrays; import java.util.Collection; import java.util.Collections; +import java.util.LinkedHashSet; import java.util.List; import java.util.Map; import java.util.Map.Entry; +import java.util.Objects; import java.util.Optional; import java.util.Set; import java.util.regex.Pattern; @@ -60,7 +63,7 @@ public class HTTPJwtAuthenticator implements HTTPAuthenticator { private final String jwtHeaderName; private final boolean isDefaultAuthHeader; private final String jwtUrlParameter; - private final String rolesKey; + private final List rolesKey; private final String subjectKey; private final List requiredAudience; private final String requireIssuer; @@ -74,7 +77,7 @@ public HTTPJwtAuthenticator(final Settings settings, final Path configPath) { jwtUrlParameter = settings.get("jwt_url_parameter"); jwtHeaderName = settings.get("jwt_header", AUTHORIZATION); isDefaultAuthHeader = AUTHORIZATION.equalsIgnoreCase(jwtHeaderName); - rolesKey = settings.get("roles_key"); + rolesKey = settings.getAsList("roles_key"); subjectKey = settings.get("subject_key"); requiredAudience = settings.getAsList("required_audience"); requireIssuer = settings.get("required_issuer"); @@ -181,7 +184,7 @@ private AuthCredentials extractCredentials0(final SecurityRequest request) { return null; } - final String[] roles = extractRoles(claims, request); + final String[] roles = extractRoles(claims); final AuthCredentials ac = new AuthCredentials(subject, roles).markComplete(); @@ -273,40 +276,56 @@ protected String extractSubject(final Claims claims, final SecurityRequest reque } @SuppressWarnings("unchecked") - protected String[] extractRoles(final Claims claims, final SecurityRequest request) { - // no roles key specified - if (rolesKey == null) { + protected String[] extractRoles(final Claims claims) { + + // Nothing configured → nothing to extract + if (rolesKey == null || rolesKey.isEmpty()) { return new String[0]; } - // try to get roles from claims, first as Object to avoid having to catch the ExpectedTypeException - final Object rolesObject = claims.get(rolesKey, Object.class); - if (rolesObject == null) { - log.warn( - "Failed to get roles from JWT claims with roles_key '{}'. Check if this key is correct and available in the JWT payload.", - rolesKey - ); - return new String[0]; + + // ── 1. Traverse the nested structure ─────────────────────────────────────── + Object node = claims; // start at root + for (String key : rolesKey) { + if (!(node instanceof Map map)) { // unexpected shape + log.warn( + "While following roles_key path {}, expected a JSON object before '{}', " + "but found '{}' ({}).", + rolesKey, + key, + node, + node.getClass() + ); + return new String[0]; + } + node = map.get(key); + if (node == null) { // key missing + log.warn("Failed to find '{}' in JWT claims while following roles_key path {}.", key, rolesKey); + return new String[0]; + } } - String[] roles = String.valueOf(rolesObject).split(","); + // ── 2. Interpret the leaf value ──────────────────────────────────────────── + Set collected = new LinkedHashSet<>(); // dedupe + keep order + + if (node instanceof String str) { + Arrays.stream(str.split(",")) // "admin,dev" + .map(String::trim) + .filter(s -> !s.isEmpty()) + .forEach(collected::add); - // We expect a String or Collection. If we find something else, convert to String but issue a warning - if (!(rolesObject instanceof String) && !(rolesObject instanceof Collection)) { + } else if (node instanceof Collection col) { + col.stream().filter(Objects::nonNull).map(Object::toString).map(String::trim).filter(s -> !s.isEmpty()).forEach(collected::add); + + } else { // something odd log.warn( - "Expected type String or Collection for roles in the JWT for roles_key {}, but value was '{}' ({}). Will convert this value to String.", + "Expected a String or Collection at the end of roles_key path {}, " + "but found '{}' ({}). Converting to String.", rolesKey, - rolesObject, - rolesObject.getClass() + node, + node.getClass() ); - } else if (rolesObject instanceof Collection) { - roles = ((Collection) rolesObject).toArray(new String[0]); - } - - for (int i = 0; i < roles.length; i++) { - roles[i] = roles[i].trim(); + collected.add(node.toString().trim()); } - return roles; + return collected.toArray(new String[0]); } } From 87d1be851828c853874b9a47d97eaba8691bf57d Mon Sep 17 00:00:00 2001 From: Craig Perkins Date: Wed, 21 May 2025 11:50:20 -0400 Subject: [PATCH 2/7] Remove sysout Signed-off-by: Craig Perkins --- .../opensearch/security/http/JwtAuthorizationHeaderFactory.java | 1 - 1 file changed, 1 deletion(-) diff --git a/src/integrationTest/java/org/opensearch/security/http/JwtAuthorizationHeaderFactory.java b/src/integrationTest/java/org/opensearch/security/http/JwtAuthorizationHeaderFactory.java index b5d22747d6..d189d3062d 100644 --- a/src/integrationTest/java/org/opensearch/security/http/JwtAuthorizationHeaderFactory.java +++ b/src/integrationTest/java/org/opensearch/security/http/JwtAuthorizationHeaderFactory.java @@ -55,7 +55,6 @@ Header generateValidToken(String username, String... roles) { .setExpiration(new Date(now.getTime() + 3600 * 1000)) .signWith(privateKey, RS256) .compact(); - System.out.println("Generated token: " + token); return toHeader(token); } From 7b22f81d5fe003a0497c79449aefa5ade7a8048b Mon Sep 17 00:00:00 2001 From: Craig Perkins Date: Wed, 21 May 2025 11:53:23 -0400 Subject: [PATCH 3/7] Remove unused code Signed-off-by: Craig Perkins --- .../JwtAuthenticationNestedClaimsTests.java | 49 +------------------ 1 file changed, 1 insertion(+), 48 deletions(-) diff --git a/src/integrationTest/java/org/opensearch/security/http/JwtAuthenticationNestedClaimsTests.java b/src/integrationTest/java/org/opensearch/security/http/JwtAuthenticationNestedClaimsTests.java index 525eec37a4..47f4a9c980 100644 --- a/src/integrationTest/java/org/opensearch/security/http/JwtAuthenticationNestedClaimsTests.java +++ b/src/integrationTest/java/org/opensearch/security/http/JwtAuthenticationNestedClaimsTests.java @@ -18,7 +18,6 @@ import com.carrotsearch.randomizedtesting.annotations.ThreadLeakScope; import org.apache.hc.core5.http.Header; -import org.junit.BeforeClass; import org.junit.ClassRule; import org.junit.Rule; import org.junit.Test; @@ -26,13 +25,11 @@ import org.opensearch.test.framework.JwtConfigBuilder; import org.opensearch.test.framework.TestSecurityConfig; -import org.opensearch.test.framework.TestSecurityConfig.Role; import org.opensearch.test.framework.cluster.ClusterManager; import org.opensearch.test.framework.cluster.LocalCluster; import org.opensearch.test.framework.cluster.TestRestClient; import org.opensearch.test.framework.cluster.TestRestClient.HttpResponse; import org.opensearch.test.framework.log.LogsRule; -import org.opensearch.transport.client.Client; import io.jsonwebtoken.SignatureAlgorithm; import io.jsonwebtoken.security.Keys; @@ -42,13 +39,9 @@ import static org.hamcrest.Matchers.containsInAnyOrder; import static org.hamcrest.Matchers.equalTo; import static org.hamcrest.Matchers.hasSize; -import static org.opensearch.action.support.WriteRequest.RefreshPolicy.IMMEDIATE; -import static org.opensearch.security.Song.SONGS; import static org.opensearch.security.http.JwtAuthenticationTests.POINTER_BACKEND_ROLES; import static org.opensearch.security.http.JwtAuthenticationTests.POINTER_USERNAME; -import static org.opensearch.test.framework.TestSecurityConfig.AuthcDomain.AUTHC_HTTPBASIC_INTERNAL; import static org.opensearch.test.framework.TestSecurityConfig.AuthcDomain.BASIC_AUTH_DOMAIN_ORDER; -import static org.opensearch.test.framework.TestSecurityConfig.Role.ALL_ACCESS; @RunWith(com.carrotsearch.randomizedtesting.RandomizedRunner.class) @ThreadLeakScope(ThreadLeakScope.Scope.NONE) @@ -58,24 +51,8 @@ public class JwtAuthenticationNestedClaimsTests { public static final List CLAIM_ROLES = List.of("attributes", "roles"); public static final String USER_SUPERHERO = "superhero"; - public static final String ROLE_VP = "role_vp"; - - public static final String QA_DEPARTMENT = "qa-department"; - - public static final String CLAIM_DEPARTMENT = "department"; - - public static final String DEPARTMENT_SONG_INDEX_PATTERN = String.format("song_lyrics_${attr.jwt.%s}", CLAIM_DEPARTMENT); - - public static final String QA_SONG_INDEX_NAME = String.format("song_lyrics_%s", QA_DEPARTMENT); - private static final KeyPair KEY_PAIR1 = Keys.keyPairFor(SignatureAlgorithm.RS256); private static final String PUBLIC_KEY1 = new String(Base64.getEncoder().encode(KEY_PAIR1.getPublic().getEncoded()), US_ASCII); - - private static final KeyPair KEY_PAIR2 = Keys.keyPairFor(SignatureAlgorithm.RS256); - private static final String PUBLIC_KEY2 = new String(Base64.getEncoder().encode(KEY_PAIR2.getPublic().getEncoded()), US_ASCII); - - static final TestSecurityConfig.User ADMIN_USER = new TestSecurityConfig.User("admin").roles(ALL_ACCESS); - private static final String JWT_AUTH_HEADER = "jwt-auth"; private static final JwtAuthorizationHeaderFactory tokenFactory1 = new JwtAuthorizationHeaderFactory( @@ -88,42 +65,18 @@ public class JwtAuthenticationNestedClaimsTests { "jwt", BASIC_AUTH_DOMAIN_ORDER - 1 ).jwtHttpAuthenticator( - new JwtConfigBuilder().jwtHeader(JWT_AUTH_HEADER) - .signingKey(List.of(PUBLIC_KEY1, PUBLIC_KEY2)) - .subjectKey(CLAIM_USERNAME) - .rolesKey(CLAIM_ROLES) + new JwtConfigBuilder().jwtHeader(JWT_AUTH_HEADER).signingKey(List.of(PUBLIC_KEY1)).subjectKey(CLAIM_USERNAME).rolesKey(CLAIM_ROLES) ).backend("noop"); - public static final String SONG_ID_1 = "song-id-01"; - - public static final Role DEPARTMENT_SONG_LISTENER_ROLE = new Role("department-song-listener-role").indexPermissions( - "indices:data/read/search" - ).on(DEPARTMENT_SONG_INDEX_PATTERN); @ClassRule public static final LocalCluster cluster = new LocalCluster.Builder().clusterManager(ClusterManager.SINGLENODE) .anonymousAuth(false) - .nodeSettings( - Map.of("plugins.security.restapi.roles_enabled", List.of("user_" + ADMIN_USER.getName() + "__" + ALL_ACCESS.getName())) - ) - .authc(AUTHC_HTTPBASIC_INTERNAL) - .users(ADMIN_USER) - .roles(DEPARTMENT_SONG_LISTENER_ROLE) .authc(JWT_AUTH_DOMAIN) .build(); @Rule public LogsRule logsRule = new LogsRule("org.opensearch.security.auth.http.jwt.HTTPJwtAuthenticator"); - @BeforeClass - public static void createTestData() { - try (Client client = cluster.getInternalNodeClient()) { - client.prepareIndex(QA_SONG_INDEX_NAME).setId(SONG_ID_1).setRefreshPolicy(IMMEDIATE).setSource(SONGS[0].asMap()).get(); - } - try (TestRestClient client = cluster.getRestClient(ADMIN_USER)) { - client.createRoleMapping(ROLE_VP, DEPARTMENT_SONG_LISTENER_ROLE.getName()); - } - } - // TODO write tests for scenarios where roles are in nested claim. i.e. rolesKey: ['attributes', 'roles'] @Test public void shouldAuthenticateWithNestedRolesClaim() { From 79585cfbc1aee6e025ef5f9eb2ece21a6805e3cd Mon Sep 17 00:00:00 2001 From: Craig Perkins Date: Wed, 21 May 2025 12:26:43 -0400 Subject: [PATCH 4/7] Add to CHANGELOG Signed-off-by: Craig Perkins --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 0ed574679e..452b712335 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), - [Resource Permissions] Introduces Centralized Resource Access Control Framework ([#5281](https://github.com/opensearch-project/security/pull/5281)) - Github workflow for changelog verification ([#5318](https://github.com/opensearch-project/security/pull/5318)) - Register cluster settings listener for `plugins.security.cache.ttl_minutes` ([#5324](https://github.com/opensearch-project/security/pull/5324)) +- Handle roles in nested claim for JWT auth backends ([#5355](https://github.com/opensearch-project/security/pull/5355)) ### Changed - Use extendedPlugins in integrationTest framework for sample resource plugin testing ([#5322](https://github.com/opensearch-project/security/pull/5322)) From 23cd9847cedab4b9eb8b2222572eb697fa9b9f81 Mon Sep 17 00:00:00 2001 From: Craig Perkins Date: Thu, 22 May 2025 15:46:04 -0400 Subject: [PATCH 5/7] Increase test coverage Signed-off-by: Craig Perkins --- ...wtKeyByOpenIdConnectAuthenticatorTest.java | 22 ++++++++++ .../auth/http/jwt/keybyoidc/TestJwts.java | 40 ++++++++++++++++++- 2 files changed, 61 insertions(+), 1 deletion(-) diff --git a/src/test/java/org/opensearch/security/auth/http/jwt/keybyoidc/HTTPJwtKeyByOpenIdConnectAuthenticatorTest.java b/src/test/java/org/opensearch/security/auth/http/jwt/keybyoidc/HTTPJwtKeyByOpenIdConnectAuthenticatorTest.java index 8d01cc8a0f..debe581fa6 100644 --- a/src/test/java/org/opensearch/security/auth/http/jwt/keybyoidc/HTTPJwtKeyByOpenIdConnectAuthenticatorTest.java +++ b/src/test/java/org/opensearch/security/auth/http/jwt/keybyoidc/HTTPJwtKeyByOpenIdConnectAuthenticatorTest.java @@ -263,6 +263,28 @@ public void testRoles() { assertThat(creds.getBackendRoles(), Matchers.is(TestJwts.TEST_ROLES)); } + @Test + public void testRolesInNestedClaim() { + Settings settings = Settings.builder() + .put("openid_connect_url", mockIdpServer.getDiscoverUri()) + .putList("roles_key", TestJwts.NESTED_ROLES_CLAIM) + .put("required_issuer", TestJwts.TEST_ISSUER) + .put("required_audience", TestJwts.TEST_AUDIENCE) + .build(); + + HTTPJwtKeyByOpenIdConnectAuthenticator jwtAuth = new HTTPJwtKeyByOpenIdConnectAuthenticator(settings, null); + + AuthCredentials creds = jwtAuth.extractCredentials( + new FakeRestRequest(ImmutableMap.of("Authorization", TestJwts.MC_COY_SIGNED_NESTED_ROLES_OCT_1), new HashMap()) + .asSecurityRequest(), + null + ); + + Assert.assertNotNull(creds); + assertThat(creds.getUsername(), Matchers.is(TestJwts.MCCOY_SUBJECT)); + assertThat(creds.getBackendRoles(), Matchers.is(TestJwts.TEST_ROLES)); + } + @Test public void testExp() { Settings settings = Settings.builder().put("openid_connect_url", mockIdpServer.getDiscoverUri()).build(); diff --git a/src/test/java/org/opensearch/security/auth/http/jwt/keybyoidc/TestJwts.java b/src/test/java/org/opensearch/security/auth/http/jwt/keybyoidc/TestJwts.java index 3562ec1be1..c120bf7e45 100644 --- a/src/test/java/org/opensearch/security/auth/http/jwt/keybyoidc/TestJwts.java +++ b/src/test/java/org/opensearch/security/auth/http/jwt/keybyoidc/TestJwts.java @@ -11,6 +11,9 @@ package org.opensearch.security.auth.http.jwt.keybyoidc; +import java.util.HashMap; +import java.util.List; +import java.util.Map; import java.util.Set; import com.google.common.collect.ImmutableSet; @@ -29,6 +32,7 @@ class TestJwts { static final String ROLES_CLAIM = "roles"; + static final List NESTED_ROLES_CLAIM = List.of("attributes", "roles"); static final Set TEST_ROLES = ImmutableSet.of("role1", "role2"); static final String TEST_ROLES_STRING = String.join(",", TEST_ROLES); @@ -42,6 +46,14 @@ class TestJwts { static final JWTClaimsSet MC_COY_2 = create(MCCOY_SUBJECT, TEST_AUDIENCE, TEST_ISSUER, ROLES_CLAIM, TEST_ROLES_STRING); + static final JWTClaimsSet MC_COY_NESTED_ROLES = create( + MCCOY_SUBJECT, + TEST_AUDIENCE, + TEST_ISSUER, + NESTED_ROLES_CLAIM, + TEST_ROLES_STRING + ); + static final JWTClaimsSet MC_COY_NO_AUDIENCE = create(MCCOY_SUBJECT, null, TEST_ISSUER, ROLES_CLAIM, TEST_ROLES_STRING); static final JWTClaimsSet MC_COY_NO_ISSUER = create(MCCOY_SUBJECT, TEST_AUDIENCE, null, ROLES_CLAIM, TEST_ROLES_STRING); @@ -60,6 +72,7 @@ class TestJwts { static final String MC_COY_SIGNED_OCT_2 = createSigned(MC_COY_2, TestJwk.OCT_2); + static final String MC_COY_SIGNED_NESTED_ROLES_OCT_1 = createSigned(MC_COY_NESTED_ROLES, TestJwk.OCT_1); static final String MC_COY_SIGNED_NO_AUDIENCE_OCT_1 = createSigned(MC_COY_NO_AUDIENCE, TestJwk.OCT_1); static final String MC_COY_SIGNED_NO_ISSUER_OCT_1 = createSigned(MC_COY_NO_ISSUER, TestJwk.OCT_1); @@ -94,7 +107,32 @@ static JWTClaimsSet create(String subject, String audience, String issuer, Objec if (moreClaims != null) { for (int i = 0; i < moreClaims.length; i += 2) { - claimsBuilder.claim(String.valueOf(moreClaims[i]), moreClaims[i + 1]); + Object claimPath = moreClaims[i]; + Object claimValue = moreClaims[i + 1]; + + if (claimPath instanceof List pathParts) { + // Handle nested path specified as List + if (!pathParts.isEmpty()) { + Map nestedMap = new HashMap<>(); + Map currentMap = nestedMap; + + // Build nested structure for all but last element + for (int j = 0; j < pathParts.size() - 1; j++) { + Map nextMap = new HashMap<>(); + currentMap.put(String.valueOf(pathParts.get(j)), nextMap); + currentMap = nextMap; + } + + // Set the final value at the deepest level + currentMap.put(String.valueOf(pathParts.get(pathParts.size() - 1)), claimValue); + + // Add the top-level claim + claimsBuilder.claim(String.valueOf(pathParts.get(0)), nestedMap.get(pathParts.get(0))); + } + } else { + // Handle simple claim + claimsBuilder.claim(String.valueOf(claimPath), claimValue); + } } } From 5980e092d199720c071643f19560b2b650b0725e Mon Sep 17 00:00:00 2001 From: Craig Perkins Date: Fri, 30 May 2025 10:28:50 -0400 Subject: [PATCH 6/7] Address code review feedback Signed-off-by: Craig Perkins --- .../security/auth/http/jwt/HTTPJwtAuthenticator.java | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/src/main/java/org/opensearch/security/auth/http/jwt/HTTPJwtAuthenticator.java b/src/main/java/org/opensearch/security/auth/http/jwt/HTTPJwtAuthenticator.java index 177a85b780..8d0500b9c9 100644 --- a/src/main/java/org/opensearch/security/auth/http/jwt/HTTPJwtAuthenticator.java +++ b/src/main/java/org/opensearch/security/auth/http/jwt/HTTPJwtAuthenticator.java @@ -309,11 +309,16 @@ protected String[] extractRoles(final Claims claims) { if (node instanceof String str) { Arrays.stream(str.split(",")) // "admin,dev" .map(String::trim) - .filter(s -> !s.isEmpty()) + .filter(Predicate.not(String::isEmpty)) .forEach(collected::add); } else if (node instanceof Collection col) { - col.stream().filter(Objects::nonNull).map(Object::toString).map(String::trim).filter(s -> !s.isEmpty()).forEach(collected::add); + col.stream() + .filter(Objects::nonNull) + .map(Object::toString) + .map(String::trim) + .filter(Predicate.not(String::isEmpty)) + .forEach(collected::add); } else { // something odd log.warn( From 146c76f0d2055aeda9e233420cfe2c5a9fae6880 Mon Sep 17 00:00:00 2001 From: Craig Perkins Date: Fri, 30 May 2025 16:34:14 -0400 Subject: [PATCH 7/7] Add import Signed-off-by: Craig Perkins --- .../opensearch/security/auth/http/jwt/HTTPJwtAuthenticator.java | 1 + 1 file changed, 1 insertion(+) diff --git a/src/main/java/org/opensearch/security/auth/http/jwt/HTTPJwtAuthenticator.java b/src/main/java/org/opensearch/security/auth/http/jwt/HTTPJwtAuthenticator.java index 8d0500b9c9..51ada0f87f 100644 --- a/src/main/java/org/opensearch/security/auth/http/jwt/HTTPJwtAuthenticator.java +++ b/src/main/java/org/opensearch/security/auth/http/jwt/HTTPJwtAuthenticator.java @@ -25,6 +25,7 @@ import java.util.Objects; import java.util.Optional; import java.util.Set; +import java.util.function.Predicate; import java.util.regex.Pattern; import org.apache.http.HttpStatus;