On Thu, 10 Apr 2025 23:54:03 GMT, Martin Balao <mba...@openjdk.org> wrote:

>> Hi,
>> 
>> I would like to request a review for the fix of JDK-8350661. In this fix, we 
>> translate the native PKCS 11 error code into an 
>> `InvalidAlgorithmParameterException`, as documented in the `KDF::deriveKey` 
>> API. With that said, different PKCS 11 libraries may throw different errors 
>> and may even (in theory) delay the error until the key is used, as _SunJCE_ 
>> does. I believe that this is an improvement but further adjustments may be 
>> needed in the future.
>> 
>> No regressions observed in `test/jdk/sun/security/pkcs11/KDF/TestHKDF.java`.
>> 
>> Thanks,
>> Martin.-
>
> 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.

Hi @martinuy,

Thanks for your proposal, I left four comments. Two of them are 
suggestions/ideas, but unless my static analysis is bogus, I also found a minor 
bug (one comment explains the reasoning, the other suggests a low-hanging fruit 
test case to confirm).

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

test/jdk/sun/security/pkcs11/KDF/TestHKDF.java line 643:

> 641:                 32,
> 642:                 "Derivation of an invalid key algorithm");
> 643:     }

I suggest adding a case with an invalid key algorithm whose key info map entry 
doesn't have `KeyInfo.keyType=CKK_GENERIC_SECRET`. For example, 
`PBEWithHmacSHA224AndAES_256`, where `KeyInfo.keyType=CKK_AES`.

-------------

Changes requested by fferrari (Committer).

PR Review: https://git.openjdk.org/jdk/pull/24526#pullrequestreview-2765121407
PR Review Comment: https://git.openjdk.org/jdk/pull/24526#discussion_r2042623420
PR Review Comment: https://git.openjdk.org/jdk/pull/24526#discussion_r2042751519

Reply via email to