On Tue, 17 Mar 2026 17:10:57 GMT, Artur Barashev <[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? Note that this is the "max" not the default. Thus, you should not use a conservative value such as 64 MiB. Right shift 10 equals `/ 2^10`. I can add some comment for this and the `Long.MAX_VALUE`. As for the meaning of `MEMORY_MAX`, it is used to check the `memory` value. Note that the actual number of memory blocks can be different from `memory` when its value is not multiples of `4*p`. So, `MAX_MEM_BLOCKS` can be incorrect at times. Not too sure about `the limit for (t * m) value`, are you suggesting specifying a limit for this combination instead of `t` and `m` separately? Or Do you mean to use a limit for this combination instead of limiting `t`? > 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? `r.maxMemory(`) is in bytes, so the extra 10 is to convert that to KiB. So combined this w/ 2 (for the 1/4) => 12. Will think about the `Long.MAX_VALUE` case. I thought about limiting iterations, but not sure what would be a reasonable limit, so I have not added it. Any suggestion? > 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? I can rename it (ARGON2_SYNC_POINT is the name used in RFC 9106). As for the ordering, I will take a close look and update this class. > 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`? No, it's `Argon2id` as this is the only supported flavor in the Argon2 family algorithms per the feedback that I received. > 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`? Well, we should design the public APIs so that other providers' implementations can support Argon2i, Argon2d, 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? leftover typo. Should be `mPower` > 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`? Hmm, not necessarily, the exception is for indicating an internal error, e.g. say an additional argument is added which alters the expected format string length but the switch() block failed to handle this case. It's not about the arguments being invalid. We can sure switch to a more-specific exception than RuntimeException. > 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? Nope, the PHC strings that I found so far contains `m`, `t`, `p` only. None contains the 2 optional arguments somehow. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/29597#discussion_r2949884349 PR Review Comment: https://git.openjdk.org/jdk/pull/29597#discussion_r2933438700 PR Review Comment: https://git.openjdk.org/jdk/pull/29597#discussion_r2932891739 PR Review Comment: https://git.openjdk.org/jdk/pull/29597#discussion_r2784001540 PR Review Comment: https://git.openjdk.org/jdk/pull/29597#discussion_r2820672315 PR Review Comment: https://git.openjdk.org/jdk/pull/29597#discussion_r2932823138 PR Review Comment: https://git.openjdk.org/jdk/pull/29597#discussion_r2820717302 PR Review Comment: https://git.openjdk.org/jdk/pull/29597#discussion_r2933520647
