Some comments in-line:
On 05/31/2018 10:04 PM, Xuelei Fan wrote:
> http://cr.openjdk.java.net/~xuelei/8196584/webrev-full.00
AlpnExtension.java
------------------
Looks fine to me.
CertificateRequest.java
-----------------------
Looks fine to me.
CertificateStatus.java
----------------------
- 42 * Pack of the CertificateStatus handshake message.
+ 42 * Pack of the CertificateStatus handshake message. [RFC 6066]
May be nice if adding the RFC number that defines this handshake
message. Otherwise, looks fine to me.
JJN: Yeah, we can do that. And probably reference both 6066 and 6961
since both are represented in this handshake message. Perhaps a full
header comment that describes the structure of the message might be
appropriate here, too. It would help maintainers perhaps.
CertificateVerify.java
----------------------
-129 if (x509Credentials == null) {
+129 if (x509Credentials == null ||
x509Credentials.popPublicKey == null) {
130 shc.conContext.fatal(Alert.HANDSHAKE_FAILURE,
131 "No X509 credentials negotiated for CertificateVerify");
132 }
May be safe to check the x509Credentials.popPublicKey as well. Similar
to line 357-360, 607-610, 916-919.
-233 if (x509Possession == null) {
+233 if (x509Possession == null ||
x509Possession.popPrivateKey == null) {
234 if (SSLLogger.isOn && SSLLogger.isOn("ssl,handshake")) {
235 SSLLogger.fine(
236 "No X.509 credentials negotiated for CertificateVerify");
237 }
238
239 return null;
240 }
May be safe to check the x509Possession.popPrivateKey as well. Similar
to line 458-466, 697-704, 1021-1027.
Otherwise, looks fine to me.
CertStatusExtension.java
------------------------
326 private final int encodedLen;
Looks like this field is not used. Remove it?
425 if (!responderIds.isEmpty()) {
426 ridStr = responderIds.toString();
-427
428 }
One unnecessary blank line. Otherwise, looks fine to me.
JJN: For the last two comments, yes to both. That encodedLen field is a
remnant from the JDK 9 implementation and it was used there.
Xuelei
On 5/25/2018 4:45 PM, Xuelei Fan wrote:
Hi,
I'd like to invite you to review the TLS 1.3 implementation. I
appreciate it if I could have compatibility and specification
feedback before May 31, 2018, and implementation feedback before June
7, 2018.
Here is the webrev:
http://cr.openjdk.java.net/~xuelei/8196584/webrev-full.00
The formal TLS 1.3 specification is not finalized yet, although it
had been approved to be a standard. The implementation is based on
the draft version 28:
https://tools.ietf.org/html/draft-ietf-tls-tls13-28
For the overall description of this enhancement, please refer to JEP
332:
http://openjdk.java.net/jeps/332
For the compatibility and specification update, please refer to CSR
8202625:
https://bugs.openjdk.java.net/browse/JDK-8202625
Note that we are using the sandbox for the development right now.
For more information, please refer to Bradford's previous email:
http://mail.openjdk.java.net/pipermail/security-dev/2018-May/017139.html
Thanks & Regards,
Xuelei
