On Tue, 15 Apr 2025 13:20:34 GMT, Martin Balao <[email protected]> wrote:
>> 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.
BTW, I don't like the partial "Tls" string comparison much because it's making
an assumption about the algorithm name.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/24526#discussion_r2044533071
