On Fri, 2 Aug 2024 19:19:54 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). >> >> Work was begun in [another PR](https://github.com/openjdk/jdk/pull/18924). > > Kevin Driver has updated the pull request with a new target base due to a > merge or a rebase. The incremental webrev excludes the unrelated changes > brought in by the merge/rebase. The pull request contains 16 additional > commits since the last revision: > > - update test to include Spi updates > - Update with latest from master > > Merge remote-tracking branch 'origin/master' into kdf-jep-wip > # Please enter a commit message to explain why this merge is necessary, > # especially if it merges an updated upstream into a topic branch. > # > # Lines starting with '#' will be ignored, and an empty message aborts > # the commit. > - add engineGetKDFParameters to the KDFSpi > - code review comment fix for javadoc specification > - change course on null return values from derive methods > - code review comments > - threading refactor + code review comments > - review comments > - review comments > - update code snippet type in KDF > - ... and 6 more: https://git.openjdk.org/jdk/compare/2ded4949...dd2ee48f src/java.base/share/classes/com/sun/crypto/provider/HkdfKeyDerivation.java line 74: > 72: if (kdfParameters != null) { > 73: throw new InvalidAlgorithmParameterException( > 74: "RFC 5869 has no parameters for its KDF algorithms"); I think the exception should just say something like: `hmacAlgName + " does not support parameters"`. The algorithm name isn't necessary here if it is displayed somewhere along the exception stack. I don't think putting an RFC number is helpful. src/java.base/share/classes/com/sun/crypto/provider/HkdfKeyDerivation.java line 144: > 142: salt = consolidateKeyMaterial(salts); > 143: } catch (InvalidKeyException ike) { > 144: throw (InvalidAlgorithmParameterException) new > InvalidAlgorithmParameterException( Why are you using `initCause()` here when there is a [constructor](https://docs.oracle.com/en/java/javase/21/docs/api/java.base/java/security/InvalidAlgorithmParameterException.html#%3Cinit%3E(java.lang.String,java.lang.Throwable)) is available and the following catch uses the `NSAE` version of the constructor? This isn't the only usage of `initCause()` src/java.base/share/classes/com/sun/crypto/provider/HkdfKeyDerivation.java line 245: > 243: } > 244: > 245: private static boolean isNullOrEmpty(Collection<?> c) { This appears to not be used. src/java.base/share/classes/com/sun/crypto/provider/HkdfKeyDerivation.java line 258: > 256: byte[] workItemBytes = CipherCore.getKeyBytes(checkIt); > 257: return new SecretKeySpec(workItemBytes, "Generic"); > 258: } else { I think this is less error prone and easier to read than what you have below: ByteArrayOutputStream os = new ByteArrayOutputStream(); for (SecretKey workItem : localKeys) { try { os.write(CipherCore.getKeyBytes(workItem)); } catch (IOException e) { // won't happen } } return new SecretKeySpec(os.toByteArray(), "Generic"); And if your concerned about the extra copy from `toByteArray()`, you could consider using an internal class `AEADBufferedStream` which extends ByteArrayOutputStream but will return the internal copy to avoid extra mem allocation. src/java.base/share/classes/javax/crypto/KDF.java line 121: > 119: private Iterator<Service> serviceIterator; > 120: > 121: private final Object lock; Why are you using an `Object` as a lock instead of something like `ReentrantLock`? I realize older implementations used this style, but does this need to be mimicked? src/java.base/share/classes/javax/crypto/KDF.java line 199: > 197: > 198: /** > 199: * Returns a {@code KDF} object that implements the specified > algorithm. This could be considered a CSR comment as well. You may not want to say that this object "implements the specified algorithm". Given the provider implements the algorithm and the provider is delayed initialization. It maybe better to say this "Returns a KDF instance initialized with the specified algorithm." The same applies for other `getInstance()` methods below. src/java.base/share/classes/javax/crypto/KDF.java line 222: > 220: } catch (InvalidAlgorithmParameterException e) { > 221: throw new NoSuchAlgorithmException( > 222: "Received an InvalidAlgorithmParameterException. Does > this " I think it would be better than to say "KDFParameters must be specified." You already attached the initial exception and there isn't much mystery as the cause given this method passes "null" src/java.base/share/classes/javax/crypto/KDF.java line 291: > 289: } catch (InvalidAlgorithmParameterException e) { > 290: throw new NoSuchAlgorithmException( > 291: "Received an InvalidAlgorithmParameterException. Does > this " same as mentioned above src/java.base/share/classes/javax/crypto/KDF.java line 441: > 439: } > 440: > 441: private static KDF handleException(NoSuchAlgorithmException e) My comment originates more with the callers of this method. While I appreciate that you are trying to throw correct exception for the situation, you may have noticed that if the developer calls a `getInstance()` which only throws `NSAE` (line 216 for example), you could be in a situation where you unwrap the causing `IAPE` from the wrapping `NSAE`, to then rewrap it in a `NSAE` on line 219. I may be just better to let the provider throw what they want and not try to modify it. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1710548188 PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1714462088 PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1714473347 PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1714494293 PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1710564628 PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1714442021 PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1714354680 PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1714356731 PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1714385454
