8350661: PKCS11 HKDF throws ProviderException when requesting a 31-byte AES key #24526

martinuy · 2025-04-08T20:02:56Z

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.-

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue

Issue

JDK-8350661: PKCS11 HKDF throws ProviderException when requesting a 31-byte AES key (Bug - P3)

Reviewers

Daniel Jeliński (@djelinski - Reviewer) Review applies to 8feb31ea
Francisco Ferrari Bihurriet (@franferrax - Committer)
Valerie Peng (@valeriepeng - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/24526/head:pull/24526
$ git checkout pull/24526

Update a local copy of the PR:
$ git checkout pull/24526
$ git pull https://git.openjdk.org/jdk.git pull/24526/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 24526

View PR using the GUI difftool:
$ git pr show -t 24526

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/24526.diff

Using Webrev

Link to Webrev Comment

…te AES key

bridgekeeper · 2025-04-08T20:03:42Z

👋 Welcome back mbalao! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

martinuy · 2025-04-08T20:04:12Z

@franferrax @valeriepeng @wangweij may I ask you to have a look at this proposal? Thanks!

openjdk · 2025-04-08T20:04:26Z

@martinuy This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8350661: PKCS11 HKDF throws ProviderException when requesting a 31-byte AES key

Reviewed-by: fferrari, valeriep, djelinski

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 240 new commits pushed to the master branch:

f2587d9: 8354938: ZGC: Disable UseNUMA when ZFakeNUMA is used
0f1c448: 8354922: ZGC: Use MAP_FIXED_NOREPLACE when reserving memory
9eeb86d: 8354341: Open some JTable bugs 7
... and 237 more: https://git.openjdk.org/jdk/compare/676cfae91c8bd4799adfedff2ad59a9aab953ece...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

openjdk · 2025-04-08T20:05:21Z

@martinuy The following label will be automatically applied to this pull request:

security

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

mlbridge · 2025-04-08T20:08:32Z

Webrevs

wangweij · 2025-04-08T21:23:22Z

Looks good to me, but again I am not a PKCS #11 expert.

djelinski · 2025-04-09T06:45:14Z

I think the usual way to handle this is by calling P11KeyGenerator.checkKeySize

myankelev · 2025-04-09T10:57:45Z

test/jdk/sun/security/pkcs11/KDF/TestHKDF.java

+            KDF k = KDF.getInstance("HKDF-SHA256", p11Provider);
+            k.deriveKey("AES", HKDFParameterSpec.ofExtract()
+                    .thenExpand(null, 31));
+            throw new Exception("No exception thrown.");


I think this throw is unnecessary, you can just call this code straight on line 619 and remove the catch block below

reportTestFailure(new Exception("Derivation of an AES key of " + "invalid size (31 bytes) expected to throw " + "InvalidAlgorithmParameterException.", e));

What do you think?

The catch block would still be needed because another exception (such as ProviderException, as is the case now) might be thrown and we want to make sure that it does not occur.

myankelev · 2025-04-09T11:03:52Z

src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11HKDF.java

 import static sun.security.pkcs11.TemplateManager.*;
 import sun.security.pkcs11.wrapper.*;
 import static sun.security.pkcs11.wrapper.PKCS11Constants.*;
+import static sun.security.pkcs11.wrapper.PKCS11Exception.RV.*;


Nitpick: Does this import need to be a *? Wouldn't it be better to just have a import static sun.security.pkcs11.wrapper.PKCS11Exception.RV.CKR_KEY_SIZE_RANGE; ?

martinuy · 2025-04-09T13:19:45Z

I think the usual way to handle this is by calling P11KeyGenerator.checkKeySize

We discussed calling P11KeyGenerator::checkKeySize with @franferrax but were not sure of taking this approach. We found that for DES(3) cases some fixed values are considered valid but wondered if, in theory, the PKCS 11 library can be configured to be more restrictive and reject some of them. Given that this is an error-path and should be exceptional, we thought that the cost of passing the operation to the token and handling the error was affordable. Perhaps we can do both: check beforehand and handle the error afterwards. I'll give it some more thinking.

djelinski · 2025-04-09T14:31:45Z

Perhaps we can do both: check beforehand and handle the error afterwards.

That sounds reasonable.

Whatever you decide, I think it would be good to make sure P11HKDF, P11SecretKeyFactory and P11KeyGenerator perform the same checks during key generation.

valeriepeng · 2025-04-10T03:08:32Z

test/jdk/sun/security/pkcs11/KDF/TestHKDF.java

+            KDF k = KDF.getInstance("HKDF-SHA256", p11Provider);
+            k.deriveKey("AES", HKDFParameterSpec.ofExtract()
+                    .thenExpand(null, 31));
+            throw new Exception("No exception thrown.");


nit: "Expected InvalidAlgorithmParameterException not thrown" is clearer?

This exception will be the cause, the wrapper exception will inform that InvalidAlgorithmParameterException was expected to be thrown.

valeriepeng · 2025-04-10T03:11:23Z

test/jdk/sun/security/pkcs11/KDF/TestHKDF.java

+            reportTestFailure(new Exception("Derivation of an AES key of " +
+                    "invalid size (31 bytes) expected to throw " +
+                    "InvalidAlgorithmParameterException.", e));


Why not just use reportTestFailure(e)? I don't find the extra layer of exception too useful.

To be more informative, because the original exception —not the one that we would throw if no exception is thrown— does not state which exception was expected to be thrown, is just a ProviderException which may even look good/appropriate for an invalid key size.

valeriepeng · 2025-04-10T03:27:19Z

On a related note, I am working on #24393 and noticed that JSSE calls HKDF.deriveKey(...) with various names such as "TlsFinishedSecret", "TlsResumptionMasterSecret" as the key algorithm. This causes errors when using PKCS11 HKDF since the P11SecretKeyFactory.getKeyInfo() look up returns null and leads to InvalidAlgorithmParameterException. I am debating whether to do the special handling as below:

         P11SecretKeyFactory.KeyInfo ki = P11SecretKeyFactory.getKeyInfo(alg);
         if (ki == null) {
-            throw new InvalidAlgorithmParameterException("A PKCS #11 key " +
-                    "type (CKK_*) was not found for a key of the algorithm '" +
-                    alg + "'.");
+            // special handling for TLS
+            if (alg.startsWith("Tls")) {
+                ki = P11SecretKeyFactory.getKeyInfo("Generic");
+            } else {
+                throw new InvalidAlgorithmParameterException("A PKCS #11 key " +
+                        "type (CKK_*) was not found for a key of the algorithm '" +
+                        alg + "'.");
+            }

Comments or suggestions? @martinuy

martinuy · 2025-04-10T03:54:26Z

On a related note, I am working on #24393 and noticed that JSSE calls HKDF.deriveKey(...) with various names such as "TlsFinishedSecret", "TlsResumptionMasterSecret" as the key algorithm. This causes errors when using PKCS11 HKDF since the P11SecretKeyFactory.getKeyInfo() look up returns null and leads to InvalidAlgorithmParameterException. I am debating whether to do the special handling as below:
         P11SecretKeyFactory.KeyInfo ki = P11SecretKeyFactory.getKeyInfo(alg);
         if (ki == null) {
-            throw new InvalidAlgorithmParameterException("A PKCS #11 key " +
-                    "type (CKK_*) was not found for a key of the algorithm '" +
-                    alg + "'.");
+            // special handling for TLS
+            if (alg.startsWith("Tls")) {
+                ki = P11SecretKeyFactory.getKeyInfo("Generic");
+            } else {
+                throw new InvalidAlgorithmParameterException("A PKCS #11 key " +
+                        "type (CKK_*) was not found for a key of the algorithm '" +
+                        alg + "'.");
+            }
Comments or suggestions? @martinuy

Thanks for the heads up. I was just looking at the possibility of passing key algorithms for derived keys that were not considered before but are part of the map (e.g. hmac, pbe, etc.), and wondered how that should be handled. Tls could be the opposite case: not part of the map but needs to be handled. Will give it some thinking.

I was also thinking of extending the P11SecretKeyFactory::keyInfo map to include key generation mechanisms that would facilitate querying CK_MECHANISM_INFO and obtaining valid key size ranges for each key type. This would simplify the call to P11KeyGenerator::checkKeySize from P11SecretKeyFactory::createKey and allow a call from P11HKDF. While the idea suggested by @djelinski sounds reasonable, I want to notice an implicit assumption that may not hold true for all PKCS 11 libraries: CK_MECHANISM_INFO data for mechanisms such as CKM_AES_KEY_GEN will be used for mechanisms such as CKM_HKDF_DATA/CKM_HKDF_DERIVE. P11SecretKeyFactory::createKey has a similar type of assumption.

…ation for TLS.

martinuy · 2025-04-10T23:53:49Z

@valeriepeng @djelinski @franferrax can you please take a look at this new proposal? Thanks!

martinuy · 2025-04-11T00:00:39Z

What I have found with Tls* keys is that they are in the map but we need to translate their pseudo-mechanism to a valid one (CKK_GENERIC_SECRET). Is that enough for #24393?

djelinski

LGTM.

valeriepeng · 2025-04-11T19:47:38Z

What I have found with Tls* keys is that they are in the map but we need to translate their pseudo-mechanism to a valid one (CKK_GENERIC_SECRET). Is that enough for #24393?

What I found is that there are more "TlsXXX" than those defined in P11SecretKeyFactory class which are mapped to PCKK_xxx. So, we will need to decide if those self-defined "TlsXXX" algorithms are allowed (e.g. PKCS11 will treat them as Generic secret keys or changing the TLS code to use a key algorithm recognized by PKCS11). Beside this, we need to make sure the current pseudo key type works, e.g. translating to a valid key type when necessary, as you stated.

valeriepeng · 2025-04-11T21:28:30Z

src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11HKDF.java

+                if (ki.keyType != CKK_GENERIC_SECRET ||
+                        alg.equalsIgnoreCase("Generic")) {
+                    return ki.keyType;
+                }


What is this check for? It's not immediately clear to me why do we look up the keyInfo using alg but then again to check to error out when the found keyType is CKK_GENERIC_SECRET && alg not equals "Generic". This seems to directly contradicts the special workaround for "TlsXXX" in my other PR?

For the TlsXXX issue I check the pseudo-mechanism. That works if all algorithms are known to the map. I'll check how many we have and see what are the pros/cons of having them in the map. I prefer symmetric key algorithms to be in the map.

The reason for the check you referred is to block deriving keys such as HmacSHA256, PBEWithHmacSHA224AndAES_256, etc. which are not the result of HKDF derivations, but of Mac and PBE derivation.

As far as I understand it, HmacSHA256 is blocked, but not PBEWithHmacSHA224AndAES_256.

HmacSHA256

Has an HMACKeyInfo entry with the following non-static fields:

KeyInfo.algo = "HmacSHA256"

KeyInfo.keyType = CKK_GENERIC_SECRET

KeyInfo.keyGenMech = CK_UNAVAILABLE_INFORMATION

HMACKeyInfo.mech = CKM_SHA256_HMAC

HMACKeyInfo.keyLen = 256

Given ki.keyType is CKK_GENERIC_SECRET and alg is HmacSHA256, in P11HKDF::getDerivedKeyType it will enter the first case but not the if. So it will finally throw the expected exception:

InvalidAlgorithmParameterException("A key of algorithm 'HmacSHA256' is not valid for derivation.")

PBEWithHmacSHA224AndAES_256

Has an AESPBEKeyInfo entry with the following non-static fields:

KeyInfo.algo = "PBEWithHmacSHA224AndAES_256"

KeyInfo.keyType = CKK_AES

KeyInfo.keyGenMech = CK_UNAVAILABLE_INFORMATION

PBEKeyInfo.kdfMech = CKM_PKCS5_PBKD2

PBEKeyInfo.prfMech = CKP_PKCS5_PBKD2_HMAC_SHA224

PBEKeyInfo.keyLen = 256

PBEKeyInfo.extraAttrs = new CK_ATTRIBUTE[] { CK_ATTRIBUTE.ENCRYPT_TRUE }

Given ki.keyType is CKK_AES, in P11HKDF::getDerivedKeyType it will enter the first case and also the if, returning CKK_AES. Later, in P11KeyGenerator::checkKeySize(..., Token token), P11KeyGenerator::getSupportedRange will return null, because ki.keyGenMech is CK_UNAVAILABLE_INFORMATION. This will make P11KeyGenerator::checkKeySize(..., CK_MECHANISM_INFO range) enter the default case, and finally return the unmodified keySize. No exception is thrown, unless I'm missing something.

Also, we could save one of the if conditions by creating a separate case for CKK_GENERIC_SECRET:

switch ((int) ki.keyType) { case (int) CKK_DES, (int) CKK_DES3, (int) CKK_AES, (int) CKK_RC4, (int) CKK_BLOWFISH, (int) CKK_CHACHA20 -> { return ki.keyType; } case (int) CKK_GENERIC_SECRET -> { if (alg.equalsIgnoreCase("Generic")) { return ki.keyType; } } // [...] }

In my view, this is quicker to understand, what do you think?

Yes, I much prefer this idea of separate case for CKK_GENERIC_SECRET.

valeriepeng · 2025-04-11T21:32:47Z

src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11HKDF.java

+            case (int) PCKK_TLSPREMASTER, (int) PCKK_TLSRSAPREMASTER,
+                    (int) PCKK_TLSMASTER -> {
+                return CKK_GENERIC_SECRET;
+            }


It's easier to troubleshoot to add a default case and not let it fall through to the exception on line 253? It's possible that P11SecretKeyFactory is enhanced with more KeyInfo, but the newly added keyType is not added here. Lumping different causes into the same exception may be harder to debug.

The exception informs the algorithm, and we know that the algorithm was found in the map because, otherwise, we would have not been able to get the KeyInfo ki received by parameter. I can add two separate exceptions if you want, but should not make much of a difference because the reason for the exception is the same: the algorithm is not valid for derivation, regardless if its underlying mechanism is CKK_GENERIC_SECRET or something else.

martinuy · 2025-04-11T23:46:49Z

What I have found with Tls* keys is that they are in the map but we need to translate their pseudo-mechanism to a valid one (CKK_GENERIC_SECRET). Is that enough for #24393?

What I found is that there are more "TlsXXX" than those defined in P11SecretKeyFactory class which are mapped to PCKK_xxx. So, we will need to decide if those self-defined "TlsXXX" algorithms are allowed (e.g. PKCS11 will treat them as Generic secret keys or changing the TLS code to use a key algorithm recognized by PKCS11). Beside this, we need to make sure the current pseudo key type works, e.g. translating to a valid key type when necessary, as you stated.

Good, let me check this.

franferrax

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).

franferrax · 2025-04-14T17:44:53Z

src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11SecretKeyFactory.java


        putKeyInfo(new KeyInfo("TlsPremasterSecret", PCKK_TLSPREMASTER));
        putKeyInfo(new KeyInfo("TlsRsaPremasterSecret", PCKK_TLSRSAPREMASTER));
        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:

jdk/src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/wrapper/Functions.java

Line 1258 in 6ddbcc3

addKeyType(PCKK_ANY, "*");

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.

A new PCKK_TLSKEY pseudo key type looks good to me. Alternatively, and just thinking out loud, how about introducing a new TlsKeyInfo and using ki instanceof TlsKeyInfo in P11KDF::getDerivedKeyType?

Perhaps we could also add a new KeyInfo.supportsHKDF boolean field and store that information in the map, replacing the whole P11KDF::getDerivedKeyType call by a ki.supportsHKDF check. This would also solve the PBEWithHmacSHA224AndAES_256 case.

KeyInfo.supportsHKDF could be a valid approach. My only concern would be overloading the map/map-objects with information that is specific to each use of a key type. For example, the same criteria could be applied to store whether a key is suitable for C_CreateObject in P11SecretKeyFactory::createKey. What I liked about storing the key gen mech is that we have different users benefiting from the same information. If there is interest in exploring this idea, I can propose something.

I liked the TlsKeyInfo idea to remove PCKK_TLS* completely. We can create these keys and assign GENERIC, same as HMACKeyInfo.

franferrax · 2025-04-14T18:53:12Z

test/jdk/sun/security/pkcs11/KDF/TestHKDF.java

+                "PBKDF2WithHmacSHA1",
+                32,
+                "Derivation of an invalid key algorithm");
+    }


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.

Sounds good!

franferrax · 2025-04-14T19:01:00Z

src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11HKDF.java

+                if (ki.keyType != CKK_GENERIC_SECRET ||
+                        alg.equalsIgnoreCase("Generic")) {
+                    return ki.keyType;
+                }


As far as I understand it, HmacSHA256 is blocked, but not PBEWithHmacSHA224AndAES_256.

HmacSHA256

Has an HMACKeyInfo entry with the following non-static fields:

KeyInfo.algo = "HmacSHA256"

KeyInfo.keyType = CKK_GENERIC_SECRET

KeyInfo.keyGenMech = CK_UNAVAILABLE_INFORMATION

HMACKeyInfo.mech = CKM_SHA256_HMAC

HMACKeyInfo.keyLen = 256

Given ki.keyType is CKK_GENERIC_SECRET and alg is HmacSHA256, in P11HKDF::getDerivedKeyType it will enter the first case but not the if. So it will finally throw the expected exception:

InvalidAlgorithmParameterException("A key of algorithm 'HmacSHA256' is not valid for derivation.")

PBEWithHmacSHA224AndAES_256

Has an AESPBEKeyInfo entry with the following non-static fields:

KeyInfo.algo = "PBEWithHmacSHA224AndAES_256"

KeyInfo.keyType = CKK_AES

KeyInfo.keyGenMech = CK_UNAVAILABLE_INFORMATION

PBEKeyInfo.kdfMech = CKM_PKCS5_PBKD2

PBEKeyInfo.prfMech = CKP_PKCS5_PBKD2_HMAC_SHA224

PBEKeyInfo.keyLen = 256

PBEKeyInfo.extraAttrs = new CK_ATTRIBUTE[] { CK_ATTRIBUTE.ENCRYPT_TRUE }

Given ki.keyType is CKK_AES, in P11HKDF::getDerivedKeyType it will enter the first case and also the if, returning CKK_AES. Later, in P11KeyGenerator::checkKeySize(..., Token token), P11KeyGenerator::getSupportedRange will return null, because ki.keyGenMech is CK_UNAVAILABLE_INFORMATION. This will make P11KeyGenerator::checkKeySize(..., CK_MECHANISM_INFO range) enter the default case, and finally return the unmodified keySize. No exception is thrown, unless I'm missing something.

franferrax · 2025-04-14T19:01:45Z

src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11HKDF.java

+                if (ki.keyType != CKK_GENERIC_SECRET ||
+                        alg.equalsIgnoreCase("Generic")) {
+                    return ki.keyType;
+                }


Also, we could save one of the if conditions by creating a separate case for CKK_GENERIC_SECRET:

switch ((int) ki.keyType) { case (int) CKK_DES, (int) CKK_DES3, (int) CKK_AES, (int) CKK_RC4, (int) CKK_BLOWFISH, (int) CKK_CHACHA20 -> { return ki.keyType; } case (int) CKK_GENERIC_SECRET -> { if (alg.equalsIgnoreCase("Generic")) { return ki.keyType; } } // [...] }

In my view, this is quicker to understand, what do you think?

martinuy · 2025-04-15T13:46:56Z

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).

Thanks for your review.

Yes, there is a hole that allows derivation for algorithms such as PBEWithHmacSHA224AndAES_256. Well spotted! I'm planning to restrict PBE algorithms based on the PBEKeyInfo subclass. Perhaps checking HMACKeyInfo doesn't hurt, even when these should not pass the mechanism check.

martinuy · 2025-04-17T00:25:07Z

I've implemented the TLSKeyInfo suggestion and added all Tls* keys to the map —after verifying they reach KDF::deriveKey in #24393.

valeriepeng · 2025-04-17T00:47:00Z

src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11HKDF.java

+                throw new InvalidAlgorithmParameterException("Invalid key " +
+                        "size for algorithm '" + alg + "'.", e);


nit: include the requested key size in the exception message?

Sounds good!

valeriepeng · 2025-04-17T20:52:52Z

src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11SecretKeyFactory.java

    }

-    static sealed class KeyInfo permits PBEKeyInfo, HMACKeyInfo, HKDFKeyInfo {
+    static sealed class KeyInfo permits PBEKeyInfo, HMACKeyInfo, HKDFKeyInfo,


Can we add some comment about the purpose of KeyInfo and the PKCS11 classes which depend on it? E.g. HKDF will use the key algorithm to look up the corresponding key type. Also some comment for the various child key info classes would be nice.

valeriepeng · 2025-04-17T20:56:57Z

src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11SecretKeyFactory.java

        }
    }

+    static final class TLSKeyInfo extends KeyInfo {


Documenting this TLSKeyInfo is to support JSSE using HKDF to derive various keys whose algorithms are named following the "TlsXXX" convention?

valeriepeng · 2025-04-17T22:59:49Z

src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11SecretKeyFactory.java

+                case (int) CKK_DES, (int) CKK_DES3, (int) CKK_AES, (int) CKK_RC4,
+                        (int) CKK_BLOWFISH, (int) CKK_CHACHA20 -> {
+                    keyLength = P11KeyGenerator.checkKeySize(ki.keyGenMech, n,
+                            token);
+                    if (keyType == CKK_DES || keyType == CKK_DES3) {
+                        fixDESParity(encoded, 0);
+                    }
+                    if (keyType == CKK_DES3) {
+                        fixDESParity(encoded, 8);
+                        if (keyLength == 112) {
+                            keyType = CKK_DES2;
+                        } else {
+                            fixDESParity(encoded, 16);
+                        }
                    }
                }


Hmm, how about separating out AES, RC4, Blowfish and ChaCha20 to a separate case? Only DES and DES3 needs parity checking and they are very legacy.

We would need to repeat code if we separate (invocation to P11KeyGenerator::checkKeySize). Does not look complex enough in my opinion to merit this split.

The separation can remove 1 conditional block, so only 1 extra line and the flow looks cleaner in my opinion, e.g.

Suggested change

case (int) CKK_DES, (int) CKK_DES3, (int) CKK_AES, (int) CKK_RC4,

(int) CKK_BLOWFISH, (int) CKK_CHACHA20 -> {

keyLength = P11KeyGenerator.checkKeySize(ki.keyGenMech, n,

token);

if (keyType == CKK_DES || keyType == CKK_DES3) {

fixDESParity(encoded, 0);

}

if (keyType == CKK_DES3) {

fixDESParity(encoded, 8);

if (keyLength == 112) {

keyType = CKK_DES2;

} else {

fixDESParity(encoded, 16);

}

}

}

case (int) CKK_DES, (int) CKK_DES3 -> {

keyLength = P11KeyGenerator.checkKeySize(ki.keyGenMech, n,

token);

fixDESParity(encoded, 0);

if (keyType == CKK_DES3) {

fixDESParity(encoded, 8);

if (keyLength == 112) {

keyType = CKK_DES2;

} else {

fixDESParity(encoded, 16);

}

}

}

case (int) CKK_AES, (int) CKK_RC4, (int) CKK_BLOWFISH, (int) CKK_CHACHA20 -> {

keyLength = P11KeyGenerator.checkKeySize(ki.keyGenMech, n,

token);

}

If you'd still like lumping them together as you have it now, then at least move the if (keyType == CKK_DES3) block (line 624-630) to inside the previous if-block (line 621-623)?

valeriepeng · 2025-04-17T23:52:56Z

src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11HKDF.java

+        if (!kiClass.equals(P11SecretKeyFactory.TLSKeyInfo.class) &&
+                !(kiClass.equals(P11SecretKeyFactory.KeyInfo.class) &&
+                        canDeriveKeyInfoType(ki.keyType))) {
+            throw new InvalidAlgorithmParameterException("A key of algorithm " +
+                    "'" + alg + "' is not valid for derivation.");
+        }


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.

valeriepeng

Updates look good to me. Thanks~

franferrax

Thanks for the updates, it looks good to me.

martinuy · 2025-04-22T14:36:49Z

/integrate

openjdk · 2025-04-22T14:37:11Z

Going to push as commit 5264d80.
Since your change was applied there have been 240 commits pushed to the master branch:

f2587d9: 8354938: ZGC: Disable UseNUMA when ZFakeNUMA is used
0f1c448: 8354922: ZGC: Use MAP_FIXED_NOREPLACE when reserving memory
9eeb86d: 8354341: Open some JTable bugs 7
... and 237 more: https://git.openjdk.org/jdk/compare/676cfae91c8bd4799adfedff2ad59a9aab953ece...master

Your commit was automatically rebased without conflicts.

openjdk · 2025-04-22T14:37:23Z

@martinuy Pushed as commit 5264d80.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

8350661: PKCS11 HKDF throws ProviderException when requesting a 31-by…

848dd1e

…te AES key

openjdk bot added the rfr Pull request is ready for review label Apr 8, 2025

openjdk bot added the security [email protected] label Apr 8, 2025

myankelev reviewed Apr 9, 2025

View reviewed changes

Minor import adjustment.

10da8b2

valeriepeng reviewed Apr 10, 2025

View reviewed changes

Algorithm and key size checking before derivation. Mechanism normaliz…

8feb31e

…ation for TLS.

djelinski approved these changes Apr 11, 2025

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Apr 11, 2025

valeriepeng reviewed Apr 11, 2025

View reviewed changes

franferrax suggested changes Apr 14, 2025

View reviewed changes

Key type check refactoring (derivation).

d74278b

TLS keys added to the map.

89d15ba

openjdk bot removed the ready Pull request is ready to be integrated label Apr 17, 2025

valeriepeng reviewed Apr 17, 2025

View reviewed changes

Inform key sizes in the exception when failing check.

1ffde71

valeriepeng reviewed Apr 17, 2025

View reviewed changes

martinuy added 2 commits April 18, 2025 16:18

KeyInfo class and subclasses documentation.

50e4e92

Minor change.

7bec0df

valeriepeng approved these changes Apr 21, 2025

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Apr 21, 2025

franferrax approved these changes Apr 22, 2025

View reviewed changes

openjdk bot added the integrated Pull request has been integrated label Apr 22, 2025

openjdk bot closed this Apr 22, 2025

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Apr 22, 2025

		throw new InvalidAlgorithmParameterException("Invalid key " +
		"size for algorithm '" + alg + "'.", e);

8350661: PKCS11 HKDF throws ProviderException when requesting a 31-byte AES key #24526

8350661: PKCS11 HKDF throws ProviderException when requesting a 31-byte AES key #24526

Uh oh!

Conversation

martinuy commented Apr 8, 2025 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Reviewing

Uh oh!

bridgekeeper bot commented Apr 8, 2025

Uh oh!

martinuy commented Apr 8, 2025

Uh oh!

openjdk bot commented Apr 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openjdk bot commented Apr 8, 2025

Uh oh!

mlbridge bot commented Apr 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

wangweij commented Apr 8, 2025

Uh oh!

djelinski commented Apr 9, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

martinuy commented Apr 9, 2025

Uh oh!

djelinski commented Apr 9, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

valeriepeng commented Apr 10, 2025

Uh oh!

martinuy commented Apr 10, 2025

Uh oh!

martinuy commented Apr 10, 2025

Uh oh!

martinuy commented Apr 11, 2025

Uh oh!

djelinski left a comment

Choose a reason for hiding this comment

Uh oh!

valeriepeng commented Apr 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

HmacSHA256

PBEWithHmacSHA224AndAES_256

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

martinuy commented Apr 11, 2025

Uh oh!

martinuy commented Apr 8, 2025 •

edited by openjdk bot

Loading

openjdk bot commented Apr 8, 2025 •

edited

Loading

mlbridge bot commented Apr 8, 2025 •

edited

Loading

valeriepeng commented Apr 11, 2025 •

edited

Loading

`HmacSHA256`

`PBEWithHmacSHA224AndAES_256`

`HmacSHA256`

`PBEWithHmacSHA224AndAES_256`