Re: RFR: 8297878: KEM: Implementation [v21]
> The KEM API and DHKEM impl. Note that this PR uses new methods in > https://github.com/openjdk/jdk/pull/13250. Weijun Wang has updated the pull request incrementally with one additional commit since the last revision: New and old SunJCE entries in class comment - Changes: - all: https://git.openjdk.org/jdk/pull/13256/files - new: https://git.openjdk.org/jdk/pull/13256/files/2c7ef3d5..bdd0c63b Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=13256&range=20 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=13256&range=19-20 Stats: 4 lines in 1 file changed: 4 ins; 0 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/13256.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13256/head:pull/13256 PR: https://git.openjdk.org/jdk/pull/13256
Re: RFR: 8297878: KEM: Implementation [v20]
> The KEM API and DHKEM impl. Note that this PR uses new methods in > https://github.com/openjdk/jdk/pull/13250. Weijun Wang has updated the pull request incrementally with one additional commit since the last revision: comment for RSA-KEM - Changes: - all: https://git.openjdk.org/jdk/pull/13256/files - new: https://git.openjdk.org/jdk/pull/13256/files/b3982bdb..2c7ef3d5 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=13256&range=19 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=13256&range=18-19 Stats: 6 lines in 1 file changed: 6 ins; 0 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/13256.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13256/head:pull/13256 PR: https://git.openjdk.org/jdk/pull/13256
Re: RFR: 8297878: KEM: Implementation [v19]
On Thu, 25 May 2023 20:51:55 GMT, Weijun Wang wrote: >> The KEM API and DHKEM impl. Note that this PR uses new methods in >> https://github.com/openjdk/jdk/pull/13250. > > Weijun Wang has updated the pull request incrementally with one additional > commit since the last revision: > > test update Marked as reviewed by mullan (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/13256#pullrequestreview-1444650953
Re: RFR: 8297878: KEM: Implementation [v18]
On Thu, 18 May 2023 17:07:40 GMT, Weijun Wang wrote: >> The KEM API and DHKEM impl. Note that this PR uses new methods in >> https://github.com/openjdk/jdk/pull/13250. > > Weijun Wang 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 18 additional > commits since the last revision: > > - Merge branch 'master' into 8297878 > - Merge branch 'master' into 8297878 > - to and length and comments > - deterministic randomness > - Resolve Siba's comment > - providerName > - more @since and about nulls > - Merge branch 'master' into 8297878 > - no more pk/sk, AIOOBE to IOOBE > - small spec change > - ... and 8 more: https://git.openjdk.org/jdk/compare/288e6369...7cace182 New commit. Resolve Sean's comments. Make SecureRandom settable. - PR Comment: https://git.openjdk.org/jdk/pull/13256#issuecomment-1563486984
Re: RFR: 8297878: KEM: Implementation [v18]
On Thu, 25 May 2023 19:17:51 GMT, Sean Mullan wrote: >> Weijun Wang 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 18 additional >> commits since the last revision: >> >> - Merge branch 'master' into 8297878 >> - Merge branch 'master' into 8297878 >> - to and length and comments >> - deterministic randomness >> - Resolve Siba's comment >> - providerName >> - more @since and about nulls >> - Merge branch 'master' into 8297878 >> - no more pk/sk, AIOOBE to IOOBE >> - small spec change >> - ... and 8 more: https://git.openjdk.org/jdk/compare/288e6369...7cace182 > > test/jdk/javax/crypto/KEM/RSA_KEM.java line 26: > >> 24: /* >> 25: * @test >> 26: * @bug 8149417 > > bugid is wrong Oops, where did I copy it from? - PR Review Comment: https://git.openjdk.org/jdk/pull/13256#discussion_r1205987629
Re: RFR: 8297878: KEM: Implementation [v19]
> The KEM API and DHKEM impl. Note that this PR uses new methods in > https://github.com/openjdk/jdk/pull/13250. Weijun Wang has updated the pull request incrementally with one additional commit since the last revision: test update - Changes: - all: https://git.openjdk.org/jdk/pull/13256/files - new: https://git.openjdk.org/jdk/pull/13256/files/7cace182..b3982bdb Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=13256&range=18 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=13256&range=17-18 Stats: 26 lines in 2 files changed: 14 ins; 0 del; 12 mod Patch: https://git.openjdk.org/jdk/pull/13256.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13256/head:pull/13256 PR: https://git.openjdk.org/jdk/pull/13256
Re: RFR: 8297878: KEM: Implementation [v18]
On Thu, 18 May 2023 17:07:40 GMT, Weijun Wang wrote: >> The KEM API and DHKEM impl. Note that this PR uses new methods in >> https://github.com/openjdk/jdk/pull/13250. > > Weijun Wang 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 18 additional > commits since the last revision: > > - Merge branch 'master' into 8297878 > - Merge branch 'master' into 8297878 > - to and length and comments > - deterministic randomness > - Resolve Siba's comment > - providerName > - more @since and about nulls > - Merge branch 'master' into 8297878 > - no more pk/sk, AIOOBE to IOOBE > - small spec change > - ... and 8 more: https://git.openjdk.org/jdk/compare/86c6bb79...7cace182 test/jdk/com/sun/crypto/provider/DHKEM/Compliance.java line 62: > 60: } > 61: > 62: // Encapsulated comformance checks s/comformance/conformance/ test/jdk/javax/crypto/KEM/RSA_KEM.java line 26: > 24: /* > 25: * @test > 26: * @bug 8149417 bugid is wrong - PR Review Comment: https://git.openjdk.org/jdk/pull/13256#discussion_r1205831599 PR Review Comment: https://git.openjdk.org/jdk/pull/13256#discussion_r1205918247
Re: RFR: 8297878: KEM: Implementation [v18]
On Thu, 18 May 2023 17:07:40 GMT, Weijun Wang wrote: >> The KEM API and DHKEM impl. Note that this PR uses new methods in >> https://github.com/openjdk/jdk/pull/13250. > > Weijun Wang 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 18 additional > commits since the last revision: > > - Merge branch 'master' into 8297878 > - Merge branch 'master' into 8297878 > - to and length and comments > - deterministic randomness > - Resolve Siba's comment > - providerName > - more @since and about nulls > - Merge branch 'master' into 8297878 > - no more pk/sk, AIOOBE to IOOBE > - small spec change > - ... and 8 more: https://git.openjdk.org/jdk/compare/b1a8e951...7cace182 I'm fine with the changes - Marked as reviewed by ascarpino (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/13256#pullrequestreview-1437321126
Re: RFR: 8297878: KEM: Implementation [v18]
> The KEM API and DHKEM impl. Note that this PR uses new methods in > https://github.com/openjdk/jdk/pull/13250. Weijun Wang 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 18 additional commits since the last revision: - Merge branch 'master' into 8297878 - Merge branch 'master' into 8297878 - to and length and comments - deterministic randomness - Resolve Siba's comment - providerName - more @since and about nulls - Merge branch 'master' into 8297878 - no more pk/sk, AIOOBE to IOOBE - small spec change - ... and 8 more: https://git.openjdk.org/jdk/compare/bccb8927...7cace182 - Changes: - all: https://git.openjdk.org/jdk/pull/13256/files - new: https://git.openjdk.org/jdk/pull/13256/files/655d2523..7cace182 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=13256&range=17 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=13256&range=16-17 Stats: 5872 lines in 78 files changed: 4595 ins; 1087 del; 190 mod Patch: https://git.openjdk.org/jdk/pull/13256.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13256/head:pull/13256 PR: https://git.openjdk.org/jdk/pull/13256
Re: RFR: 8297878: KEM: Implementation [v17]
> The KEM API and DHKEM impl. Note that this PR uses new methods in > https://github.com/openjdk/jdk/pull/13250. Weijun Wang 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 17 additional commits since the last revision: - Merge branch 'master' into 8297878 - to and length and comments - deterministic randomness - Resolve Siba's comment - providerName - more @since and about nulls - Merge branch 'master' into 8297878 - no more pk/sk, AIOOBE to IOOBE - small spec change - some constants, no local reverse - ... and 7 more: https://git.openjdk.org/jdk/compare/e1e90f92...655d2523 - Changes: - all: https://git.openjdk.org/jdk/pull/13256/files - new: https://git.openjdk.org/jdk/pull/13256/files/ea8f7964..655d2523 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=13256&range=16 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=13256&range=15-16 Stats: 145061 lines in 2474 files changed: 112277 ins; 13968 del; 18816 mod Patch: https://git.openjdk.org/jdk/pull/13256.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13256/head:pull/13256 PR: https://git.openjdk.org/jdk/pull/13256
Re: RFR: 8297878: KEM: Implementation [v16]
On Wed, 17 May 2023 07:10:49 GMT, Ferenc Rakoczi wrote:
>> Weijun Wang has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> to and length and comments
>
> src/java.base/share/classes/javax/crypto/KEM.java line 233:
>
>> 231: * of the shared secret as a 128-bit AES key.
>> 232: * @throws IndexOutOfBoundsException if {@code from < 0},
>> 233: * {@code from > to}, or {@code to > secretSize()}
>
> Shouldn't this say "{@code to - from > secretSize()}"?
`from` and `to` are indexes into the secret itself, so it's indeed `to` cannot
exceed `secretSize()`.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13256#discussion_r1196492411
Re: RFR: 8297878: KEM: Implementation [v16]
On Tue, 16 May 2023 16:28:26 GMT, Weijun Wang wrote:
>> The KEM API and DHKEM impl. Note that this PR uses new methods in
>> https://github.com/openjdk/jdk/pull/13250.
>
> Weijun Wang has updated the pull request incrementally with one additional
> commit since the last revision:
>
> to and length and comments
src/java.base/share/classes/javax/crypto/KEM.java line 233:
> 231: * of the shared secret as a 128-bit AES key.
> 232: * @throws IndexOutOfBoundsException if {@code from < 0},
> 233: * {@code from > to}, or {@code to > secretSize()}
Shouldn't this say "{@code to - from > secretSize()}"?
src/java.base/share/classes/javax/crypto/KEM.java line 356:
> 354: * decapsulation process
> 355: * @throws IndexOutOfBoundsException if {@code from < 0},
> 356: * {@code from > to}, or {@code to > secretSize()}
Shouldn't this say "{@code to - from > secretSize()}"?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13256#discussion_r1196042549
PR Review Comment: https://git.openjdk.org/jdk/pull/13256#discussion_r1196043626
Re: RFR: 8297878: KEM: Implementation [v16]
> The KEM API and DHKEM impl. Note that this PR uses new methods in > https://github.com/openjdk/jdk/pull/13250. Weijun Wang has updated the pull request incrementally with one additional commit since the last revision: to and length and comments - Changes: - all: https://git.openjdk.org/jdk/pull/13256/files - new: https://git.openjdk.org/jdk/pull/13256/files/128cefe7..ea8f7964 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=13256&range=15 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=13256&range=14-15 Stats: 35 lines in 5 files changed: 25 ins; 0 del; 10 mod Patch: https://git.openjdk.org/jdk/pull/13256.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13256/head:pull/13256 PR: https://git.openjdk.org/jdk/pull/13256
Re: RFR: 8297878: KEM: Implementation [v15]
On Mon, 15 May 2023 19:07:59 GMT, Anthony Scarpino
wrote:
>> Weijun Wang has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> deterministic randomness
>
> src/java.base/share/classes/javax/crypto/KEM.java line 217:
>
>> 215: *
>> 216: * An implementation may choose to not support arbitrary
>> combinations
>> 217: * of {@code from}, {@code to}, and {@code algorithm}.
>
> As previously discussed, I think having a code example of the `from` and `to`
> would be good idea. That way it's very clear going from 0 to 32 is 32 bytes
> and not 33. And an example would be a good idea in the SPI.
Added some comments and example lines. And I found A REAL BUG in `DHKEM`!!!
Having two styles of slicing an array is indeed a problem.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13256#discussion_r1195412465
Re: RFR: 8297878: KEM: Implementation [v15]
On Thu, 11 May 2023 20:56:54 GMT, Weijun Wang wrote:
>> The KEM API and DHKEM impl. Note that this PR uses new methods in
>> https://github.com/openjdk/jdk/pull/13250.
>
> Weijun Wang has updated the pull request incrementally with one additional
> commit since the last revision:
>
> deterministic randomness
src/java.base/share/classes/javax/crypto/KEM.java line 217:
> 215: *
> 216: * An implementation may choose to not support arbitrary
> combinations
> 217: * of {@code from}, {@code to}, and {@code algorithm}.
As previously discussed, I think having a code example of the `from` and `to`
would be good idea. That way it's very clear going from 0 to 32 is 32 bytes
and not 33. And an example would be a good idea in the SPI.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13256#discussion_r1194258367
Re: RFR: 8297878: KEM: Implementation [v15]
On Thu, 11 May 2023 20:56:54 GMT, Weijun Wang wrote:
>> The KEM API and DHKEM impl. Note that this PR uses new methods in
>> https://github.com/openjdk/jdk/pull/13250.
>
> Weijun Wang has updated the pull request incrementally with one additional
> commit since the last revision:
>
> deterministic randomness
test/jdk/com/sun/crypto/provider/DHKEM/Compliance.java line 205:
> 203: byte[] enc1 = e.encapsulate().encapsulation();
> 204: byte[] enc2 = e.encapsulate().encapsulation();
> 205: Asserts.assertFalse(Arrays.equals(enc1, enc2));
Another case,
KEM kem = KEM.getInstance("DHKEM");
KEM.Encapsulator e1 = kem.newEncapsulator(pk, random);
KEM kem1 = KEM.getInstance("DHKEM");
KEM.Encapsulator e2 = kem1.newEncapsulator(pk, random);
byte[] enc1 = e1.encapsulate().encapsulation();
byte[] enc2 = e2.encapsulate().encapsulation();
Asserts.assertFalse(Arrays.equals(enc1, enc2));
Can this case be added too?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13256#discussion_r1192194605
Re: RFR: 8297878: KEM: Implementation [v15]
> The KEM API and DHKEM impl. Note that this PR uses new methods in > https://github.com/openjdk/jdk/pull/13250. Weijun Wang has updated the pull request incrementally with one additional commit since the last revision: deterministic randomness - Changes: - all: https://git.openjdk.org/jdk/pull/13256/files - new: https://git.openjdk.org/jdk/pull/13256/files/c9f1d8f3..128cefe7 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=13256&range=14 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=13256&range=13-14 Stats: 45 lines in 1 file changed: 41 ins; 3 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/13256.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13256/head:pull/13256 PR: https://git.openjdk.org/jdk/pull/13256
Re: RFR: 8297878: KEM: Implementation [v14]
On Mon, 8 May 2023 16:47:48 GMT, Weijun Wang wrote: >>> I added another proposal in you PR at [#13470 >>> (comment)](https://github.com/openjdk/jdk/pull/13470#issuecomment-1508915798). >> >> I like the idea to use abstract class. The concerns about provider() method >> get addressed. > > I take back the proposal. I take back the proposal. - PR Review Comment: https://git.openjdk.org/jdk/pull/13256#discussion_r1187661541
Re: RFR: 8297878: KEM: Implementation [v14]
On Fri, 14 Apr 2023 16:33:48 GMT, Xue-Lei Andrew Fan wrote: >> I added another proposal in you PR at >> https://github.com/openjdk/jdk/pull/13470#issuecomment-1508915798. > >> I added another proposal in you PR at [#13470 >> (comment)](https://github.com/openjdk/jdk/pull/13470#issuecomment-1508915798). > > I like the idea to use abstract class. The concerns about provider() method > get addressed. I take back the proposal. - PR Review Comment: https://git.openjdk.org/jdk/pull/13256#discussion_r1187661073
Re: RFR: 8297878: KEM: Implementation [v14]
> The KEM API and DHKEM impl. Note that this PR uses new methods in > https://github.com/openjdk/jdk/pull/13250. Weijun Wang has updated the pull request incrementally with one additional commit since the last revision: Resolve Siba's comment - Changes: - all: https://git.openjdk.org/jdk/pull/13256/files - new: https://git.openjdk.org/jdk/pull/13256/files/c2daf4e3..c9f1d8f3 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=13256&range=13 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=13256&range=12-13 Stats: 7 lines in 1 file changed: 1 ins; 5 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/13256.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13256/head:pull/13256 PR: https://git.openjdk.org/jdk/pull/13256
Re: RFR: 8297878: KEM: Implementation [v12]
On Wed, 3 May 2023 06:04:51 GMT, Sibabrata Sahoo wrote: >> Weijun Wang has updated the pull request incrementally with one additional >> commit since the last revision: >> >> more @since and about nulls > > test/jdk/com/sun/crypto/provider/DHKEM/Compliance.java line 112: > >> 110: >> ExChecker.of(InvalidKeyException.class).by(DHKEM.class)); >> 111: >> 112: // Not an EC key at all, rejected by framework coz >> SupportedClasses > > Do you mean UnsupportedClasses in comment section? I meant the "SupportedKeyClasses" attribute in `SunJCE.java`. Will fix the word. - PR Review Comment: https://git.openjdk.org/jdk/pull/13256#discussion_r1183635243
Re: RFR: 8297878: KEM: Implementation [v12]
On Wed, 3 May 2023 05:22:04 GMT, Sibabrata Sahoo wrote:
>> Weijun Wang has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> more @since and about nulls
>
> test/jdk/com/sun/crypto/provider/DHKEM/Compliance.java line 97:
>
>> 95: // Cannot detect invalid params before provider selection
>> 96: //Utils.runAndCheckException(
>> 97: //() -> KEM.getInstance("DHKEM", new KEMParameterSpec()
>> {}),
>
> Commented code block can be removed. Signature of getInstance() doesn't exist.
Oops. Will remove.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13256#discussion_r1183631427
Re: RFR: 8297878: KEM: Implementation [v12]
On Thu, 27 Apr 2023 15:40:53 GMT, Weijun Wang wrote:
>> The KEM API and DHKEM impl. Note that this PR uses new methods in
>> https://github.com/openjdk/jdk/pull/13250.
>
> Weijun Wang has updated the pull request incrementally with one additional
> commit since the last revision:
>
> more @since and about nulls
test/jdk/com/sun/crypto/provider/DHKEM/Compliance.java line 97:
> 95: // Cannot detect invalid params before provider selection
> 96: //Utils.runAndCheckException(
> 97: //() -> KEM.getInstance("DHKEM", new KEMParameterSpec()
> {}),
Commented code block can be removed. Signature of getInstance() doesn't exist.
test/jdk/com/sun/crypto/provider/DHKEM/Compliance.java line 112:
> 110: ExChecker.of(InvalidKeyException.class).by(DHKEM.class));
> 111:
> 112: // Not an EC key at all, rejected by framework coz
> SupportedClasses
Do you mean UnsupportedClasses in comment section?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13256#discussion_r1183247590
PR Review Comment: https://git.openjdk.org/jdk/pull/13256#discussion_r1183269537
Re: RFR: 8297878: KEM: Implementation [v12]
On Thu, 27 Apr 2023 15:40:53 GMT, Weijun Wang wrote: >> The KEM API and DHKEM impl. Note that this PR uses new methods in >> https://github.com/openjdk/jdk/pull/13250. > > Weijun Wang has updated the pull request incrementally with one additional > commit since the last revision: > > more @since and about nulls New commit pushed. `s/provider/providerName/`. Now we can claim the `Encapsulator` and `Decapsulator` classes are immutable. - PR Comment: https://git.openjdk.org/jdk/pull/13256#issuecomment-1531899641
Re: RFR: 8297878: KEM: Implementation [v13]
> The KEM API and DHKEM impl. Note that this PR uses new methods in > https://github.com/openjdk/jdk/pull/13250. Weijun Wang has updated the pull request incrementally with one additional commit since the last revision: providerName - Changes: - all: https://git.openjdk.org/jdk/pull/13256/files - new: https://git.openjdk.org/jdk/pull/13256/files/51be3375..c2daf4e3 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=13256&range=12 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=13256&range=11-12 Stats: 13 lines in 2 files changed: 1 ins; 0 del; 12 mod Patch: https://git.openjdk.org/jdk/pull/13256.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13256/head:pull/13256 PR: https://git.openjdk.org/jdk/pull/13256
Re: RFR: 8297878: KEM: Implementation [v12]
On Thu, 27 Apr 2023 15:40:53 GMT, Weijun Wang wrote: >> The KEM API and DHKEM impl. Note that this PR uses new methods in >> https://github.com/openjdk/jdk/pull/13250. > > Weijun Wang has updated the pull request incrementally with one additional > commit since the last revision: > > more @since and about nulls Maybe 1.6 is already modern enough. ;-) `to` is useful if `secretSize` is 48 but you want an AES-256 key, then you would call `encapsulate(0, 32, "AES")` or `encapsulate(16, 48, "AES")`, depending on how your protocol demands. - PR Comment: https://git.openjdk.org/jdk/pull/13256#issuecomment-1528128560
Re: RFR: 8297878: KEM: Implementation [v12]
On Fri, 28 Apr 2023 00:47:39 GMT, Weijun Wang wrote: > My feeling is that modern APIs use `from` and `to` instead of `from` and > `length`. For example, `Arrays.copyOfRange`, `Arrays.equals` and > `Arrays.compare` vs `new String(daata, from, length)`. I don't see this as a modern vs old API style, it's consistency. `Arrays.copyOfRange()` is from 1.6. I think it's being consistent with other JCA classes, like Cipher, Signature, MessageDigest, and Mac. I'm not aware of any JCA that uses (from, to). Also `Arrays` methods use length of any size. KEM does not. In fact it is very obvious what the length is as it must be the key size when looking at `DHKEM.decapsulate(...)`: Objects.checkFromToIndex(from, to, params.Nsecret); One could argue that `to` isn't necessary given the length is fixed by the `params`. - PR Comment: https://git.openjdk.org/jdk/pull/13256#issuecomment-1527859403
Re: RFR: 8297878: KEM: Implementation [v12]
On Fri, 28 Apr 2023 00:47:39 GMT, Weijun Wang wrote: > I feel the use of `from` and `to` is error prone. I know I never remember if > Arrays.copyOfRange() includes or excludes the `to` index. I think less > mistakes would be made if it used `index` and `length` instead. I think from and to are confusing mainly to old C programmers who got used to the "for (I = 0; I < n; i++)" way of expressing a range of length n, a syntax of "for I going from 0 to n-1" would have been more natural (to me at least). When you say "I will be on vacation from the 12th to the 14th of August", you wouldn't think you would be back at work on the 14th, would you (well, maybe an old C programmer would :-) )? To me, "from, to" and "start, length" are equivalent (provided from == start && length == to - from + 1). - PR Comment: https://git.openjdk.org/jdk/pull/13256#issuecomment-1527201102
Re: RFR: 8297878: KEM: Implementation [v12]
On Thu, 27 Apr 2023 22:53:04 GMT, Anthony Scarpino wrote: > I feel the use of `from` and `to` is error prone. I know I never remember if > Arrays.copyOfRange() includes or excludes the `to` index. I think less > mistakes would be made if it used `index` and `length` instead. My feeling is that modern APIs use `from` and `to` instead of `from` and `length`. For example, `Arrays.copyOfRange`, `Arrays.equals` and `Arrays.compare` vs `new String(daata, from, length)`. - PR Comment: https://git.openjdk.org/jdk/pull/13256#issuecomment-1526824465
Re: RFR: 8297878: KEM: Implementation [v12]
On Thu, 27 Apr 2023 15:40:53 GMT, Weijun Wang wrote: >> The KEM API and DHKEM impl. Note that this PR uses new methods in >> https://github.com/openjdk/jdk/pull/13250. > > Weijun Wang has updated the pull request incrementally with one additional > commit since the last revision: > > more @since and about nulls I feel the use of `from` and `to` is error prone. I know I never remember if Arrays.copyOfRange() includes or excludes the `to` index. I think less mistakes would be made if it used `index` and `length` instead. - PR Comment: https://git.openjdk.org/jdk/pull/13256#issuecomment-1526680954
Re: RFR: 8297878: KEM: Implementation [v10]
On Thu, 27 Apr 2023 13:22:38 GMT, Sean Mullan wrote:
>> I checked for our implementations, and until now all I found is IKE. Even
>> the PBEWithMD5AndDES algorithm mentioned in
>> https://bugs.openjdk.org/browse/JDK-4953551 also throws an IKE.
>>
>> I'd rather follow the existing style. If it’s not clear enough, I can update
>> the line to
>>
>> @throws InvalidKeyException if {@code publicKey} is {@code null} or invalid
>>
>>
>> If we are worried that a security provider might throw an NPE, how about we
>> add a check on the user API side:
>>
>> public Encapsulator newEncapsulator(PublicKey publicKey,
>> AlgorithmParameterSpec spec, SecureRandom secureRandom)
>> throws InvalidAlgorithmParameterException, InvalidKeyException {
>> if (publicKey == null) {
>> throw new InvalidKeyException("Null key provided");
>> }
>> return delayed != null
>> ? delayed.newEncapsulator(publicKey, spec, secureRandom)
>> : new Encapsulator(spi.engineNewEncapsulator(publicKey,
>> spec, secureRandom), provider);
>> }
>
>> I checked for our implementations, and until now all I found is IKE. Even
>> the PBEWithMD5AndDES algorithm mentioned in
>> https://bugs.openjdk.org/browse/JDK-4953551 also throws an IKE.
>>
>> I'd rather follow the existing style. If it’s not clear enough, I can update
>> the line to
>>
>> ```
>> @throws InvalidKeyException if {@code publicKey} is {@code null} or invalid
>> ```
>
> I'll leave it up to you, or consensus from others. This one particular issue
> is probably not going to overly affect the API much.
>
> The APIs you referenced were created a long time ago before more accepted API
> best practices evolved. Also, this API already throws NPE in other methods
> when a required parameter is `null`, so these methods seem inconsistent with
> the way it treats `null` parameters.
The new `KEM` class still follows the API/SPI design principal. I gave up
designing `DecapsulteException` as an uncheck exception and I still use class
instead of record for `Encapsulated`. So let's consistently traditional.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13256#discussion_r1179528648
Re: RFR: 8297878: KEM: Implementation [v7]
On Mon, 24 Apr 2023 19:55:34 GMT, Weijun Wang wrote:
>> src/java.base/share/classes/com/sun/crypto/provider/DHKEM.java line 366:
>>
>>> 364: private static byte[] concat(byte[]... inputs) {
>>> 365: ByteArrayOutputStream o = new ByteArrayOutputStream();
>>> 366: Arrays.stream(inputs).forEach(o::writeBytes);
>>
>> Unless I'm missing something there is no `stream(byte[])` support, so I'm
>> not sure how this is compiling. I didn't think the generics would work
>> with this.
>> More importantly, `forEach()` the API states that stream is in a
>> non-deterministic order. I think you want `forEachOrdered()`
>
> `inputs` is `byte[][]` and there is a `Arrays.stream(T[])`.
>
> The above method says a `sequential Stream` is returned. Does that mean
> calling `forEach` is safe?
Ok, this is fine, I didn't realize sequential streams were default
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13256#discussion_r1179500876
Re: RFR: 8297878: KEM: Implementation [v12]
On Thu, 27 Apr 2023 15:22:17 GMT, Weijun Wang wrote: >> The KEM API and DHKEM impl. Note that this PR uses new methods in >> https://github.com/openjdk/jdk/pull/13250. > > Weijun Wang has updated the pull request incrementally with one additional > commit since the last revision: > > more @since and about nulls I didn't realize `@` in a commit message can still be linked to a user. - PR Comment: https://git.openjdk.org/jdk/pull/13256#issuecomment-1525895220
Re: RFR: 8297878: KEM: Implementation [v10]
On Thu, 27 Apr 2023 14:38:26 GMT, Weijun Wang wrote:
>> src/java.base/share/classes/javax/crypto/KEM.java line 100:
>>
>>> 98: * @see Encapsulator#encapsulate(int, int, String)
>>> 99: */
>>> 100: public static final class Encapsulated {
>>
>> Missing `@since 21`.
>
> I thought there is no need to add this to an inner class. I'll ask Joe for
> advice.
I've added `@since` to all inner classes.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13256#discussion_r1179337827
Re: RFR: 8297878: KEM: Implementation [v12]
> The KEM API and DHKEM impl. Note that this PR uses new methods in > https://github.com/openjdk/jdk/pull/13250. Weijun Wang has updated the pull request incrementally with one additional commit since the last revision: more @since and about nulls - Changes: - all: https://git.openjdk.org/jdk/pull/13256/files - new: https://git.openjdk.org/jdk/pull/13256/files/8ba16065..51be3375 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=13256&range=11 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=13256&range=10-11 Stats: 22 lines in 2 files changed: 10 ins; 0 del; 12 mod Patch: https://git.openjdk.org/jdk/pull/13256.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13256/head:pull/13256 PR: https://git.openjdk.org/jdk/pull/13256
Re: RFR: 8297878: KEM: Implementation [v10]
On Thu, 27 Apr 2023 12:54:03 GMT, Sean Mullan wrote:
>> Weijun Wang has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> no more pk/sk, AIOOBE to IOOBE
>
> src/java.base/share/classes/javax/crypto/KEM.java line 90:
>
>> 88:
>> 89: /**
>> 90: * This class specifies the returned value of the encapsulate method
>> of
>
> s/returned/return/
OK.
> src/java.base/share/classes/javax/crypto/KEM.java line 100:
>
>> 98: * @see Encapsulator#encapsulate(int, int, String)
>> 99: */
>> 100: public static final class Encapsulated {
>
> Missing `@since 21`.
I thought there is no need to add this to an inner class. I'll ask Joe for
advice.
> src/java.base/share/classes/javax/crypto/KEM.java line 148:
>
>> 146: * Returns the optional parameters in a byte array.
>> 147: *
>> 148: * @return the optional parameters in a byte array. A new copy
>> of the
>
> Should probably say "the optional parameters in a byte array or `null` if not
> specified`.
OK.
> src/java.base/share/classes/javax/crypto/KEM.java line 239:
>
>> 237: * Returns the size of the shared secret.
>> 238: *
>> 239: * This method can be called to find out the length of the
>> share secret
>
> s/share/shared/
Oops.
> src/java.base/share/classes/javax/crypto/KEM.java line 362:
>
>> 360: * Returns the size of the shared secret.
>> 361: *
>> 362: * This method can be called to find out the length of the
>> share secret
>
> s/share/shared/
Oops.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13256#discussion_r1179263143
PR Review Comment: https://git.openjdk.org/jdk/pull/13256#discussion_r1179262971
PR Review Comment: https://git.openjdk.org/jdk/pull/13256#discussion_r1179262197
PR Review Comment: https://git.openjdk.org/jdk/pull/13256#discussion_r1179261989
PR Review Comment: https://git.openjdk.org/jdk/pull/13256#discussion_r1179261856
Re: RFR: 8297878: KEM: Implementation [v10]
On Wed, 26 Apr 2023 19:02:53 GMT, Weijun Wang wrote:
>> The KEM API and DHKEM impl. Note that this PR uses new methods in
>> https://github.com/openjdk/jdk/pull/13250.
>
> Weijun Wang has updated the pull request incrementally with one additional
> commit since the last revision:
>
> no more pk/sk, AIOOBE to IOOBE
src/java.base/share/classes/javax/crypto/KEM.java line 90:
> 88:
> 89: /**
> 90: * This class specifies the returned value of the encapsulate method
> of
s/returned/return/
src/java.base/share/classes/javax/crypto/KEM.java line 100:
> 98: * @see Encapsulator#encapsulate(int, int, String)
> 99: */
> 100: public static final class Encapsulated {
Missing `@since 21`.
src/java.base/share/classes/javax/crypto/KEM.java line 148:
> 146: * Returns the optional parameters in a byte array.
> 147: *
> 148: * @return the optional parameters in a byte array. A new copy
> of the
Should probably say "the optional parameters in a byte array or `null` if not
specified`.
src/java.base/share/classes/javax/crypto/KEM.java line 239:
> 237: * Returns the size of the shared secret.
> 238: *
> 239: * This method can be called to find out the length of the share
> secret
s/share/shared/
src/java.base/share/classes/javax/crypto/KEM.java line 362:
> 360: * Returns the size of the shared secret.
> 361: *
> 362: * This method can be called to find out the length of the share
> secret
s/share/shared/
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13256#discussion_r1179113396
PR Review Comment: https://git.openjdk.org/jdk/pull/13256#discussion_r1179115920
PR Review Comment: https://git.openjdk.org/jdk/pull/13256#discussion_r1179119002
PR Review Comment: https://git.openjdk.org/jdk/pull/13256#discussion_r1179122638
PR Review Comment: https://git.openjdk.org/jdk/pull/13256#discussion_r1179127665
Re: RFR: 8297878: KEM: Implementation [v10]
On Wed, 26 Apr 2023 21:36:19 GMT, Weijun Wang wrote:
> I checked for our implementations, and until now all I found is IKE. Even the
> PBEWithMD5AndDES algorithm mentioned in
> https://bugs.openjdk.org/browse/JDK-4953551 also throws an IKE.
>
> I'd rather follow the existing style. If it’s not clear enough, I can update
> the line to
>
> ```
> @throws InvalidKeyException if {@code publicKey} is {@code null} or invalid
> ```
I'll leave it up to you, or consensus from others. This one particular issue is
probably not going to overly affect the API much.
The APIs you referenced were created a long time ago before more accepted API
best practices evolved. Also, this API already throws NPE in other methods when
a required parameter is `null`, so these methods seem inconsistent with the way
it treats `null` parameters.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13256#discussion_r1179150432
Re: RFR: 8297878: KEM: Implementation [v11]
> The KEM API and DHKEM impl. Note that this PR uses new methods in > https://github.com/openjdk/jdk/pull/13250. Weijun Wang 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 11 additional commits since the last revision: - Merge branch 'master' into 8297878 - no more pk/sk, AIOOBE to IOOBE - small spec change - some constants, no local reverse - fine tuning spec - move Encapsulator before Decapsulator - fine tune spec - let implementor validate arguments, null key throws IKE - spec changes - spec change, getAlgorithm - ... and 1 more: https://git.openjdk.org/jdk/compare/13457d46...8ba16065 - Changes: - all: https://git.openjdk.org/jdk/pull/13256/files - new: https://git.openjdk.org/jdk/pull/13256/files/7ada6f0b..8ba16065 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=13256&range=10 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=13256&range=09-10 Stats: 311246 lines in 3091 files changed: 261349 ins; 31771 del; 18126 mod Patch: https://git.openjdk.org/jdk/pull/13256.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13256/head:pull/13256 PR: https://git.openjdk.org/jdk/pull/13256
Re: RFR: 8297878: KEM: Implementation [v10]
On Wed, 26 Apr 2023 20:56:38 GMT, Sean Mullan wrote:
>> Every other crypto engine class throws IKE when initing with a null key,
>> including `Signature`, `Cipher`, `KeyAgreement`, `Mac`. So I follow the same
>> style.
>
> Hmm, while I understand your thinking, this seems like the wrong pattern to
> follow though (ex: see Effective Java, Item 70), and it doesn't seem like
> something we should have to adhere to for API consistency. There are various
> bugs filed over the years on confusion over this style (search for "Cipher
> init NullPointerException"), where providers threw NPE or users thought the
> APIs should. Actually, there is still an open bug:
> https://bugs.openjdk.org/browse/JDK-4955097
I checked for our implementations, and until now all I found is IKE. Even the
PBEWithMD5AndDES algorithm mentioned in
https://bugs.openjdk.org/browse/JDK-4953551 also throws an IKE.
I'd rather follow the existing style. If it’s not clear enough, I can update
the line to
@throws InvalidKeyException if {@code publicKey} is {@code null} or invalid
If we are worried that a security provider might throw an NPE, how about we add
a check on the user API side:
public Encapsulator newEncapsulator(PublicKey publicKey,
AlgorithmParameterSpec spec, SecureRandom secureRandom)
throws InvalidAlgorithmParameterException, InvalidKeyException {
if (publicKey == null) {
throw new InvalidKeyException("Null key provided");
}
return delayed != null
? delayed.newEncapsulator(publicKey, spec, secureRandom)
: new Encapsulator(spi.engineNewEncapsulator(publicKey, spec,
secureRandom), provider);
}
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13256#discussion_r1178422376
Re: RFR: 8297878: KEM: Implementation [v10]
On Wed, 26 Apr 2023 20:36:02 GMT, Weijun Wang wrote:
>> src/java.base/share/classes/javax/crypto/KEM.java line 606:
>>
>>> 604: * @param publicKey the receiver's public key, must not be {@code
>>> null}
>>> 605: * @return the encapsulator for this key
>>> 606: * @throws InvalidKeyException if {@code publicKey} is invalid
>>
>> What about throwing `NullPointerException` if `publicKey` is `null`? In this
>> case a `RuntimeException` seems more appropriate, and throwing NPE seems
>> more consistent with how you treat `null` parameters of other methods.
>
> Every other crypto engine class throws IKE when initing with a null key,
> including `Signature`, `Cipher`, `KeyAgreement`, 'Mac`. So I follow the same
> style.
Hmm, while I understand your thinking, this seems like the wrong pattern to
follow though (ex: see Effective Java, Item 70), and it doesn't seem like
something we should have to adhere to for API consistency. There are various
bugs filed over the years on confusion over this style (search for "Cipher init
NullPointerException"), where providers threw NPE or users thought the APIs
should. Actually, there is still an open bug:
https://bugs.openjdk.org/browse/JDK-4955097
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13256#discussion_r1178390743
Re: RFR: 8297878: KEM: Implementation [v10]
On Wed, 26 Apr 2023 20:21:49 GMT, Sean Mullan wrote:
>> Weijun Wang has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> no more pk/sk, AIOOBE to IOOBE
>
> src/java.base/share/classes/javax/crypto/KEM.java line 606:
>
>> 604: * @param publicKey the receiver's public key, must not be {@code
>> null}
>> 605: * @return the encapsulator for this key
>> 606: * @throws InvalidKeyException if {@code publicKey} is invalid
>
> What about throwing `NullPointerException` if `publicKey` is `null`? In this
> case a `RuntimeException` seems more appropriate, and throwing NPE seems more
> consistent with how you treat `null` parameters of other methods.
Every other crypto engine class throws IKE when initing with a null key,
including `Signature`, `Cipher`, `KeyAgreement`, 'Mac`. So I follow the same
style.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13256#discussion_r1178371353
Re: RFR: 8297878: KEM: Implementation [v10]
On Wed, 26 Apr 2023 19:02:53 GMT, Weijun Wang wrote:
>> The KEM API and DHKEM impl. Note that this PR uses new methods in
>> https://github.com/openjdk/jdk/pull/13250.
>
> Weijun Wang has updated the pull request incrementally with one additional
> commit since the last revision:
>
> no more pk/sk, AIOOBE to IOOBE
src/java.base/share/classes/javax/crypto/KEM.java line 606:
> 604: * @param publicKey the receiver's public key, must not be {@code
> null}
> 605: * @return the encapsulator for this key
> 606: * @throws InvalidKeyException if {@code publicKey} is invalid
What about throwing `NullPointerException` if `publicKey` is `null`? In this
case a `RuntimeException` seems more appropriate, and throwing NPE seems more
consistent with how you treat `null` parameters of other methods.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13256#discussion_r1178356851
Re: RFR: 8297878: KEM: Implementation [v10]
On Wed, 26 Apr 2023 19:02:53 GMT, Weijun Wang wrote: >> The KEM API and DHKEM impl. Note that this PR uses new methods in >> https://github.com/openjdk/jdk/pull/13250. > > Weijun Wang has updated the pull request incrementally with one additional > commit since the last revision: > > no more pk/sk, AIOOBE to IOOBE CSR also updated. - PR Comment: https://git.openjdk.org/jdk/pull/13256#issuecomment-1523915569
Re: RFR: 8297878: KEM: Implementation [v10]
> The KEM API and DHKEM impl. Note that this PR uses new methods in > https://github.com/openjdk/jdk/pull/13250. Weijun Wang has updated the pull request incrementally with one additional commit since the last revision: no more pk/sk, AIOOBE to IOOBE - Changes: - all: https://git.openjdk.org/jdk/pull/13256/files - new: https://git.openjdk.org/jdk/pull/13256/files/c33ef612..7ada6f0b Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=13256&range=09 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=13256&range=08-09 Stats: 59 lines in 2 files changed: 3 ins; 0 del; 56 mod Patch: https://git.openjdk.org/jdk/pull/13256.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13256/head:pull/13256 PR: https://git.openjdk.org/jdk/pull/13256
Re: RFR: 8297878: KEM: Implementation [v9]
On Wed, 26 Apr 2023 17:56:12 GMT, Sean Mullan wrote:
>> Weijun Wang has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> small spec change
>
> src/java.base/share/classes/javax/crypto/KEMSpi.java line 127:
>
>> 125: * the key encapsulation message, and optional
>> parameters.
>> 126: * @throws ArrayIndexOutOfBoundsException if {@code from < 0},
>> 127: * {@code from > to}, or {@code to > secretSize()}
>
> Should this throw an `IndexOutOfBoundsException` instead here? That's what
> the DHKEM impl does and this seems more reasonable since you aren't actually
> trying to access an array.
Yes, you're right. I actually didn't realize the difference between IOOBE and
AIOOBE at all. In fact, I checked for IOOBE in the `Compliance.java` test.
> src/java.base/share/classes/javax/crypto/KEMSpi.java line 168:
>
>> 166: *
>> 167: * An implementation must support the case where {@code from}
>> is 0,
>> 168: * {@code from} is the same as the output of {@code
>> secretSize()},
>
> Same comment as above.
Will fix.
> src/java.base/share/classes/javax/crypto/KEMSpi.java line 184:
>
>> 182: * @throws DecapsulateException if an error occurs during the
>> 183: * decapsulation process
>> 184: * @throws ArrayIndexOutOfBoundsException if {@code from < 0},
>
> Same comment as above.
Will fix.
> src/java.base/share/classes/javax/crypto/KEMSpi.java line 242:
>
>> 240: * @see KEM#newDecapsulator(PrivateKey, AlgorithmParameterSpec)
>> 241: */
>> 242: DecapsulatorSpi engineNewDecapsulator(PrivateKey sk,
>> AlgorithmParameterSpec spec)
>
> Bit of a nit, but suggest changing variable names to be more descriptive:
> `privKey` (or `privateKey`) and `pubKey` (or `publicKey`), for the
> `engineEncapsulate` method. I assume `sk` is taken from RFC 9180, but I don't
> think we need to use the same variable names.
OK, I'll change them.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13256#discussion_r1178254497
PR Review Comment: https://git.openjdk.org/jdk/pull/13256#discussion_r1178256035
PR Review Comment: https://git.openjdk.org/jdk/pull/13256#discussion_r1178255927
PR Review Comment: https://git.openjdk.org/jdk/pull/13256#discussion_r1178247018
Re: RFR: 8297878: KEM: Implementation [v9]
On Wed, 26 Apr 2023 17:34:51 GMT, Sean Mullan wrote:
>> src/java.base/share/classes/javax/crypto/KEMSpi.java line 236:
>>
>>> 234: *
>>> 235: * @param sk the receiver's private key. This argument is never
>>> {@code null}.
>>> 236: * @param spec the parameter, can be {@code null}
>>
>> "the optional parameter, ..." (use same wording as engineNewEncapsulator).
>
> Don't see that this is fixed yet.
Oops. Will fix now.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13256#discussion_r1178241538
Re: RFR: 8297878: KEM: Implementation [v9]
On Wed, 26 Apr 2023 14:29:25 GMT, Weijun Wang wrote:
>> The KEM API and DHKEM impl. Note that this PR uses new methods in
>> https://github.com/openjdk/jdk/pull/13250.
>
> Weijun Wang has updated the pull request incrementally with one additional
> commit since the last revision:
>
> small spec change
src/java.base/share/classes/javax/crypto/KEMSpi.java line 111:
> 109: * The key encapsulation function.
> 110: *
> 111: * Each invocation of this method must generates a new secret
> key and key
s/generates/generate/
src/java.base/share/classes/javax/crypto/KEMSpi.java line 115:
> 113: *
> 114: * An implementation must support the case where {@code from} is
> 0,
> 115: * {@code from} is the same as the output of {@code
> secretSize()},
Should "from" be "to"?
Maybe also say "return value" instead of "output".
src/java.base/share/classes/javax/crypto/KEMSpi.java line 125:
> 123: * @return an {@link KEM.Encapsulated} object containing a
> portion of
> 124: * the shared secret as a key with the specified
> algorithm,
> 125: * the key encapsulation message, and optional
> parameters.
Remove "the".
src/java.base/share/classes/javax/crypto/KEMSpi.java line 127:
> 125: * the key encapsulation message, and optional
> parameters.
> 126: * @throws ArrayIndexOutOfBoundsException if {@code from < 0},
> 127: * {@code from > to}, or {@code to > secretSize()}
Should this throw an `IndexOutOfBoundsException` instead here? That's what the
DHKEM impl does and this seems more reasonable since you aren't actually trying
to access an array.
src/java.base/share/classes/javax/crypto/KEMSpi.java line 168:
> 166: *
> 167: * An implementation must support the case where {@code from} is
> 0,
> 168: * {@code from} is the same as the output of {@code
> secretSize()},
Same comment as above.
src/java.base/share/classes/javax/crypto/KEMSpi.java line 184:
> 182: * @throws DecapsulateException if an error occurs during the
> 183: * decapsulation process
> 184: * @throws ArrayIndexOutOfBoundsException if {@code from < 0},
Same comment as above.
src/java.base/share/classes/javax/crypto/KEMSpi.java line 242:
> 240: * @see KEM#newDecapsulator(PrivateKey, AlgorithmParameterSpec)
> 241: */
> 242: DecapsulatorSpi engineNewDecapsulator(PrivateKey sk,
> AlgorithmParameterSpec spec)
Bit of a nit, but suggest changing variable names to be more descriptive:
`privKey` (or `privateKey`) and `pubKey` (or `publicKey`), for the
`engineEncapsulate` method. I assume `sk` is taken from RFC 9180, but I don't
think we need to use the same variable names.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13256#discussion_r1178210665
PR Review Comment: https://git.openjdk.org/jdk/pull/13256#discussion_r1178212708
PR Review Comment: https://git.openjdk.org/jdk/pull/13256#discussion_r1178214239
PR Review Comment: https://git.openjdk.org/jdk/pull/13256#discussion_r1178219972
PR Review Comment: https://git.openjdk.org/jdk/pull/13256#discussion_r1178221846
PR Review Comment: https://git.openjdk.org/jdk/pull/13256#discussion_r1178220363
PR Review Comment: https://git.openjdk.org/jdk/pull/13256#discussion_r1178207872
Re: RFR: 8297878: KEM: Implementation [v9]
On Wed, 12 Apr 2023 15:22:19 GMT, Sean Mullan wrote:
>> Weijun Wang has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> small spec change
>
> src/java.base/share/classes/javax/crypto/KEMSpi.java line 236:
>
>> 234: *
>> 235: * @param sk the receiver's private key. This argument is never
>> {@code null}.
>> 236: * @param spec the parameter, can be {@code null}
>
> "the optional parameter, ..." (use same wording as engineNewEncapsulator).
Don't see that this is fixed yet.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13256#discussion_r1178200175
Re: RFR: 8297878: KEM: Implementation [v9]
> The KEM API and DHKEM impl. Note that this PR uses new methods in > https://github.com/openjdk/jdk/pull/13250. Weijun Wang has updated the pull request incrementally with one additional commit since the last revision: small spec change - Changes: - all: https://git.openjdk.org/jdk/pull/13256/files - new: https://git.openjdk.org/jdk/pull/13256/files/38608993..c33ef612 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=13256&range=08 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=13256&range=07-08 Stats: 6 lines in 2 files changed: 0 ins; 1 del; 5 mod Patch: https://git.openjdk.org/jdk/pull/13256.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13256/head:pull/13256 PR: https://git.openjdk.org/jdk/pull/13256
Re: RFR: 8297878: KEM: Implementation [v8]
> The KEM API and DHKEM impl. Note that this PR uses new methods in > https://github.com/openjdk/jdk/pull/13250. Weijun Wang has updated the pull request incrementally with one additional commit since the last revision: some constants, no local reverse - Changes: - all: https://git.openjdk.org/jdk/pull/13256/files - new: https://git.openjdk.org/jdk/pull/13256/files/d1a7f3e8..38608993 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=13256&range=07 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=13256&range=06-07 Stats: 75 lines in 4 files changed: 25 ins; 22 del; 28 mod Patch: https://git.openjdk.org/jdk/pull/13256.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13256/head:pull/13256 PR: https://git.openjdk.org/jdk/pull/13256
Re: RFR: 8297878: KEM: Implementation [v7]
On Mon, 24 Apr 2023 20:50:23 GMT, Anthony Scarpino wrote: >> Is the size always `Npk`? It could be less (if small) or bigger (if MSB is >> 1). > > If I understand what your saying, if `Npk` is larger than `uArray`, then > `copyOf()` will pad with zeros at the end. I think that would change the > value. And I don't see why `Npk` would be smaller than `uArray` otherwise > this would be returning an invalid key In fact, since the byte array is reversed, it's actually OK to pad zeros at the end without changing the value. On the other hand, if the big integer's size is exact `Npk` but the most-significant-bit is 1, `toByteArray` will add an extra 00 at the beginning to keep it positive. In this case, the byte array length is `Npk + 1`. After reversed we can truncate the extra 00 byte now at the end. Fascinating, isn't it? - PR Review Comment: https://git.openjdk.org/jdk/pull/13256#discussion_r1175847108
Re: RFR: 8297878: KEM: Implementation [v7]
On Mon, 24 Apr 2023 19:36:53 GMT, Weijun Wang wrote:
>> src/java.base/share/classes/com/sun/crypto/provider/DHKEM.java line 202:
>>
>>> 200: } else {
>>> 201: byte[] uArray = ((XECPublicKey)
>>> k).getU().toByteArray();
>>> 202: return Arrays.copyOf(reverse(uArray), Npk);
>>
>> You could just return the reversed array. It is already a copy of the
>> BigInteger 'u'.
>
> Is the size always `Npk`? It could be less (if small) or bigger (if MSB is 1).
If I understand what your saying, if `Npk` is larger than `uArray`, then
`copyOf()` will pad with zeros at the end. I think that would change the
value. And I don't see why `Npk` would be smaller than `uArray` otherwise this
would be returning an invalid key
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13256#discussion_r1175769576
Re: RFR: 8297878: KEM: Implementation [v7]
On Mon, 24 Apr 2023 18:11:03 GMT, Anthony Scarpino
wrote:
>> Weijun Wang has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> fine tuning spec
>
> src/java.base/share/classes/com/sun/crypto/provider/DHKEM.java line 366:
>
>> 364: private static byte[] concat(byte[]... inputs) {
>> 365: ByteArrayOutputStream o = new ByteArrayOutputStream();
>> 366: Arrays.stream(inputs).forEach(o::writeBytes);
>
> Unless I'm missing something there is no `stream(byte[])` support, so I'm not
> sure how this is compiling. I didn't think the generics would work with
> this.
> More importantly, `forEach()` the API states that stream is in a
> non-deterministic order. I think you want `forEachOrdered()`
`inputs` is `byte[][]` and there is a `Arrays.stream(T[])`.
The above method says a `sequential Stream` is returned. Does that mean calling
`forEach` is safe?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13256#discussion_r1175723152
Re: RFR: 8297878: KEM: Implementation [v7]
On Mon, 24 Apr 2023 17:53:09 GMT, Anthony Scarpino
wrote:
>> Weijun Wang has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> fine tuning spec
>
> src/java.base/share/classes/com/sun/crypto/provider/DHKEM.java line 175:
>
>> 173: this.keyAlgorithm = keyAlgorithm;
>> 174: this.hkdfAlgorithm = hkdfAlgorithm;
>> 175: suiteId = concat("KEM".getBytes(StandardCharsets.UTF_8),
>
> This is a general comment for all the `getBytes()` calls. I think these
> should be static final values. Each one of these usages is allocating a new
> String and byte[] every time.
OK.
> src/java.base/share/classes/com/sun/crypto/provider/DHKEM.java line 346:
>
>> 344: private Params paramsFromKey(Key k) throws InvalidKeyException {
>> 345: if (k instanceof ECKey eckey) {
>> 346: if (ECUtil.equals(eckey.getParams(),
>> CurveDB.lookup("secp256r1"))) {
>
> These lookup calls look like they could be static final values
Maybe I can define the static final values inside `CurveDB`.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13256#discussion_r1175719339
PR Review Comment: https://git.openjdk.org/jdk/pull/13256#discussion_r1175720421
Re: RFR: 8297878: KEM: Implementation [v7]
On Fri, 21 Apr 2023 22:50:00 GMT, Anthony Scarpino
wrote:
>> Weijun Wang has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> fine tuning spec
>
> src/java.base/share/classes/com/sun/crypto/provider/DHKEM.java line 202:
>
>> 200: } else {
>> 201: byte[] uArray = ((XECPublicKey) k).getU().toByteArray();
>> 202: return Arrays.copyOf(reverse(uArray), Npk);
>
> You could just return the reversed array. It is already a copy of the
> BigInteger 'u'.
Is the size always `Npk`? It could be less (if small) or bigger (if MSB is 1).
> src/java.base/share/classes/com/sun/crypto/provider/DHKEM.java line 206:
>
>> 204: }
>> 205:
>> 206: private static byte[] reverse(byte[] arr) {
>
> It would be better to swap the bytes than allocating another array.
> DeserializePublicKey() may need to copy 'data' or have two different reverse
> methods
Yes, I can.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13256#discussion_r1175706882
PR Review Comment: https://git.openjdk.org/jdk/pull/13256#discussion_r1175707757
Re: RFR: 8297878: KEM: Implementation [v7]
On Tue, 18 Apr 2023 22:07:11 GMT, Weijun Wang wrote:
>> The KEM API and DHKEM impl. Note that this PR uses new methods in
>> https://github.com/openjdk/jdk/pull/13250.
>
> Weijun Wang has updated the pull request incrementally with one additional
> commit since the last revision:
>
> fine tuning spec
src/java.base/share/classes/com/sun/crypto/provider/DHKEM.java line 175:
> 173: this.keyAlgorithm = keyAlgorithm;
> 174: this.hkdfAlgorithm = hkdfAlgorithm;
> 175: suiteId = concat("KEM".getBytes(StandardCharsets.UTF_8),
This is a general comment for all the `getBytes()` calls. I think these should
be static final values. Each one of these usages is allocating a new String
and byte[] every time.
src/java.base/share/classes/com/sun/crypto/provider/DHKEM.java line 202:
> 200: } else {
> 201: byte[] uArray = ((XECPublicKey) k).getU().toByteArray();
> 202: return Arrays.copyOf(reverse(uArray), Npk);
You could just return the reversed array. It is already a copy of the
BigInteger 'u'.
src/java.base/share/classes/com/sun/crypto/provider/DHKEM.java line 206:
> 204: }
> 205:
> 206: private static byte[] reverse(byte[] arr) {
It would be better to swap the bytes than allocating another array.
DeserializePublicKey() may need to copy 'data' or have two different reverse
methods
src/java.base/share/classes/com/sun/crypto/provider/DHKEM.java line 346:
> 344: private Params paramsFromKey(Key k) throws InvalidKeyException {
> 345: if (k instanceof ECKey eckey) {
> 346: if (ECUtil.equals(eckey.getParams(),
> CurveDB.lookup("secp256r1"))) {
These lookup calls look like they could be static final values
src/java.base/share/classes/com/sun/crypto/provider/DHKEM.java line 366:
> 364: private static byte[] concat(byte[]... inputs) {
> 365: ByteArrayOutputStream o = new ByteArrayOutputStream();
> 366: Arrays.stream(inputs).forEach(o::writeBytes);
Unless I'm missing something there is no `stream(byte[])` support, so I'm not
sure how this is compiling. I didn't think the generics would work with this.
More importantly, `forEach()` the API states that stream is in a
non-deterministic order. I think you want `forEachOrdered()`
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13256#discussion_r1175614103
PR Review Comment: https://git.openjdk.org/jdk/pull/13256#discussion_r1174211748
PR Review Comment: https://git.openjdk.org/jdk/pull/13256#discussion_r1174224442
PR Review Comment: https://git.openjdk.org/jdk/pull/13256#discussion_r1175620516
PR Review Comment: https://git.openjdk.org/jdk/pull/13256#discussion_r1175630598
Re: RFR: 8297878: KEM: Implementation [v7]
> The KEM API and DHKEM impl. Note that this PR uses new methods in > https://github.com/openjdk/jdk/pull/13250. Weijun Wang has updated the pull request incrementally with one additional commit since the last revision: fine tuning spec - Changes: - all: https://git.openjdk.org/jdk/pull/13256/files - new: https://git.openjdk.org/jdk/pull/13256/files/fe6d73b3..d1a7f3e8 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=13256&range=06 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=13256&range=05-06 Stats: 85 lines in 2 files changed: 29 ins; 18 del; 38 mod Patch: https://git.openjdk.org/jdk/pull/13256.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13256/head:pull/13256 PR: https://git.openjdk.org/jdk/pull/13256
Re: RFR: 8297878: KEM: Implementation [v7]
On Fri, 14 Apr 2023 00:42:52 GMT, Weijun Wang wrote: >> src/java.base/share/classes/javax/crypto/KEM.java line 246: >> >>> 244: >>> 245: /** >>> 246: * The key encapsulation function. >> >> I think it would be useful to add a sentence explaining when this method >> would be useful. > > Then at the same time I should add a sentence to the usage of the other > method. However, it's a little difficult to describe it if KDF is not defined > yet. Added some words in the latest commit. - PR Review Comment: https://git.openjdk.org/jdk/pull/13256#discussion_r1170609614
Re: RFR: 8297878: KEM: Implementation [v5]
On Tue, 18 Apr 2023 16:32:49 GMT, Weijun Wang wrote: >> The KEM API and DHKEM impl. Note that this PR uses new methods in >> https://github.com/openjdk/jdk/pull/13250. > > Weijun Wang has updated the pull request incrementally with one additional > commit since the last revision: > > fine tune spec The latest commit only moves the `Encapsulator` class before `Decapsulator` class. No content update at all. - PR Comment: https://git.openjdk.org/jdk/pull/13256#issuecomment-1513535792
Re: RFR: 8297878: KEM: Implementation [v6]
> The KEM API and DHKEM impl. Note that this PR uses new methods in > https://github.com/openjdk/jdk/pull/13250. Weijun Wang has updated the pull request incrementally with one additional commit since the last revision: move Encapsulator before Decapsulator - Changes: - all: https://git.openjdk.org/jdk/pull/13256/files - new: https://git.openjdk.org/jdk/pull/13256/files/39c3a202..fe6d73b3 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=13256&range=05 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=13256&range=04-05 Stats: 114 lines in 1 file changed: 22 ins; 22 del; 70 mod Patch: https://git.openjdk.org/jdk/pull/13256.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13256/head:pull/13256 PR: https://git.openjdk.org/jdk/pull/13256
Re: RFR: 8297878: KEM: Implementation [v5]
> The KEM API and DHKEM impl. Note that this PR uses new methods in > https://github.com/openjdk/jdk/pull/13250. Weijun Wang has updated the pull request incrementally with one additional commit since the last revision: fine tune spec - Changes: - all: https://git.openjdk.org/jdk/pull/13256/files - new: https://git.openjdk.org/jdk/pull/13256/files/6455a521..39c3a202 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=13256&range=04 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=13256&range=03-04 Stats: 8 lines in 2 files changed: 4 ins; 0 del; 4 mod Patch: https://git.openjdk.org/jdk/pull/13256.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13256/head:pull/13256 PR: https://git.openjdk.org/jdk/pull/13256
Re: RFR: 8297878: KEM: Implementation [v2]
On Fri, 14 Apr 2023 14:21:05 GMT, Weijun Wang wrote:
>> src/java.base/share/classes/javax/crypto/KEM.java line 242:
>>
>>> 240: * shared secret as a key with algorithm being
>>> "Generic",
>>> 241: * the key encapsulation message, and optional
>>> parameters.
>>> 242: * The return value should not be {@code null}.
>>
>> "should" means it *could* still return null. I assume that is not what we
>> want. Although I would be more inclined to only specify cases where null may
>> be returned, and if it isn't mentioned, then it should be implied that null
>> is not a legal return value. If in doubt, perhaps check with Joe/CCC for
>> advice when you file the CSR.
>>
>> This general comment applies to the other return types in this API where you
>> say "not null". I think you can omit those.
>
> I do mean "must not be null". Maybe I can ask Joe directly.
I've removed the "not null" words for return values.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13256#discussion_r1170288201
Re: RFR: 8297878: KEM: Implementation [v4]
On Fri, 14 Apr 2023 01:59:59 GMT, Xue-Lei Andrew Fan wrote: >> I can rewrite this into something like "The caller of this method must >> validate..." so it becomes a requirement. We'll make sure the `KEM` class >> follows it. Any other class that wishes to call it directly must do it as >> well. > > You can make it a required part of the specification. But it is a > error-prone design. What happens if an application does not follow the spec? > It may be more complicated to handle the case. No more validation for providers. - PR Review Comment: https://git.openjdk.org/jdk/pull/13256#discussion_r1170289644
Re: RFR: 8297878: KEM: Implementation [v4]
On Fri, 14 Apr 2023 14:33:56 GMT, Weijun Wang wrote:
>> src/java.base/share/classes/javax/crypto/KEM.java line 94:
>>
>>> 92: * @see KEM#newEncapsulator(PublicKey, AlgorithmParameterSpec,
>>> SecureRandom)
>>> 93: */
>>> 94: public record Encapsulated(SecretKey key, byte[] encapsulation,
>>> byte[] params) {
>>
>> We need to decide if the encapsulation and params arrays should be
>> defensively cloned. I would lean towards cloning it since immutability is a
>> feature of this API, and I think it would be surprising if this type was
>> not.
>>
>> We can potentially switch to frozen arrays later.
>
> If we need to clone defensively I'll switch back to normal class, and then we
> can clone in both the constructor and the getters. IMO records are meant to
> be deadly simple.
Rewrite in plain normal class. Clone on both sides.
>> src/java.base/share/classes/javax/crypto/KEMSpi.java line 211:
>>
>>> 209: * The caller of this method has already validated the parameters
>>> to
>>> 210: * ensure that {@code pk} is not {@code null}. Therefore an
>>> implementation
>>> 211: * of this method does not to validate it.
>>
>> s/not to/not need to/
>>
>> Also, suggest saying who the caller is, "The caller (KEM.newEncapsulator) of
>> this method ..."
>
> Fixed.
>
> Not sure if the caller needs to be written out. `KEM.newEncapsulator` is the
> ultimate caller, but for delayed provider selection, the direct caller is in
> `Delayed`. Also, this can be considered as implementation detail. If I write
> out the name, then we cannot change.
>
> `engineEncapsulate` and `engineDecapsulate` also use "the caller of this
> method".
Removed.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13256#discussion_r1170288830
PR Review Comment: https://git.openjdk.org/jdk/pull/13256#discussion_r1170290779
Re: RFR: 8297878: KEM: Implementation [v3]
On Fri, 14 Apr 2023 16:54:49 GMT, Weijun Wang wrote: >> The KEM API and DHKEM impl. Note that this PR uses new methods in >> https://github.com/openjdk/jdk/pull/13250. > > Weijun Wang has updated the pull request incrementally with one additional > commit since the last revision: > > spec changes New commit pushed. The "the caller has validated" sentences are removed since that will be an implementation detail. Now each provider needs to validate the arguments themselves. Also, when a null key is provided to `newEncapsulator` and `newDecapsulator`, they throw an `InvalidKeyException` instead of a `NullPointerException` now. - PR Comment: https://git.openjdk.org/jdk/pull/13256#issuecomment-1513451629
Re: RFR: 8297878: KEM: Implementation [v4]
> The KEM API and DHKEM impl. Note that this PR uses new methods in > https://github.com/openjdk/jdk/pull/13250. Weijun Wang has updated the pull request incrementally with one additional commit since the last revision: let implementor validate arguments, null key throws IKE - Changes: - all: https://git.openjdk.org/jdk/pull/13256/files - new: https://git.openjdk.org/jdk/pull/13256/files/b398b03e..6455a521 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=13256&range=03 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=13256&range=02-03 Stats: 252 lines in 5 files changed: 51 ins; 180 del; 21 mod Patch: https://git.openjdk.org/jdk/pull/13256.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13256/head:pull/13256 PR: https://git.openjdk.org/jdk/pull/13256
Re: RFR: 8297878: KEM: Implementation [v2]
On Thu, 13 Apr 2023 22:29:34 GMT, Weijun Wang wrote:
>> The KEM API and DHKEM impl. Note that this PR uses new methods in
>> https://github.com/openjdk/jdk/pull/13250.
>
> Weijun Wang has updated the pull request incrementally with one additional
> commit since the last revision:
>
> spec change, getAlgorithm
src/java.base/share/classes/javax/crypto/KEM.java line 78:
> 76: *assert Arrays.equals(k1.getEncoded(), k2.getEncoded());
> 77: * }
> 78: */
Missing an `@since 21`.
src/java.base/share/classes/javax/crypto/KEMSpi.java line 45:
> 43: * {@code AlgorithmParameterSpec} argument that is specified when creating
> 44: * an encapsulator or decapsulator. The result of calling
> 45: * {@link #engineNewDecapsulator} or {@link #engineNewDecapsulator} must
> be of
First one should be engineNewEncapsulator.
Also, maybe say "... must return an encapsulator or decapsulator that maps to a
single configuration, ..."
src/java.base/share/classes/javax/crypto/KEMSpi.java line 47:
> 45: * {@link #engineNewDecapsulator} or {@link #engineNewDecapsulator} must
> be of
> 46: * a single configuration, where its {@link
> EncapsulatorSpi#engineSecretSize()}
> 47: * and {@link EncapsulatorSpi#engineEncapsulationSize()} are constants.
Maybe change "are constants" to "methods return constant values."
src/java.base/share/classes/javax/crypto/KEMSpi.java line 54:
> 52: *
> 53: * {@code EncapsulatorSpi} and {@code DecapsulatorSpi} implementations
> must
> 54: * be immutable. It must be safe to invoke multiple {@code encapsulate}
> and
suggest "... must also be immutable."
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13256#discussion_r1166869294
PR Review Comment: https://git.openjdk.org/jdk/pull/13256#discussion_r1166871954
PR Review Comment: https://git.openjdk.org/jdk/pull/13256#discussion_r1166885398
PR Review Comment: https://git.openjdk.org/jdk/pull/13256#discussion_r1166887194
Re: RFR: 8297878: KEM: Implementation [v3]
> The KEM API and DHKEM impl. Note that this PR uses new methods in > https://github.com/openjdk/jdk/pull/13250. Weijun Wang has updated the pull request incrementally with one additional commit since the last revision: spec changes - Changes: - all: https://git.openjdk.org/jdk/pull/13256/files - new: https://git.openjdk.org/jdk/pull/13256/files/8a227daf..b398b03e Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=13256&range=02 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=13256&range=01-02 Stats: 169 lines in 4 files changed: 106 ins; 19 del; 44 mod Patch: https://git.openjdk.org/jdk/pull/13256.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13256/head:pull/13256 PR: https://git.openjdk.org/jdk/pull/13256
Re: RFR: 8297878: KEM: Implementation [v2]
On Fri, 14 Apr 2023 16:23:03 GMT, Weijun Wang wrote: >>> 2. Do validate parameters on your own (because no one else does) >>> >> I would say: validate the parameters on your own, because not everyone else >> does. > > I added another proposal in you PR at > https://github.com/openjdk/jdk/pull/13470#issuecomment-1508915798. > I added another proposal in you PR at [#13470 > (comment)](https://github.com/openjdk/jdk/pull/13470#issuecomment-1508915798). I like the idea to use abstract class. The concerns about provider() method get addressed. - PR Review Comment: https://git.openjdk.org/jdk/pull/13256#discussion_r1167054730
Re: RFR: 8297878: KEM: Implementation [v2]
On Fri, 14 Apr 2023 15:42:55 GMT, Xue-Lei Andrew Fan wrote: >> I see. So the security providers are told: >> 1. Don't implement `provider()` (If you do, we won't look at it) >> 2. Do validate parameters on your own (because no one else does) >> >> Let me think about it. I can even ask a security provider what their opinion >> is. > >> 2. Do validate parameters on your own (because no one else does) >> > I would say: validate the parameters on your own, because not everyone else > does. I added another proposal in you PR at https://github.com/openjdk/jdk/pull/13470#issuecomment-1508915798. - PR Review Comment: https://git.openjdk.org/jdk/pull/13256#discussion_r1167045135
Re: RFR: 8297878: KEM: Implementation [v2]
On Fri, 14 Apr 2023 15:00:51 GMT, Weijun Wang wrote: > 2. Do validate parameters on your own (because no one else does) > I would say: validate the parameters on your own, because not everyone else does. - PR Review Comment: https://git.openjdk.org/jdk/pull/13256#discussion_r1166995710
Re: RFR: 8297878: KEM: Implementation [v2]
On Thu, 13 Apr 2023 21:43:24 GMT, Xue-Lei Andrew Fan wrote:
>>> Currently, `provider()` is a method of `KEM.Encapsulator`. If `KEMSpi.
>>> newEncapsulator` also returns this interface, then what value should its
>>> `provider()` method return? This is what I meant registering itself to a
>>> provider.
>>>
>>> When I said different instances, I was asking
>>>
>>> ```
>>> var k = KEM.getInstance("DHKEM", p);
>>> var e = k.newEncapsulator(pk);
>>> // now, is p == e.provider()?
>>> ```
>>>
>>> Or, are you suggesting we should define `provider()` somewhere else? It's
>>> possible, but I have difficulty making every class immutable.
>>
>> If the provider() method in KEM.Encapsulator is the only reason, the cost to
>> support it may be too high with so many duplicated/similar
>> specifications/names and code.
>>
>> Option 1: Remove the KEM.Encapsulator.provider() method, and provide no
>> access to the underlying provider object.
>>
>>> do you expect it to return new SunJCE()? This means the p in
>>> getInstance("DHKEM", p) will be a different instance from the value
>>> returned by getProvider().
>>
>> The Provider class is mutable, we may not want to change the provider object
>> asked for "DHKEM". I think you have used a solution to pass the provider
>> object in the KEM.java implementation currently. Maybe, it could be twitted
>> a little bit so that the provider can be passed to a delegated
>> KM.Encapsulator interface implementation.
>>
>> Option 2:
>>
>> public final class KEM {
>> interface Encapsulator {
>> ...
>> KEM.Encapsulated encapsulate(...);
>> ...
>>
>> default Provider provider() {
>> return null;
>> }
>> }
>>
>> private static class DelegatedEncapsulator implements Encapsulator {
>> private final Provider p;
>> private DelegatedEncapsulator(Encapsulator e, Provider p) {
>> this.p = p;
>> ...
>> }
>> public Provider provider() {
>> return this.p;
>> }
>> }
>>
>> ...
>> KEMSpi spi = (KEMSpi) service.newInstance(null);
>> return new DelegatedEncapsulator(
>>spi.engineNewEncapsulator(pk, spec, secureRandom),
>> // This is the interface implementation, use the same provider as KEM.
>> service.getProvider());// This is the provider passed to
>> the delegated KEM.Encapsulator object.
>> ...
>> }
>
> For more details about option 2, please refer to
> https://github.com/openjdk/jdk/pull/13470/files. The KEM.java and
> KEMSpi.java is pretty much the same except the clean up of En/Decapsulator(s)
> in this PR.
I see. So the security providers are told:
1. Don't implement `provider()` (If you do, we won't look at it)
2. Do validate parameters on your own (because no one else does)
Let me think about it. I can even ask a security provider what their opinion is.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13256#discussion_r1166948493
Re: RFR: 8297878: KEM: Implementation
Some interesting side discussion here. I wanted to chime in to point out that HPKE is built upon the “primitives” of KEM and HKDF. As mentioned on the list, KEM is underway. I am also spearheading our effort reviving the HKDF JEP<https://bugs.openjdk.org/browse/JDK-8189808> which has gone a bit stale. HPKE is certainly something we’re looking into as well. Once the building blocks of KEM and HKDF are in place, HPKE will ramp up next. Kevin Driver Mobile: +1.512.431.5690 Java Security Libraries Subject: Re: RFR: 8297878: KEM: Implementation Date: Thu, 13 Apr 2023 21:31:43 +0100 From: Stephen Farrell To: Xue-Lei Andrew Fan , security-dev@openjdk.org Hi, Apologies for the interruption from the sidelines but I have a query if that's ok. Is there any relationship between this work and RFC1980 which defines HPKE, being a way of encrypting to a public value using a KEM? Reason to ask is HPKE is a mechanism that'll be needed for TLS Encrypted Client Hello and the MLS protocol, so it'd be a fine thing if these additions were suitable for that too. Cheers, S. PS: I implemented HPKE for OpenSSL so if there's interest in supporting that here too, I'd be happy to help a bit. OpenPGP_0xE4D8E9F997A833DD.asc Description: OpenPGP_0xE4D8E9F997A833DD.asc
Re: RFR: 8297878: KEM: Implementation [v2]
On Thu, 13 Apr 2023 21:20:51 GMT, Sean Mullan wrote:
>> Weijun Wang has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> spec change, getAlgorithm
>
> src/java.base/share/classes/javax/crypto/KEM.java line 94:
>
>> 92: * @see KEM#newEncapsulator(PublicKey, AlgorithmParameterSpec,
>> SecureRandom)
>> 93: */
>> 94: public record Encapsulated(SecretKey key, byte[] encapsulation,
>> byte[] params) {
>
> We need to decide if the encapsulation and params arrays should be
> defensively cloned. I would lean towards cloning it since immutability is a
> feature of this API, and I think it would be surprising if this type was not.
>
> We can potentially switch to frozen arrays later.
If we need to clone defensively I'll switch back to normal class, and then we
can clone in both the constructor and the getters. IMO records are meant to be
deadly simple.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13256#discussion_r1166916179
Re: RFR: 8297878: KEM: Implementation [v2]
On Fri, 14 Apr 2023 13:32:43 GMT, Sean Mullan wrote:
>> Weijun Wang has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> spec change, getAlgorithm
>
> src/java.base/share/classes/javax/crypto/KEM.java line 242:
>
>> 240: * shared secret as a key with algorithm being
>> "Generic",
>> 241: * the key encapsulation message, and optional
>> parameters.
>> 242: * The return value should not be {@code null}.
>
> "should" means it *could* still return null. I assume that is not what we
> want. Although I would be more inclined to only specify cases where null may
> be returned, and if it isn't mentioned, then it should be implied that null
> is not a legal return value. If in doubt, perhaps check with Joe/CCC for
> advice when you file the CSR.
>
> This general comment applies to the other return types in this API where you
> say "not null". I think you can omit those.
I do mean "must not be null". Maybe I can ask Joe directly.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13256#discussion_r1166901173
Re: RFR: 8297878: KEM: Implementation
On Fri, 14 Apr 2023 08:18:16 GMT, Stephen Farrell wrote: > Grand. I'll get out of the way of this thread so:-) But > again, if interested, do reach out, as I'm keen to see ECH > support ending up widespread and HPKE is a fine precursor > for that. Thanks a lot. I'll remember that. - PR Comment: https://git.openjdk.org/jdk/pull/13256#issuecomment-1508613565
Re: RFR: 8297878: KEM: Implementation [v2]
On Thu, 13 Apr 2023 22:29:34 GMT, Weijun Wang wrote:
>> The KEM API and DHKEM impl. Note that this PR uses new methods in
>> https://github.com/openjdk/jdk/pull/13250.
>
> Weijun Wang has updated the pull request incrementally with one additional
> commit since the last revision:
>
> spec change, getAlgorithm
src/java.base/share/classes/javax/crypto/KEM.java line 49:
> 47: * If a provider is not specified in the {@code getInstance} method when
> 48: * instantiating a {@code KEM} object, the {@code newEncapsulator} and
> 49: * {@code newDecapsulator} method may return encapsulators or
> decapsulators
s/method/methods/
src/java.base/share/classes/javax/crypto/KEM.java line 51:
> 49: * {@code newDecapsulator} method may return encapsulators or
> decapsulators
> 50: * from different providers. The provider selected is based on the
> parameters
> 51: * passed to the the {@code newEncapsulator} or {@code newDecapsulator}
> methods:
s/the the/the/
src/java.base/share/classes/javax/crypto/KEM.java line 54:
> 52: * the private or public key and the optional {@code
> AlgorithmParameterSpec}.
> 53: * The {@link Encapsulator#provider} and {@link Decapsulator#provider}
> methods
> 54: * return the selected provider."
remove "".
src/java.base/share/classes/javax/crypto/KEM.java line 103:
> 101: * @param key the shared secret as a key, must not be {@code
> null}.
> 102: * @param encapsulation the key encapsulation message, must not
> be {@code null}.
> 103: * @param params additional parameters, can be {@code null}.
Nit: remove periods.
src/java.base/share/classes/javax/crypto/KEM.java line 105:
> 103: * @param params additional parameters, can be {@code null}.
> 104: */
> 105: public Encapsulated {
Should this also have an `@throws NullPointerException` clause?
src/java.base/share/classes/javax/crypto/KEM.java line 128:
> 126: * Returns the provider.
> 127: *
> 128: * @return thr provider
s/thr/the/
src/java.base/share/classes/javax/crypto/KEM.java line 143:
> 141: * @param encapsulation the key encapsulation message from the
> sender
> 142: * @return the shared secret as a {@code SecretKey} with
> 143: * algorithm being "Generic", not {@code null}
See my comment below, I don't think you need to specify that this does not
return null, it should be implied.
src/java.base/share/classes/javax/crypto/KEM.java line 160:
> 158: * @param algorithm the algorithm for the secret key returned
> 159: * @return a portion of the shared secret as a {@code SecretKey}
> with
> 160: * the specified algorithm, not {@code null}
Same comment as above.
src/java.base/share/classes/javax/crypto/KEM.java line 242:
> 240: * shared secret as a key with algorithm being
> "Generic",
> 241: * the key encapsulation message, and optional
> parameters.
> 242: * The return value should not be {@code null}.
"should" means it *could* still return null. I assume that is not what we want.
Although I would be more inclined to only specify cases where null may be
returned, and if it isn't mentioned, then it should be implied that null is not
a legal return value. If in doubt, perhaps check with Joe/CCC for advice when
you file the CSR.
This general comment applies to the other return types in this API where you
say "not null". I think you can omit those.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13256#discussion_r1166801682
PR Review Comment: https://git.openjdk.org/jdk/pull/13256#discussion_r1166802062
PR Review Comment: https://git.openjdk.org/jdk/pull/13256#discussion_r1166802596
PR Review Comment: https://git.openjdk.org/jdk/pull/13256#discussion_r1166808093
PR Review Comment: https://git.openjdk.org/jdk/pull/13256#discussion_r1166811613
PR Review Comment: https://git.openjdk.org/jdk/pull/13256#discussion_r1166826429
PR Review Comment: https://git.openjdk.org/jdk/pull/13256#discussion_r1166829885
PR Review Comment: https://git.openjdk.org/jdk/pull/13256#discussion_r1166830271
PR Review Comment: https://git.openjdk.org/jdk/pull/13256#discussion_r1166841781
Re: RFR: 8297878: KEM: Implementation [v2]
On Thu, 13 Apr 2023 22:36:04 GMT, Weijun Wang wrote:
>> src/java.base/share/classes/javax/crypto/KEMSpi.java line 119:
>>
>>> 117: * of {@code from} and {@code to} are within the correct range.
>>> 118: * Therefore an implementation of this method does not need to
>>> 119: * validate them.
>>
>> The KEM caller does validate the parameters, but the caller may be more
>> widely other than the KEM. Then, the statement here could be wrong at that
>> time.
>
> I can rewrite this into something like "The caller of this method must
> validate..." so it becomes a requirement. We'll make sure the `KEM` class
> follows it. Any other class that wishes to call it directly must do it as
> well.
You can make it a required part of the specification. But it is a error-prone
design.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13256#discussion_r1166196969
Re: RFR: 8297878: KEM: Implementation [v2]
On Thu, 13 Apr 2023 20:59:17 GMT, Sean Mullan wrote: >> Weijun Wang has updated the pull request incrementally with one additional >> commit since the last revision: >> >> spec change, getAlgorithm > > src/java.base/share/classes/javax/crypto/KEM.java line 168: > >> 166: * is not supported by the decapsulator >> 167: */ >> 168: public SecretKey decapsulate(byte[] encapsulation, > > WDYT about throwing IllegalArgumentException if the size of encapsulation is > not equal to encapsulationSize()? > > If not, I think you should add a sentence to the @param encapsulation such as > "The size must be equal to ..., or a DecapsulateException will be thrown." I'll choose the latter. - PR Review Comment: https://git.openjdk.org/jdk/pull/13256#discussion_r1166163967
Re: RFR: 8297878: KEM: Implementation [v2]
On Thu, 13 Apr 2023 20:35:23 GMT, Sean Mullan wrote: >> Weijun Wang has updated the pull request incrementally with one additional >> commit since the last revision: >> >> spec change, getAlgorithm > > src/java.base/share/classes/javax/crypto/KEM.java line 246: > >> 244: >> 245: /** >> 246: * The key encapsulation function. > > I think it would be useful to add a sentence explaining when this method > would be useful. Then at the same time I should add a sentence to the usage of the other method. However, it's a little difficult to describe it if KDF is not defined yet. - PR Review Comment: https://git.openjdk.org/jdk/pull/13256#discussion_r1166160581
Re: RFR: 8297878: KEM: Implementation [v2]
Hiya, On 13/04/2023 23:35, Weijun Wang wrote: Apologies for the interruption from the sidelines but I have a query if that's ok. Is there any relationship between this work and RFC1980 which defines HPKE, being a way of encrypting to a public value using a KEM? We know about HPKE, Of course:-) and it can makes use of the DHKEM implementation here (if the AuthEncap/AuthDecap functions are not used). FWIW, I'm not aware of any protocol yet attempting to make use of the authenticated HPKE modes, so that seems very reasonable. (OTOH, it's not that hard for a library to support all modes, so it may be worth some consideration.) However, we (Oracle's Java SE Security Team) don't have a plan to include HPKE inside OpenJDK yet. Entirely fair. If doing so is of interest (to you or others), I'd be happy to try help. (Ping me on/off-list if that is of interest.) This PR is mainly about adding the KEM SPI so 3rd security providers can implement other KEM algorithms. DHKEM is included mainly to prove that the API is usable. Grand. I'll get out of the way of this thread so:-) But again, if interested, do reach out, as I'm keen to see ECH support ending up widespread and HPKE is a fine precursor for that. Cheers, S. OpenPGP_0xE4D8E9F997A833DD.asc Description: OpenPGP public key OpenPGP_signature Description: OpenPGP digital signature
Re: RFR: 8297878: KEM: Implementation [v2]
On Thu, 13 Apr 2023 21:46:22 GMT, Xue-Lei Andrew Fan wrote:
>> Weijun Wang has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> spec change, getAlgorithm
>
> src/java.base/share/classes/javax/crypto/KEMSpi.java line 119:
>
>> 117: * of {@code from} and {@code to} are within the correct range.
>> 118: * Therefore an implementation of this method does not need to
>> 119: * validate them.
>
> The KEM caller does validate the parameters, but the caller may be more
> widely other than the KEM. Then, the statement here could be wrong at that
> time.
I can rewrite this into something like "The caller of this method must
validate..." so it becomes a requirement. We'll make sure the `KEM` class
follows it. Any other class that wishes to call it directly must do it as well.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13256#discussion_r1166090688
Re: RFR: 8297878: KEM: Implementation [v2]
On Thu, 13 Apr 2023 22:29:34 GMT, Weijun Wang wrote: >> The KEM API and DHKEM impl. Note that this PR uses new methods in >> https://github.com/openjdk/jdk/pull/13250. > > Weijun Wang has updated the pull request incrementally with one additional > commit since the last revision: > > spec change, getAlgorithm > _Mailing list message from [Stephen > Farrell](mailto:stephen.farr...@cs.tcd.ie) on > [security-dev](mailto:security-...@mail.openjdk.org):_ > > Hi, > > Apologies for the interruption from the sidelines but I have a query if > that's ok. > > Is there any relationship between this work and RFC1980 which defines HPKE, > being a way of encrypting to a public value using a KEM? We know about HPKE, and it can makes use of the DHKEM implementation here (if the AuthEncap/AuthDecap functions are not used). However, we (Oracle's Java SE Security Team) don't have a plan to include HPKE inside OpenJDK yet. This PR is mainly about adding the KEM SPI so 3rd security providers can implement other KEM algorithms. DHKEM is included mainly to prove that the API is usable. - PR Comment: https://git.openjdk.org/jdk/pull/13256#issuecomment-1507688812
Re: RFR: 8297878: KEM: Implementation [v2]
> The KEM API and DHKEM impl. Note that this PR uses new methods in > https://github.com/openjdk/jdk/pull/13250. Weijun Wang has updated the pull request incrementally with one additional commit since the last revision: spec change, getAlgorithm - Changes: - all: https://git.openjdk.org/jdk/pull/13256/files - new: https://git.openjdk.org/jdk/pull/13256/files/e953d9ec..8a227daf Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=13256&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=13256&range=00-01 Stats: 72 lines in 3 files changed: 20 ins; 3 del; 49 mod Patch: https://git.openjdk.org/jdk/pull/13256.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13256/head:pull/13256 PR: https://git.openjdk.org/jdk/pull/13256
Re: RFR: 8297878: KEM: Implementation
On Fri, 31 Mar 2023 02:25:04 GMT, Weijun Wang wrote:
> The KEM API and DHKEM impl. Note that this PR uses new methods in
> https://github.com/openjdk/jdk/pull/13250.
src/java.base/share/classes/javax/crypto/KEM.java line 79:
> 77:
> 78: /**
> 79: * Type for the return value of the {@link Encapsulator#encapsulate}
> method.
I think the description should more generally describe what this is, maybe "The
encapsulated data generated by a Key Encapsulation Mechanism (KEM), which
includes the shared secret (as a SecretKey), the key encapsulation message, and
additional optional parameters."
src/java.base/share/classes/javax/crypto/KEM.java line 94:
> 92: * @see KEM#newEncapsulator(PublicKey, AlgorithmParameterSpec,
> SecureRandom)
> 93: */
> 94: public record Encapsulated(SecretKey key, byte[] encapsulation,
> byte[] params) {
We need to decide if the encapsulation array should be defensively cloned. I
would lean towards cloning it since immutability is a feature of this API, and
I think it would be surprising if this type was not.
We can potentially switch to frozen arrays later.
src/java.base/share/classes/javax/crypto/KEM.java line 109:
> 107:
> 108: /**
> 109: * A decapsulator, generated by {@link #newDecapsulator}.
See comments on Encapsulator about adding more description here.
src/java.base/share/classes/javax/crypto/KEM.java line 140:
> 138: * @param encapsulation the key encapsulation message from the
> sender
> 139: * @return the shared secret as a {@code SecretKey} with
> 140: * algorithm being "Generic", not {@code null}
suggest "with an algorithm name of "Generic" ..."
src/java.base/share/classes/javax/crypto/KEM.java line 150:
> 148:
> 149: /**
> 150: * The key decapsulation function.
See encapsulate for similar comments.
src/java.base/share/classes/javax/crypto/KEM.java line 155:
> 153: * @param from the initial index of the shared secret to be
> returned, inclusive
> 154: * @param to the final index of the shared secret to be
> returned, exclusive.
> 155: * @param algorithm the algorithm for the secret key returned
Suggest: " ... algorithm name of the secret key that is returned" (or generated)
src/java.base/share/classes/javax/crypto/KEM.java line 168:
> 166: * is not supported by the decapsulator
> 167: */
> 168: public SecretKey decapsulate(byte[] encapsulation,
WDYT about throwing IllegalArgumentException if the size of encapsulation is
not equal to encapsulationSize()?
If not, I think you should add a sentence to the @param encapsulation such as
"The size must be equal to ..., or a DecapsulateException will be thrown."
src/java.base/share/classes/javax/crypto/KEM.java line 193:
> 191:
> 192: /**
> 193: * Returns the size of the key encapsulation message.
I think both the `secretSize` and `encapsulationSize` methods could use a
sentence or two explaining why they are useful. This will help both users
understand when they might need to call them and implementors to implement
them correctly.
src/java.base/share/classes/javax/crypto/KEM.java line 206:
> 204:
> 205: /**
> 206: * An encapsulator, generated by {@link #newEncapsulator}.
Can you add more description here? "An encapsulator, generated by {@link
#newEncapsulator}. This class represents the key encapsulation function of a
KEM. Each invocation of the {@link encapsulate} method generates a new secret
key and key encapsulation message that is returned in an {@link Encapsulated}
object."
src/java.base/share/classes/javax/crypto/KEM.java line 223:
> 221: * Returns the provider.
> 222: *
> 223: * @return thr provider
s/thr/the/
src/java.base/share/classes/javax/crypto/KEM.java line 230:
> 228:
> 229: /**
> 230: * The key encapsulation function.
I think you should include this text in both encapsulate methods: "Each
invocation of this method generates a new secret key and key encapsulation
message that is returned in an {@link Encapsulated} object."
src/java.base/share/classes/javax/crypto/KEM.java line 238:
> 236: * @return an {@link KEM.Encapsulated} object containing the full
> 237: * shared secret as a key with algorithm being
> "Generic",
> 238: * the key encapsulation message, and optional
> parameters.
Maybe break up into two sentences: "an {@link KEM.Encapsulated} object
containing the shared secret, key encapsulation message, and optional
parameters. The shared secret is a {@code SecretKey} containing all of the
bytes of the secret, and an algorithm name of "Generic".
src/java.base/share/classes/javax/crypto/KEM.java line 246:
> 244:
> 245: /**
> 246: * The key encapsulation function.
I think it would be useful to add a sentence explaining when this method would
be useful.
src/jav
Re: RFR: 8297878: KEM: Implementation
On Fri, 31 Mar 2023 02:25:04 GMT, Weijun Wang wrote:
> The KEM API and DHKEM impl. Note that this PR uses new methods in
> https://github.com/openjdk/jdk/pull/13250.
Changes requested by xuelei (Reviewer).
src/java.base/share/classes/javax/crypto/KEMSpi.java line 119:
> 117: * of {@code from} and {@code to} are within the correct range.
> 118: * Therefore an implementation of this method does not need to
> 119: * validate them.
The KEM caller does validate the parameters, but the caller may be more widely
other than the KEM. Then, the statement here could be wrong at that time.
src/java.base/share/classes/javax/crypto/KEMSpi.java line 172:
> 170: * within the correct range. Therefore an implementation of this
> method
> 171: * does not to validate them.
> 172: *
Same comment as above.
src/java.base/share/classes/javax/crypto/KEMSpi.java line 211:
> 209: * The caller of this method has already validated the parameters to
> 210: * ensure that {@code pk} is not {@code null}. Therefore an
> implementation
> 211: * of this method does not to validate it.
Same as above, the caller may not validate the parameters yet. For example,
the instance could be accessed just like what you do in the KEM implementation
manner (use Provider method and Service look-up APIs), but without validating
the parameters as you did.
-
PR Review: https://git.openjdk.org/jdk/pull/13256#pullrequestreview-1384268215
PR Review Comment: https://git.openjdk.org/jdk/pull/13256#discussion_r1166059684
PR Review Comment: https://git.openjdk.org/jdk/pull/13256#discussion_r1166059934
PR Review Comment: https://git.openjdk.org/jdk/pull/13256#discussion_r1166061964
Re: RFR: 8297878: KEM: Implementation
On Thu, 13 Apr 2023 19:01:24 GMT, Xue-Lei Andrew Fan wrote:
>> Currently, `provider()` is a method of `KEM.Encapsulator`. If `KEMSpi.
>> newEncapsulator` also returns this interface, then what value should its
>> `provider()` method return? This is what I meant registering itself to a
>> provider.
>>
>> When I said different instances, I was asking
>>
>> var k = KEM.getInstance("DHKEM", p);
>> var e = k.newEncapsulator(pk);
>> // now, is p == e.provider()?
>>
>>
>> Or, are you suggesting we should define `provider()` somewhere else? It's
>> possible, but I have difficulty making every class immutable.
>
>> Currently, `provider()` is a method of `KEM.Encapsulator`. If `KEMSpi.
>> newEncapsulator` also returns this interface, then what value should its
>> `provider()` method return? This is what I meant registering itself to a
>> provider.
>>
>> When I said different instances, I was asking
>>
>> ```
>> var k = KEM.getInstance("DHKEM", p);
>> var e = k.newEncapsulator(pk);
>> // now, is p == e.provider()?
>> ```
>>
>> Or, are you suggesting we should define `provider()` somewhere else? It's
>> possible, but I have difficulty making every class immutable.
>
> If the provider() method in KEM.Encapsulator is the only reason, the cost to
> support it may be too high with so many duplicated/similar
> specifications/names and code.
>
> Option 1: Remove the KEM.Encapsulator.provider() method, and provide no
> access to the underlying provider object.
>
>> do you expect it to return new SunJCE()? This means the p in
>> getInstance("DHKEM", p) will be a different instance from the value returned
>> by getProvider().
>
> The Provider class is mutable, we may not want to change the provider object
> asked for "DHKEM". I think you have used a solution to pass the provider
> object in the KEM.java implementation currently. Maybe, it could be twitted
> a little bit so that the provider can be passed to a delegated
> KM.Encapsulator interface implementation.
>
> Option 2:
>
> public final class KEM {
> interface Encapsulator {
> ...
> KEM.Encapsulated encapsulate(...);
> ...
>
> default Provider provider() {
> return null;
> }
> }
>
> private static class DelegatedEncapsulator implements Encapsulator {
> private final Provider p;
> private DelegatedEncapsulator(Encapsulator e, Provider p) {
> this.p = p;
> ...
> }
> public Provider provider() {
> return this.p;
> }
> }
>
> ...
> KEMSpi spi = (KEMSpi) service.newInstance(null);
> return new DelegatedEncapsulator(
>spi.engineNewEncapsulator(pk, spec, secureRandom), //
> This is the interface implementation, use the same provider as KEM.
> service.getProvider());// This is the provider passed to
> the delegated KEM.Encapsulator object.
> ...
> }
For more details about option 2, please refer to
https://github.com/openjdk/jdk/pull/13470/files. The KEM.java and KEMSpi.java
is pretty much the same except the clean up of En/Decapsulator(s) in this PR.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13256#discussion_r1166057562
Re: RFR: 8297878: KEM: Implementation
Hi, Apologies for the interruption from the sidelines but I have a query if that's ok. Is there any relationship between this work and RFC1980 which defines HPKE, being a way of encrypting to a public value using a KEM? Reason to ask is HPKE is a mechanism that'll be needed for TLS Encrypted Client Hello and the MLS protocol, so it'd be a fine thing if these additions were suitable for that too. Cheers, S. PS: I implemented HPKE for OpenSSL so if there's interest in supporting that here too, I'd be happy to help a bit. OpenPGP_0xE4D8E9F997A833DD.asc Description: OpenPGP public key OpenPGP_signature Description: OpenPGP digital signature
Re: RFR: 8297878: KEM: Implementation
On Thu, 13 Apr 2023 17:54:22 GMT, Weijun Wang wrote:
> Currently, `provider()` is a method of `KEM.Encapsulator`. If `KEMSpi.
> newEncapsulator` also returns this interface, then what value should its
> `provider()` method return? This is what I meant registering itself to a
> provider.
>
> When I said different instances, I was asking
>
> ```
> var k = KEM.getInstance("DHKEM", p);
> var e = k.newEncapsulator(pk);
> // now, is p == e.provider()?
> ```
>
> Or, are you suggesting we should define `provider()` somewhere else? It's
> possible, but I have difficulty making every class immutable.
If the provider() method in KEM.Encapsulator is the only reason, the cost to
support it may be too high with so many duplicated/similar specifications/names
and code.
Option 1: Remove the KEM.Encapsulator.provider() method, and provide no access
to the underlying provider object.
> do you expect it to return new SunJCE()? This means the p in
> getInstance("DHKEM", p) will be a different instance from the value returned
> by getProvider().
The Provider class is mutable, we may not want to change the provider object
asked for "DHKEM". I think you have used a solution to pass the provider
object in the KEM.java implementation currently. Maybe, it could be twitted a
little bit so that the provider can be passed to a delegated KM.Encapsulator
interface implementation.
Option 2:
public final class KEM {
interface Encapsulator {
...
KEM.Encapsulated encapsulate(...);
...
default Provider provider() {
return null;
}
}
private static class DelegatedEncapsulator implements Encapsulator {
private final Provider p;
private DelegatedEncapsulator(Encapsulator e, Provider p) {
this.p = p;
...
}
public Provider provider() {
return this.p;
}
}
...
KEMSpi spi = (KEMSpi) service.newInstance(null);
return new DelegatedEncapsulator(
spi.engineNewEncapsulator(pk, spec, secureRandom), //
This is the interface implementation, use the same provider as KEM.
service.getProvider());// This is the provider passed to
the delegated KEM.Encapsulator object.
...
}
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13256#discussion_r1165920458
Re: RFR: 8297878: KEM: Implementation
On Thu, 13 Apr 2023 18:21:51 GMT, Sean Mullan wrote:
>> The KEM API and DHKEM impl. Note that this PR uses new methods in
>> https://github.com/openjdk/jdk/pull/13250.
>
> src/java.base/share/classes/javax/crypto/KEM.java line 519:
>
>> 517: * @param pk the receiver's public key, must not be {@code null}
>> 518: * @param secureRandom the source of randomness for encapsulation,
>> 519: * If {@code null}, the implementation should
>> provide
>
> s/should/must/
This spec is not written for the implementor. How about "If {@code} null, a
default one from the implementation will be used.".
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13256#discussion_r1165908210
Re: RFR: 8297878: KEM: Implementation
On Fri, 31 Mar 2023 02:25:04 GMT, Weijun Wang wrote:
> The KEM API and DHKEM impl. Note that this PR uses new methods in
> https://github.com/openjdk/jdk/pull/13250.
src/java.base/share/classes/javax/crypto/KEM.java line 493:
> 491: * Creates a KEM encapsulator.
> 492: *
> 493: * The method is equivalent to {@code newEncapsulator(pk, null,
> null)}.
s/The/This/
src/java.base/share/classes/javax/crypto/KEM.java line 500:
> 498: * @throws NullPointerException if {@code pk} is {@code null}
> 499: * @throws UnsupportedOperationException if this method is not
> supported
> 500: * and a {@code AlgorithmParameterSpec} must be provided
I would say "... because an {@code AlgorithmParameterSpec} must be provided".
src/java.base/share/classes/javax/crypto/KEM.java line 515:
> 513: * Creates a KEM encapsulator.
> 514: *
> 515: * The method is equivalent to {@code newEncapsulator(pk, null,
> secureRandom)}.
s/The/This/
src/java.base/share/classes/javax/crypto/KEM.java line 518:
> 516: *
> 517: * @param pk the receiver's public key, must not be {@code null}
> 518: * @param secureRandom the source of randomness for encapsulation,
s/,/./
src/java.base/share/classes/javax/crypto/KEM.java line 519:
> 517: * @param pk the receiver's public key, must not be {@code null}
> 518: * @param secureRandom the source of randomness for encapsulation,
> 519: * If {@code null}, the implementation should
> provide
s/should/must/
src/java.base/share/classes/javax/crypto/KEM.java line 556:
> 554: * @param pk the receiver's public key, must not be {@code null}
> 555: * @param spec the optional parameter, can be {@code null}
> 556: * @param secureRandom the source of randomness for encapsulation,
s/,/./
src/java.base/share/classes/javax/crypto/KEM.java line 557:
> 555: * @param spec the optional parameter, can be {@code null}
> 556: * @param secureRandom the source of randomness for encapsulation,
> 557: * If {@code null}, the implementation should
> provide
s/should/must/
src/java.base/share/classes/javax/crypto/KEM.java line 577:
> 575: * Creates a KEM decapsulator.
> 576: *
> 577: * The method is equivalent to {@code newDecapsulator(sk, null)}.
s/The/This/
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13256#discussion_r1165877912
PR Review Comment: https://git.openjdk.org/jdk/pull/13256#discussion_r1165882608
PR Review Comment: https://git.openjdk.org/jdk/pull/13256#discussion_r1165882987
PR Review Comment: https://git.openjdk.org/jdk/pull/13256#discussion_r1165883370
PR Review Comment: https://git.openjdk.org/jdk/pull/13256#discussion_r1165883547
PR Review Comment: https://git.openjdk.org/jdk/pull/13256#discussion_r1165886660
PR Review Comment: https://git.openjdk.org/jdk/pull/13256#discussion_r1165886977
PR Review Comment: https://git.openjdk.org/jdk/pull/13256#discussion_r1165884344
Re: RFR: 8297878: KEM: Implementation
On Thu, 13 Apr 2023 18:12:09 GMT, Sean Mullan wrote: >> The KEM API and DHKEM impl. Note that this PR uses new methods in >> https://github.com/openjdk/jdk/pull/13250. > > src/java.base/share/classes/javax/crypto/KEM.java line 1: > >> 1: /* > > Did you consider adding a `getAlgorithm` method? Most (all?) primitive > classes contain this method. Yes, you're right. I'll add one. - PR Review Comment: https://git.openjdk.org/jdk/pull/13256#discussion_r1165897772
Re: RFR: 8297878: KEM: Implementation
On Fri, 31 Mar 2023 02:25:04 GMT, Weijun Wang wrote: > The KEM API and DHKEM impl. Note that this PR uses new methods in > https://github.com/openjdk/jdk/pull/13250. src/java.base/share/classes/javax/crypto/KEM.java line 1: > 1: /* Did you consider adding a `getAlgorithm` method? Most (all?) primitive classes contain this method. - PR Review Comment: https://git.openjdk.org/jdk/pull/13256#discussion_r1165875028
Re: RFR: 8297878: KEM: Implementation
On Thu, 13 Apr 2023 17:35:00 GMT, Xue-Lei Andrew Fan wrote:
>> `DHKEM.java` is the implementation, and it does not know which provider it
>> will be put into. It's inside the provider that calls `putService` or `put`
>> to add an implementation there, not that the implementation registered
>> itself in a provider.
>>
>> If `getProvider()` is implemented inside the implementation, then it can
>> only be attached to one provider. Also, do you expect it to return `new
>> SunJCE()`? This means the `p` in `getInstance("DHKEM", p)` will be a
>> different instance from the value returned by `getProvider()`. There is no
>> specification talking about if the instances must be the same or not, but
>> it's probably not a good idea to have 2 objects for the same provider.
>>
>> In fact, I can create a new provider and simply call `putService` to add
>> existing implementations (that were already provided by other providers)
>> inside it, and I can `getInstance` from this provider and its
>> `getProvider()` returns this provider.
>>
>> For this reason, the base `Encapsulator` interface cannot be defined inside
>> `KEM`. As I said earlier, it can be defined inside `KEMSpi` and then we add
>> an extra `provider()` method to its implementation in `KEM`. I just don't
>> think this is worth doing.
>
>> `DHKEM.java` is the implementation, and it does not know which provider it
>> will be put into. It's inside the provider that calls `putService` or `put`
>> to add an implementation there, not that the implementation registered
>> itself in a provider.
>>
> I did not get the idea. Why DHKEM.java need register itself in a provider?
> A DHKEM.java is a part of a provider, and the Provider implementation in the
> provider knows how to register DHKEM.
>
>> If `getProvider()` is implemented inside the implementation, then it can
>> only be attached to one provider. Also, do you expect it to return `new
>> SunJCE()`? This means the `p` in `getInstance("DHKEM", p)` will be a
>> different instance from the value returned by `getProvider()`.
>
> I did not get the idea. How could it be possible to return different
> instances. `getInstance("DHKEM", p)` returns the DHKEM implementation in
> provider p. The "DHKEM" string name here is not the DHKEM.java class in
> SunJCE provider.
>
> Back to the question you have previously:
>> If the interface is only in KEM, then it needs a provider() method, but an
>> implementation actually does not know what the provider is.
>
> Why it is needed to know the provider of the interface? Do you mean the
> Encapsulator provider could be different from the KEM provider? That's, KEM
> provider is ProviderK, and the Encapsulator provider is ProviderE, and you
> want them work together? Does it make sense that it is required that
> Encapsulator is an internal implementation of the KEM provider (i.e., both
> from the same provider)?
Currently, `provider()` is a method of `KEM.Encapsulator`. If `KEMSpi.
newEncapsulator` also returns this interface, then what value should its
`provider()` method return? This is what I meant registering itself to a
provider.
When I said different instances, I was asking
var k = KEM.getInstance("DHKEM", p);
var e = k.newEncapsulator(pk);
// now, is p == e.provider()?
Or, are you suggesting we should define `provider()` somewhere else? It's
possible, but I have difficulty making every class immutable.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13256#discussion_r1165858327
Re: RFR: 8297878: KEM: Implementation
On Thu, 13 Apr 2023 17:10:01 GMT, Weijun Wang wrote:
> `DHKEM.java` is the implementation, and it does not know which provider it
> will be put into. It's inside the provider that calls `putService` or `put`
> to add an implementation there, not that the implementation registered itself
> in a provider.
>
I did not get the idea. Why DHKEM.java need register itself in a provider? A
DHKEM.java is a part of a provider, and the Provider implementation in the
provider knows how to register DHKEM.
> If `getProvider()` is implemented inside the implementation, then it can only
> be attached to one provider. Also, do you expect it to return `new SunJCE()`?
> This means the `p` in `getInstance("DHKEM", p)` will be a different instance
> from the value returned by `getProvider()`.
I did not get the idea. How could it be possible to return different
instances. `getInstance("DHKEM", p)` returns the DHKEM implementation in
provider p. The "DHKEM" string name here is not the DHKEM.java class in SunJCE
provider.
Back to the question you have previously:
> If the interface is only in KEM, then it needs a provider() method, but an
> implementation actually does not know what the provider is.
Why it is needed to know the provider of the interface? Do you mean the
Encapsulator provider could be different from the KEM provider? That's, KEM
provider is ProviderK, and the Encapsulator provider is ProviderE, and you want
them work together? Does it make sense that it is required that Encapsulator
is an internal implementation of the KEM provider (i.e., both from the same
provider)?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13256#discussion_r1165838768
Re: RFR: 8297878: KEM: Implementation
On Thu, 13 Apr 2023 17:08:44 GMT, Sean Mullan wrote:
>> The KEM API and DHKEM impl. Note that this PR uses new methods in
>> https://github.com/openjdk/jdk/pull/13250.
>
> src/java.base/share/classes/javax/crypto/KEM.java line 430:
>
>> 428: /**
>> 429: * Returns a {@code KEM} object that implements the specified
>> algorithm
>> 430: * and supports the specified {@code KEMParameterSpec} parameters
>
> There are no parameters for this method.
Oops, yes.
> src/java.base/share/classes/javax/crypto/KEM.java line 441:
>
>> 439: * to {@link #getInstance(String)}.
>> 440: * @return the new {@code KEM} object
>> 441: * @throws NoSuchAlgorithmException if no {@code Provider} supports
>> a
>
> This sounds like the provider argument is ignored. Maybe add some more words
> to take into account both cases "if a {@code provider} is specified and it
> does not support the specified KEM algorithm, or if {@code provider} is
> {@code null} and there is no provider that supports a KEM implementation of
> the specified algorithm"
Correct. I must be copying text from the method without a provider argument.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13256#discussion_r1165825088
PR Review Comment: https://git.openjdk.org/jdk/pull/13256#discussion_r1165826805
Re: RFR: 8297878: KEM: Implementation
On Fri, 31 Mar 2023 02:25:04 GMT, Weijun Wang wrote:
> The KEM API and DHKEM impl. Note that this PR uses new methods in
> https://github.com/openjdk/jdk/pull/13250.
src/java.base/share/classes/javax/crypto/KEM.java line 430:
> 428: /**
> 429: * Returns a {@code KEM} object that implements the specified
> algorithm
> 430: * and supports the specified {@code KEMParameterSpec} parameters
There are no parameters for this method.
src/java.base/share/classes/javax/crypto/KEM.java line 441:
> 439: * to {@link #getInstance(String)}.
> 440: * @return the new {@code KEM} object
> 441: * @throws NoSuchAlgorithmException if no {@code Provider} supports a
This sounds like the provider argument is ignored. Maybe add some more words to
take into account both cases "if a {@code provider} is specified and it does
not support the specified KEM algorithm, or if {@code provider} is {@code null}
and there is no provider that supports a KEM implementation of the specified
algorithm"
src/java.base/share/classes/javax/crypto/KEM.java line 460:
> 458: /**
> 459: * Returns a {@code KEM} object that implements the specified
> algorithm
> 460: * and supports the specified {@code KEMParameterSpec} parameters
There are no parameters.
src/java.base/share/classes/javax/crypto/KEM.java line 471:
> 469: * to {@link #getInstance(String)}.
> 470: * @return the new {@code KEM} object
> 471: * @throws NoSuchAlgorithmException if no {@code Provider} supports a
See above for similar comments.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13256#discussion_r1165814719
PR Review Comment: https://git.openjdk.org/jdk/pull/13256#discussion_r1165816835
PR Review Comment: https://git.openjdk.org/jdk/pull/13256#discussion_r1165821821
PR Review Comment: https://git.openjdk.org/jdk/pull/13256#discussion_r1165822406
Re: RFR: 8297878: KEM: Implementation
On Thu, 13 Apr 2023 02:51:28 GMT, Xue-Lei Andrew Fan wrote:
>> If the interface is only in `KEM`, then it needs a `provider()` method, but
>> an implementation actually does not know what the provider is. An
>> implementation can be registered in any (or even multiple) providers.
>
>> If the interface is only in `KEM`, then it needs a `provider()` method, but
>> an implementation actually does not know what the provider is.
>
> With "implementation", do you mean the javax/crypto/KEPSpi.java or
> src/java.base/share/classes/com/sun/crypto/provider/DHKEM.java?
>
> If it is refer to KEPSpi.java, why KEPSpi.java need to know what the provider
> is? Is it sufficient to use engineNewEncapsulator() to get the provider
> implementation?
>
> If it is refer to DHKEM.java, I did not get the idea why the provider is
> unknown.
>
>> An implementation can be registered in any (or even multiple) providers.
>
> I did not get the idea. Why it is not registered in SunJCE?
>
> I think you may have evaluated the following idea, but I'm not why it is not
> work. I may missed something. Would you mind explain in more details?
>
>
> public final class KEM {
> interface Encapsulator {
> ...
> KEM.Encapsulated encapsulate(...);
> ...
> }
>
> public static KEM getInstance(String algorithm) {
> ...
> }
>
> // Search for the registered providers, return the 1st non-null
> provider.newEncapsulator() or throw exception.
> public Encapsulator newEncapsulator(PublicKey pk,
> AlgorithmParameterSpec spec, SecureRandom secureRandom)
> ...
> }
> }
>
> public interface KEMSpi {
> // A provider implementation will implement the KEM.Encapsulator
> // interface internally. If a provider does not support the parameters,
> // null or nil object will be returned.
> public KEM.Encapsulator newEncapsulator(PublicKey pk,
> AlgorithmParameterSpec spec, SecureRandom secureRandom);
> }
>
> Use case:
> KEM.getInstance(DHKEM).newEncapsulator(...);
`DHKEM.java` is the implementation, and it does not know which provider it will
be put into. It's inside the provider that calls `putService` or `put` to add
an implementation there, not that the implementation registered itself in a
provider.
If `getProvider()` is implemented inside the implementation, then it can only
be attached to one provider. Also, do you expect it to return `new SunJCE()`?
This means the `p` in `getInstance("DHKEM", p)` will be a different instance
from the value returned by `getProvider()`. There is no specification talking
about if the instances must be the same or not, but it's probably not a good
idea to have 2 objects for the same provider.
In fact, I can create a new provider and simply call `putService` to add
existing implementations (that were already provided by other providers) inside
it, and I can `getInstance` from this provider and its `getProvider()` returns
this provider.
For this reason, the base `Encapsulator` interface cannot be defined inside
`KEM`. As I said earlier, it can be defined inside `KEMSpi` and then we add an
extra `provider()` method to its implementation in `KEM`. I just don't think
this is worth doing.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13256#discussion_r1165815968
Re: RFR: 8297878: KEM: Implementation
On Fri, 31 Mar 2023 02:25:04 GMT, Weijun Wang wrote:
> The KEM API and DHKEM impl. Note that this PR uses new methods in
> https://github.com/openjdk/jdk/pull/13250.
src/java.base/share/classes/javax/crypto/KEM.java line 36:
> 34:
> 35: /**
> 36: * The Key Encapsulation Mechanism.
How about a bit more here: "This class provides the functionality of a Key
Encapsulation Mechanism (KEM). A KEM can be used to encrypt symmetric keys
using asymmetric or public key cryptography."
src/java.base/share/classes/javax/crypto/KEM.java line 49:
> 47: * or public key, and the optional {@code AlgorithmParameterSpec} object,
> 48: * the {@code newEncapsulator} or {@code newDecapsulator} method
> 49: * may return encapsulators or decapsulators from different providers.
> The user
This sentence is kind of long. Suggest breaking it up, ex: "If a provider is
not specified in the {@code getInstance} method when instantiating a {@code
KEM} object, the {@code newEncapsulator} and {@code newDecapsulator} method
may return encapsulators or decapsulators from different providers. The
provider selected is based on the parameters passed to the the {@code
newEncapsulator} or {@code newDecapsulator} methods: the private or public key
and the optional {@code AlgorithmParameterSpec}."
src/java.base/share/classes/javax/crypto/KEM.java line 50:
> 48: * the {@code newEncapsulator} or {@code newDecapsulator} method
> 49: * may return encapsulators or decapsulators from different providers.
> The user
> 50: * can call the {@link Encapsulator#provider} or {@link
> Decapsulator#provider}
I would avoid referring to "The user". Suggest: "The {@link
Encapsulator#provider} and {@link Decapsulator#provider} methods return the
selected provider."
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13256#discussion_r1165787134
PR Review Comment: https://git.openjdk.org/jdk/pull/13256#discussion_r1165798901
PR Review Comment: https://git.openjdk.org/jdk/pull/13256#discussion_r1165800833
