Re: Java 11 RC build - HTTPS handshake failure against a previously working server
Sending "supported_groups" in ServerHello does not comply to the
extension specification. Is it possible the HTTPS server fix this problem?
I filed a bug in OpenJDK for the tracking:
https://bugs.openjdk.java.net/browse/JDK-8209965
Thanks,
Xuelei
On 8/25/2018 5:03 AM, Jaikiran Pai wrote:
As noted in that exception message, it appears that the server is
sending a "supported_groups" extension in its ServerHello message
(TLSv1.2). Reading about it, this seems to be a common issue with
certain servers and certain SSL implementations have added support to be
lenient with such servers https://github.com/openssl/openssl/pull/4463/files
-Jaikiran
On 25/08/18 11:58 AM, Jaikiran Pai wrote:
While testing the recently released RC of JDK11 against the Apache Ant
project, I happened to run into an odd error. I have now been able to
reproduce this using the following, pretty trivial code:
import java.net.URL;
import java.io.InputStream;
public class Fetch {
public static void main(final String[] args) throws Exception {
final URL targetURL = new
URL("https://repository.jboss.org/nexus/content/groups/public/javax/media/jai-core/1.1.3/jai-core-1.1.3.pom";);
try (final InputStream is =
targetURL.openConnection().getInputStream()) {
is.read();
}
System.out.println("Done");
}
}
All it does is opens a (HTTPS) connection against an endpoint to read
some content. This code works fine in Java 8 and even Java 10. I'm
pretty sure this was working fine even in Java 11 early access builds,
but I don't have any such build/binary at hand to be certain.
However, using the latest (OpenJDK) RC of Java 11 (both on Mac OS and
Linux) downloaded from[1]:
openjdk version "11" 2018-09-25
OpenJDK Runtime Environment 18.9 (build 11+28)
OpenJDK 64-Bit Server VM 18.9 (build 11+28, mixed mode)
it fails with:
Exception in thread "main" javax.net.ssl.SSLHandshakeException:
extension (10) should not be presented in server_hello
at java.base/sun.security.ssl.Alert.createSSLException(Alert.java:128)
at java.base/sun.security.ssl.Alert.createSSLException(Alert.java:117)
at
java.base/sun.security.ssl.TransportContext.fatal(TransportContext.java:308)
at
java.base/sun.security.ssl.TransportContext.fatal(TransportContext.java:264)
at
java.base/sun.security.ssl.TransportContext.fatal(TransportContext.java:255)
at
java.base/sun.security.ssl.SSLExtensions.(SSLExtensions.java:71)
at
java.base/sun.security.ssl.ServerHello$ServerHelloMessage.(ServerHello.java:173)
at
java.base/sun.security.ssl.ServerHello$ServerHelloConsumer.consume(ServerHello.java:864)
at
java.base/sun.security.ssl.SSLHandshake.consume(SSLHandshake.java:392)
at
java.base/sun.security.ssl.HandshakeContext.dispatch(HandshakeContext.java:444)
at
java.base/sun.security.ssl.HandshakeContext.dispatch(HandshakeContext.java:421)
at
java.base/sun.security.ssl.TransportContext.dispatch(TransportContext.java:178)
at java.base/sun.security.ssl.SSLTransport.decode(SSLTransport.java:164)
at
java.base/sun.security.ssl.SSLSocketImpl.decode(SSLSocketImpl.java:1152)
at
java.base/sun.security.ssl.SSLSocketImpl.readHandshakeRecord(SSLSocketImpl.java:1063)
at
java.base/sun.security.ssl.SSLSocketImpl.startHandshake(SSLSocketImpl.java:402)
at
java.base/sun.net.www.protocol.https.HttpsClient.afterConnect(HttpsClient.java:567)
at
java.base/sun.net.www.protocol.https.AbstractDelegateHttpsURLConnection.connect(AbstractDelegateHttpsURLConnection.java:185)
at
java.base/sun.net.www.protocol.http.HttpURLConnection.getInputStream0(HttpURLConnection.java:1581)
at
java.base/sun.net.www.protocol.http.HttpURLConnection.getInputStream(HttpURLConnection.java:1509)
at
java.base/sun.net.www.protocol.https.HttpsURLConnectionImpl.getInputStream(HttpsURLConnectionImpl.java:245)
at Fetch.main(Fetch.java:7)
[1] http://jdk.java.net/11/
-Jaikiran
Re: Java 11 RC build - HTTPS handshake failure against a previously working server
Hi Jaikiran,
Could you build JDK 11 or JDK 12 from source code? I had a patch to
tolerate the extension in ServerHello handshake message. Please let me
know if it works or not.
If there are any other JDK 11 TLS problems with Apache Ant project, I'd
like to know as well.
Thanks,
Xuelei
On 8/25/2018 7:04 AM, Jaikiran Pai wrote:
Hi Xuelei,
On 25/08/18 7:20 PM, Xuelei Fan wrote:
Sending "supported_groups" in ServerHello does not comply to the
extension specification.
Agreed. However, given that both the client and server are using TLSv1.2
and this seems to be "working" before the newer TLSv1.3 changes, even in
recent JDK versions, is there a way the implementation could workaround
this so as to allow JDK 11 to communicate with such servers?
Is it possible the HTTPS server fix this problem?
I don't have access or control over that server, so don't really know
how it's configured or whether it can be fixed. It's a pretty frequently
used Maven repository hosted by the JBoss (Red Hat middleware) project
team. I suspect, it's not just limited to this server and could be a
common issue with some other servers too.
I filed a bug in OpenJDK for the tracking:
https://bugs.openjdk.java.net/browse/JDK-8209965
Thank you.
-Jaikiran
Thanks,
Xuelei
On 8/25/2018 5:03 AM, Jaikiran Pai wrote:
As noted in that exception message, it appears that the server is
sending a "supported_groups" extension in its ServerHello message
(TLSv1.2). Reading about it, this seems to be a common issue with
certain servers and certain SSL implementations have added support to be
lenient with such servers
https://github.com/openssl/openssl/pull/4463/files
-Jaikiran
On 25/08/18 11:58 AM, Jaikiran Pai wrote:
While testing the recently released RC of JDK11 against the Apache Ant
project, I happened to run into an odd error. I have now been able to
reproduce this using the following, pretty trivial code:
import java.net.URL;
import java.io.InputStream;
public class Fetch {
public static void main(final String[] args) throws Exception {
final URL targetURL = new
URL("https://repository.jboss.org/nexus/content/groups/public/javax/media/jai-core/1.1.3/jai-core-1.1.3.pom";);
try (final InputStream is =
targetURL.openConnection().getInputStream()) {
is.read();
}
System.out.println("Done");
}
}
All it does is opens a (HTTPS) connection against an endpoint to read
some content. This code works fine in Java 8 and even Java 10. I'm
pretty sure this was working fine even in Java 11 early access builds,
but I don't have any such build/binary at hand to be certain.
However, using the latest (OpenJDK) RC of Java 11 (both on Mac OS and
Linux) downloaded from[1]:
openjdk version "11" 2018-09-25
OpenJDK Runtime Environment 18.9 (build 11+28)
OpenJDK 64-Bit Server VM 18.9 (build 11+28, mixed mode)
it fails with:
Exception in thread "main" javax.net.ssl.SSLHandshakeException:
extension (10) should not be presented in server_hello
at
java.base/sun.security.ssl.Alert.createSSLException(Alert.java:128)
at
java.base/sun.security.ssl.Alert.createSSLException(Alert.java:117)
at
java.base/sun.security.ssl.TransportContext.fatal(TransportContext.java:308)
at
java.base/sun.security.ssl.TransportContext.fatal(TransportContext.java:264)
at
java.base/sun.security.ssl.TransportContext.fatal(TransportContext.java:255)
at
java.base/sun.security.ssl.SSLExtensions.(SSLExtensions.java:71)
at
java.base/sun.security.ssl.ServerHello$ServerHelloMessage.(ServerHello.java:173)
at
java.base/sun.security.ssl.ServerHello$ServerHelloConsumer.consume(ServerHello.java:864)
at
java.base/sun.security.ssl.SSLHandshake.consume(SSLHandshake.java:392)
at
java.base/sun.security.ssl.HandshakeContext.dispatch(HandshakeContext.java:444)
at
java.base/sun.security.ssl.HandshakeContext.dispatch(HandshakeContext.java:421)
at
java.base/sun.security.ssl.TransportContext.dispatch(TransportContext.java:178)
at
java.base/sun.security.ssl.SSLTransport.decode(SSLTransport.java:164)
at
java.base/sun.security.ssl.SSLSocketImpl.decode(SSLSocketImpl.java:1152)
at
java.base/sun.security.ssl.SSLSocketImpl.readHandshakeRecord(SSLSocketImpl.java:1063)
at
java.base/sun.security.ssl.SSLSocketImpl.startHandshake(SSLSocketImpl.java:402)
at
java.base/sun.net.www.protocol.https.HttpsClient.afterConnect(HttpsClient.java:567)
at
java.base/sun.net.www.protocol.https.AbstractDelegateHttpsURLConnection.connect(AbstractDelegateHttpsURLConnection.java:185)
at
java.base/sun.net.www.protocol.http.HttpURLConnection.getInputStream0(HttpURLConnection.java:1581)
at
java.base/sun.net.www.protocol.http.HttpURLConnection.getInputStream(HttpURLConnection.java:1509)
at
java.ba
Re: Java 11 RC build - HTTPS handshake failure against a previously working server
Hi Jaikiran,
Thank you very much for the help!
JDK 12 repo (JDK repo):
http://hg.openjdk.java.net/jdk/jdk
JDK 11 repo:
http://hg.openjdk.java.net/jdk/jdk11
The patch should work for both repositories.
Thanks,
Xuelei
On 8/25/2018 7:44 AM, Jaikiran Pai wrote:
Hi Xuelei,
I can definitely build JDK 12 (jdk repo) from source and apply your
attached patch and give it a try. As for JDK 11, I haven't been
following the version control discussions/process, does it have a
separate repo now? Or is it some branch within jdk repo itself? Either
way, once I know the right repo location, I can (and in fact prefer)
building that repo with this patch to give it a try.
-Jaikiran
On 25/08/18 8:10 PM, Xuelei Fan wrote:
Hi Jaikiran,
Could you build JDK 11 or JDK 12 from source code? I had a patch to
tolerate the extension in ServerHello handshake message. Please let
me know if it works or not.
If there are any other JDK 11 TLS problems with Apache Ant project,
I'd like to know as well.
Thanks,
Xuelei
On 8/25/2018 7:04 AM, Jaikiran Pai wrote:
Hi Xuelei,
On 25/08/18 7:20 PM, Xuelei Fan wrote:
Sending "supported_groups" in ServerHello does not comply to the
extension specification.
Agreed. However, given that both the client and server are using TLSv1.2
and this seems to be "working" before the newer TLSv1.3 changes, even in
recent JDK versions, is there a way the implementation could workaround
this so as to allow JDK 11 to communicate with such servers?
Is it possible the HTTPS server fix this problem?
I don't have access or control over that server, so don't really know
how it's configured or whether it can be fixed. It's a pretty frequently
used Maven repository hosted by the JBoss (Red Hat middleware) project
team. I suspect, it's not just limited to this server and could be a
common issue with some other servers too.
I filed a bug in OpenJDK for the tracking:
https://bugs.openjdk.java.net/browse/JDK-8209965
Thank you.
-Jaikiran
Thanks,
Xuelei
On 8/25/2018 5:03 AM, Jaikiran Pai wrote:
As noted in that exception message, it appears that the server is
sending a "supported_groups" extension in its ServerHello message
(TLSv1.2). Reading about it, this seems to be a common issue with
certain servers and certain SSL implementations have added support
to be
lenient with such servers
https://github.com/openssl/openssl/pull/4463/files
-Jaikiran
On 25/08/18 11:58 AM, Jaikiran Pai wrote:
While testing the recently released RC of JDK11 against the Apache
Ant
project, I happened to run into an odd error. I have now been able to
reproduce this using the following, pretty trivial code:
import java.net.URL;
import java.io.InputStream;
public class Fetch {
public static void main(final String[] args) throws
Exception {
final URL targetURL = new
URL("https://repository.jboss.org/nexus/content/groups/public/javax/media/jai-core/1.1.3/jai-core-1.1.3.pom";);
try (final InputStream is =
targetURL.openConnection().getInputStream()) {
is.read();
}
System.out.println("Done");
}
}
All it does is opens a (HTTPS) connection against an endpoint to read
some content. This code works fine in Java 8 and even Java 10. I'm
pretty sure this was working fine even in Java 11 early access
builds,
but I don't have any such build/binary at hand to be certain.
However, using the latest (OpenJDK) RC of Java 11 (both on Mac OS and
Linux) downloaded from[1]:
openjdk version "11" 2018-09-25
OpenJDK Runtime Environment 18.9 (build 11+28)
OpenJDK 64-Bit Server VM 18.9 (build 11+28, mixed mode)
it fails with:
Exception in thread "main" javax.net.ssl.SSLHandshakeException:
extension (10) should not be presented in server_hello
at
java.base/sun.security.ssl.Alert.createSSLException(Alert.java:128)
at
java.base/sun.security.ssl.Alert.createSSLException(Alert.java:117)
at
java.base/sun.security.ssl.TransportContext.fatal(TransportContext.java:308)
at
java.base/sun.security.ssl.TransportContext.fatal(TransportContext.java:264)
at
java.base/sun.security.ssl.TransportContext.fatal(TransportContext.java:255)
at
java.base/sun.security.ssl.SSLExtensions.(SSLExtensions.java:71)
at
java.base/sun.security.ssl.ServerHello$ServerHelloMessage.(ServerHello.java:173)
at
java.base/sun.security.ssl.ServerHello$ServerHelloConsumer.consume(ServerHello.java:864)
at
java.base/sun.security.ssl.SSLHandshake.consume(SSLHandshake.java:392)
at
java.base/sun.security.ssl.HandshakeContext.dispatch(HandshakeContext.java:444)
at
java.base/sun.security.ssl.HandshakeContext.dispatch(HandshakeContext.java:421)
at
java.base/sun.security.ssl.TransportContext.dispatch(TransportContext.java:178)
at
java.base/sun.security.ssl.SSLTransport.decode(SS
Re: Java 11 RC build - HTTPS handshake failure against a previously working server
Thanks for the test!
Xuelei
On 8/26/2018 6:19 AM, Jaikiran Pai wrote:
I have now applied the patch to the JDK11 repo on top of:
hg sum
parent: 51151:1ddf9a99e4ad tip
Added tag jdk-11+28 for changeset 76072a077ee1
branch: default
and built the new images and run the testsuite against this version. The
tests passed without any issues. Ran the sample code from my original
mail, against this patched JDK 11 version and that too passed.
-Jaikiran
On 25/08/18 9:58 PM, Jaikiran Pai wrote:
Hi Xuelei,
I had the JDK 12 repo checked out already with the latest code as of today:
hg sum
parent: 51538:a716460217ed
8209911: More blob types in hs_err printout
branch: default
I applied the patch you had attached in this thread against this and
built it afresh.
With this new image, I was able to run the Apache Ant testsuite (the one
which was originally and still fails with JDK 11 RC build) without any
issues. I even ran the sample program that I had listed in this thread
against this patched 12.x build and that too went fine. I verified that
the patch has indeed taken effect by enabling javax.net.debug logging
and I do indeed see the new log:
javax.net.ssl|DEBUG|01|main|2018-08-25 21:20:57.860
IST|PreSharedKeyExtension.java:606|No session to resume.
javax.net.ssl|DEBUG|01|main|2018-08-25 21:20:57.860
IST|SSLExtensions.java:250|Ignore, context unavailable extension:
pre_shared_key
javax.net.ssl|DEBUG|01|main|2018-08-25 21:20:57.864
IST|ClientHello.java:633|Produced ClientHello handshake message (
"ClientHello": {
"client version" : "TLSv1.2",
}
)
javax.net.ssl|DEBUG|01|main|2018-08-25 21:20:57.865
IST|SSLSocketOutputRecord.java:241|WRITE: TLS13 handshake, length = 446
javax.net.ssl|DEBUG|01|main|2018-08-25 21:20:58.664
IST|SSLSocketInputRecord.java:213|READ: TLSv1.2 handshake, length = 99
javax.net.ssl|DEBUG|01|main|2018-08-25 21:20:58.664
IST|SSLSocketInputRecord.java:249|READ: TLSv1.2 handshake, length = 99
javax.net.ssl|WARNING|01|main|2018-08-25 21:20:58.665
IST|SSLExtensions.java:79|Buggy supported_groups in ServerHello
javax.net.ssl|DEBUG|01|main|2018-08-25 21:20:58.667
IST|ServerHello.java:862|Consuming ServerHello handshake message (
"ServerHello": {
"server version" : "TLSv1.2",
"random" : "4C 62 53 A1 56 4D 82 EE 3A 44 E3 25 0D 2F BD
CB 02 EE FD 3B 8E 4E D1 2B 52 5F AD 5B 0B B5 BC 98",
"session id" : "A9 BC 19 7D 36 84 25 F8 6B 77 3F 1D 93 5E B4
52 DE AE 41 90 67 2B F2 80 BB 85 3B BE 36 A1 F3 1C",
"cipher suite" : "TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384(0xC030)",
"compression methods" : "00",
"extensions" : [
"renegotiation_info (65,281)": {
"renegotiated connection": []
},
"server_name (0)": {
},
"ec_point_formats (11)": {
"formats": [uncompressed]
}
]
}
)
As for trying this against the JDK 11 repo, I have initiated a clone,
but it's going to take a while (as expected). I don't expect it to
finish soon, so I'm going to let it clone overnight and I will apply
this patch against that repo too and run this same testsuite against it.
I don't expect it to fail but I just want to make sure there aren't any
surprises. I will send out a note once that's done tomorrow.
I'll anyway be running some more extensive testsuites, over the next few
days, with this patched version and see how it goes.
Thank you very much for the quick response and the patch.
-Jaikiran
On 25/08/18 8:18 PM, Xuelei Fan wrote:
Hi Jaikiran,
Thank you very much for the help!
JDK 12 repo (JDK repo):
http://hg.openjdk.java.net/jdk/jdk
JDK 11 repo:
http://hg.openjdk.java.net/jdk/jdk11
The patch should work for both repositories.
Thanks,
Xuelei
On 8/25/2018 7:44 AM, Jaikiran Pai wrote:
Hi Xuelei,
I can definitely build JDK 12 (jdk repo) from source and apply your
attached patch and give it a try. As for JDK 11, I haven't been
following the version control discussions/process, does it have a
separate repo now? Or is it some branch within jdk repo itself? Either
way, once I know the right repo location, I can (and in fact prefer)
building that repo with this patch to give it a try.
-Jaikiran
On 25/08/18 8:10 PM, Xuelei Fan wrote:
Hi Jaikiran,
Could you build JDK 11 or JDK 12 from source code? I had a patch to
tolerate the extension in ServerHello handshake message. Please let
me know if it works or not.
If there are any other JDK 11 TLS problems with Apache Ant project,
I'd like to know as well.
Thanks,
Xuelei
On 8/25/2018 7:04 AM, Jaikiran Pai wrote:
Hi Xuelei,
On 25/08/18 7:20 PM, Xuelei Fan wrote:
Sending "supported_groups" in ServerHello does not comply to the
extension specification.
Agreed. However, given that
Code Review Request JDK-8209965 : The "supported_groups" extension in ServerHellos
Hi, Please review a compatibility fix for SunJSSE provider: http://cr.openjdk.java.net/~xuelei/8209965/webrev.00 There are servers that send the supported_groups extension in the ServerHello handshake message. It does not comply to the specification. However, as there are a few deployments already with the buggy implementation, we may want to tolerate this behavior in JDK. Note that although this extension is allowed in the ServerHello, it should be ignored and have no impact on the client behavior. The problem was reported and the fix was tested in OopenJDK: http://mail.openjdk.java.net/pipermail/security-dev/2018-August/018005.html Thanks, Xuelei
Re: Align SSLSocket and SSLEngine Javadocs
H Simone, There is no change for the SSLSocket.startHandshake() and SSLEngine.beginHandshake() specification. They are can be used for new handshake and key update. We want the specification independent from TLS versions as much as possible. An application developer only need to know the functionalities, but not necessarily to understand the TLS protocol details. For TLS 1.2 and prior versions, the key update is performed with renegotiation; for TLS 1.3, it is the KeyUpdate post-handshake. Thanks, Xuelei On 8/27/2018 2:37 AM, Simone Bordet wrote: Hi, SSLSocket.startHandshake() and SSLEngine.beginHandshake() are similar in that they start the TLS handshake, but they can also be used after the TLS handshake. SSLSocket.startHandshake() Javadoc seems to be more generic, describing that the method may not only start a new handshake but also be used to update encryption keys etc. Especially in light of TLS 1.3 where renegotiation is forbidden, I would like the Javadoc of these method to align and describe exactly when they do with respect to the TLS protocol version. Thanks!
Re: Code Review Request JDK-8209965 : The "supported_groups" extension in ServerHellos
Hi Tony, I thought about to limit the workaround to TLS 1.2 and prior version. However, just as you noticed that the implementation is not effective as it is needed to wait and check for the supported_versions extension if it presents. As may make the workaround a lot complicated. I would prefer to a simple change for now. Thanks, Xuelei On 8/26/2018 2:35 PM, Anthony Scarpino wrote: The change looks fine but I have a question about restricting it. This sounds like a problem with servers using 1.2 or before, does it make sense to throw an error for 1.3? I don’t like allowing buggy implementation to continue because we will never be able to undo this workaround. It would be nice if when someday 1.2 is removed, this change won’t persist in our code base. I realize this maybe a lot to ask given the decision of the protocol version hasn’t been made yet I believe. If it’s unreasonable, that is ok. I’m fine with the change as is. Tony On Aug 26, 2018, at 7:39 AM, Xuelei Fan wrote: Hi, Please review a compatibility fix for SunJSSE provider: http://cr.openjdk.java.net/~xuelei/8209965/webrev.00 There are servers that send the supported_groups extension in the ServerHello handshake message. It does not comply to the specification. However, as there are a few deployments already with the buggy implementation, we may want to tolerate this behavior in JDK. Note that although this extension is allowed in the ServerHello, it should be ignored and have no impact on the client behavior. The problem was reported and the fix was tested in OopenJDK: http://mail.openjdk.java.net/pipermail/security-dev/2018-August/018005.html Thanks, Xuelei
Re: Align SSLSocket and SSLEngine Javadocs
Hi Simone,
I see your point now. I filed a bug for the tracking:
https://bugs.openjdk.java.net/browse/JDK-8209992
Thanks,
Xuelei
On 8/27/2018 7:22 AM, Simone Bordet wrote:
Xuelei,
On Mon, Aug 27, 2018 at 4:00 PM Xuelei Fan wrote:
H Simone,
There is no change for the SSLSocket.startHandshake() and
SSLEngine.beginHandshake() specification. They are can be used for new
handshake and key update.
We want the specification independent from TLS versions as much as
possible. An application developer only need to know the
functionalities, but not necessarily to understand the TLS protocol details.
For TLS 1.2 and prior versions, the key update is performed with
renegotiation; for TLS 1.3, it is the KeyUpdate post-handshake.
Perhaps I was not clear. I'm not talking about the specification (what
the method does), just about the Javadoc.
A developer needs to know if calling a method causes a renegotiation or not :)
Would be great if your paragraph above ("For TLS 1.2 and prior ...")
would be included in the Javadoc.
In particular for SSLEngine, the current Javadoc says:
"Initiates handshaking (initial or renegotiation) on this SSLEngine."
It does not mention TLS 1.3 and does not mention KeyUpdate, so would
be great if it does.
And would be great if beginHandshake() and startHandshake() have a
very similar Javadoc.
Thanks!
Re: RFR 8209995: java.base does not need to export sun.security.ssl to java.security.jgss
Looks fine to me. Thanks, Xuelei On 8/27/2018 8:09 AM, Weijun Wang wrote: Since we've removed all krb5-based ciphersuites from JSSE, this qualified export is useless anymore. Please review the patch below: diff --git a/src/java.base/share/classes/module-info.java b/src/java.base/share/classes/module-info.java --- a/src/java.base/share/classes/module-info.java +++ b/src/java.base/share/classes/module-info.java @@ -284,8 +284,6 @@ java.naming; exports sun.security.rsa to jdk.crypto.cryptoki; -exports sun.security.ssl to -java.security.jgss; exports sun.security.timestamp to jdk.jartool; exports sun.security.tools to Thanks Max
Release note review, JDK-8210070, Release Note: The "supported_groups" extension should not present in the ServerHellos handshake message
Hi, Please review this release note: https://bugs.openjdk.java.net/browse/JDK-8210070 Per the "supported_groups" extension specification, the supported_groups extension should not present in the ServerHello handshake message. JDK 11 cannot work with TLS servers that wrap the extension in ServerHello handshake messages. We fixed the issue in JDK-8209965. We may backport this update in a future update. Thanks, Xuelei
Re: RFR 8201317: X25519/X448 code improvements (xs)
As you are already there, would you mind describe the differences between multByInt() and mult()? For method description, the comment is normally start with "/**" for javadoc friendly. Previously, you have the comment: // must work when a==r Do you want to keep it somehow? Otherwise, looks fine to me. Xuelei On 8/28/2018 9:58 AM, Adam Petcher wrote: I received some suggestions for improvements to the X25519/X448 code after the completion of the code review back in March. The improvements are some additional comments on methods and some rearranged expressions to prevent integer overflow. The change is very small, please review if you have 10 minutes to spare. JBS: https://bugs.openjdk.java.net/browse/JDK-8201317 Webrev: http://cr.openjdk.java.net/~apetcher/8201317/webrev.00/
Re: RFR 8201317: X25519/X448 code improvements (xs)
Looks fine to me. Thanks, Xuelei On 8/28/2018 10:50 AM, Adam Petcher wrote: New webrev: http://cr.openjdk.java.net/~apetcher/8201317/webrev.01/ Let me know if your concerns are addressed. One response is inline below. On 8/28/2018 1:10 PM, Xuelei Fan wrote: As you are already there, would you mind describe the differences between multByInt() and mult()? For method description, the comment is normally start with "/**" for javadoc friendly. Previously, you have the comment: // must work when a==r Do you want to keep it somehow? A re-worded form of this comment is at the end of each comment block. Otherwise, looks fine to me. Xuelei On 8/28/2018 9:58 AM, Adam Petcher wrote: I received some suggestions for improvements to the X25519/X448 code after the completion of the code review back in March. The improvements are some additional comments on methods and some rearranged expressions to prevent integer overflow. The change is very small, please review if you have 10 minutes to spare. JBS: https://bugs.openjdk.java.net/browse/JDK-8201317 Webrev: http://cr.openjdk.java.net/~apetcher/8201317/webrev.00/
Re: RFR(s): 8208641: SSLSocket should throw an exception when configuring DTLS
Looks fine to me. Please update the copyright years as well. Thanks, Xuelei On 8/28/2018 9:47 PM, Anthony Scarpino wrote: I need a review of this fix. Simple change to throw an UnsupportedOperationException using SSLSocket with DTLS. Additionally use SSLEngine for some of the generic methods that were defaulting to SSLSocket This is only for the JSSE provider. http://cr.openjdk.java.net/~ascarpino/8208641/webrev/ Tony
Re: RFR[12] JDK-8209362: sun/security/ssl/SSLSocketImpl/ReuseAddr.java failed due to "BindException: Address already in use (Bind failed)"
Maybe, run the test twice and pass if one passed? Xuelei On 8/29/2018 7:34 PM, sha.ji...@oracle.com wrote: Hi, In test sun/security/ssl/SSLSocketImpl/ReuseAddr.java, the second server tries to reuse the first server's port after it stops. But the port may already be occupied by another test before this rebinding. Although I'm not sure a way to resolve this problem thoroughly, it's a chance to refactor this test with SSLSocketTemplate.java. Webrev: http://cr.openjdk.java.net/~jjiang/8209362/webrev.00/ Issue: https://bugs.openjdk.java.net/browse/JDK-8209362 Best regards, John Jiang
Re: RFR[12] JDK-8209362: sun/security/ssl/SSLSocketImpl/ReuseAddr.java failed due to "BindException: Address already in use (Bind failed)"
On 8/30/2018 6:16 PM, sha.ji...@oracle.com wrote: Hi Xuelei, It still be possible that two test runs fail. Yes. I was wondering the possibility of the failure may go down. Thanks, Xuelei Best regards, John Jiang On 2018/8/30 22:02, Xuelei Fan wrote: Maybe, run the test twice and pass if one passed? Xuelei On 8/29/2018 7:34 PM, sha.ji...@oracle.com wrote: Hi, In test sun/security/ssl/SSLSocketImpl/ReuseAddr.java, the second server tries to reuse the first server's port after it stops. But the port may already be occupied by another test before this rebinding. Although I'm not sure a way to resolve this problem thoroughly, it's a chance to refactor this test with SSLSocketTemplate.java. Webrev: http://cr.openjdk.java.net/~jjiang/8209362/webrev.00/ Issue: https://bugs.openjdk.java.net/browse/JDK-8209362 Best regards, John Jiang
Re: RFR[12] JDK-8209362: sun/security/ssl/SSLSocketImpl/ReuseAddr.java failed due to "BindException: Address already in use (Bind failed)"
Okay. Thanks, Xuelei On 8/30/2018 6:46 PM, sha.ji...@oracle.com wrote: On 2018/8/31 09:22, Xuelei Fan wrote: On 8/30/2018 6:16 PM, sha.ji...@oracle.com wrote: Hi Xuelei, It still be possible that two test runs fail. Yes. I was wondering the possibility of the failure may go down. I searched this test in JBS, and didn't find many failures on this test. So, it may be unnecessary to concern it too much. Run all javax/net/ssl and sun/security/ssl tests with this fix via Mach5, the result is green. Best regards, John Jiang Thanks, Xuelei Best regards, John Jiang On 2018/8/30 22:02, Xuelei Fan wrote: Maybe, run the test twice and pass if one passed? Xuelei On 8/29/2018 7:34 PM, sha.ji...@oracle.com wrote: Hi, In test sun/security/ssl/SSLSocketImpl/ReuseAddr.java, the second server tries to reuse the first server's port after it stops. But the port may already be occupied by another test before this rebinding. Although I'm not sure a way to resolve this problem thoroughly, it's a chance to refactor this test with SSLSocketTemplate.java. Webrev: http://cr.openjdk.java.net/~jjiang/8209362/webrev.00/ Issue: https://bugs.openjdk.java.net/browse/JDK-8209362 Best regards, John Jiang
Re: Release note review, JDK-8210070, Release Note: The "supported_groups" extension should not present in the ServerHellos handshake message
As we don't fix it in JDK 11, it is intended to have a known issue note for JDK 11. Xuelei On 8/31/2018 12:25 AM, Alan Bateman wrote: On 28/08/2018 15:17, Xuelei Fan wrote: Hi, Please review this release note: https://bugs.openjdk.java.net/browse/JDK-8210070 Per the "supported_groups" extension specification, the supported_groups extension should not present in the ServerHello handshake message. JDK 11 cannot work with TLS servers that wrap the extension in ServerHello handshake messages. We fixed the issue in JDK-8209965. We may backport this update in a future update. The release note for JDK-8209965 is showing up on the JDK 11 release notes [1] for some reason. -Alan [1] http://jdk.java.net/11/release-notes
Re: RFR 8210338: Better output for GenerationTests.java
Looks fine to me. Xuelei On 9/3/2018 7:50 PM, Weijun Wang wrote: Please take a review at http://cr.openjdk.java.net/~weijun/8210338/webrev.00/ This test enhancement adds section breaks and ensures everything written to stdout is flushed asap. With these info we will have a better understanding of when timeout happens in this test and what the best fix (to the parent bug) is. Thanks Max
Re: RFR 8171279: Support X25519 and X448 in TLS 1.3
I have no finished the full code review. So far, I have a few question
about the struct of the code.
1. XECParameters
I can see the reason to dynamic parameters for something other than
X25519/X448. But for JSSE, currently, only named curves are used. It
would be nice to use the name for the static parameters.
2. "TlsPremasterSecret" only XDH key agreement
It would be nice the JCE implementation can support key agreement other
than TLS protocols and providers other than SunJSSE provider. It would
be nice if the JCE implementation does not bind to the SunJSSE provider
private algorithm name.
We used to use SunJSSE private name in the JCE crypto implementation.
But there are some known problems with this dependence.
Is there a problem to support generic key agreement?
3. NamedGroupFunctions
It might be more straightforward to define these functions in
NamedGroup. If comparing nameGroup.getFunctions().getParameterSpec()
and namedGroup.getParameterSpec(), I'd prefer the latter.
4. SSLKeyAgreementCredentials
I did not see too much benefit of this new interface. It is not always
true that a key agreement could have a public key. Although we can
simplify the key agreement for public key based, but it also add
additional layers.
I know where this improvement comes from. Maybe, you can consolidate
the named group functions, and encode/decode the credentials there.
Xuelei
On 8/30/2018 8:58 AM, Adam Petcher wrote:
Webrev: http://cr.openjdk.java.net/~apetcher/8171279/webrev.00/
JBS: https://bugs.openjdk.java.net/browse/JDK-8171279
Please review the following change to add support for X25519 and X448
(XDH) to TLS 1.3. This change includes some refactoring to remove code
that was duplicated for DH and ECDH, and to avoid adding more for XDH.
In addition to running the included regression test, I tested by
connecting to an openssl server and confirmed that the connection was
established using TLS 1.3 and X25519/X448.
Here are some detailed notes:
*) The NamedGroupFunctions class was added to hold the functions that
are needed for key agreement with some named group. Most of the
group-specific code was moved into subclasses of NamedGroupFunctions.
This allowed me to remove a bunch of code like "if (type == ECDHE) {...}
else if (type == FFDHE) {...}".
*) There are a couple of files in the webrev with no changes due to a
webrev issue. Please ignore them.
*) I moved some code related to XDH parameters and encoding into
java.base. ECUtil now has code to encode/decode XDH public keys, and the
XECParameters file was moved into java.base/sun.security.util. This
organization is similar to how CurveDB and NamedCurve are in java.base,
and it allows the TLS implementation to encode/decode keys without using
the jdk.crypto.ec module.
Re: RFR 8171279: Support X25519 and X448 in TLS 1.3
On 9/5/2018 10:09 AM, Adam Petcher wrote: Updated webrev: http://cr.openjdk.java.net/~apetcher/8171279/webrev.01/ On 9/4/2018 3:25 PM, Xuelei Fan wrote: I have no finished the full code review. So far, I have a few question about the struct of the code. 1. XECParameters I can see the reason to dynamic parameters for something other than X25519/X448. But for JSSE, currently, only named curves are used. It would be nice to use the name for the static parameters. Sorry, I don't think I understand what you are suggesting here. As far as I know, nothing in sun.security.ssl uses XECParameters directly. Okay, it is used indirectly. It is used indirectly through ECUtil to encode and decode keys, and in this case the name is used to look up the parameters. Can the public NamedParameterSpec be used, instead of the private XECParameters? I think XECParameters may be better in the EC provider, please try to mitigate the dependence between modules. If one day the sun.security.util.XECParameters get updated, the EC implementation get impacted a lot. I prefer to use public APIs if possible. Is there some place in the code where JSSE is doing something too complicated related to these parameters? Yes, the algorithm name is sufficient, I'm not sure the necessary to use XECParameters in sun.security.util. 2. "TlsPremasterSecret" only XDH key agreement It would be nice the JCE implementation can support key agreement other than TLS protocols and providers other than SunJSSE provider. It would be nice if the JCE implementation does not bind to the SunJSSE provider private algorithm name. We used to use SunJSSE private name in the JCE crypto implementation. But there are some known problems with this dependence. Is there a problem to support generic key agreement? I simply continued the pattern that was used in DH and ECDH. We can use a different string here, but I worry that people will be surprised if DH and ECDH support "TlsPreMasterSecret" but XDH doesn't. It could support "TlsPreMasterSecret", right? My concern is about not limited to this one only. I would rather continue to use "TlsPreMasterSecret" for now, and then add support for a standard, TLS-independent name (that means the same thing) in a separate ticket. But if you feel strongly that XDH should not support "TlsPreMasterSecret", then perhaps we can use a different string now. In this case, let me know if you have any suggestions for what string to use. See above. 3. NamedGroupFunctions It might be more straightforward to define these functions in NamedGroup. If comparing nameGroup.getFunctions().getParameterSpec() and namedGroup.getParameterSpec(), I'd prefer the latter. A simple way to accomplish this is to leave the structure alone and add methods in NamedGroup that pass through to the corresponding methods in NamedGroupFunctions. I made this change in the updated webrev. Let me know what you think. It looks like a problem to me to use this before it constructed. this.functions = new ECDHFunctions(this); and the use of new object for each named group is not effective. The current NamedGroupFunctions abstract class does not sound good enough to me, considering the numbers of the named groups. I have no idea so far, but I think you can improve it. Probably use static methods? 4. SSLKeyAgreementCredentials I did not see too much benefit of this new interface. It is not always true that a key agreement could have a public key. Although we can simplify the key agreement for public key based, but it also add additional layers. I know where this improvement comes from. Maybe, you can consolidate the named group functions, and encode/decode the credentials there. This interface is only used to validate public keys against algorithm constraints. In the latest webrev, I moved all of this functionality into NamedGroupFunctions and removed the SSLKeyAgreementCredentials interface. Thanks, I like it the new design. Xuelei
Code Review Request, JDK-8210334, TLS 1.3 server fails if ClientHello doesn't have pre_shared_key and psk_key_exchange_modes
Hi, Please review: http://cr.openjdk.java.net/~xuelei/8210334/webrev.00/ Simple update, no new regression test. Thanks, Xuelei
Re: RFR 8171279: Support X25519 and X448 in TLS 1.3
On 9/5/2018 12:49 PM, Adam Petcher wrote: New webrev: http://cr.openjdk.java.net/~apetcher/8171279/webrev.02/ On 9/5/2018 1:35 PM, Xuelei Fan wrote: On 9/5/2018 10:09 AM, Adam Petcher wrote: Is there some place in the code where JSSE is doing something too complicated related to these parameters? Yes, the algorithm name is sufficient, I'm not sure the necessary to use XECParameters in sun.security.util. The algorithm name is not quite sufficient. See the new methods that were added to ECUtil that encode/decode public keys. We also need to know the key length (which is in XECParameters) in order to encode/decode public keys. I did not get your point. The public key sizes for x25519 and x448 are fixed, right? I simply continued the pattern that was used in DH and ECDH. We can use a different string here, but I worry that people will be surprised if DH and ECDH support "TlsPreMasterSecret" but XDH doesn't. It could support "TlsPreMasterSecret", right? My concern is about not limited to this one only. Yes, it supports "TlsPreMasterSecret" in the webrev now. Perhaps I'm not sure what you are suggesting here. OKay, let me say it in another way. Thank you for making it works with the SunJSSE provider as you use a SunJSSE provider private algorithm name "TlsPreMasterSecret", and implement the algorithm in the cyrpto provider. That's good. The "TlsPreMasterSecret" is not a public one, and it is not expected to be used in other JSSE provider. If I want to implement a JSSE provider by myself, and I use the name "x25519", the crypto provider will deny it. So I have to use the "TlsPreMasterSecret" for my provider. However, the name is not supported, and the name can be changed at anytime in the future. How should I do to use the crypto provider for my JSSE provider? Looks like no way to use the JDK cyrpto provider unless I use the SunJSSE private name. Should I implement my own crypto provider? I don't want to do that unless it is really necessary. For the KeyAgreement.generateSecret(String algorithm) method, it seems that the supported algorithms of each provider are missed in the documentation. As may make the method hard to use. This issue is not specific to XDH. I'm fine if you don't want to address it in this update. 3. NamedGroupFunctions It might be more straightforward to define these functions in NamedGroup. If comparing nameGroup.getFunctions().getParameterSpec() and namedGroup.getParameterSpec(), I'd prefer the latter. A simple way to accomplish this is to leave the structure alone and add methods in NamedGroup that pass through to the corresponding methods in NamedGroupFunctions. I made this change in the updated webrev. Let me know what you think. It looks like a problem to me to use this before it constructed. this.functions = new ECDHFunctions(this); and the use of new object for each named group is not effective. The current NamedGroupFunctions abstract class does not sound good enough to me, considering the numbers of the named groups. I have no idea so far, but I think you can improve it. Probably use static methods? In the latest webrev, I changed it so there is a single static NamedGroupFunctions of each type, and the NamedGroup is passed in as the first argument to each method that requires it (rather than being a member of NamedGroupFunctions). After the re-org, I guess you can put the inner classes in NamedGroup enum and declare them as private? The getFunctions() method may be unnecessary then. Thanks, Xuelei
Re: RFR 8171279: Support X25519 and X448 in TLS 1.3
On 9/6/2018 10:04 AM, Adam Petcher wrote: On 9/6/2018 12:10 PM, Xuelei Fan wrote: The algorithm name is not quite sufficient. See the new methods that were added to ECUtil that encode/decode public keys. We also need to know the key length (which is in XECParameters) in order to encode/decode public keys. I did not get your point. The public key sizes for x25519 and x448 are fixed, right? Yes, the key sizes are fixed. All we need in ECUtil is a mapping from curve name to this (fixed) size. Are you suggesting some other solution, other than using the XECParameters to map curve names to key sizes? Using name only (NamedParameterSpec?) and have the JCE provider handle it, then you don't need to move XECParameters into java.base module. Xuelei
Re: RFR 8171279: Support X25519 and X448 in TLS 1.3
On 9/6/2018 10:17 AM, Adam Petcher wrote:
In the latest webrev, I changed it so there is a single static
NamedGroupFunctions of each type, and the NamedGroup is passed in as
the first argument to each method that requires it (rather than
being a member of NamedGroupFunctions).
After the re-org, I guess you can put the inner classes in NamedGroup
enum and declare them as private? The getFunctions() method may be
unnecessary then.
I don't know if that works, exactly, due to the fact that I can't
reference static enum members in the body of an enum constructor. How
about this alternative? I can move the NamedGroup enum and all the
NamedGroupFunction classes into a separate class (in a separate file)
called NamedGroups. Then all the NamedGroupFunction classes can be
private in this class, but the NamedGroup enum can still have package
access. I would prefer to leave the getFunctions() method of
NamedGroup (and keep it private) because the functions object may be
missing and the Optional return type of getFunctions() forces me to
deal with this when I call it from within NamedGroup.
I think it should be able to use the "functions" field directly.
Optional ngf = getFunctions()
if (ngf.isEmpty() {
...
}
V.S.
if (functions != null) {
...
}
I did not see the benefits of the getFunctions() method.
Actually, it does work. I just have to move the static members of each
NamedGroupFunctions subclass into its subclass (e.g. make them
singletons). Still, I like my proposed alternative better, because it
allows us to simplify SupportedGroupsExtension. Let me know if you have
a preference.
My concerns are mainly about:
1. the NamedGroupFunctions should be private and should not be used
other than the NamedGroup enum impl.
2. the ffdh/ecdh/xdhFunctions static fields should be private of the
NamedGroup enum as well, and better be lazy instantiated as you are
using Map objects in the NamedGroupFunctions implementation (for
performance).
3. NamedGroupFunctions.namedGroupParams is fine in general, but in this
context, it means the map will always be generated. We used to use a
SupportedGroups to wrap and cache the parameters, and don't care about
the unsupported groups. But in the new re-org, looks like the
unsupported groups may also have a chance to cache/use the parameters.
#3 is a new find when I'm trying to understand your proposal. It would
be nice if you could think about the SupportedGroups impact.
For the question about how it works, there are a few approaches. You can
use singletons as you said or inner enum (See CipherSuite.java).
Xuelei
Re: RFR 8171279: Support X25519 and X448 in TLS 1.3
I think I suggested to use NamedParameterSpec, which is a public API. NamedParameterSpec -> name "x25519" -> key size is the key size of x25519. Please let me know if the logic is wrong. > Also, why do you object to having XECParameters in java.base? Please read my previous comments. > ... similar classes like ECParameters and CurveDB. Previously, there is no NamedParameterSpec, so we have to workaround to use ECParameters. Now, we don't have to do the ugly thing any more if I did not miss something. Thanks, Xuelei On 9/6/2018 12:13 PM, Adam Petcher wrote: On 9/6/2018 1:55 PM, Xuelei Fan wrote: Yes, the key sizes are fixed. All we need in ECUtil is a mapping from curve name to this (fixed) size. Are you suggesting some other solution, other than using the XECParameters to map curve names to key sizes? Using name only (NamedParameterSpec?) and have the JCE provider handle it, then you don't need to move XECParameters into java.base module. Do you have a specific suggestion on how I can do that? I don't think there is anything in the JCE API for XDH that allows a lookup from name to key length. Are you proposing that I enhance the public API to avoid using XECParameters here? Also, why do you object to having XECParameters in java.base? Most of the crypto code is in java.base, including similar classes like ECParameters and CurveDB. I admit that it is unfortunate that XECParameters is used directly here, instead of going over JCE---is that what you object to?
Re: RFR 8171279: Support X25519 and X448 in TLS 1.3
I asked the question in a previous email. The key size for x25529 is fixed, right? If it is not right, stop here and tell me that it is not right. Keep reading if it is right. OK, as the key size for x25519 is fixed, when you know the algorithm is x25519, you know the key size. Does it sound right to you? If it is not right, stop here and tell me that it is not right. Otherwise, keep reading. From the name you know the key size, when you create a NamedParameterSpec object for "x25519", you know the name and key size from the object, right? Let's look at the x25519 case at first. If we figure it out, we then can look into the x488. Thanks, Xuelei On 9/6/2018 1:43 PM, Adam Petcher wrote: On 9/6/2018 3:19 PM, Xuelei Fan wrote: I think I suggested to use NamedParameterSpec, which is a public API. NamedParameterSpec -> name "x25519" -> key size is the key size of x25519. Please let me know if the logic is wrong. It's that last arrow that I still don't get. How does this code figure out that "X25519" -> 255 and "X448" -> 448? Perhaps you can reply with some code to illustrate how you think this should work.
Re: RFR: JDK-8140466: ChaCha20-Poly1305 TLS cipher suites
SSLCipher.java -- line 2159-2164 in the update vs line 1992-1997 in the old file. The new code is fine, but it takes me a while to analysis the code, and comparing with the old one. Maybe, we can use the same implementation code for the same logic for maintenance? Just a very personal preference. You make the final choice. If you accept it, please consider other places that compute the nonce value. 2180 sequence); 'sn' should be used here. The 'sequence' variable may be different from the one used for the cipher. Otherwise, looks fine to me. Thanks, Xuelei On 9/5/2018 9:51 PM, Jamil Nimeh wrote: Hello all, This change will add ChaCha20-Poly1305 cipher suites to our TLS 1.2 and TLS 1.3 implementations. A few test cases had to be updated to reflect the new suites as well. JBS: https://bugs.openjdk.java.net/browse/JDK-8140466 CSR: https://bugs.openjdk.java.net/browse/JDK-8204192 Webrev: http://cr.openjdk.java.net/~jnimeh/reviews/8140466/webrev.01/ Thanks, --Jamil
Re: RFR: JDK-8140466: ChaCha20-Poly1305 TLS cipher suites
> On Sep 6, 2018, at 5:02 PM, Jamil Nimeh wrote: > > Hi Xuelei, thank you for the comments - my replies are in-line: > >> On 9/6/2018 2:31 PM, Xuelei Fan wrote: >> SSLCipher.java >> -- >> line 2159-2164 in the update vs line 1992-1997 in the old file. >> >> The new code is fine, but it takes me a while to analysis the code, and >> comparing with the old one. Maybe, we can use the same implementation code >> for the same logic for maintenance? Just a very personal preference. You >> make the final choice. If you accept it, please consider other places that >> compute the nonce value. > Respectfully, I think the way the AES-GCM code sets up the cipher doesn't > match very well with how ChaCha20 does it. Even the RFC itself says that the > nonce construction is different. There's no per-record nonce_explicit and > it's really just a padded sequence number XORed with the client or server > read/write IV. I think the current code follows the procedure in 7905 > closely. This is a sound reason to me. Okay, keep it. Xuelei > Taking the GCM construction will muddy it a bit, since things like > recordIvSize get brought in...for CC20 that's always zero, so why have it at > all? It just clutters things IMO. > >> >> 2180 sequence); >> 'sn' should be used here. The 'sequence' variable may be different from the >> one used for the cipher. > Oh! Good catch. I will fix this. >> >> Otherwise, looks fine to me. >> > Thanks Xuelei, much appreciated, > --Jamil > >> Thanks, >> Xuelei >> >>> On 9/5/2018 9:51 PM, Jamil Nimeh wrote: >>> Hello all, >>> This change will add ChaCha20-Poly1305 cipher suites to our TLS 1.2 and TLS >>> 1.3 implementations. A few test cases had to be updated to reflect the new >>> suites as well. >>> >>> JBS: https://bugs.openjdk.java.net/browse/JDK-8140466 >>> CSR: https://bugs.openjdk.java.net/browse/JDK-8204192 >>> Webrev: http://cr.openjdk.java.net/~jnimeh/reviews/8140466/webrev.01/ >>> >>> Thanks, >>> --Jamil >
Re: RFR 8171279: Support X25519 and X448 in TLS 1.3
> The NamedParameterSpec object holds the name only, and not the key size.
The name is not a meaningless string, it refer to a specific thing. For
more examples, please refer to the standard names documentation, every
name has its specific meaning and the background. If the name is just a
meaningless string, there is nothing we can do for it and we may not
want to define a meaningless API.
The parse of the NamedParameterSpec name is really about the
implementation details. For example, for KeyFactory:
public PublicKey engineGeneratePublic(KeySpec keySpec) {
if (the algorithm is 'x25519') {
// use the X25519 parameters, including key size
} else if ('x448') {
// use the X25519 parameters, including key size
}
}
There are a few alternatice ways. You can define a enum in the XDH
provider, or just use switch, or use Map, or something else you like.
Which one is a better one, it may depends on the implementation details.
Please don't define the x25519 parameters in JSSE. JSSE should use the
'x25519' name (via NamedParameterSpec object) only. The underlying JCE
provider should take the responsibility to support the
NamedParameterSpec and defines the internal/private parameters for the
specific name.
Thanks,
Xuelei
On 9/7/2018 5:49 AM, Adam Petcher wrote:
On 9/6/2018 4:49 PM, Xuelei Fan wrote:
I asked the question in a previous email. The key size for x25529 is
fixed, right?
Right.
If it is not right, stop here and tell me that it is not right. Keep
reading if it is right.
OK, as the key size for x25519 is fixed, when you know the algorithm
is x25519, you know the key size. Does it sound right to you?
Possibly right---it depends on what you mean by "know". If all you have
is the name, then you need use a static mapping to look up the key length.
If it is not right, stop here and tell me that it is not right.
Otherwise, keep reading.
From the name you know the key size, when you create a
NamedParameterSpec object for "x25519", you know the name and key size
from the object, right?
The NamedParameterSpec object holds the name only, and not the key size.
We create the NamedParameterSpec from the algorithm name in the
NamedGroup enum, which also doesn't have the key size. Are you
suggesting that I add the key size to this enum as well? Like this:
// x25519 and x448
X25519 (0x001D, "x25519", true, "x25519", 255,
ProtocolVersion.PROTOCOLS_TO_13),
X448 (0x001E, "x448", true, "x448", 448,
ProtocolVersion.PROTOCOLS_TO_13),
The constructor will take this length and store it. Then we can get this
value out of the NamedGroup in XDHKeyExchange and pass it in to the
methods of ECUtil so we don't need to get it from XECParameters. Is this
what you had in mind?
Re: RFR 8171279: Support X25519 and X448 in TLS 1.3
On 9/7/2018 6:34 AM, Xuelei Fan wrote:
> The NamedParameterSpec object holds the name only, and not the key size.
The name is not a meaningless string, it refer to a specific thing. For
more examples, please refer to the standard names documentation, every
name has its specific meaning and the background. If the name is just a
meaningless string, there is nothing we can do for it and we may not
want to define a meaningless API.
The parse of the NamedParameterSpec name is really about the
implementation details. For example, for KeyFactory:
public PublicKey engineGeneratePublic(KeySpec keySpec) {
if (the algorithm is 'x25519') {
// use the X25519 parameters, including key size
} else if ('x448') {
// use the X25519 parameters, including key size
stupid copy and past: X25519 -> X448
}
}
There are a few alternatice ways. You can define a enum in the XDH
provider, or just use switch, or use Map, or something else you like.
Which one is a better one, it may depends on the implementation details.
Please don't define the x25519 parameters in JSSE. JSSE should use the
'x25519' name (via NamedParameterSpec object) only. The underlying JCE
provider should take the responsibility to support the
NamedParameterSpec and defines the internal/private parameters for the
specific name.
Thanks,
Xuelei
On 9/7/2018 5:49 AM, Adam Petcher wrote:
On 9/6/2018 4:49 PM, Xuelei Fan wrote:
I asked the question in a previous email. The key size for x25529 is
fixed, right?
Right.
If it is not right, stop here and tell me that it is not right. Keep
reading if it is right.
OK, as the key size for x25519 is fixed, when you know the algorithm
is x25519, you know the key size. Does it sound right to you?
Possibly right---it depends on what you mean by "know". If all you
have is the name, then you need use a static mapping to look up the
key length.
If it is not right, stop here and tell me that it is not right.
Otherwise, keep reading.
From the name you know the key size, when you create a
NamedParameterSpec object for "x25519", you know the name and key
size from the object, right?
The NamedParameterSpec object holds the name only, and not the key
size. We create the NamedParameterSpec from the algorithm name in the
NamedGroup enum, which also doesn't have the key size. Are you
suggesting that I add the key size to this enum as well? Like this:
// x25519 and x448
X25519 (0x001D, "x25519", true, "x25519", 255,
ProtocolVersion.PROTOCOLS_TO_13),
X448 (0x001E, "x448", true, "x448", 448,
ProtocolVersion.PROTOCOLS_TO_13),
The constructor will take this length and store it. Then we can get
this value out of the NamedGroup in XDHKeyExchange and pass it in to
the methods of ECUtil so we don't need to get it from XECParameters.
Is this what you had in mind?
Re: RFR 8171279: Support X25519 and X448 in TLS 1.3
Please let me know if you understand the following logic. Otherwise, I
will see if I could do something for you, maybe a prototype, maybe a
more detailed description. However, I may need more time for a
prototype/detailed description.
Per RFC 8446/7748, the X25519 key size is 32 bytes. Here we know the
key size. [1]
Per RFC 8446, the X25519 public key is encoded as byte string inputs and
outputs, as defined in RFC 7748. Her we know the encoding of the key. [2]
Suppose x25519 is used [3], then we know that the key sized per [1] and
the encoded key per [2]. Next step, let convert the encoded key bytes
to PublicKey.
EncodedKeySpec keySpec = ... // find a way to construct the keySpec
// at least, we can use:
//new EncodedKeySpec(byte[]);
// But please check if there's a better one
KeyFactory kFac = KeyFactory.getInstance("x25519");
// we know the name in [3]
PublicKey pubKey = kFac.generatePublic(keySpec);
Here you got the public key.
We may also need to generate the key pair.
KeyPairGenerator kpg = KeyPairGenerator.getInstance("x25519");
// we know the name in [3]
// may be optional, we know the name in [3].
NamedParameterSpec nps = new NamedParameterSpec("x25519");
kpg.initialize(nps);
KeyPair kp = kpg.generateKeyPair();
How you get the private key.
That's it. I know you may need to adjust the crypto implementation if
the provider does not support the above scenarios yet.
Xuelei
On 9/7/2018 7:30 AM, Adam Petcher wrote:
On 9/7/2018 9:34 AM, Xuelei Fan wrote:
JSSE should use the 'x25519' name (via NamedParameterSpec object) only.
This is the part that I don't know how to do. In JSSE, we convert
between the array containing the encoded public key and the BigInteger
representation of the public key used in XECPublicKeySpec. To do this,
you need to know the length of the key, in bits. That means that JSSE
needs to know the length of the key, in addition to the name, in order
to do this conversion. I understand that there are lots of ways to get
this information in JSSE, but I don't know which ways you would find
acceptable.
We keep going back and forth, saying the exact same things, and we don't
seem to be making any progress. Can you please provide some code to
illustrate what you want me to do? All I need is an acceptable
implementation of the following method, that is used by JSSE to decode
public keys:
public static XECPublicKeySpec decodeXecPublicKey(byte[] key,
NamedParameterSpec spec)
throws InvalidParameterSpecException {
XECParameters params = XECParameters.get(
InvalidParameterSpecException::new, spec);
BigInteger u = decodeXecPublicKey(key, params.getBits());
return new XECPublicKeySpec(spec, u);
}
For reference, here is the implementation of the helper method that does
the actual decoding, using the length.
public static BigInteger decodeXecPublicKey(byte[] key,
int bits) {
ArrayUtil.reverse(key);
// clear the extra bits
int bitsMod8 = bits % 8;
if (bitsMod8 != 0) {
int mask = (1 << bitsMod8) - 1;
key[0] &= mask;
}
return new BigInteger(1, key);
}
Re: RFR 8171279: Support X25519 and X448 in TLS 1.3
On 9/7/2018 9:03 AM, Adam Petcher wrote:
This is more clear, thanks. See below.
On 9/7/2018 11:34 AM, Xuelei Fan wrote:
EncodedKeySpec keySpec = ... // find a way to construct the keySpec
// at least, we can use:
// new EncodedKeySpec(byte[]);
// But please check if there's a better one
Do you mean X509EncodedKeySpec, or some class that is specific to XDH?
I did not search the spec definitions. At least we can use the
EncodedKeySpec class. It's nice if you can find a better one, or
define a new one for XDH.
Your following comments make sense to me.
Thanks,
Xuelei
An X509EncodedKeySpec would probably work---we would just need to put
the key into a SubjectPublicKeyInfo, which means I need the OID. Is it
okay for me to put the OID in JSSE? I could put it in the NamedGroup
enum, like this:
X25519 (0x001D, "x25519", true, "x25519", "1.3.101.110",
ProtocolVersion.PROTOCOLS_TO_13),
X448 (0x001E, "x448", true, "x448", "1.3.101.111",
ProtocolVersion.PROTOCOLS_TO_13),
I'm not sure if the second x25519/x448 strings (for algorithm and
NamedParameterSpec names) would still be needed, since I can use the OID
in some of these places. If it's not needed, then perhaps I can remove
it and only use the OID to interact with the JCA provider.
We don't have a type for XDH encoded public keys. It would be nice to be
able to do something simple like:
byte[] keyBytes = ...
NamedParameterSpec paramSpec = new NamedParameterSpec("X25519");
XECEncodedPublicKeySpec keySpec = new XECEncodedKeySpec(paramSpec,
keyBytes);
and then give keySpec to the "XDH" key factory. But this
XECEncodedKeySpec type does not exist, so this would require an
enhancement to the API.
Re: RFR 8171279: Support X25519 and X448 in TLS 1.3
> I'm not sure if the second x25519/x448 strings (for algorithm and
> NamedParameterSpec names) would still be needed, since I can use
> the OID in some of these places.
I see your points. However, we may want to think if third party wants
to implement a JSSE provider, whether the JCE providers are sufficient
for them and how the JCE provider could be used for them. Using OID
works, but it is not as straightforward as a name. We should document
the names in the Standard Names document. So using OIDs might not be as
good as using names.
I guess we have documented the 'x25519" algorithm name for JDK 11? If
it is true, then we should be able to use them.
Xuelei
On 9/7/2018 9:03 AM, Adam Petcher wrote:
This is more clear, thanks. See below.
On 9/7/2018 11:34 AM, Xuelei Fan wrote:
EncodedKeySpec keySpec = ... // find a way to construct the keySpec
// at least, we can use:
// new EncodedKeySpec(byte[]);
// But please check if there's a better one
Do you mean X509EncodedKeySpec, or some class that is specific to XDH?
An X509EncodedKeySpec would probably work---we would just need to put
the key into a SubjectPublicKeyInfo, which means I need the OID. Is it
okay for me to put the OID in JSSE? I could put it in the NamedGroup
enum, like this:
X25519 (0x001D, "x25519", true, "x25519", "1.3.101.110",
ProtocolVersion.PROTOCOLS_TO_13),
X448 (0x001E, "x448", true, "x448", "1.3.101.111",
ProtocolVersion.PROTOCOLS_TO_13),
I'm not sure if the second x25519/x448 strings (for algorithm and
NamedParameterSpec names) would still be needed, since I can use the OID
in some of these places. If it's not needed, then perhaps I can remove
it and only use the OID to interact with the JCA provider.
We don't have a type for XDH encoded public keys. It would be nice to be
able to do something simple like:
byte[] keyBytes = ...
NamedParameterSpec paramSpec = new NamedParameterSpec("X25519");
XECEncodedPublicKeySpec keySpec = new XECEncodedKeySpec(paramSpec,
keyBytes);
and then give keySpec to the "XDH" key factory. But this
XECEncodedKeySpec type does not exist, so this would require an
enhancement to the API.
Re: Conceptual feedback on new ECC JEP
Hi, I have not had a chance to review this JEP yet. Personally, if possible, I would expect there is no public APIs update so that more applications can benefit from the enhancement, and SunJSSE could benefits from more crypto providers. I'm not sure if it is possible or not now, or how could we minimize the APIs update. I will see if I could be here next week. Please go ahead if you have an agreement before I look into this JEP. Thanks, Xuelei On 9/7/2018 12:08 PM, Adam Petcher wrote: This is a good suggestion. I don't have particularly strong feelings about using separate providers vs a property in a single provider. I think the fundamental issues are the same, and this choice mostly affects API details. Do you think this should be a system property, security property, or something else? Should it be modifiable at any time? Perhaps it has to be in order to address Mike's desire to put the provider in "import/export mode". Would the property affect existing keys? Again, I think it would have to, so you can generate a key, turn off branchless mode, and then export it. What about curves other than P256, P384, and P521? We can't do branchless operations in those curves, so any attempt to use them when this property is enabled would result in an exception. The questions above are for everybody, if you have thoughts on any of this, please share. My initial thoughts are that using a property may give us some additional flexibility, and may improve the interface, but the main cost is additional complexity in the implementation, since we'll need to implement some checks that would otherwise be accomplished by provider selection and having separate code. On 9/7/2018 1:53 PM, Anthony Scarpino wrote: Adam, I tend to agree with Mike that disallowing import/export of keys using BigInteger is not the value of a branchless implementation. As you point out in the JEP the provider is greatly hindered by this design choice. I feel it would be better to implementing the BigInteger parts and have a property to shut them off for a pure branchless implementation. That should allow the provider to be used in the default provider list and the ‘opt-in’ would be the property to turn off BigInteger or any other branching situation. I am concerned the desire for a purest provider will result in it being unused. Documentation can be clear about the import/export situation, the preference toward PKCS8EncodedKeySpec, and the property to lock it down. Tony On Aug 23, 2018, at 10:50 AM, Adam Petcher wrote: I'm starting work on yet another ECC JEP[1], this time with the goal of developing improved implementations of existing algorithms, rather than implementing new ones. The JEP will re-implement ECDH and ECDSA for the 256-, 384-, and 521-bit NIST prime curves. The new implementation will be all Java, and will resist side-channel attacks by not branching on secrets. It will go in a new provider which is not in the provider list in the java.security file by default. So it will need to be manually enabled by changing the configuration or putting the new provider name in the code. It will only support a subset of the API that is supported by the implementation in SunEC. In particular, it will reject any private keys with scalar values specified using BigInteger (as in ECPrivateKeySpec), and its private keys will not return scalar values as BigInteger (as in ECPrivateKey.getS()). Please take a look and send me any feedback you have. I'm especially looking for suggestions on how this new implementation should fit into the API. I would prefer to have it enabled by default, but I can't think of a way to do that without either branching on secrets in some cases (converting a BigInteger private key to an array) or breaking compatibility (throwing an exception when it gets a BigInteger private key). [1] https://bugs.openjdk.java.net/browse/JDK-8204574
Re: RFR 8171279: Support X25519 and X448 in TLS 1.3
On 9/10/2018 8:14 AM, Adam Petcher wrote:
Xuelei,
Here is the latest webrev:
http://cr.openjdk.java.net/~apetcher/8171279/webrev.04/
I modified the TLS implementation so that it uses X509EncodedKeySpec
when interacting with the provider. I did a small amount of refactoring
in X509Key to expose the functionality I needed to do this.
I don't think it is not straightforward choice to us X509EncodedKeySpec.
We have to use the right OID, and make sure the key bytes for X.509
protocols are exactly the same as TLS. Not mention to the cost to add
the OID and remove the OID.
Maybe, using EncodedKeySpec is more simple.
package sun.security.ssl;
private class XDHEncodeKeySpec extension EncodedKeySpec {
// algorithm: x25519 or x448
XDHEncodeKeySpec(byte[] encodedKey, String algorithm) {
super(encodedKey, algorithm);
}
@Override
String getFormat() {
return "raw";
}
...
}
package sun.security.ec;
public class XDHKeyFactory extends KeyFactorySpi {
private PublicKey generatePublicImpl(KeySpec keySpec)
throws InvalidKeyException, InvalidKeySpecException {
...
} else if (keySpec instanceof EncodedKeySpec &&
encodedKeySpec.getAlgorithm() is "x25519" &&
encodedKeySpec.getFormat is "raw") {
// raw public key
byte[] radPublicKey = encodedKeySpec.getEncoded();
} else {
}
}
}
This change
removed the dependency on XECParameters, and I moved it back into
jdk.crypto.ec. I also moved some more functions into NamedGroup (e.g.
isAvailableGroup).
This webrev also has the change to NamedGroup that makes
NamedGroupFunctions private. I put the NamedGroup enum into its own
file, which I think is reasonable because it is used in several places
other than SupportedGroupsExtension. It also makes sense to separate all
of the known named groups and their properties (NamedGroups.java) from
the supported_groups extension and the logic related to which groups are
supported for key exchange (SupportedGroupsExtension.java). This change
required changes to the import statements of several files.
They do not sound like good reasons to me. You may not want to do that
again if you understand the following questions you have.
I'm not totally sure I understood your concern related to the
AlgorithmParameters map. The map is always created, but we only create
and cache parameters when they are first needed.
In the current implementation, there is only one map and the parameters
are not create unless they are supported.
In your implementation, there are three maps, which are created always,
even if FFDHE/XDH is not supported. This is a minor issue to me.
If I did not miss something, in your implementation, the parameters can
be created and push to the map even it is not supported? Am I right?
We used to try avoid to generate parameters for unsupported groups.
Of course, the map is
not a proper cache, and the objects stay in memory forever once they are
created. Is this your concern, or is it something else?
I've made several structural changes since the first webrev, and I
haven't been paying too close attention to style, so you probably
shouldn't either. Once the approach and structure look good to you, I
can clean up the code and submit another webrev so you can check it for
style, formatting, etc. I also haven't merged in changes from the
default branch in a while, so I'll need to do that, too.
Yes, let's work out the big picture at first.
Help it helps.
Xuelei
On 9/7/2018 12:12 PM, Xuelei Fan wrote:
On 9/7/2018 9:03 AM, Adam Petcher wrote:
This is more clear, thanks. See below.
On 9/7/2018 11:34 AM, Xuelei Fan wrote:
EncodedKeySpec keySpec = ... // find a way to construct the keySpec
// at least, we can use:
// new EncodedKeySpec(byte[]);
// But please check if there's a better
one
Do you mean X509EncodedKeySpec, or some class that is specific to XDH?
I did not search the spec definitions. At least we can use the
EncodedKeySpec class. It's nice if you can find a better one, or
define a new one for XDH.
Your following comments make sense to me.
Thanks,
Xuelei
An X509EncodedKeySpec would probably work---we would just need to put
the key into a SubjectPublicKeyInfo, which means I need the OID. Is
it okay for me to put the OID in JSSE? I could put it in the
NamedGroup enum, like this:
X25519 (0x001D, "x25519", true, "x25519", "1.3.101.110",
ProtocolVersion.PROTOCOLS_TO_13),
X448 (0x001E, "x448", true, "x448", "1.3.101.111",
ProtocolVersion.PRO
Re: Conceptual feedback on new ECC JEP
Can I have the links to the new formulas that you will be used? Are they part of any current standards? Thanks, Xuelei On 8/23/2018 10:50 AM, Adam Petcher wrote: I'm starting work on yet another ECC JEP[1], this time with the goal of developing improved implementations of existing algorithms, rather than implementing new ones. The JEP will re-implement ECDH and ECDSA for the 256-, 384-, and 521-bit NIST prime curves. The new implementation will be all Java, and will resist side-channel attacks by not branching on secrets. It will go in a new provider which is not in the provider list in the java.security file by default. So it will need to be manually enabled by changing the configuration or putting the new provider name in the code. It will only support a subset of the API that is supported by the implementation in SunEC. In particular, it will reject any private keys with scalar values specified using BigInteger (as in ECPrivateKeySpec), and its private keys will not return scalar values as BigInteger (as in ECPrivateKey.getS()). Please take a look and send me any feedback you have. I'm especially looking for suggestions on how this new implementation should fit into the API. I would prefer to have it enabled by default, but I can't think of a way to do that without either branching on secrets in some cases (converting a BigInteger private key to an array) or breaking compatibility (throwing an exception when it gets a BigInteger private key). [1] https://bugs.openjdk.java.net/browse/JDK-8204574
Re: Conceptual feedback on new ECC JEP
On 9/10/2018 1:23 PM, Adam Petcher wrote: The paper[1] containing the proposed formulas is referenced in the JEP. Thanks! As far as I know, they are not part of any standard. If you know of any standardized, complete EC point arithmetic formulas, then let me know. I don't know, either. I think it is okay to use these formulas as long as they produce the same result as the operations in Section 2.2 of SEC 1[2]. The motivation of the JEP is that some formulas may be more easier to expose attacks. It's true that the formulas impact the security level of the implementation. I was just wondering if the JEP proposed formulas have been well analyze. A standard or formal recommendation may indicate the good quality of the formulas. It's a concern to me that interoperability is listed as "non-goals". In general, it may introduce a lot of problems in the current JCE framework. I don't know your detailed design yet, and not sure if you are able to mitigate the impact. Looks like the debate was mainly about the BigInteger. If the keys are used in the same provider, I don't think the BigInteger is a problem as you can extend a private BigInteger that you like. If the keys are use between two providers, add a thine clue to export/import BigInteger may be worthy for provider interoperability. I'm a little bit nervous about the two providers design. It simplify the initial crypto implementation, but left the complexity to developers and sustaining. I don't have a good sense about how to use them in JSSE. What if the proposed formulas is vulnerable in the future? If you believe it is a better one, please consider to replace the current EC implementation. I know it may only support secp256r1/secp384r1/secp521r1 now, but we can use it to replace the implementation of the three curves for now. Thanks, Xuelei [1] https://eprint.iacr.org/2015/1060.pdf [2] http://www.secg.org/sec1-v2.pdf On 9/10/2018 2:23 PM, Xuelei Fan wrote: Can I have the links to the new formulas that you will be used? Are they part of any current standards? Thanks, Xuelei On 8/23/2018 10:50 AM, Adam Petcher wrote: I'm starting work on yet another ECC JEP[1], this time with the goal of developing improved implementations of existing algorithms, rather than implementing new ones. The JEP will re-implement ECDH and ECDSA for the 256-, 384-, and 521-bit NIST prime curves. The new implementation will be all Java, and will resist side-channel attacks by not branching on secrets. It will go in a new provider which is not in the provider list in the java.security file by default. So it will need to be manually enabled by changing the configuration or putting the new provider name in the code. It will only support a subset of the API that is supported by the implementation in SunEC. In particular, it will reject any private keys with scalar values specified using BigInteger (as in ECPrivateKeySpec), and its private keys will not return scalar values as BigInteger (as in ECPrivateKey.getS()). Please take a look and send me any feedback you have. I'm especially looking for suggestions on how this new implementation should fit into the API. I would prefer to have it enabled by default, but I can't think of a way to do that without either branching on secrets in some cases (converting a BigInteger private key to an array) or breaking compatibility (throwing an exception when it gets a BigInteger private key). [1] https://bugs.openjdk.java.net/browse/JDK-8204574
Re: Expose SSLContextImpl#AbstractTrustManagerWrapper so it can be used with custom SSLEngine / SSLContextSPI implementations as well
Hi Norman, It may be doable by adding a delegation mode to public TrustManagerFactory: TrustManagerFactory.init(X509TrustManager proxy) However, the X509ExtendedTrustManager should be recommended for now since its introducing in JDK 7. Do you know how many users are still using the X509TrustManager implementation? Thanks, Xuelei On 9/11/2018 3:32 AM, Norman Maurer wrote: Hi all, Would it be possible to consider exposing SSLContextImpl#AbstractTrustManagerWrapper somehow so it would be possible to reuse it when a custom SSLEngine / SSLContextSpi is provided ? I am asking because it provides really nice extra functionality by wrapping for X509TrustManager implementation and do extra hostname checks etc. At the moment we can not make use of this extra functionality in netty with our custom SSLEngine implementation as there is no way to access this. Which means depending on if the user use our implementation or the default implementation the behaviour if quite different when using a X509TrustManager in the sense that when using the default implementation a lot of extra checks are done. As the extra checks done in AbstractTrustManagerWrapper is not really depending on the underlying SSLContextSpi implementation (at least as far as I was able to understand it so far) it would be nice to be able to make use of it. Bye Norman
Code Review Request, JDK-8209916 : NPE in SupportedGroupsExtension
Hi Jamil, Would you please review the fix for the NPE issue: http://cr.openjdk.java.net/~xuelei/8209916/webrev.00/ The issue may happen if the client supports a SunJSSE provider known but not supported named group. Thanks, Xuelei
Re: [PATCH] Trivial typo fix in X509ExtendedKeyManager javadoc
Hi Jaikiran, Thanks for the find and the patch. The patch looks fine to me, and I sponsored the committing and now it is in the JDK repository. http://hg.openjdk.java.net/jdk/jdk/rev/6c394ed56b07 Thanks, Xuelei On 9/14/2018 7:58 PM, Jaikiran Pai wrote: I was looking at some key management code and noticed this typo in X509ExtendedKeyManager. Attached is a trivial patch which fixes it. I'm not a committer but have a signed OCA, so I'll need help from a sponsor. -Jaikiran
Re: TLSv.1.3 interropt problems with OpenSSL 1.1.1 when used on the client side with mutual auth
Hi Norman,
I have not had a chance to look into the details. But sure, it helps a
lot if you can provide a java client to reproduce the issue.
Thanks,
Xuelei
On 9/14/2018 10:29 PM, Norman Maurer wrote:
Is there any more details you need ?
Just wondering. If you say so I can also provide a pure jdk client (without the
Netty wrapper) that shows the problem when used with OpenSSL on the server in
the next days.
Bye
Norman
Am 13.09.2018 um 21:07 schrieb Norman Maurer :
Hi all,
I am currently in the process of adding TLS 1.3 support into netty-tcnative[1]
which uses JNI to make use of OpenSSL for it. During this work I noticed that I
received test-failures when mutual auth is used and the JDK implementation is
used on the client side. When using the JDK implementation on the server and
client side all works as expected. Also if I use another protocol (like
TLSv1.2) all works as expected.
The problem I am observing is that the client seems to sent the certificate
“too late” and so the server (which uses openssl) will report and error that
the client did not provide an certificate (even when it was required).
To reproduce this you can use openssl s_server like this and just create your
usual SSLSocket with a KeyManagerFactory configured.
./bin/openssl s_server -tls1_3 -cert
~/Documents/workspace/netty/handler/src/test/resources/io/netty/handler/ssl/test.crt
-key
~/Documents/workspace/netty/handler/src/test/resources/io/netty/handler/ssl/test_unencrypted.pem
-4 -accept localhost:8443 -state -debug -Verify 1
When now try to connect to it via the JDK TLS1.3 implementation I see the
following output:
SSL_accept:before SSL initialization
read from 0x7fe400f050c0 [0x7fe40300f603] (5 bytes => 5 (0x5))
- 16 03 03 01 60`
read from 0x7fe400f050c0 [0x7fe40300f608] (352 bytes => 352 (0x160))
- 01 00 01 5c 03 03 22 da-02 d7 86 40 6e 7d c5 a7 ...\.."@n}..
0010 - ea 34 47 a4 fa d0 bb 92-f5 62 ec f6 21 e5 ec da .4G..b..!...
0020 - d6 6b 75 aa b9 34 20 b7-57 a6 83 7b c8 bc a2 0f .ku..4 .W..{
0030 - 52 82 11 6f a3 1a 84 c5-4b fd e0 80 58 3c 2a bf R..oK...X<*.
0040 - af 54 32 4c 7d 4f fe 00-14 c0 2c c0 2b c0 2f c0 .T2L}O,.+./.
0050 - 13 c0 14 00 9c 00 2f 00-35 13 01 13 02 01 00 00 ../.5...
0060 - ff 00 05 00 05 01 00 00-00 00 00 0a 00 20 00 1e . ..
0070 - 00 17 00 18 00 19 00 09-00 0a 00 0b 00 0c 00 0d
0080 - 00 0e 00 16 01 00 01 01-01 02 01 03 01 04 00 0b
0090 - 00 02 01 00 00 0d 00 28-00 26 04 03 05 03 06 03 ...(.&..
00a0 - 08 04 08 05 08 06 08 09-08 0a 08 0b 04 01 05 01
00b0 - 06 01 04 02 03 03 03 01-03 02 02 03 02 01 02 02
00c0 - 00 32 00 28 00 26 04 03-05 03 06 03 08 04 08 05 .2.(.&..
00d0 - 08 06 08 09 08 0a 08 0b-04 01 05 01 06 01 04 02
00e0 - 03 03 03 01 03 02 02 03-02 01 02 02 00 11 00 09
00f0 - 00 07 02 00 04 00 00 00-00 00 17 00 00 00 2b 00 ..+.
0100 - 09 08 03 04 03 03 03 02-03 01 00 2d 00 02 01 01 ...-
0110 - 00 33 00 47 00 45 00 17-00 41 04 4e da b3 f2 63 .3.G.E...A.N...c
0120 - ee 6e bf e3 af 73 be c9-92 c5 ec 70 ff c7 64 b8 .n...s.p..d.
0130 - 8a 9a cc fd f9 d6 36 ef-ce e0 dc 81 01 2f 87 57 ..6../.W
0140 - 56 f0 e4 2d 8b c8 73 14-eb 5f 21 0a 5e 94 46 ba V..-..s.._!.^.F.
0150 - de d1 33 57 4c b5 b3 66-c9 26 fb ff 01 00 01 00 ..3WL..f.&..
SSL_accept:before SSL initialization
SSL_accept:SSLv3/TLS read client hello
SSL_accept:SSLv3/TLS write server hello
SSL_accept:SSLv3/TLS write change cipher spec
SSL_accept:TLSv1.3 write encrypted extensions
SSL_accept:SSLv3/TLS write certificate request
SSL_accept:SSLv3/TLS write certificate
SSL_accept:TLSv1.3 write server certificate verify
write to 0x7fe400f050c0 [0x7fe403018a00] (1430 bytes => 1430 (0x596))
- 16 03 03 00 9b 02 00 00-97 03 03 bc 7f 3b 07 ad .;..
0010 - fb 21 9c 6f 7c 4a 9d 84-9a 82 6e 9c 1a b4 e3 5d .!.o|Jn]
0020 - a8 d3 9d 52 a7 e1 93 c3-cc 8c 82 20 b7 57 a6 83 ...R... .W..
0030 - 7b c8 bc a2 0f 52 82 11-6f a3 1a 84 c5 4b fd e0 {R..oK..
0040 - 80 58 3c 2a bf af 54 32-4c 7d 4f fe 13 01 00 00 .X<*..T2L}O.
0050 - 4f 00 2b 00 02 03 04 00-33 00 45 00 17 00 41 04 O.+.3.E...A.
0060 - 7d 81 11 ab ff a6 60 e7-5f 23 82 ed 22 35 76 24 }.`._#.."5v$
0070 - b0 47 09 25 0c 79 b9 07-5b 3e 28 b7 3c d8 d3 ce .G.%.y..[>(.<...
0080 - 6b 89 c6 01 21 28 c9 97-ae 50 a5 e7 43 35 ae c7 k...!(...P..C5..
0090 - 73 10 60 62 57 25 9b c9-f1 93 28 70 03 44 e1 a0 s.`bW%(p.D..
00a0 - 14 03 03 00 01 01 17 03-03 00 27 0f 8b fb 2d 33 ..'...-3
00b0 - 72 c6 a8 28 0b 7d e1 c3-b7 d0 f3 d9 18 5b ca e0 r..(.}...[..
00c0 - 56 09 74 48 ba 28 16 1c-15 11 d9 fa 6e b3 bc b9 V.tH.(..n...
00d0 - 4d 54 17 03 03 00 42 35-53 5b 9a 8e 09 df 86 c4 MTB5S[..
00e0 - 00 28 05 6d a8 c9 bb 38-e2 77
Re: Expose SSLContextImpl#AbstractTrustManagerWrapper so it can be used with custom SSLEngine / SSLContextSPI implementations as well
Hi Norman, Please file an issue for the tracking. Thanks, Xuelei On 9/17/2018 5:57 AM, Norman Maurer wrote: Should I open an issue somewhere to keep track of it or will you take care of it ? Bye Norman On 11. Sep 2018, at 17:01, Norman Maurer wrote: This sounds great. I have no idea how many people still use X509TrustManager, sorry. It may be a good idea to add something to the java docs to tell people to prefer X509ExtendedTrustManager as well. Bye Norman Am 11.09.2018 um 16:44 schrieb Xuelei Fan : Hi Norman, It may be doable by adding a delegation mode to public TrustManagerFactory: TrustManagerFactory.init(X509TrustManager proxy) However, the X509ExtendedTrustManager should be recommended for now since its introducing in JDK 7. Do you know how many users are still using the X509TrustManager implementation? Thanks, Xuelei On 9/11/2018 3:32 AM, Norman Maurer wrote: Hi all, Would it be possible to consider exposing SSLContextImpl#AbstractTrustManagerWrapper somehow so it would be possible to reuse it when a custom SSLEngine / SSLContextSpi is provided ? I am asking because it provides really nice extra functionality by wrapping for X509TrustManager implementation and do extra hostname checks etc. At the moment we can not make use of this extra functionality in netty with our custom SSLEngine implementation as there is no way to access this. Which means depending on if the user use our implementation or the default implementation the behaviour if quite different when using a X509TrustManager in the sense that when using the default implementation a lot of extra checks are done. As the extra checks done in AbstractTrustManagerWrapper is not really depending on the underlying SSLContextSpi implementation (at least as far as I was able to understand it so far) it would be nice to be able to make use of it. Bye Norman
Re: TLSv.1.3 interropt problems with OpenSSL 1.1.1 when used on the client side with mutual auth
Hi Norman, Thank you so much for the reproducing code. Would you mind file a bug for this issue? Thanks, Xuelei On 9/17/2018 3:39 AM, Norman Maurer wrote: Hi all, As requested I pushed a pure JDK reproducer to GitHub which you can easily use to reproduce the problem. All the details how to run it etc are in the README.md file. I also included a server to show that all works if we use the JDK on the client side and server side. Also as stated before you will see that the cert will be send even if you use OpenSSL on the serverside if you replace “-Verify 1” with “-verify 1” (which is kind of the same as setWantClientAuth(true)). Please don't hesitate to ping me if you need any more details or have any more questions. https://github.com/normanmaurer/jdktls13bugreproducer Here is the output with debugging enabled on the client side. javax.net.ssl|DEBUG|01|main|2018-09-17 11:51:54.515 CEST|SSLContextImpl.java:427|System property jdk.tls.client.cipherSuites is set to 'null' javax.net.ssl|DEBUG|01|main|2018-09-17 11:51:54.529 CEST|SSLContextImpl.java:427|System property jdk.tls.server.cipherSuites is set to 'null' javax.net.ssl|DEBUG|01|main|2018-09-17 11:51:54.563 CEST|SSLCipher.java:437|jdk.tls.keyLimits: entry = AES/GCM/NoPadding KeyUpdate 2^37. AES/GCM/NOPADDING:KEYUPDATE = 137438953472 javax.net.ssl|DEBUG|01|main|2018-09-17 11:51:54.577 CEST|SSLContextImpl.java:401|Ignore disabled cipher suite: TLS_ECDHE_ECDSA_WITH_3DES_EDE_CBC_SHA javax.net.ssl|ALL|01|main|2018-09-17 11:51:54.577 CEST|SSLContextImpl.java:410|Ignore unsupported cipher suite: TLS_ECDHE_ECDSA_WITH_3DES_EDE_CBC_SHA javax.net.ssl|DEBUG|01|main|2018-09-17 11:51:54.578 CEST|SSLContextImpl.java:401|Ignore disabled cipher suite: TLS_ECDHE_RSA_WITH_3DES_EDE_CBC_SHA javax.net.ssl|ALL|01|main|2018-09-17 11:51:54.578 CEST|SSLContextImpl.java:410|Ignore unsupported cipher suite: TLS_ECDHE_RSA_WITH_3DES_EDE_CBC_SHA javax.net.ssl|DEBUG|01|main|2018-09-17 11:51:54.578 CEST|SSLContextImpl.java:401|Ignore disabled cipher suite: SSL_RSA_WITH_3DES_EDE_CBC_SHA javax.net.ssl|ALL|01|main|2018-09-17 11:51:54.578 CEST|SSLContextImpl.java:410|Ignore unsupported cipher suite: SSL_RSA_WITH_3DES_EDE_CBC_SHA javax.net.ssl|DEBUG|01|main|2018-09-17 11:51:54.578 CEST|SSLContextImpl.java:401|Ignore disabled cipher suite: TLS_ECDH_ECDSA_WITH_3DES_EDE_CBC_SHA javax.net.ssl|ALL|01|main|2018-09-17 11:51:54.579 CEST|SSLContextImpl.java:410|Ignore unsupported cipher suite: TLS_ECDH_ECDSA_WITH_3DES_EDE_CBC_SHA javax.net.ssl|DEBUG|01|main|2018-09-17 11:51:54.579 CEST|SSLContextImpl.java:401|Ignore disabled cipher suite: TLS_ECDH_RSA_WITH_3DES_EDE_CBC_SHA javax.net.ssl|ALL|01|main|2018-09-17 11:51:54.579 CEST|SSLContextImpl.java:410|Ignore unsupported cipher suite: TLS_ECDH_RSA_WITH_3DES_EDE_CBC_SHA javax.net.ssl|DEBUG|01|main|2018-09-17 11:51:54.579 CEST|SSLContextImpl.java:401|Ignore disabled cipher suite: SSL_DHE_RSA_WITH_3DES_EDE_CBC_SHA javax.net.ssl|ALL|01|main|2018-09-17 11:51:54.579 CEST|SSLContextImpl.java:410|Ignore unsupported cipher suite: SSL_DHE_RSA_WITH_3DES_EDE_CBC_SHA javax.net.ssl|DEBUG|01|main|2018-09-17 11:51:54.580 CEST|SSLContextImpl.java:401|Ignore disabled cipher suite: SSL_DHE_DSS_WITH_3DES_EDE_CBC_SHA javax.net.ssl|ALL|01|main|2018-09-17 11:51:54.580 CEST|SSLContextImpl.java:410|Ignore unsupported cipher suite: SSL_DHE_DSS_WITH_3DES_EDE_CBC_SHA javax.net.ssl|DEBUG|01|main|2018-09-17 11:51:54.581 CEST|SSLContextImpl.java:401|Ignore disabled cipher suite: TLS_ECDH_anon_WITH_3DES_EDE_CBC_SHA javax.net.ssl|ALL|01|main|2018-09-17 11:51:54.581 CEST|SSLContextImpl.java:410|Ignore unsupported cipher suite: TLS_ECDH_anon_WITH_3DES_EDE_CBC_SHA javax.net.ssl|DEBUG|01|main|2018-09-17 11:51:54.581 CEST|SSLContextImpl.java:401|Ignore disabled cipher suite: SSL_DH_anon_WITH_3DES_EDE_CBC_SHA javax.net.ssl|ALL|01|main|2018-09-17 11:51:54.581 CEST|SSLContextImpl.java:410|Ignore unsupported cipher suite: SSL_DH_anon_WITH_3DES_EDE_CBC_SHA javax.net.ssl|DEBUG|01|main|2018-09-17 11:51:54.581 CEST|SSLContextImpl.java:401|Ignore disabled cipher suite: TLS_ECDHE_ECDSA_WITH_RC4_128_SHA javax.net.ssl|ALL|01|main|2018-09-17 11:51:54.582 CEST|SSLContextImpl.java:410|Ignore unsupported cipher suite: TLS_ECDHE_ECDSA_WITH_RC4_128_SHA javax.net.ssl|DEBUG|01|main|2018-09-17 11:51:54.582 CEST|SSLContextImpl.java:401|Ignore disabled cipher suite: TLS_ECDHE_RSA_WITH_RC4_128_SHA javax.net.ssl|ALL|01|main|2018-09-17 11:51:54.582 CEST|SSLContextImpl.java:410|Ignore unsupported cipher suite: TLS_ECDHE_RSA_WITH_RC4_128_SHA javax.net.ssl|DEBUG|01|main|2018-09-17 11:51:54.582 CEST|SSLContextImpl.java:401|Ignore disabled cipher suite: SSL_RSA_WITH_RC4_128_SHA javax.net.ssl|ALL|01|main|2018-09-17 11:51:54.582 CEST|SSLContextImpl.java:410|Ignore unsupported cipher suite: SSL_RSA_WITH_RC4_128_SHA javax.net.ssl|DEBUG|01|main|2018-09-17 11:51:54.582 CEST|SSLContextImpl.java:401|Ignore disabled cipher suite: TLS_ECDH_ECDSA_WITH_RC4
Re: sun.security.ssl.ProtocolVersion.valueOf(...) in Java8 and TLSv1.3
Hi Norman, In general, it is risk to support unknown protocol version in the key/trust manager implementation. The trust manager implemented for TLS 1.2 and prior versions might not work with TLS 1.3 probably. Did you make the evaluation? Would you mind contribute a patch? Please feel free to file an enhancement request, for further evaluation when there is a chance. Thanks, Xuelei On 9/17/2018 5:28 AM, Norman Maurer wrote: Hi all, In netty we support using OpenSSL for native SSL which recently added TLSv1.3 support as part of OpenSSL 1.1.1. In an ideal world we would be able to use this even with Java8 which is almost true except the fact that sun.security.ssl.ProtocolVersion.valueOf(…) will throw an IllegalArgumentException when the string “TLSv1.3” is provided. This is problematic as this happens during validation in the default TrustManager used by the OpenJDK. The stack looks something like this: java.lang.IllegalArgumentException: TLSv1.3 at sun.security.ssl.ProtocolVersion.valueOf(ProtocolVersion.java:187) at sun.security.ssl.X509TrustManagerImpl.checkTrusted(X509TrustManagerImpl.java:258) at sun.security.ssl.X509TrustManagerImpl.checkServerTrusted(X509TrustManagerImpl.java:136) I could work around this by just wrap the SSLSession and return TLSv1.2 during validation as the same validation should be done as in the TLSv1.2 implementation but this is really just a hack.So I wonder if you would consider to either add support for parsing TLSv1.3 to the internal enum or just handle it more gracefully. Another thing that would work of course is to always provide a custom X509ExtendedTrustManager but I hoped to be able to re-use the default implementation as it already implements a lot of verification logic. WDYT ? Norman
Re: Java 11 RC - Handshake failure in certain specific cases throws a different exception than previous versions
Hi Jaikiran, Normally, the thrown exception class can be an implementation choice, and may be not reliable from version to version. We were trying to use the same exception, but we may miss the use cases. I may suggest to make the code independent from it. However, if the impact is significant, please feel free file a bug and we will evaluate if there is something we can do. Thanks, Xuelei On 9/17/2018 6:30 PM, Jaikiran Pai wrote: Just checking back on this one. Is this an expected change? Personally, it's not a big issue in the code where this is happening for me. I'll probably just change the catch block to a more generic IOException. However, for any other code which relied on the previous SocketException catch block, they will now have to expect a different exception depending on what version of Java runtime it's running against. -Jaikiran On 12/09/18 9:11 PM, Jaikiran Pai wrote: Please consider the code that's at the end of this mail. It is a simple client/server code where a HTTPS server is created and set to "needClientAuth". The client then uses HttpsURLConnection and (in this case intentionally) doesn't present any certificate, expecting the handshake to fail. In previous versions of Java, the handshake failure in this code would throw a java.net.SocketException as below: Exception in thread "main" java.net.SocketException: Connection reset at java.net.SocketInputStream.read(SocketInputStream.java:210) at java.net.SocketInputStream.read(SocketInputStream.java:141) at sun.security.ssl.InputRecord.readFully(InputRecord.java:465) at sun.security.ssl.InputRecord.read(InputRecord.java:503) at sun.security.ssl.SSLSocketImpl.readRecord(SSLSocketImpl.java:983) at sun.security.ssl.SSLSocketImpl.waitForClose(SSLSocketImpl.java:1779) at sun.security.ssl.HandshakeOutStream.flush(HandshakeOutStream.java:124) at sun.security.ssl.Handshaker.sendChangeCipherSpec(Handshaker.java:1156) at sun.security.ssl.ClientHandshaker.sendChangeCipherAndFinish(ClientHandshaker.java:1266) at sun.security.ssl.ClientHandshaker.serverHelloDone(ClientHandshaker.java:1178) at sun.security.ssl.ClientHandshaker.processMessage(ClientHandshaker.java:348) at sun.security.ssl.Handshaker.processLoop(Handshaker.java:1052) at sun.security.ssl.Handshaker.process_record(Handshaker.java:987) at sun.security.ssl.SSLSocketImpl.readRecord(SSLSocketImpl.java:1072) at sun.security.ssl.SSLSocketImpl.performInitialHandshake(SSLSocketImpl.java:1385) at sun.security.ssl.SSLSocketImpl.startHandshake(SSLSocketImpl.java:1413) at sun.security.ssl.SSLSocketImpl.startHandshake(SSLSocketImpl.java:1397) at sun.net.www.protocol.https.HttpsClient.afterConnect(HttpsClient.java:559) at sun.net.www.protocol.https.AbstractDelegateHttpsURLConnection.connect(AbstractDelegateHttpsURLConnection.java:185) at sun.net.www.protocol.http.HttpURLConnection.getInputStream0(HttpURLConnection.java:1564) at sun.net.www.protocol.http.HttpURLConnection.getInputStream(HttpURLConnection.java:1492) at sun.net.www.protocol.https.HttpsURLConnectionImpl.getInputStream(HttpsURLConnectionImpl.java:263) at ClientCertTest.main(ClientCertTest.java:26) However, in the Java 11 (release candidate) as well as Java 12 (upstream), this code now throws a javax.net.ssl.SSLProtocolException with the java.net.SocketException wrapped in it: Exception in thread "main" javax.net.ssl.SSLProtocolException: Connection reset at java.base/sun.security.ssl.Alert.createSSLException(Alert.java:126) at java.base/sun.security.ssl.TransportContext.fatal(TransportContext.java:321) at java.base/sun.security.ssl.TransportContext.fatal(TransportContext.java:264) at java.base/sun.security.ssl.TransportContext.fatal(TransportContext.java:259) at java.base/sun.security.ssl.SSLSocketImpl.handleException(SSLSocketImpl.java:1314) at java.base/sun.security.ssl.SSLSocketImpl$AppInputStream.read(SSLSocketImpl.java:839) at java.base/java.io.BufferedInputStream.fill(BufferedInputStream.java:252) at java.base/java.io.BufferedInputStream.read1(BufferedInputStream.java:292) at java.base/java.io.BufferedInputStream.read(BufferedInputStream.java:351) at java.base/sun.net.www.http.HttpClient.parseHTTPHeader(HttpClient.java:746) at java.base/sun.net.www.http.HttpClient.parseHTTP(HttpClient.java:689) at java.base/sun.net.www.http.HttpClient.parseHTTP(HttpClient.java:717) at java.base/sun.net.www.protocol.http.HttpURLConnection.getInputStream0(HttpURLConnection.java:1604) at java.base/sun.net.www.protocol.http.HttpURLConnection.getInputStream(HttpURLConnection.java:1509) at java.base/sun.net.www.protocol.https.HttpsURLConnectionImpl.getInputStream(HttpsURLConnectionImpl.java:245) at ClientCertTest.main(ClientCertTest.java:26) Caused by: java.net.SocketException: Connection reset at java.base/java.net.SocketInputStream.read(
Re: TLSv.1.3 interropt problems with OpenSSL 1.1.1 when used on the client side with mutual auth
Hi Norman, It is just a initial version set. Thanks, Xuelei On 9/19/2018 8:49 AM, Norman Maurer wrote: I see this is now tracked as https://bugs.openjdk.java.net/projects/JDK/issues/JDK-8210846?filter=allopenissues :) Just one question, I saw it list 12 as fix version. Is this just the initial version set or do you not plan to fix it in Java11 ? IMHO this is a quite an important thing to fix as otherwise you may run into issues when using SSL on the client-side as most real-world apps use OpenSSL on the server-side. Bye Norman On 17. Sep 2018, at 10:44, Norman Maurer <mailto:norman.mau...@googlemail.com>> wrote: Of course not… ID: 9057280 Thanks Norman On 17. Sep 2018, at 19:35, Xuelei Fan <mailto:xuelei@oracle.com>> wrote: Hi Norman, Thank you so much for the reproducing code. Would you mind file a bug for this issue? Thanks, Xuelei On 9/17/2018 3:39 AM, Norman Maurer wrote: Hi all, As requested I pushed a pure JDK reproducer to GitHub which you can easily use to reproduce the problem. All the details how to run it etc are in the README.md file. I also included a server to show that all works if we use the JDK on the client side and server side. Also as stated before you will see that the cert will be send even if you use OpenSSL on the serverside if you replace “-Verify 1” with “-verify 1” (which is kind of the same as setWantClientAuth(true)). Please don't hesitate to ping me if you need any more details or have any more questions. https://github.com/normanmaurer/jdktls13bugreproducer Here is the output with debugging enabled on the client side. javax.net.ssl|DEBUG|01|main|2018-09-17 11:51:54.515 CEST|SSLContextImpl.java:427|System property jdk.tls.client.cipherSuites is set to 'null' javax.net.ssl|DEBUG|01|main|2018-09-17 11:51:54.529 CEST|SSLContextImpl.java:427|System property jdk.tls.server.cipherSuites is set to 'null' javax.net.ssl|DEBUG|01|main|2018-09-17 11:51:54.563 CEST|SSLCipher.java:437|jdk.tls.keyLimits: entry = AES/GCM/NoPadding KeyUpdate 2^37. AES/GCM/NOPADDING:KEYUPDATE = 137438953472 javax.net.ssl|DEBUG|01|main|2018-09-17 11:51:54.577 CEST|SSLContextImpl.java:401|Ignore disabled cipher suite: TLS_ECDHE_ECDSA_WITH_3DES_EDE_CBC_SHA javax.net.ssl|ALL|01|main|2018-09-17 11:51:54.577 CEST|SSLContextImpl.java:410|Ignore unsupported cipher suite: TLS_ECDHE_ECDSA_WITH_3DES_EDE_CBC_SHA javax.net.ssl|DEBUG|01|main|2018-09-17 11:51:54.578 CEST|SSLContextImpl.java:401|Ignore disabled cipher suite: TLS_ECDHE_RSA_WITH_3DES_EDE_CBC_SHA javax.net.ssl|ALL|01|main|2018-09-17 11:51:54.578 CEST|SSLContextImpl.java:410|Ignore unsupported cipher suite: TLS_ECDHE_RSA_WITH_3DES_EDE_CBC_SHA javax.net.ssl|DEBUG|01|main|2018-09-17 11:51:54.578 CEST|SSLContextImpl.java:401|Ignore disabled cipher suite: SSL_RSA_WITH_3DES_EDE_CBC_SHA javax.net.ssl|ALL|01|main|2018-09-17 11:51:54.578 CEST|SSLContextImpl.java:410|Ignore unsupported cipher suite: SSL_RSA_WITH_3DES_EDE_CBC_SHA javax.net.ssl|DEBUG|01|main|2018-09-17 11:51:54.578 CEST|SSLContextImpl.java:401|Ignore disabled cipher suite: TLS_ECDH_ECDSA_WITH_3DES_EDE_CBC_SHA javax.net.ssl|ALL|01|main|2018-09-17 11:51:54.579 CEST|SSLContextImpl.java:410|Ignore unsupported cipher suite: TLS_ECDH_ECDSA_WITH_3DES_EDE_CBC_SHA javax.net.ssl|DEBUG|01|main|2018-09-17 11:51:54.579 CEST|SSLContextImpl.java:401|Ignore disabled cipher suite: TLS_ECDH_RSA_WITH_3DES_EDE_CBC_SHA javax.net.ssl|ALL|01|main|2018-09-17 11:51:54.579 CEST|SSLContextImpl.java:410|Ignore unsupported cipher suite: TLS_ECDH_RSA_WITH_3DES_EDE_CBC_SHA javax.net.ssl|DEBUG|01|main|2018-09-17 11:51:54.579 CEST|SSLContextImpl.java:401|Ignore disabled cipher suite: SSL_DHE_RSA_WITH_3DES_EDE_CBC_SHA javax.net.ssl|ALL|01|main|2018-09-17 11:51:54.579 CEST|SSLContextImpl.java:410|Ignore unsupported cipher suite: SSL_DHE_RSA_WITH_3DES_EDE_CBC_SHA javax.net.ssl|DEBUG|01|main|2018-09-17 11:51:54.580 CEST|SSLContextImpl.java:401|Ignore disabled cipher suite: SSL_DHE_DSS_WITH_3DES_EDE_CBC_SHA javax.net.ssl|ALL|01|main|2018-09-17 11:51:54.580 CEST|SSLContextImpl.java:410|Ignore unsupported cipher suite: SSL_DHE_DSS_WITH_3DES_EDE_CBC_SHA javax.net.ssl|DEBUG|01|main|2018-09-17 11:51:54.581 CEST|SSLContextImpl.java:401|Ignore disabled cipher suite: TLS_ECDH_anon_WITH_3DES_EDE_CBC_SHA javax.net.ssl|ALL|01|main|2018-09-17 11:51:54.581 CEST|SSLContextImpl.java:410|Ignore unsupported cipher suite: TLS_ECDH_anon_WITH_3DES_EDE_CBC_SHA javax.net.ssl|DEBUG|01|main|2018-09-17 11:51:54.581 CEST|SSLContextImpl.java:401|Ignore disabled cipher suite: SSL_DH_anon_WITH_3DES_EDE_CBC_SHA javax.net.ssl|ALL|01|main|2018-09-17 11:51:54.581 CEST|SSLContextImpl.java:410|Ignore unsupported cipher suite: SSL_DH_anon_WITH_3DES_EDE_CBC_SHA javax.net.ssl|DEBUG|01|main|2018-09-17 11:51:54.581 CEST|SSLContextImpl.java:401|Ignore disabled cipher suite: TLS_ECDHE_ECDSA_WITH_RC4_128_SHA javax.net.ssl|ALL|01|main|2018-09-17 11:51:
Re: Java 11 - SSL handshake for ECDH cipher suites trigger Invalid ECDH ServerKeyExchange with non-default security provider
Hi Jaikiran, Does it happen if using JDK crypto provider? Thanks, Xuelei On 9/20/2018 6:16 AM, Jaikiran Pai wrote: Just checking - does this look like a genuine issue? Anything else I can provide to help reproduce this? -Jaikiran On 18/09/18 7:06 PM, Jaikiran Pai wrote: I have been testing some projects that I know of, with Java 11 RC. There's one specific test that has been failing for me, for a while now, during SSL handshake. Took me a while to narrow it down given the size of the application and the nature of the exception. The exception occurs during SSL handshake between a client and a server (both Java and both using Java 11 RC) and it kept throwing: Exception in thread "main" javax.net.ssl.SSLHandshakeException: Invalid ECDH ServerKeyExchange signature at java.base/sun.security.ssl.Alert.createSSLException(Alert.java:128) at java.base/sun.security.ssl.Alert.createSSLException(Alert.java:117) at java.base/sun.security.ssl.TransportContext.fatal(TransportContext.java:308) at java.base/sun.security.ssl.TransportContext.fatal(TransportContext.java:264) at java.base/sun.security.ssl.TransportContext.fatal(TransportContext.java:255) at java.base/sun.security.ssl.ECDHServerKeyExchange$ECDHServerKeyExchangeMessage.(ECDHServerKeyExchange.java:329) at java.base/sun.security.ssl.ECDHServerKeyExchange$ECDHServerKeyExchangeConsumer.consume(ECDHServerKeyExchange.java:535) at java.base/sun.security.ssl.ServerKeyExchange$ServerKeyExchangeConsumer.consume(ServerKeyExchange.java:103) at java.base/sun.security.ssl.SSLHandshake.consume(SSLHandshake.java:392) at java.base/sun.security.ssl.HandshakeContext.dispatch(HandshakeContext.java:444) at java.base/sun.security.ssl.HandshakeContext.dispatch(HandshakeContext.java:421) at java.base/sun.security.ssl.TransportContext.dispatch(TransportContext.java:178) at java.base/sun.security.ssl.SSLTransport.decode(SSLTransport.java:164) at java.base/sun.security.ssl.SSLSocketImpl.decode(SSLSocketImpl.java:1152) at java.base/sun.security.ssl.SSLSocketImpl.readHandshakeRecord(SSLSocketImpl.java:1063) at java.base/sun.security.ssl.SSLSocketImpl.startHandshake(SSLSocketImpl.java:402) at java.base/sun.net.www.protocol.https.HttpsClient.afterConnect(HttpsClient.java:567) at java.base/sun.net.www.protocol.https.AbstractDelegateHttpsURLConnection.connect(AbstractDelegateHttpsURLConnection.java:185) at java.base/sun.net.www.protocol.http.HttpURLConnection.getInputStream0(HttpURLConnection.java:1581) at java.base/sun.net.www.protocol.http.HttpURLConnection.getInputStream(HttpURLConnection.java:1509) at java.base/sun.net.www.protocol.https.HttpsURLConnectionImpl.getInputStream(HttpsURLConnectionImpl.java:245) ... This is consistently reproducible if, in the scheme of things: 1. the cipher suite selected is a ECDHE one. For example TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384. The TLS version itself is TLSv1.2 (both server and client). 2. One side of the client/server, is backed by BouncyCastle as the security provider (very specifically for SignatureSpi). Assuming the server side is using BouncyCastle provider, what's happening is that during the handshake, the server side uses the RSASSA-PSS algorithm, backed by this provider and writes out the signature. The client side uses SunRsaSign (default provider) and tries to verify this RSASSA-PSS signature and fails with that above exception. FWIW, I don't believe the signature algorithm matters as long as the cipher is ECDHE backed and the client and server side use a different security provider. All this works perfectly fine when both the sides use the default provider and bouncycastle isn't involved. I was able to get this reproducible in a very simple server/client standalone program. I think this can even be demonstrated in a jtreg test but I don't have enough experience with it to see how to trigger a server in a separate JVM and then use a client for testing. The reproducer code (Server.java and Client.java) is at the end of this mail along with instructions on how to reproduce it. I was also able to narrow down this issue down to a specific part of the JDK code. sun.security.ssl.SignatureScheme#getSignature[1] inits the Signature instance for either signing or verifying. However it sets up the signature parameters _after_ the init is done and in fact, there's an explicit note[2] stating what/why it's doing it there. I admit, I don't have much knowledge of the Java SSL parts and none in these internal details and don't understand the details of that implementation notes. However, just to try it out, I switched the order of it by using this local patch: diff -r fbb71a7edc1a src/java.base/share/classes/sun/security/ssl/SignatureScheme.java --- a/src/java.base/share/classes/sun/security/ssl/SignatureScheme.java Sat Aug 25 20:16:43 2018 +0530 +++ b/src/java.base/share/classes/sun/security/ssl/SignatureScheme.java Tue Sep 18 1
Re: Java 11 - SSL handshake for ECDH cipher suites trigger Invalid ECDH ServerKeyExchange with non-default security provider
Thanks for the quick reply, Jaikiran! Per your diff code, it sounds like a crypto provider implementation bugs. JDK is using a lazy initialization so that the right provider get used. Third party's provider may not do this way. Would you please help to verify if the parameters get used in BouncyCastle if it is set after the init method? Thanks, Xuelei On 9/20/2018 8:22 AM, Jaikiran Pai wrote: Hello Xuelei, It doesn't happen if both the server side and client side use the JDK crypto provider. However if any one side uses a different crypto provider (bouncycastle in this case) then it throws this exception. -Jaikiran On 20/09/18 8:37 PM, Xuelei Fan wrote: Hi Jaikiran, Does it happen if using JDK crypto provider? Thanks, Xuelei On 9/20/2018 6:16 AM, Jaikiran Pai wrote: Just checking - does this look like a genuine issue? Anything else I can provide to help reproduce this? -Jaikiran On 18/09/18 7:06 PM, Jaikiran Pai wrote: I have been testing some projects that I know of, with Java 11 RC. There's one specific test that has been failing for me, for a while now, during SSL handshake. Took me a while to narrow it down given the size of the application and the nature of the exception. The exception occurs during SSL handshake between a client and a server (both Java and both using Java 11 RC) and it kept throwing: Exception in thread "main" javax.net.ssl.SSLHandshakeException: Invalid ECDH ServerKeyExchange signature at java.base/sun.security.ssl.Alert.createSSLException(Alert.java:128) at java.base/sun.security.ssl.Alert.createSSLException(Alert.java:117) at java.base/sun.security.ssl.TransportContext.fatal(TransportContext.java:308) at java.base/sun.security.ssl.TransportContext.fatal(TransportContext.java:264) at java.base/sun.security.ssl.TransportContext.fatal(TransportContext.java:255) at java.base/sun.security.ssl.ECDHServerKeyExchange$ECDHServerKeyExchangeMessage.(ECDHServerKeyExchange.java:329) at java.base/sun.security.ssl.ECDHServerKeyExchange$ECDHServerKeyExchangeConsumer.consume(ECDHServerKeyExchange.java:535) at java.base/sun.security.ssl.ServerKeyExchange$ServerKeyExchangeConsumer.consume(ServerKeyExchange.java:103) at java.base/sun.security.ssl.SSLHandshake.consume(SSLHandshake.java:392) at java.base/sun.security.ssl.HandshakeContext.dispatch(HandshakeContext.java:444) at java.base/sun.security.ssl.HandshakeContext.dispatch(HandshakeContext.java:421) at java.base/sun.security.ssl.TransportContext.dispatch(TransportContext.java:178) at java.base/sun.security.ssl.SSLTransport.decode(SSLTransport.java:164) at java.base/sun.security.ssl.SSLSocketImpl.decode(SSLSocketImpl.java:1152) at java.base/sun.security.ssl.SSLSocketImpl.readHandshakeRecord(SSLSocketImpl.java:1063) at java.base/sun.security.ssl.SSLSocketImpl.startHandshake(SSLSocketImpl.java:402) at java.base/sun.net.www.protocol.https.HttpsClient.afterConnect(HttpsClient.java:567) at java.base/sun.net.www.protocol.https.AbstractDelegateHttpsURLConnection.connect(AbstractDelegateHttpsURLConnection.java:185) at java.base/sun.net.www.protocol.http.HttpURLConnection.getInputStream0(HttpURLConnection.java:1581) at java.base/sun.net.www.protocol.http.HttpURLConnection.getInputStream(HttpURLConnection.java:1509) at java.base/sun.net.www.protocol.https.HttpsURLConnectionImpl.getInputStream(HttpsURLConnectionImpl.java:245) ... This is consistently reproducible if, in the scheme of things: 1. the cipher suite selected is a ECDHE one. For example TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384. The TLS version itself is TLSv1.2 (both server and client). 2. One side of the client/server, is backed by BouncyCastle as the security provider (very specifically for SignatureSpi). Assuming the server side is using BouncyCastle provider, what's happening is that during the handshake, the server side uses the RSASSA-PSS algorithm, backed by this provider and writes out the signature. The client side uses SunRsaSign (default provider) and tries to verify this RSASSA-PSS signature and fails with that above exception. FWIW, I don't believe the signature algorithm matters as long as the cipher is ECDHE backed and the client and server side use a different security provider. All this works perfectly fine when both the sides use the default provider and bouncycastle isn't involved. I was able to get this reproducible in a very simple server/client standalone program. I think this can even be demonstrated in a jtreg test but I don't have enough experience with it to see how to trigger a server in a separate JVM and then use a client for testing. The reproducer code (Server.java and Client.java) is at the end of this mail along with instructions on how to reproduce it. I was also able to narrow down this issue down to a specific part of the JDK code. su
Code Review Request, JDK-8210974 : No extensions debug log for ClientHello
Hi, Please review this simple fix for SunJSSE debug log: http://cr.openjdk.java.net/~xuelei/8210974/webrev.00/ The debug log for ClientHello message does not appear in JDK 12. Trivial update, no new regression test. Thanks, Xuelei
Re: RFR: backport of JDK-8209916, JDK-8210334, JDK-8210846 to jdk11u
Looks fine to me. Thanks, Xuelei On 9/20/2018 11:51 PM, Jamil Nimeh wrote: Hello all, This review is for a backport of 3 TLS interoperability issues that have come up over the past week or so. These are already in jdk/jdk. They cover the following issues: * An NPE thrown during processing of the supported groups extension with curves not enabled by default * A problem processing client hellos where pre_shared_key and psk_key_exchange_modes extensions are both omitted * A problem where JSSE TLS clients send empty Certificate messages during client certificate authentication Webrev: http://cr.openjdk.java.net/~jnimeh/reviews/jdk11u-tls-compat/webrev.01/ JBS Issues: https://bugs.openjdk.java.net/browse/JDK-8209916 https://bugs.openjdk.java.net/browse/JDK-8210334 https://bugs.openjdk.java.net/browse/JDK-8210846 Thanks, --Jamil
Re: RFR: JDK-8210918, Add test to exercise server-side client hello processing
Once there is a test case failed, it may be not straightforward to identify which one is failed. Especially, currently, the testing blog may be swallowed if it is too long. Would you please consider one case per test? Or break immediately if a test case failed, instead of waiting for all to complete? Thanks, Xuelei On 9/21/2018 2:35 PM, Jamil Nimeh wrote: Hello all, This adds a test that lets us send different kinds of client hellos to a JSSE server. It can be extended to handle different kinds of corner cases for client hello extension sets as well as fuzzing test cases in the future. It also provides some extra test coverage for JDK-8210334 and JDK-8209916. Webrev: http://cr.openjdk.java.net/~jnimeh/reviews/8210918/webrev.01/ JBS: https://bugs.openjdk.java.net/browse/JDK-8210918 Thanks and have a good weekend, --Jamil
Re: RFR: JDK-8210918, Add test to exercise server-side client hello processing
On 9/21/2018 4:00 PM, Jamil Nimeh wrote: Are you suggesting having multiple run lines or something like that? I think we could do that. I would prefer to to the run lines. I would like to have it run all cases rather than short-circuit on the first failure, as each case doesn't depend on the others. It should be fine to break earlier as normally the test should be passed. Let me play around with the run directives and see if we can make it work more along the lines you want. Thanks! Xuelei --Jamil On 09/21/2018 03:55 PM, Xuelei Fan wrote: Once there is a test case failed, it may be not straightforward to identify which one is failed. Especially, currently, the testing blog may be swallowed if it is too long. Would you please consider one case per test? Or break immediately if a test case failed, instead of waiting for all to complete? Thanks, Xuelei On 9/21/2018 2:35 PM, Jamil Nimeh wrote: Hello all, This adds a test that lets us send different kinds of client hellos to a JSSE server. It can be extended to handle different kinds of corner cases for client hello extension sets as well as fuzzing test cases in the future. It also provides some extra test coverage for JDK-8210334 and JDK-8209916. Webrev: http://cr.openjdk.java.net/~jnimeh/reviews/8210918/webrev.01/ JBS: https://bugs.openjdk.java.net/browse/JDK-8210918 Thanks and have a good weekend, --Jamil
Re: RFR: JDK-8210918, Add test to exercise server-side client hello processing
> On Sep 21, 2018, at 4:45 PM, Jamil Nimeh wrote: > > Hi Xuelei, > > I started getting into making the one test per run approach - having these > controlled from command line args in the run line gets a little weird after a > while. We have different hello messages that are byte arrays, so you have to > map them to strings (easy), but then some test cases (in the future, not now) > might need SSLContexts created with different alg names, might throw > different exceptions, we may want to take slightly different actions based on > how the processClientHello reacts to a given message, etc. Those things are > easier to write into the body of the test. > > Would you be OK with an approach where the output on stdout clearly indicates > a PASS/FAIL for each test it performs? Then if it fails one only needs to > look at stdout to see which test went haywire and go from there. > It would help simplify the failure evaluation. But there is still a problem that when we run a lot test, the debug log may be swallowed, for example over 5000 lines. The result is that the failure output may not appear in the debug log. However, it is a very minor issue. We can consider make the improvement later when we have more cycles. I’m fine with the current code. Thanks, Xuelei > --Jamil > >> On 9/21/2018 4:15 PM, Xuelei Fan wrote: >>> On 9/21/2018 4:00 PM, Jamil Nimeh wrote: >>> Are you suggesting having multiple run lines or something like that? I >>> think we could do that. >> I would prefer to to the run lines. >> >>> I would like to have it run all cases rather than short-circuit on the >>> first failure, as each case doesn't depend on the others. >> It should be fine to break earlier as normally the test should be passed. >> >>> Let me play around with the run directives and see if we can make it work >>> more along the lines you want. >>> >> Thanks! >> >> Xuelei >> >>> --Jamil >>> >>> >>>> On 09/21/2018 03:55 PM, Xuelei Fan wrote: >>>> Once there is a test case failed, it may be not straightforward to >>>> identify which one is failed. Especially, currently, the testing blog may >>>> be swallowed if it is too long. Would you please consider one case per >>>> test? Or break immediately if a test case failed, instead of waiting for >>>> all to complete? >>>> >>>> Thanks, >>>> Xuelei >>>> >>>>> On 9/21/2018 2:35 PM, Jamil Nimeh wrote: >>>>> Hello all, >>>>> >>>>> This adds a test that lets us send different kinds of client hellos to a >>>>> JSSE server. It can be extended to handle different kinds of corner cases >>>>> for client hello extension sets as well as fuzzing test cases in the >>>>> future. It also provides some extra test coverage for JDK-8210334 and >>>>> JDK-8209916. >>>>> >>>>> Webrev: http://cr.openjdk.java.net/~jnimeh/reviews/8210918/webrev.01/ >>>>> JBS: https://bugs.openjdk.java.net/browse/JDK-8210918 >>>>> >>>>> Thanks and have a good weekend, >>>>> --Jamil >>> >
Re: RFR: JDK-8210918, Add test to exercise server-side client hello processing
I like the updated test code. Thanks! Xuelei On 9/22/2018 11:33 AM, Jamil Nimeh wrote: Hi Brad, Xuelei, et al., Thanks for all your comments. I've udpated the test with Brad's findings and made it use separate @run lines for each test as Xuelei requested. Webrev: http://cr.openjdk.java.net/~jnimeh/reviews/8210918/webrev.02 --Jamil On 09/21/2018 02:35 PM, Jamil Nimeh wrote: Hello all, This adds a test that lets us send different kinds of client hellos to a JSSE server. It can be extended to handle different kinds of corner cases for client hello extension sets as well as fuzzing test cases in the future. It also provides some extra test coverage for JDK-8210334 and JDK-8209916. Webrev: http://cr.openjdk.java.net/~jnimeh/reviews/8210918/webrev.01/ JBS: https://bugs.openjdk.java.net/browse/JDK-8210918 Thanks and have a good weekend, --Jamil
Re: NPE during SSL handshake caused by HostnameChecker
Hi Norman,
It looks like a bug to me. Would you please file a new bug?
Thanks,
Xuelei
On 9/22/2018 3:40 PM, Norman Maurer wrote:
Hi all,
I think I found another bug in the the SSL implementation (well really
in the TrustManager related part) which leads to a NPE. I was able to
reproduce this on Java8 and Java11 (ea28) but I am sure it also exists
on 9 and 10.
While trying to write some test code for netty I did something stupid
while creating the SSLEngine by passing a hostname as parameter for the
server which then ended up in an NPE during handshake. I would argue we
should not fail with a NPE.
Basically something like:
SSLEngine serverEngine = serverCtx.createSSLEngine("localhost", -1);
I think this is caused by
sun.security.ssl.X509TrustManagerImpl.checkIdentity(…) missing a null
check for the hostname before calling
sun.security.util.HostnameChecker.match(…)
A full reproduce (which I extracted from my netty testcase) can be
found here (there is a README.md which explains how to run it):
https://github.com/normanmaurer/jdk_ssl_npe_reproducer
The stack I see is:
|Exception in thread "main" java.lang.RuntimeException: Delegated task
threw Exception/Error at
sun.security.ssl.Handshaker.checkThrown(Handshaker.java:1527) at
sun.security.ssl.SSLEngineImpl.checkTaskThrown(SSLEngineImpl.java:535)
at
sun.security.ssl.SSLEngineImpl.writeAppRecord(SSLEngineImpl.java:1214)
at sun.security.ssl.SSLEngineImpl.wrap(SSLEngineImpl.java:1186) at
javax.net.ssl.SSLEngine.wrap(SSLEngine.java:469) at
JDKSslReproducer.handshake(JDKSslReproducer.java:76) at
JDKSslReproducer.main(JDKSslReproducer.java:51) Caused by:
java.lang.NullPointerException at
sun.net.util.IPAddressUtil.textToNumericFormatV4(IPAddressUtil.java:49)
at
sun.net.util.IPAddressUtil.isIPv4LiteralAddress(IPAddressUtil.java:241)
at
sun.security.util.HostnameChecker.isIpAddress(HostnameChecker.java:125)
at sun.security.util.HostnameChecker.match(HostnameChecker.java:93) at
sun.security.ssl.X509TrustManagerImpl.checkIdentity(X509TrustManagerImpl.java:455)
at
sun.security.ssl.AbstractTrustManagerWrapper.checkAdditionalTrust(SSLContextImpl.java:1068)
at
sun.security.ssl.AbstractTrustManagerWrapper.checkServerTrusted(SSLContextImpl.java:1007)
at
sun.security.ssl.ClientHandshaker.serverCertificate(ClientHandshaker.java:1601)
at
sun.security.ssl.ClientHandshaker.processMessage(ClientHandshaker.java:216)
at sun.security.ssl.Handshaker.processLoop(Handshaker.java:1052) at
sun.security.ssl.Handshaker$1.run(Handshaker.java:992) at
sun.security.ssl.Handshaker$1.run(Handshaker.java:989) at
java.security.AccessController.doPrivileged(Native Method) at
sun.security.ssl.Handshaker$DelegatedTask.run(Handshaker.java:1467) at
JDKSslReproducer.runDelegatedTasks(JDKSslReproducer.java:131) at
JDKSslReproducer.handshake(JDKSslReproducer.java:99) ... 1 more|
This only happens if a X509Trustmanager is used (not the Extended
version) and when setEndpointIdentificationAlgorithm(…) is used on the
client-side.
Please let me know if you agree this is a bug and I am happy to open a
bug for it.
Thanks
Norman
Re: Conceptual feedback on new ECC JEP
I did not follow the discussion. But it does not sound right to me to have an application to be provider dependent (#3). I was not confident that a new provider instead of updating the existing provider is a good idea. It might be a significant effort to update existing provider. However, if we don't do that, the cost to use the new provider is not minimal. As we discussed previous, lacking interop could face significant issues and result in complicated coding in practice. Thinking about SunPKCS11 and SunMSCAPI provider, and how many trouble we have had for them, and how many workaround we have patched for them. Unless it is not possible to have an interop-able implementation, I would suggest take more time to have an interop-able design and impl. Is it possible to have an interop-able impl? If it is possible, how much effort will it take? Thanks, Xuelei On 9/25/2018 7:40 AM, Adam Petcher wrote: Thanks, everyone for your feedback on this JEP. I have incorporated this feedback (received on this mailing list and elsewhere) into the draft JEP[1]. Here is a summary of the current JEP and plan: *) A new provider (name TBD) will be developed to hold the new ECC implementation for the three curves. This provider will feature the interoperability-limiting restrictions on its API that were discussed at length on this mailing list. The new provider will be at the end of the list, so it won't be used by default. *) The operations of the new implementation will also be added to SunEC for the three curves. This means that the new implementation will be used by default, in a completely compatible way (without any restrictions on its API). Using the new implementation through SunEC will not provide the same level of security against side-channel attacks as using it through the new provider. *) We will add some tests that make sure that TLS still work when the new provider is used instead of SunEC. We may need to make some small changes to the TLS implementation in order to get these tests to pass. *) A couple of people asked me about whether we could modernize the implementation of more curves in the future. I added a section at the end of the JEP to discuss this. Of course, none of this is set in stone, and we still have some API details to work out in the CSR. I'll be doing the CSR next, and I'm happy to accept feedback at any time. [1] https://bugs.openjdk.java.net/browse/JDK-8204574 On 8/23/2018 1:50 PM, Adam Petcher wrote: I'm starting work on yet another ECC JEP[1], this time with the goal of developing improved implementations of existing algorithms, rather than implementing new ones. The JEP will re-implement ECDH and ECDSA for the 256-, 384-, and 521-bit NIST prime curves. The new implementation will be all Java, and will resist side-channel attacks by not branching on secrets. It will go in a new provider which is not in the provider list in the java.security file by default. So it will need to be manually enabled by changing the configuration or putting the new provider name in the code. It will only support a subset of the API that is supported by the implementation in SunEC. In particular, it will reject any private keys with scalar values specified using BigInteger (as in ECPrivateKeySpec), and its private keys will not return scalar values as BigInteger (as in ECPrivateKey.getS()). Please take a look and send me any feedback you have. I'm especially looking for suggestions on how this new implementation should fit into the API. I would prefer to have it enabled by default, but I can't think of a way to do that without either branching on secrets in some cases (converting a BigInteger private key to an array) or breaking compatibility (throwing an exception when it gets a BigInteger private key). [1] https://bugs.openjdk.java.net/browse/JDK-8204574
Re: Conceptual feedback on new ECC JEP
On 9/25/2018 8:34 AM, Adam Petcher wrote: On 9/25/2018 11:15 AM, Xuelei Fan wrote: I did not follow the discussion. But it does not sound right to me to have an application to be provider dependent (#3). There will be nothing provider-dependent in the TLS implementation. The point of #3 is to say that we should test the TLS implementation to ensure that it will work with either "EC" provider. The only required changes to TLS code will be using PKCS8 private keys instead of BigInteger private keys. I read it as there is no need to change TLS implementation, right? The change from BigInteger private keys to PKCS8 private keys is for test only, right? What if we don't change test code as well? Can an existing application survive if it uses BigInteger private keys (okay, I this is a interop question)? I was not confident that a new provider instead of updating the existing provider is a good idea. It might be a significant effort to update existing provider. However, if we don't do that, the cost to use the new provider is not minimal. As we discussed previous, lacking interop could face significant issues and result in complicated coding in practice. Thinking about SunPKCS11 and SunMSCAPI provider, and how many trouble we have had for them, and how many workaround we have patched for them. Unless it is not possible to have an interop-able implementation, I would suggest take more time to have an interop-able design and impl. Is it possible to have an interop-able impl? If it is possible, how much effort will it take? Yes, it is possible, at the expense of some assurance related to security against side-channel attacks. We may not want to have an impl to expose to side-channel attacks. Okay, let me ask the question in another way. Is it possible to have an interop-able impl without losing the quality of the new formula (side-channel attacks, etc)? How much effort will it take to make it possible (please consider even we have to update the BigInteger APIs as well)? Sorry for so much question, I did not take enough time for the new formula. So I depend on the questions to you so that I can have a better feel of the design. Thanks, Xuelei This interoperable implementation will be available by default in SunEC. A higher-assurance form of the same implementation will be available in the new provider. The additional effort required to put this implementation in both providers is expected to be relatively small.
Re: Conceptual feedback on new ECC JEP
On 9/25/2018 8:34 AM, Adam Petcher wrote: Yes, it is possible, at the expense of some assurance related to security against side-channel attacks. This interoperable implementation will be available by default in SunEC. A higher-assurance form of the same implementation will be available in the new provider. The additional effort required to put this implementation in both providers is expected to be relatively small. Can we have the same security level impl in SunEC in some circumstances? For example, when the key is not imported for the 4 named curves. Using a new provider means we force applications to choose between weak and interop, just because we cannot provide both at the same time. Thanks, Xuelei
Re: RFR: 8210989 TLSv1.2 not authenticating using PSS certificates
Looks nice to me. To help to remember the decision, would you mind add a comment in the T12CertificateRequestConsumer.consume() block about why we don't use the CertificateRequest.certificate_types any more. Maybe, some words like, "Note that the certificate_types field is not used here. The supported_signature_algorithms field has provide sufficient information". Thanks, Xuelei On 10/7/2018 9:33 AM, Jamil Nimeh wrote: Hello all, this fixes an issue where for TLSv1.2 connections specifically, clients will not authenticate using PSS certs even when PSS signature algorithms are asserted in the CertificateRequest message. This brings in a method for client certificate selection similar to how we do it for TLS 1.3. TLS 1.3, 1.1 and 1.0 client certificate selection is not affected by this fix. JBS: https://bugs.openjdk.java.net/browse/JDK-8210989 Webrev: http://cr.openjdk.java.net/~jnimeh/reviews/8210989/webrev.01/ --Jamil
Re: RFR: JDK-8211866 TLS 1.3 CertificateRequest message sometimes offers disallowed signature algorithms
Looks fine to me. Can the following two lines joined into one? Looks like the length does not exceed 80 characters. int vectorLen = SignatureScheme.sizeInRecord() * sigAlgs.size(); Thanks, Xuelei On 10/11/2018 10:11 AM, Jamil Nimeh wrote: Hello all, This fixes an issue with the TLS 1.3 CertificateRequest message. In cases where the server side can initially support multiple protocol versions by the time it issues a CertificateRequest message it collects the list of supported signature schemes for the signature_algorithms and signature_algorithms_cert extensions using all supported protocols as a filtering mechanism. This change alters the filtering process to use only the negotiated protocol, so only those sig algs allowed for that one protocol version will be asserted. Webrev: http://cr.openjdk.java.net/~jnimeh/reviews/8211866/webrev.01/ JBS: https://bugs.openjdk.java.net/browse/JDK-8211866
Re: RFR[12] JDK-8210632: Add key exchange algorithm to javax/net/ssl/TLSCommon/CipherSuite.java
Looks fine to me. There is another potential improvement to divide the ECDHE_RSA, and other KeyExAlgorithms, into two parts, the key exchange algorithm (ECDHE), certificate key algorithm (EC) and the optional certificate signature algorithms (RSA). Thanks, Xuelei On 10/16/2018 1:08 AM, sha.ji...@oracle.com wrote: Ping... On 2018/10/10 11:25, sha.ji...@oracle.com wrote: Hi, It would be better that javax/net/ssl/TLSCommon/CipherSuite.java has an attribute on key exchange algorithm. This attribute could be used on selecting appropriate certificates by some tests. Issue: https://bugs.openjdk.java.net/browse/JDK-8210632 Webrev: http://cr.openjdk.java.net/~jjiang/8210632/webrev.00/ Best regards, John Jiang
Re: Update: RFR JDK-8211806: TLS 1.3 handshake server name indication is missing on a session resume
Looks fine to me. Thanks, Xuelei On 10/19/2018 11:04 AM, Jamil Nimeh wrote: Hello everyone, I've added a test to go along with the bugfix. No changes to the actual fix itself. Updated webrev: http://cr.openjdk.java.net/~jnimeh/reviews/8211806/webrev.02/ Thanks, --Jamil On 10/12/18 9:39 PM, Jamil Nimeh wrote: Hello all, This addresses an issue where the client hello in a resumed TLS 1.3 session lacks the server_name client hello extension. This can cause servers who use this extension field to direct traffic to websites to present other certificate chains for other websites than the one the client actually desires (and specified in the original client hello where the extension is present). JBS: https://bugs.openjdk.java.net/browse/JDK-8211806 Webrev: http://cr.openjdk.java.net/~jnimeh/reviews/8211806/ Happy Friday! --Jamil
Code Review Request, JDK-8212738, Incorrectly named signature scheme ecdsa_secp512r1_sha512
Hi, Please review the update: http://cr.openjdk.java.net/~xuelei/8212738/webrev.00/ The signature algorithm name should be ""ecdsa_secp521r1_sha512", instead of "ecdsa_secp512r1_sha512". No new regression test. Trivial update, no impact on the behaviors except the debug log message. Thanks, Xuelei
A new proposal to add methods to HttpsURLConnection to access SSLSession
Hi, For the current HttpsURLConnection, there is not much security parameters exposed in the public APIs. An application may need richer information for the underlying TLS connections, for example the negotiated TLS protocol version. Please let me know if you have concerns to add a new method HttpsURLConnection.getSSLSession() and deprecate the duplicated methods, by the end of Nov. 2, 2018. Here is the proposal: https://bugs.openjdk.java.net/browse/JDK-8213161 Thanks, Xuelei
Re: A new proposal to add methods to HttpsURLConnection to access SSLSession
On 10/31/2018 8:52 AM, Chris Hegarty wrote: Xuelei, On 30/10/18 20:55, Xuelei Fan wrote: Hi, For the current HttpsURLConnection, there is not much security parameters exposed in the public APIs. An application may need richer information for the underlying TLS connections, for example the negotiated TLS protocol version. Please let me know if you have concerns to add a new method HttpsURLConnection.getSSLSession() and deprecate the duplicated methods, by the end of Nov. 2, 2018. Here is the proposal: https://bugs.openjdk.java.net/browse/JDK-8213161 Thanks, Xuelei The new method looks fine. On the deprecation, minimally the annotation should contain the "since" element, which will have a value of `12`. Hm, it looks better now if using the "since" element. Thanks! Also, I wonder, now that I see the spec, whether or not it is actually a good idea to deprecate these methods. The reason I ask this is that the new method, getSSLSession, can throw UOE, which effectively makes it an optional method. Access to the specific security parameters provided by the existing methods is non-optional. This is a clear difference, and possibly a reason, for folk not interested in the "new" parameters, to continue to use the existing methods. Yes. I had the same concern about the optional operation. However, I came to a different conclusion if I'm from viewpoint of these users that need to use this new operation. If an application have to use this new operation, for example to access the negotiated TLS version, this operation is essential to it. Unfortunately, because of compatibility impact, we cannot force all implementation supports this new added operation. It is a potential problem to those applications that need it. It would be nice if an implementation can support this operation as soon as possible. If we just add the operation, providers may not aware there is a need to update their implementation. Unfortunately. there is not much we can do to notify the provider. If we deprecated the duplicated methods, it is easier for providers to catch up with this new added operation, and move forward to support it. As the deprecating will show up building warnings, or even error if run in strict building mode. To make it more clear, I added a implNote in the getSSLSession() method, and recommend to support it in all implementations. I prefer to deprecate these methods a little bit more, but not very strong. If there is a strong preference, I can live with it. BTW, I also updated the java.net.SecureCacheResponse accordingly. I'm not sure if SecureCacheResponse is really used in practice. I did not find the implementation of it in JDK. Here is the webrev if you want to look at the implementation details: http://cr.openjdk.java.net/~xuelei/8212261/webrev.00/ Thanks, Xuelei
Re: RFR (12): 8212669: Add note to Cipher javadoc about using full transformation and not relying on defaults
What do you think if adding a note that the default value may be different for each provider, and may be changed from time to time with the development of crypto analysis? Xuelei On 11/1/2018 7:57 AM, Sean Mullan wrote: Please review this javadoc-only change to the Cipher class. An @apiNote has been added to each of the getInstance methods to recommend that the full transformation be specified when creating a Cipher and to avoid relying on the defaults. Also a link to the defaults used by the JDK providers has been added as an @implNote. webrev: http://cr.openjdk.java.net/~mullan/webrevs/8212669/webrev.00/ bug: https://bugs.openjdk.java.net/browse/JDK-8212669 Thanks, Sean
Re: RFR (12): 8212669: Add note to Cipher javadoc about using full transformation and not relying on defaults
Okay. Looks fine to me. Thanks, Xuelei On 11/1/2018 8:47 AM, Sean Mullan wrote: On 11/1/18 11:27 AM, Xuelei Fan wrote: What do you think if adding a note that the default value may be different for each provider, and may be changed from time to time with the development of crypto analysis? I didn't want to get too wordy, just to make a concise point that defaults can be problematic and are not recommended. My preference would be to put more wording like that in the security guides. --Sean Xuelei On 11/1/2018 7:57 AM, Sean Mullan wrote: Please review this javadoc-only change to the Cipher class. An @apiNote has been added to each of the getInstance methods to recommend that the full transformation be specified when creating a Cipher and to avoid relying on the defaults. Also a link to the defaults used by the JDK providers has been added as an @implNote. webrev: http://cr.openjdk.java.net/~mullan/webrevs/8212669/webrev.00/ bug: https://bugs.openjdk.java.net/browse/JDK-8212669 Thanks, Sean
Re: A new proposal to add methods to HttpsURLConnection to access SSLSession
I removed the deprecation parts in the update. Here is the new webrev: http://cr.openjdk.java.net/~xuelei/8212261/webrev.01/ And the CSR was updated accordingly. https://bugs.openjdk.java.net/browse/JDK-8213161 Thanks, Xuelei On 11/1/2018 4:57 AM, Chris Hegarty wrote: On 31 Oct 2018, at 20:03, Xuelei Fan <mailto:xuelei@oracle.com>> wrote: ... Yes. I had the same concern about the optional operation. However, I came to a different conclusion if I'm from viewpoint of these users that need to use this new operation. If an application have to use this new operation, for example to access the negotiated TLS version, this operation is essential to it. Unfortunately, because of compatibility impact, we cannot force all implementation supports this new added operation. It is a potential problem to those applications that need it. It would be nice if an implementation can support this operation as soon as possible. If we just add the operation, providers may not aware there is a need to update their implementation. Unfortunately. there is not much we can do to notify the provider. If we deprecated the duplicated methods, it is easier for providers to catch up with this new added operation, and move forward to support it. As the deprecating will show up building warnings, or even error if run in strict building mode. I have no issue with the addition of the new method to access the SSLSession. Unfortunately, due to the evolutionary constraints of this API, the `getSSLSession` method will likely need to be specified to throw UOE. The actual JDK's HTTPS protocol handler implementation will not throw UOE. Clearly there is a desire for non-JDK HTTPS protocol handler implementations to quickly determine the need to update their code and override the default implementation of `getSSLSession` ( to do something useful ), hence the request to deprecate the the existing individual security parameter accessor methods. I don't believe that deprecating these individual security parameter accessors is a good idea for the following reasons: 1) Deprecated warnings are only issued at compile-time, so only HTTPS protocol handler implementations being recompiled may encounter the warnings. Existing binary artifacts will not. 2) More importantly. Forever more, developers wanting access to say, the peer principle or the server's certificate chain, will be encouraged to get them through an optional API ( rather than a non-optional API ). This increases the risk that the code will encounter an UOE. I don't think that this increased risk is justified by the desire to not-have-two-ways-to-do-the-same-thing. I would agree if this were a new API, but it is not. HTTPS protocol handler implementations actively being maintained will likely quickly get requests from their users to provide a better implementation of this new method, as folk move towards TLS 1.3, etc. Maybe such requests will be sufficient to help adoption, at which point the deprecation of these methods could then be re-considered. -Chris.
Re: A new proposal to add methods to HttpsURLConnection to access SSLSession
On 11/1/2018 11:24 AM, Sean Mullan wrote: On 10/31/18 11:52 AM, Chris Hegarty wrote: Xuelei, On 30/10/18 20:55, Xuelei Fan wrote: Hi, For the current HttpsURLConnection, there is not much security parameters exposed in the public APIs. An application may need richer information for the underlying TLS connections, for example the negotiated TLS protocol version. Please let me know if you have concerns to add a new method HttpsURLConnection.getSSLSession() and deprecate the duplicated methods, by the end of Nov. 2, 2018. Here is the proposal: https://bugs.openjdk.java.net/browse/JDK-8213161 Are there any security issues associated with returning the SSLSession, since it is mutable? It should be fine. The update APIs of the session (invalidating, bind values) does not impact the connection. + * SHOULD override this method with appropriate implementation. s/appropriate/an appropriate/ I would probably not capitalize "SHOULD" and just say "should". "SHOULD" is more common in RFCs. I don't see that much in javadocs. + * @implNote The JDK Reference Implementation supports this operation. + * As an application may have to use this operation for more + * security parameters, it is recommended to support this + * operation in all implementations. I think it should be obvious that the JDK implementation would override this method so not sure that first sentence is necessary. The other sentence seems like it could be combined with the previous sentence, ex: "Subclasses should override this method with an appropriate implementation since an application may need to access additional parameters associated with the SSL session." Updated accordingly, in the CSR and webrev: https://bugs.openjdk.java.net/browse/JDK-8213161 http://cr.openjdk.java.net/~xuelei/8212261/webrev.02/ Thanks, Xuelei
Re: A new proposal to add methods to HttpsURLConnection to access SSLSession
Thanks for the review and suggestions, Chris and Sean. I just finalized the CSR. Thanks, Xuelei On 11/2/2018 5:58 AM, Chris Hegarty wrote: Thanks for the updates Xuelei, some minor comments inline.. On 1 Nov 2018, at 23:42, Xuelei Fan <mailto:xuelei@oracle.com>> wrote: On 11/1/2018 11:24 AM, Sean Mullan wrote: On 10/31/18 11:52 AM, Chris Hegarty wrote: Xuelei, On 30/10/18 20:55, Xuelei Fan wrote: Hi, For the current HttpsURLConnection, there is not much security parameters exposed in the public APIs. An application may need richer information for the underlying TLS connections, for example the negotiated TLS protocol version. Please let me know if you have concerns to add a new method HttpsURLConnection.getSSLSession() and deprecate the duplicated methods, by the end of Nov. 2, 2018. Here is the proposal: https://bugs.openjdk.java.net/browse/JDK-8213161 Are there any security issues associated with returning the SSLSession, since it is mutable? It should be fine. The update APIs of the session (invalidating, bind values) does not impact the connection. Alternatively, as is done in the new HTTP Client, an immutable SSLSession instance can be returned. + * SHOULD override this method with appropriate implementation. s/appropriate/an appropriate/ I would probably not capitalize "SHOULD" and just say "should". "SHOULD" is more common in RFCs. I don't see that much in javadocs. + * @implNote The JDK Reference Implementation supports this operation. + * As an application may have to use this operation for more + * security parameters, it is recommended to support this + * operation in all implementations. I think it should be obvious that the JDK implementation would override this method so not sure that first sentence is necessary. The other sentence seems like it could be combined with the previous sentence, ex: "Subclasses should override this method with an appropriate implementation since an application may need to access additional parameters associated with the SSL session." Updated accordingly, in the CSR and webrev: https://bugs.openjdk.java.net/browse/JDK-8213161 The CSR looks good. I made a few minor edits to the verbiage and added myself as reviewer. The title will need to be updated to reflect the addition of the new method in SecureCacheResponse. Maybe: "Add SSLSession accessors to HttpsURLConnection and SecureCacheResponse" -Chris.
Re: A new proposal to add methods to HttpsURLConnection to access SSLSession
Hi Chris and Sean, There are a few feedback for the CSR approval. I updated to use Optional for the returned value. For more details, please refer to: https://bugs.openjdk.java.net/browse/JDK-8213161 http://cr.openjdk.java.net/~xuelei/8212261/webrev.03/ Please let me know if you are OK with this change. Thanks, Xuelei On 11/2/2018 11:42 AM, Xuelei Fan wrote: Thanks for the review and suggestions, Chris and Sean. I just finalized the CSR. Thanks, Xuelei On 11/2/2018 5:58 AM, Chris Hegarty wrote: Thanks for the updates Xuelei, some minor comments inline.. On 1 Nov 2018, at 23:42, Xuelei Fan <mailto:xuelei@oracle.com>> wrote: On 11/1/2018 11:24 AM, Sean Mullan wrote: On 10/31/18 11:52 AM, Chris Hegarty wrote: Xuelei, On 30/10/18 20:55, Xuelei Fan wrote: Hi, For the current HttpsURLConnection, there is not much security parameters exposed in the public APIs. An application may need richer information for the underlying TLS connections, for example the negotiated TLS protocol version. Please let me know if you have concerns to add a new method HttpsURLConnection.getSSLSession() and deprecate the duplicated methods, by the end of Nov. 2, 2018. Here is the proposal: https://bugs.openjdk.java.net/browse/JDK-8213161 Are there any security issues associated with returning the SSLSession, since it is mutable? It should be fine. The update APIs of the session (invalidating, bind values) does not impact the connection. Alternatively, as is done in the new HTTP Client, an immutable SSLSession instance can be returned. + * SHOULD override this method with appropriate implementation. s/appropriate/an appropriate/ I would probably not capitalize "SHOULD" and just say "should". "SHOULD" is more common in RFCs. I don't see that much in javadocs. + * @implNote The JDK Reference Implementation supports this operation. + * As an application may have to use this operation for more + * security parameters, it is recommended to support this + * operation in all implementations. I think it should be obvious that the JDK implementation would override this method so not sure that first sentence is necessary. The other sentence seems like it could be combined with the previous sentence, ex: "Subclasses should override this method with an appropriate implementation since an application may need to access additional parameters associated with the SSL session." Updated accordingly, in the CSR and webrev: https://bugs.openjdk.java.net/browse/JDK-8213161 The CSR looks good. I made a few minor edits to the verbiage and added myself as reviewer. The title will need to be updated to reflect the addition of the new method in SecureCacheResponse. Maybe: "Add SSLSession accessors to HttpsURLConnection and SecureCacheResponse" -Chris.
Re: RFR[s] 8211339: NPE during SSL handshake caused by HostnameChecker
It looks fine to me. Thanks, Xuelei On 11/5/2018 10:59 AM, Anthony Scarpino wrote: Hi, I'd like to get a code review of this simple change. It's a simple check to make sure the hostname variable is not null and throw a proper exception. http://cr.openjdk.java.net/~ascarpino/8211339/webrev.00/ Tony
Re: RFR CSR for 8213400: Support choosing curve name in keytool keypair generation
On 11/5/2018 7:13 PM, Weijun Wang wrote:
Please take a review at the CSR at
https://bugs.openjdk.java.net/browse/JDK-8213401
As for implementation, I intend to report an error when -keyalg is not EC but
-curvename is provided. If both -curvename and -keysize are provided, I intend
to ignore -keysize no matter if they match or not.
Why not use a strict mode: fail if not match. It might be misleading if
ignoring unmatched options.
Another question: in sun.security.util.CurveDB, we have
// Return EC parameters for the specified field size. If there are known
// NIST recommended parameters for the given length, they are returned.
// Otherwise, if there are multiple matches for the given size, an
// arbitrary one is returns.
// If no parameters are known, the method returns null.
// NOTE that this method returns both prime and binary curves.
static NamedCurve lookup(int length) {
return lengthMap.get(length);
}
FIPS 186-4 has 2 recommendations (K- and B-) for a binary curve field size. Do
we have a choice?
In fact, CurveDB.java seems to have a bug when adding the curves:
add("sect163k1 [NIST K-163]", "1.3.132.0.1", BD,...
add("sect163r2 [NIST B-163]", "1.3.132.0.15", BD,... // Another default?
add("sect233k1 [NIST K-233]", "1.3.132.0.26", BD,...
add("sect233r1 [NIST B-233]", "1.3.132.0.27", B,...
and now 163 is sect163r2 and 233 is sect233k1.
I assume we should always prefer the K- one?
TLS 1.3 uses secp256r1/secp384r1/secp521r1, no K- curves.
Do you mean if no -curvename option, there is a need to choose a named
curve?
Xuelei
Re: RFR CSR for 8213400: Support choosing curve name in keytool keypair generation
On 11/5/2018 8:37 PM, Weijun Wang wrote:
On Nov 6, 2018, at 12:12 PM, Xuelei Fan wrote:
On 11/5/2018 7:13 PM, Weijun Wang wrote:
Please take a review at the CSR at
https://bugs.openjdk.java.net/browse/JDK-8213401
As for implementation, I intend to report an error when -keyalg is not EC but
-curvename is provided. If both -curvename and -keysize are provided, I intend
to ignore -keysize no matter if they match or not.
Why not use a strict mode: fail if not match. It might be misleading if
ignoring unmatched options.
We can do that, but misleading to what? That we treat -curvename and -keysize
the same important?
If the option "-keysize 256 -curvename sect163k1" work, I may think that
the key size if 256 bits. I want to create a 256 bits sect163k1 EC key,
and the tool allows this behavior, so I should get a 256 bits sect163k1
EC key. Sure, that's incorrect, but I don't know it is incorrect as the
tool ignore the key size. What's the problem of the command, I don't
know either unless I clearly understand sect163k1 is not 256 bits. The
next question to me, what's the key size actually is? 256 bits or 163
bits? which option are used? It adds more confusing to me.
We can ignore the -keysize option, but it is complicated to me to use
the tool.
Another question: in sun.security.util.CurveDB, we have
// Return EC parameters for the specified field size. If there are known
// NIST recommended parameters for the given length, they are returned.
// Otherwise, if there are multiple matches for the given size, an
// arbitrary one is returns.
// If no parameters are known, the method returns null.
// NOTE that this method returns both prime and binary curves.
static NamedCurve lookup(int length) {
return lengthMap.get(length);
}
FIPS 186-4 has 2 recommendations (K- and B-) for a binary curve field size. Do
we have a choice?
In fact, CurveDB.java seems to have a bug when adding the curves:
add("sect163k1 [NIST K-163]", "1.3.132.0.1", BD,...
add("sect163r2 [NIST B-163]", "1.3.132.0.15", BD,... // Another default?
add("sect233k1 [NIST K-233]", "1.3.132.0.26", BD,...
add("sect233r1 [NIST B-233]", "1.3.132.0.27", B,...
and now 163 is sect163r2 and 233 is sect233k1.
I assume we should always prefer the K- one?
TLS 1.3 uses secp256r1/secp384r1/secp521r1, no K- curves.
There is no ambiguity for prime curves.
Do you mean if no -curvename option, there is a need to choose a named curve?
ECKeyPairGenerator::initialize(int) will choose one and keytool will use it. I
just meant if we have a bug here and if we should be more public on what curve
is chosen.
I see your concerns.
It might be a potential issue if we use a named curve if no curvename
specified. If the compatibility is not serious, I may suggest supported
named curves only, or use arbitrary curves but with a warning.
Xuelei
Re: RFR, JDK-8212885: TLS 1.3 resumed session does not retain peer certificate chain
Nice update! For the update in ClientHello.java, I may suggest moving it to pre_shared_key extension class. It may be a little bit safer if the extension can be loaded in other places. Thanks, Xuelei On 11/5/2018 11:51 PM, Jamil Nimeh wrote: Hello all, This fixes an issue where TLS 1.3 resumed sessions were not carrying forward many of the parameters from the parent session, namely the peer certificates, but also the local certificates and a few other SSLSessionImpl fields. This also moves the fix from an earlier, related issue with SNI names (JDK-8211806) into this new solution. JBS: https://bugs.openjdk.java.net/browse/JDK-8212885 Webrev: http://cr.openjdk.java.net/~jnimeh/reviews/8212885/webrev.01 Thanks, --Jamil
Re: RFR CSR for 8213400: Support choosing curve name in keytool keypair generation
As -curvename is a new option, I would second the comments that don't
allow mixing curve names and keysize at the same time.
Xuelei
On 11/5/2018 11:41 PM, Bernd Eckenfels wrote:
Hello,
I would agree ignoring an (conflicting) option adds confusion. When
specifying a curve is a new feature we don’t need to worry about beeing
compatible, therefore I would forbid mixing curve names and keysize at
all (even when the size matches).
I guess we cannot remove the option to only pass the keysize (to have
the generator pick a curve) to stay compatible. But the chosen curve
should be printed, and I would also deprecate this usage.
I guess clarifying the keysize-only init() method would be a different
topic, but something like deprecating it and restricting the selection
to „SunEC only selects NIST prime curves“ would be a good thing.
Gruss
Bernd
Gruss
Bernd
--
http://bernd.eckenfels.net
*Von:* security-dev im Auftrag
von Xuelei Fan
*Gesendet:* Dienstag, November 6, 2018 7:38 AM
*An:* Weijun Wang
*Cc:* security-dev@openjdk.java.net
*Betreff:* Re: RFR CSR for 8213400: Support choosing curve name in
keytool keypair generation
On 11/5/2018 8:37 PM, Weijun Wang wrote:
>
>> On Nov 6, 2018, at 12:12 PM, Xuelei Fan wrote:
>>
>> On 11/5/2018 7:13 PM, Weijun Wang wrote:
>>> Please take a review at the CSR at
>>> https://bugs.openjdk.java.net/browse/JDK-8213401
>>> As for implementation, I intend to report an error when -keyalg is
not EC but -curvename is provided. If both -curvename and -keysize are
provided, I intend to ignore -keysize no matter if they match or not.
>> Why not use a strict mode: fail if not match. It might be misleading
if ignoring unmatched options.
>
> We can do that, but misleading to what? That we treat -curvename and
-keysize the same important?
>
If the option "-keysize 256 -curvename sect163k1" work, I may think that
the key size if 256 bits. I want to create a 256 bits sect163k1 EC key,
and the tool allows this behavior, so I should get a 256 bits sect163k1
EC key. Sure, that's incorrect, but I don't know it is incorrect as the
tool ignore the key size. What's the problem of the command, I don't
know either unless I clearly understand sect163k1 is not 256 bits. The
next question to me, what's the key size actually is? 256 bits or 163
bits? which option are used? It adds more confusing to me.
We can ignore the -keysize option, but it is complicated to me to use
the tool.
>>
>>> Another question: in sun.security.util.CurveDB, we have
>>> // Return EC parameters for the specified field size. If there are
known
>>> // NIST recommended parameters for the given length, they are
returned.
>>> // Otherwise, if there are multiple matches for the given size, an
>>> // arbitrary one is returns.
>>> // If no parameters are known, the method returns null.
>>> // NOTE that this method returns both prime and binary curves.
>>> static NamedCurve lookup(int length) {
>>> return lengthMap.get(length);
>>> }
>>> FIPS 186-4 has 2 recommendations (K- and B-) for a binary curve
field size. Do we have a choice?
>>> In fact, CurveDB.java seems to have a bug when adding the curves:
>>> add("sect163k1 [NIST K-163]", "1.3.132.0.1", BD,...
>>> add("sect163r2 [NIST B-163]", "1.3.132.0.15", BD,... // Another
default?
>>> add("sect233k1 [NIST K-233]", "1.3.132.0.26", BD,...
>>> add("sect233r1 [NIST B-233]", "1.3.132.0.27", B,...
>>> and now 163 is sect163r2 and 233 is sect233k1.
>>> I assume we should always prefer the K- one?
>> TLS 1.3 uses secp256r1/secp384r1/secp521r1, no K- curves.
>
> There is no ambiguity for prime curves.
>
>>
>> Do you mean if no -curvename option, there is a need to choose a
named curve?
>
> ECKeyPairGenerator::initialize(int) will choose one and keytool will
use it. I just meant if we have a bug here and if we should be more
public on what curve is chosen.
>
I see your concerns.
It might be a potential issue if we use a named curve if no curvename
specified. If the compatibility is not serious, I may suggest supported
named curves only, or use arbitrary curves but with a warning.
Xuelei
Re: RFR CSR for 8213400: Support choosing curve name in keytool keypair generation
Some typos:
"When multiple curves have the same field size, and one of them is a
prime curve or a Koblitz curve, it will be used."
Which one will be used? prime curve or Koblitz curve. It will not be
documented, right? Otherwise, there are may be more curve categories.
As it is not the recommended option, I may just remove this and the
following one sentence.
Xuelei
On 11/6/2018 8:31 AM, Weijun Wang wrote:
Thanks everyone. CSR updated, and I describe the behavior in the Solution part.
If you are all happy I'll start coding.
And yes, KeyPairGenerator::init(int) needs some clarification, but I don't know
in which class/method we should add it. Maybe the JDK Provider Doc?
--Max
On Nov 7, 2018, at 12:00 AM, Xuelei Fan wrote:
As -curvename is a new option, I would second the comments that don't allow
mixing curve names and keysize at the same time.
Xuelei
On 11/5/2018 11:41 PM, Bernd Eckenfels wrote:
Hello,
I would agree ignoring an (conflicting) option adds confusion. When specifying
a curve is a new feature we don’t need to worry about beeing compatible,
therefore I would forbid mixing curve names and keysize at all (even when the
size matches).
I guess we cannot remove the option to only pass the keysize (to have the
generator pick a curve) to stay compatible. But the chosen curve should be
printed, and I would also deprecate this usage.
I guess clarifying the keysize-only init() method would be a different topic,
but something like deprecating it and restricting the selection to „SunEC only
selects NIST prime curves“ would be a good thing.
Gruss
Bernd
Gruss
Bernd
--
http://bernd.eckenfels.net
*Von:* security-dev im Auftrag von Xuelei Fan
*Gesendet:* Dienstag, November 6, 2018 7:38 AM
*An:* Weijun Wang
*Cc:* security-dev@openjdk.java.net
*Betreff:* Re: RFR CSR for 8213400: Support choosing curve name in keytool
keypair generation
On 11/5/2018 8:37 PM, Weijun Wang wrote:
On Nov 6, 2018, at 12:12 PM, Xuelei Fan wrote:
On 11/5/2018 7:13 PM, Weijun Wang wrote:
Please take a review at the CSR at
https://bugs.openjdk.java.net/browse/JDK-8213401
As for implementation, I intend to report an error when -keyalg is not EC but
-curvename is provided. If both -curvename and -keysize are provided, I intend
to ignore -keysize no matter if they match or not.
Why not use a strict mode: fail if not match. It might be misleading if
ignoring unmatched options.
We can do that, but misleading to what? That we treat -curvename and -keysize
the same important?
If the option "-keysize 256 -curvename sect163k1" work, I may think that
the key size if 256 bits. I want to create a 256 bits sect163k1 EC key,
and the tool allows this behavior, so I should get a 256 bits sect163k1
EC key. Sure, that's incorrect, but I don't know it is incorrect as the
tool ignore the key size. What's the problem of the command, I don't
know either unless I clearly understand sect163k1 is not 256 bits. The
next question to me, what's the key size actually is? 256 bits or 163
bits? which option are used? It adds more confusing to me.
We can ignore the -keysize option, but it is complicated to me to use
the tool.
Another question: in sun.security.util.CurveDB, we have
// Return EC parameters for the specified field size. If there are known
// NIST recommended parameters for the given length, they are returned.
// Otherwise, if there are multiple matches for the given size, an
// arbitrary one is returns.
// If no parameters are known, the method returns null.
// NOTE that this method returns both prime and binary curves.
static NamedCurve lookup(int length) {
return lengthMap.get(length);
}
FIPS 186-4 has 2 recommendations (K- and B-) for a binary curve field size. Do
we have a choice?
In fact, CurveDB.java seems to have a bug when adding the curves:
add("sect163k1 [NIST K-163]", "1.3.132.0.1", BD,...
add("sect163r2 [NIST B-163]", "1.3.132.0.15", BD,... // Another default?
add("sect233k1 [NIST K-233]", "1.3.132.0.26", BD,...
add("sect233r1 [NIST B-233]", "1.3.132.0.27", B,...
and now 163 is sect163r2 and 233 is sect233k1.
I assume we should always prefer the K- one?
TLS 1.3 uses secp256r1/secp384r1/secp521r1, no K- curves.
There is no ambiguity for prime curves.
Do you mean if no -curvename option, there is a need to choose a named curve?
ECKeyPairGenerator::initialize(int) will choose one and keytool will use it. I
just meant if we have a bug here and if we should be more public on what curve
is chosen.
I see your concerns.
It might be a potential issue if we use a named curve if no curvename
specified. If the compatibility is not serious, I may suggest supported
named curves only, or use arbitrary curves but with a warning.
Xuelei
Re: RFR, JDK-8212885: TLS 1.3 resumed session does not retain peer certificate chain
Looks fine to me. As you are already there, would you mind have an additional improvement in PreSharedKeyExtension.java? - MessageDigest md = MessageDigest.getInstance(hashAlg.toString());; + MessageDigest md = MessageDigest.getInstance(hashAlg.toString()); Normally, the toString() is not a reliable method to get the precise algorithm name. Would you mind update to use hashAlg.name instead? - MessageDigest md = MessageDigest.getInstance(hashAlg.toString());; + MessageDigest md = MessageDigest.getInstance(hashAlg.name); Thanks, Xuelei On 11/6/2018 11:05 AM, Jamil Nimeh wrote: Hi Xuelei, updated review here: http://cr.openjdk.java.net/~jnimeh/reviews/8212885/webrev.02 I followed your suggestions and also cleaned up some remnant comments and removed a double-semicolon...just cosmetic stuff. --Jamil On 11/6/18 10:11 AM, Jamil Nimeh wrote: Okay, I can move this into PreSharedKeyExtension.java and re-run the local tests that were having issues with it. Should work pretty well. I'll put out another code review shortly. Thanks, --Jamil On 11/6/2018 7:36 AM, Xuelei Fan wrote: Nice update! For the update in ClientHello.java, I may suggest moving it to pre_shared_key extension class. It may be a little bit safer if the extension can be loaded in other places. Thanks, Xuelei On 11/5/2018 11:51 PM, Jamil Nimeh wrote: Hello all, This fixes an issue where TLS 1.3 resumed sessions were not carrying forward many of the parameters from the parent session, namely the peer certificates, but also the local certificates and a few other SSLSessionImpl fields. This also moves the fix from an earlier, related issue with SNI names (JDK-8211806) into this new solution. JBS: https://bugs.openjdk.java.net/browse/JDK-8212885 Webrev: http://cr.openjdk.java.net/~jnimeh/reviews/8212885/webrev.01 Thanks, --Jamil
Re: RFR, JDK-8212885: TLS 1.3 resumed session does not retain peer certificate chain
Thanks for the update! No more comments from me. Xuelei On 11/6/2018 11:38 AM, Jamil Nimeh wrote: Hi Xuelei, I've made the change. I think in this specific case CipherSuite.hashAlg.toString is just a simple return of the name field so it should be no less reliable than hitting the name field directly. Changing it does make it more consistent with other places in the method, so that's good. http://cr.openjdk.java.net/~jnimeh/reviews/8212885/webrev.03 --Jamil On 11/6/2018 11:29 AM, Xuelei Fan wrote: Looks fine to me. As you are already there, would you mind have an additional improvement in PreSharedKeyExtension.java? - MessageDigest md = MessageDigest.getInstance(hashAlg.toString());; + MessageDigest md = MessageDigest.getInstance(hashAlg.toString()); Normally, the toString() is not a reliable method to get the precise algorithm name. Would you mind update to use hashAlg.name instead? - MessageDigest md = MessageDigest.getInstance(hashAlg.toString());; + MessageDigest md = MessageDigest.getInstance(hashAlg.name); Thanks, Xuelei On 11/6/2018 11:05 AM, Jamil Nimeh wrote: Hi Xuelei, updated review here: http://cr.openjdk.java.net/~jnimeh/reviews/8212885/webrev.02 I followed your suggestions and also cleaned up some remnant comments and removed a double-semicolon...just cosmetic stuff. --Jamil On 11/6/18 10:11 AM, Jamil Nimeh wrote: Okay, I can move this into PreSharedKeyExtension.java and re-run the local tests that were having issues with it. Should work pretty well. I'll put out another code review shortly. Thanks, --Jamil On 11/6/2018 7:36 AM, Xuelei Fan wrote: Nice update! For the update in ClientHello.java, I may suggest moving it to pre_shared_key extension class. It may be a little bit safer if the extension can be loaded in other places. Thanks, Xuelei On 11/5/2018 11:51 PM, Jamil Nimeh wrote: Hello all, This fixes an issue where TLS 1.3 resumed sessions were not carrying forward many of the parameters from the parent session, namely the peer certificates, but also the local certificates and a few other SSLSessionImpl fields. This also moves the fix from an earlier, related issue with SNI names (JDK-8211806) into this new solution. JBS: https://bugs.openjdk.java.net/browse/JDK-8212885 Webrev: http://cr.openjdk.java.net/~jnimeh/reviews/8212885/webrev.01 Thanks, --Jamil
Re: RFR CSR for 8213400: Support choosing curve name in keytool keypair generation
Maybe, the -groupname/-curvename option can be replaced by extending the existing -keyalg option: -keyalg secp256r1 Then there is no conflict between the curve/group name and the key alg. Xuelei On 11/7/2018 7:48 AM, Weijun Wang wrote: CSR updated. With such a generalized option, I won't recommend -groupname over -keysize now, although I still intend to print some warning for EC. Please take a review. Thanks Max On Nov 7, 2018, at 10:36 PM, Adam Petcher wrote: One issue that just came to me: How will this work for EdDSA? I think the CSR could be generalized a bit: 1) Make the first item in the "Solution" more general. Instead of limiting it to "EC" allow any valid algorithm/curve combination. 2) (Optional) Use -groupname instead of -curvename and change "curve" to "group" everywhere in the CSR. Then this mechanism can also be used for DSA (with named groups) and other algorithms that use groups that aren't curves. Also, see below for a comment about curve ambiguity. On 11/6/2018 7:59 PM, Weijun Wang wrote: Otherwise, there are may be more curve categories. As it is not the recommended option, I may just remove this and the following one sentence. I'll just leave it there as a FYI since it's not part of the spec. I agree with Xuelei that this part should be removed. Unless you are planning on implementing this curve selection logic in keytool, then we can't control which curve is selected, and it wholly depends on the behavior of the providers. We can't even guarantee that there is any relationship between "key size" and the field size of the curve. Also, we shouldn't use the word "random" here unless we plan to actually randomize the selection of the curve at runtime (similar to random iteration order for maps/sets). I suggest something more general and vague like: If only -keysize is specified, an arbitrary curve of the specified size is used
Re: RFR CSR for 8213400: Support choosing curve name in keytool keypair generation
On 11/7/2018 3:38 PM, Weijun Wang wrote:
This sounds a little misleading to me. Alg name and alg params are 2 different things.
This is like asking user to call KeyPairGenerator.getInstance("secp256r1").
Well, KeyPairGenerator.getInstance("x25519") is a case that JDK 11 has
supported now.
Otherwise, there is a need to check the conflict of alg name and group name.
Xuelei
--Max
On Nov 8, 2018, at 1:47 AM, Xuelei Fan wrote:
Maybe, the -groupname/-curvename option can be replaced by extending the
existing -keyalg option:
-keyalg secp256r1
Then there is no conflict between the curve/group name and the key alg.
Xuelei
On 11/7/2018 7:48 AM, Weijun Wang wrote:
CSR updated. With such a generalized option, I won't recommend -groupname over
-keysize now, although I still intend to print some warning for EC.
Please take a review.
Thanks
Max
On Nov 7, 2018, at 10:36 PM, Adam Petcher wrote:
One issue that just came to me: How will this work for EdDSA? I think the CSR
could be generalized a bit:
1) Make the first item in the "Solution" more general. Instead of limiting it to
"EC" allow any valid algorithm/curve combination.
2) (Optional) Use -groupname instead of -curvename and change "curve" to
"group" everywhere in the CSR. Then this mechanism can also be used for DSA (with named
groups) and other algorithms that use groups that aren't curves.
Also, see below for a comment about curve ambiguity.
On 11/6/2018 7:59 PM, Weijun Wang wrote:
Otherwise, there are may be more curve categories. As it is not the recommended
option, I may just remove this and the following one sentence.
I'll just leave it there as a FYI since it's not part of the spec.
I agree with Xuelei that this part should be removed. Unless you are planning on implementing this
curve selection logic in keytool, then we can't control which curve is selected, and it wholly
depends on the behavior of the providers. We can't even guarantee that there is any relationship
between "key size" and the field size of the curve. Also, we shouldn't use the word
"random" here unless we plan to actually randomize the selection of the curve at runtime
(similar to random iteration order for maps/sets). I suggest something more general and vague like:
If only -keysize is specified, an arbitrary curve of the specified size is used
Re: A new proposal to add methods to HttpsURLConnection to access SSLSession
On 11/7/2018 1:30 PM, Sean Mullan wrote: https://bugs.openjdk.java.net/browse/JDK-8213161 http://cr.openjdk.java.net/~xuelei/8212261/webrev.03/ I didn't see a test for SecureCacheResponse - is it possible? JDK does not have the reference implementation of SecureCacheResponse. You could also add a test case for IllegalStateExc. Added in the same test file. lines 108-113 of HttpsSession.java should be indented 4 more spaces. Otherwise looks ok to me. Updated. New webrev: http://cr.openjdk.java.net/~xuelei/8212261/webrev.04/ Thanks, Xuelei
Re: RFR CSR for 8213400: Support choosing curve name in keytool keypair generation
I don't think the underlying provider is ready to support named curves.
Additional RFEs may be required to standardize the names and improve the
underlying provider.
Xuelei
On 11/7/2018 7:05 PM, Weijun Wang wrote:
In CurveDB.java, we have
add("secp256r1 [NIST P-256, X9.62 prime256v1]", "1.2.840.10045.3.1.7", PD,
"0001",
"0001FFFC",
"5AC635D8AA3A93E7B3EBBD55769886BC651D06B0CC53B0F63BCE3C3E27D2604B",
"6B17D1F2E12C4247F8BCE6E563A440F277037D812DEB33A0F4A13945D898C296",
"4FE342E2FE1A7F9B8EE7EB4A7C0F9E162BCE33576B315ECECBB6406837BF51F5",
"BCE6FAADA7179E84F3B9CAC2FC632551",
1, nameSplitPattern);
So the aliases of secp256r1 are now "NIST P-256" and "X9.62 prime256v1". Do we
really want to keep the organization name prefix after JDK-8208156? The alias can be used in
ECGenParameterSpec and the proposed keytool -groupname option.
The following shows this behavior.
jshell> KeyPairGenerator.getInstance("EC")
$3 ==> java.security.KeyPairGenerator$Delegate@64bfbc86
jshell> $3.initialize(new ECGenParameterSpec("secp256r1"))
jshell> $3.initialize(new ECGenParameterSpec("prime256v1"))
| Exception java.security.InvalidAlgorithmParameterException: Unknown curve
name: prime256v1
|at ECKeyPairGenerator.initialize (ECKeyPairGenerator.java:103)
|at KeyPairGenerator$Delegate.initialize (KeyPairGenerator.java:699)
|at KeyPairGenerator.initialize (KeyPairGenerator.java:436)
|at (#6:1)
jshell> $3.initialize(new ECGenParameterSpec("X9.62 prime256v1"))
Thanks
Max
On Nov 7, 2018, at 11:48 PM, Weijun Wang wrote:
CSR updated. With such a generalized option, I won't recommend -groupname over
-keysize now, although I still intend to print some warning for EC.
Please take a review.
Thanks
Max
Re: A new proposal to add methods to HttpsURLConnection to access SSLSession
To make sure the SecureCacheResponse class work, two new tests are added (DefaultCacheResponse for default implementation, DummyCacheResponse for a test implementation): http://cr.openjdk.java.net/~xuelei/8212261/webrev.04/ Thanks, Xuelei On 11/8/2018 7:03 AM, Sean Mullan wrote: On 11/7/18 7:22 PM, Xuelei Fan wrote: On 11/7/2018 1:30 PM, Sean Mullan wrote: https://bugs.openjdk.java.net/browse/JDK-8213161 http://cr.openjdk.java.net/~xuelei/8212261/webrev.03/ I didn't see a test for SecureCacheResponse - is it possible? JDK does not have the reference implementation of SecureCacheResponse. Ah, I see. I am sure there is a good reason, but why doesn't it have an implementation? You could also add a test case for IllegalStateExc. Added in the same test file. lines 108-113 of HttpsSession.java should be indented 4 more spaces. Otherwise looks ok to me. Updated. New webrev: http://cr.openjdk.java.net/~xuelei/8212261/webrev.04/ Looks good. --Sean
CSR Review Request, JDK-8213577, Update the default SSL session cache size to 20480
Hi, Please review the proposal to update the default SSL session cache size from infinite to 20480. https://bugs.openjdk.java.net/browse/JDK-8213577 I know that the default 20480 does not fit all. I'd appreciate your feedback if the value is acceptable. Thanks, Xuelei
Code Review Request : JDK-8213694 : Test Timeout.java should run in othervm mode
Hi, Could I get the following small change reviewed please? http://cr.openjdk.java.net/~xuelei/8213694/webrev.00/ The test SSLSessionContextImpl/Timeout.java is running in the default mode. As the test initializes the SSLContext with the current System Properties, while the SunJSSE provider does not support dynamic System Properties, this test may impact other test cases running in samevm/agentvm mode. The impact is very hard to debugging. I updated the test to run in othervm mode, and cleanup the code by removing commented out codes and close the server socket with a try-with-resource. Thanks, Xuelei
Re: CSR Review Request, JDK-8213577, Update the default SSL session cache size to 20480
On 11/9/2018 9:34 AM, Jamil Nimeh wrote: Hi Xuelei, Content looks good. I'd remove specific references to Amazon from the CSR (it's fine to leave it in the source bug though). Removed. Where'd you get the 20480 session cache limit from? I saw a similar limit using the builtin SSL session cache from NGINX, is that where that number comes from? Or is that common to other TLS library or webserver cache sizes? The number is coming from what NGINX is using. In the bug description, it is said 10K could be a good one. However, the number is really depends on the platform resources. I'd like to use a bigger one so that the performance impact of the existing applications is as minimal as possible. Thanks, Xuelei --Jamil On 11/8/2018 8:00 PM, Xuelei Fan wrote: Hi, Please review the proposal to update the default SSL session cache size from infinite to 20480. https://bugs.openjdk.java.net/browse/JDK-8213577 I know that the default 20480 does not fit all. I'd appreciate your feedback if the value is acceptable. Thanks, Xuelei
Re: RFR, JDK-8212885: TLS 1.3 resumed session does not retain peer certificate chain
Looks fine to me. Thanks, Xuelei On 11/12/2018 11:46 PM, Jamil Nimeh wrote: Hello all, This is an update that addresses a few additional fields that needed to be carried over into the new SSLSession, as well as adding some more testing on the retrieved resumed SSLSession object. http://cr.openjdk.java.net/~jnimeh/reviews/8212885/webrev.04/ Thanks, --Jamil
Re: RFR 8212003: Obsoleting the default keytool -keyalg option
I may want to have the warning message with more explicit guide to cleanup the warning. For example: Warning: No -keyalg option. The default key algorithm ... Otherwise, looks fine to me. I added myself as the reviewer. Thanks, Xuelei On 11/14/2018 2:07 AM, Weijun Wang wrote: The CSR is re-opened. It is now focusing on -keyalg only. Please take a review: https://bugs.openjdk.java.net/browse/JDK-8212111 Thanks Max On Nov 7, 2018, at 11:51 PM, Weijun Wang wrote: Oops, I take this back. The CSR needs more update. Sorry if you have already start reading it. Thanks Max On Nov 7, 2018, at 9:27 AM, Weijun Wang wrote: After some discussion, we decided to cover -keysize and -sigalg in this deprecation process too. Please review the updated CSR at https://bugs.openjdk.java.net/browse/JDK-8212111 No webrev available yet. Thanks Max On Oct 18, 2018, at 10:34 AM, Weijun Wang wrote: Please review the code change and CSR for JBS: https://bugs.openjdk.java.net/browse/JDK-8212003 at webrev: http://cr.openjdk.java.net/~weijun/8212003/webrev.00/ CSR: https://bugs.openjdk.java.net/browse/JDK-8212111 When -keyalg is not provided for -genkeypair or -genseckey, keytool will print out a warning. We plan to make this an error in a future release. A new regression test ObsoleteKeyalg.java added. "-keyalg DSA" or "-keyalg DES" added to other tests. A Mach5 job on tier1 and tier2 running now. Thanks Max
Re: [12] RFR: 8211787: javax/net/ssl/TLSCommon/TLSTest.java throws java.net.SocketTimeoutException: Read timed out
Looks fine to me. Thanks, Xuelei On 11/14/2018 2:21 AM, Sibabrata Sahoo wrote: Hi Xuelei, Please review a minor fix for, JBS: https://bugs.openjdk.java.net/browse/JDK-8211787 Webrev: http://cr.openjdk.java.net/~ssahoo/8211787/webrev.00/ The original intention to “setSoTimeout()” is on serverSocket where the server will fail with timeout if it exceed certain amount of time before accepting a client connection. But by mistake it was set on wrong socket object which is created from accept() and causing a timeout if the InputStream exceed certain amount of time. I have corrected to setSoTimeout() to be set through serverSocket only. Thanks, Siba
Code Review Request, JDK-8213577, Update the default SSL session cache size to 20480
Hi, Please review this simple update: http://cr.openjdk.java.net/~xuelei/8210985/webrev.00/ The default value for the maximum number of entries in the SSL session cache (SSLSessionContext.getSessionCacheSize()) is infinite now. In the request, the default value is updated to 20480 for performance consideration. For the detailed behavior update, please refer to CSR: https://bugs.openjdk.java.net/browse/JDK-8213577 Thanks, Xuelei
Re: Code Review Request, JDK-8213577, Update the default SSL session cache size to 20480
On 11/14/2018 9:16 AM, Jamil Nimeh wrote: Hi Xuelei, The fix looks fine to me. I think it would be good to have an else branch off of the check on line 205 so any < 0 value has a warning log entry stating that an invalid value was detected and the cache is getting set to DEFAULT_MAX_CACHE_SIZE. Good point! I updated with warnings of invalid property: http://cr.openjdk.java.net/~xuelei/8210985/webrev.01/ Xuelei --Jamil On 11/14/2018 8:59 AM, Xuelei Fan wrote: Hi, Please review this simple update: http://cr.openjdk.java.net/~xuelei/8210985/webrev.00/ The default value for the maximum number of entries in the SSL session cache (SSLSessionContext.getSessionCacheSize()) is infinite now. In the request, the default value is updated to 20480 for performance consideration. For the detailed behavior update, please refer to CSR: https://bugs.openjdk.java.net/browse/JDK-8213577 Thanks, Xuelei
Re: FYI: new javadoc tag to document system properties
In JCE and JSSE, the public APIs definition (javax.net.ssl) and the
internal implementation (sun.security.ssl) are separated. The system
property can be defined in the internal implementation classes. I think
we should add the @systemProperty on the public APIs, right?
The public API class and the implementation class may be not a
one-to-one map. Multiple public APIs may be impacted by the system
property. How to handle such cases?
Thanks,
Xuelei
On 11/14/2018 2:27 PM, Jonathan Gibbons wrote:
This is an FYI to announce some initial, long-overdue support in javadoc
for documenting system properties (JDK-5076751).
Currently, system properties are just documented using ad-hoc narrative
text, which is fine if you know where to look for any given property.
JDK 12 introduces a new inline javadoc tag, {@systemProperty
/property-name/} which can be used to "declare" the name of a system
property. You can use this tag as a drop-in replacement for the
plain-text property name; it will have no overt effect on the narrative
text, but it will cause the property name to be put into the search
index and the A-Z index. Thus, property names will become searchable, to
allow users to easily find the definition of any such system property.
Adding support into javadoc is only part of the story. Now that the
support is in javadoc, we want to update the API documentation, so that
the documentation is updated for all Java SE and JDK system properties.
Where should the tag be used? The tag should be used in the text of the
defining instance of the property. This is where the characteristics of
the system property are described, which may include information like:
"what is the property for", "how and when is it set", "can it be
modified", and so on. For example, it could be used in the docs for
System.getProperties [1] or Networking Properties [2] In general, it
should -not- be used in situations that merely refer to the system
property by name. For example, the docs for Path.toAbsolutePath [3]
make a reference to the system property user.dir, but that is not a
definition of the property.
Going forward, in future releases, we expect to explore some
enhancements to this feature. Here are some of the ideas that have been
suggested:
* Create a "summary page" that lists all the system properties. This
would be somewhat similar to the current top-level "Constant Values"
page.
* Add information regarding the "scope" of the definition: is it
defined by the Java SE specification, or JDK, or JavaFX, etc. This
information could be used to organize the content on the summary page.
* Update @see and {@link} to be able to refer to system properties.
* Allow a short description to be included in the {@systemProperty}
tag. This text could be included in the search index, the A-Z index,
and the summary page.
-- Jon
[1]:
https://docs.oracle.com/en/java/javase/11/docs/api/java.base/java/lang/System.html#getProperties()
[2]:
https://docs.oracle.com/en/java/javase/11/docs/api/java.base/java/net/doc-files/net-properties.html
[3]:
https://docs.oracle.com/en/java/javase/11/docs/api/java.base/java/nio/file/Path.html#toAbsolutePath()
Re: Code Review Request, JDK-8213577, Update the default SSL session cache size to 20480
Hi Sean,
Are you OK if we do it later? I'm waiting for the @systemProperty tag,
proposed within JDK-5076751. I will file a bug to use the tag for more
cleanup.
Thanks,
Xuelei
On 11/15/2018 11:55 AM, Sean Mullan wrote:
This is a good opportunity to document the
javax.net.ssl.sessionCacheSize system property in the SSLSessionContext
API (and use the new @systemProperty tag) in an @implNote, for example:
/**
* Returns the size of the cache used for storing
* SSLSession objects grouped under this
* SSLSessionContext.
*
* @implNote The JDK implementation returns the cache size as set by
* the {@code setSessionCacheSize method}, or if not set, the value
* of the {@systemProperty javax.net.ssl.sessionCacheSize} system
* property. If neither is set, it returns a default value of 20480.
*
* @return size of the session cache; zero means there is no size
limit.
* @see #setSessionCacheSize
*/
public int getSessionCacheSize();
On 11/14/18 11:59 AM, Xuelei Fan wrote:
Hi,
Please review this simple update:
http://cr.openjdk.java.net/~xuelei/8210985/webrev.00/
The default value for the maximum number of entries in the SSL session
cache (SSLSessionContext.getSessionCacheSize()) is infinite now. In
the request, the default value is updated to 20480 for performance
consideration.
For the detailed behavior update, please refer to CSR:
https://bugs.openjdk.java.net/browse/JDK-8213577
Thanks,
Xuelei
Re: Code Review Request, JDK-8213577, Update the default SSL session cache size to 20480
It's time to use the systemProperty tag as it is ready.
As we are already there, I also update the setSessionCacheSize() for
more clarification.
Please review both CSR and webrev:
https://bugs.openjdk.java.net/browse/JDK-8213577
http://cr.openjdk.java.net/~xuelei/8210985/webrev.02/
Thanks,
Xuelei
On 11/16/2018 8:19 AM, Sean Mullan wrote:
On 11/15/18 3:37 PM, Xuelei Fan wrote:
Hi Sean,
Are you OK if we do it later? I'm waiting for the @systemProperty
tag, proposed within JDK-5076751. I will file a bug to use the tag
for more cleanup.
JDK-5076751 is completed and pushed to JDK 12, so you can use the new
tag now.
I think it would be easier to do it now, it seems pretty simple and that
way there is no need to worry about it later.
--Sean
Thanks,
Xuelei
On 11/15/2018 11:55 AM, Sean Mullan wrote:
This is a good opportunity to document the
javax.net.ssl.sessionCacheSize system property in the
SSLSessionContext API (and use the new @systemProperty tag) in an
@implNote, for example:
/**
* Returns the size of the cache used for storing
* SSLSession objects grouped under this
* SSLSessionContext.
*
* @implNote The JDK implementation returns the cache size as
set by
* the {@code setSessionCacheSize method}, or if not set, the value
* of the {@systemProperty javax.net.ssl.sessionCacheSize} system
* property. If neither is set, it returns a default value of
20480.
*
* @return size of the session cache; zero means there is no
size limit.
* @see #setSessionCacheSize
*/
public int getSessionCacheSize();
On 11/14/18 11:59 AM, Xuelei Fan wrote:
Hi,
Please review this simple update:
http://cr.openjdk.java.net/~xuelei/8210985/webrev.00/
The default value for the maximum number of entries in the SSL
session cache (SSLSessionContext.getSessionCacheSize()) is infinite
now. In the request, the default value is updated to 20480 for
performance consideration.
For the detailed behavior update, please refer to CSR:
https://bugs.openjdk.java.net/browse/JDK-8213577
Thanks,
Xuelei
Re: Code Review Request, JDK-8213577, Update the default SSL session cache size to 20480
Thanks for the review, Jmail & Sean.
New webrev:
http://cr.openjdk.java.net/~xuelei/8210985/webrev.03/
I will update CSR when we come to an agreement.
On 11/16/2018 11:33 AM, Sean Mullan wrote:
122 * @apiNote Both the session timeout and cache size impact
performance
123 * of future connections. It is not recommended to
use too big
124 * or too small timeout or cache size limit.
Applications should
125 * carefully weigh the limits and performance for the
specific
126 * circumstances.
I am not really sure if the @apiNote is that useful or appropriate,
I worry about the default value actually. A new bug may be filed again
and argue if the default value is not a proper one. The default value
of session timeout and cache size really depends on the real world
circumstances. I think we'd better make a clarification in the spec,
and remind application tune them accordingly.
but
it seems a bit odd to talk about the session timeout in this method.
The performance impact is a combination of the session timeout limit and
cache size. I would like application consider them together if need to
tune the values, but not individually.
If
you still want to add this, I would add an @apiNote to each of the
setSessionCacheSize and setSessionTimeout methods and just discuss each
property separately.
I added the update spec to both setSessionCacheSize and setSessionTimeout.
Also, unless you say what "too big" or "too small" is, I would avoid
giving this advice.
It makes sense to me.
Thanks,
Xuelei
--Sean
On 11/16/18 1:30 PM, Xuelei Fan wrote:
It's time to use the systemProperty tag as it is ready.
As we are already there, I also update the setSessionCacheSize() for
more clarification.
Please review both CSR and webrev:
https://bugs.openjdk.java.net/browse/JDK-8213577
http://cr.openjdk.java.net/~xuelei/8210985/webrev.02/
Thanks,
Xuelei
On 11/16/2018 8:19 AM, Sean Mullan wrote:
On 11/15/18 3:37 PM, Xuelei Fan wrote:
Hi Sean,
Are you OK if we do it later? I'm waiting for the @systemProperty
tag, proposed within JDK-5076751. I will file a bug to use the tag
for more cleanup.
JDK-5076751 is completed and pushed to JDK 12, so you can use the new
tag now.
I think it would be easier to do it now, it seems pretty simple and
that way there is no need to worry about it later.
--Sean
Thanks,
Xuelei
On 11/15/2018 11:55 AM, Sean Mullan wrote:
This is a good opportunity to document the
javax.net.ssl.sessionCacheSize system property in the
SSLSessionContext API (and use the new @systemProperty tag) in an
@implNote, for example:
/**
* Returns the size of the cache used for storing
* SSLSession objects grouped under this
* SSLSessionContext.
*
* @implNote The JDK implementation returns the cache size as
set by
* the {@code setSessionCacheSize method}, or if not set, the
value
* of the {@systemProperty javax.net.ssl.sessionCacheSize} system
* property. If neither is set, it returns a default value of
20480.
*
* @return size of the session cache; zero means there is no
size limit.
* @see #setSessionCacheSize
*/
public int getSessionCacheSize();
On 11/14/18 11:59 AM, Xuelei Fan wrote:
Hi,
Please review this simple update:
http://cr.openjdk.java.net/~xuelei/8210985/webrev.00/
The default value for the maximum number of entries in the SSL
session cache (SSLSessionContext.getSessionCacheSize()) is
infinite now. In the request, the default value is updated to
20480 for performance consideration.
For the detailed behavior update, please refer to CSR:
https://bugs.openjdk.java.net/browse/JDK-8213577
Thanks,
Xuelei
Re: RFR 8214568: Use {@systemProperty} for definitions of system properties
Looks fine to me. Xuelei > On Dec 12, 2018, at 6:25 PM, Weijun Wang wrote: > > Ping again. > > This is an RFE so needs to be fixed today unless we request for an approval. > > Thanks, > Max > >> On Dec 7, 2018, at 11:21 PM, Weijun Wang wrote: >> >> Please take a review at >> >> https://cr.openjdk.java.net/~weijun/8214568/webrev.00 >> >> Thanks >> Max >
Re: RFR[12] JDK-8214937: sun/security/tools/jarsigner/warnings/NoTimestampTest.java failed due to unexpected expiration date
Nice catch and looks good to me. Xuelei > On Dec 12, 2018, at 7:14 PM, sha.ji...@oracle.com wrote: > > Hi, > This test should determine the cert expiration date from the cert itself, but > not try to calculate that date. > > Issue: https://bugs.openjdk.java.net/browse/JDK-8214937 > Webrev: http://cr.openjdk.java.net/~jjiang/8214937/webrev.00/ > > Best regards, > John Jiang >
Re: [12] RFR 8215694: keytool cannot generate RSASSA-PSS certificates
Looks fine to me.
I was wondering, if it is more simple to define PSSParamsHolder as an
enum? Anyway, it is just very minor comment. You can leave it as it is.
Thanks,
Xuelei
Hi Xuelei,
Webrev updated at
https://cr.openjdk.java.net/~weijun/8215694/webrev.01
A new method AlgorithmId::getWithParameterSpec is added. I also cached 6
constants in AlgorithmId $PSSParamsHolder, although the static block inside it
to generate the AlgorithmIds are a little heavy. We can extract the encoding
lines inside PSSParameters::engineGetEncoded to create something like
PSSParameterSpec::getEncoded(), but that will be an RFE.
Thanks,
Max
On Jan 3, 2019, at 11:27 PM, Xue-Lei Fan wrote:
On 1/3/2019 2:10 AM, Weijun Wang wrote:
On Jan 2, 2019, at 11:56 PM, Xue-Lei Fan wrote:
sigAlg.equalsIgnoreCase("RSASSA-PSS"):
Do you really want to ignore the case? I used to think that an algorithm name
is case sensitive.
getInstance(alg) is always case-insensitive.
Hm, I missed it.
Main.java:1445 minor, 4 more indent?
Then it's longer than 80 chars. How about I un-indent lines 1443 and 1444?
maybe, use 4 white spaces? See also the following comment.
AlgorithmId.java:1073-1091:
I may prefer to use cached parameters (for both AlgorithmParameters and
AlgorithmParameterSpec) for each size, for performance.
OK for AlgorithmParameterSpec. Which AlgorithmParameters do you mean? The one
in SignatureUtil?
Yes, replacing SignatureUtil.createAlgorithmParameters(). Then we don't need
to worry about the indents above.
Thanks,
Xuelei
Thanks,
Max
Xuelei
On 12/21/2018 1:44 AM, Weijun Wang wrote:
Please take a review at
https://cr.openjdk.java.net/~weijun/8215694/webrev.00/
This bug reveals several issues:
1. Encoding of the RSASSA-PSS signature algorithm in PKCS10 and X509CertImpl.
2. The missing of setParameter() call for PKCS10 and X509CertImpl.
3. All keytool commands of -genkeypair, -certreq, -gencert, -selfcert are
affected.
4. Wrong NULL after encoding of RSASSA-PSS key algorithm.
Please confirm this is safe to be fixed in JDK 12.
Thanks,
Max
Re: [12] RFR 8215694: keytool cannot generate RSASSA-PSS certificates
Okay.
Xuelei
On 1/15/2019 7:20 PM, Weijun Wang wrote:
I don't use enum a lot, and we have 2 types of objects there. I'll keep it.
Thanks,
Max
On Jan 16, 2019, at 11:15 AM, Xuelei Fan wrote:
Looks fine to me.
I was wondering, if it is more simple to define PSSParamsHolder as an enum?
Anyway, it is just very minor comment. You can leave it as it is.
Thanks,
Xuelei
Hi Xuelei,
Webrev updated at
https://cr.openjdk.java.net/~weijun/8215694/webrev.01
A new method AlgorithmId::getWithParameterSpec is added. I also cached 6
constants in AlgorithmId $PSSParamsHolder, although the static block inside it
to generate the AlgorithmIds are a little heavy. We can extract the encoding
lines inside PSSParameters::engineGetEncoded to create something like
PSSParameterSpec::getEncoded(), but that will be an RFE.
Thanks,
Max
On Jan 3, 2019, at 11:27 PM, Xue-Lei Fan wrote:
On 1/3/2019 2:10 AM, Weijun Wang wrote:
On Jan 2, 2019, at 11:56 PM, Xue-Lei Fan wrote:
sigAlg.equalsIgnoreCase("RSASSA-PSS"):
Do you really want to ignore the case? I used to think that an algorithm name
is case sensitive.
getInstance(alg) is always case-insensitive.
Hm, I missed it.
Main.java:1445 minor, 4 more indent?
Then it's longer than 80 chars. How about I un-indent lines 1443 and 1444?
maybe, use 4 white spaces? See also the following comment.
AlgorithmId.java:1073-1091:
I may prefer to use cached parameters (for both AlgorithmParameters and
AlgorithmParameterSpec) for each size, for performance.
OK for AlgorithmParameterSpec. Which AlgorithmParameters do you mean? The one
in SignatureUtil?
Yes, replacing SignatureUtil.createAlgorithmParameters(). Then we don't need
to worry about the indents above.
Thanks,
Xuelei
Thanks,
Max
Xuelei
On 12/21/2018 1:44 AM, Weijun Wang wrote:
Please take a review at
https://cr.openjdk.java.net/~weijun/8215694/webrev.00/
This bug reveals several issues:
1. Encoding of the RSASSA-PSS signature algorithm in PKCS10 and X509CertImpl.
2. The missing of setParameter() call for PKCS10 and X509CertImpl.
3. All keytool commands of -genkeypair, -certreq, -gencert, -selfcert are
affected.
4. Wrong NULL after encoding of RSASSA-PSS key algorithm.
Please confirm this is safe to be fixed in JDK 12.
Thanks,
Max
