> http://cr.openjdk.java.net/~xuelei/8196584/webrev-full.00
MaxFragExtension.java
---------------------
Looks fine to me.
KeyUpdate.java
--------------
- 74 new IOException("KeyUpdate message value invalid: " + status);
+ 74 throw new IOException("KeyUpdate message value invalid: " +
status);
Or
+ 74 context.conContext.fatal(...);
The exception is not thrown. Personally, I may prefer a
RuntimeException as it is used in generation side, or send a fatal
alert to indicate the problem.
81 if (m.remaining() != 1) {
82 throw new IOException("KeyUpdate has an unexpected length of "+
83 m.remaining());
84 }
Personally, I'd prefer to send a fatal alert.
87 if (status > 1) {
88 new IOException("KeyUpdate message value invalid: " + status);
89 }
The exception is not thrown. I may use a fatal alert.
108 @Override
109 public String toString() {
110 throw new UnsupportedOperationException("Not supported yet.");
111 }
For debugging, please implement this method and dump debug information
in the consumer and producer accordingly.
145 public void consume(ConnectionContext context,
146 ByteBuffer message) throws IOException {
I think we need to catch up with the following spec in this method:
Implementations that receive a KeyUpdate message prior to receiving
a Finished message MUST terminate the connection with an
unexpected_message" alert.
151 if (km.getStatus() == KeyUpdateMessage.NOTREQUSTED) {
152 return;
153 }
If I'm correct, the "update_not_requested" status means "recipient of
the KeyUpdate" may not respond with its own KeyUpdate", but still need
to update its receiving keys. Am I missing something?
165 SSLKeyDerivation skd = kdg.createKeyDerivation(hc,
166 hc.conContext.inputRecord.readCipher.baseSecret);
167 SecretKey nplus1 = skd.deriveKey("TlsUpdateNplus1", null);
168 if (skd == null) {
169 // unlikely
170 hc.conContext.fatal(Alert.INTERNAL_ERROR,
171 "no key derivation");
172 return;
173 }
Move line 167 bellow 173? he skd non-null checking can be performed
198 new KeyUpdateMessage(hc,
KeyUpdateMessage.NOTREQUSTED));earlier.
The blank line 191 may be not necessary.
196 // Send Reply
197 handshakeProducer.produce(hc,
198 new KeyUpdateMessage(hc, KeyUpdateMessage.NOTREQUSTED));
If one update the 151-153 block, please don't forgot to update this
block as well if needed: send reply if the status is REQUSTED.
219 if (km.getStatus() == KeyUpdateMessage.REQUSTED) {
220 secret = hc.conContext.outputRecord.writeCipher.baseSecret;
221 } else {
222 km.write(hc.handshakeOutput);
223 hc.handshakeOutput.flush();
224 return null;
225 }
Similar comment as 151-153. For NOTREQUSTED status, may still need to
update the write keys.
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
