Add ClientRegistration from OpenID Connect Discovery #5355

rwinch · 2018-05-16T17:35:44Z

With this user's can now create a ClientRegistration using something like this:

@Bean
ReactiveClientRegistrationRepository clientRegistrationRepository() {
	ClientRegistration registration = OidcConfigurationProvider.issuer("https://accounts.google.com")
			.clientId("client-id")
			.clientSecret("client-secret")
			.build();
	return new InMemoryReactiveClientRegistrationRepository(registration);
}

Fixes: gh-4413

Fixes: spring-projectsgh-4413

sparty02 · 2018-05-17T04:55:35Z

What about defining the OIDC discovery config in application.yaml (as described in #5155)?

rwinch · 2018-05-17T13:48:29Z

@sparty02 Thanks for your feedback!

Making it simple to create the Bean definition will be a feature added to the Spring Boot AutoConfiguration support for Spring Security once this gets merged in. This aligns with the way creating ClientRegistration works today. Spring Security provides an API to create the ClientRegistration and Spring Boot provides a configuration convention to convert configuration properties into a ClientRegistrationRepository using the provided APIs. This arrangement is necessary because only with Spring Boot can we conditionally create the Beans and back out of the way.

In the meantime you can always Externalize Configuration yourself.

I hope this arrangement makes sense. Please let me know if you have additional questions or concerns.

sparty02 · 2018-05-18T03:03:47Z

@rwinch thanks, sounds good!

This reverts commit 9fe0f50. The original commit was accidentally pushed prior to PR. We attempted to revert the commit hoping the PR would open again. This did not work. We are going to do a Polish commit instead. Issue: gh-5355

jgrandja · 2018-05-18T13:18:37Z

...c/main/java/org/springframework/security/config/oauth2/client/OidcConfigurationProvider.java

+ * limitations under the License.
+ */
+
+package org.springframework.security.config.oauth2.client;


Add to package ...oauth2.client.oidc

jgrandja · 2018-05-18T13:20:52Z

...c/main/java/org/springframework/security/config/oauth2/client/OidcConfigurationProvider.java

+	 * 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


Use "&quot ;" entity for all

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

jgrandja · 2018-05-18T13:26:19Z

...c/main/java/org/springframework/security/config/oauth2/client/OidcConfigurationProvider.java

+public final class OidcConfigurationProvider {
+
+	/**
+	 * Given the <a href="http://openid.net/specs/openid-connect-core-1_0.html#IssuerIdentifier">Issuer</a> creates a


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

jgrandja · 2018-05-18T13:28:45Z

...c/main/java/org/springframework/security/config/oauth2/client/OidcConfigurationProvider.java

+	 * 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


"For example, if the..."

jgrandja · 2018-05-18T13:29:54Z

...c/main/java/org/springframework/security/config/oauth2/client/OidcConfigurationProvider.java

+	 * @return a {@link ClientRegistration.Builder} that was initialized by the OpenID Provider Configuration.
+	 */
+	public static ClientRegistration.Builder issuer(String issuer) {
+		RestTemplate rest = new RestTemplate();


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

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

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

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.

jgrandja · 2018-05-18T13:31:51Z

...c/main/java/org/springframework/security/config/oauth2/client/OidcConfigurationProvider.java

+	 * @return a {@link ClientRegistration.Builder} that was initialized by the OpenID Provider Configuration.
+	 */
+	public static ClientRegistration.Builder issuer(String issuer) {
+		RestTemplate rest = new RestTemplate();


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

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

Is not need timeout settings?

@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

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.

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

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

jgrandja · 2018-05-18T13:46:44Z

...c/main/java/org/springframework/security/config/oauth2/client/OidcConfigurationProvider.java

+		Scope scope = metadata.getScopes();
+		if (scope == null) {
+			// If null, default to "openid" which must be supported
+			return Arrays.asList("openid");


Use OidcScopes.OPENID

jgrandja · 2018-05-18T13:54:52Z

...t/java/org/springframework/security/config/oauth2/client/OidcConfigurationProviderTests.java

+ * limitations under the License.
+ */
+
+package org.springframework.security.config.oauth2.client;


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

jgrandja · 2018-05-18T14:08:37Z

...c/main/java/org/springframework/security/config/oauth2/client/OidcConfigurationProvider.java

+		List<com.nimbusds.oauth2.sdk.auth.ClientAuthenticationMethod> metadataAuthMethods = metadata.getTokenEndpointAuthMethods();
+			// 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);


We also support ClientAuthenticationMethod.POST

jgrandja · 2018-05-18T14:16:48Z

...c/main/java/org/springframework/security/config/oauth2/client/OidcConfigurationProvider.java

+		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();


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.

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.

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

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

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.

jgrandja · 2018-05-18T14:53:24Z

I'd like to make a proposal for enhancing this feature.

I see the need for using the OpenID Provider Metadata in other features as well.

For example, I have an outstanding task for ID Token validation, see this comment

Also, we may leverage some of the signature/encryption related meatadata in one or more JwtDecoder's on the Resource Server side or possible on Client side for ID Token verification.

What if we extracted some of the code in OidcConfigurationProvider into:

package org.springframework.security.oauth2.client.oidc.discovery;


public final class OidcProviderDiscovery {

	public static OidcProviderMetadata discover(String issuer) {
		OidcProviderMetadata providerMetadata = new OidcProviderMetadata();

		// TODO Obtain OpenID Provider Configuration Information



		return providerMetadata;
	}

	public static class OidcProviderMetadata {

	}
}

This way OidcConfigurationProvider can leverage OidcProviderDiscovery and it can be further leveraged in features implemented at a later point.

What do you think?

rwinch · 2018-05-18T15:38:09Z

@jgrandja I am not sure if this makes sense. I was thinking that we will eventually add additional information to the ClientRegistration to account for things ike signature/encryption related metadata. From my perspective, all the information obtained from the endpoint is registration information. In any event, I'd suggest we hold off until we get that far.

jgrandja · 2018-05-18T15:44:19Z

I was thinking that we will eventually add additional information to the ClientRegistration to account for things like signature/encryption related metadata.

I'm not seeing that we would add this to ClientRegistration. A lot of that information in the Provider Metadata is not necessarily ClientRegistration information. Some of it is provider specific info and we would use OidcProviderMetadata in other features that are not contained in the ClientRegistration. I want to be careful that we don't bloat ClientRegistration too much.

Either way, we can hold off on this. I just figure it would be good to get this done now since you were in this already.

Issue: gh-5355

Validate the issuer that was returned matches the issuer that was was requested. Issue: gh-5355

rwinch · 2018-05-18T20:16:50Z

@sparty02 Now that this is merged into master, I created a Boot ticket for the Auto Configuration Support spring-projects/spring-boot#13210

See spring-projectsgh-5355

See gh-5355

Add ClientRegistration from OpenID Connect Discovery

0598d47

Fixes: spring-projectsgh-4413

rwinch added New Feature in: oauth2 An issue in OAuth2 modules (oauth2-core, oauth2-client, oauth2-resource-server, oauth2-jose) labels May 16, 2018

rwinch added this to the 5.1.0.M2 milestone May 16, 2018

rwinch assigned jgrandja May 16, 2018

rwinch requested a review from jgrandja May 16, 2018 17:35

jgrandja added OIDC in: config An issue in spring-security-config labels May 18, 2018

rwinch merged commit 0598d47 into spring-projects:master May 18, 2018

jgrandja reviewed May 18, 2018

View reviewed changes

rwinch added a commit that referenced this pull request May 18, 2018

Move OidcConfigurationProvider to .oidc package

9862c7b

Issue: gh-5355

rwinch added a commit that referenced this pull request May 18, 2018

Polish OidcConfigurationProvider Javadoc

cbf9a7b

Issue: gh-5355

rwinch added a commit that referenced this pull request May 18, 2018

OidcConfigurationProvider uses OidcScopes.OPENID

7853c75

Issue: gh-5355

rwinch added a commit that referenced this pull request May 18, 2018

Add OidcConfigurationProvider ClientAuthenticationMethod.POST support

18c8af8

Issue: gh-5355

rwinch added a commit that referenced this pull request May 18, 2018

OidcConfigurationProvider improve invalid issuer error

db88997

Issue: gh-5355

rwinch added a commit that referenced this pull request May 18, 2018

OidcConfigurationProvider validate returned issuer

9d55a64

Validate the issuer that was returned matches the issuer that was was requested. Issue: gh-5355

kazuki43zoo added a commit to kazuki43zoo/spring-security that referenced this pull request May 19, 2018

Polishing the OidcConfigurationProvider

e851b23

See spring-projectsgh-5355

kazuki43zoo mentioned this pull request May 19, 2018

Polishing the OidcConfigurationProvider #5363

Merged

rwinch pushed a commit that referenced this pull request May 21, 2018

Polishing the OidcConfigurationProvider

2c92496

See gh-5355

rwinch added the type: enhancement A general enhancement label May 3, 2019

Add ClientRegistration from OpenID Connect Discovery #5355

Add ClientRegistration from OpenID Connect Discovery #5355

Uh oh!

Conversation

rwinch commented May 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sparty02 commented May 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rwinch commented May 17, 2018

Uh oh!

sparty02 commented May 18, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jgrandja commented May 18, 2018

Uh oh!

rwinch commented May 18, 2018

Uh oh!

jgrandja commented May 18, 2018

Uh oh!

rwinch commented May 18, 2018

Uh oh!

Uh oh!

rwinch commented May 16, 2018 •

edited

Loading

sparty02 commented May 17, 2018 •

edited

Loading