On Fri, 28 Feb 2025 16:05:58 GMT, Kevin Driver <kdri...@openjdk.org> wrote:
>> Weijun Wang has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> example and KAT
>
> src/java.base/share/classes/com/sun/crypto/provider/DHKEM.java line 323:
>
>> 321: }
>> 322: byte[] bytes =
>> kdf.deriveData(extract.thenExpand(labeledInfo(
>> 323: suiteId, CANDIDATE, I2OSP(counter, 1),
>> Nsk), Nsk));
>
> I'm not through every class yet, but is more input validation needed on
> `Nsk`, which ultimately becomes the length in the `HKDFParameterSpec`? Later
> in this class I see that it is checked to not exceed `65536`, but an
> `IllegalArgumentException` may be thrown here if the value is < 0. I see that
> you're throwing `Exception` from this method, but I thought I'd mention it
> since you are doing `HKDFParameterSpec` initialization in-line with the
> `deriveData` call.
These are internally hardcoded numbers and should never be illegal. If it
really goes wrong, there must be a coding error somewhere. I've added
statements like `assert n < 65536` to ensure these. I didn't add an assertion
like `assert n >= 0` because I think it's even more unlikely to happen.
> src/java.base/share/classes/com/sun/crypto/provider/DHKEM.java line 406:
>
>> 404: }
>> 405:
>> 406: public static byte[] I2OSP(int n, int w) {
>
> A comment (non-javadoc) might be beneficial to explain why this method is
> doing what it is doing.
Sure. I can add some references into the RFC.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/18411#discussion_r1975905712
PR Review Comment: https://git.openjdk.org/jdk/pull/18411#discussion_r1975907356
