RFR 8211018: Session Resumption without Server-Side State

[Anthony Scarpino]

On 6/5/19 9:58 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):
> ---------------------------------------
>   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.

Are you saying this because you have seen the bytebuffer underneath 
changed by the TLS code for other operations, or is it just because the 
object definition is mutable?   Does the ByteBuffer extra data in there 
that it should not?

I've gone through and redid a lot of this, but it creates an extra copy 
of the ~700byte array, and I don't like it and want to revert back.   So 
a lot of the below bytebuffer comments below are unanswered

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

Sure.

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

Pseudo random is not slow,  I don't feel comfortable using some 
manufactured IV for many of the bytes may not be unique.

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

It became it's own class because SessionStateSpec is used by 
SessionTicketMessage, NewSessionTicketMessage, and PreSharedKeyMessage.
> 
>   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.

I don't like the format of the SSLExtension, the text dump is not 
important given this is crypto code.  But maybe in the future I'll add a 
4 column 8 byte printout to Utilities.

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