We should register a RestOperations @Bean in OAuth2ClientConfiguration that is configured with default settings and is used as the default via setRestOperations(restOperations) in the various oauth2-client flows - see #5601.
The well-known @Bean should be named springSecurityOAuth2RestOperations and automatically be injected/used as the default in all oauth2-client flows. The user has the ability to register a @Bean with the same name, which will effectively override the default.
For now, the default @Bean will be configured with default connect and read timeout settings for 30 secs. However, the user may override this @Bean and provide custom settings for Proxy, SSL, HTTP Client backing implementation, etc.
Comment From: rwinch
I'm not sure we should be quite as granular as springSecurityOAuth2RestOperations. Otherwise we run into a pretty slippery slope on bean names. Do we add springSecurityOAuth2ClientRestOperations, springSecurityOAuth2ResourceServerRestOperations, etc? Likely springSecurityRestOperations is good enough or potentially even just a Bean rather than the name.
Comment From: jgrandja
Likely springSecurityRestOperations is good enough or potentially even just a Bean rather than the name.
I'm fine with springSecurityRestOperations. But I'm not sure about...
even just a Bean rather than the name
How do we know RestOperations bean is meant for oauth2-client flows? The user may have configured a bean for another use case within the app.
Comment From: jgrandja
Instead of registering a well-known @Bean name, we should discover the @Bean in OAuth2ClientConfiguration by type only.
For the @Bean type, we should consider borrowing the design from JwtDecoderFactory for a possible solution.
For example, OidcIdTokenDecoderFactory implements JwtDecoderFactory, which is supplied a ClientRegistration to determine the type of JwtDecoder to create for the specific client.
public interface JwtDecoderFactory<C> {
// context - provides contextual information used to create a specific JwtDecoder
JwtDecoder createDecoder(C context);
}
public final class OidcIdTokenDecoderFactory implements JwtDecoderFactory<ClientRegistration> {
@Override
public JwtDecoder createDecoder(ClientRegistration clientRegistration) {
....
}
We could have a similar design for creating RestOperations, as follows:
public interface RestOperationsFactory<C> {
// context - provides contextual information used to create a specific RestOperations
RestOperations createRestOperations(C context);
}
public final class OAuth2ClientRestOperationsFactory implements RestOperationsFactory<ClientRegistration> {
@Override
public RestOperations createRestOperations(ClientRegistration clientRegistration) {
....
}
If a user needs to provide a configured RestOperations to be used in the various oauth2-client flows (eg. ClientRegistrations) then they can register a RestOperationsFactory<ClientRegistration> @Bean, which will be discovered in OAuth2ClientConfiguration and directly configured in the components implementing the oauth2-client flows.
For example, the RestOperations created from the RestOperationsFactory<ClientRegistration> @Bean would be configured directly to DefaultAuthorizationCodeTokenResponseClient.setRestOperations().
We'll need to figure out how to configure the RestTemplate in ClientRegistrations. We could expose public static void setRestOperations(RestOperations restOperations) and the RestOperationsFactory<ClientRegistration> @Bean would supply the RestOperations. But the issue here is that there is no ClientRegistration context that can be provided to RestOperationsFactory<ClientRegistration> so some further thought is required for this case.
Comment From: rwinch
Again I think this is too granular. We don't want to add factories for everything that can be created.
Comment From: jgrandja
@rwinch Can you elaborate why you think this design is too granular?
We don't want to add factories for everything that can be created
Why do you think a factory is not suitable here? FYI, we only have one factory defined in OAuth 2.0 codebase thus far - JwtDecoderFactory. I'm just trying to understand as your comment does not provide much of an explanation.
Is there another solution you have in mind that you can propose?
Comment From: rwinch
If you are creating RestOperationsFactory<ClientRegistration> you might as well just define the DefaultAuthorizationCodeTokenResponseClient bean in the first place.
Perhaps we could allow users to define their own ResourceRetriever and that would be used throughout Nimbus? Although that likely misses things like creating the ClientRegistrations.
Alternatively, I could see looking for a RestTemplate as a default for all of Spring Security, but I don't want to have custom factories for each Bean
Comment From: jgrandja
If you are creating RestOperationsFactory
you might as well just define the DefaultAuthorizationCodeTokenResponseClient bean in the first place.
Registering DefaultAuthorizationCodeTokenResponseClient (and other clients) will still involve a lot of manual configuration from the user, which is what we are trying to solve. The factory approach I have in mind will be a lot less configuration from user. I don't feel I'm getting my idea across. It might be best to put together a draft PR to demonstrate.
I could see looking for a RestTemplate as a default for all of Spring Security
Ok, so it sounds like you are favouring a well-known bean name, as per comment?
Comment From: rwinch
Yes. Can we just have a single well known bean name for all of security? I doubt that most setups will need multiple different RestTemplate instances. If they do, they can always define the beans explicitly
Comment From: xenoterracide
one possible exception, tests.
Comment From: larsw
I don't have any strong opinions with regards to the design of the API, but we've met this obstacle while trying to customize the trust store to be used for the outgoing connection against the authorization server.
After reading through the source, we found out that the RestTemplate instance was new'ed locally, and as far as we could tell, was not externally customizable.
It was quite confusing to discover that the client used RestTemplate internally with one way of doing customization, while the the client we're using to do the actual API call is using WebFlux with another customization strategy :-/
a) Is there any known workarounds for doing this (apart from using the java command line option to set the trust store or change the "global" cacerts file) ? b) If not, will this issue be fixed in the foreseeable future?
Comment From: phisad
We also ran into a similar issue, when facing a http proxy to be configured.
First, we simply put the Spring Security 5.2.2.RELEASE modules spring-security-oauth2-resource-server and spring-security-oauth2-jose into the classpath to get the default configurations. (Spring Boot 2.2.5.RELEASE)
This lead to the following exception on startup, because the initial openid-configuration was not fetchable because of the companies internal http proxy.
Caused by: org.springframework.web.client.ResourceAccessException: I/O error on GET request for "https://login.microsoftonline.com/fcb2b37b-5da0-466b-9b83-0014b67a7c78/v2.0/.well-known/openid-configuration": Operation timed out (Connection timed out); nested exception is java.net.ConnectException: Operation timed out (Connection timed out)
Then we set the JVM arguments '-Dhttps.proxyHost' and '-Dhttps.proxyPort' which lead to a successful retrieval of the openid-configuration.
Nonetheless, another error occured now at runtime, when an incoming token had to be validated. The NimbusReactiveJwtDecoder is trying to fetch the jwks using a WebClient.
Caused by: io.netty.channel.AbstractChannel$AnnotatedConnectException: finishConnect(..) failed: Operation timed out: login.microsoftonline.com/40.126.1.166:443 Suppressed: reactor.core.publisher.FluxOnAssembly$OnAssemblyException: Error has been observed at the following site(s): |_ checkpoint Request to GET https://login.microsoftonline.com/fcb2b37b-5da0-466b-9b83-0014b67a7c78/discovery/v2.0/keys [DefaultWebClient]
The reason for this is that we are using Spring Cloud Gateway which requires Webflux. Now, with Webflux on the classpath, the reactive WebClient is initialized. In constrast to this, the initial retrieval went fine, because JwtDecoderProviderConfigurationUtils uses a RestTemplate which is applying the JVM args.
The main problem here is as far as I can see that the Netty WebClient does not honor the JVM arguments. (https://github.com/reactor/reactor-netty/issues/887#issuecomment-549439355)
So we had to configure the WebClient to be aware of the proxy. Unfortunately, the NimbusReactiveJwtDecoder is by default using a processor() with a web client created by WebClient.create() instead of using a pre-configured one.
Thus in the end, we had to provide a fully customized decoder. We copied the default configuration procedure and applied builder.webClient(proxyWebClient()) on the builder.
@Bean
public ReactiveJwtDecoder jwtDecoder() {
final Map<String, Object> configuration = JwtDecoderProviderConfigurationUtils.getConfigurationForOidcIssuerLocation(oidcIssuerLocation);
JwtDecoderProviderConfigurationUtils.validateIssuer(configuration, oidcIssuerLocation);
final OAuth2TokenValidator<Jwt> jwtValidator = JwtValidators.createDefaultWithIssuer(oidcIssuerLocation);
final NimbusReactiveJwtDecoder jwtDecoder = withJwkSetUri(configuration.get("jwks_uri").toString())
.webClient(proxyWebClient())
.build();
jwtDecoder.setJwtValidator(jwtValidator);
return jwtDecoder;
}
It would be really nice to make at least webClient(...) available to ReactiveJwtDecoders or NimbusReactiveJwtDecoder. The best option would be a spring-provided workaround for creating the WebClient or making the WebClient automatically aware of proxy arguments.
Comment From: Budlee
One point here is that as the resource server can now support multiple tennants you may not want to use the same Rest Template for each issuer. You may have different setups for each one (different proxies, different security commections). I have found an issue with this in the JwtDecoderProviderConfigurationUtils. the RestTemplate not being exposed is a problem.
I have made a draft PR to open it up. By no means do I think this is the correct solution, however it is a start on being able to open this up. would be great to get some feedback on thoughts https://github.com/spring-projects/spring-security/pull/8588
Comment From: jgrandja
Closing in favour of #8882. Please provide any additional feedback there.