From a9151db735083d88ffc2518d3dd8273690ac5780 Mon Sep 17 00:00:00 2001 From: Yang Wang Date: Mon, 6 Jul 2020 11:36:41 +1000 Subject: [PATCH] Map only specific type of OIDC Claims (#58524) (#59043) 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. Co-authored-by: Ioannis Kakavas --- .../authentication/oidc-guide.asciidoc | 3 + .../authc/oidc/OpenIdConnectRealm.java | 78 ++++++++-------- .../authc/oidc/OpenIdConnectRealmTests.java | 89 +++++++++++++++++-- 3 files changed, 120 insertions(+), 50 deletions(-) diff --git a/x-pack/docs/en/security/authentication/oidc-guide.asciidoc b/x-pack/docs/en/security/authentication/oidc-guide.asciidoc index 56feab8794008..7d0bddfdbf893 100644 --- a/x-pack/docs/en/security/authentication/oidc-guide.asciidoc +++ b/x-pack/docs/en/security/authentication/oidc-guide.asciidoc @@ -256,6 +256,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 579675b77c643..4cbd35af7a7ef 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 @@ -55,7 +55,6 @@ import java.util.List; import java.util.Map; import java.util.Objects; -import java.util.Set; import java.util.function.Function; import java.util.regex.Matcher; import java.util.regex.Pattern; @@ -211,19 +210,13 @@ private void buildUserFromClaims(JWTClaimsSet claims, ActionListener userMetadata = new HashMap<>(); + final Map userMetadata; if (populateUserMetadata) { - Map claimsMap = claims.getClaims(); - /* - * We whitelist the Types that we want to parse as metadata from the Claims, explicitly filtering out {@link Date}s - */ - Set allowedEntries = claimsMap.entrySet().stream().filter(entry -> { - Object v = entry.getValue(); - return (v instanceof String || v instanceof Boolean || v instanceof Number || v instanceof Collection); - }).collect(Collectors.toSet()); - for (Map.Entry entry : allowedEntries) { - userMetadata.put("oidc(" + entry.getKey() + ")", entry.getValue()); - } + userMetadata = claims.getClaims().entrySet().stream() + .filter(entry -> isAllowedTypeForClaim(entry.getValue())) + .collect(Collectors.toMap(entry -> "oidc(" + entry.getKey() + ")", Map.Entry::getValue)); + } else { + userMetadata = Collections.emptyMap(); } final List groups = groupsAttribute.getClaimValues(claims); final String dn = dnAttribute.getClaimValue(claims); @@ -390,6 +383,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; @@ -417,6 +419,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 = Collections.emptyList(); + } else if (claimValueObject instanceof String) { + values = Collections.singletonList((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) { @@ -428,19 +446,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 = Collections.emptyList(); - } else if (claimValueObject instanceof String) { - values = Collections.singletonList((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); @@ -464,21 +471,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 Collections.emptyList(); - } else if (claimValueObject instanceof String) { - return Collections.singletonList((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.toList()); - }); + claims -> parseClaimValues(claims, claimName, RealmSettings.getFullSettingKey(realmConfig, setting.getClaim())) + .stream() + .filter(Objects::nonNull) + .collect(Collectors.toList())); } } 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 fb04dfacc8a55..dbaf748dab504 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; @@ -84,7 +85,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)); @@ -103,6 +104,72 @@ 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 = org.elasticsearch.common.collect.Map.of( + "groups", org.elasticsearch.common.collect.List.of( + org.elasticsearch.common.collect.Map.of("key1", org.elasticsearch.common.collect.List.of("value1", "value2")), + org.elasticsearch.common.collect.Map.of("key2", org.elasticsearch.common.collect.List.of("value1", "value2"))) + ); + Map claimsWithNumber = org.elasticsearch.common.collect.Map.of( + "groups", org.elasticsearch.common.collect.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 = org.elasticsearch.common.collect.Map.of( + "string", "String", + "number", 232, + "boolean", true, + "string_array", org.elasticsearch.common.collect.List.of("one", "two", "three"), + "number_array", org.elasticsearch.common.collect.List.of(1, 2, 3), + "boolean_array", org.elasticsearch.common.collect.List.of(true, false, true), + "object", org.elasticsearch.common.collect.Map.of("key", org.elasticsearch.common.collect.List.of("value1", "value2")), + "object_array", org.elasticsearch.common.collect.List.of( + org.elasticsearch.common.collect.Map.of("key1", org.elasticsearch.common.collect.List.of("value1", "value2")), + org.elasticsearch.common.collect.Map.of("key2", org.elasticsearch.common.collect.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); @@ -113,7 +180,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)); @@ -131,7 +198,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)); } @@ -333,8 +400,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 { final MockLookupRealm lookupRealm = new MockLookupRealm( new RealmConfig(new RealmConfig.RealmIdentifier("mock", "mock_lookup"), globalSettings, env, threadContext)); @@ -355,7 +422,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))) @@ -365,9 +432,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);