-
Notifications
You must be signed in to change notification settings - Fork 6k
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.
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
Introduce JwtEncoder #9208
Introduce JwtEncoder #9208
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, @jgrandja! I've left some feedback inline.
crypto/src/main/java/org/springframework/security/crypto/key/CryptoKey.java
Outdated
Show resolved
Hide resolved
crypto/src/main/java/org/springframework/security/crypto/key/CryptoKey.java
Outdated
Show resolved
Hide resolved
...oauth2-jose/src/main/java/org/springframework/security/oauth2/jose/jws/NimbusJwsEncoder.java
Outdated
Show resolved
Hide resolved
oauth2/oauth2-jose/src/main/java/org/springframework/security/oauth2/jwt/JwtClaimsSet.java
Outdated
Show resolved
Hide resolved
crypto/src/main/java/org/springframework/security/crypto/key/SymmetricKey.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've left comments inline
crypto/src/main/java/org/springframework/security/crypto/key/AsymmetricKey.java
Outdated
Show resolved
Hide resolved
crypto/src/main/java/org/springframework/security/crypto/key/CryptoKey.java
Outdated
Show resolved
Hide resolved
crypto/src/main/java/org/springframework/security/crypto/key/CryptoKey.java
Outdated
Show resolved
Hide resolved
crypto/src/test/java/org/springframework/security/crypto/key/KeyGeneratorUtils.java
Outdated
Show resolved
Hide resolved
crypto/src/test/java/org/springframework/security/crypto/key/TestCryptoKeys.java
Outdated
Show resolved
Hide resolved
...oauth2-jose/src/main/java/org/springframework/security/oauth2/jose/jws/NimbusJwsEncoder.java
Outdated
Show resolved
Hide resolved
oauth2/oauth2-jose/src/test/java/org/springframework/security/oauth2/jwt/TestJwtClaimsSets.java
Outdated
Show resolved
Hide resolved
oauth2/oauth2-jose/src/test/java/org/springframework/security/oauth2/jose/TestJoseHeaders.java
Outdated
Show resolved
Hide resolved
crypto/src/main/java/org/springframework/security/crypto/key/CryptoKeySource.java
Outdated
Show resolved
Hide resolved
oauth2/oauth2-jose/src/main/java/org/springframework/security/oauth2/jwt/JwtEncoder.java
Outdated
Show resolved
Hide resolved
Thanks for the feedback @jzheaux @rwinch. I decided to remove I introduced The |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, @jgrandja! I've left some feedback inline.
"Found multiple signing keys for algorithm '" + jwsAlgorithm.getName() + "'")); | ||
} | ||
|
||
return new NimbusJwsEncoder(jwks.get(0)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How will this setup support rotation? I would have expected NimbusJwsEncoder
to select its own JWK
based on the JoseHeader
given when encode
is invoked.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch! I'll update to support a key selection strategy.
oauth2/oauth2-jose/src/main/java/org/springframework/security/oauth2/jwt/NimbusJwsEncoder.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the updates, @jgrandja. I've left some more feedback inline.
crypto/src/main/java/org/springframework/security/crypto/key/KeySource.java
Outdated
Show resolved
Hide resolved
* @param jwtCustomizer the {@link Jwt} customizer to be provided the | ||
* {@link JoseHeader.Builder} and {@link JwtClaimsSet.Builder} | ||
*/ | ||
public void setJwtCustomizer(BiConsumer<JoseHeader.Builder, JwtClaimsSet.Builder> jwtCustomizer) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This has a strange feeling to it. If I call encode
with already built headers and claims, where did these builders come from? It leaks the fact that the implementation is doing an internal copy of the headers and claims.
IIRC, this was anticipating some future use cases, but I don't remember anymore. If there isn't a concrete use case for it, it may be best to leave it out for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It leaks the fact that the implementation is doing an internal copy of the headers and claims
At a minimum, the JwtEncoder
needs to enhance the JoseHeader
with some metadata of the selected key, for example kid
. Therefore, it needs to make a copy of it so it can add the specific JOSE header(s).
Furthermore, this customizer allows an application to add custom claims and/or headers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* @return a {@link Jwt} | ||
* @throws JwtEncodingException if an error occurs while attempting to encode the JWT | ||
*/ | ||
Jwt encode(JoseHeader headers, JwtClaimsSet claims) throws JwtEncodingException; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I'd prefer this to be
Jwt encode(Jwt.Builder jwtBuilder) throws JwtEncodingException;
which the average caller would call with:
encoder.encode(Jwt.withAlgorithm(JwsAlgorithm.RS256).subject("subject"))
Some nice things about this:
- Fewer classes to maintain - it reduces this PR to only
JoseHeaderNames
,JwtEncoder
,NimbusJwsEncoder
, and a small enhancement toJwt
- It's nice to only have one object to prepare from the calling side. This simplifies invocation as well as any post-processing
In early discussions about the contract, one concern that was raised with this proposal was reading the claims and headers from the builder, though now I've verified that the implementation can use the claims(Consumer)
and headers(Consumer)
methods to achieve this.
I think that there are some adjustments to the Nimbus API that would simplify this approach further, which also happens to align with how other JWT libraries mint tokens.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the idea of reducing the code, but not at the expense of an input that isn't completely filled out. If it has optional values they should be clearly marked that way and be optional in all context that object is used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm in favour of keeping the operation as-is or consider changing it to:
Jwt encode(JoseHeader.Builder headersBuilder, JwtClaimsSet.Builder claimsBuilder)
Introducing the JoseHeader and JwtClaimsSet constructs aligns with the spec.
The Jwt.Builder
currently supports setting the typed and custom claims but the headers only generically via header()
or headers()
. Adding all the possible JWS and JWE headers to Builder
is going to make the Builder
quite large and not as easy to understand without the clear separation of claims and headers, which the new constructs would achieve.
I would even prefer to re-work Jwt
, specifically the Builder
and associated factory method withTokenValue(String tokenValue)
. For example, some of the proposed changes would be:
`JoseHeader Jwt.getHeaders()`
`JwtClaimsSet Jwt.getClaims()`
static Jwt.with(JoseHeader, JwtClaimsSet)
I know what I'm proposing as far as the re-work for Jwt
is a big change but I strongly feel we should have separate representations of the JOSE headers and Jwt Claims set. I believe this will keep things simpler (in each class), easier to maintain and easier to grow when we introduce support for JWE at a later point (which adds new headers to the mix).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do think separate constructs are a good idea. The problem with passing Jwt.Builder
is the user doesn't know what fields are required. If Jwt.Builder
is provided, how does the user know (without reading docs) that tokenValue
will be provided by the JwtEncoder
?
I also don't like that if we used the builder as an input we are augmenting the input and returning a value. We presumably use immutability to help with thread safety, but passing around a mutable object (that is indeed mutated by the APIs) we lose that value.
I would even prefer to re-work Jwt, specifically the Builder and associated factory method withTokenValue(String tokenValue). For example, some of the proposed changes would be:
I'm not sure I understand the proposal as I don't think this would compile. What is the return type of Jwt.with(JOseHeader, JwtClaimsSet)
? Where are these methods being added? Can these changes be in a separate PR or are they necessary for JwtEncocer
? I'd prefer we focus on exactly what is needed and add another ticket if we think there are separate items we want to consider.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there is value in separate header and claim constructs as well. It also seems reasonable for Jwt
to use these constructs in a future PR.
Additionally, thank you both for the feedback about builder inputs. While I was originally attracted to the idea of sending in builders as method parameters, I agree that it amounts to mutable inputs, which is smelly.
I'd like to see if we can find an encode
contract where it's easier to supply headers optionally. I'd also like to see if there is a change that can made made to the contract such that there can be one interface and Nimbus implementation instead of multiple once JWE support is introduced.
Most of the time, callers don't care about what the headers are and this is also considered a more complex part of the spec, especially once JWE is introduced. It seems quite reasonable to me that a security framework would make these security decisions for me. This PR already looks up a number of headers via the kid
, which is very helpful, and even more helpful would be not needing to supply any headers at all.
Simple things like inspecting the JWKSource
or having a default algorithm configured in NimbusJwsEncoder
would allow for the implementation to draw very reasonable inferences about the alg
header, allowing the caller to simply give the claims. These assumptions on the part of the framework could certainly be overridden if the caller does specify headers.
I believe the PR's contract can already satisfy my request if an implementation allows null
to be passed into the headers
parameter (for example, encoder.encode(null, claims)
), though in my opinion it would be a nicer experience to not have to specify anything.
One way to do this would be a parameters object with some named constructors:
JwtEncoderParameters parameters = JwtEncoderParameters.sign(claims);
Jwt signedJwt = encoder.encode(parameters);
where JwtEncoderParameters
is a new class that would be introduced in this PR.
The caller could, of course, also supply headers if so desired:
Jwt signedJwt = encoder.encode(JwtEncoderParameters.sign(header, claims));
A combined parameters object would still maintain headers and claims as separate constructs and would reduce boilerplate in many cases. For example, instead of doing:
JoseHeader header = JoseHeader.withAlgorithm(SignatureAlgorithm.RS256).build();
JwtClaimsSet claims = ...
return encoder.encode(header, claims);
You could do:
JwtClaimsSet claims = ...
JwtEncoderParameters parameters = JwtEncoderParameters.sign(claims);
return encoder.encode(parameters);
While not much of a line-saver, reducing this kind of boilerplate is nice since I believe that it reduces the likelihood of security misconfigurations.
A related concern that I have about the PR's contract is that it requires the caller to either:
- know which encoder implementation is being used to know what will happen to their claims (signed/encrypted/signed-and-encrypted) or
- know enough about JWS/JWE headers to understand how to induce a particular behavior
The nice thing about a parameters object is that it's also then possible to introduce something like the following:
Jwt signedAndEncryptedJwt = encoder.encode(JwtEncoderParameters.signThenEncrypt(claims));
without changing the contract. As I understand the current proposal (please expand NimbusJweEncoderTests
when following the link), this particular use case would be several lines of code and would require additional interfaces.
My point is not to outline exactly what should be done, and there may be other ways to achieve this. I prefer a parameters object because it allows for a single interface and Nimbus implementation that can today do JWS and in the future also do JWE and JWS+JWE. It's also nice that the caller can request these modes without needing to specify headers.
On the other hand, if it's important that headers always be specified or if requiring headers is not a concern, then I'd alternatively recommend that the interface be called JwsEncoder
to clarify to the caller that the resulting Jwt
is a JWS, or potentially renaming JoseHeader
to JwsHeader
and parameterizing JwtEncoder
(e.g. JwtEncoder<JwsHeader>
).
Or, if I'm totally off base here, feel free to ignore. :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the Nested JWT use case, the returned object cannot be a
Jwt
.
I don't really see why this is the case, could you elaborate? Couldn't Jwt
contain three members, like so:
JwtClaimsSet getClaimsSet();
JwsHeader getJwsHeader();
JweHeader getJweHeader();
Flattening a structure into its relevant parts like this is a practical way for an application to consume it. It's uncommon for folks to need to know the headers anyway -- most callers only need to know the resulting encoded value and its claims.
I imagine that a change like this will have to happen anyway to Jwt
so that Jwt NimbusJwtDecoder#decode(String)
can decrypt and verify an encrypted and signed JWT.
If Jwt
cannot be made to suit, I wonder if this means that JwtEncoder#encode
's return type needs to be different from Jwt
. I believe returning a String
can address this concern.
Practically speaking, returning a serialized value is less of a risk than it may seem on the surface. In all cases, the claims must be specified by the caller, so the caller does not lose access to the claims when a string is returned. I believe it will be uncommon for the caller to need to know the headers that were used -- but if this is needed, an application can easily parse the return value with something like JWTParser.parse
from Nimbus.
The use case we currently have in the codebase, NimbusJwtClientAuthenticationParametersConverter
, only needs the serialized value. A brief Find Usages search in Spring Authorization Server shows that only the serialized value and the claims are needed to fulfill those use cases.
While I strongly prefer one of the above two, if Jwt
cannot be made to suit and if String
feels too risky, I wonder if we could introduce JwtEncoderResult
and return that. It could initially contain only the encoded String
. Additional members could be added if needed.
I'm keen on making this aspect of the contract work. Given that JwtDecoder
has the potential to decode both signed JWTs and Nested JWTs because of its contract, I think it's reasonable to see if JwtEncoder
can encode both signed JWTs and Nested JWTs with the right contract. Similar to applications asking Spring Security to "please decrypt and verify this" without exposing them to Nested JWT internals (Nimbus does exactly this), it seems quite reasonable to give Spring Security a set of claims and say "please sign and encrypt this".
Parenthetically, if we can't do this, I think it will be less confusing if the interface is called JwsEncoder
so that users don't think to align the capabilities of JwtDecoder
with JwtEncoder
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm marking this as resolved because the code now uses JwtEncoderParameters. We can add any additional feedback on the updated code.
oauth2/oauth2-jose/src/main/java/org/springframework/security/oauth2/jwt/NimbusJwsEncoder.java
Outdated
Show resolved
Hide resolved
JWSHeader jwsHeader = new JWSHeader(jwsAlgorithm); | ||
JWKSelector jwkSelector = new JWKSelector(JWKMatcher.forJWSHeader(jwsHeader)); | ||
List<JWK> jwks = jwkSelector.select(this.jwkSetProvider.get()); | ||
if (jwks.size() > 1) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you clarify why having more than one is exceptional? If I'm rotating keys, then there will be a period of time when I've got more than one candidate in order to facilitate zero-downtime for resource servers.
I'm assuming that the JWK source here is the same JWK source that an application would use for their /jwks
authorization server endpoint. Is that a bad assumption?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm assuming that the JWK source here is the same JWK source that an application would use for their
/jwks
authorization server endpoint
No. The JWK Source for the Authorization Server would behave differently than the one with Resource Server.
When a JWT needs to be signed, the JWK Source must return one JWK
for signing. If it returns multiple, then how does it choose which one to use for signing? There is no metadata associated to the JWK indicating the "active" (current) key. NOTE: This was the original thinking behind the design of the ManagedKey
, as it would hold this metadata attribute indicating the active flag. Now that we removed ManagedKey
, the JWK Source must return the "current" key used for signing. But to be clear, the /jwks
endpoint would return all the active keys - current and active (rotated).
The resource server could contain 2 keys that are active but it will narrow the selection process using a unique JOSE header, e.g. kid
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jgrandja I agree with you, there can only be one signing key at a time. Is it possible to not require the caller code to provide the kid or alg in the header ? This could be guess from the signing key source if provided and will avoid the caller code to have access to the jwksource, parse it and extract header value which is already done by the encoder.
I think in most use case, it is the key that drive the kid and alg set in the header. The caller code just want to encode a jwt and don't bother which alg or key will be used. It is often generated outside and rolling. If the algorithm change, the caller code need to change if it has been hard coded like the sample given above encoder.encode(Jwt.withAlgorithm(JwsAlgorithm.RS256).subject("subject"))
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@scrocquesel NOTE: The code in this PR is out-of-date and needs to be synced with the code currently in the package org.springframework.security.oauth2.client.endpoint. The implementation NimbusJwsEncoder
is used for Jwt Client Authentication introduced in 5.5
.
You will notice that all classes are package-private
as we were not ready to expose JwtEncoder
in 5.5
but we needed to add the Jwt Client Authentication feature, hence the reason for package-private
.
If you have a need for the JwtEncoder
API, it would be helpful if you could detail your requirements and we could consider exposing it in 5.6
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, I didn't notice it was old code. Should I express needs in this issue ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. Please add a new comment at bottom of this PR detailing your requirements and the need for the JwtEncoder
API. The more detail the better. Thanks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've left some comments inline.
A general bit of feedback is that I think we need to make some changes to avoid classes that have interface based methods with inputs/outputs that belong to external libraries (i.e. Nimbus). An example of this is NimbusKeySourceJWKSetConverter
which has a return value of JWKSet
(part of Nimbus) on an interface method.
When an interface method accepts/returns a value that belongs to an external library that interface cannot be used by anything except by another API that uses Nimbus. In scenarios where this is absolutely required, we typically nest the class so it is clear it belongs to a specific implementation.
crypto/src/main/java/org/springframework/security/crypto/key/KeySource.java
Outdated
Show resolved
Hide resolved
* @see KeySource | ||
* @see com.nimbusds.jose.jwk.JWKSet | ||
*/ | ||
public final class NimbusKeySourceJWKSetConverter implements Converter<KeySource, JWKSet> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What contexts would this be used? Is there a reason we are including this here vs in authorization server?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a bit surprising that this converts to a Nimbus type vs Spring Security.Since it converts to a Nimbus type, I'd prefer it be kept where we use it (perhaps even private) vs making it so widely available.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NimbusKeySourceJWKSetConverter
will be removed, see comment
oauth2/oauth2-jose/src/main/java/org/springframework/security/oauth2/jwt/JoseHeader.java
Outdated
Show resolved
Hide resolved
...e/src/main/java/org/springframework/security/oauth2/jose/NimbusKeySourceJWKSetConverter.java
Outdated
Show resolved
Hide resolved
oauth2/oauth2-jose/src/main/java/org/springframework/security/oauth2/jwt/JoseHeader.java
Outdated
Show resolved
Hide resolved
oauth2/oauth2-jose/src/main/java/org/springframework/security/oauth2/jwt/JoseHeader.java
Outdated
Show resolved
Hide resolved
oauth2/oauth2-jose/src/main/java/org/springframework/security/oauth2/jwt/JwtClaimsSet.java
Outdated
Show resolved
Hide resolved
oauth2/oauth2-jose/src/main/java/org/springframework/security/oauth2/jwt/JwtEncoder.java
Outdated
Show resolved
Hide resolved
* @return a {@link Jwt} | ||
* @throws JwtEncodingException if an error occurs while attempting to encode the JWT | ||
*/ | ||
Jwt encode(JoseHeader headers, JwtClaimsSet claims) throws JwtEncodingException; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the idea of reducing the code, but not at the expense of an input that isn't completely filled out. If it has optional values they should be clearly marked that way and be optional in all context that object is used.
|
||
// @formatter:off | ||
/* | ||
* IMPORTANT DESIGN DECISION |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jzheaux FYI
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rwinch I'm currently working on some updates that may very well resolve this. Let's keep it for now until I complete updates.
@jzheaux I looked at the Nimbus API Also, I checked the Nimbus API Maybe you're seeing something that I'm not seeing? |
@rwinch @jzheaux I'm having second thoughts on introducing From my understanding in our discussion, the main goal of
We decided that we would default to public final class JoseHeaders {
public static final JoseHeader RS256_ALG = JoseHeader.withAlgorithm(SignatureAlgorithm.RS256).build();
private JoseHeaders() {
}
} With usage:
This amounts to the same lines of code as the proposed solution for It still introduces a new class I'd like to understand what are the other benefits that I really want to keep things simple and not introduce any extra classes that are not needed. IMO, the extra boilerplate code |
My apologies, I don't remember saying that. Part of the confusion might have been my overloading the term "implementation". I meant it in the OOP sense: The implementation ( I imagine this obligation to figure out the algorithm could be handled by the underlying library, though something like that won't affect
Yes, that's my recollection, too. While I'm open to both, I lean towards this defaulting be done by And while a prototype |
I think @jzheaux already highlighted why I prefer an argument that contains the parameters, but to be explicit it provides us with the option of explicitly providing the headers, defaulted by the headers, or by the |
@jzheaux @rwinch Thanks for all the feedback! I'm quite happy with the improvements. This is now ready for a full review. I believe I have addressed all your concerns @jzheaux. NOTE: You will notice a commit "Disable JWE tests" , which simply comments out some code in |
@jzheaux @rwinch I just pushed the updates for @jzheaux Your feedback was very valuable. Thank you! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, @jgrandja, for the updates! I've left some additional feedback inline about the changes.
A
JwtEncoder
encodes the JOSE headers and JWT Claims Set into a JSON Web Token (JWT).The initial implementation
NimbusJwtEncoder
supports encoding a JWS (JSON Web Signature).