First cut of cipherspec queues

First cut of cipherspec queues

[ekr-rietveld, 2016-03-24 19:38:40]

https://codereview.appspot.com/289590043/diff/1/external_tests/ssl_gtest/libs...
File external_tests/ssl_gtest/libssl_internals.c (right):

https://codereview.appspot.com/289590043/diff/1/external_tests/ssl_gtest/libs...
external_tests/ssl_gtest/libssl_internals.c:105: 
WS

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

https://codereview.appspot.com/289590043/diff/1/lib/ssl/dtlscon.c#newcode937
lib/ssl/dtlscon.c:937:
dtls_FreeHandshakeMessages(&ss->ssl3.hs.lastMessageFlight);
Actually do both for TLS < 1.3

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

https://codereview.appspot.com/289590043/diff/1/lib/ssl/tls13con.c#newcode82
lib/ssl/tls13con.c:82: 
WS

https://codereview.appspot.com/289590043/diff/1/lib/ssl/tls13con.c#newcode1307
lib/ssl/tls13con.c:1307: PORT_Assert(spec->refCt > 0);
Move this to release

https://codereview.appspot.com/289590043/diff/1/lib/ssl/tls13con.c#newcode1509
lib/ssl/tls13con.c:1509: 
WS

https://codereview.appspot.com/289590043/diff/1/lib/ssl/tls13con.c#newcode1661
lib/ssl/tls13con.c:1661: pwSpec = ss->ssl3.pwSpec;
needed?

https://codereview.appspot.com/289590043/diff/1/lib/ssl/tls13con.c#newcode2753
lib/ssl/tls13con.c:2753: 
extra

[ekr-rietveld, 2016-03-24 20:26:18]

https://codereview.appspot.com/289590043/diff/1/external_tests/ssl_gtest/libs...
File external_tests/ssl_gtest/libssl_internals.c (right):

https://codereview.appspot.com/289590043/diff/1/external_tests/ssl_gtest/libs...
external_tests/ssl_gtest/libssl_internals.c:105: 
On 2016/03/24 19:38:40, ekr-webrtc wrote:
> WS

Done.

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

https://codereview.appspot.com/289590043/diff/1/lib/ssl/dtlscon.c#newcode937
lib/ssl/dtlscon.c:937:
dtls_FreeHandshakeMessages(&ss->ssl3.hs.lastMessageFlight);
On 2016/03/24 19:38:40, ekr-webrtc wrote:
> Actually do both for TLS < 1.3

Done.

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

https://codereview.appspot.com/289590043/diff/1/lib/ssl/tls13con.c#newcode82
lib/ssl/tls13con.c:82: 
On 2016/03/24 19:38:40, ekr-webrtc wrote:
> WS

Done.

https://codereview.appspot.com/289590043/diff/1/lib/ssl/tls13con.c#newcode1307
lib/ssl/tls13con.c:1307: PORT_Assert(spec->refCt > 0);
On 2016/03/24 19:38:40, ekr-webrtc wrote:
> Move this to release

Done.

https://codereview.appspot.com/289590043/diff/1/lib/ssl/tls13con.c#newcode1509
lib/ssl/tls13con.c:1509: 
On 2016/03/24 19:38:40, ekr-webrtc wrote:
> WS

Done.

https://codereview.appspot.com/289590043/diff/1/lib/ssl/tls13con.c#newcode1661
lib/ssl/tls13con.c:1661: pwSpec = ss->ssl3.pwSpec;
On 2016/03/24 19:38:40, ekr-webrtc wrote:
> needed?

Done.

https://codereview.appspot.com/289590043/diff/1/lib/ssl/tls13con.c#newcode2753
lib/ssl/tls13con.c:2753: 
On 2016/03/24 19:38:40, ekr-webrtc wrote:
> extra

Done.

[ekr-rietveld, 2016-03-24 20:43:12]

PTAL

[mt, 2016-03-29 00:09:27]

I think that it would a lot better if this could be done for TLS <= 1.2 as well.
 The only thing that concerns me there is where the holddown timer is started. 
I think that overall it's easier to implement for <= 1.2.

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

https://codereview.appspot.com/289590043/diff/20001/external_tests/ssl_gtest/...
external_tests/ssl_gtest/libssl_internals.c:96: PRInt32
SSLInt_CountTls13CipherSuites(PRFileDesc *fd)
s/CipherSuites/CipherSpecs

And is this really TLS 1.3 specific?

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

https://codereview.appspot.com/289590043/diff/20001/external_tests/ssl_gtest/...
external_tests/ssl_gtest/ssl_loopback_unittest.cc:997: WAIT_(false, 31000); /*
Wait for the holddown timer to expire */
This is crazy-long.  Is there some way you can modify this so that we don't
double the time taken to run all the tests?

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

https://codereview.appspot.com/289590043/diff/20001/external_tests/ssl_gtest/...
external_tests/ssl_gtest/tls_connect.cc:24: static const std::string
kTlsModesDatagramArr[] = {"DTLS"};
Oops, my bad.

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

https://codereview.appspot.com/289590043/diff/20001/lib/ssl/sslimpl.h#newcode978
lib/ssl/sslimpl.h:978: ssl3CipherSpec *pwSpec; /* pending write spec. */
Any reason why you are only changing this for TLS 1.3?  The logic is fairly
straightforward for 1.2 as well.

https://codereview.appspot.com/289590043/diff/20001/lib/ssl/sslimpl.h#newcode997
lib/ssl/sslimpl.h:997: ssl3CipherSpec specs[2]; /* one is current, one is
pending. */
See above.

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

https://codereview.appspot.com/289590043/diff/20001/lib/ssl/tls13con.c#newcod...
lib/ssl/tls13con.c:1212: spec->refCt = 2;
/* one for read, one for write */

https://codereview.appspot.com/289590043/diff/20001/lib/ssl/tls13con.c#newcod...
lib/ssl/tls13con.c:1221: tls13_CipherSpecAddRef(ssl3CipherSpec *spec)
Do you need to hold the spec write lock here?

https://codereview.appspot.com/289590043/diff/20001/lib/ssl/tls13con.c#newcod...
lib/ssl/tls13con.c:1260: spec->refCt = 2;
/* one for read, one for write */

https://codereview.appspot.com/289590043/diff/20001/lib/ssl/tls13con.c#newcod...
lib/ssl/tls13con.c:1268: * what ssl3_SetupPendingCipherSpec does. */
So refactor?  It looks like ssl3_SetupPendingCipherSpec only reads ss->version
and ss->hs.cipher_suite, so maybe you could use that to your advantage.

BTW, most of the crap in the cipher specs is just duplicated crap.  I'm fairly
sure that the ssl3 code could handle having some of that code deleted.

https://codereview.appspot.com/289590043/diff/20001/lib/ssl/tls13con.c#newcod...
lib/ssl/tls13con.c:1306: (*specp) = (ssl3CipherSpec *)PR_NEXT_LINK((PRCList
*)spec);
&spec->link

https://codereview.appspot.com/289590043/diff/20001/lib/ssl/tls13con.c#newcod...
lib/ssl/tls13con.c:1597: * 0-RTT. */
I'm surprised that you didn't just do:

if (hashType == unknown) {
  PORT_Assert(0);
}

Until this is tested, who knows what it does.

[ekr-rietveld, 2016-04-01 10:30:39]

MT, I spent some time on the TLS 1.2 version and it's not trivial. In
particular, there are assumptions that prSpec will be valid at earlier times and
that stuff (e.g., SNI) will be copied over. I'd like to defer this, I think

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

https://codereview.appspot.com/289590043/diff/20001/external_tests/ssl_gtest/...
external_tests/ssl_gtest/libssl_internals.c:96: PRInt32
SSLInt_CountTls13CipherSuites(PRFileDesc *fd)
On 2016/03/29 00:09:26, mt wrote:
> s/CipherSuites/CipherSpecs
> 
> And is this really TLS 1.3 specific?

It is now. If we do the TLS 1.2 version it won't be.

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

https://codereview.appspot.com/289590043/diff/20001/external_tests/ssl_gtest/...
external_tests/ssl_gtest/ssl_loopback_unittest.cc:997: WAIT_(false, 31000); /*
Wait for the holddown timer to expire */
On 2016/03/29 00:09:26, mt wrote:
> This is crazy-long.  Is there some way you can modify this so that we don't
> double the time taken to run all the tests?

Well, this is the actual number. I guess we can reach in via the internals.

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

https://codereview.appspot.com/289590043/diff/20001/lib/ssl/sslimpl.h#newcode978
lib/ssl/sslimpl.h:978: ssl3CipherSpec *pwSpec; /* pending write spec. */
On 2016/03/29 00:09:27, mt wrote:
> Any reason why you are only changing this for TLS 1.3?  The logic is fairly
> straightforward for 1.2 as well.

Conservatism. I'll take a stab at 1.2 but if it gets complicated I will want to
land it as a separate patch.

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

https://codereview.appspot.com/289590043/diff/20001/lib/ssl/tls13con.c#newcod...
lib/ssl/tls13con.c:1221: tls13_CipherSpecAddRef(ssl3CipherSpec *spec)
On 2016/03/29 00:09:27, mt wrote:
> Do you need to hold the spec write lock here?

Not here, but where I write crSpecabove.

https://codereview.appspot.com/289590043/diff/20001/lib/ssl/tls13con.c#newcod...
lib/ssl/tls13con.c:1260: spec->refCt = 2;
On 2016/03/29 00:09:27, mt wrote:
> /* one for read, one for write */

Done.

https://codereview.appspot.com/289590043/diff/20001/lib/ssl/tls13con.c#newcod...
lib/ssl/tls13con.c:1268: * what ssl3_SetupPendingCipherSpec does. */
On 2016/03/29 00:09:27, mt wrote:
> So refactor?  It looks like ssl3_SetupPendingCipherSpec only reads ss->version
> and ss->hs.cipher_suite, so maybe you could use that to your advantage.
> 
> BTW, most of the crap in the cipher specs is just duplicated crap.  I'm fairly
> sure that the ssl3 code could handle having some of that code deleted.

I'd like to leave this for a separate patch.

https://codereview.appspot.com/289590043/diff/20001/lib/ssl/tls13con.c#newcod...
lib/ssl/tls13con.c:1306: (*specp) = (ssl3CipherSpec *)PR_NEXT_LINK((PRCList
*)spec);
On 2016/03/29 00:09:27, mt wrote:
> &spec->link

These are the same. Why do you prefer that?

[mt, 2016-04-02 13:00:47]

Yep, ship it.

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

https://codereview.appspot.com/289590043/diff/20001/lib/ssl/tls13con.c#newcod...
lib/ssl/tls13con.c:1306: (*specp) = (ssl3CipherSpec *)PR_NEXT_LINK((PRCList
*)spec);
On 2016/04/01 10:30:39, ekr-webrtc wrote:
> On 2016/03/29 00:09:27, mt wrote:
> > &spec->link
> 
> These are the same. Why do you prefer that?

&spec->link makes it clearer that you know what you are doing, and there is no
cast.

[ekr-rietveld, 2016-04-02 13:59:16]

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

https://codereview.appspot.com/289590043/diff/20001/lib/ssl/tls13con.c#newcod...
lib/ssl/tls13con.c:1306: (*specp) = (ssl3CipherSpec *)PR_NEXT_LINK((PRCList
*)spec);
On 2016/04/02 13:00:47, mt wrote:
> On 2016/04/01 10:30:39, ekr-webrtc wrote:
> > On 2016/03/29 00:09:27, mt wrote:
> > > &spec->link
> > 
> > These are the same. Why do you prefer that?
> 
> &spec->link makes it clearer that you know what you are doing, and there is no
> cast.
OK