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
