Add support for dynamic JWS signature algorithm with JWKs (2) - Issue 7160

[hitchan]

CLA has been submitted.

This PR adds support for creating a JWSVerificationKeySelector dynamically based on the availability of JWK algorithms from a given JWK Set provided by an OIDC discovery document (Issue 7160). Non-standard or unknown algorithms will be silently ignored (see SignatureAlgorithm class). A warning log has also been added to let developers know if there was a problem fetching the JWK Set, although this does not halt the application from starting.

The original functionality is still in place that adds the RS256 algorithm if none are provided during initial configuration for backwards compatibility.

This functionality has been implemented for both standard and reactive versions.
This is the second version of this Pull Request. That simplifies the implementation and fixes unit tests.

[jzheaux]

Thanks, @hitchan, for the new PR, and for simplifying the approach.

I've left some feedback inline, but also I wonder if something even simpler would work that's closer to:

List<JWK> jwks = jwkSource.get(new JWKSelector(new JWKMatcher.Builder()
		.keyUses(SIGNATURE, null)
		.algorithms((Set) new JWSAlgorithm.Family())
		.build()), null);
Set<JWSAlgorithm> signatureAlgorithms = new HashSet<>();
for (JWK jwk : jwks) {
	signatureAlgorithms.add((JWSAlgorithm) jwk.getAlgorithm());
}
return new JWSVerificationKeySelector<>(signatureAlgorithms, jwkSource);

For brevity, I've left the error handling out.

What I like about it is that it uses the Nimbus API a bit more to do some of the work that you are doing manually.

[jzheaux]

Let's please use the JWKSource to retrieve JWKs. That will allow you to use a JWKMatcher that removes some of the validation you are doing in parseAlgorithms.

[hitchan]

This was my initial approach, however for reasons I can't explain (hopefully somebody smarter than me can explain), importing the JWKMatcher.Builder() class causes a torrent of unit test failures on seemingly unrelated unit tests such as "ClassDefNotFound" for classes such as "LdapServerBeanDefinitionParserTests". If I can get that issue resolved i would be more than happy to replace this with the JWKSource.

Another issue with using JWKSource in the NimbusReactiveJwkDecoder is that (as far as i can tell) JWKSecurityContextJWKSet is passed in during invocation of the processor() method, calling the get() method on that class at this stage will always yield no results. I do not have a context to pass into it that contains JWKs.

[jzheaux]

At the framework level, logging at DEBUG or TRACE is preferred.

Also, please include the exception in the log so that information isn't lost.

[hitchan]

Debug it is :-D

[jzheaux]

Please don't use * imports.

[hitchan]

Intellij =p, will fix.

[jzheaux]

I think that the code should probably throw an exception if there's a problem loading the JWK Set. That way, the context of the error isn't lost.

[hitchan]

How about creating a new exception class called "JwkException" for JWK related problems? I could use a similar message and throw that particular exception. Nimbus already provides a JwkException, but it is a checked exception. I would like to include this one within Spring Security as an unchecked exception.

[jzheaux]

If we create exceptions, it's important that it be clear whether it's the client's problem or the server's problem. That's one thing that's nice about IllegalArgumentException vs IllegalStateException.

Before we add a type, though, what's the use case you are trying to address? For example, is there a need to catch this exception and handle it differently from other runtime exceptions?

[hitchan]

There is no use case I can think of where this would be caught and handled differently. Would you recommend to use IllegalStateException over IllegalArgumentException? I think IllegalStateException would be more appropriate.

[jzheaux]

I typically use IllegalArgumentException to indicate user error, e.g. the application ought to be configuring itself differently.

[jzheaux]

Thanks for the PR, @hitchan. I took a look at some of the issues that you were having on your branch, and I simplified the solution by moving the support to JwtDecoders and ReactiveJwtDecoders. JwtDecoders already makes a network call on startup, but NimbusJwtDecoder does not, so moving this keeps this true as well as simplifies the code change.

This is now merged via your contributions in 2907864 and a polish of 366146f.