From b2423905f27cd29670c53a66ab1066394f0b89fa Mon Sep 17 00:00:00 2001 From: Ioannis Kakavas Date: Wed, 22 May 2019 12:00:21 +0300 Subject: [PATCH] Merge claims from userinfo and ID Token correctly (#42277) Enhance the handling of merging the claims sets of the ID Token and the UserInfo response. JsonObject#merge would throw a runtime exception when attempting to merge two objects with the same key and different values. This could happen for an OP that returns different vales for the same claim in the ID Token and the UserInfo response ( Google does that for profile claim ). If a claim is contained in both sets, we attempt to merge the values if they are objects or arrays, otherwise the ID Token claim value takes presedence and overwrites the userinfo response. --- .../oidc/OpenIdConnectAuthenticator.java | 88 ++++++++++- .../oidc/OpenIdConnectAuthenticatorTests.java | 141 +++++++++++++++++- 2 files changed, 226 insertions(+), 3 deletions(-) diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/oidc/OpenIdConnectAuthenticator.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/oidc/OpenIdConnectAuthenticator.java index 32cffc80071c3..c652a39b90912 100644 --- a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/oidc/OpenIdConnectAuthenticator.java +++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/oidc/OpenIdConnectAuthenticator.java @@ -37,6 +37,7 @@ import com.nimbusds.openid.connect.sdk.token.OIDCTokens; import com.nimbusds.openid.connect.sdk.validators.AccessTokenValidator; import com.nimbusds.openid.connect.sdk.validators.IDTokenValidator; +import net.minidev.json.JSONArray; import net.minidev.json.JSONObject; import org.apache.commons.codec.Charsets; import org.apache.http.Header; @@ -401,15 +402,16 @@ private void handleUserinfoResponse(HttpResponse httpResponse, JWTClaimsSet veri if (httpResponse.getStatusLine().getStatusCode() == 200) { if (ContentType.parse(contentHeader.getValue()).getMimeType().equals("application/json")) { final JWTClaimsSet userInfoClaims = JWTClaimsSet.parse(contentAsString); + validateUserInfoResponse(userInfoClaims, verifiedIdTokenClaims.getSubject(), claimsListener); if (LOGGER.isTraceEnabled()) { LOGGER.trace("Successfully retrieved user information: [{}]", userInfoClaims.toJSONObject().toJSONString()); } final JSONObject combinedClaims = verifiedIdTokenClaims.toJSONObject(); - combinedClaims.merge(userInfoClaims.toJSONObject()); + mergeObjects(combinedClaims, userInfoClaims.toJSONObject()); claimsListener.onResponse(JWTClaimsSet.parse(combinedClaims)); } else if (ContentType.parse(contentHeader.getValue()).getMimeType().equals("application/jwt")) { //TODO Handle validating possibly signed responses - claimsListener.onFailure(new IllegalStateException("Unable to parse Userinfo Response. Signed/encryopted JWTs are" + + claimsListener.onFailure(new IllegalStateException("Unable to parse Userinfo Response. Signed/encrypted JWTs are" + "not currently supported")); } else { claimsListener.onFailure(new IllegalStateException("Unable to parse Userinfo Response. Content type was expected to " + @@ -435,6 +437,19 @@ private void handleUserinfoResponse(HttpResponse httpResponse, JWTClaimsSet veri } } + /** + * Validates that the userinfo response contains a sub Claim and that this claim value is the same as the one returned in the ID Token + */ + private void validateUserInfoResponse(JWTClaimsSet userInfoClaims, String expectedSub, ActionListener claimsListener) { + if (userInfoClaims.getSubject().isEmpty()) { + claimsListener.onFailure(new ElasticsearchSecurityException("Userinfo Response did not contain a sub Claim")); + } else if (userInfoClaims.getSubject().equals(expectedSub) == false) { + claimsListener.onFailure(new ElasticsearchSecurityException("Userinfo Response is not valid as it is for " + + "subject [{}] while the ID Token was for subject [{}]", userInfoClaims.getSubject(), + expectedSub)); + } + } + /** * Attempts to make a request to the Token Endpoint of the OpenID Connect provider in order to exchange an * authorization code for an Id Token (and potentially an Access Token) @@ -606,6 +621,75 @@ private void setMetadataFileWatcher(String jwkSetPath) throws IOException { watcherService.add(watcher, ResourceWatcherService.Frequency.MEDIUM); } + /** + * Merges the JsonObject with the claims of the ID Token with the JsonObject with the claims of the UserInfo response. This is + * necessary as some OPs return slightly different values for some claims (i.e. Google for the profile picture) and + * {@link JSONObject#merge(Object)} would throw a runtime exception. The merging is performed based on the following rules: + * + * + * @param userInfo The JsonObject with the ID Token claims + * @param idToken The JsonObject with the UserInfo Response claims + * @return the merged JsonObject + */ + // pkg protected for testing + static JSONObject mergeObjects(JSONObject idToken, JSONObject userInfo) { + for (Map.Entry entry : idToken.entrySet()) { + Object value1 = entry.getValue(); + Object value2 = userInfo.get(entry.getKey()); + if (value2 == null) { + continue; + } + if (value1 instanceof JSONArray) { + idToken.put(entry.getKey(), mergeArrays((JSONArray) value1, value2)); + } else if (value1 instanceof JSONObject) { + idToken.put(entry.getKey(), mergeObjects((JSONObject) value1, value2)); + } else if (value1.getClass().equals(value2.getClass()) == false) { + throw new IllegalStateException("Error merging ID token and userinfo claim value for claim [" + entry.getKey() + "]. " + + "Cannot merge [" + value1.getClass().getName() + "] with [" + value2.getClass().getName() + "]"); + } + } + for (Map.Entry entry : userInfo.entrySet()) { + if (idToken.containsKey(entry.getKey()) == false) { + idToken.put(entry.getKey(), entry.getValue()); + } + } + return idToken; + } + + private static JSONObject mergeObjects(JSONObject jsonObject1, Object jsonObject2) { + if (jsonObject2 == null) { + return jsonObject1; + } + if (jsonObject2 instanceof JSONObject) { + return mergeObjects(jsonObject1, (JSONObject) jsonObject2); + } + throw new IllegalStateException("Error while merging ID token and userinfo claims. " + + "Cannot merge JSONObject with [" + jsonObject2.getClass().getName() + "]"); + } + + private static JSONArray mergeArrays(JSONArray jsonArray1, Object jsonArray2) { + if (jsonArray2 == null) { + return jsonArray1; + } + if (jsonArray2 instanceof JSONArray) { + return mergeArrays(jsonArray1, (JSONArray) jsonArray2); + } + if (jsonArray2 instanceof String) { + jsonArray1.add(jsonArray2); + } + return jsonArray1; + } + + private static JSONArray mergeArrays(JSONArray jsonArray1, JSONArray jsonArray2) { + jsonArray1.addAll(jsonArray2); + return jsonArray1; + } + protected void close() { try { this.httpClient.close(); diff --git a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/oidc/OpenIdConnectAuthenticatorTests.java b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/oidc/OpenIdConnectAuthenticatorTests.java index 64e976d90d1e3..43b58b8d4b521 100644 --- a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/oidc/OpenIdConnectAuthenticatorTests.java +++ b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/oidc/OpenIdConnectAuthenticatorTests.java @@ -37,6 +37,8 @@ import com.nimbusds.openid.connect.sdk.claims.AccessTokenHash; import com.nimbusds.openid.connect.sdk.validators.IDTokenValidator; import com.nimbusds.openid.connect.sdk.validators.InvalidHashException; +import net.minidev.json.JSONArray; +import net.minidev.json.JSONObject; import org.elasticsearch.ElasticsearchSecurityException; import org.elasticsearch.action.ActionListener; import org.elasticsearch.action.support.PlainActionFuture; @@ -72,6 +74,7 @@ import java.util.UUID; import static java.time.Instant.now; +import static org.hamcrest.Matchers.containsInAnyOrder; import static org.hamcrest.Matchers.containsString; import static org.hamcrest.Matchers.equalTo; import static org.hamcrest.Matchers.instanceOf; @@ -96,7 +99,9 @@ public void setup() { @After public void cleanup() { - authenticator.close(); + if (authenticator != null) { + authenticator.close(); + } } private OpenIdConnectAuthenticator buildAuthenticator() throws URISyntaxException { @@ -632,6 +637,140 @@ public void testImplicitFlowFailsWithUnsignedJwt() throws Exception { assertThat(e.getCause().getMessage(), containsString("Signed ID token expected")); } + public void testJsonObjectMerging() throws Exception { + final Nonce nonce = new Nonce(); + final String subject = "janedoe"; + final Tuple keyMaterial = getRandomJwkForType(randomFrom("ES", "RS")); + final JWK jwk = keyMaterial.v2().getKeys().get(0); + RelyingPartyConfiguration rpConfig = getRpConfig(jwk.getAlgorithm().getName()); + OpenIdConnectProviderConfiguration opConfig = getOpConfig(); + JSONObject address = new JWTClaimsSet.Builder() + .claim("street_name", "12, Test St.") + .claim("locality", "New York") + .claim("region", "NY") + .claim("country", "USA") + .build() + .toJSONObject(); + JSONObject idTokenObject = new JWTClaimsSet.Builder() + .jwtID(randomAlphaOfLength(8)) + .audience(rpConfig.getClientId().getValue()) + .expirationTime(Date.from(now().plusSeconds(3600))) + .issuer(opConfig.getIssuer().getValue()) + .issueTime(Date.from(now().minusSeconds(200))) + .notBeforeTime(Date.from(now().minusSeconds(200))) + .claim("nonce", nonce) + .claim("given_name", "Jane Doe") + .claim("family_name", "Doe") + .claim("profile", "https://test-profiles.com/jane.doe") + .claim("name", "Jane") + .claim("email", "jane.doe@example.com") + .claim("roles", new JSONArray().appendElement("role1").appendElement("role2").appendElement("role3")) + .claim("address", address) + .subject(subject) + .build() + .toJSONObject(); + + JSONObject userinfoObject = new JWTClaimsSet.Builder() + .claim("given_name", "Jane Doe") + .claim("family_name", "Doe") + .claim("profile", "https://test-profiles.com/jane.doe") + .claim("name", "Jane") + .claim("email", "jane.doe@example.com") + .subject(subject) + .build() + .toJSONObject(); + + OpenIdConnectAuthenticator.mergeObjects(idTokenObject, userinfoObject); + assertTrue(idTokenObject.containsKey("given_name")); + assertTrue(idTokenObject.containsKey("family_name")); + assertTrue(idTokenObject.containsKey("profile")); + assertTrue(idTokenObject.containsKey("name")); + assertTrue(idTokenObject.containsKey("email")); + assertTrue(idTokenObject.containsKey("address")); + assertTrue(idTokenObject.containsKey("roles")); + assertTrue(idTokenObject.containsKey("nonce")); + assertTrue(idTokenObject.containsKey("sub")); + assertTrue(idTokenObject.containsKey("jti")); + assertTrue(idTokenObject.containsKey("aud")); + assertTrue(idTokenObject.containsKey("exp")); + assertTrue(idTokenObject.containsKey("iss")); + assertTrue(idTokenObject.containsKey("iat")); + assertTrue(idTokenObject.containsKey("email")); + + // Claims with different types throw an error + JSONObject wrongTypeInfo = new JWTClaimsSet.Builder() + .claim("given_name", "Jane Doe") + .claim("family_name", 123334434) + .claim("profile", "https://test-profiles.com/jane.doe") + .claim("name", "Jane") + .claim("email", "jane.doe@example.com") + .subject(subject) + .build() + .toJSONObject(); + + final IllegalStateException e = expectThrows(IllegalStateException.class, () -> { + OpenIdConnectAuthenticator.mergeObjects(idTokenObject, wrongTypeInfo); + }); + + // Userinfo Claims overwrite ID Token claims + JSONObject overwriteUserInfo = new JWTClaimsSet.Builder() + .claim("given_name", "Jane Doe") + .claim("family_name", "Doe") + .claim("profile", "https://test-profiles.com/jane.doe2") + .claim("name", "Jane") + .claim("email", "jane.doe@mail.com") + .subject(subject) + .build() + .toJSONObject(); + + OpenIdConnectAuthenticator.mergeObjects(idTokenObject, overwriteUserInfo); + assertThat(idTokenObject.getAsString("email"), equalTo("jane.doe@example.com")); + assertThat(idTokenObject.getAsString("profile"), equalTo("https://test-profiles.com/jane.doe")); + + // Merging Arrays + JSONObject userInfoWithRoles = new JWTClaimsSet.Builder() + .claim("given_name", "Jane Doe") + .claim("family_name", "Doe") + .claim("profile", "https://test-profiles.com/jane.doe") + .claim("name", "Jane") + .claim("email", "jane.doe@example.com") + .claim("roles", new JSONArray().appendElement("role4").appendElement("role5")) + .subject(subject) + .build() + .toJSONObject(); + + OpenIdConnectAuthenticator.mergeObjects(idTokenObject, userInfoWithRoles); + assertThat((JSONArray) idTokenObject.get("roles"), containsInAnyOrder("role1", "role2", "role3", "role4", "role5")); + + // Merging nested objects + JSONObject addressUserInfo = new JWTClaimsSet.Builder() + .claim("street_name", "12, Test St.") + .claim("locality", "New York") + .claim("postal_code", "10024") + .build() + .toJSONObject(); + JSONObject userInfoWithAddress = new JWTClaimsSet.Builder() + .claim("given_name", "Jane Doe") + .claim("family_name", "Doe") + .claim("profile", "https://test-profiles.com/jane.doe") + .claim("name", "Jane") + .claim("email", "jane.doe@example.com") + .claim("roles", new JSONArray().appendElement("role4").appendElement("role5")) + .claim("address", addressUserInfo) + .subject(subject) + .build() + .toJSONObject(); + OpenIdConnectAuthenticator.mergeObjects(idTokenObject, userInfoWithAddress); + assertTrue(idTokenObject.containsKey("address")); + JSONObject combinedAddress = (JSONObject) idTokenObject.get("address"); + assertTrue(combinedAddress.containsKey("street_name")); + assertTrue(combinedAddress.containsKey("locality")); + assertTrue(combinedAddress.containsKey("street_name")); + assertTrue(combinedAddress.containsKey("postal_code")); + assertTrue(combinedAddress.containsKey("region")); + assertTrue(combinedAddress.containsKey("country")); + } + private OpenIdConnectProviderConfiguration getOpConfig() throws URISyntaxException { return new OpenIdConnectProviderConfiguration( new Issuer("https://op.example.com"),