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
