On Mon, 12 May 2025 19:45:47 GMT, Kevin Driver <kdri...@openjdk.org> wrote:
>> Valerie Peng has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Address review feedbacks from Brad. > > src/java.base/share/classes/sun/security/ssl/SSLBasicKeyDerivation.java line > 57: > >> 55: KDF hkdf = KDF.getInstance(hkdfAlg); >> 56: return hkdf.deriveKey(type, >> 57: HKDFParameterSpec.expandOnly(secret, hkdfInfo, >> keyLen)); > > I noticed we have done away with the `AlgorithmParameterSpec` parameter to > this method. While the new implementation makes sense in light of the new > parameter set, I wonder whether there was a deliberate use-case lost here... > > In the previous implementation, presumably, if the `keySpec`'s length was > shorter than the expanded value, only a portion of the value would be used. > With the new implementation, the hash algorithm's length of bytes is always > used. > > I'm not sure how relevant this is, but it could be worth noting. If we had > kept the old parameter, I suppose we could have used `deriveData` and > truncated the result to `((SecretSizeSpec)keySpec).length` before returning a > `SecretKey`. Searching among the JSSE code base, it looks that `SSLBasicKeyDerivation` class is only used in `Finished` class and the particular `SecretSizeSpec` value is always same as the current `keyLen` field. There is no other usage as far as I can tell. If we ended up using `SSLBasicKeyDerivation` class with a shorter key length, we can add an explicit `keyLen` argument to the `SSLBasicKeyDerivation` constructor. I see no need for the `SecretSizeSpec` class given the code flow in `Finished` class. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/24393#discussion_r2085646297
