Updated webrev:
http://cr.openjdk.java.net/~wetmore/8171279/webrev.01/
Xuelei wrote:
> ECDHClientKeyExchange.java
> --------------------------
> The enum builtin method valueOf(String) is used. There is not problem
> here. But as requires the enum name in NamedGroup is exactly the same as
> the name defined in NamedParameterSpec. It might be a potential risk
> for future update of the names.
Unfortunately, all I have available is the key.getAlgorithm() and the
key.getParams() String from the NamedParameterSpec at this point, so
I've switched to NamedGroup.nameOf(String) which is what other places in
the code do.
> ECDHKeyExchange.java
> --------------------
> 471 break search;
>
> The use of break label branching makes me a little bit nervous. I'm
> fine if you like it.
This is one of the rare circumstances where this construct is
appropriate, as the alternative is harder to read. Will leave as is for
now.
Sean Mullan wrote:
Just noticed a couple things so far:
Is there more coming? I plan to putback tomorrow to make RDP1. We can
do an RFE during RDP1->RDP2 if you find something.
* CipherSuite.java
1139 this.allowed = JsseJce.ALLOW_ECC;
Any reason why you don't just pass this in as the value of the allowed
parameter? Then you don't need to iterate over the array.
Done.
* XECParameters.java
2 * Copyright (c) 2019, Oracle and/or its affiliates. All rights reserved.
Should retain 2018.
Missed this, it now says 2018, 2019,
Brad
--Sean
On 6/6/19 4:27 PM, Bradford Wetmore wrote:
Good morning/afternoon/evening/night,
This RFE adds TLS protocol versions 1->1.3 support for the x25519/x448
curves in the SunJSSE provider. These algorithms are preferred by
many of the major browsers for their efficiency and security properties.
This work is the natural extension of JDK-8181595/JEP 324[2] which
added the RFC 7748 [1] Curve25519 and Curve448 (ECDH) to the Java
Crypto Mechanism in JDK 11.
RFE: https://bugs.openjdk.java.net/browse/JDK-8171279
CSR: https://bugs.openjdk.java.net/browse/JDK-8224520
Webrev: http://cr.openjdk.java.net/~wetmore/8171279/webrev.00/
There is a fair amount of change, so the webrevs are hard to follow in
spots. I'll introduce many of the implementation changes here:
1. The JCE design choice of using a parallel set of APIs and Names
ECPublicKey/XECPublicKey, KeyExchange.getInstance("EC"/"XDH"), etc.
for these curves made things more challenging than expected.
2. The key agreement key derivation code was identically replicated 3
times (FFDHE/ECDHE/XDH), so this was consolidated into KAKeyDerivation.
3. NamedGroups were broken out of SupportedGroupsExtension.
3a. Added a function table to do NamedGroup operations in an
algorithm-independent and consistent fashion for FFDHE/ECDHE/XDH.
Things like checking Constraints, getting parameters,
encoding/decoding Possession keys, creating possession, and whether
the algorithms are available.
For example, given a negotiated group:
namedGroup->encodePossessionPublicKey->Functions->
((XDHKeyExchange.XDHEPossession)poss).encode()
Both the ECDHE/XDH functions call into the existing ECDHE code, as
they use some of the same underlying structures/messages, which needed
to be updated a bit to be algorithm independent. This is for TLSv1->1.3.
The FFDHE functions for TLSv1.3 also call into the existing DH code,
but I did not update the existing FFDHE code for TLSv1-1.2 to use the
function tables since it was not necessary. If reviewers feel this is
desired, I can do this as an enhancement later.
4. Added NamedGroupPossession/NamedGroupCredentials interfaces to
make code cleaner/less error prone.
5. The ciphersuite KeyExchanges (K_EC*_*) were updated to reflect
that either ECDHE/XDH can be used.
6. Create a new ECDHKeyExchange.ECDHEXDHKAGenerator class to handle
creating TLSv1-1.2 KeyDerivations, based on whether it was ECDHE/XDH.
7. RFC 8422 (Appendix B) deprecated/removed the TLS_ECDH_*
ciphersuites. Our KeyManager APIs currently do not allow for
selecting specific curve entries. I've made a best effort for
supporting client-side ECDH, but we won't support server-side ECDH at
this point. TBD if we'll add support as API changes will be
necessary, and not be worth the time if no one should/will be using ECDH.
8. Is there a common byte array reverse function? Seems so simple,
but seems to be duplicated in different modules/packages.
9. A few minor tweaks that helped code readability (typos/comments). ;)
Thanks to Adam Petcher, who started work on this project last year. I
took several of his ideas in my final implementation.
Thanks,
Brad
[1] https://tools.ietf.org/html/rfc7748
[2] https://bugs.openjdk.java.net/browse/JDK-8181595
