JWE - Explicitly check JCA alg name when looking up SignatureAlgorithms #712
Conversation
- Added JWE AeadAlgorithm and KeyAlgorithm and supporting interfaces/implementations, and refactored SignatureAlgorithm to be an interface instead of an enum to enable custom algorithms - NoneSignatureAlgorithm cleanup. Added UnsupportedKeyExceptionTest. - Added JWK support!
…ved the previous SignatureAlgorithm implementation concepts (Provider/Signer/Validator implementations). Implementations are now interface-driven and fully pluggable.
… class tests to impl module to avoid mocking static calls due to bridge/reflection logic.
… eliminating use of RuntimeEnvironment in favor of new Providers implementation. JJWT will no longer modify the system providers list. 2. Changed SecretKeyGenerator.generateKey() to KeyBuilderSupplier.keyBuilder() and a new SecretKeyBuilder interface. This allows users to customize the Provider and SecureRandom used during key generation if desired. 3. Added KeyLengthSupplier to allow certain algorithms the ability to determine a key bit length without forcing a key generation. 4. Ensured Pbes2 algorithms defaulted to OWASP-recommended iteration counts if not specified by the user.
… helper method 2. Updated RequiredTypeConverter exception message to represent the expected type as well as the found type
2. Ensured CompressionException extends from io.jsonwebtoken.io.IOException 3. Deleted old POC unused JwkParser interface 4. Ensured NoneSignatureAlgorithm reflected the correct exception message for `sign`
|
@dogeared I changed the target branch after creating this PR, any idea how to restart the Snyk checks? |
| Map<String, SignatureAlgorithm> lookupMap = new HashMap<>(6); | ||
| for (SignatureAlgorithm alg : algs) { | ||
| lookupMap.put(alg.jcaName.toUpperCase(Locale.ENGLISH), alg); | ||
| lookupMap.put(alg.pkcs12Name.toUpperCase(Locale.ENGLISH), alg); |
There was a problem hiding this comment.
FWIW, the pkcs12 name is dotted numerals - no alphabetic characters, so changing case isn't strictly necessary. ;). But leaving it the way you have it is probably a little more obvious/self-documenting and could theoretically handle any other names.
There was a problem hiding this comment.
Out of curiosity - is this concept of acceptable HMAC names used anywhere else given the io.jsonwebtoken.SignatureAlgorithm --> io.jsonwebtoken.security.SignatureAlgorithm API change in the jwe branch? I'd like to ensure it exists in one place only to ensure we don't duplicate logic that might accidentally become out of sync.
There was a problem hiding this comment.
Everything I found was sill in io.jsonwebtoken.SignatureAlgorithm
But MacSignatureAlgorithm also lists out the three algs too:
https://github.com/jwtk/jjwt/blob/dc87c42/impl/src/main/java/io/jsonwebtoken/impl/security/MacSignatureAlgorithm.java#L28
That might make a better home for these util method, but, it crosses the api/impl module, it probably makes the most sense to keep it in io.jsonwebtoken.SignatureAlgorithm until after we remove the deprecated code (or move the related bits into the impl package)
| @@ -113,6 +116,7 @@ | |||
| //purposefully ordered higher to lower: | |||
| private static final List<SignatureAlgorithm> PREFERRED_HMAC_ALGS = Collections.unmodifiableList(Arrays.asList( | |||
| SignatureAlgorithm.HS512, SignatureAlgorithm.HS384, SignatureAlgorithm.HS256)); | |||
| private static final Map<String, SignatureAlgorithm> PREFERRED_HMAC_ALGS_LOOKUP = preferredHmacAlgorithmMap(); | |||
There was a problem hiding this comment.
Nit: Suggested name of HMAC_ALGS_BY_JCA_ID (and corresponding helper method)? I tend to name lookup maps in this way to self-document the map keys (since 'lookup' tends to be generic').
Also, unless I'm missing something, we can drop the PREFERRED prefix because nothing about the map is used for preferred ordering checks, no?
There was a problem hiding this comment.
Agreed! I renamed that var three times locally trying to figure out a better name 😆
| @@ -576,8 +575,16 @@ public static SignatureAlgorithm forSigningKey(Key key) throws InvalidKeyExcepti | |||
| if (key instanceof SecretKey) { | |||
|
|
|||
| SecretKey secretKey = (SecretKey)key; | |||
| String secretKeyAlg = secretKey.getAlgorithm(); | |||
There was a problem hiding this comment.
Might want an assertion here to ensure that secretKey.getAlgorithm() always returns a non-null/non-empty String value, since without it, the added line 582 below would cause a null-pointer exception.
| int bitLength = io.jsonwebtoken.lang.Arrays.length(secretKey.getEncoded()) * Byte.SIZE; | ||
|
|
||
| // first, check the key alg name | ||
| SignatureAlgorithm algFromJcaName = PREFERRED_HMAC_ALGS_LOOKUP.get(secretKeyAlg.toUpperCase(Locale.ENGLISH)); | ||
| if (algFromJcaName != null && bitLength >= algFromJcaName.minKeyLength) { |
There was a problem hiding this comment.
I'm not quite sure this is correct.
Only if bitLength is 0 and you found a SignatureAlgorithm from the JCA ID lookup, do you want to short circuit and return the discovered SignatureAlgorithm instance.
Otherwise you want to fall back to the PREFERRED_HMAC_ALGS priority-based lookup to retain existing behavior.
If we don't want to do it this way, we will probably have to change the JavaDoc to reflect different semantics, and we'd need to think through the repercussions of different semantics.
That is, if someone calls it today and it returns alg A, and they call it after a new release and it returns alg B, that probably wouldn't be a good thing.
There was a problem hiding this comment.
That is the original issue from #709
Doing something like this currently doesn't work:
byte[] bytes = new byte[48]
new Random().nextBytes(bytes)
// create a secret key with a given algorithm name
SecretKey key = new SecretKeySpec(bytes, SignatureAlgorithm.HS256.getJcaName())
SignatureAlgorithm signatureAlgorithm = SignatureAlgorithm.forSigningKey(key)
assertEquals SignatureAlgorithm.HS256, signatureAlgorithm This currently returns a HS384 (based on only the key size, and ignores the name)
|
Closing as a matter of housekeeping. |
Port #709 to the JWE branch