Support symmetric key for JwtDecoder #6495
Conversation
...h2/oauth2-jose/src/main/java/org/springframework/security/oauth2/jose/jws/JwsAlgorithms.java
Outdated
Show resolved
Hide resolved
...h2/oauth2-jose/src/main/java/org/springframework/security/oauth2/jose/jws/JwsAlgorithms.java
Outdated
Show resolved
Hide resolved
...ose/src/test/java/org/springframework/security/oauth2/jwt/NimbusReactiveJwtDecoderTests.java
Outdated
Show resolved
Hide resolved
...gframework/security/oauth2/client/oidc/authentication/ReactiveOidcIdTokenDecoderFactory.java
Outdated
Show resolved
Hide resolved
oauth2/oauth2-jose/src/main/java/org/springframework/security/oauth2/jwt/JwtProcessors.java
Outdated
Show resolved
Hide resolved
...th2-jose/src/main/java/org/springframework/security/oauth2/jwt/NimbusReactiveJwtDecoder.java
Outdated
Show resolved
Hide resolved
| } | ||
|
|
||
| public static SecretKey secretKey() throws NoSuchAlgorithmException { | ||
| KeyGenerator keyGenerator = KeyGenerator.getInstance("AES"); |
There was a problem hiding this comment.
Is there any reason to have this be generated? It would be nice if it were a static value, e.g.
| KeyGenerator keyGenerator = KeyGenerator.getInstance("AES"); | |
| return new SecretKeySpec(Base64.getDecoder().decode("Ky8xmu1fg/OqVlUr9PRKhutauHvuj0rLHExRk+5XkSU="), "AES") |
There was a problem hiding this comment.
There is no particular reason. I feel secretKey() can be useful as some tests may require their own secret. Is there a benefit to having a static value other than the one-time optimization at class load?
There was a problem hiding this comment.
Having a "pre-generated" one is faster. To be clear, I think secretKey(), should stay, it should just use load a pre-generated key instead of a live-generated key.
Also, taking a look at a few other tests, it is fairly common to use a pre-generated value, for example, NimbusReactiveJwtDecoderTests#decodeWhenRSAPublicKeyThenSuccess, and the resource server tests.
There was a problem hiding this comment.
Having a "pre-generated" one is faster
True, but the generated key is done once at class load so I don't think this will make the tests run slower.
There was a problem hiding this comment.
I'd prefer to have this be a static value too. This is not only faster, but it ensures the tests are consistent and predictable.
There was a problem hiding this comment.
Looking through the tests, I don't see this method being used. Let's remove it, adding it back in when there is a clear need for a test to have a dynamically-generated secret key.
|
@rwinch I've implemented dedicated types for the JWS algorithms. package org.springframework.security.oauth2.jose.jws;
public enum MacAlgorithm implements JwaAlgorithm {
...
public enum SignatureAlgorithm implements JwaAlgorithm {
...I've also considered this design for when we implement support for JWE. package org.springframework.security.oauth2.jose.jwe;
public enum SecreKeyEncryptionAlgorithm implements JwaAlgorithm {
...
public enum PublicKeyEncryptionAlgorithm implements JwaAlgorithm {
... |
| * @param jwsAlgorithm the JWS algorithm to use | ||
| * @return a {@link SecretKeyJwtProcessorBuilder} for further configurations | ||
| */ | ||
| public SecretKeyJwtProcessorBuilder jwsAlgorithm(MacAlgorithm jwsAlgorithm) { |
There was a problem hiding this comment.
Does it make sense to rename this method and argument now that it is of type MacAlgorithm?
| } | ||
|
|
||
| public static SecretKey secretKey() throws NoSuchAlgorithmException { | ||
| KeyGenerator keyGenerator = KeyGenerator.getInstance("AES"); |
There was a problem hiding this comment.
I'd prefer to have this be a static value too. This is not only faster, but it ensures the tests are consistent and predictable.
| * @param secretKey the {@code SecretKey} | ||
| * @param jwsAlgorithm the {@link MacAlgorithm JWS algorithm} | ||
| */ | ||
| public NimbusReactiveJwtDecoder(SecretKey secretKey, MacAlgorithm jwsAlgorithm) { |
There was a problem hiding this comment.
Should we rename jwsAlgorithm to macAlgorithm?
jgrandja
force-pushed
the
gh-5465-symmetric-decoder
branch
from
0b66889 to
92dcb52
Compare
|
@rwinch @jzheaux There were quite a few changes to This resulted in quite a few conflicts and was very difficult to merge. So I had no choice but to start from master and apply my updates manually and force push. I know timing is tight to get this into M2 but it would be ideal as this one has been around for quite some time. |
oauth2/oauth2-jose/src/main/java/org/springframework/security/oauth2/jwt/JwtProcessors.java
Outdated
Show resolved
Hide resolved
| } | ||
|
|
||
| public static SecretKey secretKey() throws NoSuchAlgorithmException { | ||
| KeyGenerator keyGenerator = KeyGenerator.getInstance("AES"); |
There was a problem hiding this comment.
Looking through the tests, I don't see this method being used. Let's remove it, adding it back in when there is a clear need for a test to have a dynamically-generated secret key.
|
Merged via bed3371 |
rwinch
added
type: enhancement
Resolves #5465