-
Notifications
You must be signed in to change notification settings - Fork 6.3k
8314323: TLS 1.3 Hybrid Key Exchange #27614
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
base: master
Are you sure you want to change the base?
Conversation
👋 Welcome back hchao! A progress list of the required criteria for merging this PR into |
❗ This change is not yet ready to be integrated. |
@haimaychao The following label will be automatically applied to this pull request:
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. |
/contributor add @jnimeh |
/contributor add @wangweij |
@haimaychao |
@haimaychao |
algParams.init(keAlgParamSpec); | ||
// Skip AlgorithmParameters for KEMs (not supported) | ||
if (namedGroupSpec == NamedGroupSpec.NAMED_GROUP_KEM) { | ||
if (defaultProviderName == 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.
We assume that if provider is not null then it must be DH without doing any checks to confirm that. It would be cleaner to call getProvider() instead.
Provider p = getProvider();
if (p == null) {
KeyFactory.getInstance(name);
} else {
KeyFactory.getInstance(name, p);
}
|
||
private static KEM getKEM(String name) throws NoSuchAlgorithmException { | ||
if (name.startsWith("secp") || name.equals("X25519") || | ||
name.equals("X448")) { |
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 we need X448
here? Also, please provide a comment for this method with functionality description.
X25519(32, 32, | ||
"XDH", "XDH", NamedParameterSpec.X25519), | ||
|
||
X448(56, 56, |
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.
Why do we need X448
and P521
?
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.
Need, no. Want, yes. The support for traditional curves that are not part of the first round of hybrid KEMs lays the groundwork for future hybrid KEMs that might use these larger curves. It also gives us the base framework to move these algorithms as named groups to KEM implementations in the future.
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 see, thanks for the explanation! I guess it makes sense if we expect those curves to be used in the future rounds of hybrid KEM.
implements KEMSpi.EncapsulatorSpi, KEMSpi.DecapsulatorSpi { | ||
private final PublicKey pkR; | ||
private final PrivateKey skR; | ||
private final AlgorithmParameterSpec spec; |
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.
This variable is not being used, it can be removed together with the constructor parameter.
} | ||
} else if (k instanceof XECKey xkey | ||
&& xkey.getParams() instanceof NamedParameterSpec ns) { | ||
if (ns.getName().equalsIgnoreCase("X25519")) { |
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.
Maybe use NamedParameterSpec.X25519?
ProtocolVersion.PROTOCOLS_TO_13, | ||
PredefinedDHParameterSpecs.ffdheParams.get(8192)), | ||
|
||
ML_KEM_512(0x0200, "MLKEM512", |
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.
Are they needed for this Jep?
// Finite Field Groups (XDH) | ||
NAMED_GROUP_XDH("XDH", XDHScheme.instance), | ||
|
||
NAMED_GROUP_KEM("PQC", KEMScheme.instance), |
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.
That Choice of Name needs probably an explaining comment if it is for pure PQC and/ormhybrid?
} else { // default groups | ||
NamedGroup[] groups = new NamedGroup[] { | ||
|
||
// Hybrid key agreements |
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.
It feels like all the infra for X448MLKEM1024 is there, so rather than removing x448 from this patch, why not implement it (it’s more obvious than P511 Variants)
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.
Indeed the infrastructure is there, but I don't see an IETF draft that covers that hybrid variant for TLS, nor do I see an IANA mapping for it here: https://www.iana.org/assignments/tls-parameters/tls-parameters.xhtml#tls-parameters-8
There needs to be a standard for TLS 1.3 backing these hybrid KEMs before we implement them.
FFDHE_2048, | ||
FFDHE_3072, | ||
FFDHE_4096, | ||
FFDHE_6144, |
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.
Unrelated change?
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.
No, the choise to knock out ffdhe6144 and 8192 from the default list was done on purpose. I don't think they get much use and they can always be re-enabled via SSLParameters or the system property. We're open to feedback on this if you or others feel like they should remain in place, though.
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 change is I think ok, doesn’t make much of a difference for most cases I was just thinking it needed its own commit and ticket reference but if it was intentional fine as well.
} | ||
|
||
private static AlgorithmParameterSpec getSpec(String name) { | ||
if (name.startsWith("secp")) { |
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.
This seems to be repeated multiple times including the case sensitive string, maybe have an APS.isGenericEC() helper?
|
||
private static int leftPublicLength(String name) { | ||
return switch (name.toLowerCase(Locale.ROOT)) { | ||
case "secp256r1" -> 65; |
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.
Isn’t that Part formte named groups, maybe Look it up instead?
|
||
private static byte[] concat(byte[]... inputs) { | ||
ByteArrayOutputStream o = new ByteArrayOutputStream(); | ||
Arrays.stream(inputs).forEach(o::writeBytes); |
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 we really want a non presized buffer and a stream in the handshake hot path?
Implement hybrid key exchange support for TLS 1.3 by adding three post-quantum hybrid named groups: X25519MLKEM768, SecP256r1MLKEM768, and SecP384r1MLKEM1024.
Please see JEP 527 for details about this change.
Progress
Issue
Contributors
<[email protected]>
<[email protected]>
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/27614/head:pull/27614
$ git checkout pull/27614
Update a local copy of the PR:
$ git checkout pull/27614
$ git pull https://git.openjdk.org/jdk.git pull/27614/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 27614
View PR using the GUI difftool:
$ git pr show -t 27614
Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/27614.diff
Using Webrev
Link to Webrev Comment