JWT encoding support for OAuth 2.0

[krajcsovszkig-ms]

This PR is to satisfy the JWT-signing and -encoding requirements of #6881

See also PR #8445

This mainly concerns the spring-security-oauth2-jose project.

The missing pieces we aim to add here are:

• Encoder counterparts to the existing Decoders in org.springframework.security.oauth2.jwt
• A class (or set of classes) similar to Jwt that can store a full or partial set of JWT claims and JOSE headers
• Optionally adding support for EC (and potentially further) signing algorithms to NimbusJwtDecoder

[pivotal-issuemaster]

@krajcsovszkig-ms Please sign the Contributor License Agreement!

Click here to manually synchronize the status of this Pull Request.

See the FAQ for frequently asked questions.

[krajcsovszkig-ms]

@jgrandja

I thought the JwtEncoder should mimic the JwtDecoder - that converts a String to a Jwt, so the encoder should convert a Jwt to a String. I'm fine with it returning a Jwt as well since that contains the String representation in its tokenValue and we obviously can't use Jwt as the argument as it is now, because it needs to have a tokenValue which this encoder is supposed to create. I've had another look and I think we should create a JwtClaimsSet class which will serve this purpose, and could also be used to store override claims in the ClientRegistration to replace ones we would auto-generate.

I figured an implementation of the JwtEncoder would be configured with the signing algorithm and the keys and the JWT claims would be passed to the encode method. I'm not sure yet if we can rely on the encoder to create the JWT headers or we should also have a way of supplying them to it.

[pivotal-issuemaster]

@krajcsovszkig-ms Thank you for signing the Contributor License Agreement!

[jzheaux]

It might give a more type-safe experience to do:

String encode(Consumer<Jwt.Builder> jwtBuilderConsumer) throws JwtException

The other nice thing about this is that it makes it easier for the caller to override whatever defaults the JwtEncoder implementation sets since the implementation would apply the Consumer just before signing.

cc @jgrandja

[krajcsovszkig-ms]

@jzheaux I don't think the JwtEncoder should set any default claims, those are going to come from the ClientRegistration, so a Consumer doesn't really feel intuitive to me here. For type safety we could create a JwtClaimsSet class, which could be used for the overrides as well in the ClientRegistration.

[jgrandja]

@krajcsovszkig-ms @jzheaux We are also needing a JwtEncoder in the new Spring Authorization Server project #spring-authorization-server/81 and the initial design has started there too. Please follow that issue so we can align on the API for JwtEncoder with the goal/hope that we can derive a unified interface to suit both client and authorization server.

/cc @anoopgarlapati

I think the design proposed by @jzheaux might work.

Given the following:

String encode(Consumer<Jwt.Builder> jwtBuilderConsumer) throws JwtException

An application would configure the Consumer and then customize the Jwt as follows:

builder
	.headers(headers -> headers.put("custom-header", "header-value"))
	.claims(claims -> claims.put("custom-claim", "claim-value"));

We already have the capability to set a Consumer for both the claims and headers, which would allow an application to enhance/override the claims set and/or headers. The JwtEncoder implementation would initialize the claims and headers first before allowing the Consumer to apply its own customizations.

The application would configure the Consumer via the JwtEncoder implementation, for example:

public final class NimbusJwtEncoder implements JwtEncoder {

	public void setJwtCustomizer(Consumer<Jwt.Builder> jwtCustomizer) {
	}

Another option to consider is possibly using a JwtEncoderFactory for configuring the Consumer. See how OidcIdTokenDecoderFactory is implemented and how it configures and creates a NimbusJwtDecoder.

An alternative option for JwtEncoder to consider:

Jwt encode(Map<String, Object> headers, Map<String, Object> claimsSet)

NOTE: I'm not convinced on the Map inputs, we might consider type-safe classes instead. I think starting off with Map is fine for initial design and then we could consider new classes if necessary.

Thoughts?

[krajcsovszkig-ms]

@jgrandja

Reusing this stuff in the AS project makes sense.

I think mimicking the design of the existing JwtDecoder would also make sense. Since that has a decode String -> Jwt method, I think an encode Jwt -> String would make the most sense here, but since the Jwt is not particularly suitable for this purpose, it could be an encode JwtClaimsSet -> String, which is still similar enough.

I don't think overriding the headers would be necessary very often (correct me if my assumption is wrong), so providing that via some other means (e.g. the builder for the implementation) would be good. Alternatively instead of a JwtClaimsSet we could create an PlainJwt type which has the claims and the headers.

If it's deemed necessary to have a Consumer for overriding the claims supplied to encode, I think it should also be configured via the builder for the implementation since it's more of an optional feature (and it could also go in a followup PR).

WDYT?

[jgrandja]

@krajcsovszkig-ms

I don't think overriding the headers would be necessary very often

I'm 99% sure there will be a use case where the header(s) would need to be modified so we need to account for this. Given that a Jwt cannot exist without headers and claims set, it seems logical that the input to encode is supplied both of these in someway. I also feel that the return type should be Jwt instead of String.

[krajcsovszkig-ms]

@jgrandja

Sure, it'll be necessary occasionally, but perhaps not often enough to require it being a parameter to the method. However it also feels natural to make it a parameter, so I'm fine either way.

What would you prefer, separate parameters for the headers and the claims, or a new type that has both?

I'm OK with returning Jwt as well.

[anoopgarlapati]

I agree that using Consumer<Jwt.Builder> jwtCustomizer as a way to customize the headers and claims just before the Jwt is built is the best way to provide custom headers and claims. I also feel returning a Jwt is better in case some functionality in future might need access to headers and claims.

However, I agree with @krajcsovszkig-ms that encode() should accept Map<String, Object> jwtClaimsSet or something like PlainJwt which implements JwtClaimsAccessor. Even if application wants to add custom headers, the Consumer<Jwt.Builder> jwtCustomizerconfigured via builder or setter would be sufficient as headers are dependent on JWS algorithm and the key used and not on the input in payload.

Also, instead of generic Map<String, Object> jwtClaimsSet, I believe using JwtClaimsAccessor as input to encode() will be beneficial on both client and authorization server side as many of the key JWT claims can be accessible in Java types from JwtClaimsAccessor implementation. This implementation could also possibly have headers so that headers could be provided in input if needed.

[jgrandja]

I've been re-thinking the JwtEncoder API:

String encode(Consumer<Jwt.Builder> jwtBuilderConsumer) throws JwtException

Although this may work, it doesn't feel right that the JwtEncoder receive a Jwt.Builder. The way I see it is that the JwtEncoder implementation is responsible for creating a JWS / JWE and therefore it will create a Jwt.Builder internally before building the JWS / JWE.

@krajcsovszkig-ms

Alternatively instead of a JwtClaimsSet we could create an PlainJwt type which has the claims and the headers.

I think this makes a lot of sense. And @anoopgarlapati is in agreement as well

encode() should accept Map<String, Object> jwtClaimsSet or something like PlainJwt which implements JwtClaimsAccessor

The JWT RFC defines the term Unsecured JWTs so I think it makes sense to provide a new class called UnsecuredJwt, which would look like this:

public class UnsecuredJwt {
	private JoseHeaders headers;
	private JwtClaimsSet claimsSet;

}

public class JoseHeaders {

}

public class JwtClaimsSet implements JwtClaimAccessor {

}

And the JwtEncoder would look like this:

public interface JwtEncoder {

	Jwt encode(UnsecuredJwt unsecuredJwt);

}

And we would provide a way for applications to customize the claims and/or headers via:

public final class NimbusJwtEncoder implements JwtEncoder {

	public final void setJwtCustomizer(Consumer<Jwt.Builder> jwtCustomizer) {
	}

Thoughts?

@krajcsovszkig-ms @anoopgarlapati Given that both of you are working on this on different ends, I want to make sure we are not duplicating efforts. So before implementation begins let's figure out how to get this done without duplicating code. We can figure this out after we align on API design.

[krajcsovszkig-ms]

@jgrandja sounds good.

[jzheaux]

Although this may work, it doesn't feel right that the JwtEncoder receive a Jwt.Builder.

Just to be clear, this isn't what's being proposed, but instead a Consumer<Jwt.Builder>. If I've misunderstood this point, could you elaborate? I think the distinction is important because of your next point:

The way I see it is that the JwtEncoder implementation is responsible for creating a JWS / JWE and therefore it will create a Jwt.Builder internally before building the JWS / JWE.

This is actually the case already with encode(Consumer<Jwt.Builder>), the encoder will create the Jwt.Builder, set the values it wants to, and then the Consumer will further customize it.

public final void setJwtCustomizer(Consumer<Jwt.Builder> jwtCustomizer) {
}

The concern I have with setJwtCustomizer is that this moves customization to the bean configuration level. If I wanted to use more than the provided Jwt.Builder to make a customization decision, then I'd need to construct multiple NimbusJwtEncoder beans. I might not want to customize the Jwt in the same way across all requests, e.g. I might need context from the HttpServletRequest in order to know what customizations to make. It's also a bit of a bummer that my claims and headers are being specified in two separate places (in the customizer and when constructing the UnsecuredJwt).

(Note that this has recently come up a great deal with the SAML support - and while OAuth != SAML, that's just what's making me think of this.)

Regarding some of the ideas around UnsecuredJwt -> Jwt, I'm concerned about Jwt having to become quite a bit more complex in order to express everything that can be in a JWT. For example, a Jwt is not a JWS, and so it would feel weird to add a signature to it, and even weirder to add encryption information to it (since it's not a JWE either). I believe String gives the API the least complexity going forward - down the road, a NimbusJwtEncoder could potentially be composed of a NimbusJwtEncrypter that returns a Jwe and a NimbusJwtSigner that returns a Jws.

For those reasons, I'd still recommend returning a String since it hides that complexity. Is there a use case for needing the decoded value after it's been serialized by NimbusJwtEncoder?

[krajcsovszkig-ms]

@jzheaux @jgrandja

If we return a String, we don't need to create a Jwt or a Jwt.Builder.

What's wrong with providing an UnsecuredJwt with the correct contents instead of a Consumer to update whatever contents the encoder generates?

Also, why would something called an "encoder" even generate stuff to encode? Where would an implementation get the data to use for generating the JWT contents, if not the encode method? That'd still require something like a ClientRegistration field in the implementation, which would again reduce its instances' reusability significantly.

If the user needs a Consumer to preprocess their claims and headers before signing them, couldn't they apply one to the UnsecuredJwt before passing it to encode?

One other problem I have with the Consumer<Jwt.Builder> approach is that the Jwt.Bulder needs a token value to construct, which we won't have until signing the final headers and claims, which the builder is supposed to produce in this case. (Of course we can pass null, but that feels a bit weird to me.)

[jzheaux]

What's wrong with providing an UnsecuredJwt with the correct contents instead of a Consumer to update whatever contents the encoder generates?

If the JwtEncoder wants to provide reasonable defaults, then there's no way for the application to override them. Are you suggesting that NimbusJwtEncoder would not be allowed to set any headers or any claims?

While I agree that there may be value that design constraint, I see a JwtEncoder that sets my jti, iat, exp, nbf, iss, kid, and alg claims for me to be convenient. Then, I could do:

String encoded = this.jwtEncoder.encode(jwt -> jwt.subject("subject"));

And not have to worry about the rest.

Or, in the circumstance where I'd like to have a globally configured strategy that JwtEncoder uses for computing those claims, now I can also override it incidentally with:

Instant myOverride = ...;
String encoded = this.jwtEncoder.encode(jwt -> jwt
        .subject("subject")
        .expiresAt(myOverride));

Also, why would something called an "encoder" even generate stuff to encode?

Fair question. For me, I prefer to leave that to the implementer.

Stating via the contract that an implementation MAY add headers and claims doesn't prevent implementers from writing a JwtEncoder that adds no headers or claims. Doing Consumer<Jwt.Builder> gives the implementer that flexibility while UnsecuredJwt does not.

Where would an implementation get the data to use for generating the JWT contents, if not the encode method?

Good point. I wonder if @jgrandja's idea regarding JwtEncoderFactory would come into play here. I believe this is how Spring Security OAuth 2.0 Client uses a ClientRegistration to get client-contextual JwtDecoder instances.

[jzheaux]

As a side note, I wonder if we should consider merging this into Spring Authorization Server instead for now so that folks can start playing with it. That would probably be helpful feedback in evaluating different contracts. Trying to merge this into Spring Security before Spring Authorization Server can use it I think really raises the bar for consensus.

[krajcsovszkig-ms]

If the JwtEncoder wants to provide reasonable defaults, then there's no way for the application to override them. Are you suggesting that NimbusJwtEncoder would not be allowed to set any headers or any claims?

I'm suggesting that ClientRegistration should contain the information to derive default claims from and any overrides for them. Where we need a signed JWT, we call a utility to create the claims and headers from the ClientRegistration (it could even be in ClientRegistration or in a new inner class in it), then pass those to the encoder for signing. If the user doesn't find this flexible enough, we could add an optional Consumer to ClientRegistration to go over the generated claims and headers before passing them to the encoder. To me these conceptually belong there more than in the encoder.

However I'm completely unfamiliar with the plans for the authorization server, so I don't know if something there would make this approach awkward.

[jgrandja]

@jzheaux

This is actually the case already with encode(Consumer<Jwt.Builder>), the encoder will create the Jwt.Builder, set the values it wants to, and then the Consumer will further customize it.

I see my original comment wasn't clear. Yes agreed that the JwtEncoder will create the Jwt.Builder and pass it to the customizer before build().

Let me restate my main points:

• encode() receives a Consumer which doesn't provide easy access for the JwtEncoder to analyze the input argument. Yes, the JwtEncoder could apply the Consumer and then inspect the claims afterwards but this is not the intention since the customizer is meant to be called after the default claims have been set by the JwtEncoder.
• When I look at this API - String encode(Consumer<Jwt.Builder> jwtBuilderConsumer) - I see a FunctionalInterface that accepts one arg FunctionalInterface. This somehow doesn't feel right. In most cases I've seen, the input arguments are provided to the method so that it can evaluate the values and do its thing. The Consumer<Jwt.Builder> doesn't provide this easy access, however, UnsecuredJwt does.
• I don't feel that the JwtEncoder will be able to set much of the default claims. Most of the claims will come from the client (ClientRegistration, RegisteredClient) and resource owner (Authentication) constructs. The JwtEncoder cannot have direct dependencies to these constructs. Therefore, the caller (client or authorization server) will set the default claims and headers and pass it via UnsecuredJwt.
• Returning String instead of Jwt is not ideal. The caller may want to inspect the Jwt claims/headers and the only way of doing it is to parse the raw String into a Jwt.

Regarding some of the ideas around UnsecuredJwt -> Jwt, I'm concerned about Jwt having to become quite a bit more complex in order to express everything that can be in a JWT.

The original design intent of Jwt was that it can be represented as a JWS or JWE, as stated in javadoc. The tokenValue is either a Jws or Jwe or it can simply be a "plain" Jwt although not recommended. We could potentially add Jwt.isJws() and Jwt.isJwe(), which would be fairly simple to implement as we could deduce this from the headers. I don't think creating new classes Jwe and Jws is needed at this point.

[jgrandja]

@krajcsovszkig-ms We're going to hold off with implementing this on the client side for now.

@jzheaux makes a great point as far as driving out the design and implementation on the authorization server side first as it will be more involved on that end. After we're comfortable with the design and implementation, we could proceed on the client side.

So for now, let's hold off on this PR. Please track the development in spring-authorization-server#81 and see if what we are designing there will suit client as well. Thank you so much for all your feedback.

[krajcsovszkig-ms]

@jgrandja

How long do you think that's going to take? Unfortunately we can't really afford to wait too long with this feature.

Do you see any reason why a Jwt encode(UnsecuredJwt) interface wouldn't work for the AS? I could start implementing that and make changes later if necessary. It might be easier for the developers working on the AS to reason about the interface if they can see and try an example for one of the options as well. We can leave this unmerged or on a separate branch until a definite decision is made.

[jgrandja]

@krajcsovszkig-ms

How long do you think that's going to take?

@anoopgarlapati Is working on this task now. I'm thinking we'll have the initial implementation merged in a couple of weeks.

Unfortunately we can't really afford to wait too long with this feature.

Understood. At the same time, we cannot afford to not get this feature right the first time around. We need to ensure the API is flexible for any use case. Design takes time and we need to ensure we give it the time. The one thing I will say is that we are still targeting to get this into 5.4.

The plan now is to flush out the design and implementation on AS side. After we feel comfortable with the implementation on AS, we will merge it into Spring Security. When it's merged into Spring Security, then we can continue with the JWT client auth. If you want, you could use the JwtEncoder code (straight copy) from AS and test it out with the client auth work in parallel to ensure what we are doing in AS will in fact work in client.

[jgrandja]

@krajcsovszkig-ms I'm going to close this PR given the work is being done in AS side. After we complete the work there and merge into Spring Security then you can submit a new PR based on the new JwtEncoder.

[krajcsovszkig-ms]

OK.

@anoopgarlapati @jgrandja please let me know if there's any way I can help, and keep us updated on how the design evolves.

[jgrandja]

@krajcsovszkig-ms For sure we'll keep you in the loop.