Skip to content
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

Make RestTemplate used by ClientRegistrations (Discovery) configurable #7027

Closed
shibuyaku opened this issue Jun 20, 2019 · 15 comments
Closed
Assignees
Labels
in: oauth2 An issue in OAuth2 modules (oauth2-core, oauth2-client, oauth2-resource-server, oauth2-jose) status: duplicate A duplicate of another issue

Comments

@shibuyaku
Copy link

shibuyaku commented Jun 20, 2019

Summary

spring-security-oauth2-client uses a RestTemplate for openid/oauth Discovery that is not configurable. This is unuseable in scenarios where you need to adjust the RestTemplate. Example: You need to use a proxy and configure auth.

Actual Behavior

ClientRegistrations class uses a RestTemplate for doing OpenId Discovery that is not configurable, since it is not using RestTemplateBuilder or something comparable.

OpenId discovery is done by querying issuerUri + "/.well-known/openid-configuration" (for oidc) or isserUri + "/.well-known/oauth-authorization-server" (for oauth).

Current implementation:
RestTemplate rest = new RestTemplate()

Expected Behavior

ClientRegistrations should use a configurable RestTemplate for doing OpenId Discovery. One should be able to configure the requestFactory, interceptors, errorHandler and so on of that RestTemplate.

Configuration

Version

5.2.0.M3 and 5.1.5.RELEASE

Sample

Related #5607

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Jun 20, 2019
@shibuyaku
Copy link
Author

shibuyaku commented Jun 20, 2019

We usually use org.springframework.boot.web.client.RestTemplateBuilder to achieve this in our code, but I can not immediately tell if that would be a valid approach in the spring-security project.

If so:
RestTemplateBuilder().build() instead of RestTemplate() would allow a user to configure everything he wants using org.springframework.boot.web.client.RestTemplateCustomizer.

@shibuyaku
Copy link
Author

FYI: Found 6 more occurences of new RestTemplate() that will also be affected and unconfigurable, 2 of them in spring-security-oauth2-client.

We stumbled upon this because the RestTemplate() will try to use the host/port of a proxy that is configured via environment variables, but ignores the user and password.

Complete list:

org.springframework.security.oauth2.client.registration.ClientRegistrations
org.springframework.security.oauth2.client.userinfo.DefaultOAuth2UserService
org.springframework.security.oauth2.client.userinfo.CustomUserTypesOAuth2UserService

org.springframework.security.oauth2.jwt.JwtDecoders
org.springframework.security.oauth2.jwt.NimbusJwtDecoder
org.springframework.security.oauth2.jwt.ReactiveJwtDecoders

org.springframework.security.oauth2.server.resource.introspection.NimbusOAuth2TokenIntrospectionClient

I'll think about a solution for this over the weekend, to write a pull request. For now I only had bad ideas that even I dont like.

@shibuyaku
Copy link
Author

#5607 is related, I think.

@jgrandja
Copy link
Contributor

@shibuyaku Instead of using OpenID Connect Discovery via ClientRegistrations.fromOidcIssuerLocation(), you can explicitly configure the provider properties (authorization-uri, token-uri, jwk-set-uri, etc.) and than supply a custom configured RestTemplate to DefaultOAuth2UserService.setRestOperations() and than supply the DefaultOAuth2UserService to OidcUserService.setOauth2UserService(). The OidcUserService instance can than be configured via oauth2Login().userInfoEndpoint().oidcUserService(). This should take care of your use case.

Also see the reference doc.

As far as allowing a configurable RestOperations for ClientRegistrations, this will be tricky given that it was designed as a utility with static factory methods. We'll have to give this some more thought. Either way, I've provided you a solution that will work for your use case - without using OIDC Discovery.

@jgrandja jgrandja added in: oauth2 An issue in OAuth2 modules (oauth2-core, oauth2-client, oauth2-resource-server, oauth2-jose) and removed status: waiting-for-triage An issue we've not yet triaged labels Jun 25, 2019
@shibuyaku
Copy link
Author

Thanks for the input. We got it running with your solution, without OIDC discovery.
For users facing the same issue:
-Configuration class providing an InMemoryClientRegistrationRepository bean with manual config, as described by @jgrandja #7027 (comment)
-ProxyConfig class configuring Authenticator.setDefault(...) - this allowed us to avoid supplying a custom configured rest template. Probably you want something like this anyways when using a proxy.

@Budlee
Copy link
Contributor

Budlee commented Oct 22, 2019

This is the same problem as with
#7391 but instead of RestTemplate it is WebClient. @jgrandja What do you recommend in this case
e.g.
org/springframework/security/oauth2/jwt/ReactiveRemoteJWKSource.java:45

@xenoterracide
Copy link

xenoterracide commented Oct 22, 2019

Same case https://stackoverflow.com/q/58495332/206466 I think, I need to customize it for debugging what's going wrong. I'm not sure what @jgrandja describes works in my use case... I certainly don't understand it well enough to implement.

could we just have a customizer for this instead of having to reimplement all the services, especially since they are final and can't be extended?

@jgrandja
Copy link
Contributor

jgrandja commented Oct 24, 2019

@Budlee ReactiveRemoteJWKSource.webClient will get set if you configure JwkSetUriReactiveJwtDecoderBuilder.withJwkSetUri(jwkSetUri).webClient(webClient)....

@jgrandja
Copy link
Contributor

@xenoterracide I took a look at your SO question. You can supply a custom RestOperations as follows:

Current

@Bean
   OAuth2AuthorizedClientManager authorizedClientManager(
        ClientRegistrationRepository clientRegistrationRepository,
        OAuth2AuthorizedClientRepository authorizedClientRepository) {

        OAuth2AuthorizedClientProvider authorizedClientProvider =
            OAuth2AuthorizedClientProviderBuilder.builder()
                .clientCredentials()
                .build();

        DefaultOAuth2AuthorizedClientManager authorizedClientManager =
            new DefaultOAuth2AuthorizedClientManager(
                clientRegistrationRepository, authorizedClientRepository);
        authorizedClientManager.setAuthorizedClientProvider(authorizedClientProvider);

        return authorizedClientManager;
    }

Updated

	@Bean
	OAuth2AuthorizedClientManager authorizedClientManager(
			ClientRegistrationRepository clientRegistrationRepository,
			OAuth2AuthorizedClientRepository authorizedClientRepository) {

		OAuth2AuthorizedClientProvider authorizedClientProvider =
				OAuth2AuthorizedClientProviderBuilder.builder()
						.clientCredentials(configurer ->
								configurer.accessTokenResponseClient(clientCredentialsTokenResponseClient()))
						.build();

		DefaultOAuth2AuthorizedClientManager authorizedClientManager =
				new DefaultOAuth2AuthorizedClientManager(
						clientRegistrationRepository, authorizedClientRepository);
		authorizedClientManager.setAuthorizedClientProvider(authorizedClientProvider);

		return authorizedClientManager;
	}

	private OAuth2AccessTokenResponseClient<OAuth2ClientCredentialsGrantRequest> clientCredentialsTokenResponseClient() {
		RestOperations accessTokenClient = null;		// TODO Configure
		DefaultClientCredentialsTokenResponseClient clientCredentialsTokenResponseClient = new DefaultClientCredentialsTokenResponseClient();
		clientCredentialsTokenResponseClient.setRestOperations(accessTokenClient);
		return clientCredentialsTokenResponseClient;
	}

@jgrandja jgrandja self-assigned this Oct 24, 2019
@xenoterracide
Copy link

ok, but it'd be nice to be able to just setRestOperations on all objects at once (side note, I'm a little against converting to WebClient right now as there's an open bug for getting this same kind of logging out of it in reactor-netty)

@jgrandja
Copy link
Contributor

@xenoterracide We have an open ticket for this #5607

@xenoterracide
Copy link

Thanks, I only found this one

@andysworkshop
Copy link

Just hit this issue myself in our corporate environment. Because of this line I'll have to duplicate all the nice provided functionality using an http client configured with our proxy. Will be watching this issue with interest.

@shmyer
Copy link

shmyer commented Jul 20, 2020

Came across this issue aswell since we are using a corporate proxy and OIDC Discovery was ignoring the proxy username and password provided via system properties (-Dhttp.* and -Dhttps.*) during startup. Disabled Discovery for now and using static uri's in provider configuration.

@jgrandja
Copy link
Contributor

Closing in favour of #8882. Please see ClientRegistrations and provide any additional feedback there.

@jgrandja jgrandja removed the type: enhancement A general enhancement label Jul 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: oauth2 An issue in OAuth2 modules (oauth2-core, oauth2-client, oauth2-resource-server, oauth2-jose) status: duplicate A duplicate of another issue
Projects
None yet
Development

No branches or pull requests

7 participants