RFR 8211018: Session Resumption without Server-Side State

[Anthony Scarpino]

On 6/6/19 9:18 AM, Xuelei Fan wrote:
> 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.

Sure

> 
> ----------
>   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.
> 

I tooke SessionTicketMessage out.  It really isn't useful with 
SessionTicketSpec doing most of the operations.

> 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?

This was something I got from the 1.3 CHPreSharedKeyProducer.produce(). 
I can remove it, should I remove it from PSK as well?
> 
> ----------
>   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?

Apparently I cut-n-pasted a bit too much.  I got rid of all the PSK in ST
> 
> ----------
>   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.
> 

The ST extension runs twice.  The CH consumer has resumption checking 
before the extensions are consumed normally from getEnabledExtensions(). 
  I wasn't able figure out how to remove ST from the consumption list, 
so I used shc.statelessResumption.  If shc.statelessResumption is set, 
it skips the consumption.  Because I already have the boolean, I used it 
for this purpose.

> 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 understand this comment.  The second connection does not reuse 
the shc.statelessResumption value from the first connection.

> 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.

Yes

> 
> 
> ----------
>   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?

if shc.statelessResumption is false NST is not added as a consumer. 
shc.statelessResumption is true when both sides sent their ST in the CH 
& SH.

> 
> ----------
>   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?

Then no ticket is stored and the next session is a full handshake.

> 
> Did we need the HandshakeAbsence handler for NewSessionTicket handshake 
> message?

I don't believe so.

> 
> 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
> 
> 
> 
>