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):
---------------------------------------
  231         SessionStateSpec(ByteBuffer buf) throws IOException {
  ...
  240             sessiondata = buf;
The local filed 'sessiondata' shares the 'buf' object.  While the 'buf' 
object is mutable, and could be updated later.  The SessionStateSpec was 
used after the construction.  It looks like a potential issue.

  244             return sessiondata.array();
The array() method is an optional method, which may be not implemented. 
If I read the spec correctly, ByteBuffer.array() "Returns the byte array 
that backs this buffer" but not the usable data.  For example, the 
ByteBuffer could be a very big buffer, but use only 2 bytes for the 
useful data.  I think you are want to get the 2 bytes, but not the very 
big buffer.

  248             return (sessiondata.array().length == 0);
The same comment as above.

  261                 c.init(Cipher.ENCRYPT_MODE, key.key, new 
GCMParameterSpec(GCM_TAG_LEN, iv));
  262                 encrypted = c.doFinal(session.write());
I may use explicit AAD rather than the default one.  For example, using 
key.num as the additional authentication data.

  270                 System.arraycopy(iv,0, result, Integer.BYTES, 
iv.length);
  271                 System.arraycopy(encrypted,0, result,
It's nice to add a whitespace before '0'.

  258                 SecureRandom random = hc.sslContext.getSecureRandom();
  259                 random.nextBytes(iv);
The IV is not necessarily be random number, it's good as if it is 
unique.  Random number generation could be slow, I may use a sequence 
number as the IV.

  296                 byte[] data = sessiondata.array();
See above comment about ByteBuffer.array().  The array may not start 
from the ticket data.


  219     static final class SessionStateSpec implements SSLExtensionSpec {
  309     static final class SessionTicketMessage implements 
SSLExtensionSpec {
I did not get the point here: why there are two SSLExtensionSpec 
implementations for one SessionTicket extension?

I guess SessionTicketMessage is used for SSLExtensionSpec, while 
SessionStateSpec is a internal help class?  It was a little bit 
confusing to me.  I may rename SessionTicketMessage to 
SessionTicketSpec, and don't have SessionStateSpec implement 
SSLExtensionSpec interface (rename it without ending with 'Spec') and 
make it private.  As may need to re-arrange the code and 
SessionStateSpec is used by other classes.

  325         public String toString() {
I may add the "@Override" annotation to this method.

  318             sts = new SessionStateSpec(buffer);
  322             return sts.getData();
  334             Object[] messageFields = {
  335                     Utilities.toHexString(sts.getData()),
See above comment about ByteBuffer.array().  Line 318 could expose the 
risks.


  330             MessageFormat messageFormat = new MessageFormat(
  331                     "\"ticket\":    \"{0}\"\n",
  332                     Locale.ENGLISH);
I'm not very sure of the log format.  The SSLExtension should have 
already stringlized the extension name and ID.  In general, for the 
specific extension, it is sufficient to stringlize the content here. 
The 'ticket' prefix may be not duplicated and extension name.

  318             sts = new SessionStateSpec(buffer);
  240             sessiondata = buf;
  244             return sessiondata.array();

These lines took me to the cooperation behaviors between RFC 5077 and 
RFC 4507.  It looks like we don't support RFC 4507 format of 
SessionTicket extension.  As RFC 5077 and RFC 4507 use the same 
extension ID for different extension format.  There are potential 
compatibility issues, and make session resumption impossible.  I would 
like to have a workaround to accept both formats.  For example, using 
the a cookie at the beginning of the ticket, as described in appendix-A 
of RFC 5077.


I will review the rest of this class in the afternoon or tomorrow.

Thanks,
Xuelei