On Fri, 28 Feb 2025 16:14:16 GMT, Kevin Driver <[email protected]> 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/HPKE.java line 101:
>
>> 99: @Override
>> 100: protected AlgorithmParameters engineGetParameters() {
>> 101: return null;
>
> In traditional JCE, wouldn't we return a representation of the
> `HPKEParameterSpec` which extends `AlgorithmParameters`?
Usually I think `AlgorithmParameters` is used when parameters has a defined
ASN.1 encoding starting with an algorithm identifier and ends with the
parameters byte. In this case, I am not aware of one. We can consider add it if
there is one.
> src/java.base/share/classes/com/sun/crypto/provider/HPKE.java line 292:
>
>> 290: return kdf.deriveKey(algorithm,
>> HKDFParameterSpec.expandOnly(exporter_secret,
>> 291: DHKEM.labeledInfo(suite_id,
>> "sec".getBytes(StandardCharsets.UTF_8),
>> 292: exporter_context, L), L));
>
> See other comment about input validation on `L` and whether it is useful to
> detect the case where `L` < 0 separately in the method that throws
> `Exception`.
Good point. I'm now relying on the HKDF implementation below to reject `L` that
is either negative or too big. But there could be different exceptions and I
need to wrap them into one kind. I'll also add more tests on every weird input.
> src/java.base/share/classes/com/sun/crypto/provider/HPKE.java line 582:
>
>> 580: // deriveData must be called because we need to
>> increment nonce, the info must be allowed
>> 581: var base_nonce =
>> kdf.deriveData(secret_x.thenExpand(DHKEM.labeledInfo(suite_id,
>> "base_nonce".getBytes(StandardCharsets.UTF_8),
>> 582: key_schedule_context, aead.Nn), aead.Nn));
>
> There are a few more of the in-lining with a length call here, but I assume
> you have more control over these values and/or some assurance that they
> aren't negative.
Yes, these are the hardcoded ones.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/18411#discussion_r1975920973
PR Review Comment: https://git.openjdk.org/jdk/pull/18411#discussion_r1975923066
PR Review Comment: https://git.openjdk.org/jdk/pull/18411#discussion_r1975923439
