RFR 8211018: Session Resumption without Server-Side State

[Xuelei Fan]

Continue for the SessionTicketExtension.java.

> On 6/3/2019 5:42 PM, Anthony Scarpino wrote:
>> http://cr.openjdk.java.net/~ascarpino/stateless/webrev.02 
SessionTicketExtension.java (continue):
---------------------------------------
  368             if (!((SSLSessionContextImpl)chc.sslContext.
  369 
engineGetClientSessionContext()).statelessEnabled()) {
  370                 return null;
  371             }

Would you like add a log if the stateless is not enabled?  Which may be 
useful for debugging.

----------
  378                 return new SessionTicketMessage().getEncoded();
When I read the code, I was wondering if 'SessionTicketMessage' is an 
handshake message.  It could be easier if we use a consistent naming 
style, for example "SessionTicketSpec", as we did in other extension 
classes.

As the encode for empty ST is the same, I may just use a static bytes array.

----------
  381             if (chc.localSupportedSignAlgs == null) {
  382                 chc.localSupportedSignAlgs =
  383                         SignatureScheme.getSupportedAlgorithms(
  384                                 chc.algorithmConstraints, 
chc.activeProtocols);
  385             }
  386
  387             // Make sure the list of supported signature 
algorithms matches
  388             Collection<SignatureScheme> sessionSigAlgs =
  389 
chc.resumingSession.getLocalSupportedSignatureSchemes();
  390             if 
(!chc.localSupportedSignAlgs.containsAll(sessionSigAlgs)) {
  391                 if (SSLLogger.isOn && 
SSLLogger.isOn("ssl,handshake")) {
  392                     SSLLogger.fine("Existing session uses 
different " +
  393                             "signature algorithms");
  394                 }
  395                 return null;
  396             }

I did not get the idea here.  Does the session signature algorithms 
impact the use of session ticket extension?

----------
  398             if (chc.resumingSession.getPskIdentity() == null) {
  399                 if (SSLLogger.isOn && 
SSLLogger.isOn("ssl,handshake")) {
  400                     SSLLogger.fine(
  401                             "SessionTicket has no identity, or 
identity was already used");
  402                 }
  403                 return null;
  404             }
  405
  406             return chc.resumingSession.getPskIdentity();

It's confusing to me that PSK is used here.  PSK is a different 
extension other than session ticket extension.  I had a quick look at 
the NewSessionTicket.java, and I can see where you came from.  It could 
be misleading if we want to support PSK in the future.

----------
  400                     SSLLogger.fine(
  401                             "SessionTicket has no identity, or 
identity was already used");

I did not follow the message.   With "identify was already used", does 
it mean the same session ticket can only be used one time?

----------
  421             // Skip if extension is not provided
  422             if (!shc.sslConfig.isAvailable(CH_SESSION_TICKET)) {
  423                 return;
  424             }

I may add a debug log and record that the session ticket extension is 
not supported in server side, and the extension is ignored.

----------
  426             // If no buffer or we are already using stateless 
resumption
  427             if (buffer == null || shc.statelessResumption) {
  428                 return;
  429             }
I did not follow the idea of the checking.  I think the buffer should 
never be null. For the 'shc.statelessResumption', as this method is to 
parse the session ticket in the ClientHello handshake message, there 
might be no chance to set the shc.statelessResumption yet for initial 
handshaking.

For renegotiation, the previous handshaking may have set the 
shc.statelessResumption to true.  For the current handshaking, I think 
we still need to check the validity of the extension, rather than using 
the previous handshaking result.

I don't think the check is necessary. But I may missed something.

----------
  434             if (!cache.statelessEnabled()) {
  435                 return;
  436             }

It might be nice to add more debug log.


----------
  438             if (buffer.capacity() == 0) {
The return value of ByteBuffer.capacity() may be not the same as the 
available value in the buffer.  The capacity could be 2^14, but the 
available bytes could be just 2 bytes.


----------
  499     private static final class T12SHSessionTicketConsumer 
implements ExtensionConsumer {

It might be nice to add more debug log in this class.

----------
  509             // Skip if extension is not provided
  510             if (!hc.sslConfig.isAvailable(SH_SESSION_TICKET)) {
  511                 return;
  512             }

If the extension is not provided, but receive the NST handshake message 
from server, is it expected to terminate the connection with an 
unexpected_message" alert?

----------
  514             // If the context does not allow stateless tickets, exit
  515             if (!((SSLSessionContextImpl)hc.sslContext.
  516 
engineGetClientSessionContext()).statelessEnabled()) {
  517                 return;
  518             }

Similar to above comment.


----------
What's client behavior if the server response with ST in ServerHello, 
but does not send the NewSessionTicket handshake message?

Did we need the HandshakeAbsence handler for NewSessionTicket handshake 
message?

All right.  I complete the review for the SessionTicketExtension.java. 
Most of them are trivial issues.  I don't think any of them that we 
cannot fix after RDP1 or even JDK 14.

Thanks,
Xuelei