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
