On Mon, 14 Apr 2025 17:44:53 GMT, Francisco Ferrari Bihurriet
<[email protected]> wrote:
>> Martin Balao has updated the pull request incrementally with two additional
>> commits since the last revision:
>>
>> - Algorithm and key size checking before derivation. Mechanism
>> normalization for TLS.
>> - Minor import adjustment.
>
> src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11SecretKeyFactory.java
> line 240:
>
>> 238: putKeyInfo(new KeyInfo("TlsPremasterSecret",
>> PCKK_TLSPREMASTER));
>> 239: putKeyInfo(new KeyInfo("TlsRsaPremasterSecret",
>> PCKK_TLSRSAPREMASTER));
>> 240: putKeyInfo(new KeyInfo("TlsMasterSecret", PCKK_TLSMASTER));
>
> Have you considered removing `PCKK_TLSPREMASTER`, `PCKK_TLSRSAPREMASTER` and
> `PCKK_TLSMASTER` from everywhere? We could just use `CKK_GENERIC_SECRET` for
> the `TlsPremasterSecret`, `TlsRsaPremasterSecret` and `TlsMasterSecret` key
> info entries.
>
> Unlike `PCKK_ANY`, which is used for the `TemplateManager` to map `*` entries
> in _SunPKCS11_ configuration files [1], these other 3 pseudo key types are
> only used here in `P11SecretKeyFactory`. Additionally, any time these 3
> pseudo key types are used, it is to map to `CKK_GENERIC_SECRET`.
>
> [1] _SunPKCS11_ configuration files can't contain `PCKK_TLS*MASTER`
> attributes entries, only `*` is parsable, and corresponds with `PCKK_ANY`:
> https://github.com/openjdk/jdk/blob/6ddbcc34c019d780fc12d8f636e3aa3de33ecaaa/src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/wrapper/Functions.java#L1258
I like this idea but the downside I see is that we would need string comparison
in `P11KDF::getDerivedKeyType` to allow TLS keys. What if we merge all
`PCKK_TLSPREMASTER`, `PCKK_TLSRSAPREMASTER` and `PCKK_TLSMASTER` into
`PCKK_TLSKEY` and then do the translation to `CKK_GENERIC_SECRET` as needed?
This will also help with the new Tls* keys that I am planning to add to the map.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/24526#discussion_r2044527101
