Merged draft-13 patch

Merged draft-13 patch

[ekr-rietveld, 2016-06-10 04:16:57]

Things are starting to get annoying with a lot of patches in flight. Here is a
mostly merged patch for your review, consisting of the new key schedule, session
tickets, and 0-RTT code PTAL.

[ekr-rietveld, 2016-06-10 04:17:23]

P.S. Sorry about it being a new issue, but like I said, confusing.

[ekr-rietveld, 2016-06-20 20:20:44]

MT, PTAL

[mt, 2016-06-21 07:27:33]

I'm not super-happy with the tests, but it looks like the coverage is OK.

https://codereview.appspot.com/300370043/diff/20001/external_tests/ssl_gtest/...
File external_tests/ssl_gtest/libssl_internals.c (right):

https://codereview.appspot.com/300370043/diff/20001/external_tests/ssl_gtest/...
external_tests/ssl_gtest/libssl_internals.c:147: fprintf(stderr, "%s != NULL\n",
#secret);     \
Check formatting.  (clang-format is your friend)

https://codereview.appspot.com/300370043/diff/20001/external_tests/ssl_gtest/...
external_tests/ssl_gtest/libssl_internals.c:155: PR_FALSE;
You need to return this.

https://codereview.appspot.com/300370043/diff/20001/external_tests/ssl_gtest/...
File external_tests/ssl_gtest/ssl_agent_unittest.cc (right):

https://codereview.appspot.com/300370043/diff/20001/external_tests/ssl_gtest/...
external_tests/ssl_gtest/ssl_agent_unittest.cc:281: * so trial decryption isn't
engaged. */
// Comments in c++ code.

"isn't engaged and 0-RTT messages cause an error."

https://codereview.appspot.com/300370043/diff/20001/external_tests/ssl_gtest/...
File external_tests/ssl_gtest/ssl_loopback_unittest.cc (right):

https://codereview.appspot.com/300370043/diff/20001/external_tests/ssl_gtest/...
external_tests/ssl_gtest/ssl_loopback_unittest.cc:1: 
Remove.

https://codereview.appspot.com/300370043/diff/20001/external_tests/ssl_gtest/...
external_tests/ssl_gtest/ssl_loopback_unittest.cc:1169: TEST_F(TlsConnectTest,
TestTls13ZeroRttServerRejectByOption) {
I think that these tests are more complicated than they need to be.  If you
drive the handshake manually, then you should be able to make these a lot
simpler.

First thing to do is factor out the first connection part.  That's repeated a
lot already.  I would include the Reset() and as much of the setup for the
second round as possible (ConfigureSessionCache(), Set0RttEnabled(),
ExpectResumption()).

The next thing is that I think that the filters make this very hard to follow. 
For the second connection, driving the handshake manually seems to be easier:

client_->StartHandshake();
server_->StartHandshake();
ExpectResumption(RESUME_TICKET);

client_->Handshake();
PR_Write(client_->ssl_fd(), ...); // factor into Write0Rtt()
server_->Handshake();
PR_Read(server_->ssl_fd(), ...); // factor into Read0Rtt()

// Then complete the handshake properly and see that it works.
Connect();
SendReceive();
// check the extension, etc...

The only downside I see is that once the server handshakes, the client will then
complete the handshake on its next write.  So you can't engineer a situation
where the client can send to the server for a long time.  For that, I would
capture and block the server's encrypted handshake flight.

https://codereview.appspot.com/300370043/diff/20001/external_tests/ssl_gtest/...
File external_tests/ssl_gtest/tls_agent.cc (right):

https://codereview.appspot.com/300370043/diff/20001/external_tests/ssl_gtest/...
external_tests/ssl_gtest/tls_agent.cc:565: expected = PR_FALSE;
This mixing of PRBool and bool is confusing.

PSM takes the view that C++ uses bool and true/false always.  Should we?

https://codereview.appspot.com/300370043/diff/20001/external_tests/ssl_gtest/...
File external_tests/ssl_gtest/tls_connect.cc (right):

https://codereview.appspot.com/300370043/diff/20001/external_tests/ssl_gtest/...
external_tests/ssl_gtest/tls_connect.cc:273: CheckEarlyDataAccepted();
I would inline this function, just as you did for CheckSecretsDestroyed()

https://codereview.appspot.com/300370043/diff/20001/lib/ssl/SSLerrs.h
File lib/ssl/SSLerrs.h (right):

https://codereview.appspot.com/300370043/diff/20001/lib/ssl/SSLerrs.h#newcode477
lib/ssl/SSLerrs.h:477: "SSL received an end of early data alert.")
See previous comment.  I don't think that this needs to be in the name, just the
description.

https://codereview.appspot.com/300370043/diff/20001/lib/ssl/ssl.h
File lib/ssl/ssl.h (right):

https://codereview.appspot.com/300370043/diff/20001/lib/ssl/ssl.h#newcode225
lib/ssl/ssl.h:225: /* Allow 0-RTT data (for TLS 1.3) */
This needs a lot of documentation.  Off the top of my head:

* How you use 0-RTT when you enable it (on both server and client).
* The risks associated with using 0-RTT.
* The contract that we provide, in particular that the handshake callback marks
the transition.
* How to get information about the state of the connection when you are doing
this.
* How this relates to client authentication.

https://codereview.appspot.com/300370043/diff/20001/lib/ssl/ssl3con.c
File lib/ssl/ssl3con.c (right):

https://codereview.appspot.com/300370043/diff/20001/lib/ssl/ssl3con.c#newcode...
lib/ssl/ssl3con.c:6081: return rv; /* error code set by
tls13_MaybeDo0RttHandshake */
Should we be returning SECFailure here?  return rv has been so risky...

https://codereview.appspot.com/300370043/diff/20001/lib/ssl/ssl3con.c#newcode...
lib/ssl/ssl3con.c:6086: return rv;
As above, except SECSuccess.

https://codereview.appspot.com/300370043/diff/20001/lib/ssl/ssl3con.c#newcode...
lib/ssl/ssl3con.c:8650: #ifndef SSL_SNI_ALLOW_NAME_CHANGE_2HS
Missed one.

https://codereview.appspot.com/300370043/diff/20001/lib/ssl/ssl3con.c#newcode...
lib/ssl/ssl3con.c:11093: ss, ticket.data, ticket.len, 2);
One line

https://codereview.appspot.com/300370043/diff/20001/lib/ssl/ssl3ecc.c
File lib/ssl/ssl3ecc.c (right):

https://codereview.appspot.com/300370043/diff/20001/lib/ssl/ssl3ecc.c#newcode379
lib/ssl/ssl3ecc.c:379: SECItem ecPoint = {siBuffer, NULL, 0};
What does clang-format do here?  Because you added new structs elsewhere in this
patch that have spaces inside the braces.

https://codereview.appspot.com/300370043/diff/20001/lib/ssl/ssl3ext.c
File lib/ssl/ssl3ext.c (right):

https://codereview.appspot.com/300370043/diff/20001/lib/ssl/ssl3ext.c#newcode...
lib/ssl/ssl3ext.c:1991: /* Copy master secret. */
This re-indenting will be destroyed by clang-format.

https://codereview.appspot.com/300370043/diff/20001/lib/ssl/ssl3ext.c#newcode...
lib/ssl/ssl3ext.c:3552: 
Missing: if (maxBytes < (PRUint32)extension_length)

https://codereview.appspot.com/300370043/diff/20001/lib/ssl/ssl3ext.c#newcode...
lib/ssl/ssl3ext.c:3647: if (!(sid->u.ssl3.locked.sessionTicket.flags &
ticket_allow_early_data))
I prefer == 0 for something like this, but will leave it to you.

https://codereview.appspot.com/300370043/diff/20001/lib/ssl/ssl3ext.c#newcode...
lib/ssl/ssl3ext.c:3701: if (data->len) {
If we go with masking for the ticket_age, maybe we should just pack it into the
early_data extension instead of having a new extension.

https://codereview.appspot.com/300370043/diff/20001/lib/ssl/ssl3ext.c#newcode...
lib/ssl/ssl3ext.c:3711: **/
I would have said:

IMPORTANT: Record the presence of the extension only.  This is only one of
several things that the 0-RTT processing code uses in determining whether to
accept 0-RTT.

Also, the trailing **

https://codereview.appspot.com/300370043/diff/20001/lib/ssl/ssl3ext.c#newcode...
lib/ssl/ssl3ext.c:3725: 
Missing maxBytes check.

https://codereview.appspot.com/300370043/diff/20001/lib/ssl/sslinfo.c
File lib/ssl/sslinfo.c (right):

https://codereview.appspot.com/300370043/diff/20001/lib/ssl/sslinfo.c#newcode93
lib/ssl/sslinfo.c:93: }
You know, you could just remove the outer check.

Maybe add this as a sanity check:
PORT_Assert(!inf.earlyDataAccepted || 
            ss->version < SSL_LIBRARY_VERSION_TLS_1_3);

https://codereview.appspot.com/300370043/diff/20001/lib/ssl/sslsecur.c
File lib/ssl/sslsecur.c (right):

https://codereview.appspot.com/300370043/diff/20001/lib/ssl/sslsecur.c#newcod...
lib/ssl/sslsecur.c:926: **/
This ** on the tail of the comment is odd, I see that you have used it twice
though.

https://codereview.appspot.com/300370043/diff/20001/lib/ssl/sslsecur.c#newcod...
lib/ssl/sslsecur.c:931: ss->opt.enable0RttData) {
One line.

Unless you think that you should check that this is on the client (like you do
in the actual check).  I don't know if the locking guarantees on
ss->sec.isServer permit that though.

https://codereview.appspot.com/300370043/diff/20001/lib/ssl/sslsecur.c#newcod...
lib/ssl/sslsecur.c:965: ss->ssl3.hs.doing0Rtt);
one line.

Do you want to replicate the check above?

https://codereview.appspot.com/300370043/diff/20001/lib/ssl/sslt.h
File lib/ssl/sslt.h (right):

https://codereview.appspot.com/300370043/diff/20001/lib/ssl/sslt.h#newcode193
lib/ssl/sslt.h:193: * This field only has meaning in TLS >= 1.3.
Maybe say what it means as well.

https://codereview.appspot.com/300370043/diff/20001/lib/ssl/tls13con.c
File lib/ssl/tls13con.c (right):

https://codereview.appspot.com/300370043/diff/20001/lib/ssl/tls13con.c#newcod...
lib/ssl/tls13con.c:820: (ss->ssl3.crSpec->master_secret);
Bad wrapping, break after =.

https://codereview.appspot.com/300370043/diff/20001/lib/ssl/tls13con.c#newcod...
lib/ssl/tls13con.c:968: & ticket_allow_early_data);
OK, that's too ugly.  Boolean operation on logical operation on boolean
operation.

if (!(flags & allow_early_data)) {
  doing0Rtt = PR_FALSE;
}

https://codereview.appspot.com/300370043/diff/20001/lib/ssl/tls13con.c#newcod...
lib/ssl/tls13con.c:1332: * wrong in draft-13. */
Bug number?

https://codereview.appspot.com/300370043/diff/20001/lib/ssl/tls13con.c#newcod...
lib/ssl/tls13con.c:2306: // TODO(ekr@rtfm.com): Actually process this rather
than ignoring it.
/**/

https://codereview.appspot.com/300370043/diff/20001/lib/ssl/tls13con.c#newcod...
lib/ssl/tls13con.c:2321: // TODO(ekr@rtfm.com): Implement the ticket_age xtn.
/**/

https://codereview.appspot.com/300370043/diff/20001/lib/ssl/tls13con.c#newcod...
lib/ssl/tls13con.c:2364: SSL3Hashes tbsHash;
You still haven't explained what tbs stands for.

https://codereview.appspot.com/300370043/diff/20001/lib/ssl/tls13con.c#newcod...
lib/ssl/tls13con.c:2488: * hash. TODO(ekr@rtfm.com): We can fix this. */
bug number

https://codereview.appspot.com/300370043/diff/20001/lib/ssl/tls13con.c#newcod...
lib/ssl/tls13con.c:2915: flags, sizeof(flags));
one line

https://codereview.appspot.com/300370043/diff/20001/lib/ssl/tls13con.c#newcod...
lib/ssl/tls13con.c:3077: ExtensionSendEncrypted },
Didn't you earlier complain that this was wrong in the spec and that it requires
that this be in the clear?

https://codereview.appspot.com/300370043/diff/20001/lib/ssl/tls13con.c#newcod...
lib/ssl/tls13con.c:3083: ExtensionClientOnly }
For later: each of these fit on the one line, which would make this more
readable.

https://codereview.appspot.com/300370043/diff/20001/lib/ssl/tls13con.c#newcod...
lib/ssl/tls13con.c:3355: * preliminary info. */
OK, here's my suggestion for handling socket info during the handshake.

Use the preliminary info.  We should commit to TLS 1.3 and mark the version
field as present as soon as we start sending 0-RTT.  You need to do that in this
function, since this is where we commit to the 0-RTT thing.  We can reuse the
existing version field, because we can't downgrade version.

Preliminary info probably needs a boolean field that indicates that we are doing
0-RTT.  We don't seem to have one of those yet.  That doesn't need a
corresponding flag.

The cipher suite we are using in 0-RTT should be a new field, since that could
be different to what we eventually use.  We don't need to hide PSK on this one
though.  

The boolean argument to SetCipherSuite can be used to tell if this is handshake
or pre-handshake.  It can then set the appropriate flags depending on which way
it goes.

https://codereview.appspot.com/300370043/diff/20001/lib/ssl/tls13con.c#newcod...
lib/ssl/tls13con.c:3397: void *buf, PRInt32 len)
one line

https://codereview.appspot.com/300370043/diff/20001/lib/ssl/tls13hkdf.c
File lib/ssl/tls13hkdf.c (right):

https://codereview.appspot.com/300370043/diff/20001/lib/ssl/tls13hkdf.c#newco...
lib/ssl/tls13hkdf.c:96: ikm2 = zeroKey;
This is the first time I've ever seen someone overwrite an argument in NSS.  Are
we cool with that?

https://codereview.appspot.com/300370043/diff/20001/lib/ssl/tls13hkdf.c#newco...
lib/ssl/tls13hkdf.c:125: * Label, plus HandshakeHash. If it's ever to small, the
code will abort.
The code will only abort in a debug build.

[ekr-rietveld, 2016-06-21 20:01:27]

https://codereview.appspot.com/300370043/diff/20001/external_tests/ssl_gtest/...
File external_tests/ssl_gtest/libssl_internals.c (right):

https://codereview.appspot.com/300370043/diff/20001/external_tests/ssl_gtest/...
external_tests/ssl_gtest/libssl_internals.c:155: PR_FALSE;
On 2016/06/21 07:27:31, mt wrote:
> You need to return this.

You're right. Thanks.

https://codereview.appspot.com/300370043/diff/20001/external_tests/ssl_gtest/...
File external_tests/ssl_gtest/ssl_loopback_unittest.cc (right):

https://codereview.appspot.com/300370043/diff/20001/external_tests/ssl_gtest/...
external_tests/ssl_gtest/ssl_loopback_unittest.cc:1: 
On 2016/06/21 07:27:32, mt wrote:
> Remove.

Done.

https://codereview.appspot.com/300370043/diff/20001/external_tests/ssl_gtest/...
File external_tests/ssl_gtest/tls_agent.cc (right):

https://codereview.appspot.com/300370043/diff/20001/external_tests/ssl_gtest/...
external_tests/ssl_gtest/tls_agent.cc:565: expected = PR_FALSE;
On 2016/06/21 07:27:32, mt wrote:
> This mixing of PRBool and bool is confusing.
> 
> PSM takes the view that C++ uses bool and true/false always.  Should we?

seems OK when I change it

https://codereview.appspot.com/300370043/diff/20001/external_tests/ssl_gtest/...
File external_tests/ssl_gtest/tls_connect.cc (right):

https://codereview.appspot.com/300370043/diff/20001/external_tests/ssl_gtest/...
external_tests/ssl_gtest/tls_connect.cc:273: CheckEarlyDataAccepted();
On 2016/06/21 07:27:32, mt wrote:
> I would inline this function, just as you did for CheckSecretsDestroyed()

I'd rather not because the function refers tomembers.

https://codereview.appspot.com/300370043/diff/20001/lib/ssl/SSLerrs.h
File lib/ssl/SSLerrs.h (right):

https://codereview.appspot.com/300370043/diff/20001/lib/ssl/SSLerrs.h#newcode477
lib/ssl/SSLerrs.h:477: "SSL received an end of early data alert.")
On 2016/06/21 07:27:32, mt wrote:
> See previous comment.  I don't think that this needs to be in the name, just
the
> description.

But this is actually set even when we expect it.

https://codereview.appspot.com/300370043/diff/20001/lib/ssl/ssl.h
File lib/ssl/ssl.h (right):

https://codereview.appspot.com/300370043/diff/20001/lib/ssl/ssl.h#newcode225
lib/ssl/ssl.h:225: /* Allow 0-RTT data (for TLS 1.3) */
On 2016/06/21 07:27:32, mt wrote:
> This needs a lot of documentation.  Off the top of my head:
> 
> * How you use 0-RTT when you enable it (on both server and client).
> * The risks associated with using 0-RTT.
> * The contract that we provide, in particular that the handshake callback
marks
> the transition.
> * How to get information about the state of the connection when you are doing
> this.
> * How this relates to client authentication.

I added some. I don't think the client auth thing is relevant.

https://codereview.appspot.com/300370043/diff/20001/lib/ssl/ssl.h#newcode225
lib/ssl/ssl.h:225: /* Allow 0-RTT data (for TLS 1.3) */
On 2016/06/21 07:27:32, mt wrote:
> This needs a lot of documentation.  Off the top of my head:
> 
> * How you use 0-RTT when you enable it (on both server and client).
> * The risks associated with using 0-RTT.
> * The contract that we provide, in particular that the handshake callback
marks
> the transition.
> * How to get information about the state of the connection when you are doing
> this.
> * How this relates to client authentication.

I added some. I don't think the client auth thing is relevant.

https://codereview.appspot.com/300370043/diff/20001/lib/ssl/ssl3con.c
File lib/ssl/ssl3con.c (right):

https://codereview.appspot.com/300370043/diff/20001/lib/ssl/ssl3con.c#newcode...
lib/ssl/ssl3con.c:6081: return rv; /* error code set by
tls13_MaybeDo0RttHandshake */
On 2016/06/21 07:27:32, mt wrote:
> Should we be returning SECFailure here?  return rv has been so risky...

Done.

https://codereview.appspot.com/300370043/diff/20001/lib/ssl/ssl3con.c#newcode...
lib/ssl/ssl3con.c:6086: return rv;
On 2016/06/21 07:27:32, mt wrote:
> As above, except SECSuccess.

Done.

https://codereview.appspot.com/300370043/diff/20001/lib/ssl/ssl3con.c#newcode...
lib/ssl/ssl3con.c:8650: #ifndef SSL_SNI_ALLOW_NAME_CHANGE_2HS
On 2016/06/21 07:27:32, mt wrote:
> Missed one.

https://codereview.appspot.com/300370043/diff/20001/lib/ssl/ssl3con.c#newcode...
lib/ssl/ssl3con.c:11093: ss, ticket.data, ticket.len, 2);
On 2016/06/21 07:27:32, mt wrote:
> One line

Done.

https://codereview.appspot.com/300370043/diff/20001/lib/ssl/ssl3ecc.c
File lib/ssl/ssl3ecc.c (right):

https://codereview.appspot.com/300370043/diff/20001/lib/ssl/ssl3ecc.c#newcode379
lib/ssl/ssl3ecc.c:379: SECItem ecPoint = {siBuffer, NULL, 0};
On 2016/06/21 07:27:32, mt wrote:
> What does clang-format do here?  Because you added new structs elsewhere in
this
> patch that have spaces inside the braces.

Done.

https://codereview.appspot.com/300370043/diff/20001/lib/ssl/ssl3ext.c
File lib/ssl/ssl3ext.c (right):

https://codereview.appspot.com/300370043/diff/20001/lib/ssl/ssl3ext.c#newcode...
lib/ssl/ssl3ext.c:1991: /* Copy master secret. */
On 2016/06/21 07:27:32, mt wrote:
> This re-indenting will be destroyed by clang-format.

Done.

https://codereview.appspot.com/300370043/diff/20001/lib/ssl/ssl3ext.c#newcode...
lib/ssl/ssl3ext.c:3552: 
On 2016/06/21 07:27:32, mt wrote:
> Missing: if (maxBytes < (PRUint32)extension_length)

Done.

https://codereview.appspot.com/300370043/diff/20001/lib/ssl/ssl3ext.c#newcode...
lib/ssl/ssl3ext.c:3647: if (!(sid->u.ssl3.locked.sessionTicket.flags &
ticket_allow_early_data))
On 2016/06/21 07:27:32, mt wrote:
> I prefer == 0 for something like this, but will leave it to you.

Done.

https://codereview.appspot.com/300370043/diff/20001/lib/ssl/ssl3ext.c#newcode...
lib/ssl/ssl3ext.c:3701: if (data->len) {
On 2016/06/21 07:27:32, mt wrote:
> If we go with masking for the ticket_age, maybe we should just pack it into
the
> early_data extension instead of having a new extension.

Sure.

https://codereview.appspot.com/300370043/diff/20001/lib/ssl/ssl3ext.c#newcode...
lib/ssl/ssl3ext.c:3711: **/
On 2016/06/21 07:27:32, mt wrote:
> I would have said:
> 
> IMPORTANT: Record the presence of the extension only.  This is only one of
> several things that the 0-RTT processing code uses in determining whether to
> accept 0-RTT.
> 
> Also, the trailing **

Done.

https://codereview.appspot.com/300370043/diff/20001/lib/ssl/ssl3ext.c#newcode...
lib/ssl/ssl3ext.c:3725: 
On 2016/06/21 07:27:32, mt wrote:
> Missing maxBytes check.

Done.

https://codereview.appspot.com/300370043/diff/20001/lib/ssl/sslinfo.c
File lib/ssl/sslinfo.c (right):

https://codereview.appspot.com/300370043/diff/20001/lib/ssl/sslinfo.c#newcode93
lib/ssl/sslinfo.c:93: }
On 2016/06/21 07:27:32, mt wrote:
> You know, you could just remove the outer check.
> 
> Maybe add this as a sanity check:
> PORT_Assert(!inf.earlyDataAccepted || 
>             ss->version < SSL_LIBRARY_VERSION_TLS_1_3);

Done.

https://codereview.appspot.com/300370043/diff/20001/lib/ssl/sslsecur.c
File lib/ssl/sslsecur.c (right):

https://codereview.appspot.com/300370043/diff/20001/lib/ssl/sslsecur.c#newcod...
lib/ssl/sslsecur.c:926: **/
On 2016/06/21 07:27:32, mt wrote:
> This ** on the tail of the comment is odd, I see that you have used it twice
> though.

Done.

https://codereview.appspot.com/300370043/diff/20001/lib/ssl/sslsecur.c#newcod...
lib/ssl/sslsecur.c:931: ss->opt.enable0RttData) {
On 2016/06/21 07:27:32, mt wrote:
> One line.
> 
> Unless you think that you should check that this is on the client (like you do
> in the actual check).  I don't know if the locking guarantees on
> ss->sec.isServer permit that though.

I think it's fine. Moved it out to be faster.

https://codereview.appspot.com/300370043/diff/20001/lib/ssl/sslsecur.c#newcod...
lib/ssl/sslsecur.c:965: ss->ssl3.hs.doing0Rtt);
On 2016/06/21 07:27:32, mt wrote:
> one line.
> 
> Do you want to replicate the check above?

Done

https://codereview.appspot.com/300370043/diff/20001/lib/ssl/sslt.h
File lib/ssl/sslt.h (right):

https://codereview.appspot.com/300370043/diff/20001/lib/ssl/sslt.h#newcode193
lib/ssl/sslt.h:193: * This field only has meaning in TLS >= 1.3.
On 2016/06/21 07:27:32, mt wrote:
> Maybe say what it means as well.

Done.

https://codereview.appspot.com/300370043/diff/20001/lib/ssl/tls13con.c
File lib/ssl/tls13con.c (right):

https://codereview.appspot.com/300370043/diff/20001/lib/ssl/tls13con.c#newcod...
lib/ssl/tls13con.c:820: (ss->ssl3.crSpec->master_secret);
On 2016/06/21 07:27:33, mt wrote:
> Bad wrapping, break after =.

Done.

https://codereview.appspot.com/300370043/diff/20001/lib/ssl/tls13con.c#newcod...
lib/ssl/tls13con.c:968: & ticket_allow_early_data);
On 2016/06/21 07:27:33, mt wrote:
> OK, that's too ugly.  Boolean operation on logical operation on boolean
> operation.
> 
> if (!(flags & allow_early_data)) {
>   doing0Rtt = PR_FALSE;
> }

== 0 as per your previous comment.

https://codereview.appspot.com/300370043/diff/20001/lib/ssl/tls13con.c#newcod...
lib/ssl/tls13con.c:1332: * wrong in draft-13. */
On 2016/06/21 07:27:33, mt wrote:
> Bug number?

Done.

https://codereview.appspot.com/300370043/diff/20001/lib/ssl/tls13con.c#newcod...
lib/ssl/tls13con.c:2306: // TODO(ekr@rtfm.com): Actually process this rather
than ignoring it.
On 2016/06/21 07:27:33, mt wrote:
> /**/

Done.

https://codereview.appspot.com/300370043/diff/20001/lib/ssl/tls13con.c#newcod...
lib/ssl/tls13con.c:2321: // TODO(ekr@rtfm.com): Implement the ticket_age xtn.
On 2016/06/21 07:27:33, mt wrote:
> /**/

Done.

https://codereview.appspot.com/300370043/diff/20001/lib/ssl/tls13con.c#newcod...
lib/ssl/tls13con.c:2364: SSL3Hashes tbsHash;
On 2016/06/21 07:27:33, mt wrote:
> You still haven't explained what tbs stands for.

Done.

https://codereview.appspot.com/300370043/diff/20001/lib/ssl/tls13con.c#newcod...
lib/ssl/tls13con.c:2488: * hash. TODO(ekr@rtfm.com): We can fix this. */
On 2016/06/21 07:27:33, mt wrote:
> bug number

Done.

https://codereview.appspot.com/300370043/diff/20001/lib/ssl/tls13con.c#newcod...
lib/ssl/tls13con.c:2915: flags, sizeof(flags));
On 2016/06/21 07:27:33, mt wrote:
> one line

Done.

https://codereview.appspot.com/300370043/diff/20001/lib/ssl/tls13con.c#newcod...
lib/ssl/tls13con.c:3077: ExtensionSendEncrypted },
On 2016/06/21 07:27:33, mt wrote:
> Didn't you earlier complain that this was wrong in the spec and that it
requires
> that this be in the clear?

yeah, I fixed it.

https://codereview.appspot.com/300370043/diff/20001/lib/ssl/tls13con.c#newcod...
lib/ssl/tls13con.c:3083: ExtensionClientOnly }
On 2016/06/21 07:27:33, mt wrote:
> For later: each of these fit on the one line, which would make this more
> readable.

Did it anyway.

https://codereview.appspot.com/300370043/diff/20001/lib/ssl/tls13con.c#newcod...
lib/ssl/tls13con.c:3355: * preliminary info. */
On 2016/06/21 07:27:33, mt wrote:
> OK, here's my suggestion for handling socket info during the handshake.
> 
> Use the preliminary info.  We should commit to TLS 1.3 and mark the version
> field as present as soon as we start sending 0-RTT.  You need to do that in
this
> function, since this is where we commit to the 0-RTT thing.  We can reuse the
> existing version field, because we can't downgrade version.
> 
> Preliminary info probably needs a boolean field that indicates that we are
doing
> 0-RTT.  We don't seem to have one of those yet.  That doesn't need a
> corresponding flag.
> 
> The cipher suite we are using in 0-RTT should be a new field, since that could
> be different to what we eventually use.  We don't need to hide PSK on this one
> though.  
> 
> The boolean argument to SetCipherSuite can be used to tell if this is
handshake
> or pre-handshake.  It can then set the appropriate flags depending on which
way
> it goes.

Filed a bug.

https://codereview.appspot.com/300370043/diff/20001/lib/ssl/tls13con.c#newcod...
lib/ssl/tls13con.c:3397: void *buf, PRInt32 len)
On 2016/06/21 07:27:33, mt wrote:
> one line

Done.

https://codereview.appspot.com/300370043/diff/20001/lib/ssl/tls13hkdf.c
File lib/ssl/tls13hkdf.c (right):

https://codereview.appspot.com/300370043/diff/20001/lib/ssl/tls13hkdf.c#newco...
lib/ssl/tls13hkdf.c:96: ikm2 = zeroKey;
On 2016/06/21 07:27:33, mt wrote:
> This is the first time I've ever seen someone overwrite an argument in NSS. 
Are
> we cool with that?

I generally am, but since we don't seem to, I'll change it.

https://codereview.appspot.com/300370043/diff/20001/lib/ssl/tls13hkdf.c#newco...
lib/ssl/tls13hkdf.c:125: * Label, plus HandshakeHash. If it's ever to small, the
code will abort.
On 2016/06/21 07:27:33, mt wrote:
> The code will only abort in a debug build.

I don't believe you are correct. There is a jump to abort.

[mt, 2016-06-24 01:45:55]

LGTM

The tests look much nicer, thanks for doing that.

I recommend running clang-format, even if it's the version you get with
homebrew; this is going to burn when it lands and a small fixup patch will hurt
less.

https://codereview.appspot.com/300370043/diff/20001/lib/ssl/tls13hkdf.c
File lib/ssl/tls13hkdf.c (right):

https://codereview.appspot.com/300370043/diff/20001/lib/ssl/tls13hkdf.c#newco...
lib/ssl/tls13hkdf.c:17: // TODO(ekr@rtfm.com): Export this separately.
/**/

https://codereview.appspot.com/300370043/diff/20001/lib/ssl/tls13hkdf.c#newco...
lib/ssl/tls13hkdf.c:125: * Label, plus HandshakeHash. If it's ever to small, the
code will abort.
On 2016/06/21 20:01:27, ekr-webrtc wrote:
> On 2016/06/21 07:27:33, mt wrote:
> > The code will only abort in a debug build.
> 
> I don't believe you are correct. There is a jump to abort.

Acknowledged.

https://codereview.appspot.com/300370043/diff/40001/external_tests/ssl_gtest/...
File external_tests/ssl_gtest/ssl_loopback_unittest.cc (right):

https://codereview.appspot.com/300370043/diff/40001/external_tests/ssl_gtest/...
external_tests/ssl_gtest/ssl_loopback_unittest.cc:1096: TEST_F(TlsConnectTest,
TestTls13ZeroRttServerRejectByOption) {
Do you think that a new class would help here?  TlsConnectTest is getting pretty
big already.

https://codereview.appspot.com/300370043/diff/40001/external_tests/ssl_gtest/...
File external_tests/ssl_gtest/tls_connect.cc (right):

https://codereview.appspot.com/300370043/diff/40001/external_tests/ssl_gtest/...
external_tests/ssl_gtest/tls_connect.cc:425: std::cerr << "Read " << rv << "
bytes\n";
nit: Include "0-RTT" in the string here.

https://codereview.appspot.com/300370043/diff/40001/external_tests/ssl_gtest/...
File external_tests/ssl_gtest/tls_hkdf_unittest.cc (right):

https://codereview.appspot.com/300370043/diff/40001/external_tests/ssl_gtest/...
external_tests/ssl_gtest/tls_hkdf_unittest.cc:214: // TODO(ekr@rtfm.com): Tests
for SHA-384 (bug 1281020)
Tim landed his code, so you will need to rebase this.

https://codereview.appspot.com/300370043/diff/40001/lib/ssl/SSLerrs.h
File lib/ssl/SSLerrs.h (right):

https://codereview.appspot.com/300370043/diff/40001/lib/ssl/SSLerrs.h#newcode477
lib/ssl/SSLerrs.h:477: "SSL received an end of early data alert.")
Am I wrong about this?  Or are we miscommunicating somehow?  It's not always an
error to receive an end of early data alert, it's only an error if it's at the
wrong time.

https://codereview.appspot.com/300370043/diff/40001/lib/ssl/sslinfo.c
File lib/ssl/sslinfo.c (right):

https://codereview.appspot.com/300370043/diff/40001/lib/ssl/sslinfo.c#newcode86
lib/ssl/sslinfo.c:86: else {
} else {

https://codereview.appspot.com/300370043/diff/40001/lib/ssl/tls13con.c
File lib/ssl/tls13con.c (right):

https://codereview.appspot.com/300370043/diff/40001/lib/ssl/tls13con.c#newcod...
lib/ssl/tls13con.c:665: /* Extract off the resumptionPsk (if present), else
zeroes */
This could be a confusing comment.  The point is that resumptionPsk might be
NULL, but that's OK because HkdfExtract turns NULL into an all-zero value.

https://codereview.appspot.com/300370043/diff/40001/lib/ssl/tls13con.c#newcod...
lib/ssl/tls13con.c:1057: return SECFailure;
Given that you don't have any cleanup to do here, is there any reason you
actually need to use goto at all?  I prefer not to use it if it isn't needed.

https://codereview.appspot.com/300370043/diff/40001/lib/ssl/tls13con.c#newcod...
lib/ssl/tls13con.c:1383: return rv; /* err code is set. */
return SECFailure

https://codereview.appspot.com/300370043/diff/40001/lib/ssl/tls13con.c#newcod...
lib/ssl/tls13con.c:1389: return rv; /* error code is set. */
return SECFailure

https://codereview.appspot.com/300370043/diff/40001/lib/ssl/tls13con.c#newcod...
lib/ssl/tls13con.c:2432: PORT_Free(buf.data);
We should really use SECITEM_FreeItem(&buf, PR_FALSE) here, but none of the
functions that produce the signature use SECITEM_AllocItem...

https://codereview.appspot.com/300370043/diff/40001/lib/ssl/tls13con.c#newcod...
lib/ssl/tls13con.c:2673: }
Looking at this function, it seems like it might be better constructed as a
generic function that is called by non-generic functions.

i.e., tls13_Handle0RttFinished(ss, b, len, hashes) and
tls13_HandleServerFinished() and tls13_HandleClientFinished(), all of which call
down to the tls13_VerifyFinished() that calculates the hashes and does the
comparison.  Or you could expand ComputeFinished to take on that work.

Very little of the code in this function is common.

https://codereview.appspot.com/300370043/diff/40001/lib/ssl/tls13hkdf.c
File lib/ssl/tls13hkdf.c (right):

https://codereview.appspot.com/300370043/diff/40001/lib/ssl/tls13hkdf.c#newco...
lib/ssl/tls13hkdf.c:102: 
trailing space

[ekr-rietveld, 2016-06-25 02:41:10]

MT, this probably needs one more pass because I made significant changes to
Finished handling and added tests for it.

https://codereview.appspot.com/300370043/diff/20001/lib/ssl/tls13hkdf.c
File lib/ssl/tls13hkdf.c (right):

https://codereview.appspot.com/300370043/diff/20001/lib/ssl/tls13hkdf.c#newco...
lib/ssl/tls13hkdf.c:17: // TODO(ekr@rtfm.com): Export this separately.
On 2016/06/24 01:45:53, mt wrote:
> /**/

Done.

https://codereview.appspot.com/300370043/diff/40001/external_tests/ssl_gtest/...
File external_tests/ssl_gtest/ssl_loopback_unittest.cc (right):

https://codereview.appspot.com/300370043/diff/40001/external_tests/ssl_gtest/...
external_tests/ssl_gtest/ssl_loopback_unittest.cc:1096: TEST_F(TlsConnectTest,
TestTls13ZeroRttServerRejectByOption) {
On 2016/06/24 01:45:54, mt wrote:
> Do you think that a new class would help here?  TlsConnectTest is getting
pretty
> big already.

No, I think it would just be a superfluous subclass....

https://codereview.appspot.com/300370043/diff/40001/external_tests/ssl_gtest/...
File external_tests/ssl_gtest/tls_connect.cc (right):

https://codereview.appspot.com/300370043/diff/40001/external_tests/ssl_gtest/...
external_tests/ssl_gtest/tls_connect.cc:425: std::cerr << "Read " << rv << "
bytes\n";
On 2016/06/24 01:45:54, mt wrote:
> nit: Include "0-RTT" in the string here.

Done.

https://codereview.appspot.com/300370043/diff/40001/external_tests/ssl_gtest/...
File external_tests/ssl_gtest/tls_hkdf_unittest.cc (right):

https://codereview.appspot.com/300370043/diff/40001/external_tests/ssl_gtest/...
external_tests/ssl_gtest/tls_hkdf_unittest.cc:214: // TODO(ekr@rtfm.com): Tests
for SHA-384 (bug 1281020)
On 2016/06/24 01:45:54, mt wrote:
> Tim landed his code, so you will need to rebase this.

Will do after you review this patch

https://codereview.appspot.com/300370043/diff/40001/lib/ssl/SSLerrs.h
File lib/ssl/SSLerrs.h (right):

https://codereview.appspot.com/300370043/diff/40001/lib/ssl/SSLerrs.h#newcode477
lib/ssl/SSLerrs.h:477: "SSL received an end of early data alert.")
On 2016/06/24 01:45:54, mt wrote:
> Am I wrong about this?  Or are we miscommunicating somehow?  It's not always
an
> error to receive an end of early data alert, it's only an error if it's at the
> wrong time.

We set the error code to it in any case, but then don't PORT_SetError() it
unless it's at the wrong time. I changed it

https://codereview.appspot.com/300370043/diff/40001/lib/ssl/sslinfo.c
File lib/ssl/sslinfo.c (right):

https://codereview.appspot.com/300370043/diff/40001/lib/ssl/sslinfo.c#newcode86
lib/ssl/sslinfo.c:86: else {
On 2016/06/24 01:45:54, mt wrote:
> } else {

Done.

https://codereview.appspot.com/300370043/diff/40001/lib/ssl/tls13con.c
File lib/ssl/tls13con.c (right):

https://codereview.appspot.com/300370043/diff/40001/lib/ssl/tls13con.c#newcod...
lib/ssl/tls13con.c:665: /* Extract off the resumptionPsk (if present), else
zeroes */
On 2016/06/24 01:45:55, mt wrote:
> This could be a confusing comment.  The point is that resumptionPsk might be
> NULL, but that's OK because HkdfExtract turns NULL into an all-zero value.

I fixed this in my tree.

https://codereview.appspot.com/300370043/diff/40001/lib/ssl/tls13con.c#newcod...
lib/ssl/tls13con.c:1057: return SECFailure;
On 2016/06/24 01:45:55, mt wrote:
> Given that you don't have any cleanup to do here, is there any reason you
> actually need to use goto at all?  I prefer not to use it if it isn't needed.

Done.

https://codereview.appspot.com/300370043/diff/40001/lib/ssl/tls13con.c#newcod...
lib/ssl/tls13con.c:1383: return rv; /* err code is set. */
On 2016/06/24 01:45:54, mt wrote:
> return SECFailure

Done.

https://codereview.appspot.com/300370043/diff/40001/lib/ssl/tls13con.c#newcod...
lib/ssl/tls13con.c:1389: return rv; /* error code is set. */
On 2016/06/24 01:45:54, mt wrote:
> return SECFailure

Done.

https://codereview.appspot.com/300370043/diff/40001/lib/ssl/tls13con.c#newcod...
lib/ssl/tls13con.c:2432: PORT_Free(buf.data);
On 2016/06/24 01:45:54, mt wrote:
> We should really use SECITEM_FreeItem(&buf, PR_FALSE) here, but none of the
> functions that produce the signature use SECITEM_AllocItem...

Yeah, left as-is

https://codereview.appspot.com/300370043/diff/40001/lib/ssl/tls13con.c#newcod...
lib/ssl/tls13con.c:2673: }
On 2016/06/24 01:45:55, mt wrote:
> Looking at this function, it seems like it might be better constructed as a
> generic function that is called by non-generic functions.
> 
> i.e., tls13_Handle0RttFinished(ss, b, len, hashes) and
> tls13_HandleServerFinished() and tls13_HandleClientFinished(), all of which
call
> down to the tls13_VerifyFinished() that calculates the hashes and does the
> comparison.  Or you could expand ComputeFinished to take on that work.
> 
> Very little of the code in this function is common.

Done.

https://codereview.appspot.com/300370043/diff/40001/lib/ssl/tls13con.c#newcod...
lib/ssl/tls13con.c:3341: rv =  ss->ssl3.hs.preliminaryInfo = 0; /*
TODO(ekr@rtfm.com) Fill this in.
OK that totally shouldn't have happened.

https://codereview.appspot.com/300370043/diff/40001/lib/ssl/tls13hkdf.c
File lib/ssl/tls13hkdf.c (right):

https://codereview.appspot.com/300370043/diff/40001/lib/ssl/tls13hkdf.c#newco...
lib/ssl/tls13hkdf.c:102: 
On 2016/06/24 01:45:55, mt wrote:
> trailing space

Done.

https://codereview.appspot.com/300370043/diff/60001/external_tests/ssl_gtest/...
File external_tests/ssl_gtest/ssl_loopback_unittest.cc (right):

https://codereview.appspot.com/300370043/diff/60001/external_tests/ssl_gtest/...
external_tests/ssl_gtest/ssl_loopback_unittest.cc:1111:
server_->CheckErrorCode(SSL_ERROR_BAD_HANDSHAKE_HASH_VALUE);
Add error codes here.

https://codereview.appspot.com/300370043/diff/60001/external_tests/ssl_gtest/...
external_tests/ssl_gtest/ssl_loopback_unittest.cc:1177:
server_->CheckErrorCode(SSL_ERROR_BAD_HANDSHAKE_HASH_VALUE);
Check erro code here.

[mt, 2016-06-27 00:54:21]

LGTM

Mostly just nits.  A little more testing would be good on the case where the
client Finished is damaged.

p.s., I realized that the sorting of files is fortuitous for my style of
reviewing.  I like reviewing the tests first :)

https://codereview.appspot.com/300370043/diff/80001/external_tests/ssl_gtest/...
File external_tests/ssl_gtest/libssl_internals.c (right):

https://codereview.appspot.com/300370043/diff/80001/external_tests/ssl_gtest/...
external_tests/ssl_gtest/libssl_internals.c:181:
PK11_FreeSymKey(ss->ssl3.hs.hsTrafficSecret);
Should you null check here first?  Return false if it isn't there?

https://codereview.appspot.com/300370043/diff/80001/external_tests/ssl_gtest/...
external_tests/ssl_gtest/libssl_internals.c:189: PRBool
SSLInt_DamageEarlyTrafficSecret(PRFileDesc *fd)
This runs the same code as the other function.

https://codereview.appspot.com/300370043/diff/80001/external_tests/ssl_gtest/...
File external_tests/ssl_gtest/ssl_loopback_unittest.cc (right):

https://codereview.appspot.com/300370043/diff/80001/external_tests/ssl_gtest/...
external_tests/ssl_gtest/ssl_loopback_unittest.cc:1113:
server_->CheckErrorCode(SSL_ERROR_BAD_HANDSHAKE_HASH_VALUE);
I think that you should continue and look for an alert from the server.

https://codereview.appspot.com/300370043/diff/80001/external_tests/ssl_gtest/...
external_tests/ssl_gtest/ssl_loopback_unittest.cc:1116: 
extra blank line

https://codereview.appspot.com/300370043/diff/80001/external_tests/ssl_gtest/...
external_tests/ssl_gtest/ssl_loopback_unittest.cc:1120: AfterRecordN(TlsAgent
*src, TlsAgent *dest, int record, VoidFunction func) :
record should be unsigned

https://codereview.appspot.com/300370043/diff/80001/external_tests/ssl_gtest/...
external_tests/ssl_gtest/ssl_loopback_unittest.cc:1130: if (ct_ == record_) {
Did you want to make the counter start from zero?

https://codereview.appspot.com/300370043/diff/80001/external_tests/ssl_gtest/...
external_tests/ssl_gtest/ssl_loopback_unittest.cc:1147: int ct_;
counter_

https://codereview.appspot.com/300370043/diff/80001/external_tests/ssl_gtest/...
external_tests/ssl_gtest/ssl_loopback_unittest.cc:1158: 1,
This is the ServerHello. A comment to that effect might help.

https://codereview.appspot.com/300370043/diff/80001/external_tests/ssl_gtest/...
external_tests/ssl_gtest/ssl_loopback_unittest.cc:1174: 1,
ClientHello

https://codereview.appspot.com/300370043/diff/80001/external_tests/ssl_gtest/...
external_tests/ssl_gtest/ssl_loopback_unittest.cc:1253: class BeforeFinished :
public TlsRecordFilter {
So... The tests that depend on this could be refactored to use AfterRecordN.  Is
the thinking that that basing decisions on the content type is more robust and
that this is better left as-is for that reason?

https://codereview.appspot.com/300370043/diff/80001/lib/ssl/tls13con.c
File lib/ssl/tls13con.c (right):

https://codereview.appspot.com/300370043/diff/80001/lib/ssl/tls13con.c#newcod...
lib/ssl/tls13con.c:498: if (!ss->sec.isServer) {
nit: invert check

https://codereview.appspot.com/300370043/diff/80001/lib/ssl/tls13con.c#newcod...
lib/ssl/tls13con.c:503: 
extra blank line

https://codereview.appspot.com/300370043/diff/80001/lib/ssl/tls13con.c#newcod...
lib/ssl/tls13con.c:1010: SECEqual) {
SECEqual should fit on the previous line

https://codereview.appspot.com/300370043/diff/80001/lib/ssl/tls13con.c#newcod...
lib/ssl/tls13con.c:2658: const TLS13CombinedHash *hashes)
This refactor looks much better.

https://codereview.appspot.com/300370043/diff/80001/lib/ssl/tls13con.c#newcod...
lib/ssl/tls13con.c:2714: return  tls13_SendClientSecondRound(ss);
extra space

[ekr-rietveld, 2016-06-27 01:28:23]

When you asked for more tests did you just mean continuing and looking for the
alert or something else?

https://codereview.appspot.com/300370043/diff/80001/external_tests/ssl_gtest/...
File external_tests/ssl_gtest/libssl_internals.c (right):

https://codereview.appspot.com/300370043/diff/80001/external_tests/ssl_gtest/...
external_tests/ssl_gtest/libssl_internals.c:181:
PK11_FreeSymKey(ss->ssl3.hs.hsTrafficSecret);
On 2016/06/27 00:54:22, mt wrote:
> Should you null check here first?  Return false if it isn't there?

Probably, though it will crash in any case

https://codereview.appspot.com/300370043/diff/80001/external_tests/ssl_gtest/...
external_tests/ssl_gtest/libssl_internals.c:189: PRBool
SSLInt_DamageEarlyTrafficSecret(PRFileDesc *fd)
On 2016/06/27 00:54:22, mt wrote:
> This runs the same code as the other function.

Do you just mean the null key creation? I can refactor that out. 

Note that it stomps a different key.

[mt, 2016-06-27 01:40:11]

On 2016/06/27 01:28:23, ekr-webrtc wrote:
> When you asked for more tests did you just mean continuing and looking for the
> alert or something else?

Nope, that's all.

> Do you just mean the null key creation? I can refactor that out. 
> 
> Note that it stomps a different key.

Different key, same code.  I'm a pattern matching machine.

[ekr-rietveld, 2016-06-27 02:11:12]

On 2016/06/27 01:40:11, mt wrote:
> On 2016/06/27 01:28:23, ekr-webrtc wrote:
> > When you asked for more tests did you just mean continuing and looking for
the
> > alert or something else?
> 
> Nope, that's all.
> 
> > Do you just mean the null key creation? I can refactor that out. 
> > 
> > Note that it stomps a different key.
> 
> Different key, same code.  I'm a pattern matching machine.

Yeah, I know. I can refactor I guess, with a wrapper function....

Or a macro!

[ekr-rietveld, 2016-06-27 16:25:08]

Landed as: https://hg.mozilla.org/projects/nss/rev/de16ad00f641

https://codereview.appspot.com/300370043/diff/80001/external_tests/ssl_gtest/...
File external_tests/ssl_gtest/libssl_internals.c (right):

https://codereview.appspot.com/300370043/diff/80001/external_tests/ssl_gtest/...
external_tests/ssl_gtest/libssl_internals.c:189: PRBool
SSLInt_DamageEarlyTrafficSecret(PRFileDesc *fd)
On 2016/06/27 01:28:23, ekr-webrtc wrote:
> On 2016/06/27 00:54:22, mt wrote:
> > This runs the same code as the other function.
> 
> Do you just mean the null key creation? I can refactor that out. 
> 
> Note that it stomps a different key.

offsetof ftw

https://codereview.appspot.com/300370043/diff/80001/external_tests/ssl_gtest/...
File external_tests/ssl_gtest/ssl_loopback_unittest.cc (right):

https://codereview.appspot.com/300370043/diff/80001/external_tests/ssl_gtest/...
external_tests/ssl_gtest/ssl_loopback_unittest.cc:1113:
server_->CheckErrorCode(SSL_ERROR_BAD_HANDSHAKE_HASH_VALUE);
On 2016/06/27 00:54:22, mt wrote:
> I think that you should continue and look for an alert from the server.

Done.

https://codereview.appspot.com/300370043/diff/80001/external_tests/ssl_gtest/...
external_tests/ssl_gtest/ssl_loopback_unittest.cc:1116: 
On 2016/06/27 00:54:22, mt wrote:
> extra blank line

Done.

https://codereview.appspot.com/300370043/diff/80001/external_tests/ssl_gtest/...
external_tests/ssl_gtest/ssl_loopback_unittest.cc:1130: if (ct_ == record_) {
On 2016/06/27 00:54:22, mt wrote:
> Did you want to make the counter start from zero?

I thought it would conceptually easier to have it start from one but I guess we
do zero elsewhere.

https://codereview.appspot.com/300370043/diff/80001/external_tests/ssl_gtest/...
external_tests/ssl_gtest/ssl_loopback_unittest.cc:1147: int ct_;
On 2016/06/27 00:54:22, mt wrote:
> counter_

Done.

https://codereview.appspot.com/300370043/diff/80001/external_tests/ssl_gtest/...
external_tests/ssl_gtest/ssl_loopback_unittest.cc:1158: 1,
On 2016/06/27 00:54:22, mt wrote:
> This is the ServerHello. A comment to that effect might help.

Done.

https://codereview.appspot.com/300370043/diff/80001/external_tests/ssl_gtest/...
external_tests/ssl_gtest/ssl_loopback_unittest.cc:1174: 1,
On 2016/06/27 00:54:22, mt wrote:
> ClientHello

Done.

https://codereview.appspot.com/300370043/diff/80001/external_tests/ssl_gtest/...
external_tests/ssl_gtest/ssl_loopback_unittest.cc:1253: class BeforeFinished :
public TlsRecordFilter {
On 2016/06/27 00:54:22, mt wrote:
> So... The tests that depend on this could be refactored to use AfterRecordN. 
Is
> the thinking that that basing decisions on the content type is more robust and
> that this is better left as-is for that reason?

I didn't really think it through, but yes, I think that's better. I avoid this
counting where I can.

https://codereview.appspot.com/300370043/diff/80001/lib/ssl/tls13con.c
File lib/ssl/tls13con.c (right):

https://codereview.appspot.com/300370043/diff/80001/lib/ssl/tls13con.c#newcod...
lib/ssl/tls13con.c:498: if (!ss->sec.isServer) {
On 2016/06/27 00:54:22, mt wrote:
> nit: invert check

Done.

https://codereview.appspot.com/300370043/diff/80001/lib/ssl/tls13con.c#newcod...
lib/ssl/tls13con.c:503: 
On 2016/06/27 00:54:22, mt wrote:
> extra blank line

Done.

https://codereview.appspot.com/300370043/diff/80001/lib/ssl/tls13con.c#newcod...
lib/ssl/tls13con.c:1010: SECEqual) {
On 2016/06/27 00:54:22, mt wrote:
> SECEqual should fit on the previous line