On Thu, 17 Apr 2025 23:52:56 GMT, Valerie Peng <valer...@openjdk.org> wrote:
>> Martin Balao has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Inform key sizes in the exception when failing check.
>
> src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11HKDF.java line
> 252:
>
>> 250: throw new InvalidAlgorithmParameterException("A key of
>> algorithm " +
>> 251: "'" + alg + "' is not valid for derivation.");
>> 252: }
>
> So, essentially, we only allow HKDF to derive keys when ki is either
> `TLSKeyInfo` or `KeyInfo` w/ the key types in `canDeriveKeyInfoType(long)`
> method? Are you checking each key algorithm just to rule out `RC2` and
> `IDEA`? Not sure if it's worth the extra checking. Besides, if more `CKK_xxx`
> is added to `KeyInfo`, it'd need to be added to `canDeriveKeyInfoType(long)`
> which can be easily missed.
That's right: only `TLSKeyInfo` and selected `KeyInfo` keys allowed for HKDF
derivation. If something is added to the map, it's not implicitly allowed. Same
is true for `P11SecretKeyFactory::createKey` (`C_CreateObject`). Even
`P11KeyGenerator` may need adjustments depending on the key type. I think that
adding something should require to think what's the impact on different
services, and not rely on a default/implicit behavior. I don't expect new
symmetric key updates to be that frequent, though. If this becomes a problem,
we can implement what @franferrax suggested and move the "uses" information to
the map.
I was not thinking of `RC2` or `IDEA` but of key algorithms that are not the
result of a HKDF derivation and did not go through the checks that we would
enforce otherwise. For example, a key of algorithm
`PBEWithHmacSHA512AndAES_256` should be the result of a PBE derivation and
should have a specific key size. It would be inconsistent to obtain a key of
this algorithm with a different size through a HKDF derivation. When we check
or convert keys to be used in services, we rely on this information.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/24526#discussion_r2051070244
