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

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.

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

Ok.

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

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

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

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

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

Yes, missed this one.

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

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.

I wish we could use s.destroy() here instead.

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

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

Sounds good, thanks for the suggestion.

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

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.

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

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.

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.

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

unused variable, can delete.

Can remove import now.

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

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?

Ah, yes you are right.

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"?

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.

Is any specific salt needed here like in TLS?

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

Copyright update.

Will fix.

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?

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.

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

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

Is any specific salt needed here like in TLS?

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! ;)

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.

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

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

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

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

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

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?

Very minor nit: might be worth accessing this field once and passing this.keyLen to createHkdfInfo instead.