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 1 commit
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
41 changes: 27 additions & 14 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,15 @@ public String toString() {
return name;
}

private static void validateClaimType(Object claimValueObject, String settingKey) {
if (claimValueObject instanceof String == false &&
Copy link
Member

Choose a reason for hiding this comment

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

I'd prefer a more descriptive method name if possible. The current feels like it will validate the claim value is a certain specified type. But it really just asserts the value is either:

  • A String or
  • If it is a collection, it must be a collection of String

Now I am not sure if this is what we really want? Is it OK if the value is a Number?

Copy link
Member Author

Choose a reason for hiding this comment

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

Hey can you elaborate on what you mean be "descriptive" here? The method does what it intends to do as described in the PR description:

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.

Doing so, seems to me to be "Validating the type of the claim value" and as such validateClaimType seems appropriate. What would be an alternative that you are more comfortable with ?

Copy link
Member

Choose a reason for hiding this comment

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

What I mean is the name gave me the first impression that the signature would be something like:

void validateCliamType(Object value, Class<?> type)

But instead the type is not specified but should always be String or collection of it. So I'd personally prefer something like validateParsableClaim.

Copy link
Member Author

@jkakavas jkakavas 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.

Fair enough, I don't necessarily agree that validateClaimType is off :) but I don't dislike validateParsableClaim enough to fortify my hill :)

Copy link
Member

Choose a reason for hiding this comment

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

Thanks Ioannis. The more important piece of my comment here is about the behaviour, i.e. the if check does not seem to do what we want to ensure, i.e. String or Collection<String>:

claimValueObject instanceof String == false && 
claimValueObject instanceof Collection && 
((Collection) claimValueObject).stream().allMatch(c -> c instanceof String) == false

If the value is a Number, the if branch will not be executed, i.e. a Number will be considered as parsable.

Copy link
Member Author

Choose a reason for hiding this comment

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

Aha! Now I saw it , thanks, will address

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) {

Expand All @@ -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<String> 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<String>) claimValueObject;
} else {
throw new SettingsException("Setting [" + RealmSettings.getFullSettingKey(realmConfig, setting.getClaim())
Expand Down Expand Up @@ -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<String>) claimValueObject).stream()
Copy link
Member

Choose a reason for hiding this comment

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

The new validateClaimType method does not assert claimValueObject is a List. In theory it could be a Set or a Number.

Copy link
Member Author

Choose a reason for hiding this comment

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

validateClaimType right above validates that is either a Collection or a String. Then, if this is not a String, it is a Collection. How could it get to be a Set or a Number ? Maybe I didn't get what you mean

.filter(Objects::nonNull)
.collect(Collectors.toUnmodifiableList());
.filter(Objects::nonNull)
.collect(Collectors.toUnmodifiableList());
});
}
} else if (required) {
Expand Down
82 changes: 73 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,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;
Expand All @@ -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;
Expand Down Expand Up @@ -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));
Expand All @@ -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<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(
"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<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 +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));
Expand All @@ -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));
}
Expand Down Expand Up @@ -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<String, Object> additionalClaims)
throws Exception {
RealmConfig.RealmIdentifier realmIdentifier = new RealmConfig.RealmIdentifier("mock", "mock_lookup");
final MockLookupRealm lookupRealm = new MockLookupRealm(
Expand All @@ -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)))
Expand All @@ -370,9 +430,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