-
Notifications
You must be signed in to change notification settings - Fork 6.2k
8328119: Support HKDF in SunPKCS11 (Preview) #22215
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.
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
Conversation
Co-authored-by: Martin Balao Alonso <[email protected]> Co-authored-by: Francisco Ferrari Bihurriet <[email protected]>
|
👋 Welcome back mbalao! A progress list of the required criteria for merging this PR into |
|
@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: 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 83 new commits pushed to the
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 |
Webrevs
|
Co-authored-by: Martin Balao Alonso <[email protected]> Co-authored-by: Francisco Ferrari Bihurriet <[email protected]>
| } | ||
|
|
||
| SecretKey getKeyMaterial() { | ||
| return null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the ikms list is empty, this would return a null key and convertKey does not like it. You might say this should not happen, but it seems the RFC has not forbidden this and the one in SunJCE does support it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right, good catch!
| token.p11.C_DestroyObject(session.id(), derivedObjectID); | ||
| } | ||
| } else { | ||
| ret = P11Key.secretKey(session, derivedObjectID, alg, outLen, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the key length in bytes or bits here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good question. Should be converted to bits. The same issue repeats one more time.
|
I set CKA_EXTRACTABLE = false and generate an AES key and its Update: I read https://docs.oasis-open.org/pkcs11/pkcs11-spec/v3.1/csd01/pkcs11-spec-v3.1-csd01.html#_Toc98177685 and https://docs.oasis-open.org/pkcs11/pkcs11-profiles/v3.1/os/pkcs11-profiles-v3.1-os.html#_Toc142307348 and it shows that some |
Yes, my interpretation is that data (the derivation output in this case) is fine. What shouldn't happen is to have a non-extractable key derived extracted. |
Conversion of bytes to bits when calling P11Key::secretKey. Co-authored-by: Martin Balao Alonso <[email protected]> Co-authored-by: Francisco Ferrari Bihurriet <[email protected]>
Co-authored-by: Martin Balao Alonso <[email protected]> Co-authored-by: Francisco Ferrari Bihurriet <[email protected]>
|
|
||
| long saltType = CKF_HKDF_SALT_NULL; | ||
| byte[] saltBytes = null; | ||
| P11Key p11SaltKey = null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For Expand, salt is null.
Did you test with assertion on?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For Expand, salt should be null. Do you mean adding or hitting an assertion?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Assertion on line 162 will throw an NPE.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, you're right, the assertion throws an NPE. Fixed.
Co-authored-by: Martin Balao Alonso <[email protected]> Co-authored-by: Francisco Ferrari Bihurriet <[email protected]>
| d(KDF, "HKDF-SHA384", P11KDF, m(CKM_SHA384_HMAC), | ||
| m(CKM_HKDF_DERIVE, CKM_HKDF_DATA)); | ||
| d(KDF, "HKDF-SHA512", P11KDF, m(CKM_SHA512_HMAC), | ||
| m(CKM_HKDF_DERIVE, CKM_HKDF_DATA)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We only defined HKDF-SHA256 and later in the Java Security Standard Names doc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We included SHA1 because there could be a legacy use case to support and it's part of the test vectors for RFC 5869 (HMAC-based Extract-and-Expand Key Derivation Function (HKDF)). We don't recommend using it, and will probably filter it out once we have the Filter integrated, but would you be okay with keeping it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you have any data on how many legacy use cases use it? I think for new mechanisms we should be forward looking and refrain from adding support for weak or not recommended algorithms unless there is a very good reason. It is often harder to remove something than to add it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I see your point. We don't have data to back it up. If someone comes up with a strong case, we can reconsider it in the future. We removed it.
|
I have never been a PKCS11 expert, but the code looks fine to me. The |
|
@driverkt should review this too. |
|
It's on my list, thanks. |
Co-authored-by: Martin Balao Alonso <[email protected]> Co-authored-by: Francisco Ferrari Bihurriet <[email protected]>
Thanks for your review. Unused import removed. |
| d(SKF, "ChaCha20", P11SecretKeyFactory, | ||
| m(CKM_CHACHA20_POLY1305)); | ||
| d(SKF, "Generic", P11SecretKeyFactory, | ||
| m(CKM_GENERIC_SECRET_KEY_GEN)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How useful is this? Is it only used to import a "Generic" SecretKeySpec into a token? I see it's used in the test when adding a key. Can you simply add the SecretKeySpec key there?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generic is a native PKCS11 key type (CKK_GENERIC_SECRET) that could have been added to SunPKCS11 before, irrespective of HKDF. It's convenient for the test to have key material in the token and test consolidation (IKM or salt).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you think we can add it to the Java Security Standard Names document?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, sure. I added a comment to the Solution section of the CSR and the name to the table in Specification.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BTW, what else can this key be used? I tried in HmacSHA256 and there is a CKR_KEY_TYPE_INCONSISTENT error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @wangweij,
What test have you executed? I'm able to use "Generic" keys for HmacSHA256, in a local slowdebug build of this branch.
cat >providersList.properties <<'EOF'
security.provider.1=SunPKCS11 --\\n\
name = NSS\\n\
nssLibraryDirectory = /usr/lib64\\n\
nssDbMode = noDb
security.provider.2=SUN
security.provider.3=SunRsaSign
security.provider.4=SunEC
security.provider.5=SunJSSE
security.provider.6=SunJCE
security.provider.7=SunJGSS
security.provider.8=SunSASL
security.provider.9=XMLDSig
security.provider.10=SunPCSC
security.provider.11=JdkLDAP
security.provider.12=JdkSASL
EOFcat >Main.java <<'EOF'
import java.util.HexFormat;
import javax.crypto.Mac;
import javax.crypto.SecretKey;
import javax.crypto.SecretKeyFactory;
import javax.crypto.spec.SecretKeySpec;
public final class Main {
public static void main(String[] args) throws Exception {
byte [] keyMaterial = "Secret-Bytes".getBytes();
SecretKeySpec spec = new SecretKeySpec(keyMaterial, "Generic");
SecretKeyFactory skf = SecretKeyFactory.getInstance("Generic");
SecretKey sk = skf.generateSecret(spec);
System.out.println(sk);
Mac mac = Mac.getInstance("HmacSHA256");
mac.init(sk);
mac.update("test".getBytes());
System.out.println(HexFormat.of().formatHex(mac.doFinal()));
}
}
EOF./build/linux-x86_64-server-slowdebug/images/jdk/bin/java \
-Djava.security.properties=providersList.properties Main.java
rm providersList.properties Main.javaOutput:
SunPKCS11-NSS Generic secret key, 96 bits session object, not sensitive, extractable)
c5dca603b87a1a1fe264f3cab2f851d513afdd2a7dd5ed3ee337356e2d7a001a
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried on my machine and see the same result. However, at least on my machine, Mac.getInstance actually chose the SunJCE implementation. If I explicitly getInstance from SunPKCS11-NSS I see the error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The key has to have CKA_SIGN = true in order to be used for a HMAC operation in NSS. For example, you can modify the code snippet shared by @franferrax to include the line Mac mac = Mac.getInstance("HmacSHA256", "SunPKCS11-NSS"); in Main.java (instead of Mac mac = Mac.getInstance("HmacSHA256");) and the line attributes = compatibility in providersList.properties. With these changes, I get the following output:
./bin/java -Djava.security.properties=providersList.properties Main.java
SunPKCS11-NSS Generic secret key, 96 bits session object, not sensitive, extractable)
c5dca603b87a1a1fe264f3cab2f851d513afdd2a7dd5ed3ee337356e2d7a001a
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it works now with the attributes = compatibility line. Told you I am not an expert. Thanks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi, sorry for the confusion. Even when SunPKCS11 was the first provider in the list, I should have checked what provider was implementing the Mac.
In my previous example, due to delayed provider selection, the provider is chosen during the mac.init(sk) call. SunPKCS11 throws the InvalidKeyException you were reproducing (caused by PKCS11Exception: CKR_KEY_TYPE_INCONSISTENT), but this exception is ignored and SunJCE is finally selected. This doesn't happen if attributes = compatibility or attributes(*,CKO_SECRET_KEY,CKK_GENERIC_SECRET)={ CKA_SIGN=true } is used.
Co-authored-by: Martin Balao Alonso <[email protected]> Co-authored-by: Francisco Ferrari Bihurriet <[email protected]>
|
Just curious, if I disable the |
Yes, you're right. The mechanism, by design, can be used to avoid registering services for which there is no support in the token or has been disabled in the configuration. However, we decided not to make |
|
/label remove core-libs |
wangweij
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
valeriepeng
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updates look fine.
Yes.
@valeriepeng @driverkt @wangweij - can you please review this CSR? |
Make a PKCS openjdk#11 library supporting only CKM_HKDF_DERIVE or CKM_HKDF_DATA to also register the HKDF algorithms. Notice that the corresponding HMAC mechanism is still required. In case only one of the mechanisms is available, KDF::deriveKey or KDF::deriveData will fail at run time. If the user disables one of CKM_HKDF_DERIVE or CKM_HKDF_DATA through sun.security.pkcs11.Config, also refrain from using it. Co-authored-by: Martin Balao Alonso <[email protected]> Co-authored-by: Francisco Ferrari Bihurriet <[email protected]>
Fix trivial copyright conflict in: src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11SecretKeyFactory.java
|
Hi @valeriepeng, @driverkt, @wangweij, We needed to make more changes to align the behavior with the CSR description, see 87f4820. Would you mind giving it a look? Please note that the CSR has also been updated. |
|
CSR and PR were re-approved since our last changes. We intend to integrate this PR soon, if no one has any other concern. |
|
/author set @franferrax |
|
/author @martinuy |
|
@martinuy |
|
@martinuy |
|
/contributor add fferrari |
|
/contributor add mbalao |
|
@martinuy |
|
@martinuy |
|
/integrate |
|
Going to push as commit 6ddbcc3.
Your commit was automatically rebased without conflicts. |
We would like to propose an implementation of the HKDF algorithms for SunPKCS11, aligned with the KDF API proposed for JDK 24 (see JEP 478: Key Derivation Function API (Preview)).
This implementation will be under the Preview umbrella until the KDF API becomes stable in a future JDK release. The benefit of this early proposal is to gather more feedback about the KDF API for future improvements.
The
P11KDFclass has the core implementation and Java calls to the PKCS 11 API. Different native mechanism were used to merge key material: CKM_CONCATENATE_BASE_AND_DATA (key and data), CKM_CONCATENATE_BASE_AND_KEY (key and key) and CKM_CONCATENATE_DATA_AND_BASE (data and key). The implementation also supports merging data with data, at the Java level. List of HKDF algorithms supported: HKDF-SHA256, HKDF-SHA384, and, HKDF-SHA512.Derivation modes supported: extract, expand, and, extract-expand.
We further advanced the consolidation of algorithm and key info in the P11SecretKeyFactory map —this effort started with the PBE support enhancement and has helped to avoid duplication—. The map has now information about HMAC (
HMACKeyInfoclass) and HKDF (HKDFKeyInfoclass) algorithms. P11Mac is now aligned to take the information from the map.Generic keys now supported in SecretKeyFactory. Derived keys could be Generic.
Testing
TestHKDF.java test added
No regressions observed in jdk/sun/security/pkcs11 (114 tests passed, 0 failed)
A CSR will be proposed.
This proposal is a contribution of @martinuy and @franferrax .
Progress
Issues
Reviewers
Contributors
<[email protected]><[email protected]>Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/22215/head:pull/22215$ git checkout pull/22215Update a local copy of the PR:
$ git checkout pull/22215$ git pull https://git.openjdk.org/jdk.git pull/22215/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 22215View PR using the GUI difftool:
$ git pr show -t 22215Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/22215.diff
Using Webrev
Link to Webrev Comment