Refactor JwtConfigurer

[rwinch]

Summary

I think we should refactor JwtConfigurer in a few ways

http
    .oauth2()
        .resourceServer()
                .jwt().signature().keys(new URL(issuer.getJwkSetUri()));

The first thing is that we should not configure the JWK Set URI on signature(). The reason is that according to the specification:

JWKs and JWK Sets are used in the JSON Web Signature [JWS] and JSON Web Encryption [JWE] specifications.

That means that JWK Set URI is not specific to signatures. It is also used in encryption. Instead, we should remove signature() and configure the JWK Set URI on the JwtConfigurer

The next thing is that the keys(URL) method should be refactored in two ways.

• It should be renamed to include the expected format (i.e. jwkSetUri). The first reason is that there may be multiple ways of exposing keys via a URL. We should be explicit about the expected format. The second reason is by naming it jwkSetUri vs keys users know exactly what to google when they are looking for determining what the parameter is and what the format of the response should be without the need to even refer to the javadoc.
• It should accept a String instead of a URL to reduce the verbosity (both new URI and the handling of the Exception that URI constructor throws) and eliminate the unnecessary Object creation.

[jgrandja]

I managed to find the comments I provided in the commits early this week.

Here is the feedback I provided for the SecurityConfigurer:

The current DSL specifies the following configuration points:

    http
        .oauth2()
            .resourceServer()
                .realmName("oauth-realm")
                .jwt()
                    .algorithm("algorithm")
                    .authoritiesPopulator(authoritiesPopulator)
                    .scopeAttributeName("scp")
                    .signature()
                        .keys("https://provide.com/jwk/keys");

The following configuration points are missing:

    http
        .oauth2()
            .resourceServer()
                .bearerTokenResolver(bearerTokenResolver)
                .authenticationEntryPoint(authenticationEntryPoint)
                .accessDeniedHandler(accessDeniedHandler)

I don't think we need .resourceServer().realmName("oauth-realm") as this can be set in BearerTokenAuthenticationEntryPoint.setDefaultRealmName() and than supplied to .resourceServer().authenticationEntryPoint()

Please remove .resourceServer().jwt().algorithm("algorithm") as this really is an implementation detail. We can provide the default algorithm(s) when the Configurer creates the JwtDecoder.

I believe the .resourceServer().jwt().scopeAttributeName("scp") should be configured in the .authoritiesPopulator instance. It's responsible for extracting token attributes into authorities. Therefore, it should know how to extract scope attributes and translate those to authorities. I think we can remove this as well and should be configured in the .authoritiesPopulator instance within the Configurer.

Regarding .resourceServer().jwt().signature().keys()...if I recall correctly during our previous discussion, I thought we were going with .resourceServer().jwt().keys() for all Key configuration? All being, signature verification keys, decryption keys, jwk set keys, single PublicKey, etc. I'm not sure if we need to split the hierarchy with jwt().signature() and later jwt().decryption()?

[jzheaux]

The first thing is that we should not configure the JWK Set URI on signature().

Agreed--this was definitely a consequence of attempting to overload the keys method for every scenario, which is an over-abstraction.

If we say that we don't want to overload the keys method for JWK Set URI, then the draw to pull it under signature is moot.

It should be renamed to include the expected format

I'm fine with this. There is also value here in aligning it with the name used in ClientRegistration on the client side.

It should accept a String instead of a URL

Agreed that this is a good simplification insofar as the type was originally there to allow keys to be overloaded for additional use cases.

if I recall correctly during our previous discussion, I thought we were going with .resourceServer().jwt().keys()

Somehow I do not remember this conversation. For historical purposes, if you find the link to that convo, would you link it here?

I'm not sure if we need to split the hierarchy with jwt().signature() and later jwt().decryption()?

I think this is an open discussion, yes, though I do lean towards having this distinction in some way to support things like SecretKeys. It might not be manifest in the method name, though. Since we've resolved to just do what is necessary for JWK Set Uris for now, I think we can defer that to a future ticket.

So, just one more thing, @rwinch and @jgrandja, and that is that I just want to keep an eye out for our discussion about IssuerRegistrations. Since the jwkSetUri on the client sits on ClientRegistrations, I am a little nervous about placing it right here on the DSL. Thoughts about our ability to pivot given the future result of that conversation?

[jgrandja]

if I recall correctly during our previous discussion, I thought we were going with .resourceServer().jwt().keys()

I think we spoke over zoom about this? Either way can't find the convo

I'm not sure if we need to split the hierarchy with jwt().signature() and later jwt().decryption()?

As @rwinch mentioned, JWT can be JWS and/or JWE and configured via the jwkSetUrl. So pushing it up to the JwtConfigurer makes sense. Otherwise we would need to provide:

jwt().signature().jwkSetUri()
jwt().decryption().jwkSetUri()

which wouldn't be great.

I'm still mulling over something like this:

http
  .oauth2()
    .resourceServer()
    .jwt()
      .keys()
        .jwkSetUri(jwkSetUri)
        .verifyKey(rsaKey1)
        .decryptKey(rsaKey2)

Thoughts?

[jzheaux]

Either way can't find the convo

np

Thoughts?

The indentation seems to communicate that verifyKey and decryptKey are couched under JWK Sets. This would require folks to configure a remote uri in order to also configure local keys, where I think of these as two distinct approaches to providing keys.

(Or, that might have just been a rendering error. If so, ignore my above comments. :) )

So, I'd probably tweak that to be:

http
    .oauth2().resourceServer()
        .jwt()
            .keys().jwkSetUri(...)

or

http
    .oauth2().resourceServer()
        .jwt()
            .keys()
                .verify(... some different types possible ...)
                .decrypt(.... some different types possible ...)

Where some of the different types possible are java.security.Key, and some kind of KeyProvider.

And while I do like the structural benefit of having all "key" configuration underneath its own parent, keys(), it is more for the user to type, and so we should only do it if the extra typing lends the user clarity and/or expressive power.

I see this as being just as clear to the user:

http
    .oauth2().resourceServer()
        .jwt()
            .jwkSetUri(...)

or

http
    .oauth2().resourceServer()
        .jwt()
            .verifyKeys(...)
            .decryptKeys(...) // my ocd DRY doesn't like having two methods end in "Keys" :)

Do you see keys() as being clearer or more expressive?

[jgrandja]

The indentation seems to communicate that verifyKey and decryptKey are couched under JWK Sets.

My mistake. It's all under keys(). I corrected the original DSL.

Do you see keys() as being clearer or more expressive?

keys() allows the grouping of all Key Configuration.

We may have other configuration points under jwt() that is not Key Configuration related.

I prefer your suggested naming:

http
    .oauth2()
      .resourceServer()
        .jwt()
            .keys()
                .jwkSetUri(jwkSetUri)
                .verify(... some different types possible ...)
                .decrypt(.... some different types possible ...)

[rwinch]

Typically more hierarchy is added when something is optional. I think that keys() is unnecessary hierarchy since keys are inherent to JWT. I think that this is the best option.

http
    .oauth2().resourceServer()
        .jwt()
            .jwkSetUri(...)