> http://cr.openjdk.java.net/~ascarpino/8226338/webrev.01/
NewSessionTicket.java
---------------------
- 189 if (m.remaining() < 14) {
+ 189 if (m.remaining() < 9) {
I did not get the point of this update. I think 14 should be fine so
that to fail earlier if no sufficient data.
317 public byte[] produce(ConnectionContext context) ...
324 public byte[] produce(ServerHandshakeContext shc) ...
433 public byte[] produce(PostHandshakeContext hc) ...
I did not get the point to use three methods here. Read more inlines,
please.
On 7/15/2019 4:04 PM, Anthony Scarpino wrote:
I've updated the webrev
http://cr.openjdk.java.net/~ascarpino/8226338/webrev.01/
As I need to push this by wednesday, please review it soon.
There are fixes for the comments that were made, added a NST for
boundVaules, a very basic test to make sure the post handshake NST is
sent after boundValues have changed.
Did you mean send a new NST of the boundValues is changed? What if the
previous NST was used for resumption?
Thanks,
Xuelei
There is no change to invalidate() as these is not enough time to
address that.
Tony
On 7/7/19 9:14 PM, Anthony Scarpino wrote:
On 7/4/19 10:14 PM, Xuelei Fan wrote:
SSLSessionImpl.java
-------------------
526 boolean checkSessionTicket(HandshakeContext hc) {
As the return value is a boolean type, the method is better to use
"isSomething". Otherwise, the meaning of the code
"!checkSessionTicket()" is clear and need to read the implementation
code.
I'll see if I can come up with a "is*" name
NewSessionTicket.java
---------------------
104 // opaque ticket<1..2^16-1>;
108 if (m.remaining() < 6) {
Per section 3.3, RFC 5077, line 104 should be:
opaque ticket<0..2^16-1>;
I'm glad you saw that because it was confusing me why a zero buffer
was allowed in 5077
To be better understand the number 6, as you are already there, would
you please update line 104 as well?
I did not get the idea of the update of
T13NewSessionTicketMessage.T13NewSessionTicketMessage(). It looks
like that the updated code does not comply to TLS 1.3 specification
of NewSessionTicket.
So I assume you are talking about the zero length ticket. I never
noticed that 1.3 changed behavior away from zero length tickets. It
would appear the spec writings made every attempt to make NST usage
confusing between versions.
453 NewSessionTicketMessage nstm;
454 nstm = new T12NewSessionTicketMessage(shc,
sessionTimeoutSeconds,
Maybe better to use one statement.
453 NewSessionTicketMessage nstm =
454 new T12NewSessionTicketMessage(shc,
sessionTimeoutSeconds,
499 if (nstm.ticket.length == 0) {
Empty ticket should be not allowed in TLS 1.3 >
ServerHelloDone.java
--------------------
110 if (shc.statelessResumption) {
111 shc.handshakeConsumers.put(SSLHandshake.NEW_SESSION_TICKET.id,
112 SSLHandshake.NEW_SESSION_TICKET);
113 }
I think the NewSessionTicket is produced by server and consumed in
client side. I did not get the idea to have NST consumer in server
side. Am I missing something?
Hmm.. I remember making this change, and I thought it was a good idea,
but clearly it does nothing.
SessionTicketExtension.java
---------------------------
260 if (!hc.handshakeSession.checkSessionTicket(hc)) {
261 return new byte[0];
262 }
From the above line, I guess why you use empty ticket for TLS 1.3,
although empty ticket is not allowed in TLS 1.3.
I may suggest never use empty ticket. Always use a proper ticket in
a proper NewSessionTicket message, at least in format. When there is
a problem to retrieve the values, cache the session and prefer to use
the case rather than the ticket for session resumption. As may be a
little bit more interop friendly.
Yes, the 1.3 non-zero length sure complicates the code.. Very
unfortunate.
295 if (SSLLogger.isOn &&
SSLLogger.isOn("ssl,handshake")) {
296 SSLLogger.fine("Encryption failed." +
e.getMessage());
297 e.printStackTrace(); // Helps with
session.write()
298 }
Line 297 does not use SSLLogger. It could be:
SSLLogger.fine("Encryption failed", e);
I didn't noticed logger accepted exceptions.
On 7/2/2019 10:35 AM, Anthony Scarpino wrote:
Hi,
I need a code review on some updates to the stateless resumption.
1) Changing peerSupportedSignAlgs from a String[] to Collection[]
2) Additional items added to the stateless ticket
3) Not provide a stateless ticket when the masterkey is not
accessible (FIPS) or when boundValues are used
I did not whether the update works if bound values are set after the
handshake completed (i.e., after the NewSessionTicket has been
produced and used). Did you have a test for the case?
It was my understanding from previous discussions that we were going
to take a wait and see approach with boundValues and NST.
When I took a glance at how boundValues were added, they appear to be
outside of the normal flow of the ConnectionContext.
Thanks,
Xuelei
4) Be more graceful in case of stateless ticket generation failure
http://cr.openjdk.java.net/~ascarpino/8226338/webrev.00/
Tony
