From 84bcfc525f702ffac773c6bbb6b9f668eb5e03d0 Mon Sep 17 00:00:00 2001 From: Ioannis Kakavas Date: Tue, 21 May 2019 10:25:50 +0300 Subject: [PATCH 1/2] Merge claims from userinfo and ID Token correctly 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 | 87 ++++++++++++++++++- .../oidc/OpenIdConnectAuthenticatorTests.java | 84 +++++++++++++++++- 2 files changed, 168 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..542b7dd7a3e3e 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()); + merge(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,74 @@ 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 merge(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(), merge((JSONArray) value1, value2)); + } else if (value1 instanceof JSONObject) { + idToken.put(entry.getKey(), merge((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 merge(JSONObject jsonObject1, Object jsonObject2) { + if (jsonObject2 == null) + return jsonObject1; + if (jsonObject2 instanceof JSONObject) + return merge(jsonObject1, (JSONObject) jsonObject2); + throw new IllegalStateException("Error while merging ID token and userinfo claims. " + + "Cannot merge JSONObject with [" + jsonObject2.getClass().getName() + "]"); + } + + private static JSONArray merge(JSONArray jsonArray1, Object jsonArray2) { + if (jsonArray2 == null) { + return jsonArray1; + } + if (jsonArray2 instanceof JSONArray) { + return merge(jsonArray1, (JSONArray) jsonArray2); + } + if (jsonArray2 instanceof String) { + jsonArray1.add(jsonArray2); + } + return jsonArray1; + } + + private static JSONArray merge(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..8f6cc52a399a7 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,7 @@ 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.JSONObject; import org.elasticsearch.ElasticsearchSecurityException; import org.elasticsearch.action.ActionListener; import org.elasticsearch.action.support.PlainActionFuture; @@ -96,7 +97,9 @@ public void setup() { @After public void cleanup() { - authenticator.close(); + if (authenticator != null) { + authenticator.close(); + } } private OpenIdConnectAuthenticator buildAuthenticator() throws URISyntaxException { @@ -632,6 +635,85 @@ 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 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") + .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.merge(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("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")); + + 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.merge(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.merge(idTokenObject, overwriteUserInfo); + assertThat(idTokenObject.getAsString("email"), equalTo("jane.doe@example.com")); + assertThat(idTokenObject.getAsString("profile"), equalTo("https://test-profiles.com/jane.doe")); + } + private OpenIdConnectProviderConfiguration getOpConfig() throws URISyntaxException { return new OpenIdConnectProviderConfiguration( new Issuer("https://op.example.com"), From 585d5e6ff518abbcff97c227bc53d3cebfaa08d4 Mon Sep 17 00:00:00 2001 From: Ioannis Kakavas Date: Wed, 22 May 2019 09:42:55 +0300 Subject: [PATCH 2/2] Address feedback --- .../oidc/OpenIdConnectAuthenticator.java | 29 ++++----- .../oidc/OpenIdConnectAuthenticatorTests.java | 63 ++++++++++++++++++- 2 files changed, 75 insertions(+), 17 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 542b7dd7a3e3e..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 @@ -407,7 +407,7 @@ private void handleUserinfoResponse(HttpResponse httpResponse, JWTClaimsSet veri LOGGER.trace("Successfully retrieved user information: [{}]", userInfoClaims.toJSONObject().toJSONString()); } final JSONObject combinedClaims = verifiedIdTokenClaims.toJSONObject(); - merge(combinedClaims, 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 @@ -626,9 +626,9 @@ private void setMetadataFileWatcher(String jwkSetPath) throws IOException { * 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: *
    - *
  • If the values for a given claim are strings, the value from the ID Token is retained
    • + *
  • If the values for a given claim are primitives (of the the same type), the value from the ID Token is retained
    • *
  • If the values for a given claim are Objects, the values are merged
    • - *
  • If the values for a given claim are Arrays, the values are merged
    • + *
  • If the values for a given claim are Arrays, the values are merged without removing duplicates
    • *
  • If the values for a given claim are of different types, an exception is thrown
    • *
* @@ -637,7 +637,7 @@ private void setMetadataFileWatcher(String jwkSetPath) throws IOException { * @return the merged JsonObject */ // pkg protected for testing - static JSONObject merge(JSONObject idToken, JSONObject userInfo) { + static JSONObject mergeObjects(JSONObject idToken, JSONObject userInfo) { for (Map.Entry entry : idToken.entrySet()) { Object value1 = entry.getValue(); Object value2 = userInfo.get(entry.getKey()); @@ -645,10 +645,9 @@ static JSONObject merge(JSONObject idToken, JSONObject userInfo) { continue; } if (value1 instanceof JSONArray) { - idToken.put(entry.getKey(), merge((JSONArray) value1, value2)); + idToken.put(entry.getKey(), mergeArrays((JSONArray) value1, value2)); } else if (value1 instanceof JSONObject) { - idToken.put(entry.getKey(), merge((JSONObject) value1, value2)); - + 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() + "]"); @@ -662,21 +661,23 @@ static JSONObject merge(JSONObject idToken, JSONObject userInfo) { return idToken; } - private static JSONObject merge(JSONObject jsonObject1, Object jsonObject2) { - if (jsonObject2 == null) + private static JSONObject mergeObjects(JSONObject jsonObject1, Object jsonObject2) { + if (jsonObject2 == null) { return jsonObject1; - if (jsonObject2 instanceof JSONObject) - return merge(jsonObject1, (JSONObject) jsonObject2); + } + 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 merge(JSONArray jsonArray1, Object jsonArray2) { + private static JSONArray mergeArrays(JSONArray jsonArray1, Object jsonArray2) { if (jsonArray2 == null) { return jsonArray1; } if (jsonArray2 instanceof JSONArray) { - return merge(jsonArray1, (JSONArray) jsonArray2); + return mergeArrays(jsonArray1, (JSONArray) jsonArray2); } if (jsonArray2 instanceof String) { jsonArray1.add(jsonArray2); @@ -684,7 +685,7 @@ private static JSONArray merge(JSONArray jsonArray1, Object jsonArray2) { return jsonArray1; } - private static JSONArray merge(JSONArray jsonArray1, JSONArray jsonArray2) { + private static JSONArray mergeArrays(JSONArray jsonArray1, JSONArray jsonArray2) { jsonArray1.addAll(jsonArray2); return jsonArray1; } 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 8f6cc52a399a7..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,7 @@ 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; @@ -73,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; @@ -642,6 +644,13 @@ public void testJsonObjectMerging() throws Exception { 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()) @@ -655,6 +664,8 @@ public void testJsonObjectMerging() throws Exception { .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(); @@ -669,12 +680,14 @@ public void testJsonObjectMerging() throws Exception { .build() .toJSONObject(); - OpenIdConnectAuthenticator.merge(idTokenObject, userinfoObject); + 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")); @@ -684,6 +697,7 @@ public void testJsonObjectMerging() throws Exception { 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) @@ -695,7 +709,7 @@ public void testJsonObjectMerging() throws Exception { .toJSONObject(); final IllegalStateException e = expectThrows(IllegalStateException.class, () -> { - OpenIdConnectAuthenticator.merge(idTokenObject, wrongTypeInfo); + OpenIdConnectAuthenticator.mergeObjects(idTokenObject, wrongTypeInfo); }); // Userinfo Claims overwrite ID Token claims @@ -709,9 +723,52 @@ public void testJsonObjectMerging() throws Exception { .build() .toJSONObject(); - OpenIdConnectAuthenticator.merge(idTokenObject, overwriteUserInfo); + 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 {