On Fri, 6 Feb 2026 02:23:08 GMT, Valerie Peng <[email protected]> wrote:
> Initial commit containing the public API changes and related regression tests. > > --------- > - [x] I confirm that I make this contribution in accordance with the [OpenJDK > Interim AI Policy](https://openjdk.org/legal/ai). src/java.base/share/classes/com/sun/crypto/provider/Argon2Impl.java line 64: > 62: MEMORY_MAX = 1 << 21; // 2 GiB? > 63: } else { > 64: MEMORY_MAX = memLimit >>> 12; // 1/4 of maximum jvm memory - Not sure about 2GiB. What if it's a small server/VM/Docker running on 2GiB of total RAM? I think we should either have a configurable security property or use the most conservative value of 64 MiB. - I think we should document better this code block in comments: why we compare to `Long.MAX_VALUE`? Why we shift by 21 and 12? - IMO, the code would be more readable if we rename `MEMORY_MAX` to `MAX_MEM_BLOCKS` and add a comment that 1 block = 1 KiB. - What about the limit for (t * m) value? src/java.base/share/classes/com/sun/crypto/provider/Argon2Impl.java line 202: > 200: this.type = type; > 201: this.lanes = parallelism; > 202: this.segLen = memory / (parallelism * 4); Now when we have `ARGON2_SLICE_NUM` declared above, we can replace `4` with it. src/java.base/share/classes/com/sun/crypto/provider/Argon2Impl.java line 216: > 214: this.passes = passes; > 215: this.b = new Block[this.lanes][this.columns]; > 216: } Nit: wouldn't this look cleaner? private final Type type; private final int lanes; private final int passes; private final int segLen; private final int columns; private final int blockNum; private final Block[][] b; Argon2Instance(Type type, int parallelism, int memory, int passes) { this.type = type; this.lanes = parallelism; this.segLen = memory / (this.lanes * 4); this.columns = segLen * 4; this.blockNum = this.columns * this.lanes; this.passes = passes; this.b = new Block[this.lanes][this.columns]; } src/java.base/share/classes/com/sun/crypto/provider/Argon2Impl.java line 414: > 412: Runtime r = Runtime.getRuntime(); > 413: // memory limit = 1/4 of maximum amount of the jvm memory limit > 414: MEMORY_MAX = r.maxMemory() >>> 12; Did you mean `>>> 2`? Also: - `r.maxMemory()` may return `java.lang.Long#MAX_VALUE` in some environments, we should check for that and use something else in such case. - Since we check for memory and parallelism, should we set some reasonable limit for iterations? src/java.base/share/classes/com/sun/crypto/provider/Argon2Impl.java line 420: > 418: > 419: // constants > 420: public static final int ARGON2_SYNC_POINTS = 4; Nit: rename `ARGON2_SYNC_POINTS` to `ARGON2_SLICE_NUM` and move it to the top of the class together with other `public static final` constants below? src/java.base/share/classes/com/sun/crypto/provider/SunJCE.java line 463: > 461: ps("KDF", "HKDF-SHA512", > 462: "com.sun.crypto.provider.HKDFKeyDerivation$HKDFSHA512"); > 463: ps("KDF", "Argon2id", "com.sun.crypto.provider.Argon2ID"); This should be `Argon2`, right? And then we specify the exact variant with `Argon2ParameterSpec` when calling `deriveData`? src/java.base/share/classes/javax/crypto/spec/Argon2ParameterSpec.java line 53: > 51: * Type of Argon2 algorithms > 52: */ > 53: public enum Type { Should we remove this enum if we only support `Argon2id`? src/java.base/share/classes/javax/crypto/spec/Argon2ParameterSpec.java line 226: > 224: * Set the memory value to the builder. > 225: * > 226: * @param mPower set memory value to 2^{@code mc} Kibibytes What's `mc` here? src/java.base/share/classes/javax/crypto/spec/Argon2ParameterSpec.java line 461: > 459: > 460: /** > 461: * Destroy this object by clearing out the {@code msg} and {@code > secret} s/msg/password/ src/java.base/share/classes/sun/security/util/Argon2Util.java line 58: > 56: case 31 ->{ return String.format(baseFormat, m, t, p, > 57: enc.encodeToString(k), enc.encodeToString(x)); } > 58: default ->{ throw new RuntimeException Should it be `IllegalArgumentException`? test/jdk/com/sun/crypto/provider/Argon2/TestArgon2EncodedHash.java line 56: > 54: > 55: private static TestVector[] TEST_VALUES = { > 56: new > TestVector("$argon2i$v=19$m=16,t=2,p=1$d3VVZ0s3bnBIN25UbXVzQw$h682lr+siItjK7c6QJhhcw", Do we have tests for `keyid` and `data` PHC processing? ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/29597#discussion_r2948312948 PR Review Comment: https://git.openjdk.org/jdk/pull/29597#discussion_r2940782552 PR Review Comment: https://git.openjdk.org/jdk/pull/29597#discussion_r2907083839 PR Review Comment: https://git.openjdk.org/jdk/pull/29597#discussion_r2886920353 PR Review Comment: https://git.openjdk.org/jdk/pull/29597#discussion_r2907096562 PR Review Comment: https://git.openjdk.org/jdk/pull/29597#discussion_r2774533386 PR Review Comment: https://git.openjdk.org/jdk/pull/29597#discussion_r2784108133 PR Review Comment: https://git.openjdk.org/jdk/pull/29597#discussion_r2926708810 PR Review Comment: https://git.openjdk.org/jdk/pull/29597#discussion_r2930669997 PR Review Comment: https://git.openjdk.org/jdk/pull/29597#discussion_r2806094901 PR Review Comment: https://git.openjdk.org/jdk/pull/29597#discussion_r2873382497
