Skip to content

Conversation

@valeriepeng
Copy link
Contributor

@valeriepeng valeriepeng commented Apr 2, 2025

This PR removes the internal JSSE HKDF impl and changes to use the KDF API for the HKDF support from JCA/JCE providers.

This is just code refactoring. Known-answer regression test for the internal JSSE HKDF impl is removed as the test vectors are already covered by the HKDF impl in SunJCE provider.

JPRT Tier1-3 result: https://mach5.us.oracle.com/mdash/jobs/vpeng-jdkOh3-20250512-2316-27739411

Thanks in advance for the review~


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-8353578: Refactor existing usage of internal HKDF impl to use the KDF API (Enhancement - P3)

Reviewers

Contributors

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 24393

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

Using diff file

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

Using Webrev

Link to Webrev Comment

@bridgekeeper
Copy link

bridgekeeper bot commented Apr 2, 2025

👋 Welcome back valeriep! 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.

@openjdk
Copy link

openjdk bot commented Apr 2, 2025

@valeriepeng 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:

8353578: Refactor existing usage of internal HKDF impl to use the KDF API

Co-authored-by: Kevin Driver <[email protected]>
Reviewed-by: djelinski, wetmore, mullan, kdriver, weijun

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 739 new commits pushed to the master branch:

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 openjdk bot added the rfr Pull request is ready for review label Apr 2, 2025
@openjdk
Copy link

openjdk bot commented Apr 2, 2025

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

@valeriepeng
Copy link
Contributor Author

/contributor driverkt

@openjdk
Copy link

openjdk bot commented Apr 2, 2025

@valeriepeng Syntax: /contributor (add|remove) [@user | openjdk-user | Full Name <email@address>]. For example:

  • /contributor add @openjdk-bot
  • /contributor add duke
  • /contributor add J. Duke <[email protected]>

User names can only be used for users in the census associated with this repository. For other contributors you need to supply the full name and email address.

@valeriepeng
Copy link
Contributor Author

/contributor add driverkt

@openjdk
Copy link

openjdk bot commented Apr 2, 2025

@valeriepeng driverkt was not found in the census.

Syntax: /contributor (add|remove) [@user | openjdk-user | Full Name <email@address>]. For example:

  • /contributor add @openjdk-bot
  • /contributor add duke
  • /contributor add J. Duke <[email protected]>

User names can only be used for users in the census associated with this repository. For other contributors you need to supply the full name and email address.

@mlbridge
Copy link

mlbridge bot commented Apr 2, 2025

@valeriepeng
Copy link
Contributor Author

/contributor add kdriver

@openjdk
Copy link

openjdk bot commented Apr 2, 2025

@valeriepeng
Contributor Kevin Driver <[email protected]> successfully added.

= hkdf.extract(zeros, ikm, "TlsEarlySecret");
SecretKey earlySecret = hkdf.deriveKey("TlsEarlySecret",
HKDFParameterSpec.ofExtract().addSalt(zeros)
.addIKM(ikm).extractOnly());
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe no need for addSalt(zeros). I remember salt is by default zeros for HKDF.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I am on the fence about this. Given the specified value is the same as the default, it can be removed. I kept it there so the new code matches the original code completely. Not much difference either way I think.

Copy link
Contributor

Choose a reason for hiding this comment

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

I like having it there to communicate that is really the intent.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok.

Copy link
Member

@djelinski djelinski left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks!

@openjdk openjdk bot added the ready Pull request is ready to be integrated label Apr 3, 2025
Copy link
Contributor

@bradfordwetmore bradfordwetmore left a comment

Choose a reason for hiding this comment

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

The rest looks good.

Nice to get this done finally!

static String digest2HKDF(String digestAlg) throws SSLHandshakeException {
String sanitizedAlg = digestAlg.replace("-", "");
return switch (sanitizedAlg) {
case "SHA256", "SHA384", "SHA512" -> "HKDF-" + sanitizedAlg;
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a nit, but currently we don't have SHA512 in CipherSuite.HashAlg. You can leave it for any future enhancements.

Copy link
Member

Choose a reason for hiding this comment

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

You could also consider storing the HKDF algorithm names in the HashAlg enum. Not sure if it would make much difference, performance wise.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, this sounds better for overall consistency. Will adopt the HashAlg enum suggestion.

= hkdf.extract(zeros, ikm, "TlsEarlySecret");
SecretKey earlySecret = hkdf.deriveKey("TlsEarlySecret",
HKDFParameterSpec.ofExtract().addSalt(zeros)
.addIKM(ikm).extractOnly());
Copy link
Contributor

Choose a reason for hiding this comment

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

I like having it there to communicate that is really the intent.

refactored code to remove unused AlgorithmParameterSpec argument.
@openjdk openjdk bot removed the ready Pull request is ready to be integrated label Apr 5, 2025
@Override
public SecretKey deriveKey(String algorithm,
AlgorithmParameterSpec params) throws IOException {
public SecretKey deriveKey(String type) throws IOException {
Copy link
Member

Choose a reason for hiding this comment

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

How about changing the variable name to typeNotUsed like SSLKeyDerivation.LegacyMasterKeyDerivation?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, missed this one.

AlgorithmParameterSpec params) throws IOException;
SecretKey deriveKey(String purpose) throws IOException;

default byte[] deriveData(String purpose) throws IOException {
Copy link
Member

Choose a reason for hiding this comment

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

This is an internal interface, so I don't think you need to make this a default method.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't add deriveData(String) impl to all the existing impls of SSLKeyDerivation. Only impls used for deriving IVs are updated to add impl for deriveData(String), so the default method is necessary.

Copy link
Member

@djelinski djelinski left a comment

Choose a reason for hiding this comment

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

Still looks good.

The changes here are not enough to get the NSS-FIPS library to complete TLS1.3 handshake, but it's a big step in the right direction. I think we can fix the remaining issues separately.

@openjdk openjdk bot added the ready Pull request is ready to be integrated label Apr 7, 2025
cleanup interim security values
changed PKCS11 HKDF impl to allow key algorithms starting with "Tls" as generic keys
@openjdk openjdk bot removed the ready Pull request is ready to be integrated label Apr 11, 2025
Copy link
Member

@djelinski djelinski left a comment

Choose a reason for hiding this comment

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

still LGTM.

} finally {
if (eae_prk instanceof SecretKeySpec s) {
SharedSecrets.getJavaxCryptoSpecAccess()
.clearSecretKeySpec(s);
Copy link
Member

Choose a reason for hiding this comment

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

I wish we could use s.destroy() here 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.

Yes, it'd be nice. I reopened https://bugs.openjdk.org/browse/JDK-8160206 and we can address this separately.

Copy link
Contributor

@bradfordwetmore bradfordwetmore May 7, 2025

Choose a reason for hiding this comment

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

Or in the meantime:

} finally {
    // Best effort
    if (eae_prk instanceof SecretKeySpec s) {
        SharedSecrets.getJavaxCryptoSpecAccess()
                .clearSecretKeySpec(s);
    } else {
        try {
            eae_prk.destroy();
        } catch (DestroyFailedException e) {
            // swallow
        }
    }
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good, thanks for the suggestion.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is what I did in my Exporter code. Assuming you go in first, I'll update mine to use your Util method.

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

// derive handshake secret
return hkdf.extract(saltSecret, sharedSecret, algorithm);
return hkdf.deriveKey(type, HKDFParameterSpec.ofExtract()
Copy link
Contributor

Choose a reason for hiding this comment

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

The line above may fail because the hkdf object has been used once on line 121 with zero-valued salt and IKM, which selected the software-based HKDF from SunJCE. At this point, however, sharedSecret is the result of ECDH and may be non-extractable if produced by an HSM, making it incompatible with the SunJCE implementation. To avoid this issue, get a new hkdf by calling hkdf = KDF.getInstance(hashAlg.hkdfAlgorithm) before this line.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, ok, interesting scenario. I add another KDF.getInstance() call as you suggested just to be safe.

Copy link
Member

Choose a reason for hiding this comment

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

I think that deserves a code comment; it's far from obvious why we do that. Also, we will make P11 work with zero-valued salt soon.

hkdfInfo, hashAlg.hashLength, "TlsBinderKey");
return hkdf.deriveKey("TlsBinderKey",
HKDFParameterSpec.expandOnly(earlySecret, hkdfInfo,
hashAlg.hashLength));
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it possible to combine the 2 deriveKey calls above into a single Extract-Then-Expand call? Then you don't need to clean up earlySecret.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should be possible, let me give it a try. Thanks~


private static SecretKey deriveBinderKey(HandshakeContext context,
SecretKey psk, SSLSessionImpl session) throws IOException {
SecretKey earlySecret = null;
Copy link
Member

Choose a reason for hiding this comment

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

unused variable, can delete.

import java.security.MessageDigest;
import java.security.NoSuchAlgorithmException;
import java.security.ProviderException;
import java.security.spec.AlgorithmParameterSpec;
Copy link
Member

Choose a reason for hiding this comment

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

Can remove import now.

}

SSLKeyDerivation handshakeKD = ke.createKeyDerivation(shc);
SecretKey handshakeSecret = handshakeKD.deriveKey(
Copy link
Member

Choose a reason for hiding this comment

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

It looks like this can be cleared after it is used to derive the key. Similar comment on line 1310.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, I am not sure if clearing handshakeSecret is ok - this handshakeSecret is passed to kd on line 636 and stored internally without cloning. Then kd is stored into shc which suggests that it may be used later. Clearing it will likely cause problems for subsequent key derivations? Same goes for line 1310. Is there something that I missed?

Copy link
Member

Choose a reason for hiding this comment

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

Ah, yes you are right.

@bradfordwetmore
Copy link
Contributor

bradfordwetmore commented Apr 30, 2025

Missing test plan in the PR Description. (i.e. tier1/tier2/JCK?)

@openjdk openjdk bot removed the ready Pull request is ready to be integrated label May 1, 2025
@valeriepeng
Copy link
Contributor Author

Missing test plan in the PR Description. (i.e. tier1/tier2/JCK?)

I always run tier 1-3 tests for all of my PRs. Don't anticipate that this would affect JCK, but will give it a try just in case.
Thanks for the suggestion~

Copy link
Contributor

@bradfordwetmore bradfordwetmore left a comment

Choose a reason for hiding this comment

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

Just a few comments to think about, and one copyright nit.

try {
HKDFParameterSpec spec =
HKDFParameterSpec.ofExtract().addIKM(s).extractOnly();
return hkdf.deriveKey("Generic", spec);
Copy link
Contributor

Choose a reason for hiding this comment

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

I haven't done much with DHKEM yet, but should the returned key have algorithm name of "Generic," or something more descriptive like the previous "HKDF-PRK"?

Copy link
Contributor Author

@valeriepeng valeriepeng May 8, 2025

Choose a reason for hiding this comment

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

Me neither. However, given HKDF-PRK is not a standard algorithm and also not recognized by the SunPKCS11 provider, I changed it to Generic. Existing HKDF impl in the SunPKCS11 provider is quite strict about the derived key algorithms and it will error out unless we add HKDF-PRK to be a recognized key algorithm for key derivation. Given these reasons, it seems Generic is the better choice here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Is any specific salt needed here like in TLS?

Copy link
Contributor

Choose a reason for hiding this comment

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

We should chat next week about an issue Weijun raised and the algorithm names in the Exporters.

Copy link
Contributor

Choose a reason for hiding this comment

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

Copyright update.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will fix.

byte[] zeros = new byte[hashAlg.hashLength];
SecretKey earlySecret = hkdf.extract(zeros, psk, "TlsEarlySecret");
KDF hkdf = KDF.getInstance(hashAlg.hkdfAlgorithm);
SecretKey earlySecret = hkdf.deriveKey("TlsEarlySecret",
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm a little worried that the proper number of salt zeros are now expected to be known in the KDF deriveKey code instead of specified specifically here (and in other similar places). Should we consider specifying them here and the other places instead to play it safe?

Copy link
Contributor

@bradfordwetmore bradfordwetmore May 7, 2025

Choose a reason for hiding this comment

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

I just found that we had talked about this previously. What was your reasoning for pulling it?

Call me paranoid, but I'm not seeing where the JDK 24 javadocs discuss what happens if salt is not supplied. RFC 8446/Section 7.1 states:

 -  "0" indicates a string of Hash.length bytes set to zero.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I will add it back just to be safe.

Copy link
Contributor

Choose a reason for hiding this comment

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

I thought there were other locations, but maybe I was just imagining it! ;)

try {
HKDFParameterSpec spec =
HKDFParameterSpec.ofExtract().addIKM(s).extractOnly();
return hkdf.deriveKey("Generic", spec);
Copy link
Contributor

Choose a reason for hiding this comment

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

Is any specific salt needed here like in TLS?

return alg.equalsIgnoreCase("TlsPremasterSecret")
|| alg.equalsIgnoreCase("Generic");
}

Copy link
Contributor

Choose a reason for hiding this comment

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

As you know, I've been working on the TLS Exporters change which will use the same KDF APIs. I've already updated that to use your style.

Looks like I've now got one more thing to change! ;)

Copy link
Contributor

Choose a reason for hiding this comment

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

This is a reply to the comment above. I don't know why GitHub does not show a reply box there.

Is any specific salt needed here like in TLS?

In DHKEM, the salt used is always empty.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So, no need to explicitly set it I assume? As this is a refactoring, I prefer to minimize changes unless consensus is different.

byte[] zeros = new byte[hashAlg.hashLength];
SecretKey earlySecret = hkdf.extract(zeros, psk, "TlsEarlySecret");
KDF hkdf = KDF.getInstance(hashAlg.hkdfAlgorithm);
SecretKey earlySecret = hkdf.deriveKey("TlsEarlySecret",
Copy link
Contributor

Choose a reason for hiding this comment

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

I thought there were other locations, but maybe I was just imagining it! ;)

} finally {
if (eae_prk instanceof SecretKeySpec s) {
SharedSecrets.getJavaxCryptoSpecAccess()
.clearSecretKeySpec(s);
Copy link
Contributor

Choose a reason for hiding this comment

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

This is what I did in my Exporter code. Assuming you go in first, I'll update mine to use your Util method.

try {
HKDFParameterSpec spec =
HKDFParameterSpec.ofExtract().addIKM(s).extractOnly();
return hkdf.deriveKey("Generic", spec);
Copy link
Contributor

Choose a reason for hiding this comment

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

We should chat next week about an issue Weijun raised and the algorithm names in the Exporters.

@openjdk openjdk bot added the ready Pull request is ready to be integrated label May 10, 2025
@bradfordwetmore
Copy link
Contributor

You're probably good to go, but might check with Weijun/Sean/DJ in case there's anything last minute.

@driverkt
Copy link
Member

I'll be looking too, but don't hold up integration to wait for my review.


// derive handshake secret
return hkdf.extract(saltSecret, sharedSecret, algorithm);
// NOTE: do not reuse the HKDF object for "TlsEarlySecret" for
Copy link
Member

Choose a reason for hiding this comment

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

Nit: There is no longer an "HKDF object". Might be worth updating this comment.

Copy link
Contributor Author

@valeriepeng valeriepeng May 12, 2025

Choose a reason for hiding this comment

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

It actually means the "KDF object w/ HKDF algorithm", I can see how that may look confusing. I can change it to "KDF object", but then it'd require everyone to re-review again. Given this is just a nit, can we leave it for later?

Comment on lines +48 to +49
this.hkdfInfo = createHkdfInfo(label, context, hashAlg.hashLength);
this.keyLen = hashAlg.hashLength;
Copy link
Member

Choose a reason for hiding this comment

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

Very minor nit: might be worth accessing this field once and passing this.keyLen to createHkdfInfo 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.

Hmm, I like to match the ordering in the constructor with the ordering of fields. Since this is relatively minor, I am inclined to leave it as is...

((SecretSizeSpec)keySpec).length, algorithm);
KDF hkdf = KDF.getInstance(hkdfAlg);
return hkdf.deriveKey(type,
HKDFParameterSpec.expandOnly(secret, hkdfInfo, keyLen));
Copy link
Member

Choose a reason for hiding this comment

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

I noticed we have done away with the AlgorithmParameterSpec parameter to this method. While the new implementation makes sense in light of the new parameter set, I wonder whether there was a deliberate use-case lost here...

In the previous implementation, presumably, if the keySpec's length was shorter than the expanded value, only a portion of the value would be used. With the new implementation, the hash algorithm's length of bytes is always used.

I'm not sure how relevant this is, but it could be worth noting. If we had kept the old parameter, I suppose we could have used deriveData and truncated the result to ((SecretSizeSpec)keySpec).length before returning a SecretKey.

Copy link
Contributor Author

@valeriepeng valeriepeng May 12, 2025

Choose a reason for hiding this comment

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

Searching among the JSSE code base, it looks that SSLBasicKeyDerivation class is only used in Finished class and the particular SecretSizeSpec value is always same as the current keyLen field. There is no other usage as far as I can tell. If we ended up using SSLBasicKeyDerivation class with a shorter key length, we can add an explicit keyLen argument to the SSLBasicKeyDerivation constructor. I see no need for the SecretSizeSpec class given the code flow in Finished class.

Copy link
Member

@driverkt driverkt left a comment

Choose a reason for hiding this comment

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

See inline.

Copy link
Member

@driverkt driverkt left a comment

Choose a reason for hiding this comment

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

Approving based on above discussion.

@valeriepeng
Copy link
Contributor Author

/integrate

@openjdk
Copy link

openjdk bot commented May 13, 2025

Going to push as commit 4c0a0ab.
Since your change was applied there have been 748 commits pushed to the master branch:

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot added the integrated Pull request has been integrated label May 13, 2025
@openjdk openjdk bot closed this May 13, 2025
@openjdk openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels May 13, 2025
@openjdk
Copy link

openjdk bot commented May 13, 2025

@valeriepeng Pushed as commit 4c0a0ab.

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

integrated Pull request has been integrated security [email protected]

Development

Successfully merging this pull request may close these issues.

6 participants