Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8328119: Support HKDF in SunPKCS11 (Preview) #22215

Closed
wants to merge 14 commits into from
+1,582 −66
Closed

8328119: Support HKDF in SunPKCS11 (Preview) #22215

Changes from 1 commit
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
e87ec99
8328119: SunPKCS#11 implementation of HKDF
martinuy Nov 18, 2024
0c13f2c
Algorithms renamed for alignment with SunJCE.
martinuy Nov 18, 2024
453aa24
Derivation of empty base keys.
martinuy Dec 11, 2024
aab90aa
Make EMPTY_KEY static.
martinuy Dec 11, 2024
bfe052f
Fix NPE in assertion.
martinuy Dec 12, 2024
c5750f0
Unused import removed.
martinuy Dec 18, 2024
52eb82f
Support for HKDF-SHA1 and HKDF-SHA224 removed.
martinuy Dec 18, 2024
af30668
Check disabled PKCS #11 mechanisms when concatenating keys and data.
martinuy Dec 19, 2024
b0eb1df
Minor changes based on review. Copyright date adjustments.
martinuy Jan 7, 2025
870230b
Renaming of P11KDF fix.
martinuy Jan 8, 2025
699683e
Improve TestContext note about expectedOpOut
franferrax Jan 14, 2025
a7c82ee
Add test case with empty inputs
franferrax Jan 17, 2025
87f4820
Align behaviour with the CSR
franferrax Feb 5, 2025
d018b4a
Merge openjdk/master into JDK-8328119
franferrax Feb 5, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 21 additions & 5 deletions src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11KDF.java
View file
Original file line number Diff line number Diff line change
@@ -43,6 +43,22 @@ final class P11KDF extends KDFSpi {
private final Token token;
private final P11SecretKeyFactory.HKDFKeyInfo svcKi;
private final long hmacMechanism;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

svcki and hmacMechanism are HKDF specific, so maybe this class should be named P11HKDF instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Our original intention was to keep it more open but it's true that in the end it's HKDF-specific. I'll rename it.

private final SecretKey EMPTY_KEY = new SecretKey() {
@Override
public String getAlgorithm() {
return "Generic";
}

@Override
public String getFormat() {
return "RAW";
}

@Override
public byte[] getEncoded() {
return new byte[0];
}
};

private static KDFParameters requireNull(KDFParameters kdfParameters,
Copy link
Contributor

@valeriepeng valeriepeng Jan 4, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would void be better since we are not using KDFParameters and require it to be null? I guess the reason for doing this is to fit this method inside the protected constructor.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We want to make sure that the KDFParameters value passed to the protected constructor is null. In the process of passing null, we check that the P11KDF constructor caller did not pass a non-null value. The method requireNull serves the purpose of ensuring that null is passed, that KDFParameters kdfParameters is null and allows code execution before the call the super class constructor, so the token, svcKi and hmacMechanism fields can be final. While null is returned, the value is technically used because it's received by the super class constructor.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, ok.

String message) throws InvalidAlgorithmParameterException {
@@ -139,7 +155,7 @@ private <T> T derive(String alg, AlgorithmParameterSpec derivationSpec,
if (salt instanceof SecretKeySpec) {
saltType = CKF_HKDF_SALT_DATA;
saltBytes = salt.getEncoded();
} else if (salt != null) {
} else if (salt != EMPTY_KEY) {
// consolidateKeyMaterial returns a salt from the token.
saltType = CKF_HKDF_SALT_KEY;
p11SaltKey = (P11Key.P11SecretKey) salt;
@@ -185,8 +201,8 @@ private <T> T derive(String alg, AlgorithmParameterSpec derivationSpec,
token.p11.C_DestroyObject(session.id(), derivedObjectID);
}
} else {
ret = P11Key.secretKey(session, derivedObjectID, alg, outLen,
null);
ret = P11Key.secretKey(session, derivedObjectID, alg,
outLen * 8, null);
}
return retType.cast(ret);
} catch (PKCS11Exception e) {
@@ -243,7 +259,7 @@ protected final P11Key.P11SecretKey p11Merge(
long derivedKeyID = token.p11.C_DeriveKey(session.id(), ckMech,
baseKeyID, attrs);
return (P11Key.P11SecretKey) P11Key.secretKey(session,
derivedKeyID, "Generic", derivedKeyLen, null);
derivedKeyID, "Generic", derivedKeyLen * 8, null);
} catch (PKCS11Exception e) {
throw new ProviderException("Failure when merging key " +
"material.", e);
@@ -265,7 +281,7 @@ protected KeyMaterialMerger merge(P11Key.P11SecretKey nextKeyMaterial) {
}

SecretKey getKeyMaterial() {
return null;
return EMPTY_KEY;
}
}

17 changes: 16 additions & 1 deletion test/jdk/sun/security/pkcs11/KDF/TestHKDF.java
View file
Original file line number Diff line number Diff line change
@@ -132,7 +132,7 @@ private static List<AlgorithmParameterSpec> generateKdfParamSpecs(
if (baseKey instanceof SecretKeySpec) {
addKeyMaterial(keyMaterialCombination, baseKey.getEncoded(),
b::addIKM, b::addIKM);
} else {
} else if (baseKey != null) {
b.addIKM(baseKey);
}
if (ctx.salt != null) {
@@ -611,6 +611,21 @@ private static void test_AES_HKDFWithHmacSHA512() {
"63fc");
}

private static void test_AES_HKDFWithHmacSHA256_EmptyBaseKey() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Glad to see this one here. Is there one for null or empty salt?

Where are the KA values coming from?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have tests in which salt is null (i.e. not calling addSalt). If you think it is worth it, we can combine that with an empty key. The effect of an empty salt (instead of null) is the same as not calling addSalt. Notice that we cannot create an instance of a SecretKeySpec with an empty byte[] as key.

Do you mean the values for the DH/ECDH key agreement?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In a7c82ee, we've just added a new test case passing an empty base key, salt, and info.

executeTest("AES - HKDF-SHA256 (empty base key)",
"HKDF-SHA256",
"AES",
(SecretKey) null,
"101112131415161718191a1b1c1d1e1f",
"a0a1a2a3a4a5a6a7a8a9aaabacadaeaf",
"cc267bd9515c1eba2cf6aaa1fc8380677f4351fcbea6d70873df5a334efc" +
"ee0d",
"cf353a33460b146c0eae3f0788ee281e5a0be15280fbeba107472aa1cd58" +
"d111",
"326e9028f51c05c1919215bad6e35668c94c88040c3777e8e6f8b6acdece" +
"85fa");
}

private static void test_HKDF_after_DH_HkdfSHA256() throws Exception {
SecretKey tlsPremasterSecret = getTlsPremasterSecretWithDHExchange(
"00bcb8fa0a6b569961782a394599a1a02a05532a836819908a9a9000ed",