Skip to content

Commit

Permalink
Map only specific type of OIDC Claims (elastic#58524)
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
@jkakavas @ywangd
jkakavas authored and ywangd committed Jul 6, 2020
1 parent 49f4227 commit 131a353
Show file tree
Hide file tree
Showing 3 changed files with 120 additions and 50 deletions.
3 changes: 3 additions & 0 deletions x-pack/docs/en/security/authentication/oidc-guide.asciidoc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
<<oidc-user-properties, the listing>> below), to the name of the claims that your
OP will release. In the example above, we have configured the `principal` and
Expand Down
78 changes: 37 additions & 41 deletions ...ecurity/src/main/java/org/elasticsearch/xpack/security/authc/oidc/OpenIdConnectRealm.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -211,19 +210,13 @@ private void buildUserFromClaims(JWTClaimsSet claims, ActionListener<Authenticat
return;
}

final Map<String, Object> userMetadata = new HashMap<>();
final Map<String, Object> userMetadata;
if (populateUserMetadata) {
Map<String, Object> claimsMap = claims.getClaims();
/*
* We whitelist the Types that we want to parse as metadata from the Claims, explicitly filtering out {@link Date}s
*/
Set<Map.Entry> 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<String> groups = groupsAttribute.getClaimValues(claims);
final String dn = dnAttribute.getClaimValue(claims);
Expand Down Expand Up @@ -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<JWTClaimsSet, List<String>> parser;
Expand Down Expand Up @@ -417,6 +419,22 @@ public String toString() {
return name;
}

private static Collection<String> parseClaimValues(JWTClaimsSet claimsSet, String claimName, String settingKey) {
Collection<String> 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<String>) 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) {

Expand All @@ -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<String> values;
if (claimValueObject == null) {
values = Collections.emptyList();
} else if (claimValueObject instanceof String) {
values = Collections.singletonList((String) claimValueObject);
} else if (claimValueObject instanceof List) {
values = (List<String>) 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<String> 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);
Expand All @@ -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<String>) 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())
Expand Down
89 changes: 80 additions & 9 deletions ...ty/src/test/java/org/elasticsearch/xpack/security/authc/oidc/OpenIdConnectRealmTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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));
Expand All @@ -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<UserRoleMapper.UserData> userData = new AtomicReference<>();
doAnswer(invocation -> {
assert invocation.getArguments().length == 2;
userData.set((UserRoleMapper.UserData) invocation.getArguments()[0]);
ActionListener<Set<String>> listener = (ActionListener<Set<String>>) 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<String, Object> 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<String, Object> 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<UserRoleMapper.UserData> userData = new AtomicReference<>();
doAnswer(invocation -> {
assert invocation.getArguments().length == 2;
userData.set((UserRoleMapper.UserData) invocation.getArguments()[0]);
ActionListener<Set<String>> listener = (ActionListener<Set<String>>) 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<String, Object> 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("[email protected]"));
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);
Expand All @@ -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));
Expand All @@ -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));
}
Expand Down Expand Up @@ -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<String, Object> additionalClaims)
throws Exception {
final MockLookupRealm lookupRealm = new MockLookupRealm(
new RealmConfig(new RealmConfig.RealmIdentifier("mock", "mock_lookup"), globalSettings, env, threadContext));
Expand All @@ -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)))
Expand All @@ -365,9 +432,13 @@ private AuthenticationResult authenticateWithOidc(String principal, UserRoleMapp
.claim("groups", Arrays.asList("group1", "group2", "groups3"))
.claim("mail", "[email protected]")
.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<String, Object> entry : additionalClaims.entrySet()) {
claimsBuilder.claim(entry.getKey(), entry.getValue());
}
}
final JWTClaimsSet claims = claimsBuilder.build();
doAnswer((i) -> {
ActionListener<JWTClaimsSet> listener = (ActionListener<JWTClaimsSet>) i.getArguments()[1];
listener.onResponse(claims);
Expand Down

0 comments on commit 131a353

Please sign in to comment.