Add reasonable timeout to JwtDecoderProviderConfigurationUtils and NimbusJwtDecoder

[vonnahme]

Expected Behavior

These classes should use reasonable default timeouts to avoid the possibility of a connection hanging.

Current Behavior

A default RestTemplate with no timeout configured is used.

https://github.com/spring-projects/spring-security/blob/6.2.0/oauth2/oauth2-jose/src/main/java/org/springframework/security/oauth2/jwt/JwtDecoderProviderConfigurationUtils.java#L66

https://github.com/spring-projects/spring-security/blob/6.2.0/oauth2/oauth2-jose/src/main/java/org/springframework/security/oauth2/jwt/NimbusJwtDecoder.java#L271

Context

We had network issues recently and saw hung threads in these classes due to the connections never being released.

It appears that nimbus sets a default timeout on http connections they make: https://www.javadoc.io/static/com.nimbusds/nimbus-jose-jwt/9.37.3/com/nimbusds/jose/jwk/source/JWKSourceBuilder.html#DEFAULT_HTTP_CONNECT_TIMEOUT

Since both of these classes rely on nimbus, perhaps the nimbus timeout settings could be re-used?

After researching this a bit, it does seem I could configure this at the JVM by setting

-Dsun.net.client.defaultConnectTimeout=
-Dsun.net.client.defaultReadTimeout=

but that's more broad than I would prefer.

[jzheaux]

Related to #11232

[jzheaux]

Thanks for reaching out, @vonnahme. I think it makes sense to provide a timeout, at least to the places where RestTemplate is not configurable like JwtDecoderProviderConfigurationUtils.

That said, I'm not sure yet if this would help your situation. Can you please provide how you are presently constructing a NimbusJwtDecoder instance in your application?

In the meantime, you can set your own RestTemplate timeouts as follows:

@Bean 
JwtDecoder jwtDecoder(RestTemplateBuilder builder) {
    RestOperations rest = builder.setConnectionTimeout(500).setReadTimeout(500).build();
    NimbusJwtDecoder jwtDecoder = NimbusJwtDecoder.withIssuerLocation("https://issuer.example.org")
        .restOperations(rest).build();
    jwtDecoder.setJwtValidator(JwtValidators.createDefaultWithIssuer("https://issuer.example.org"));
    return jwtDecoder;
}

[vonnahme]

Thanks for reaching out, @vonnahme. I think it makes sense to provide a timeout, at least to the places where RestTemplate is not configurable like JwtDecoderProviderConfigurationUtils.

Great!

That said, I'm not sure yet if this would help your situation.

It definitely would have :-) When a thread gets hung indefinitely, thread pools fill up, the server becomes unresponsive, and teams are scrambling to restart things faster than they can fill up. Having the timeout wouldn't have stopped the requests from hanging, but would have helped the infrastructure stay healthy / the failure would have stayed local rather than cascading throughout the environment.

Can you please provide how you are presently constructing a NimbusJwtDecoder instance in your application?

We set the property

spring.security.oauth2.resourceserver.jwt.issuer-uri

So spring-boot builds it. Looks like the code is here: https://github.com/spring-projects/spring-boot/blob/v2.7.18/spring-boot-project/spring-boot-autoconfigure/src/main/java/org/springframework/boot/autoconfigure/security/oauth2/resource/servlet/OAuth2ResourceServerJwtConfiguration.java#L141

In the meantime, you can set your own RestTemplate timeouts as follows:

To be clear, the root networking issue we were facing has been found and corrected. For mitigation we were adding timeouts wherever we found the problem, but it was a bit like bailing water from a sinking boat with a spoon. We'll likely add some blanket longer timeouts on many JVMs in our environment, but this seemed like a use case where a more reasonable default could be chosen.

[jzheaux]

It definitely would have :-)

Sorry, I meant that there are increasingly customizable ways to configure a decoder. Depending on what you were doing, updating only the non-configurable RestTemplate instances wouldn't have helped (since you might not be exercising those code paths).

Great!

Splendid. Can you provide a PR that sets the RestTemplate instance to have default timeout values if the JVM settings are not present already?

IOW, the connect timeout value, for example, would be computed something like this:

int connectTimeout = Integer.valueOf(System.getProperty("sun.net.client.defaultConnectTimeout", "30000"));

Also, set the initial value of JwkSetUriJwtDecoderBuilder#restOperations to this static value.

And while 30s is much higher than 500ms, we want to do our best not to break people unnecessarily when upgrading. I'd rather not have applications updating to 6.3 and suddenly having to configure a RestTemplate because now it's dropping connections. This also aligns with a similar use case in ClientRegistrations.

[spring-projects-issues]

If you would like us to look at this issue, please provide the requested information. If the information is not provided within the next 7 days this issue will be closed.

[MrJovanovic13]

Hi. I wanted to contribute to Spring Security and saw that this issue has tag ideal-for-contribution. Could I work on this issue? @jzheaux

If yes, I have a couple of questions before I start working on it.
Since RestTemplateBuilder is not available as a dependancy, the above mentioned method will not work for setting a default timeout. Should I set a ClientHttpRequestFactory which would then allow me to set Its timeout values?

That also leaves me with a question on which ClientHttpRequestFactory to use. I see multiple options, maybe SimpleClientHttpRequestFactory or JdkClientHttpRequestFactory ?

[jzheaux]

Yes, @MrJovanovic13, it would be a pleasure to work with you on this enhancement.

That also leaves me with a question on which ClientHttpRequestFactory to use. I see multiple options, maybe SimpleClientHttpRequestFactory or JdkClientHttpRequestFactory?

Yes, please see ClientRegistrations for an example while also considering this comment for computing each value.