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"),