Skip to content

Add option to use jwksCache in NimbusJwtDecoder #7639

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

Add option to use jwksCache in NimbusJwtDecoder #7639

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
11 changes: 10 additions & 1 deletion ...2/oauth2-jose/src/main/java/org/springframework/security/oauth2/jwt/NimbusJwtDecoder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,8 @@

import com.nimbusds.jose.JWSAlgorithm;
import com.nimbusds.jose.RemoteKeySourceException;
import com.nimbusds.jose.jwk.source.DefaultJWKSetCache;
import com.nimbusds.jose.jwk.source.JWKSetCache;
import com.nimbusds.jose.jwk.source.JWKSource;
import com.nimbusds.jose.jwk.source.RemoteJWKSet;
import com.nimbusds.jose.proc.JWSKeySelector;
Expand Down Expand Up @@ -212,12 +214,19 @@ public static final class JwkSetUriJwtDecoderBuilder {
private String jwkSetUri;
private Set<SignatureAlgorithm> signatureAlgorithms = new HashSet<>();
private RestOperations restOperations = new RestTemplate();
private JWKSetCache jwkSetCache = new DefaultJWKSetCache();

private JwkSetUriJwtDecoderBuilder(String jwkSetUri) {
Assert.hasText(jwkSetUri, "jwkSetUri cannot be empty");
this.jwkSetUri = jwkSetUri;
}

public JwkSetUriJwtDecoderBuilder jwkSetCache(JWKSetCache jwkSetCache) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd prefer exposing an interface that is more powerful than the JWKSetCache interface. If an application really wants to supply a JWKSetCache, then it's quite simple to construct a DefaultJWTProcessor directly and pass that to the NimbusJwtDecoder constructor.

What if this took a org.springframework.cache.Cache instead? Then this builder could internally wrap that in a CacheJWKSetCache implementation, similar to the design of RestOperationsResourceRetreiver.

The nice thing about doing that is that an application can then use whatever caching mechanism they wish, be it Caffeine, Hazelcast, or something else.

Another nice this is that using Cache allows the entries to be key-value based which is better suited for a multi-tenant resource server that is caching keys from multiple issuers.

Copy link

@20fps 20fps Apr 3, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

@jzheaux Sry, but how should it help to multi-tenant resource server? I mean in multi-tenant environment we will have a separate AuthenticationManager for each tenant, that means for each tenant we will have: unique jwks-uri -> unique decoder -> so unique jwkSetCache, I don't see possible way for multiple keys to be stored in JWKSetCache with the current implementation

Copy link
Contributor

Choose a reason for hiding this comment

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

Good question, @20fps. I guess it comes down to what parts of your infra are multi-tenant. If your cache is multi-tenant, then each AuthenticationManager could have a JwtDecoder that shares the same cache - each JWK Set keyed, for example, by that tenant's JWK Set Uri.

If each tenant has its own cache, then that point is moot. My main point is that by introducing a key to the cache grants additional flexibility - multi-tenancy is one way that flexibility could be leveraged.

Assert.notNull(jwkSetCache, "jwkSetCache cannot be null");
this.jwkSetCache = jwkSetCache;
return this;
}

/**
* Append the given signing
* <a href="https://tools.ietf.org/html/rfc7515#section-4.1.1" target="_blank">algorithm</a>
Expand Down Expand Up @@ -279,7 +288,7 @@ JWSKeySelector<SecurityContext> jwsKeySelector(JWKSource<SecurityContext> jwkSou

JWTProcessor<SecurityContext> processor() {
ResourceRetriever jwkSetRetriever = new RestOperationsResourceRetriever(this.restOperations);
JWKSource<SecurityContext> jwkSource = new RemoteJWKSet<>(toURL(this.jwkSetUri), jwkSetRetriever);
JWKSource<SecurityContext> jwkSource = new RemoteJWKSet<>(toURL(this.jwkSetUri), jwkSetRetriever, jwkSetCache);
ConfigurableJWTProcessor<SecurityContext> jwtProcessor = new DefaultJWTProcessor<>();
jwtProcessor.setJWSKeySelector(jwsKeySelector(jwkSource));

Expand Down
6 changes: 6 additions & 0 deletions ...th2-jose/src/test/java/org/springframework/security/oauth2/jwt/NimbusJwtDecoderTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -244,6 +244,12 @@ public void jwsAlgorithmWhenNullThenThrowsException() {
Assertions.assertThatCode(() -> builder.jwsAlgorithm(null)).isInstanceOf(IllegalArgumentException.class);
}

@Test
Copy link
Contributor

Choose a reason for hiding this comment

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

Please also add a unit test that confirms that the provided cache gets used.

public void jwkSetCacheWhenNullThenThrowsException() {
NimbusJwtDecoder.JwkSetUriJwtDecoderBuilder builder = withJwkSetUri(JWK_SET_URI);
Assertions.assertThatCode(() -> builder.jwkSetCache(null)).isInstanceOf(IllegalArgumentException.class);
}

@Test
public void restOperationsWhenNullThenThrowsException() {
NimbusJwtDecoder.JwkSetUriJwtDecoderBuilder builder = withJwkSetUri(JWK_SET_URI);
Expand Down