On Tue, 14 May 2024 22:14:47 GMT, Kevin Driver <kdri...@openjdk.org> wrote:
>> Introduce an API for Key Derivation Functions (KDFs), which are >> cryptographic algorithms for deriving additional keys from a secret key and >> other data. See [JEP 478](https://openjdk.org/jeps/478). > > Kevin Driver has updated the pull request incrementally with one additional > commit since the last revision: > > re-enable Delayed Provider test Some more comments on `HkdfKeyDerivation.java`. src/java.base/share/classes/com/sun/crypto/provider/HkdfKeyDerivation.java line 97: > 95: throw new InvalidParameterSpecException( > 96: "the algorithm for the resultant SecretKey may not be > null or" > 97: + " empty"); I think this should be an `IllegalArgumentException`. src/java.base/share/classes/com/sun/crypto/provider/HkdfKeyDerivation.java line 136: > 134: // we should be able to combine these Lists of keys into > single > 135: // SecretKey Objects, > 136: // unless we were passed something bogus or an unexportable > P11 key There is a bug here. `consolidateKeyMaterial()` returns the key if it's the only one in `ikms`, without checking if it's "bogus". Then, it is passed into `hkdfExtract` and very unfortunately `hmacObj.doFinal(null)` doesn't make any noise. Same in ExtractThenExpand. src/java.base/share/classes/com/sun/crypto/provider/HkdfKeyDerivation.java line 156: > 154: // This is bubbling up from the getInstance of the > Mac/Hmac. > 155: // Since we're defining these values internally, it > should be > 156: // safe to "eat" this NSAE. This could happen if someone filters out the required algorithms. In this case, we usually throw a `ProviderException`. There are similar cases below. src/java.base/share/classes/com/sun/crypto/provider/HkdfKeyDerivation.java line 364: > 362: } > 363: > 364: return kdfOutput; Since `deriveKey` is now calling `deriveData`, I suggest checking if `outLen` is the same as the length `kdfOutput` here and call `copyOf` if not. There is no need to call it in `deriveData`. src/java.base/share/classes/com/sun/crypto/provider/HkdfKeyDerivation.java line 370: > 368: public HkdfSHA256(AlgorithmParameterSpec algParameterSpec) > 369: throws InvalidAlgorithmParameterException, > 370: NoSuchAlgorithmException { You should not throw `NoSuchAlgorithmException` here. This constructor will be directly called by JCA framework and must conform to the spec described in `KDFSpi` that it can only throw `InvalidAlgorithmParameterException`. ------------- PR Review: https://git.openjdk.org/jdk/pull/18924#pullrequestreview-2056627780 PR Review Comment: https://git.openjdk.org/jdk/pull/18924#discussion_r1600771294 PR Review Comment: https://git.openjdk.org/jdk/pull/18924#discussion_r1600771763 PR Review Comment: https://git.openjdk.org/jdk/pull/18924#discussion_r1600772561 PR Review Comment: https://git.openjdk.org/jdk/pull/18924#discussion_r1600774835 PR Review Comment: https://git.openjdk.org/jdk/pull/18924#discussion_r1600775715
