From e93302d0d1c05b90a80bf331c70a1ec1f560eeed Mon Sep 17 00:00:00 2001 From: Ioannis Kakavas Date: Tue, 23 Jun 2020 15:42:52 +0300 Subject: [PATCH 1/7] Map only specific type of OIDC Claims This commit changes our behavior in 2 ways: - When mapping claims to user properties ( principal, email, groups, name), we only handle string and array of string type. Previously we would fail to recognize an array of other types and that would cause failures when trying to cast to String. - When adding unmapped claims to the user metadata, we only handle string, number, boolean and arrays of these. Previously, we would fail to recognize an array of other types and that would cause failures when attempting to process role mappings. For user properties that are inherently single valued, like principal(username) we continue to support arrays of strings where we select the first one in case this is being depended on by users but we plan on removing this leniency in the next major release. --- .../authentication/oidc-guide.asciidoc | 3 + .../authc/oidc/OpenIdConnectRealm.java | 41 ++++++---- .../authc/oidc/OpenIdConnectRealmTests.java | 82 +++++++++++++++++-- 3 files changed, 103 insertions(+), 23 deletions(-) diff --git a/x-pack/docs/en/security/authentication/oidc-guide.asciidoc b/x-pack/docs/en/security/authentication/oidc-guide.asciidoc index 2d063b6385deb..2683388d10725 100644 --- a/x-pack/docs/en/security/authentication/oidc-guide.asciidoc +++ b/x-pack/docs/en/security/authentication/oidc-guide.asciidoc @@ -258,6 +258,9 @@ The recommended steps for configuring OpenID Claims mapping are as follows: the errors, you should only request scopes that the OP supports, and which you intend to map to {es} user properties. + NOTE: You can only map claims with values that are strings, numbers, boolean values or an array + of the aforementioned. + . Configure the OpenID Connect realm in {es} to associate the {es} user properties (see <> below), to the name of the claims that your OP will release. In the example above, we have configured the `principal` and diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/oidc/OpenIdConnectRealm.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/oidc/OpenIdConnectRealm.java index 2189c629a5059..ff98873b28011 100644 --- a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/oidc/OpenIdConnectRealm.java +++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/oidc/OpenIdConnectRealm.java @@ -210,13 +210,9 @@ private void buildUserFromClaims(JWTClaimsSet claims, ActionListener userMetadata; if (populateUserMetadata) { - userMetadata = claims.getClaims().entrySet().stream().filter(entry -> { - /* - * We whitelist the Types that we want to parse as metadata from the Claims, explicitly filtering out {@link Date}s - */ - Object v = entry.getValue(); - return (v instanceof String || v instanceof Boolean || v instanceof Number || v instanceof Collection); - }).collect(Collectors.toUnmodifiableMap(entry -> "oidc(" + entry.getKey() + ")", Map.Entry::getValue)); + userMetadata = claims.getClaims().entrySet().stream() + .filter(entry -> isAllowedTypeForClaim(entry.getValue())) + .collect(Collectors.toUnmodifiableMap(entry -> "oidc(" + entry.getKey() + ")", Map.Entry::getValue)); } else { userMetadata = Map.of(); } @@ -385,6 +381,15 @@ public void close() { openIdConnectAuthenticator.close(); } + /* + * We only map claims that are of Type String, Boolean, or Number, or arrays that contain only these types + */ + private static boolean isAllowedTypeForClaim(Object o) { + return (o instanceof String || o instanceof Boolean || o instanceof Number + || (o instanceof Collection && ((Collection) o).stream() + .allMatch(c -> c instanceof String || c instanceof Boolean || c instanceof Number))); + } + static final class ClaimParser { private final String name; private final Function> parser; @@ -412,6 +417,15 @@ public String toString() { return name; } + private static void validateClaimType(Object claimValueObject, String settingKey) { + if (claimValueObject instanceof String == false && + claimValueObject instanceof Collection && ((Collection) claimValueObject).stream().allMatch(c -> c instanceof String) == false) { + throw new SettingsException("Setting [ " + settingKey + + " expects a claim with String or a String Array value but found a " + + claimValueObject.getClass().getName()); + } + } + static ClaimParser forSetting(Logger logger, OpenIdConnectRealmSettings.ClaimSetting setting, RealmConfig realmConfig, boolean required) { @@ -424,12 +438,14 @@ static ClaimParser forSetting(Logger logger, OpenIdConnectRealmSettings.ClaimSet + setting.name(realmConfig) + "]", claims -> { Object claimValueObject = claims.getClaim(claimName); + validateClaimType(claimValueObject, RealmSettings.getFullSettingKey(realmConfig, setting.getClaim())); List values; if (claimValueObject == null) { values = List.of(); } else if (claimValueObject instanceof String) { values = List.of((String) claimValueObject); - } else if (claimValueObject instanceof List) { + } else if (claimValueObject instanceof Collection) { + validateClaimType(claimValueObject, RealmSettings.getFullSettingKey(realmConfig, setting.getClaim())); values = (List) claimValueObject; } else { throw new SettingsException("Setting [" + RealmSettings.getFullSettingKey(realmConfig, setting.getClaim()) @@ -461,18 +477,15 @@ static ClaimParser forSetting(Logger logger, OpenIdConnectRealmSettings.ClaimSet "OpenID Connect Claim [" + claimName + "] for [" + setting.name(realmConfig) + "]", claims -> { Object claimValueObject = claims.getClaim(claimName); + validateClaimType(claimValueObject, RealmSettings.getFullSettingKey(realmConfig, setting.getClaim())); if (claimValueObject == null) { return List.of(); } else if (claimValueObject instanceof String) { return List.of((String) claimValueObject); - } else if (claimValueObject instanceof List == false) { - throw new SettingsException("Setting [" + RealmSettings.getFullSettingKey(realmConfig, setting.getClaim()) - + " expects a claim with String or a String Array value but found a " - + claimValueObject.getClass().getName()); } return ((List) claimValueObject).stream() - .filter(Objects::nonNull) - .collect(Collectors.toUnmodifiableList()); + .filter(Objects::nonNull) + .collect(Collectors.toUnmodifiableList()); }); } } else if (required) { diff --git a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/oidc/OpenIdConnectRealmTests.java b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/oidc/OpenIdConnectRealmTests.java index 73009ba84a358..380abe973d0b1 100644 --- a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/oidc/OpenIdConnectRealmTests.java +++ b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/oidc/OpenIdConnectRealmTests.java @@ -11,7 +11,9 @@ import com.nimbusds.openid.connect.sdk.Nonce; import org.elasticsearch.action.ActionListener; import org.elasticsearch.action.support.PlainActionFuture; +import org.elasticsearch.common.Nullable; import org.elasticsearch.common.settings.Settings; +import org.elasticsearch.common.settings.SettingsException; import org.elasticsearch.common.util.concurrent.ThreadContext; import org.elasticsearch.env.Environment; import org.elasticsearch.env.TestEnvironment; @@ -26,6 +28,7 @@ import org.elasticsearch.xpack.core.security.authc.RealmSettings; import org.elasticsearch.xpack.core.security.authc.oidc.OpenIdConnectRealmSettings; import org.elasticsearch.xpack.core.security.authc.support.DelegatedAuthorizationSettings; +import org.elasticsearch.xpack.core.security.support.Exceptions; import org.elasticsearch.xpack.core.security.user.User; import org.elasticsearch.xpack.security.authc.support.MockLookupRealm; import org.elasticsearch.xpack.core.security.authc.support.UserRoleMapper; @@ -85,7 +88,7 @@ public void testAuthentication() throws Exception { final boolean notPopulateMetadata = randomBoolean(); final String authenticatingRealm = randomBoolean() ? REALM_NAME : null; - AuthenticationResult result = authenticateWithOidc(principal, roleMapper, notPopulateMetadata, false, authenticatingRealm); + AuthenticationResult result = authenticateWithOidc(principal, roleMapper, notPopulateMetadata, false, authenticatingRealm, null); assertThat(result, notNullValue()); assertThat(result.getStatus(), equalTo(AuthenticationResult.Status.SUCCESS)); assertThat(result.getUser().principal(), equalTo(principal)); @@ -104,6 +107,63 @@ public void testAuthentication() throws Exception { } } + public void testClaimPropertyMapping() throws Exception { + final UserRoleMapper roleMapper = mock(UserRoleMapper.class); + final String principal = randomAlphaOfLength(12); + AtomicReference userData = new AtomicReference<>(); + doAnswer(invocation -> { + assert invocation.getArguments().length == 2; + userData.set((UserRoleMapper.UserData) invocation.getArguments()[0]); + ActionListener> listener = (ActionListener>) invocation.getArguments()[1]; + listener.onResponse(new HashSet<>(Arrays.asList("kibana_user", "role1"))); + return null; + }).when(roleMapper).resolveRoles(any(UserRoleMapper.UserData.class), any(ActionListener.class)); + Map claims = Map.of( + "groups", List.of(Map.of("key1", List.of("value1", "value2")), Map.of("key2", List.of("value1", "value2"))) + ); + Exception e = expectThrows(Exception.class, () -> authenticateWithOidc(principal, roleMapper, false, false, + REALM_NAME, claims)); + assertThat(e.getCause().getMessage(), containsString("expects a claim with String or a String Array value")); + } + + public void testClaimMetadataMapping() throws Exception { + final UserRoleMapper roleMapper = mock(UserRoleMapper.class); + final String principal = randomAlphaOfLength(12); + AtomicReference userData = new AtomicReference<>(); + doAnswer(invocation -> { + assert invocation.getArguments().length == 2; + userData.set((UserRoleMapper.UserData) invocation.getArguments()[0]); + ActionListener> listener = (ActionListener>) invocation.getArguments()[1]; + listener.onResponse(new HashSet<>(Arrays.asList("kibana_user", "role1"))); + return null; + }).when(roleMapper).resolveRoles(any(UserRoleMapper.UserData.class), any(ActionListener.class)); + Map claims = Map.of( + "string", "String", + "number", 232, + "boolean", true, + "string_array", List.of("one", "two", "three"), + "number_array", List.of(1, 2, 3), + "boolean_array", List.of(true, false, true), + "object", Map.of("key", List.of("value1", "value2")), + "object_array", List.of(Map.of("key1", List.of("value1", "value2")), Map.of("key2", List.of("value1", "value2"))) + ); + AuthenticationResult result = authenticateWithOidc(principal, roleMapper, false, false, REALM_NAME, claims); + assertThat(result, notNullValue()); + assertThat(result.getStatus(), equalTo(AuthenticationResult.Status.SUCCESS)); + assertThat(result.getUser().principal(), equalTo(principal)); + assertThat(result.getUser().email(), equalTo("cbarton@shield.gov")); + assertThat(result.getUser().fullName(), equalTo("Clinton Barton")); + assertThat(result.getUser().roles(), arrayContainingInAnyOrder("kibana_user", "role1")); + assertTrue(result.getUser().metadata().containsKey("oidc(string)")); + assertTrue(result.getUser().metadata().containsKey("oidc(number)")); + assertTrue(result.getUser().metadata().containsKey("oidc(boolean)")); + assertTrue(result.getUser().metadata().containsKey("oidc(string_array)")); + assertTrue(result.getUser().metadata().containsKey("oidc(boolean_array)")); + assertTrue(result.getUser().metadata().containsKey("oidc(number_array)")); + assertFalse(result.getUser().metadata().containsKey("oidc(object_array)")); + assertFalse(result.getUser().metadata().containsKey("oidc(object)")); + } + public void testWithAuthorizingRealm() throws Exception { final UserRoleMapper roleMapper = mock(UserRoleMapper.class); final String principal = randomAlphaOfLength(12); @@ -114,7 +174,7 @@ public void testWithAuthorizingRealm() throws Exception { return null; }).when(roleMapper).resolveRoles(any(UserRoleMapper.UserData.class), any(ActionListener.class)); final String authenticatingRealm = randomBoolean() ? REALM_NAME : null; - AuthenticationResult result = authenticateWithOidc(principal, roleMapper, randomBoolean(), true, authenticatingRealm); + AuthenticationResult result = authenticateWithOidc(principal, roleMapper, randomBoolean(), true, authenticatingRealm, null); assertThat(result, notNullValue()); assertThat(result.getStatus(), equalTo(AuthenticationResult.Status.SUCCESS)); assertThat(result.getUser().principal(), equalTo(principal)); @@ -132,7 +192,7 @@ public void testWithAuthorizingRealm() throws Exception { public void testAuthenticationWithWrongRealm() throws Exception{ final String principal = randomAlphaOfLength(12); AuthenticationResult result = authenticateWithOidc(principal, mock(UserRoleMapper.class), randomBoolean(), true, - REALM_NAME+randomAlphaOfLength(8)); + REALM_NAME + randomAlphaOfLength(8), null); assertThat(result, notNullValue()); assertThat(result.getStatus(), equalTo(AuthenticationResult.Status.CONTINUE)); } @@ -334,8 +394,8 @@ public void testBuildingAuthenticationRequestWithLoginHint() { } private AuthenticationResult authenticateWithOidc(String principal, UserRoleMapper roleMapper, boolean notPopulateMetadata, - boolean useAuthorizingRealm - ,String authenticatingRealm) + boolean useAuthorizingRealm, String authenticatingRealm, + @Nullable Map additionalClaims) throws Exception { RealmConfig.RealmIdentifier realmIdentifier = new RealmConfig.RealmIdentifier("mock", "mock_lookup"); final MockLookupRealm lookupRealm = new MockLookupRealm( @@ -360,7 +420,7 @@ private AuthenticationResult authenticateWithOidc(String principal, UserRoleMapp final OpenIdConnectRealm realm = new OpenIdConnectRealm(config, authenticator, roleMapper); initializeRealms(realm, lookupRealm); final OpenIdConnectToken token = new OpenIdConnectToken("", new State(), new Nonce(), authenticatingRealm); - final JWTClaimsSet claims = new JWTClaimsSet.Builder() + final JWTClaimsSet.Builder claimsBuilder = new JWTClaimsSet.Builder() .subject(principal) .audience("https://rp.elastic.co/cb") .expirationTime(Date.from(now().plusSeconds(3600))) @@ -370,9 +430,13 @@ private AuthenticationResult authenticateWithOidc(String principal, UserRoleMapp .claim("groups", Arrays.asList("group1", "group2", "groups3")) .claim("mail", "cbarton@shield.gov") .claim("name", "Clinton Barton") - .claim("id_token_hint", "thisis.aserialized.jwt") - .build(); - + .claim("id_token_hint", "thisis.aserialized.jwt"); + if (additionalClaims != null) { + for (Map.Entry entry : additionalClaims.entrySet()) { + claimsBuilder.claim(entry.getKey(), entry.getValue()); + } + } + final JWTClaimsSet claims = claimsBuilder.build(); doAnswer((i) -> { ActionListener listener = (ActionListener) i.getArguments()[1]; listener.onResponse(claims); From 4274675db37b510adb9fe33dae1e403034c2059f Mon Sep 17 00:00:00 2001 From: Ioannis Kakavas Date: Mon, 29 Jun 2020 17:59:33 +0300 Subject: [PATCH 2/7] address feedback --- .../security/authc/oidc/OpenIdConnectRealm.java | 17 ++++++++--------- .../authc/oidc/OpenIdConnectRealmTests.java | 9 +++++++-- 2 files changed, 15 insertions(+), 11 deletions(-) diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/oidc/OpenIdConnectRealm.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/oidc/OpenIdConnectRealm.java index ff98873b28011..d89071637d2f1 100644 --- a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/oidc/OpenIdConnectRealm.java +++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/oidc/OpenIdConnectRealm.java @@ -417,12 +417,11 @@ public String toString() { return name; } - private static void validateClaimType(Object claimValueObject, String settingKey) { - if (claimValueObject instanceof String == false && - claimValueObject instanceof Collection && ((Collection) claimValueObject).stream().allMatch(c -> c instanceof String) == false) { - throw new SettingsException("Setting [ " + settingKey - + " expects a claim with String or a String Array value but found a " - + claimValueObject.getClass().getName()); + private static void validateParsableClaim(Object claimValueObject, String settingKey) { + final boolean isStringOrCollectionOfStrings = claimValueObject instanceof String || + claimValueObject instanceof Collection && ((Collection) claimValueObject).stream().allMatch(c -> c instanceof String); + if (isStringOrCollectionOfStrings == false) { + throw new SettingsException("Setting [ " + settingKey + " expects a claim with String or a String Array value"); } } @@ -438,14 +437,14 @@ static ClaimParser forSetting(Logger logger, OpenIdConnectRealmSettings.ClaimSet + setting.name(realmConfig) + "]", claims -> { Object claimValueObject = claims.getClaim(claimName); - validateClaimType(claimValueObject, RealmSettings.getFullSettingKey(realmConfig, setting.getClaim())); + validateParsableClaim(claimValueObject, RealmSettings.getFullSettingKey(realmConfig, setting.getClaim())); List values; if (claimValueObject == null) { values = List.of(); } else if (claimValueObject instanceof String) { values = List.of((String) claimValueObject); } else if (claimValueObject instanceof Collection) { - validateClaimType(claimValueObject, RealmSettings.getFullSettingKey(realmConfig, setting.getClaim())); + validateParsableClaim(claimValueObject, RealmSettings.getFullSettingKey(realmConfig, setting.getClaim())); values = (List) claimValueObject; } else { throw new SettingsException("Setting [" + RealmSettings.getFullSettingKey(realmConfig, setting.getClaim()) @@ -477,7 +476,7 @@ static ClaimParser forSetting(Logger logger, OpenIdConnectRealmSettings.ClaimSet "OpenID Connect Claim [" + claimName + "] for [" + setting.name(realmConfig) + "]", claims -> { Object claimValueObject = claims.getClaim(claimName); - validateClaimType(claimValueObject, RealmSettings.getFullSettingKey(realmConfig, setting.getClaim())); + validateParsableClaim(claimValueObject, RealmSettings.getFullSettingKey(realmConfig, setting.getClaim())); if (claimValueObject == null) { return List.of(); } else if (claimValueObject instanceof String) { diff --git a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/oidc/OpenIdConnectRealmTests.java b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/oidc/OpenIdConnectRealmTests.java index 380abe973d0b1..9bc97951ba6db 100644 --- a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/oidc/OpenIdConnectRealmTests.java +++ b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/oidc/OpenIdConnectRealmTests.java @@ -118,12 +118,17 @@ public void testClaimPropertyMapping() throws Exception { listener.onResponse(new HashSet<>(Arrays.asList("kibana_user", "role1"))); return null; }).when(roleMapper).resolveRoles(any(UserRoleMapper.UserData.class), any(ActionListener.class)); - Map claims = Map.of( + Map claimsWithObject = Map.of( "groups", List.of(Map.of("key1", List.of("value1", "value2")), Map.of("key2", List.of("value1", "value2"))) ); + Map claimsWithNumber = Map.of( + "groups", List.of(2, "value2")); Exception e = expectThrows(Exception.class, () -> authenticateWithOidc(principal, roleMapper, false, false, - REALM_NAME, claims)); + REALM_NAME, claimsWithObject)); + Exception e2 = expectThrows(Exception.class, () -> authenticateWithOidc(principal, roleMapper, false, false, + REALM_NAME, claimsWithNumber)); assertThat(e.getCause().getMessage(), containsString("expects a claim with String or a String Array value")); + assertThat(e2.getCause().getMessage(), containsString("expects a claim with String or a String Array value")); } public void testClaimMetadataMapping() throws Exception { From f1b35898cdbfb9f2cd233510fb4b79ff727b6d5f Mon Sep 17 00:00:00 2001 From: Ioannis Kakavas Date: Mon, 29 Jun 2020 18:02:54 +0300 Subject: [PATCH 3/7] unused import --- .../xpack/security/authc/oidc/OpenIdConnectRealmTests.java | 2 -- 1 file changed, 2 deletions(-) diff --git a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/oidc/OpenIdConnectRealmTests.java b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/oidc/OpenIdConnectRealmTests.java index 9bc97951ba6db..fbb8e5f0bc8a4 100644 --- a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/oidc/OpenIdConnectRealmTests.java +++ b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/oidc/OpenIdConnectRealmTests.java @@ -13,7 +13,6 @@ import org.elasticsearch.action.support.PlainActionFuture; import org.elasticsearch.common.Nullable; import org.elasticsearch.common.settings.Settings; -import org.elasticsearch.common.settings.SettingsException; import org.elasticsearch.common.util.concurrent.ThreadContext; import org.elasticsearch.env.Environment; import org.elasticsearch.env.TestEnvironment; @@ -28,7 +27,6 @@ import org.elasticsearch.xpack.core.security.authc.RealmSettings; import org.elasticsearch.xpack.core.security.authc.oidc.OpenIdConnectRealmSettings; import org.elasticsearch.xpack.core.security.authc.support.DelegatedAuthorizationSettings; -import org.elasticsearch.xpack.core.security.support.Exceptions; import org.elasticsearch.xpack.core.security.user.User; import org.elasticsearch.xpack.security.authc.support.MockLookupRealm; import org.elasticsearch.xpack.core.security.authc.support.UserRoleMapper; From fd030f04477ba244fb0ab193f3ffdcec8c35f3b5 Mon Sep 17 00:00:00 2001 From: Ioannis Kakavas Date: Mon, 29 Jun 2020 23:43:11 +0300 Subject: [PATCH 4/7] Don't try to validate null claims --- .../xpack/security/authc/oidc/OpenIdConnectRealm.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/oidc/OpenIdConnectRealm.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/oidc/OpenIdConnectRealm.java index d89071637d2f1..7134c1c1344b8 100644 --- a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/oidc/OpenIdConnectRealm.java +++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/oidc/OpenIdConnectRealm.java @@ -437,11 +437,11 @@ static ClaimParser forSetting(Logger logger, OpenIdConnectRealmSettings.ClaimSet + setting.name(realmConfig) + "]", claims -> { Object claimValueObject = claims.getClaim(claimName); - validateParsableClaim(claimValueObject, RealmSettings.getFullSettingKey(realmConfig, setting.getClaim())); List values; if (claimValueObject == null) { values = List.of(); } else if (claimValueObject instanceof String) { + validateParsableClaim(claimValueObject, RealmSettings.getFullSettingKey(realmConfig, setting.getClaim())); values = List.of((String) claimValueObject); } else if (claimValueObject instanceof Collection) { validateParsableClaim(claimValueObject, RealmSettings.getFullSettingKey(realmConfig, setting.getClaim())); @@ -476,10 +476,10 @@ static ClaimParser forSetting(Logger logger, OpenIdConnectRealmSettings.ClaimSet "OpenID Connect Claim [" + claimName + "] for [" + setting.name(realmConfig) + "]", claims -> { Object claimValueObject = claims.getClaim(claimName); - validateParsableClaim(claimValueObject, RealmSettings.getFullSettingKey(realmConfig, setting.getClaim())); if (claimValueObject == null) { return List.of(); } else if (claimValueObject instanceof String) { + validateParsableClaim(claimValueObject, RealmSettings.getFullSettingKey(realmConfig, setting.getClaim())); return List.of((String) claimValueObject); } return ((List) claimValueObject).stream() From cb9e3f2532e1af157432ed46f213b5bc57677350 Mon Sep 17 00:00:00 2001 From: Ioannis Kakavas Date: Tue, 30 Jun 2020 01:32:54 +0300 Subject: [PATCH 5/7] fix parsing and validation logic --- .../authc/oidc/OpenIdConnectRealm.java | 46 ++++++++----------- 1 file changed, 19 insertions(+), 27 deletions(-) diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/oidc/OpenIdConnectRealm.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/oidc/OpenIdConnectRealm.java index 7134c1c1344b8..fb5c3d46353d2 100644 --- a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/oidc/OpenIdConnectRealm.java +++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/oidc/OpenIdConnectRealm.java @@ -417,12 +417,23 @@ public String toString() { return name; } - private static void validateParsableClaim(Object claimValueObject, String settingKey) { - final boolean isStringOrCollectionOfStrings = claimValueObject instanceof String || - claimValueObject instanceof Collection && ((Collection) claimValueObject).stream().allMatch(c -> c instanceof String); - if (isStringOrCollectionOfStrings == false) { + private static List parseClaimValues(JWTClaimsSet claimsSet, String claimName, String settingKey) { + List values; + final Object claimValueObject = claimsSet.getClaim(claimName); + if (claimValueObject == null) { + values = List.of(); + } else if (claimValueObject instanceof String) { + values = List.of((String) claimValueObject); + } else if (claimValueObject instanceof Collection) { + if (claimValueObject instanceof Collection && ((Collection) claimValueObject).stream().allMatch(c -> c instanceof String)) { + values = (List) claimValueObject; + } else { + throw new SettingsException("Setting [ " + settingKey + " expects a claim with String or a String Array value"); + } + } else { throw new SettingsException("Setting [ " + settingKey + " expects a claim with String or a String Array value"); } + return values; } static ClaimParser forSetting(Logger logger, OpenIdConnectRealmSettings.ClaimSetting setting, RealmConfig realmConfig, @@ -436,21 +447,8 @@ static ClaimParser forSetting(Logger logger, OpenIdConnectRealmSettings.ClaimSet "OpenID Connect Claim [" + claimName + "] with pattern [" + regex.pattern() + "] for [" + setting.name(realmConfig) + "]", claims -> { - Object claimValueObject = claims.getClaim(claimName); - List values; - if (claimValueObject == null) { - values = List.of(); - } else if (claimValueObject instanceof String) { - validateParsableClaim(claimValueObject, RealmSettings.getFullSettingKey(realmConfig, setting.getClaim())); - values = List.of((String) claimValueObject); - } else if (claimValueObject instanceof Collection) { - validateParsableClaim(claimValueObject, RealmSettings.getFullSettingKey(realmConfig, setting.getClaim())); - values = (List) claimValueObject; - } else { - throw new SettingsException("Setting [" + RealmSettings.getFullSettingKey(realmConfig, setting.getClaim()) - + " expects a claim with String or a String Array value but found a " - + claimValueObject.getClass().getName()); - } + List values = + parseClaimValues(claims, claimName, RealmSettings.getFullSettingKey(realmConfig, setting.getClaim())); return values.stream().map(s -> { if (s == null) { logger.debug("OpenID Connect Claim [{}] is null", claimName); @@ -475,14 +473,8 @@ static ClaimParser forSetting(Logger logger, OpenIdConnectRealmSettings.ClaimSet return new ClaimParser( "OpenID Connect Claim [" + claimName + "] for [" + setting.name(realmConfig) + "]", claims -> { - Object claimValueObject = claims.getClaim(claimName); - if (claimValueObject == null) { - return List.of(); - } else if (claimValueObject instanceof String) { - validateParsableClaim(claimValueObject, RealmSettings.getFullSettingKey(realmConfig, setting.getClaim())); - return List.of((String) claimValueObject); - } - return ((List) claimValueObject).stream() + return parseClaimValues(claims, claimName, RealmSettings.getFullSettingKey(realmConfig, setting.getClaim())) + .stream() .filter(Objects::nonNull) .collect(Collectors.toUnmodifiableList()); }); From ee0b57b30743c15824a5d0623b846f439605a416 Mon Sep 17 00:00:00 2001 From: Ioannis Kakavas Date: Tue, 30 Jun 2020 15:10:10 +0300 Subject: [PATCH 6/7] address feedback --- .../authc/oidc/OpenIdConnectRealm.java | 24 +++++++------------ 1 file changed, 9 insertions(+), 15 deletions(-) diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/oidc/OpenIdConnectRealm.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/oidc/OpenIdConnectRealm.java index fb5c3d46353d2..0e688840eed7f 100644 --- a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/oidc/OpenIdConnectRealm.java +++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/oidc/OpenIdConnectRealm.java @@ -417,19 +417,15 @@ public String toString() { return name; } - private static List parseClaimValues(JWTClaimsSet claimsSet, String claimName, String settingKey) { - List values; + private static Collection parseClaimValues(JWTClaimsSet claimsSet, String claimName, String settingKey) { + Collection values; final Object claimValueObject = claimsSet.getClaim(claimName); if (claimValueObject == null) { values = List.of(); } else if (claimValueObject instanceof String) { values = List.of((String) claimValueObject); - } else if (claimValueObject instanceof Collection) { - if (claimValueObject instanceof Collection && ((Collection) claimValueObject).stream().allMatch(c -> c instanceof String)) { - values = (List) claimValueObject; - } else { - throw new SettingsException("Setting [ " + settingKey + " expects a claim with String or a String Array value"); - } + } else if (claimValueObject instanceof Collection && ((Collection) claimValueObject).stream().allMatch(c -> c instanceof String)) { + values = (Collection) claimValueObject; } else { throw new SettingsException("Setting [ " + settingKey + " expects a claim with String or a String Array value"); } @@ -447,7 +443,7 @@ static ClaimParser forSetting(Logger logger, OpenIdConnectRealmSettings.ClaimSet "OpenID Connect Claim [" + claimName + "] with pattern [" + regex.pattern() + "] for [" + setting.name(realmConfig) + "]", claims -> { - List values = + Collection values = parseClaimValues(claims, claimName, RealmSettings.getFullSettingKey(realmConfig, setting.getClaim())); return values.stream().map(s -> { if (s == null) { @@ -472,12 +468,10 @@ static ClaimParser forSetting(Logger logger, OpenIdConnectRealmSettings.ClaimSet } else { return new ClaimParser( "OpenID Connect Claim [" + claimName + "] for [" + setting.name(realmConfig) + "]", - claims -> { - return parseClaimValues(claims, claimName, RealmSettings.getFullSettingKey(realmConfig, setting.getClaim())) - .stream() - .filter(Objects::nonNull) - .collect(Collectors.toUnmodifiableList()); - }); + claims -> parseClaimValues(claims, claimName, RealmSettings.getFullSettingKey(realmConfig, setting.getClaim())) + .stream() + .filter(Objects::nonNull) + .collect(Collectors.toUnmodifiableList())); } } else if (required) { throw new SettingsException("Setting [" + RealmSettings.getFullSettingKey(realmConfig, setting.getClaim()) From 2499dc23447d9a2d244493004a1141e7aa51d43c Mon Sep 17 00:00:00 2001 From: Ioannis Kakavas Date: Tue, 30 Jun 2020 15:17:15 +0300 Subject: [PATCH 7/7] checkstyle --- .../xpack/security/authc/oidc/OpenIdConnectRealm.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/oidc/OpenIdConnectRealm.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/oidc/OpenIdConnectRealm.java index 0e688840eed7f..af347f5dfb801 100644 --- a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/oidc/OpenIdConnectRealm.java +++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/oidc/OpenIdConnectRealm.java @@ -424,7 +424,8 @@ private static Collection parseClaimValues(JWTClaimsSet claimsSet, Strin values = List.of(); } else if (claimValueObject instanceof String) { values = List.of((String) claimValueObject); - } else if (claimValueObject instanceof Collection && ((Collection) claimValueObject).stream().allMatch(c -> c instanceof String)) { + } else if (claimValueObject instanceof Collection && + ((Collection) claimValueObject).stream().allMatch(c -> c instanceof String)) { values = (Collection) claimValueObject; } else { throw new SettingsException("Setting [ " + settingKey + " expects a claim with String or a String Array value");