Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Map only specific type of OIDC Claims #58524

Merged
merged 9 commits into from
Jul 6, 2020
Merged

Map only specific type of OIDC Claims #58524

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
e93302d
Map only specific type of OIDC Claims
jkakavas Jun 23, 2020
4274675
address feedback
jkakavas Jun 29, 2020
f1b3589
unused import
jkakavas Jun 29, 2020
1c02a7d
Merge remote-tracking branch 'origin/master' into claims-mapping-changes
jkakavas Jun 29, 2020
fd030f0
Don't try to validate null claims
jkakavas Jun 29, 2020
cb9e3f2
fix parsing and validation logic
jkakavas Jun 29, 2020
ee0b57b
address feedback
jkakavas Jun 30, 2020
2499dc2
checkstyle
jkakavas Jun 30, 2020
41aeec6
Merge remote-tracking branch 'origin/master' into claims-mapping-changes
jkakavas Jun 30, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions x-pack/docs/en/security/authentication/oidc-guide.asciidoc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
<<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
69 changes: 34 additions & 35 deletions ...ecurity/src/main/java/org/elasticsearch/xpack/security/authc/oidc/OpenIdConnectRealm.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -210,13 +210,9 @@ private void buildUserFromClaims(JWTClaimsSet claims, ActionListener<Authenticat

final Map<String, Object> 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();
}
Expand Down Expand Up @@ -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)));
Copy link
Contributor

@tvernum tvernum Jun 25, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we probably want to ensure that these are homogenous collections. I don't know how we would make sense of [ "1", true, 1.0 ]

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

think we probably want to ensure that these are heterogenous collections.

You used Greek so I get to be pedantic 🎉 s/heterogeneous/homogeneous/ :)
In all seriousness, I took a lenient stance here on purpose for metadata ( as opposed to user properties where I allowed only strings ).

I don't know how we would make sense of [ "1", true, 1.0 ]

  • Metadata can be used as metadata, for informational purposes so we do not necessarily need to make sense of the value of a given key.
  • It would work in a role mapping rule such as
"rules" : { "field" : { "groups" : "1" } },

happy to reconsider if you think this would cause an issue somewhere else or that it is too lenient for the sake of being lenient but I don't see any specific problem with it as is

Copy link
Contributor

@tvernum tvernum Jun 26, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My comment clearly says homogeneous 😈

Fair enough - as long as it's a considered position, I'm not worried.

}

static final class ClaimParser {
private final String name;
private final Function<JWTClaimsSet, List<String>> parser;
Expand Down Expand Up @@ -412,6 +417,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 = 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<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 @@ -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<String> values;
if (claimValueObject == null) {
values = List.of();
} else if (claimValueObject instanceof String) {
values = List.of((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 @@ -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<String>) 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())
Expand Down
85 changes: 76 additions & 9 deletions ...ty/src/test/java/org/elasticsearch/xpack/security/authc/oidc/OpenIdConnectRealmTests.java
View file
Open in desktop
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 @@ -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));
Expand All @@ -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<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 = Map.of(
"groups", List.of(Map.of("key1", List.of("value1", "value2")), Map.of("key2", List.of("value1", "value2")))
);
Map<String, Object> 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<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 = 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("[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 @@ -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));
Expand All @@ -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));
}
Expand Down Expand Up @@ -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<String, Object> additionalClaims)
throws Exception {
RealmConfig.RealmIdentifier realmIdentifier = new RealmConfig.RealmIdentifier("mock", "mock_lookup");
final MockLookupRealm lookupRealm = new MockLookupRealm(
Expand All @@ -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)))
Expand All @@ -370,9 +433,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