Skip to content

Add ClientRegistration from OpenID Connect Discovery #5355

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.

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

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
1 change: 1 addition & 0 deletions config/spring-security-config.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ dependencies {
testCompile apachedsDependencies
testCompile powerMock2Dependencies
testCompile spockDependencies
testCompile 'com.squareup.okhttp3:mockwebserver'
testCompile 'ch.qos.logback:logback-classic'
testCompile 'io.projectreactor.ipc:reactor-netty'
testCompile 'javax.annotation:jsr250-api:1.0'
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,118 @@
/*
* Copyright 2002-2018 the original author or authors.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

package org.springframework.security.config.oauth2.client;
Copy link
Contributor

Choose a reason for hiding this comment

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

Add to package ...oauth2.client.oidc


import java.net.URI;
import java.util.Arrays;
import java.util.List;

import org.springframework.security.oauth2.client.registration.ClientRegistration;
import org.springframework.security.oauth2.core.AuthorizationGrantType;
import org.springframework.security.oauth2.core.ClientAuthenticationMethod;
import org.springframework.security.oauth2.core.oidc.IdTokenClaimNames;
import org.springframework.web.client.RestTemplate;

import com.nimbusds.oauth2.sdk.GrantType;
import com.nimbusds.oauth2.sdk.ParseException;
import com.nimbusds.oauth2.sdk.Scope;
import com.nimbusds.openid.connect.sdk.op.OIDCProviderMetadata;

/**
* Allows creating a {@link ClientRegistration.Builder} from an
* <a href="https://openid.net/specs/openid-connect-discovery-1_0.html#ProviderConfig">OpenID Provider Configuration</a>.
*
* @author Rob Winch
* @since 5.1
*/
public final class OidcConfigurationProvider {

/**
* Given the <a href="http://openid.net/specs/openid-connect-core-1_0.html#IssuerIdentifier">Issuer</a> creates a
Copy link
Contributor

Choose a reason for hiding this comment

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

"Creates a ClientRegistration.Builder using the provided Issuer by making an..."

* {@link ClientRegistration.Builder} by making an
* <a href="https://openid.net/specs/openid-connect-discovery-1_0.html#ProviderConfigurationRequest">OpenID Provider
* Configuration Request</a> and using the values in the
* <a href="https://openid.net/specs/openid-connect-discovery-1_0.html#ProviderConfigurationResponse">OpenID
* Provider Configuration Response</a> to initialize the {@link ClientRegistration.Builder}.
*
* <p>
* For example if the issuer provided is "https://example.com", then an "OpenID Provider Configuration Request" will
Copy link
Contributor

Choose a reason for hiding this comment

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

Use "&quot ;" entity for all

Copy link
Member Author

Choose a reason for hiding this comment

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

" do not need to be encoded and this is difficult for readability. I'm going to leave this as is

Copy link
Contributor

Choose a reason for hiding this comment

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

"For example, if the..."

* be made to "https://example.com/.well-known/openid-configuration". The result is expected to be an "OpenID
* Provider Configuration Response".
* </p>
*
* <p>
* Example usage:
* </p>
* <pre>
* ClientRegistration registration = OidcConfigurationProvider.issuer("https://example.com")
* .clientId("client-id")
* .clientSecret("client-secret")
* .build();
* </pre>
* @param issuer the <a href="http://openid.net/specs/openid-connect-core-1_0.html#IssuerIdentifier">Issuer</a>
* @return a {@link ClientRegistration.Builder} that was initialized by the OpenID Provider Configuration.
*/
public static ClientRegistration.Builder issuer(String issuer) {
RestTemplate rest = new RestTemplate();
Copy link
Contributor

Choose a reason for hiding this comment

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

Add Assert.hasText(issuer)...also should we validate it's a URI?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think leaving this validation to the invocation of the RestTemplate is probably best.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's not going to provide a user-friendly message compared to "Issuer cannot be empty/null" if we apply the Assert

Copy link
Member Author

Choose a reason for hiding this comment

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

I updated this to provide an exception with the issuer in it if we fail to get a successful result back since valid URLs might not produce valid result.

Copy link
Contributor

Choose a reason for hiding this comment

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

Should we use WebClient instead? Also, set as static member so we can re-use for each request?

Copy link
Member Author

Choose a reason for hiding this comment

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

RestTemplate has fewer dependencies, so I think sticking with that makes sense. Especially since starting the container is blocking anyways.

Copy link
Contributor

Choose a reason for hiding this comment

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

Is not need timeout settings?

Copy link
Member Author

Choose a reason for hiding this comment

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

@kazuki43zoo I think at this point we are going to leave it encapsulated. We probably need a separate issue to handle things like timeout settings more holistically. Likely this will be done by allowing the user to provide a WebClient or RestOperations instance

Copy link
Contributor

Choose a reason for hiding this comment

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

Related to @kazuki43zoo's question, would RestTemplate better serve as an injected dependency? I recognize this changes the contract, since the method won't be static anymore, but just throwing it out there.

Copy link
Member Author

Choose a reason for hiding this comment

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

@jzheaux At some point. We need to decide more wholistically if this is going to be WebClient or RestOperations since this should be consistent throughout our APIs. For the moment, I think we should keep this private.

Copy link
Contributor

Choose a reason for hiding this comment

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

@rwinch Thanks for your replay. I think keeping private is OK.
But I will afraid that it is possible become timeout of infinity in current implementation (new RestTemplate() = default implementation of theSimpleClientHttpRequestFactory ).

String openidConfiguration = rest.getForObject(issuer + "/.well-known/openid-configuration", String.class);
OIDCProviderMetadata metadata = parse(openidConfiguration);
String name = URI.create(issuer).getHost();
List<com.nimbusds.oauth2.sdk.auth.ClientAuthenticationMethod> metadataAuthMethods = metadata.getTokenEndpointAuthMethods();
Copy link
Contributor

Choose a reason for hiding this comment

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

Another validation check can be applied, see 4.3. OpenID Provider Configuration Validation

The issuer value returned MUST be identical to the Issuer URL that was directly used to retrieve the configuration information. This MUST also be identical to the iss Claim value in ID Tokens issued from this Issuer.

Copy link
Member Author

Choose a reason for hiding this comment

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

Given we don't use the Issuer provided by the Configuration, what is the benefit of validating this? It is something that is up to the Provider (we are a client) to ensure that they are the same unless we (the Client) end up using this, I don't think we should validate it.

Copy link
Contributor

Choose a reason for hiding this comment

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

What if there was a MITM scenario and the metadata response returned was from evil.com instead. The client would than use evil.com authorization and token endpoints

Copy link
Contributor

Choose a reason for hiding this comment

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

@jgrandja I believe you are saying that if the returned payload has an issuer that is different from the one used to make the request, then the entire response is suspect, even if we aren't using the issuer value in the response. I agree with this assessment.

Copy link
Member Author

Choose a reason for hiding this comment

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

If you aren't using HTTPS you are in trouble no matter what. A malicious user will respond with a valid issuer but an invalid endpoint of sorts. I went ahead and add the validation, but since we don't use the returned issuer, we are really validating the OpenID Configuration of the OP for the sake of validating vs any real gain.

// if null, the default includes client_secret_basic
if (metadataAuthMethods != null && !metadataAuthMethods.contains(com.nimbusds.oauth2.sdk.auth.ClientAuthenticationMethod.CLIENT_SECRET_BASIC)) {
throw new IllegalArgumentException("Only ClientAuthenticationMethod.BASIC is supported. The issuer \"" + issuer + "\" returned a configuration of " + metadataAuthMethods);
Copy link
Contributor

Choose a reason for hiding this comment

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

We also support ClientAuthenticationMethod.POST

}
List<GrantType> grantTypes = metadata.getGrantTypes();
// If null, the default includes authorization_code
if (grantTypes != null && !grantTypes.contains(GrantType.AUTHORIZATION_CODE)) {
throw new IllegalArgumentException("Only AuthorizationGrantType.AUTHORIZATION_CODE is supported. The issuer \"" + issuer + "\" returned a configuration of " + grantTypes);
}
List<String> scopes = getScopes(metadata);
return ClientRegistration.withRegistrationId(name)
.userNameAttributeName(IdTokenClaimNames.SUB)
.scope(scopes)
.authorizationGrantType(AuthorizationGrantType.AUTHORIZATION_CODE)
.clientAuthenticationMethod(ClientAuthenticationMethod.BASIC)
.redirectUriTemplate("{baseUrl}/{action}/oauth2/code/{registrationId}")
.authorizationUri(metadata.getAuthorizationEndpointURI().toASCIIString())
.jwkSetUri(metadata.getJWKSetURI().toASCIIString())
.userInfoUri(metadata.getUserInfoEndpointURI().toASCIIString())
.tokenUri(metadata.getTokenEndpointURI().toASCIIString())
.clientName(issuer);
}

private static List<String> getScopes(OIDCProviderMetadata metadata) {
Scope scope = metadata.getScopes();
if (scope == null) {
// If null, default to "openid" which must be supported
return Arrays.asList("openid");
Copy link
Contributor

Choose a reason for hiding this comment

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

Use OidcScopes.OPENID

} else {
return scope.toStringList();
}
}

private static OIDCProviderMetadata parse(String body) {
try {
return OIDCProviderMetadata.parse(body);
}
catch (ParseException e) {
throw new RuntimeException(e);
}
}

private OidcConfigurationProvider() {}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,209 @@
/*
* Copyright 2002-2018 the original author or authors.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

package org.springframework.security.config.oauth2.client;
Copy link
Contributor

Choose a reason for hiding this comment

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

move to sub-package ...oauth2.client.oidc


import com.fasterxml.jackson.core.type.TypeReference;
import com.fasterxml.jackson.databind.ObjectMapper;
import okhttp3.mockwebserver.MockResponse;
import okhttp3.mockwebserver.MockWebServer;
import org.junit.After;
import org.junit.Before;
import org.junit.Test;
import org.springframework.http.HttpHeaders;
import org.springframework.http.MediaType;
import org.springframework.security.oauth2.client.registration.ClientRegistration;
import org.springframework.security.oauth2.core.AuthorizationGrantType;
import org.springframework.security.oauth2.core.ClientAuthenticationMethod;

import java.util.Arrays;
import java.util.Map;

import static org.assertj.core.api.Assertions.*;

/**
* @author Rob Winch
* @since 5.1
*/
public class OidcConfigurationProviderTests {

/**
* Contains all optional parameters that are found in ClientRegistration
*/
private static final String DEFAULT_RESPONSE =
"{\n"
+ " \"authorization_endpoint\": \"https://example.com/o/oauth2/v2/auth\", \n"
+ " \"claims_supported\": [\n"
+ " \"aud\", \n"
+ " \"email\", \n"
+ " \"email_verified\", \n"
+ " \"exp\", \n"
+ " \"family_name\", \n"
+ " \"given_name\", \n"
+ " \"iat\", \n"
+ " \"iss\", \n"
+ " \"locale\", \n"
+ " \"name\", \n"
+ " \"picture\", \n"
+ " \"sub\"\n"
+ " ], \n"
+ " \"code_challenge_methods_supported\": [\n"
+ " \"plain\", \n"
+ " \"S256\"\n"
+ " ], \n"
+ " \"id_token_signing_alg_values_supported\": [\n"
+ " \"RS256\"\n"
+ " ], \n"
+ " \"issuer\": \"https://example.com\", \n"
+ " \"jwks_uri\": \"https://example.com/oauth2/v3/certs\", \n"
+ " \"response_types_supported\": [\n"
+ " \"code\", \n"
+ " \"token\", \n"
+ " \"id_token\", \n"
+ " \"code token\", \n"
+ " \"code id_token\", \n"
+ " \"token id_token\", \n"
+ " \"code token id_token\", \n"
+ " \"none\"\n"
+ " ], \n"
+ " \"revocation_endpoint\": \"https://example.com/o/oauth2/revoke\", \n"
+ " \"scopes_supported\": [\n"
+ " \"openid\", \n"
+ " \"email\", \n"
+ " \"profile\"\n"
+ " ], \n"
+ " \"subject_types_supported\": [\n"
+ " \"public\"\n"
+ " ], \n"
+ " \"grant_types_supported\" : [\"authorization_code\"], \n"
+ " \"token_endpoint\": \"https://example.com/oauth2/v4/token\", \n"
+ " \"token_endpoint_auth_methods_supported\": [\n"
+ " \"client_secret_post\", \n"
+ " \"client_secret_basic\"\n"
+ " ], \n"
+ " \"userinfo_endpoint\": \"https://example.com/oauth2/v3/userinfo\"\n"
+ "}";

private MockWebServer server;

private ObjectMapper mapper = new ObjectMapper();

private Map<String, Object> response;

private String issuer;

@Before
public void setup() throws Exception {
this.server = new MockWebServer();
this.server.start();
this.response = this.mapper.readValue(DEFAULT_RESPONSE, new TypeReference<Map<String, Object>>(){});
}

@After
public void cleanup() throws Exception {
this.server.shutdown();
}

@Test
public void issuerWhenAllInformationThenSuccess() throws Exception {
ClientRegistration registration = registration("");
ClientRegistration.ProviderDetails provider = registration.getProviderDetails();

assertThat(registration.getClientAuthenticationMethod()).isEqualTo(ClientAuthenticationMethod.BASIC);
assertThat(registration.getAuthorizationGrantType()).isEqualTo(AuthorizationGrantType.AUTHORIZATION_CODE);
assertThat(registration.getRegistrationId()).isEqualTo(this.server.getHostName());
assertThat(registration.getClientName()).isEqualTo(this.issuer);
assertThat(registration.getScopes()).containsOnly("openid", "email", "profile");
assertThat(provider.getAuthorizationUri()).isEqualTo("https://example.com/o/oauth2/v2/auth");
assertThat(provider.getTokenUri()).isEqualTo("https://example.com/oauth2/v4/token");
assertThat(provider.getJwkSetUri()).isEqualTo("https://example.com/oauth2/v3/certs");
assertThat(provider.getUserInfoEndpoint().getUri()).isEqualTo("https://example.com/oauth2/v3/userinfo");
}

/**
* https://openid.net/specs/openid-connect-discovery-1_0.html#ProviderMetadata
*
* RECOMMENDED. JSON array containing a list of the OAuth 2.0 [RFC6749] scope values that this server supports. The
* server MUST support the openid scope value.
* @throws Exception
*/
@Test
public void issuerWhenScopesNullThenScopesDefaulted() throws Exception {
this.response.remove("scopes_supported");

ClientRegistration registration = registration("");

assertThat(registration.getScopes()).containsOnly("openid");
}

@Test
public void issuerWhenGrantTypesSupportedNullThenDefaulted() throws Exception {
this.response.remove("grant_types_supported");

ClientRegistration registration = registration("");

assertThat(registration.getAuthorizationGrantType()).isEqualTo(AuthorizationGrantType.AUTHORIZATION_CODE);
}

/**
* We currently only support authorization_code, so verify we have a meaningful error until we add support.
* @throws Exception
*/
@Test
public void issuerWhenGrantTypesSupportedInvalidThenException() throws Exception {
this.response.put("grant_types_supported", Arrays.asList("implicit"));

assertThatThrownBy(() -> registration(""))
.isInstanceOf(IllegalArgumentException.class)
.hasMessageContaining("Only AuthorizationGrantType.AUTHORIZATION_CODE is supported. The issuer \"" + this.issuer + "\" returned a configuration of [implicit]");
}

@Test
public void issuerWhenTokenEndpointAuthMethodsNullThenDefaulted() throws Exception {
this.response.remove("token_endpoint_auth_methods_supported");

ClientRegistration registration = registration("");

assertThat(registration.getClientAuthenticationMethod()).isEqualTo(ClientAuthenticationMethod.BASIC);
}

/**
* We currently only support client_secret_basic, so verify we have a meaningful error until we add support.
* @throws Exception
*/
@Test
public void issuerWhenTokenEndpointAuthMethodsInvalidThenException() throws Exception {
this.response.put("token_endpoint_auth_methods_supported", Arrays.asList("client_secret_post"));

assertThatThrownBy(() -> registration(""))
.isInstanceOf(IllegalArgumentException.class)
.hasMessageContaining("Only ClientAuthenticationMethod.BASIC is supported. The issuer \"" + this.issuer + "\" returned a configuration of [client_secret_post]");
}

private ClientRegistration registration(String path) throws Exception {
String body = this.mapper.writeValueAsString(this.response);
MockResponse mockResponse = new MockResponse()
.setBody(body)
.setHeader(HttpHeaders.CONTENT_TYPE, MediaType.APPLICATION_JSON_VALUE);
this.server.enqueue(mockResponse);
this.issuer = this.server.url(path).toString();

return OidcConfigurationProvider.issuer(this.issuer)
.clientId("client-id")
.clientSecret("client-secret")
.build();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
import org.springframework.util.Assert;

import java.util.Arrays;
import java.util.Collection;
import java.util.Collections;
import java.util.LinkedHashSet;
import java.util.Set;
Expand Down Expand Up @@ -324,6 +325,20 @@ public Builder scope(String... scope) {
return this;
}

/**
* Sets the scope(s) used for the client.
*
* @param scope the scope(s) used for the client
* @return the {@link Builder}
*/
public Builder scope(Collection<String> scope) {
if (scope != null && !scope.isEmpty()) {
this.scopes = Collections.unmodifiableSet(
new LinkedHashSet<>(scope));
}
return this;
}

/**
* Sets the uri for the authorization endpoint.
*
Expand Down