From 2217705a7ce821fa9ddfb44e4079320d098cb8cb Mon Sep 17 00:00:00 2001 From: Ioannis Kakavas Date: Mon, 6 Jul 2020 03:02:25 +0300 Subject: [PATCH] Map only specific type of OIDC Claims (#58524) 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 | 69 ++++++++------- .../authc/oidc/OpenIdConnectRealmTests.java | 85 +++++++++++++++++-- 3 files changed, 113 insertions(+), 44 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..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 @@ -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,22 @@ public String toString() { return name; } + 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 && + ((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"); + } + return values; + } + static ClaimParser forSetting(Logger logger, OpenIdConnectRealmSettings.ClaimSetting setting, RealmConfig realmConfig, boolean required) { @@ -423,19 +444,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) { - values = List.of((String) claimValueObject); - } else if (claimValueObject instanceof List) { - 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()); - } + Collection 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); @@ -459,21 +469,10 @@ static ClaimParser forSetting(Logger logger, OpenIdConnectRealmSettings.ClaimSet } else { 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) { - 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()); - }); + 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()) 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 257ab6c226ae7..1f8c291e20005 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,6 +11,7 @@ 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.util.concurrent.ThreadContext; import org.elasticsearch.env.Environment; @@ -85,7 +86,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 +105,68 @@ 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 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, 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 { + 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 +177,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 +195,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 +397,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 +423,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 +433,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);