Bug 1286140: HelloRetryRequest

Bug 1286140: HelloRetryRequest

[ekr-rietveld, 2016-08-01 12:02:59]

You seem to not be handling cookies correctly. At minimum you need to copy them
and replay. Note that this will also conveniently involve storing a "hrrCookie"
value which you can use instead of two extra states

https://codereview.appspot.com/301480043/diff/1/external_tests/ssl_gtest/ssl_...
File external_tests/ssl_gtest/ssl_hrr_unittest.cc (right):

https://codereview.appspot.com/301480043/diff/1/external_tests/ssl_gtest/ssl_...
external_tests/ssl_gtest/ssl_hrr_unittest.cc:26: class KeyShareFixer : public
TlsExtensionFilter {
http://searchfox.org/mozilla-central/source/security/nss/external_tests/ssl_g...

https://codereview.appspot.com/301480043/diff/1/external_tests/ssl_gtest/ssl_...
external_tests/ssl_gtest/ssl_hrr_unittest.cc:27: public:
1. This seems like it should be named "Replayer".
2. Why not put it in the usual place.
3. You should make this generic by parametrizing the extension point

https://codereview.appspot.com/301480043/diff/1/external_tests/ssl_gtest/ssl_...
external_tests/ssl_gtest/ssl_hrr_unittest.cc:98: 
There's an easier test than this. Have a canned HRR and then just send it twice
in ssl_agent_unittest.cc.

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

https://codereview.appspot.com/301480043/diff/1/lib/ssl/ssl3con.c#newcode5574
lib/ssl/ssl3con.c:5574: SSL_TRC(3, ("%d: SSL3[%d]: send client_hello handshake",
SSL_GETPID(),
It seems like the type merits being logged.

https://codereview.appspot.com/301480043/diff/1/lib/ssl/ssl3con.c#newcode7048
lib/ssl/ssl3con.c:7048: }
You won't need this code with the new negotiation.

Also, just generally sad about having this code here. How about putting it in
the CH/SH handlers. It's not like it can happen with every call.

https://codereview.appspot.com/301480043/diff/1/lib/ssl/ssl3con.c#newcode7141
lib/ssl/ssl3con.c:7141: }
Where do you check it didn't change the cipher suite?

https://codereview.appspot.com/301480043/diff/1/lib/ssl/ssl3con.c#newcode8895
lib/ssl/ssl3con.c:8895: idle_handshake)) {
It seems unfortunate to use this test for 1.2. It's also a bit unfortunate that
it works. TLS13_IN_HS_STATE() should actually enforce two-way separatio

https://codereview.appspot.com/301480043/diff/1/lib/ssl/ssl3con.c#newcode8926
lib/ssl/ssl3con.c:8926: if (IS_DTLS(ss) && ss->firstHsDone) {
Is the reason you made this change because of the assert at:
http://searchfox.org/mozilla-central/source/security/nss/lib/ssl/dtlscon.c#948

If so, I suggest making the change there instead.

https://codereview.appspot.com/301480043/diff/1/lib/ssl/ssl3con.c#newcode10075
lib/ssl/ssl3con.c:10075: params = ssl_GetDHEParams(groupDef);
Are these rebase artifacts? If not why are they in this CL?

https://codereview.appspot.com/301480043/diff/1/lib/ssl/sslimpl.h
File lib/ssl/sslimpl.h (right):

https://codereview.appspot.com/301480043/diff/1/lib/ssl/sslimpl.h#newcode779
lib/ssl/sslimpl.h:779: } SSL3WaitState;
My instinct would be to have a separate bit for "HRR in play" rather than to
introduce two new states.

https://codereview.appspot.com/301480043/diff/1/lib/ssl/sslimpl.h#newcode784
lib/ssl/sslimpl.h:784: client_hello_retransmit,   /* in DTLS when there is no
ServerHello */
"in DTLS if we receive HVR..."

All of these should follow the "starts with caps and ends with period" rule

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

https://codereview.appspot.com/301480043/diff/1/lib/ssl/tls13con.c#newcode498
lib/ssl/tls13con.c:498: PRUint32 length, SSL3Hashes *hashesPtr)
I wonder if this needs to be renamed, in that we are now handling a pre-hello
message here?

https://codereview.appspot.com/301480043/diff/1/lib/ssl/tls13con.c#newcode1140
lib/ssl/tls13con.c:1140: }
This seems like it should be an assert. Also, you can use TLS13_IN_HS_STATE, no?

https://codereview.appspot.com/301480043/diff/1/lib/ssl/tls13con.c#newcode1144
lib/ssl/tls13con.c:1144: rv = ssl3_AppendHandshakeHeader(ss,
hello_retry_request, 8);
Please compute this value.

https://codereview.appspot.com/301480043/diff/1/lib/ssl/tls13con.c#newcode1168
lib/ssl/tls13con.c:1168: rv = ssl3_AppendHandshakeNumber(ss, 0, 2);
Comment? This is empty extensions.

https://codereview.appspot.com/301480043/diff/1/lib/ssl/tls13con.c#newcode1245
lib/ssl/tls13con.c:1245: if (offer->group == selectedGroup) {
Do you think you should store the selectedGroup to ensure that nothing changes?

https://codereview.appspot.com/301480043/diff/1/lib/ssl/tls13con.c#newcode1264
lib/ssl/tls13con.c:1264: rv = tls13_CreateKeyShare(ss, selectedGroup);
good change

https://codereview.appspot.com/301480043/diff/1/lib/ssl/tls13con.c#newcode1268
lib/ssl/tls13con.c:1268: keyPair = ((sslEphemeralKeyPair
*)PR_NEXT_LINK(&ss->ephemeralKeyPairs));
Also, plese check it has one entry

https://codereview.appspot.com/301480043/diff/1/lib/ssl/tls13con.c#newcode1395
lib/ssl/tls13con.c:1395: /* Ignore the cipher suite, it's going to be removed
soon anyway. */
Put this comment above.

https://codereview.appspot.com/301480043/diff/1/lib/ssl/tls13con.c#newcode1423
lib/ssl/tls13con.c:1423: }
You should check that there isn't already a key share for this group.

[mt, 2016-08-03 02:32:08]

Using the cookie doesn't work because it isn't mandatory for the server to send:
https://tlswg.github.io/tls13-spec/#cookie

I've opened a bug to add support for the cookie, but as demonstrated by the
tests, this is patch is still good for handling the case where we don't have a
key share, which was the more pressing need.  I know that the cookie is easy,
but this is a discrete and useful increment.

See https://bugzilla.mozilla.org/show_bug.cgi?id=1290784

https://codereview.appspot.com/301480043/diff/1/external_tests/ssl_gtest/ssl_...
File external_tests/ssl_gtest/ssl_hrr_unittest.cc (right):

https://codereview.appspot.com/301480043/diff/1/external_tests/ssl_gtest/ssl_...
external_tests/ssl_gtest/ssl_hrr_unittest.cc:26: class KeyShareFixer : public
TlsExtensionFilter {
On 2016/08/01 12:02:59, ekr-webrtc wrote:
>
http://searchfox.org/mozilla-central/source/security/nss/external_tests/ssl_g...

That doesn't work because this copies the first occurrence and replaces the
second with that value.

https://codereview.appspot.com/301480043/diff/1/external_tests/ssl_gtest/ssl_...
external_tests/ssl_gtest/ssl_hrr_unittest.cc:27: public:
On 2016/08/01 12:02:59, ekr-webrtc wrote:
> 1. This seems like it should be named "Replayer".

Done.

> 2. Why not put it in the usual place.

This is single use stuff, here is fine.

> 3. You should make this generic by parametrizing the extension point

YAGNI

https://codereview.appspot.com/301480043/diff/1/external_tests/ssl_gtest/ssl_...
external_tests/ssl_gtest/ssl_hrr_unittest.cc:98: 
On 2016/08/01 12:02:59, ekr-webrtc wrote:
> There's an easier test than this. Have a canned HRR and then just send it
twice
> in ssl_agent_unittest.cc.

This test actually results in a connection if the test fails.  I figured that
that would be valuable.

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

https://codereview.appspot.com/301480043/diff/1/lib/ssl/ssl3con.c#newcode5574
lib/ssl/ssl3con.c:5574: SSL_TRC(3, ("%d: SSL3[%d]: send client_hello handshake",
SSL_GETPID(),
On 2016/08/01 12:02:59, ekr-webrtc wrote:
> It seems like the type merits being logged.

Done.

https://codereview.appspot.com/301480043/diff/1/lib/ssl/ssl3con.c#newcode7048
lib/ssl/ssl3con.c:7048: }
On 2016/08/01 12:02:59, ekr-webrtc wrote:
> You won't need this code with the new negotiation.

That's not true.  The ClientHello might still switch AES-GCM for ChaCha.

> Also, just generally sad about having this code here. How about putting it in
> the CH/SH handlers. It's not like it can happen with every call.

Done.

https://codereview.appspot.com/301480043/diff/1/lib/ssl/ssl3con.c#newcode7141
lib/ssl/ssl3con.c:7141: }
On 2016/08/01 12:02:59, ekr-webrtc wrote:
> Where do you check it didn't change the cipher suite?

See above.  However, since we're removing cipher suite from HRR, I've removed
the check on the client side.  Only the server checks now.

https://codereview.appspot.com/301480043/diff/1/lib/ssl/ssl3con.c#newcode8895
lib/ssl/ssl3con.c:8895: idle_handshake)) {
On 2016/08/01 12:02:59, ekr-webrtc wrote:
> It seems unfortunate to use this test for 1.2. It's also a bit unfortunate
that
> it works. TLS13_IN_HS_STATE() should actually enforce two-way separatio

I could use wait_2nd_client_hello without using the 1.3 flag I suppose.  That
would probably work.  WDYT?

https://codereview.appspot.com/301480043/diff/1/lib/ssl/ssl3con.c#newcode8926
lib/ssl/ssl3con.c:8926: if (IS_DTLS(ss) && ss->firstHsDone) {
On 2016/08/01 12:02:59, ekr-webrtc wrote:
> Is the reason you made this change because of the assert at:
> http://searchfox.org/mozilla-central/source/security/nss/lib/ssl/dtlscon.c#948
> 
> If so, I suggest making the change there instead.

Done.

https://codereview.appspot.com/301480043/diff/1/lib/ssl/ssl3con.c#newcode10075
lib/ssl/ssl3con.c:10075: params = ssl_GetDHEParams(groupDef);
On 2016/08/01 12:02:59, ekr-webrtc wrote:
> Are these rebase artifacts? If not why are they in this CL?

I needed to refactor the bit that creates a key share.  It meant simplifying the
way that we select the group.

https://codereview.appspot.com/301480043/diff/1/lib/ssl/sslimpl.h
File lib/ssl/sslimpl.h (right):

https://codereview.appspot.com/301480043/diff/1/lib/ssl/sslimpl.h#newcode779
lib/ssl/sslimpl.h:779: } SSL3WaitState;
On 2016/08/01 12:02:59, ekr-webrtc wrote:
> My instinct would be to have a separate bit for "HRR in play" rather than to
> introduce two new states.

Acknowledged.

https://codereview.appspot.com/301480043/diff/1/lib/ssl/sslimpl.h#newcode784
lib/ssl/sslimpl.h:784: client_hello_retransmit,   /* in DTLS when there is no
ServerHello */
On 2016/08/01 12:02:59, ekr-webrtc wrote:
> "in DTLS if we receive HVR..."
> 
> All of these should follow the "starts with caps and ends with period" rule

Done.

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

https://codereview.appspot.com/301480043/diff/1/lib/ssl/tls13con.c#newcode498
lib/ssl/tls13con.c:498: PRUint32 length, SSL3Hashes *hashesPtr)
On 2016/08/01 12:03:00, ekr-webrtc wrote:
> I wonder if this needs to be renamed, in that we are now handling a pre-hello
> message here?

I considered just removing the "Post".

https://codereview.appspot.com/301480043/diff/1/lib/ssl/tls13con.c#newcode1140
lib/ssl/tls13con.c:1140: }
On 2016/08/01 12:03:00, ekr-webrtc wrote:
> This seems like it should be an assert. Also, you can use TLS13_IN_HS_STATE,
no?

I wrote it as an assert, but this is the check that catches the case when we
receive a second ClientHello without a usable share.

https://codereview.appspot.com/301480043/diff/1/lib/ssl/tls13con.c#newcode1144
lib/ssl/tls13con.c:1144: rv = ssl3_AppendHandshakeHeader(ss,
hello_retry_request, 8);
On 2016/08/01 12:03:00, ekr-webrtc wrote:
> Please compute this value.

Done.

https://codereview.appspot.com/301480043/diff/1/lib/ssl/tls13con.c#newcode1168
lib/ssl/tls13con.c:1168: rv = ssl3_AppendHandshakeNumber(ss, 0, 2);
On 2016/08/01 12:03:00, ekr-webrtc wrote:
> Comment? This is empty extensions.

Done.

https://codereview.appspot.com/301480043/diff/1/lib/ssl/tls13con.c#newcode1245
lib/ssl/tls13con.c:1245: if (offer->group == selectedGroup) {
On 2016/08/01 12:03:00, ekr-webrtc wrote:
> Do you think you should store the selectedGroup to ensure that nothing
changes?

I would rather we just rely on the client behaving.  When/if we go stateless
reject, then we either won't need to check (because reconstructing state will
depend on them being consistent), or we can save the choice in the cookie. 
Without that, the effect is the same as having the client use the HRR to change
their mind and open a new connection.

https://codereview.appspot.com/301480043/diff/1/lib/ssl/tls13con.c#newcode1264
lib/ssl/tls13con.c:1264: rv = tls13_CreateKeyShare(ss, selectedGroup);
On 2016/08/01 12:02:59, ekr-webrtc wrote:
> good change

Acknowledged.

https://codereview.appspot.com/301480043/diff/1/lib/ssl/tls13con.c#newcode1268
lib/ssl/tls13con.c:1268: keyPair = ((sslEphemeralKeyPair
*)PR_NEXT_LINK(&ss->ephemeralKeyPairs));
On 2016/08/01 12:03:00, ekr-webrtc wrote:
> Also, plese check it has one entry

Done.

https://codereview.appspot.com/301480043/diff/1/lib/ssl/tls13con.c#newcode1395
lib/ssl/tls13con.c:1395: /* Ignore the cipher suite, it's going to be removed
soon anyway. */
On 2016/08/01 12:03:00, ekr-webrtc wrote:
> Put this comment above.

Done.

https://codereview.appspot.com/301480043/diff/1/lib/ssl/tls13con.c#newcode1423
lib/ssl/tls13con.c:1423: }
On 2016/08/01 12:03:00, ekr-webrtc wrote:
> You should check that there isn't already a key share for this group.

Done.

[ekr-rietveld, 2016-08-03 16:15:20]

I acknowledge your point about the cookie not being required now, but I think
it's insufficient. It's true it can be empty, but you can allocate a SECItem*
and have it being non-NULL be the trigger. As before, this eliminates two
otherwise unnecessary explicit states.

Note also that the server's extra state is obviously going to be a problem when
the server is stateless, so we might as well deal with that now.

https://codereview.appspot.com/301480043/diff/1/external_tests/ssl_gtest/ssl_...
File external_tests/ssl_gtest/ssl_hrr_unittest.cc (right):

https://codereview.appspot.com/301480043/diff/1/external_tests/ssl_gtest/ssl_...
external_tests/ssl_gtest/ssl_hrr_unittest.cc:26: class KeyShareFixer : public
TlsExtensionFilter {
On 2016/08/03 02:32:09, mt wrote:
> On 2016/08/01 12:02:59, ekr-webrtc wrote:
> >
>
http://searchfox.org/mozilla-central/source/security/nss/external_tests/ssl_g...
> 
> That doesn't work because this copies the first occurrence and replaces the
> second with that value.


Yes, I thought I had deleted this. Sorry

https://codereview.appspot.com/301480043/diff/1/external_tests/ssl_gtest/ssl_...
external_tests/ssl_gtest/ssl_hrr_unittest.cc:98: 
On 2016/08/03 02:32:08, mt wrote:
> On 2016/08/01 12:02:59, ekr-webrtc wrote:
> > There's an easier test than this. Have a canned HRR and then just send it
> twice
> > in ssl_agent_unittest.cc.
> 
> This test actually results in a connection if the test fails.  I figured that
> that would be valuable.

I guess. In the agent tests you just check to see if you are in the connecting
or failed states.

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

https://codereview.appspot.com/301480043/diff/1/lib/ssl/ssl3con.c#newcode7048
lib/ssl/ssl3con.c:7048: }
On 2016/08/03 02:32:09, mt wrote:
> On 2016/08/01 12:02:59, ekr-webrtc wrote:
> > You won't need this code with the new negotiation.
> 
> That's not true.  The ClientHello might still switch AES-GCM for ChaCha.

The cipher suite isn't even sent in the HRR in the new negotiation. There's no
need to specially check this value as opposed to the other values.


> > Also, just generally sad about having this code here. How about putting it
in
> > the CH/SH handlers. It's not like it can happen with every call.
> 
> Done.

https://codereview.appspot.com/301480043/diff/1/lib/ssl/ssl3con.c#newcode7141
lib/ssl/ssl3con.c:7141: }
On 2016/08/03 02:32:09, mt wrote:
> On 2016/08/01 12:02:59, ekr-webrtc wrote:
> > Where do you check it didn't change the cipher suite?
> 
> See above.  However, since we're removing cipher suite from HRR, I've removed
> the check on the client side.  Only the server checks now.


I don't think you need to check at all. See above.

https://codereview.appspot.com/301480043/diff/1/lib/ssl/ssl3con.c#newcode8895
lib/ssl/ssl3con.c:8895: idle_handshake)) {
On 2016/08/03 02:32:09, mt wrote:
> On 2016/08/01 12:02:59, ekr-webrtc wrote:
> > It seems unfortunate to use this test for 1.2. It's also a bit unfortunate
> that
> > it works. TLS13_IN_HS_STATE() should actually enforce two-way separatio
> 
> I could use wait_2nd_client_hello without using the 1.3 flag I suppose.  That
> would probably work.  WDYT?

Well, as noted above, I think it's a mistake to have extra states like this.
Let's resolve that point first.

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

https://codereview.appspot.com/301480043/diff/20001/external_tests/ssl_gtest/...
external_tests/ssl_gtest/ssl_hrr_unittest.cc:131: // Here the client receives a
HelloRetryRequest with a group that they already
Stylistic: I think we need to decide in terms of organization if this should be
instead in ssl_agent_unittest.cc. I'm not sure I feel strongly, but I think we
need to have a set of principles which we write down and then follow.

https://codereview.appspot.com/301480043/diff/20001/external_tests/ssl_gtest/...
external_tests/ssl_gtest/ssl_hrr_unittest.cc:144: };
This seems like an awkward compromise because it doesn't work correctly for
DTLS,but it's not totally canned. I would recommend actually constructing it or
having it just be byees

https://codereview.appspot.com/301480043/diff/20001/external_tests/ssl_gtest/...
external_tests/ssl_gtest/ssl_hrr_unittest.cc:152: ProcessMessage(record,
TlsAgent::STATE_ERROR,
I feel like thi

[mt, 2016-08-08 05:29:32]

To the big outstanding issue:

A SECItem* isn't sufficient, as I've said, since NSS doesn't generally allow for
the distinction between a SECItem that is empty and one that is NULL.  That's a
distinction reserved for the pointer to the SECItem usually.

That means that if this is going to work, we need an extra flag, for which I
think that the state is fine.  But it's not a big deal, I could just as easily
use a flag.

I will try out the flag alternative.  (I've uploaded amended tests for now.)

https://codereview.appspot.com/301480043/diff/1/external_tests/ssl_gtest/ssl_...
File external_tests/ssl_gtest/ssl_hrr_unittest.cc (right):

https://codereview.appspot.com/301480043/diff/1/external_tests/ssl_gtest/ssl_...
external_tests/ssl_gtest/ssl_hrr_unittest.cc:98: 
On 2016/08/03 16:15:19, ekr-webrtc wrote:
> On 2016/08/03 02:32:08, mt wrote:
> > On 2016/08/01 12:02:59, ekr-webrtc wrote:
> > > There's an easier test than this. Have a canned HRR and then just send it
> > twice
> > > in ssl_agent_unittest.cc.
> > 
> > This test actually results in a connection if the test fails.  I figured
that
> > that would be valuable.
> 
> I guess. In the agent tests you just check to see if you are in the connecting
> or failed states.

Yes, on looking at this, this is much easier - the replay is an expensive test
in terms of code, and a canned HRR is easy to generate (since I need it for
another test anyway).  I also parameterized the tests so that they run on both
DTLS and TLS.

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

https://codereview.appspot.com/301480043/diff/20001/external_tests/ssl_gtest/...
external_tests/ssl_gtest/ssl_hrr_unittest.cc:131: // Here the client receives a
HelloRetryRequest with a group that they already
On 2016/08/03 16:15:19, ekr-webrtc wrote:
> Stylistic: I think we need to decide in terms of organization if this should
be
> instead in ssl_agent_unittest.cc. I'm not sure I feel strongly, but I think we
> need to have a set of principles which we write down and then follow.

I don't mind either, but my preference is to separate tests functionally (what
the tests cover) rather than on a mechanical basis (how the tests are
constructed).

https://codereview.appspot.com/301480043/diff/20001/external_tests/ssl_gtest/...
external_tests/ssl_gtest/ssl_hrr_unittest.cc:144: };
On 2016/08/03 16:15:19, ekr-webrtc wrote:
> This seems like an awkward compromise because it doesn't work correctly for
> DTLS,but it's not totally canned. I would recommend actually constructing it
or
> having it just be byees

I've refactored this and it covers DTLS now.

[mt, 2016-08-08 06:35:45]

I've switched to using the flag.

I also added a test for retransmission of the second ClientHello.  I was a
little concerned that I would mess up the hashes there.  I had to move some code
to make it possible.  Note that adding a TLS 1.3-only test to
ssl_drop_unittest.cc would force that file from being run when 1.3 is disabled,
and I try not to do that Redhat.

[ekr-rietveld, 2016-08-09 20:23:29]

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

https://codereview.appspot.com/301480043/diff/60001/external_tests/ssl_gtest/...
external_tests/ssl_gtest/ssl_ecdh_unittest.cc:63: CheckKeys(ssl_kea_ecdh,
ssl_auth_rsa_sign, 384);
Maybe we should check that HRR is issued. Capture it with a filter?

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

https://codereview.appspot.com/301480043/diff/60001/external_tests/ssl_gtest/...
external_tests/ssl_gtest/ssl_hrr_unittest.cc:1: /* -*- Mode: C++; tab-width: 8;
indent-tabs-mode: nil; c-basic-offset: 2 -*- */
I think this needss a 0-RTT test

https://codereview.appspot.com/301480043/diff/60001/external_tests/ssl_gtest/...
external_tests/ssl_gtest/ssl_hrr_unittest.cc:104: SSL_LIBRARY_VERSION_TLS_1_3 >>
8, SSL_LIBRARY_VERSION_TLS_1_3 & 0xff,
Note: this will only work on Stream

https://codereview.appspot.com/301480043/diff/60001/external_tests/ssl_gtest/...
external_tests/ssl_gtest/ssl_hrr_unittest.cc:130:
SSL_ERROR_RX_UNEXPECTED_HELLO_RETRY_REQUEST);
This isn't the same client hello from the client's perspective. I assume that
the client provided P-384 in CH#2, so we just trap "two HRRs" before we trap
"group already there"?

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

https://codereview.appspot.com/301480043/diff/60001/external_tests/ssl_gtest/...
external_tests/ssl_gtest/tls_agent.cc:96: LOG("Changing state from " << state_
<< " to " << state);
LOGV?

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

https://codereview.appspot.com/301480043/diff/60001/external_tests/ssl_gtest/...
external_tests/ssl_gtest/tls_filter.cc:444: PacketFilter::Action
SelectiveDropFilter::Filter(const DataBuffer& input,
Assuming this is just a move

https://codereview.appspot.com/301480043/diff/60001/external_tests/ssl_gtest/...
File external_tests/ssl_gtest/tls_filter.h (right):

https://codereview.appspot.com/301480043/diff/60001/external_tests/ssl_gtest/...
external_tests/ssl_gtest/tls_filter.h:257: 
Assuming this is just a move.

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

https://codereview.appspot.com/301480043/diff/60001/lib/ssl/ssl3con.c#newcode...
lib/ssl/ssl3con.c:13750: ssl_DisableNonSuiteBGroups(ss);
Spurious merge? Rebase?

https://codereview.appspot.com/301480043/diff/60001/lib/ssl/sslsock.c
File lib/ssl/sslsock.c (right):

https://codereview.appspot.com/301480043/diff/60001/lib/ssl/sslsock.c#newcode176
lib/ssl/sslsock.c:176: { 29, FFGROUP(8192, 8192), PR_FALSE }
Does renumbering these cause a problem?

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

https://codereview.appspot.com/301480043/diff/60001/lib/ssl/tls13con.c#newcod...
lib/ssl/tls13con.c:1013: rv = ssl3_NegotiateCipherSuite(ss, suites,
!ss->ssl3.hs.helloRetry);
This seems like a merge conflict. We removed this extra argument.

https://codereview.appspot.com/301480043/diff/60001/lib/ssl/tls13con.c#newcod...
lib/ssl/tls13con.c:1021: ss->ssl3.hs.cipher_suite != previousCipherSuite) {
Note: this won't work in stateless.

https://codereview.appspot.com/301480043/diff/60001/lib/ssl/tls13con.c#newcod...
lib/ssl/tls13con.c:1092: rv = tls13_HandleClientKeyShare(ss, &retry);
Maybe name this "shouldRetry"

https://codereview.appspot.com/301480043/diff/60001/lib/ssl/tls13con.c#newcod...
lib/ssl/tls13con.c:1097: PORT_Assert(ss->ssl3.hs.helloRetry);
I would send HRR here.

https://codereview.appspot.com/301480043/diff/60001/lib/ssl/tls13con.c#newcod...
lib/ssl/tls13con.c:1270: return tls13_SendHelloRetryRequest(ss, selectedGroup);
I wonder if this would be better in the calling code.

[mt, 2016-08-12 01:23:03]

Just some replies to stuff I failed to look at while I was wrestling with 0-RTT.

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

https://codereview.appspot.com/301480043/diff/60001/external_tests/ssl_gtest/...
external_tests/ssl_gtest/ssl_ecdh_unittest.cc:63: CheckKeys(ssl_kea_ecdh,
ssl_auth_rsa_sign, 384);
On 2016/08/09 20:23:28, ekr-webrtc wrote:
> Maybe we should check that HRR is issued. Capture it with a filter?

Done.

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

https://codereview.appspot.com/301480043/diff/60001/external_tests/ssl_gtest/...
external_tests/ssl_gtest/ssl_hrr_unittest.cc:1: /* -*- Mode: C++; tab-width: 8;
indent-tabs-mode: nil; c-basic-offset: 2 -*- */
On 2016/08/09 20:23:28, ekr-webrtc wrote:
> I think this needss a 0-RTT test

Done.

https://codereview.appspot.com/301480043/diff/60001/external_tests/ssl_gtest/...
external_tests/ssl_gtest/ssl_hrr_unittest.cc:1: /* -*- Mode: C++; tab-width: 8;
indent-tabs-mode: nil; c-basic-offset: 2 -*- */
On 2016/08/09 20:23:28, ekr-webrtc wrote:
> I think this needss a 0-RTT test

Done.

https://codereview.appspot.com/301480043/diff/60001/external_tests/ssl_gtest/...
external_tests/ssl_gtest/ssl_hrr_unittest.cc:104: SSL_LIBRARY_VERSION_TLS_1_3 >>
8, SSL_LIBRARY_VERSION_TLS_1_3 & 0xff,
On 2016/08/09 20:23:28, ekr-webrtc wrote:
> Note: this will only work on Stream

Done.

https://codereview.appspot.com/301480043/diff/60001/external_tests/ssl_gtest/...
external_tests/ssl_gtest/ssl_hrr_unittest.cc:130:
SSL_ERROR_RX_UNEXPECTED_HELLO_RETRY_REQUEST);
On 2016/08/09 20:23:28, ekr-webrtc wrote:
> This isn't the same client hello from the client's perspective. I assume that
> the client provided P-384 in CH#2, so we just trap "two HRRs" before we trap
> "group already there"?

Yes, the point with including P-384 in the HRR is to ensure that the first is
seen as legitimate.  I guess that I could try P-521 here to make it clear that
it's not the duplicate curve that's the problem with the second HRR.

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

https://codereview.appspot.com/301480043/diff/60001/external_tests/ssl_gtest/...
external_tests/ssl_gtest/tls_agent.cc:96: LOG("Changing state from " << state_
<< " to " << state);
On 2016/08/09 20:23:28, ekr-webrtc wrote:
> LOGV?

Nah, this one is pretty important to log.  BTW, I just moved this here so that I
can use the operator<< for State.

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

https://codereview.appspot.com/301480043/diff/60001/external_tests/ssl_gtest/...
external_tests/ssl_gtest/tls_filter.cc:444: PacketFilter::Action
SelectiveDropFilter::Filter(const DataBuffer& input,
On 2016/08/09 20:23:28, ekr-webrtc wrote:
> Assuming this is just a move

Yep.

https://codereview.appspot.com/301480043/diff/60001/external_tests/ssl_gtest/...
File external_tests/ssl_gtest/tls_filter.h (right):

https://codereview.appspot.com/301480043/diff/60001/external_tests/ssl_gtest/...
external_tests/ssl_gtest/tls_filter.h:257: 
On 2016/08/09 20:23:28, ekr-webrtc wrote:
> Assuming this is just a move.

Yep.

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

https://codereview.appspot.com/301480043/diff/60001/lib/ssl/ssl3con.c#newcode...
lib/ssl/ssl3con.c:13750: ssl_DisableNonSuiteBGroups(ss);
On 2016/08/09 20:23:28, ekr-webrtc wrote:
> Spurious merge? Rebase?

I'm not seeing any change in this diff, so probably.

https://codereview.appspot.com/301480043/diff/60001/lib/ssl/sslsock.c
File lib/ssl/sslsock.c (right):

https://codereview.appspot.com/301480043/diff/60001/lib/ssl/sslsock.c#newcode176
lib/ssl/sslsock.c:176: { 29, FFGROUP(8192, 8192), PR_FALSE }
On 2016/08/09 20:23:28, ekr-webrtc wrote:
> Does renumbering these cause a problem?

No, the index is only used internally.

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

https://codereview.appspot.com/301480043/diff/60001/lib/ssl/tls13con.c#newcod...
lib/ssl/tls13con.c:1013: rv = ssl3_NegotiateCipherSuite(ss, suites,
!ss->ssl3.hs.helloRetry);
On 2016/08/09 20:23:28, ekr-webrtc wrote:
> This seems like a merge conflict. We removed this extra argument.

I just added the argument in this patch (necessary to avoid reinitializing
hashes when they aren't supposed to be).

https://codereview.appspot.com/301480043/diff/60001/lib/ssl/tls13con.c#newcod...
lib/ssl/tls13con.c:1021: ss->ssl3.hs.cipher_suite != previousCipherSuite) {
On 2016/08/09 20:23:28, ekr-webrtc wrote:
> Note: this won't work in stateless.

Correct.  Unless we save the previous cipher suite, but we probably won't
bother.

https://codereview.appspot.com/301480043/diff/60001/lib/ssl/tls13con.c#newcod...
lib/ssl/tls13con.c:1092: rv = tls13_HandleClientKeyShare(ss, &retry);
On 2016/08/09 20:23:28, ekr-webrtc wrote:
> Maybe name this "shouldRetry"

Done.

https://codereview.appspot.com/301480043/diff/60001/lib/ssl/tls13con.c#newcod...
lib/ssl/tls13con.c:1097: PORT_Assert(ss->ssl3.hs.helloRetry);
On 2016/08/09 20:23:28, ekr-webrtc wrote:
> I would send HRR here.

I wanted to also, but the HRR sender needs to be passed the selected group and I
didn't want to pass it as an outparam.

https://codereview.appspot.com/301480043/diff/60001/lib/ssl/tls13con.c#newcod...
lib/ssl/tls13con.c:1270: return tls13_SendHelloRetryRequest(ss, selectedGroup);
On 2016/08/09 20:23:29, ekr-webrtc wrote:
> I wonder if this would be better in the calling code.

The HRR send needs to know what the selected group was.

[ekr-rietveld, 2016-08-23 00:07:37]

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

https://codereview.appspot.com/301480043/diff/60001/external_tests/ssl_gtest/...
external_tests/ssl_gtest/ssl_hrr_unittest.cc:130:
SSL_ERROR_RX_UNEXPECTED_HELLO_RETRY_REQUEST);
On 2016/08/12 01:23:02, mt wrote:
> On 2016/08/09 20:23:28, ekr-webrtc wrote:
> > This isn't the same client hello from the client's perspective. I assume
that
> > the client provided P-384 in CH#2, so we just trap "two HRRs" before we trap
> > "group already there"?
> 
> Yes, the point with including P-384 in the HRR is to ensure that the first is
> seen as legitimate.  I guess that I could try P-521 here to make it clear that
> it's not the duplicate curve that's the problem with the second HRR.

I think a comment would be fine.

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

https://codereview.appspot.com/301480043/diff/80001/external_tests/ssl_gtest/...
external_tests/ssl_gtest/ssl_0rtt_unittest.cc:181: CheckAlpn("b");
I am assuming that these just moved around. LMK if they are actual changes.

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

https://codereview.appspot.com/301480043/diff/80001/external_tests/ssl_gtest/...
external_tests/ssl_gtest/ssl_ecdh_unittest.cc:59: TEST_P(TlsConnectGeneric,
ConnectEcdheP384Server) {
I would probably scope this test to 1.3, but whatevs.

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

https://codereview.appspot.com/301480043/diff/80001/external_tests/ssl_gtest/...
external_tests/ssl_gtest/ssl_hrr_unittest.cc:21: const char *k0RttData = "Goat
Shed";
Maybe we should have standardized k0RttData in an extern? I suggest "Such is
life."

https://codereview.appspot.com/301480043/diff/80001/external_tests/ssl_gtest/...
external_tests/ssl_gtest/ssl_hrr_unittest.cc:55: EXPECT_LT(0U,
capture_early_data->extension().len());
Shouldn't this actually be 0? I would expect the client to not send EDI.

https://codereview.appspot.com/301480043/diff/80001/external_tests/ssl_gtest/...
external_tests/ssl_gtest/ssl_hrr_unittest.cc:138: SSL_LIBRARY_VERSION_TLS_1_3 &
0xff,
Can we abstract this somehow so you don't need to repeat it?

https://codereview.appspot.com/301480043/diff/80001/external_tests/ssl_gtest/...
external_tests/ssl_gtest/ssl_hrr_unittest.cc:148: PR_ARRAY_SIZE(canned_hrr),
&hrr, seq_num);
sizeof() would be better here.

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

https://codereview.appspot.com/301480043/diff/80001/external_tests/ssl_gtest/...
external_tests/ssl_gtest/tls_connect.cc:353: EXPECT_NE(RESUME_BOTH, expected);
Interesting. I hadn't thought through this case. I tend to think the server
shouldn't be incrementing its stats in this case, because the session is resumed
once and not twice.

https://codereview.appspot.com/301480043/diff/80001/external_tests/ssl_gtest/...
external_tests/ssl_gtest/tls_connect.cc:363: // Note: hch == server counter; hsh
== client counter.
Good one. these names are kind of confusing.

https://codereview.appspot.com/301480043/diff/80001/external_tests/ssl_gtest/...
external_tests/ssl_gtest/tls_connect.cc:371: if (expected) {
Maybe a comment that NONE == 0

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

https://codereview.appspot.com/301480043/diff/80001/lib/ssl/ssl3con.c#newcode...
lib/ssl/ssl3con.c:3872: return tls13_HandleEndOfEarlyData(ss);
I wonder if we should send an alert here.

https://codereview.appspot.com/301480043/diff/80001/lib/ssl/ssl3con.c#newcode...
lib/ssl/ssl3con.c:5548: CHTYPE(renegotiation);
Comment here that these last two are DTLS and TLS <= 1.2 respectively

https://codereview.appspot.com/301480043/diff/80001/lib/ssl/ssl3con.c#newcode...
lib/ssl/ssl3con.c:5593: type == client_hello_retransmit);
IMPORTANT: Shouldn't retransmit be reinitializing the state?

https://codereview.appspot.com/301480043/diff/80001/lib/ssl/ssl3con.c#newcode...
lib/ssl/ssl3con.c:6100: {
Is the intent here to force everything into a single network datagram?

https://codereview.appspot.com/301480043/diff/80001/lib/ssl/ssl3con.c#newcode...
lib/ssl/ssl3con.c:13452: return dtls_MaybeRetransmitHandshake(ss, cText);
Wouldn't it make more sense to put this in dtls_MaybeRetransmit())

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

https://codereview.appspot.com/301480043/diff/80001/lib/ssl/ssl3ext.c#newcode...
lib/ssl/ssl3ext.c:3748: ss->ssl3.hs.zeroRttState = ssl_0rtt_sent;
Why not just re-make the null spec, now that we garbage collect these.

https://codereview.appspot.com/301480043/diff/80001/lib/ssl/sslimpl.h
File lib/ssl/sslimpl.h (right):

https://codereview.appspot.com/301480043/diff/80001/lib/ssl/sslimpl.h#newcode765
lib/ssl/sslimpl.h:765: } sslZeroRttState;
I would instead have a second state bit indicating that we are ignoring mode and
what to wait for (maybe even a function pointer). Then this can just consist of
"none", "sent" "ignored", "accepted"

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

https://codereview.appspot.com/301480043/diff/80001/lib/ssl/sslsecur.c#newcod...
lib/ssl/sslsecur.c:935: ss->ssl3.hs.zeroRttState == ssl_0rtt_accept;
Comment here would help

https://codereview.appspot.com/301480043/diff/80001/lib/ssl/sslsecur.c#newcod...
lib/ssl/sslsecur.c:968: ss->ssl3.hs.zeroRttState == ssl_0rtt_accept));
BUG?: This assert changes the behavior for TLS 1.2, which seems to allow
server-side false start. Probably during resumption. I have not investigated but
can if you need me to

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

https://codereview.appspot.com/301480043/diff/80001/lib/ssl/sslsock.c#newcode1
lib/ssl/sslsock.c:1: /* -*- Mode: C; tab-width: 8; indent-tabs-mode: nil;
c-basic-offset: 4 -*- */
I am assuming all changes in this file are rebase changes.

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

https://codereview.appspot.com/301480043/diff/80001/lib/ssl/tls13con.c#newcod...
lib/ssl/tls13con.c:988: if (ss->ssl3.hs.zeroRttState == ssl_0rtt_none) {
This comment is confusing because it doesn't tell us how this got set.

https://codereview.appspot.com/301480043/diff/80001/lib/ssl/tls13con.c#newcod...
lib/ssl/tls13con.c:999: }
This doesn't look like junk. It looks like we silently ignore. I would actually
generate an error.

https://codereview.appspot.com/301480043/diff/80001/lib/ssl/tls13con.c#newcod...
lib/ssl/tls13con.c:1016: ss->ssl3.hs.zeroRttState = ssl_0rtt_accept;
I see that this is where we check ticket age.

https://codereview.appspot.com/301480043/diff/80001/lib/ssl/tls13con.c#newcod...
lib/ssl/tls13con.c:1176: TLS13_SET_HS_STATE(ss, wait_encrypted_extensions);
This needs rebase for -14. Luckily, it makes things simpler.

https://codereview.appspot.com/301480043/diff/80001/lib/ssl/tls13con.c#newcod...
lib/ssl/tls13con.c:1254: ss->ssl3.hs.zeroRttState = ssl_0rtt_ignore_hrr;
A comment about how this happened would be nice.

https://codereview.appspot.com/301480043/diff/80001/lib/ssl/tls13con.c#newcod...
lib/ssl/tls13con.c:1260: ss->ssl3.hs.recvMessageSeq += 2;
This only applies to DTLS, right

https://codereview.appspot.com/301480043/diff/80001/lib/ssl/tls13con.c#newcod...
lib/ssl/tls13con.c:1338: PORT_Assert(ss->ssl3.hs.zeroRttState != ssl_0rtt_sent);
This is because we should hve resolved in handle client hello?

https://codereview.appspot.com/301480043/diff/80001/lib/ssl/tls13con.c#newcod...
lib/ssl/tls13con.c:1470: /* Fool me once, shame on you; fool me twice... */
"fool me once, shame on you... can't get fooled again"

https://www.youtube.com/watch?v=eKgPY1adc0A

https://codereview.appspot.com/301480043/diff/80001/lib/ssl/tls13con.c#newcod...
lib/ssl/tls13con.c:1485: }
I suggest instead just not sending end_of_early_data. It's not needed.

https://codereview.appspot.com/301480043/diff/80001/lib/ssl/tls13con.c#newcod...
lib/ssl/tls13con.c:1535: }
This code seems to appear multiple times. Let's just write a
ssl3_FindEphemeralKeyForGroup() fxn.

https://codereview.appspot.com/301480043/diff/80001/lib/ssl/tls13con.c#newcod...
lib/ssl/tls13con.c:1766: ss->opt.requestCertificate ? wait_client_cert
Maybe for future, but it's not possible to do client auth with 0-RTT or PSK any
more.

https://codereview.appspot.com/301480043/diff/80001/lib/ssl/tls13con.c#newcod...
lib/ssl/tls13con.c:1790: return SECFailure;  /* Error code already set. */
This needs to be updated because I moved it to EE.

https://codereview.appspot.com/301480043/diff/80001/lib/ssl/tls13con.c#newcod...
lib/ssl/tls13con.c:2642: ss->ssl3.hs.zeroRttState == ssl_0rtt_sent) &&
IMPORTANT: Update for -14/

https://codereview.appspot.com/301480043/diff/80001/lib/ssl/tls13con.c#newcod...
lib/ssl/tls13con.c:3880: }
See above comments about not needing to allocate null spec.

https://codereview.appspot.com/301480043/diff/80001/lib/ssl/tls13con.c#newcod...
lib/ssl/tls13con.c:3894: return SECSuccess;
Why are you setting NULL here if the other side accepted? You just need to
remove it later.

https://codereview.appspot.com/301480043/diff/80001/lib/ssl/tls13con.c#newcod...
lib/ssl/tls13con.c:3912: tls13_SetNullCipherSpec(ss, &ss->ssl3.crSpec);
See here too. 

I think we should move to only doing e_o_ed when 0-RTT is accepted.

[mt, 2016-08-23 07:22:11]

I think that we should sort out the epoch changes.  I have a patch now that
works, but it resets the epoch back to zero after 0-RTT.  That means futzing
around with the null spec in ways that I think are unpleasant, or saving it (as
I still do).

I think that the epochs that Hannes suggests are much easier to implement.  The
current arrangement is hugely fragile; I spent a lot of time debugging even the
simple changes you asked for.

If you agree that Hannes' proposed epochs are also better, then I will modify
the patch accordingly.  I'll upload what I have, then look at rebasing, since I
haven't done the -14 changes yet (which should be much easier).

Incidentally, I found that it was easier to move the eoed alert sending to the
client's second flight.  

However, I discovered that I can't easily put it into the same packet as the
rest of the flight because staging records in buffer isn't compatible with DTLS,
which uses ss->sec.ci.sendBuf, and a staged alert is buffered in ss->pendingBuf.
 DTLS asserts that pendingBuf is empty.  My inclination is to construct the eoed
alert using the handshake code.  I think that it deserves the same sort of
treatment as a handshake message.

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

https://codereview.appspot.com/301480043/diff/60001/external_tests/ssl_gtest/...
external_tests/ssl_gtest/ssl_hrr_unittest.cc:130:
SSL_ERROR_RX_UNEXPECTED_HELLO_RETRY_REQUEST);
On 2016/08/23 00:07:35, ekr-rietveld wrote:
> On 2016/08/12 01:23:02, mt wrote:
> > On 2016/08/09 20:23:28, ekr-webrtc wrote:
> > > This isn't the same client hello from the client's perspective. I assume
> that
> > > the client provided P-384 in CH#2, so we just trap "two HRRs" before we
trap
> > > "group already there"?
> > 
> > Yes, the point with including P-384 in the HRR is to ensure that the first
is
> > seen as legitimate.  I guess that I could try P-521 here to make it clear
that
> > it's not the duplicate curve that's the problem with the second HRR.
> 
> I think a comment would be fine.

Done.

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

https://codereview.appspot.com/301480043/diff/80001/external_tests/ssl_gtest/...
external_tests/ssl_gtest/ssl_0rtt_unittest.cc:25: TEST_P(TlsConnectTls13,
ZeroRtt) {
This is an entirely new test case.  I figured that it might pay to test the
simple success path.

https://codereview.appspot.com/301480043/diff/80001/external_tests/ssl_gtest/...
external_tests/ssl_gtest/ssl_0rtt_unittest.cc:181: CheckAlpn("b");
On 2016/08/23 00:07:35, ekr-rietveld wrote:
> I am assuming that these just moved around. LMK if they are actual changes.

Same tests as ever.  Note that I'm now testing for TLS 1.3 in both stream and
datagram though.

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

https://codereview.appspot.com/301480043/diff/80001/external_tests/ssl_gtest/...
external_tests/ssl_gtest/ssl_ecdh_unittest.cc:59: TEST_P(TlsConnectGeneric,
ConnectEcdheP384Server) {
On 2016/08/23 00:07:35, ekr-rietveld wrote:
> I would probably scope this test to 1.3, but whatevs.

I also wanted to test that the server picks P-384 if that is all that it is
configured to do.

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

https://codereview.appspot.com/301480043/diff/80001/external_tests/ssl_gtest/...
external_tests/ssl_gtest/ssl_hrr_unittest.cc:21: const char *k0RttData = "Goat
Shed";
On 2016/08/23 00:07:35, ekr-rietveld wrote:
> Maybe we should have standardized k0RttData in an extern? I suggest "Such is
> life."

Done.

https://codereview.appspot.com/301480043/diff/80001/external_tests/ssl_gtest/...
external_tests/ssl_gtest/ssl_hrr_unittest.cc:55: EXPECT_LT(0U,
capture_early_data->extension().len());
On 2016/08/23 00:07:35, ekr-rietveld wrote:
> Shouldn't this actually be 0? I would expect the client to not send EDI.

Thankfully only a test bug.  I forgot to reset the packet filter.  When the
extension is absent from the second ClientHello, the old value in the capture
was remembered.

https://codereview.appspot.com/301480043/diff/80001/external_tests/ssl_gtest/...
external_tests/ssl_gtest/ssl_hrr_unittest.cc:138: SSL_LIBRARY_VERSION_TLS_1_3 &
0xff,
On 2016/08/23 00:07:35, ekr-rietveld wrote:
> Can we abstract this somehow so you don't need to repeat it?

Probably, but I decided that it wasn't worth it.  It's 8 bytes.

https://codereview.appspot.com/301480043/diff/80001/external_tests/ssl_gtest/...
external_tests/ssl_gtest/ssl_hrr_unittest.cc:148: PR_ARRAY_SIZE(canned_hrr),
&hrr, seq_num);
On 2016/08/23 00:07:35, ekr-rietveld wrote:
> sizeof() would be better here.

Done.

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

https://codereview.appspot.com/301480043/diff/80001/external_tests/ssl_gtest/...
external_tests/ssl_gtest/tls_connect.cc:353: EXPECT_NE(RESUME_BOTH, expected);
On 2016/08/23 00:07:36, ekr-rietveld wrote:
> Interesting. I hadn't thought through this case. I tend to think the server
> shouldn't be incrementing its stats in this case, because the session is
resumed
> once and not twice.

I'll see what I can do.

https://codereview.appspot.com/301480043/diff/80001/external_tests/ssl_gtest/...
external_tests/ssl_gtest/tls_connect.cc:363: // Note: hch == server counter; hsh
== client counter.
On 2016/08/23 00:07:36, ekr-rietveld wrote:
> Good one. these names are kind of confusing.

Acknowledged.

https://codereview.appspot.com/301480043/diff/80001/external_tests/ssl_gtest/...
external_tests/ssl_gtest/tls_connect.cc:371: if (expected) {
On 2016/08/23 00:07:36, ekr-rietveld wrote:
> Maybe a comment that NONE == 0

code > comment; (expected == RESUME_NONE)

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

https://codereview.appspot.com/301480043/diff/80001/lib/ssl/ssl3con.c#newcode...
lib/ssl/ssl3con.c:3872: return tls13_HandleEndOfEarlyData(ss);
On 2016/08/23 00:07:36, ekr-rietveld wrote:
> I wonder if we should send an alert here.

You mean in the case where we receive a non-fatal alert?  Maybe that's best for
another patch.

https://codereview.appspot.com/301480043/diff/80001/lib/ssl/ssl3con.c#newcode...
lib/ssl/ssl3con.c:5548: CHTYPE(renegotiation);
On 2016/08/23 00:07:36, ekr-rietveld wrote:
> Comment here that these last two are DTLS and TLS <= 1.2 respectively

I was relying on the comment below, but can do.

https://codereview.appspot.com/301480043/diff/80001/lib/ssl/ssl3con.c#newcode...
lib/ssl/ssl3con.c:5593: type == client_hello_retransmit);
On 2016/08/23 00:07:36, ekr-rietveld wrote:
> IMPORTANT: Shouldn't retransmit be reinitializing the state?

It does.

Actually, we shouldn't hit this code when retransmitting the second ClientHello
because that just pulls from the saved buffer.  I should remove the retransmit
branch of the assert.

https://codereview.appspot.com/301480043/diff/80001/lib/ssl/ssl3con.c#newcode...
lib/ssl/ssl3con.c:6100: {
On 2016/08/23 00:07:36, ekr-rietveld wrote:
> Is the intent here to force everything into a single network datagram?

Yes.  I will comment to that effect.  It also prevents a bug where we would
flush the ClientHello, then resend it with the E-E and Fin when they are
flushed.  Apparently our code assumes one flush per flight.

https://codereview.appspot.com/301480043/diff/80001/lib/ssl/ssl3con.c#newcode...
lib/ssl/ssl3con.c:13452: return dtls_MaybeRetransmitHandshake(ss, cText);
On 2016/08/23 00:07:36, ekr-rietveld wrote:
> Wouldn't it make more sense to put this in dtls_MaybeRetransmit())

Done.

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

https://codereview.appspot.com/301480043/diff/80001/lib/ssl/ssl3ext.c#newcode...
lib/ssl/ssl3ext.c:3748: ss->ssl3.hs.zeroRttState = ssl_0rtt_sent;
On 2016/08/23 00:07:36, ekr-rietveld wrote:
> Why not just re-make the null spec, now that we garbage collect these.

The problem here is that on the read side you have to remember that you received
the first ClientHello.  That makes this tricky.

https://codereview.appspot.com/301480043/diff/80001/lib/ssl/sslimpl.h
File lib/ssl/sslimpl.h (right):

https://codereview.appspot.com/301480043/diff/80001/lib/ssl/sslimpl.h#newcode765
lib/ssl/sslimpl.h:765: } sslZeroRttState;
On 2016/08/23 00:07:36, ekr-rietveld wrote:
> I would instead have a second state bit indicating that we are ignoring mode
and
> what to wait for (maybe even a function pointer). Then this can just consist
of
> "none", "sent" "ignored", "accepted"

Done.

The function pointer doesn't help that much, since the code to ignore needs to
be at different points.

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

https://codereview.appspot.com/301480043/diff/80001/lib/ssl/sslsecur.c#newcod...
lib/ssl/sslsecur.c:935: ss->ssl3.hs.zeroRttState == ssl_0rtt_accept;
On 2016/08/23 00:07:36, ekr-rietveld wrote:
> Comment here would help

Done.

https://codereview.appspot.com/301480043/diff/80001/lib/ssl/sslsecur.c#newcod...
lib/ssl/sslsecur.c:968: ss->ssl3.hs.zeroRttState == ssl_0rtt_accept));
On 2016/08/23 00:07:36, ekr-rietveld wrote:
> BUG?: This assert changes the behavior for TLS 1.2, which seems to allow
> server-side false start. Probably during resumption. I have not investigated
but
> can if you need me to

I'm pretty familiar with false start, which is only ever enabled on the client. 
I double-checked.

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

https://codereview.appspot.com/301480043/diff/80001/lib/ssl/sslsock.c#newcode1
lib/ssl/sslsock.c:1: /* -*- Mode: C; tab-width: 8; indent-tabs-mode: nil;
c-basic-offset: 4 -*- */
On 2016/08/23 00:07:36, ekr-rietveld wrote:
> I am assuming all changes in this file are rebase changes.

Yep.

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

https://codereview.appspot.com/301480043/diff/80001/lib/ssl/tls13con.c#newcod...
lib/ssl/tls13con.c:988: if (ss->ssl3.hs.zeroRttState == ssl_0rtt_none) {
On 2016/08/23 00:07:37, ekr-rietveld wrote:
> This comment is confusing because it doesn't tell us how this got set.

Done.

https://codereview.appspot.com/301480043/diff/80001/lib/ssl/tls13con.c#newcod...
lib/ssl/tls13con.c:999: }
On 2016/08/23 00:07:37, ekr-rietveld wrote:
> This doesn't look like junk. It looks like we silently ignore. I would
actually
> generate an error.

We silently ignore DTLS messages that don't decrypt, but we will fail if we
receive a second set of 0-RTT messages.

https://codereview.appspot.com/301480043/diff/80001/lib/ssl/tls13con.c#newcod...
lib/ssl/tls13con.c:1016: ss->ssl3.hs.zeroRttState = ssl_0rtt_accept;
On 2016/08/23 00:07:37, ekr-rietveld wrote:
> I see that this is where we check ticket age.

Acknowledged.

https://codereview.appspot.com/301480043/diff/80001/lib/ssl/tls13con.c#newcod...
lib/ssl/tls13con.c:1176: TLS13_SET_HS_STATE(ss, wait_encrypted_extensions);
On 2016/08/23 00:07:36, ekr-rietveld wrote:
> This needs rebase for -14. Luckily, it makes things simpler.

Acknowledged.

https://codereview.appspot.com/301480043/diff/80001/lib/ssl/tls13con.c#newcod...
lib/ssl/tls13con.c:1260: ss->ssl3.hs.recvMessageSeq += 2;
On 2016/08/23 00:07:36, ekr-rietveld wrote:
> This only applies to DTLS, right

Yes.  Tweaked.

https://codereview.appspot.com/301480043/diff/80001/lib/ssl/tls13con.c#newcod...
lib/ssl/tls13con.c:1338: PORT_Assert(ss->ssl3.hs.zeroRttState != ssl_0rtt_sent);
On 2016/08/23 00:07:37, ekr-rietveld wrote:
> This is because we should hve resolved in handle client hello?

Yes.  _sent should be very much temporary for the server.

https://codereview.appspot.com/301480043/diff/80001/lib/ssl/tls13con.c#newcod...
lib/ssl/tls13con.c:1470: /* Fool me once, shame on you; fool me twice... */
On 2016/08/23 00:07:37, ekr-rietveld wrote:
> "fool me once, shame on you... can't get fooled again"
> 
> https://www.youtube.com/watch?v=eKgPY1adc0A

Heh.

https://codereview.appspot.com/301480043/diff/80001/lib/ssl/tls13con.c#newcod...
lib/ssl/tls13con.c:1485: }
On 2016/08/23 00:07:37, ekr-rietveld wrote:
> I suggest instead just not sending end_of_early_data. It's not needed.

Done.  Note that this forces servers to trial decrypt, even when they
technically don't have to.  I'm undecided whether this is a good idea or not.

https://codereview.appspot.com/301480043/diff/80001/lib/ssl/tls13con.c#newcod...
lib/ssl/tls13con.c:1535: }
On 2016/08/23 00:07:37, ekr-rietveld wrote:
> This code seems to appear multiple times. Let's just write a
> ssl3_FindEphemeralKeyForGroup() fxn.

Didn't have to: ssl_LookupEphemeralKeyPair(sslSocket *ss, const namedGroupDef
*groupDef)  Thanks for the nudge.

https://codereview.appspot.com/301480043/diff/80001/lib/ssl/tls13con.c#newcod...
lib/ssl/tls13con.c:1766: ss->opt.requestCertificate ? wait_client_cert
On 2016/08/23 00:07:37, ekr-rietveld wrote:
> Maybe for future, but it's not possible to do client auth with 0-RTT or PSK
any
> more.

Acknowledged.

https://codereview.appspot.com/301480043/diff/80001/lib/ssl/tls13con.c#newcod...
lib/ssl/tls13con.c:1790: return SECFailure;  /* Error code already set. */
On 2016/08/23 00:07:36, ekr-rietveld wrote:
> This needs to be updated because I moved it to EE.

Acknowledged.

https://codereview.appspot.com/301480043/diff/80001/lib/ssl/tls13con.c#newcod...
lib/ssl/tls13con.c:2642: ss->ssl3.hs.zeroRttState == ssl_0rtt_sent) &&
On 2016/08/23 00:07:37, ekr-rietveld wrote:
> IMPORTANT: Update for -14/

Acknowledged.

https://codereview.appspot.com/301480043/diff/80001/lib/ssl/tls13con.c#newcod...
lib/ssl/tls13con.c:3880: }
On 2016/08/23 00:07:36, ekr-rietveld wrote:
> See above comments about not needing to allocate null spec.

See my response :)

https://codereview.appspot.com/301480043/diff/80001/lib/ssl/tls13con.c#newcod...
lib/ssl/tls13con.c:3894: return SECSuccess;
On 2016/08/23 00:07:37, ekr-rietveld wrote:
> Why are you setting NULL here if the other side accepted? You just need to
> remove it later.

Removed.

https://codereview.appspot.com/301480043/diff/80001/lib/ssl/tls13con.c#newcod...
lib/ssl/tls13con.c:3912: tls13_SetNullCipherSpec(ss, &ss->ssl3.crSpec);
On 2016/08/23 00:07:37, ekr-rietveld wrote:
> See here too. 
> 
> I think we should move to only doing e_o_ed when 0-RTT is accepted.

Done.

[ekr-rietveld, 2016-08-23 16:58:32]

MT, I don't think this picked up the claimed changes.

In any case, I think we should land the TLS code, whatever of the DTLS code
seems sound, and declare DTLS not-working for now. That will get what we need
done and let us move on.

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

https://codereview.appspot.com/301480043/diff/80001/external_tests/ssl_gtest/...
external_tests/ssl_gtest/ssl_0rtt_unittest.cc:25: TEST_P(TlsConnectTls13,
ZeroRtt) {
On 2016/08/23 07:22:09, mt wrote:
> This is an entirely new test case.  I figured that it might pay to test the
> simple success path.

Huh. There used to be a test for that.

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

https://codereview.appspot.com/301480043/diff/80001/external_tests/ssl_gtest/...
external_tests/ssl_gtest/ssl_hrr_unittest.cc:148: PR_ARRAY_SIZE(canned_hrr),
&hrr, seq_num);
On 2016/08/23 07:22:09, mt wrote:
> On 2016/08/23 00:07:35, ekr-rietveld wrote:
> > sizeof() would be better here.
> 
> Done.

Doesn't look done.

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

https://codereview.appspot.com/301480043/diff/80001/external_tests/ssl_gtest/...
external_tests/ssl_gtest/tls_connect.cc:371: if (expected) {
On 2016/08/23 07:22:09, mt wrote:
> On 2016/08/23 00:07:36, ekr-rietveld wrote:
> > Maybe a comment that NONE == 0
> 
> code > comment; (expected == RESUME_NONE)

Nothing seems to have changed here either.

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

https://codereview.appspot.com/301480043/diff/80001/lib/ssl/ssl3con.c#newcode...
lib/ssl/ssl3con.c:1695: ss->sec.cipherType = cipher;
I believe this is rebase but please confirm.

https://codereview.appspot.com/301480043/diff/80001/lib/ssl/ssl3con.c#newcode...
lib/ssl/ssl3con.c:5548: CHTYPE(renegotiation);
On 2016/08/23 07:22:09, mt wrote:
> On 2016/08/23 00:07:36, ekr-rietveld wrote:
> > Comment here that these last two are DTLS and TLS <= 1.2 respectively
> 
> I was relying on the comment below, but can do.

This also doesn't seem to have changed.

[mt, 2016-08-24 01:16:57]

Clean version of the same code available at:
https://codereview.appspot.com/301660044