Certificate indexing

First round

[mt, 2016-02-21 00:18:11]

Wan-Teh, ekr, this is really just FYI.

Tim, sorry to dump a large patch on you, but I think that this is OK.

[ttaubert, 2016-02-25 15:36:53]

LGTM, great cleanup. The code is a lot easier to understand and we have RSA-PSS,
yay.

https://codereview.appspot.com/284710043/diff/1/external_tests/ssl_gtest/tls_...
File external_tests/ssl_gtest/tls_agent.h (right):

https://codereview.appspot.com/284710043/diff/1/external_tests/ssl_gtest/tls_...
external_tests/ssl_gtest/tls_agent.h:77: void
EnableCiphersByAuthAlgorithm(SSLAuthType authType);
Nit: Should we call it EnableCiphersbyAuthType() in sync with the changes in the
other files?

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

https://codereview.appspot.com/284710043/diff/1/lib/ssl/ssl3con.c#newcode769
lib/ssl/ssl3con.c:769: #include <stdio.h>
Probably left over from debugging?

https://codereview.appspot.com/284710043/diff/1/lib/ssl/ssl3con.c#newcode771
lib/ssl/ssl3con.c:771: ssl3_HasCert(sslSocket *ss, SSLAuthType authType) {
Nit: bracket on new line

https://codereview.appspot.com/284710043/diff/1/lib/ssl/ssl3con.c#newcode788
lib/ssl/ssl3con.c:788: }
The indentation of this function looks off.

https://codereview.appspot.com/284710043/diff/1/lib/ssl/ssl3con.c#newcode3791
lib/ssl/ssl3con.c:3791: (ss->ssl3.hs.kea_def->exchKeyType == ssl_kea_ecdh));
Nit: remove tab

https://codereview.appspot.com/284710043/diff/1/lib/ssl/ssl3con.c#newcode5903
lib/ssl/ssl3con.c:5903: for (j = 0; j < ssl_auth_size; ++j) {
Nit: remove tab

https://codereview.appspot.com/284710043/diff/1/lib/ssl/ssl3con.c#newcode8088
lib/ssl/ssl3con.c:8088: return rv;	/* err code was set. */
Nit: remove tab

https://codereview.appspot.com/284710043/diff/1/lib/ssl/ssl3con.c#newcode8139
lib/ssl/ssl3con.c:8139: ssl3_SelectServerCert(sslSocket *ss) {
nit: bracket on new line

https://codereview.appspot.com/284710043/diff/1/lib/ssl/ssl3con.c#newcode8741
lib/ssl/ssl3con.c:8741: serverCert = ssl_FindServerCert(ss, &sid->certSlot);
Can serverCert be NULL here?

https://codereview.appspot.com/284710043/diff/1/lib/ssl/ssl3con.c#newcode10754
lib/ssl/ssl3con.c:10754: statusToSend = serverCert->certStatusArray;
nit: remove tab

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

https://codereview.appspot.com/284710043/diff/1/lib/ssl/ssl3ecc.c#newcode944
lib/ssl/ssl3ecc.c:944: &signed_hash, isTLS);
Nit: I think this might fit on a line now?

https://codereview.appspot.com/284710043/diff/1/lib/ssl/ssl3ecc.c#newcode1098
lib/ssl/ssl3ecc.c:1098: ssl3_FirstCertificateByAuthType(sslSocket *ss,
SSLAuthType authType) {
Nit: bracket on new line

https://codereview.appspot.com/284710043/diff/1/lib/ssl/ssl3ecc.c#newcode1108
lib/ssl/ssl3ecc.c:1108: return PR_FALSE;
That should be "return NULL"?

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

https://codereview.appspot.com/284710043/diff/1/lib/ssl/ssl3ext.c#newcode1282
lib/ssl/ssl3ext.c:1282: rv = ssl3_AppendNumberToItem(&plaintext, 0, 1); /*
previously exchKeyType */
Couldn't we just remove this? Do we need to keep it for backwards compatibility?

https://codereview.appspot.com/284710043/diff/1/lib/ssl/ssl3ext.c#newcode1734
lib/ssl/ssl3ext.c:1734: temp = ssl3_ConsumeHandshakeNumber(ss, 1, &buffer,
&buffer_len);
A "if (temp < 0) goto no_ticket;" check is missing here.

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

https://codereview.appspot.com/284710043/diff/1/lib/ssl/sslcert.c#newcode59
lib/ssl/sslcert.c:59: ssl_FreeServerCert(sslServerCert *sc) {
nit: bracket on new line

https://codereview.appspot.com/284710043/diff/1/lib/ssl/sslcert.c#newcode113
lib/ssl/sslcert.c:113: ssl_OneTimeCertSetup(sslSocket *ss, const sslServerCert
*sc) {
nit: bracket on new line

https://codereview.appspot.com/284710043/diff/1/lib/ssl/sslcert.c#newcode125
lib/ssl/sslcert.c:125: sc->slot.authType == ssl_auth_rsa_decrypt ||
Which suites exactly use kx=DHE and auth=rsa_decrypt? I thought we'd always use
auth=rsa_sign but I might be wrong.

https://codereview.appspot.com/284710043/diff/1/lib/ssl/sslcert.c#newcode152
lib/ssl/sslcert.c:152: return SECSuccess;
If we return here, should we NULL serverCert and serverCertChain to make sure we
at least hit a NULL deref when we try to use them?

https://codereview.appspot.com/284710043/diff/1/lib/ssl/sslcert.c#newcode171
lib/ssl/sslcert.c:171: ssl3_FreeKeyPair(sc->serverKeyPair);
NULL serverKeyPair here too?

https://codereview.appspot.com/284710043/diff/1/lib/ssl/sslcert.c#newcode187
lib/ssl/sslcert.c:187: SECITEM_FreeArray(sc->certStatusArray, PR_TRUE);
sc->certStatusArray = NULL; ?

https://codereview.appspot.com/284710043/diff/1/lib/ssl/sslcert.c#newcode201
lib/ssl/sslcert.c:201: SECITEM_FreeItem(&sc->signedCertTimestamps, PR_FALSE);
sc->signedCertTimestamps.data = NULL (and) .len = 0; ?

https://codereview.appspot.com/284710043/diff/1/lib/ssl/sslcert.c#newcode243
lib/ssl/sslcert.c:243: ssl_FreeServerCert((sslServerCert *)oldsc);
Isn't oldsc a sslServerCert*, why the cast?

https://codereview.appspot.com/284710043/diff/1/lib/ssl/sslcert.c#newcode245
lib/ssl/sslcert.c:245: sc = ssl_NewServerCert(&certSlot);
NULL check for sc missing.

https://codereview.appspot.com/284710043/diff/1/lib/ssl/sslcert.c#newcode354
lib/ssl/sslcert.c:354: ssl_MakeKeyPairForCert(SECKEYPrivateKey *key,
SECKEYPublicKey *pubKey) {
nit: bracket on new line

https://codereview.appspot.com/284710043/diff/1/lib/ssl/sslcert.c#newcode461
lib/ssl/sslcert.c:461: tag == SEC_OID_PKCS1_RSA_ENCRYPTION;
nit: indentation

https://codereview.appspot.com/284710043/diff/1/lib/ssl/sslcert.c#newcode619
lib/ssl/sslcert.c:619: default:
ssl3_FreeKeyPair(keyPair);

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

https://codereview.appspot.com/284710043/diff/1/lib/ssl/sslcon.c#newcode3318
lib/ssl/sslcon.c:3318: sc = ssl_FindServerCert(ss, &certSlot);
sc could be NULL here, no? Maybe this can't actually happen because we wouldn't
get here with an SSLv2 config without an RSA cert, but I could see the static
analyzer complain about it.

https://codereview.appspot.com/284710043/diff/1/lib/ssl/sslcon.c#newcode3511
lib/ssl/sslcon.c:3511: ss->sec.authKeyBits   =
ss->sec.serverCert->serverKeyBits;
Nit: remove tab

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

https://codereview.appspot.com/284710043/diff/1/lib/ssl/sslinfo.c#newcode136
lib/ssl/sslinfo.c:136: #define S_RSAX	"RSAX", ssl_auth_rsa_sign /* these are
buggy export ciphers */
Nit: Guess you can remove the tabs if you're touching those lines anyway.

https://codereview.appspot.com/284710043/diff/1/lib/ssl/sslinfo.c#newcode144
lib/ssl/sslinfo.c:144: #define K_ECDHE	"ECDHE", ssl_kea_ecdh
Nit: Remove tabs here too.

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

https://codereview.appspot.com/284710043/diff/1/lib/ssl/sslsnce.c#newcode1864
lib/ssl/sslsnce.c:1864: rv = PR_FALSE;
Nit: indentation

[mt, 2016-03-03 07:28:57]

Thanks for the comments Tim.

ekr, PTAL

https://codereview.appspot.com/284710043/diff/1/external_tests/ssl_gtest/tls_...
File external_tests/ssl_gtest/tls_agent.h (right):

https://codereview.appspot.com/284710043/diff/1/external_tests/ssl_gtest/tls_...
external_tests/ssl_gtest/tls_agent.h:77: void
EnableCiphersByAuthAlgorithm(SSLAuthType authType);
On 2016/02/25 15:36:52, ttaubert wrote:
> Nit: Should we call it EnableCiphersbyAuthType() in sync with the changes in
the
> other files?

Done.

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

https://codereview.appspot.com/284710043/diff/1/lib/ssl/ssl3con.c#newcode769
lib/ssl/ssl3con.c:769: #include <stdio.h>
On 2016/02/25 15:36:52, ttaubert wrote:
> Probably left over from debugging?

Done.

https://codereview.appspot.com/284710043/diff/1/lib/ssl/ssl3con.c#newcode771
lib/ssl/ssl3con.c:771: ssl3_HasCert(sslSocket *ss, SSLAuthType authType) {
On 2016/02/25 15:36:52, ttaubert wrote:
> Nit: bracket on new line

Done.

https://codereview.appspot.com/284710043/diff/1/lib/ssl/ssl3con.c#newcode788
lib/ssl/ssl3con.c:788: }
On 2016/02/25 15:36:52, ttaubert wrote:
> The indentation of this function looks off.

Done.

https://codereview.appspot.com/284710043/diff/1/lib/ssl/ssl3con.c#newcode3791
lib/ssl/ssl3con.c:3791: (ss->ssl3.hs.kea_def->exchKeyType == ssl_kea_ecdh));
On 2016/02/25 15:36:52, ttaubert wrote:
> Nit: remove tab

Done.

https://codereview.appspot.com/284710043/diff/1/lib/ssl/ssl3con.c#newcode5903
lib/ssl/ssl3con.c:5903: for (j = 0; j < ssl_auth_size; ++j) {
On 2016/02/25 15:36:52, ttaubert wrote:
> Nit: remove tab

Done.

https://codereview.appspot.com/284710043/diff/1/lib/ssl/ssl3con.c#newcode8088
lib/ssl/ssl3con.c:8088: return rv;	/* err code was set. */
On 2016/02/25 15:36:52, ttaubert wrote:
> Nit: remove tab

As you might have guessed, I made these changes before the clang-format patch
landed.  Rebasing was not fun.

https://codereview.appspot.com/284710043/diff/1/lib/ssl/ssl3con.c#newcode8139
lib/ssl/ssl3con.c:8139: ssl3_SelectServerCert(sslSocket *ss) {
On 2016/02/25 15:36:52, ttaubert wrote:
> nit: bracket on new line

Done.

https://codereview.appspot.com/284710043/diff/1/lib/ssl/ssl3con.c#newcode8741
lib/ssl/ssl3con.c:8741: serverCert = ssl_FindServerCert(ss, &sid->certSlot);
On 2016/02/25 15:36:52, ttaubert wrote:
> Can serverCert be NULL here?

Good catch.  We insist on it being non-null later.  I think that we can break
here and let the handshake fail later.

https://codereview.appspot.com/284710043/diff/1/lib/ssl/ssl3con.c#newcode10754
lib/ssl/ssl3con.c:10754: statusToSend = serverCert->certStatusArray;
On 2016/02/25 15:36:52, ttaubert wrote:
> nit: remove tab

Done.

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

https://codereview.appspot.com/284710043/diff/1/lib/ssl/ssl3ecc.c#newcode944
lib/ssl/ssl3ecc.c:944: &signed_hash, isTLS);
On 2016/02/25 15:36:52, ttaubert wrote:
> Nit: I think this might fit on a line now?

Done.

https://codereview.appspot.com/284710043/diff/1/lib/ssl/ssl3ecc.c#newcode1108
lib/ssl/ssl3ecc.c:1108: return PR_FALSE;
On 2016/02/25 15:36:52, ttaubert wrote:
> That should be "return NULL"?

Sometimes I wish that C would catch that sort of thing.

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

https://codereview.appspot.com/284710043/diff/1/lib/ssl/ssl3ext.c#newcode1282
lib/ssl/ssl3ext.c:1282: rv = ssl3_AppendNumberToItem(&plaintext, 0, 1); /*
previously exchKeyType */
On 2016/02/25 15:36:52, ttaubert wrote:
> Couldn't we just remove this? Do we need to keep it for backwards
compatibility?

Yep, I noticed that during the rebase and removed it.

https://codereview.appspot.com/284710043/diff/1/lib/ssl/ssl3ext.c#newcode1734
lib/ssl/ssl3ext.c:1734: temp = ssl3_ConsumeHandshakeNumber(ss, 1, &buffer,
&buffer_len);
On 2016/02/25 15:36:52, ttaubert wrote:
> A "if (temp < 0) goto no_ticket;" check is missing here.

Done.

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

https://codereview.appspot.com/284710043/diff/1/lib/ssl/sslcert.c#newcode125
lib/ssl/sslcert.c:125: sc->slot.authType == ssl_auth_rsa_decrypt ||
On 2016/02/25 15:36:53, ttaubert wrote:
> Which suites exactly use kx=DHE and auth=rsa_decrypt? I thought we'd always
use
> auth=rsa_sign but I might be wrong.

The checks here are surrogate for other things.  What Kai was testing was the
possibility that the key might be needed.  Since only RSA and DSA are supported
for the DHE suites, that was his proxy.

You want to do the setup when configuring certificates because it is potentially
time-consuming and you don't want to take that hit when you are looking to
handle an inbound connection.

https://codereview.appspot.com/284710043/diff/1/lib/ssl/sslcert.c#newcode152
lib/ssl/sslcert.c:152: return SECSuccess;
On 2016/02/25 15:36:52, ttaubert wrote:
> If we return here, should we NULL serverCert and serverCertChain to make sure
we
> at least hit a NULL deref when we try to use them?

Done.

https://codereview.appspot.com/284710043/diff/1/lib/ssl/sslcert.c#newcode171
lib/ssl/sslcert.c:171: ssl3_FreeKeyPair(sc->serverKeyPair);
On 2016/02/25 15:36:53, ttaubert wrote:
> NULL serverKeyPair here too?

Done.

https://codereview.appspot.com/284710043/diff/1/lib/ssl/sslcert.c#newcode187
lib/ssl/sslcert.c:187: SECITEM_FreeArray(sc->certStatusArray, PR_TRUE);
On 2016/02/25 15:36:52, ttaubert wrote:
> sc->certStatusArray = NULL; ?

Done.

https://codereview.appspot.com/284710043/diff/1/lib/ssl/sslcert.c#newcode201
lib/ssl/sslcert.c:201: SECITEM_FreeItem(&sc->signedCertTimestamps, PR_FALSE);
On 2016/02/25 15:36:53, ttaubert wrote:
> sc->signedCertTimestamps.data = NULL (and) .len = 0; ?

SECITEM_FreeItem does that for you.

https://codereview.appspot.com/284710043/diff/1/lib/ssl/sslcert.c#newcode243
lib/ssl/sslcert.c:243: ssl_FreeServerCert((sslServerCert *)oldsc);
On 2016/02/25 15:36:53, ttaubert wrote:
> Isn't oldsc a sslServerCert*, why the cast?

Const-cast :)

https://codereview.appspot.com/284710043/diff/1/lib/ssl/sslcert.c#newcode245
lib/ssl/sslcert.c:245: sc = ssl_NewServerCert(&certSlot);
On 2016/02/25 15:36:53, ttaubert wrote:
> NULL check for sc missing.

Done.

https://codereview.appspot.com/284710043/diff/1/lib/ssl/sslcert.c#newcode354
lib/ssl/sslcert.c:354: ssl_MakeKeyPairForCert(SECKEYPrivateKey *key,
SECKEYPublicKey *pubKey) {
On 2016/02/25 15:36:53, ttaubert wrote:
> nit: bracket on new line

Done.

https://codereview.appspot.com/284710043/diff/1/lib/ssl/sslcert.c#newcode619
lib/ssl/sslcert.c:619: default:
On 2016/02/25 15:36:52, ttaubert wrote:
> ssl3_FreeKeyPair(keyPair);

It is adopted by ssl_MakeKeyPairForCert, whether it succeeds or not.  Expanded
the comment to make this clearer.

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

https://codereview.appspot.com/284710043/diff/1/lib/ssl/sslcon.c#newcode3318
lib/ssl/sslcon.c:3318: sc = ssl_FindServerCert(ss, &certSlot);
On 2016/02/25 15:36:53, ttaubert wrote:
> sc could be NULL here, no? Maybe this can't actually happen because we
wouldn't
> get here with an SSLv2 config without an RSA cert, but I could see the static
> analyzer complain about it.

I don't think that we need to spend lots of time on this one.  I can't figure a
way here, but that's not to say that there isn't a way we can mess this one up.

We're dumping this code soon.  I just need it to compile :)

https://codereview.appspot.com/284710043/diff/1/lib/ssl/sslcon.c#newcode3511
lib/ssl/sslcon.c:3511: ss->sec.authKeyBits   =
ss->sec.serverCert->serverKeyBits;
On 2016/02/25 15:36:53, ttaubert wrote:
> Nit: remove tab

Done.

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

https://codereview.appspot.com/284710043/diff/1/lib/ssl/sslinfo.c#newcode136
lib/ssl/sslinfo.c:136: #define S_RSAX	"RSAX", ssl_auth_rsa_sign /* these are
buggy export ciphers */
On 2016/02/25 15:36:53, ttaubert wrote:
> Nit: Guess you can remove the tabs if you're touching those lines anyway.

Done.

https://codereview.appspot.com/284710043/diff/1/lib/ssl/sslinfo.c#newcode144
lib/ssl/sslinfo.c:144: #define K_ECDHE	"ECDHE", ssl_kea_ecdh
On 2016/02/25 15:36:53, ttaubert wrote:
> Nit: Remove tabs here too.

Done.

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

https://codereview.appspot.com/284710043/diff/1/lib/ssl/sslsnce.c#newcode1864
lib/ssl/sslsnce.c:1864: rv = PR_FALSE;
On 2016/02/25 15:36:53, ttaubert wrote:
> Nit: indentation

Done.

[ekr-rietveld, 2016-03-05 21:31:49]

IMPORTANT: I think you may have broken the ABI. See below.

IMPORTANT: I did not review the SSLv2 stuff. Let's solve that by landing after
we remove SSLv2. I would also land this patch after we remove export.

I think WTC or Bob should review the backward compatibility code in sslcert.c.

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

https://codereview.appspot.com/284710043/diff/20001/external_tests/ssl_gtest/...
external_tests/ssl_gtest/libssl_internals.c:17: sslSocket *ss =
ssl_FindSocket(fd);
Unnecessary change.

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

https://codereview.appspot.com/284710043/diff/20001/external_tests/ssl_gtest/...
external_tests/ssl_gtest/ssl_loopback_unittest.cc:159: }
Maybe CheckKeys() should be expanded here.

Also, maybe we should just disable ssl_auth_ecdh at this point.

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

https://codereview.appspot.com/284710043/diff/20001/lib/ssl/ssl.h#newcode783
lib/ssl/ssl.h:783: ** also used: EC certificates with different curves are
separately stored.
Suggested rewrite:
The type of certificate is determined from the certificate and private
key based on how libssl uses certificates. Primarily, this uses the
value of the SSLAuthType enum. For example, different RSA certificates
might be saved for signing (ssl_auth_rsa_sign) and key encipherment
(ssl_auth_rsa_decrypt).  Unique to RSA, the same certificate can be
used for both usages.  Additional information about the certificate is
also used: EC certificates with different curves are separately
stored.

Only one certificate is stored for each type of certificate.

https://codereview.appspot.com/284710043/diff/20001/lib/ssl/ssl.h#newcode810
lib/ssl/ssl.h:810: */
How do you do that?

https://codereview.appspot.com/284710043/diff/20001/lib/ssl/ssl.h#newcode813
lib/ssl/ssl.h:813: SECKEYPrivateKey *key, SSLExtraServerCertData *data);
should |data| be const?

https://codereview.appspot.com/284710043/diff/20001/lib/ssl/ssl.h#newcode824
lib/ssl/ssl.h:824: ** providing both ECDH and ECDSA certificates.
Please use |foo| for parameters in all the comments here.

https://codereview.appspot.com/284710043/diff/20001/lib/ssl/ssl.h#newcode833
lib/ssl/ssl.h:833: ** SSL_ConfigSecureServerCert for details.
I do not see a |data| argument.

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

https://codereview.appspot.com/284710043/diff/20001/lib/ssl/ssl3con.c#oldcode...
lib/ssl/ssl3con.c:11946: sid->u.ssl3.exchKeyType = effectiveExchKeyType;
Note to self: see if this was removed in tickets.

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

https://codereview.appspot.com/284710043/diff/20001/lib/ssl/ssl3con.c#newcode306
lib/ssl/ssl3con.c:306: /* kea            exchKeyType signKeyType authKeyType,
is_limited limit tls_keygen ephemeral  oid */
What is signKeyType? Signing the cert?

https://codereview.appspot.com/284710043/diff/20001/lib/ssl/ssl3con.c#newcode309
lib/ssl/ssl3con.c:309: {kea_rsa_export,     ssl_kea_rsa,  ssl_sign_rsa,
ssl_auth_rsa_sign, PR_TRUE,  512, PR_FALSE, PR_FALSE, SEC_OID_TLS_RSA_EXPORT},
What if you have a 512-bit key?

https://codereview.appspot.com/284710043/diff/20001/lib/ssl/ssl3con.c#newcode310
lib/ssl/ssl3con.c:310: {kea_rsa_export_1024,ssl_kea_rsa,  ssl_sign_rsa,
ssl_auth_rsa_sign, PR_TRUE, 1024, PR_FALSE, PR_FALSE, SEC_OID_TLS_RSA_EXPORT},
Isn't export1024 RSA decryption,

https://codereview.appspot.com/284710043/diff/20001/lib/ssl/ssl3con.c#newcode323
lib/ssl/ssl3con.c:323: {kea_ecdh_ecdsa,     ssl_kea_ecdh, ssl_sign_ecdsa,
ssl_auth_ecdh, PR_FALSE,   0, PR_FALSE, PR_FALSE, SEC_OID_TLS_ECDH_ECDSA},
This is supposed to be ECDH but with an ECDSA cert? 

Can't we just remove this?

https://codereview.appspot.com/284710043/diff/20001/lib/ssl/ssl3con.c#newcode327
lib/ssl/ssl3con.c:327: {kea_ecdh_anon,      ssl_kea_ecdh, ssl_sign_null,
ssl_auth_null, PR_FALSE,   0, PR_FALSE, PR_TRUE,  SEC_OID_TLS_ECDH_ANON},
Not clear to me how you are handling PSS here, since you use the same cipher
suite for PKCS#1 and PSS.

https://codereview.appspot.com/284710043/diff/20001/lib/ssl/ssl3con.c#newcode478
lib/ssl/ssl3con.c:478: 0x80000000L, /* ssl_auth_kea (unused) */
What is the impact of this?

https://codereview.appspot.com/284710043/diff/20001/lib/ssl/ssl3con.c#newcode490
lib/ssl/ssl3con.c:490: 0x80000000L, /* ssl_kea_fortezza (unused) */
What's the impact of this?

https://codereview.appspot.com/284710043/diff/20001/lib/ssl/ssl3con.c#newcode879
lib/ssl/ssl3con.c:879: !PK11_TokenExists(auth_alg_defs[authType])) {
Let's break this last test into a separate clause.

https://codereview.appspot.com/284710043/diff/20001/lib/ssl/ssl3con.c#newcode887
lib/ssl/ssl3con.c:887: if (cipher_alg != calg_null &&
!PK11_TokenExists(cipher_mech)) {
Can we ever have calg_null?

https://codereview.appspot.com/284710043/diff/20001/lib/ssl/ssl3con.c#newcode...
lib/ssl/ssl3con.c:6170: svrPubKey = serverCert->serverKeyPair->pubKey;
You should check that pubkey is non-null above.

https://codereview.appspot.com/284710043/diff/20001/lib/ssl/ssl3con.c#newcode...
lib/ssl/ssl3con.c:8221: * to do either signing or decryption on demand. */
This comment should be in the definitions as ell.

https://codereview.appspot.com/284710043/diff/20001/lib/ssl/ssl3con.c#newcode...
lib/ssl/ssl3con.c:8222: if (kea_def->is_limited && kea_def->authKeyType ==
ssl_auth_rsa_sign) {
PORT_Assert that is_limited ==> ssl_auth_rsa_sign. instaead.

https://codereview.appspot.com/284710043/diff/20001/lib/ssl/ssl3con.c#newcode...
lib/ssl/ssl3con.c:8229: cert = ssl_FindServerCert(ss, &certSlot);
Note to self: ensure that we can't have duplicates of cert types.

https://codereview.appspot.com/284710043/diff/20001/lib/ssl/ssl3con.c#newcode...
lib/ssl/ssl3con.c:8305: * they are configured in. */
Nit: please be consistent about whether . has one space or two after.

https://codereview.appspot.com/284710043/diff/20001/lib/ssl/ssl3con.c#newcode...
lib/ssl/ssl3con.c:8321: }
Suggest have this be a continue and then you can have a bunch of tests ending in
continue followed by a break at the end,=,

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

https://codereview.appspot.com/284710043/diff/20001/lib/ssl/ssl3ecc.c#newcode586
lib/ssl/ssl3ecc.c:586:
PORT_Assert(SSL_IS_CURVE_NEGOTIATED(ss->ssl3.hs.negotiatedECCurves,
IMPORTANT: how are you going to handle Curve25519 when the negotiated signing
curve is P256.

https://codereview.appspot.com/284710043/diff/20001/lib/ssl/ssl3ecc.c#newcode592
lib/ssl/ssl3ecc.c:592: const sslServerCert *cert = ss->sec.serverCert;
PORT_Assert() that it's RSA.

https://codereview.appspot.com/284710043/diff/20001/lib/ssl/ssl3ecc.c#newcode...
lib/ssl/ssl3ecc.c:1116: }
Can you refactor this into ssl3_SelectServerCert?

https://codereview.appspot.com/284710043/diff/20001/lib/ssl/ssl3ecc.c#newcode...
lib/ssl/ssl3ecc.c:1168: }
Let's move this further up next to RSA where it's more natural.

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

https://codereview.appspot.com/284710043/diff/20001/lib/ssl/sslcert.c#newcode15
lib/ssl/sslcert.c:15: #include "prinit.h" /* for PR_CallOnceWithArg */
Let's declare these in sslcert.h

https://codereview.appspot.com/284710043/diff/20001/lib/ssl/sslcert.c#newcode41
lib/ssl/sslcert.c:41: }
I did not review these. Please confirm they were just moved.

https://codereview.appspot.com/284710043/diff/20001/lib/ssl/sslcert.c#newcode50
lib/ssl/sslcert.c:50: memcpy(&sc->slot, certSlot, sizeof(sslServerCertSlot));
sizeof(*slot)

https://codereview.appspot.com/284710043/diff/20001/lib/ssl/sslcert.c#newcode54
lib/ssl/sslcert.c:54: sc->signedCertTimestamps.len = 0;
Kind of unnecessary with the ZNew but I guess good practice.

https://codereview.appspot.com/284710043/diff/20001/lib/ssl/sslcert.c#newcode80
lib/ssl/sslcert.c:80: PORT_ZFree(sc, sizeof(sslServerCert));
sizeof (*sc)

https://codereview.appspot.com/284710043/diff/20001/lib/ssl/sslcert.c#newcode111
lib/ssl/sslcert.c:111: }
Seems to me that you can replace ssl3_FirstCertificateByAuthType

https://codereview.appspot.com/284710043/diff/20001/lib/ssl/sslcert.c#newcode129
lib/ssl/sslcert.c:129: if (ssl3_SelectDHParams(ss) != SECSuccess) {
Doesn't this get called with each cert? That seems unfortunate.

https://codereview.appspot.com/284710043/diff/20001/lib/ssl/sslcert.c#newcode157
lib/ssl/sslcert.c:157: }
Why not overload ssl_FreeServerCert() here

https://codereview.appspot.com/284710043/diff/20001/lib/ssl/sslcert.c#newcode202
lib/ssl/sslcert.c:202: }
Not a big fan of this populate idiom. Seems to me that you can just
free all the contents and then populate unconditionally

https://codereview.appspot.com/284710043/diff/20001/lib/ssl/sslcert.c#newcode233
lib/ssl/sslcert.c:233: * set in *data, we just need to work out any extra
information. */
|*data|

https://codereview.appspot.com/284710043/diff/20001/lib/ssl/sslcert.c#newcode276
lib/ssl/sslcert.c:276: return ssl_OneTimeCertSetup(ss, sc);
Probably best to delete the cert. Seems like you have all the code to do it.

https://codereview.appspot.com/284710043/diff/20001/lib/ssl/sslcert.c#newcode307
lib/ssl/sslcert.c:307: memcpy(&arg, data, sizeof(SSLExtraServerCertData));
sizeof arg

https://codereview.appspot.com/284710043/diff/20001/lib/ssl/sslcert.c#newcode350
lib/ssl/sslcert.c:350: }
So to be clear, this means that you can't have an ECDSA key as an ECDH key if it
can be used for signing?

https://codereview.appspot.com/284710043/diff/20001/lib/ssl/sslcert.c#newcode371
lib/ssl/sslcert.c:371: SECKEYPrivateKey *keyCopy = NULL;
privKeyCopy

https://codereview.appspot.com/284710043/diff/20001/lib/ssl/sslcert.c#newcode389
lib/ssl/sslcert.c:389: }
This probably needs review by WTC.

https://codereview.appspot.com/284710043/diff/20001/lib/ssl/sslcert.c#newcode402
lib/ssl/sslcert.c:402: }
you really want to destroy the pubkey? That seems unfortunate. I would make the
caller destroy it. Or at least make a comment *here*.

https://codereview.appspot.com/284710043/diff/20001/lib/ssl/sslcert.c#newcode510
lib/ssl/sslcert.c:510: }
If you make this public, you can omit several places where you explicitly make
dummy slots.

https://codereview.appspot.com/284710043/diff/20001/lib/ssl/sslcert.c#newcode814
lib/ssl/sslcert.c:814: 
Can we make a macro or function pointer thing for:

SetFooInSlot() that lets us span both timestamps and ocsp.

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

https://codereview.appspot.com/284710043/diff/20001/lib/ssl/sslimpl.h#newcode393
lib/ssl/sslimpl.h:393: ** parameters further narrow the slot.
slot seems like a very unfortunate name given the use of slot for PK11

https://codereview.appspot.com/284710043/diff/20001/lib/ssl/sslimpl.h#newcode...
lib/ssl/sslimpl.h:1985: #define TLS_EX_SESS_TICKET_VERSION (0x0103)
Why not 0x0102

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

https://codereview.appspot.com/284710043/diff/20001/lib/ssl/sslsnce.c#newcode639
lib/ssl/sslsnce.c:639: break;
Can't you just memcpy?

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

https://codereview.appspot.com/284710043/diff/20001/lib/ssl/sslt.h#newcode95
lib/ssl/sslt.h:95: ssl_auth_rsa_decrypt = 1,
I assume this is static RSA? Needs a comment.

https://codereview.appspot.com/284710043/diff/20001/lib/ssl/sslt.h#newcode97
lib/ssl/sslt.h:97: ssl_auth_kea = 3, /* unused */
Why not just remove this? Since these are all explicit assignments, that should
be fine.

https://codereview.appspot.com/284710043/diff/20001/lib/ssl/sslt.h#newcode101
lib/ssl/sslt.h:101: ssl_auth_rsa_pss = 7,
You seem to have created an ABI mismatch, because now ECDHE_RSA will return
ssl_auth_rsa_sign whereas before it returned value #1, now ssl_auth_rsa_decrypt.

https://codereview.appspot.com/284710043/diff/20001/lib/ssl/sslt.h#newcode106
lib/ssl/sslt.h:106: #define ssl_auth_rsa ssl_auth_rsa_sign
Please scrub for every use of this in the .c files. I only saw it one place
but...

[mt, 2016-03-07 12:12:55]

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

https://codereview.appspot.com/284710043/diff/20001/external_tests/ssl_gtest/...
external_tests/ssl_gtest/libssl_internals.c:17: sslSocket *ss =
ssl_FindSocket(fd);
On 2016/03/05 21:31:45, ekr-webrtc wrote:
> Unnecessary change.

Acknowledged.

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

https://codereview.appspot.com/284710043/diff/20001/external_tests/ssl_gtest/...
external_tests/ssl_gtest/ssl_loopback_unittest.cc:159: }
On 2016/03/05 21:31:46, ekr-webrtc wrote:
> Maybe CheckKeys() should be expanded here.
> 
> Also, maybe we should just disable ssl_auth_ecdh at this point.

Removing would be better than disabling :)  But one thing at a time.

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

https://codereview.appspot.com/284710043/diff/20001/lib/ssl/ssl.h#newcode783
lib/ssl/ssl.h:783: ** also used: EC certificates with different curves are
separately stored.
On 2016/03/05 21:31:46, ekr-webrtc wrote:
> Suggested rewrite:
> The type of certificate is determined from the certificate and private
> key based on how libssl uses certificates. Primarily, this uses the
> value of the SSLAuthType enum. For example, different RSA certificates
> might be saved for signing (ssl_auth_rsa_sign) and key encipherment
> (ssl_auth_rsa_decrypt).  Unique to RSA, the same certificate can be
> used for both usages.  Additional information about the certificate is
> also used: EC certificates with different curves are separately
> stored.
> 
> Only one certificate is stored for each type of certificate.

Done.

https://codereview.appspot.com/284710043/diff/20001/lib/ssl/ssl.h#newcode810
lib/ssl/ssl.h:810: */
On 2016/03/05 21:31:46, ekr-webrtc wrote:
> How do you do that?

I didn't define a way to do that.  I could define a way to delete a certificate,
but I decided to wait until someone asked for one.

https://codereview.appspot.com/284710043/diff/20001/lib/ssl/ssl.h#newcode813
lib/ssl/ssl.h:813: SECKEYPrivateKey *key, SSLExtraServerCertData *data);
On 2016/03/05 21:31:46, ekr-webrtc wrote:
> should |data| be const?

I wanted to have this const, but the constituent parts can't be.  Ahh, but C
doesn't require const-consistency, so yes, const is right.

https://codereview.appspot.com/284710043/diff/20001/lib/ssl/ssl.h#newcode824
lib/ssl/ssl.h:824: ** providing both ECDH and ECDSA certificates.
On 2016/03/05 21:31:46, ekr-webrtc wrote:
> Please use |foo| for parameters in all the comments here.

Done.

https://codereview.appspot.com/284710043/diff/20001/lib/ssl/ssl.h#newcode833
lib/ssl/ssl.h:833: ** SSL_ConfigSecureServerCert for details.
On 2016/03/05 21:31:46, ekr-webrtc wrote:
> I do not see a |data| argument.

Yes, it refers to SSL_ConfigSecureServerCert, updated.

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

https://codereview.appspot.com/284710043/diff/20001/lib/ssl/ssl3con.c#newcode306
lib/ssl/ssl3con.c:306: /* kea            exchKeyType signKeyType authKeyType,
is_limited limit tls_keygen ephemeral  oid */
On 2016/03/05 21:31:47, ekr-webrtc wrote:
> What is signKeyType? Signing the cert?

If there is a signature used by the cipher suite, it identifies the type of
signature.

https://codereview.appspot.com/284710043/diff/20001/lib/ssl/ssl3con.c#newcode309
lib/ssl/ssl3con.c:309: {kea_rsa_export,     ssl_kea_rsa,  ssl_sign_rsa,
ssl_auth_rsa_sign, PR_TRUE,  512, PR_FALSE, PR_FALSE, SEC_OID_TLS_RSA_EXPORT},
On 2016/03/05 21:31:47, ekr-webrtc wrote:
> What if you have a 512-bit key?

You don't send ServerKeyExchange.  But we're removing this crap.

https://codereview.appspot.com/284710043/diff/20001/lib/ssl/ssl3con.c#newcode310
lib/ssl/ssl3con.c:310: {kea_rsa_export_1024,ssl_kea_rsa,  ssl_sign_rsa,
ssl_auth_rsa_sign, PR_TRUE, 1024, PR_FALSE, PR_FALSE, SEC_OID_TLS_RSA_EXPORT},
On 2016/03/05 21:31:47, ekr-webrtc wrote:
> Isn't export1024 RSA decryption,

Actually, it's exactly the same as the one above, with a different limit on when
you switch from signing to decryption.  Both of these are switch-hitters, which
I comment about in a few places.  But both are being removed.

https://codereview.appspot.com/284710043/diff/20001/lib/ssl/ssl3con.c#newcode323
lib/ssl/ssl3con.c:323: {kea_ecdh_ecdsa,     ssl_kea_ecdh, ssl_sign_ecdsa,
ssl_auth_ecdh, PR_FALSE,   0, PR_FALSE, PR_FALSE, SEC_OID_TLS_ECDH_ECDSA},
On 2016/03/05 21:31:46, ekr-webrtc wrote:
> This is supposed to be ECDH but with an ECDSA cert? 

Hmm, this is probably wrong.  This probably means that you have an ECDH cert
that has an ECDSA signature on it from the CA.

> Can't we just remove this?

Probably not right away.

https://codereview.appspot.com/284710043/diff/20001/lib/ssl/ssl3con.c#newcode327
lib/ssl/ssl3con.c:327: {kea_ecdh_anon,      ssl_kea_ecdh, ssl_sign_null,
ssl_auth_null, PR_FALSE,   0, PR_FALSE, PR_TRUE,  SEC_OID_TLS_ECDH_ANON},
On 2016/03/05 21:31:47, ekr-webrtc wrote:
> Not clear to me how you are handling PSS here, since you use the same cipher
> suite for PKCS#1 and PSS.

PSS is only partially implemented here.  We don't actually go looking for PSS in
the code yet.  I only wanted to lay some of the ground work for PSS.

And I think that config doesn't really work for the cases we care about, but
I'll get back to that.

https://codereview.appspot.com/284710043/diff/20001/lib/ssl/ssl3con.c#newcode478
lib/ssl/ssl3con.c:478: 0x80000000L, /* ssl_auth_kea (unused) */
On 2016/03/05 21:31:46, ekr-webrtc wrote:
> What is the impact of this?

Nada.  We don't actually use a cipher suite that relies on this.  I'm just
filling the slot.

https://codereview.appspot.com/284710043/diff/20001/lib/ssl/ssl3con.c#newcode490
lib/ssl/ssl3con.c:490: 0x80000000L, /* ssl_kea_fortezza (unused) */
On 2016/03/05 21:31:46, ekr-webrtc wrote:
> What's the impact of this?

As above.

https://codereview.appspot.com/284710043/diff/20001/lib/ssl/ssl3con.c#newcode887
lib/ssl/ssl3con.c:887: if (cipher_alg != calg_null &&
!PK11_TokenExists(cipher_mech)) {
On 2016/03/05 21:31:47, ekr-webrtc wrote:
> Can we ever have calg_null?

Well, I didn't really think that one through, but maybe we had mac-only cipher
suites at one point.

https://codereview.appspot.com/284710043/diff/20001/lib/ssl/ssl3con.c#newcode...
lib/ssl/ssl3con.c:6170: svrPubKey = serverCert->serverKeyPair->pubKey;
On 2016/03/05 21:31:47, ekr-webrtc wrote:
> You should check that pubkey is non-null above.

Done.

https://codereview.appspot.com/284710043/diff/20001/lib/ssl/ssl3con.c#newcode...
lib/ssl/ssl3con.c:8221: * to do either signing or decryption on demand. */
On 2016/03/05 21:31:46, ekr-webrtc wrote:
> This comment should be in the definitions as ell.

I'll just delete the definitions when Tim lands export removal.

https://codereview.appspot.com/284710043/diff/20001/lib/ssl/ssl3con.c#newcode...
lib/ssl/ssl3con.c:8222: if (kea_def->is_limited && kea_def->authKeyType ==
ssl_auth_rsa_sign) {
On 2016/03/05 21:31:47, ekr-webrtc wrote:
> PORT_Assert that is_limited ==> ssl_auth_rsa_sign. instaead.

Acknowledged.

https://codereview.appspot.com/284710043/diff/20001/lib/ssl/ssl3con.c#newcode...
lib/ssl/ssl3con.c:8229: cert = ssl_FindServerCert(ss, &certSlot);
On 2016/03/05 21:31:46, ekr-webrtc wrote:
> Note to self: ensure that we can't have duplicates of cert types.

Acknowledged.

https://codereview.appspot.com/284710043/diff/20001/lib/ssl/ssl3con.c#newcode...
lib/ssl/ssl3con.c:8321: }
On 2016/03/05 21:31:46, ekr-webrtc wrote:
> Suggest have this be a continue and then you can have a bunch of tests ending
in
> continue followed by a break at the end,=,

Done.

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

https://codereview.appspot.com/284710043/diff/20001/lib/ssl/ssl3ecc.c#newcode586
lib/ssl/ssl3ecc.c:586:
PORT_Assert(SSL_IS_CURVE_NEGOTIATED(ss->ssl3.hs.negotiatedECCurves,
On 2016/03/05 21:31:47, ekr-webrtc wrote:
> IMPORTANT: how are you going to handle Curve25519 when the negotiated signing
> curve is P256.

Yeah, I thought about that.  This code is awful.  It makes all sorts of
assumptions about matching certificate strength to cipher choices.  My thought
was that instead we want to rank order the curves and pick the first that was
shared.

We might then want to use the strength of the certificate to change that rank
ordering, or to prefer goldilocks for a P-384 cert, for example.

For now, I wanted to try to maintain existing behaviour here as close as was
feasible.

https://codereview.appspot.com/284710043/diff/20001/lib/ssl/ssl3ecc.c#newcode592
lib/ssl/ssl3ecc.c:592: const sslServerCert *cert = ss->sec.serverCert;
On 2016/03/05 21:31:47, ekr-webrtc wrote:
> PORT_Assert() that it's RSA.

Done.

https://codereview.appspot.com/284710043/diff/20001/lib/ssl/ssl3ecc.c#newcode...
lib/ssl/ssl3ecc.c:1116: }
On 2016/03/05 21:31:47, ekr-webrtc wrote:
> Can you refactor this into ssl3_SelectServerCert?

I don't want to.  ssl3_SelectServerCert looks at curves.  This doesn't.  All
this does is act in support of the function below.

Sure, this isn't a great way to do this.  We can negotiate an EC cipher suite
and then discover that we don't have curves in common and then fail, when RSA is
perfectly good.  But that's not a problem I'm making worse.

https://codereview.appspot.com/284710043/diff/20001/lib/ssl/ssl3ecc.c#newcode...
lib/ssl/ssl3ecc.c:1168: }
On 2016/03/05 21:31:47, ekr-webrtc wrote:
> Let's move this further up next to RSA where it's more natural.

Done.

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

https://codereview.appspot.com/284710043/diff/20001/lib/ssl/sslcert.c#newcode14
lib/ssl/sslcert.c:14: #include "nss.h" /* for NSS_RegisterShutdown */
I'll make a header, but I think that half of these will need to stay here.

https://codereview.appspot.com/284710043/diff/20001/lib/ssl/sslcert.c#newcode41
lib/ssl/sslcert.c:41: }
On 2016/03/05 21:31:49, ekr-webrtc wrote:
> I did not review these. Please confirm they were just moved.

These are straight moves.

https://codereview.appspot.com/284710043/diff/20001/lib/ssl/sslcert.c#newcode50
lib/ssl/sslcert.c:50: memcpy(&sc->slot, certSlot, sizeof(sslServerCertSlot));
On 2016/03/05 21:31:48, ekr-webrtc wrote:
> sizeof(*slot)

Done.

https://codereview.appspot.com/284710043/diff/20001/lib/ssl/sslcert.c#newcode54
lib/ssl/sslcert.c:54: sc->signedCertTimestamps.len = 0;
On 2016/03/05 21:31:48, ekr-webrtc wrote:
> Kind of unnecessary with the ZNew but I guess good practice.

Acknowledged.

https://codereview.appspot.com/284710043/diff/20001/lib/ssl/sslcert.c#newcode80
lib/ssl/sslcert.c:80: PORT_ZFree(sc, sizeof(sslServerCert));
On 2016/03/05 21:31:48, ekr-webrtc wrote:
> sizeof (*sc)

Done.

https://codereview.appspot.com/284710043/diff/20001/lib/ssl/sslcert.c#newcode111
lib/ssl/sslcert.c:111: }
On 2016/03/05 21:31:48, ekr-webrtc wrote:
> Seems to me that you can replace ssl3_FirstCertificateByAuthType

Yes, I can and will.  (I didn't originally have the ec_noName free pass.)

https://codereview.appspot.com/284710043/diff/20001/lib/ssl/sslcert.c#newcode129
lib/ssl/sslcert.c:129: if (ssl3_SelectDHParams(ss) != SECSuccess) {
On 2016/03/05 21:31:48, ekr-webrtc wrote:
> Doesn't this get called with each cert? That seems unfortunate.

Yes, but it is just a wrapper around PR_CallOnce, so it's not that painful.

https://codereview.appspot.com/284710043/diff/20001/lib/ssl/sslcert.c#newcode157
lib/ssl/sslcert.c:157: }
On 2016/03/05 21:31:48, ekr-webrtc wrote:
> Why not overload ssl_FreeServerCert() here

Not sure what you are looking for.  I could implement ssl_FreeServerCert() by
calling this function, is that what you mean?

https://codereview.appspot.com/284710043/diff/20001/lib/ssl/sslcert.c#newcode202
lib/ssl/sslcert.c:202: }
On 2016/03/05 21:31:48, ekr-webrtc wrote:
> Not a big fan of this populate idiom. Seems to me that you can just
> free all the contents and then populate unconditionally

I am not a fan either, but it is necessary to maintain bugward compatibility
with the existing stuff.  My original code just made a new server cert and
stuffed values into it, but then I found that the tests failed.  They were
putting OCSP responses in place before the certs and expecting the two piece of
info to land in the same place.

https://codereview.appspot.com/284710043/diff/20001/lib/ssl/sslcert.c#newcode276
lib/ssl/sslcert.c:276: return ssl_OneTimeCertSetup(ss, sc);
On 2016/03/05 21:31:49, ekr-webrtc wrote:
> Probably best to delete the cert. Seems like you have all the code to do it.

Done.

https://codereview.appspot.com/284710043/diff/20001/lib/ssl/sslcert.c#newcode307
lib/ssl/sslcert.c:307: memcpy(&arg, data, sizeof(SSLExtraServerCertData));
On 2016/03/05 21:31:48, ekr-webrtc wrote:
> sizeof arg

Done.

https://codereview.appspot.com/284710043/diff/20001/lib/ssl/sslcert.c#newcode350
lib/ssl/sslcert.c:350: }
On 2016/03/05 21:31:48, ekr-webrtc wrote:
> So to be clear, this means that you can't have an ECDSA key as an ECDH key if
it
> can be used for signing?

Yep.  I'm only adding the special dual-use code for RSA.  I added a comment: 

        /* Yes, this means that you can't do both ECDSA and ECDH, but that is a
         * bad practice, and ECDH isn't used that much (if at all). */

https://codereview.appspot.com/284710043/diff/20001/lib/ssl/sslcert.c#newcode371
lib/ssl/sslcert.c:371: SECKEYPrivateKey *keyCopy = NULL;
On 2016/03/05 21:31:48, ekr-webrtc wrote:
> privKeyCopy

Done.

https://codereview.appspot.com/284710043/diff/20001/lib/ssl/sslcert.c#newcode389
lib/ssl/sslcert.c:389: }
On 2016/03/05 21:31:48, ekr-webrtc wrote:
> This probably needs review by WTC.

This is a copy of existing code (I changed the tail part only.)

Here's the original:
https://dxr.mozilla.org/mozilla-central/rev/b6acf4d4fc20431a8ae14bf32cdc6e43a...

https://codereview.appspot.com/284710043/diff/20001/lib/ssl/sslcert.c#newcode402
lib/ssl/sslcert.c:402: }
On 2016/03/05 21:31:48, ekr-webrtc wrote:
> you really want to destroy the pubkey? That seems unfortunate. I would make
the
> caller destroy it. Or at least make a comment *here*.

Done.

https://codereview.appspot.com/284710043/diff/20001/lib/ssl/sslcert.c#newcode510
lib/ssl/sslcert.c:510: }
On 2016/03/05 21:31:48, ekr-webrtc wrote:
> If you make this public, you can omit several places where you explicitly make
> dummy slots.

Done.  I went one step further and defined a version of ssl_FindServerCert that
took an authType argument.

https://codereview.appspot.com/284710043/diff/20001/lib/ssl/sslcert.c#newcode814
lib/ssl/sslcert.c:814: 
On 2016/03/05 21:31:48, ekr-webrtc wrote:
> Can we make a macro or function pointer thing for:
> 
> SetFooInSlot() that lets us span both timestamps and ocsp.

I know that a template would be ideal here, but there are enough variations in
these that it would be non-trivial, and a macro would be pukey.

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

https://codereview.appspot.com/284710043/diff/20001/lib/ssl/sslimpl.h#newcode393
lib/ssl/sslimpl.h:393: ** parameters further narrow the slot.
On 2016/03/05 21:31:49, ekr-webrtc wrote:
> slot seems like a very unfortunate name given the use of slot for PK11

I thought the same thing, but failed to find a different option.  Taking
donations.

https://codereview.appspot.com/284710043/diff/20001/lib/ssl/sslimpl.h#newcode...
lib/ssl/sslimpl.h:1985: #define TLS_EX_SESS_TICKET_VERSION (0x0103)
On 2016/03/05 21:31:49, ekr-webrtc wrote:
> Why not 0x0102

I believe that I missed the 2 and hit the 3.

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

https://codereview.appspot.com/284710043/diff/20001/lib/ssl/sslsnce.c#newcode639
lib/ssl/sslsnce.c:639: break;
On 2016/03/05 21:31:49, ekr-webrtc wrote:
> Can't you just memcpy?

I am fairly sure that these are different sizes.

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

https://codereview.appspot.com/284710043/diff/20001/lib/ssl/sslt.h#newcode95
lib/ssl/sslt.h:95: ssl_auth_rsa_decrypt = 1,
On 2016/03/05 21:31:49, ekr-webrtc wrote:
> I assume this is static RSA? Needs a comment.

Done.

https://codereview.appspot.com/284710043/diff/20001/lib/ssl/sslt.h#newcode97
lib/ssl/sslt.h:97: ssl_auth_kea = 3, /* unused */
On 2016/03/05 21:31:49, ekr-webrtc wrote:
> Why not just remove this? Since these are all explicit assignments, that
should
> be fine.

I considered it, but someone might have it in a switch statement, and we do sort
of promise not to break that.

https://codereview.appspot.com/284710043/diff/20001/lib/ssl/sslt.h#newcode101
lib/ssl/sslt.h:101: ssl_auth_rsa_pss = 7,
On 2016/03/05 21:31:49, ekr-webrtc wrote:
> You seem to have created an ABI mismatch, because now ECDHE_RSA will return
> ssl_auth_rsa_sign whereas before it returned value #1, now
ssl_auth_rsa_decrypt.

This is a part of the question I asked on the list before.  I have done worse
than this too, because ssl_auth_ecdh is now used for ECDH_ suites.

I think that this is OK, because we don't actually consume the ssl_auth_* values
until this patch arrived, but I agree that if someone is making decisions based
on the values we produce, we might be in trouble.

I'll open a thread.

https://codereview.appspot.com/284710043/diff/20001/lib/ssl/sslt.h#newcode106
lib/ssl/sslt.h:106: #define ssl_auth_rsa ssl_auth_rsa_sign
On 2016/03/05 21:31:49, ekr-webrtc wrote:
> Please scrub for every use of this in the .c files. I only saw it one place
> but...

You did?  Because I compiled without this line here.  I'll try again.

[mt, 2016-03-22 05:49:42]

Tim, ekr, can you sanity check this for be please?

I've highlighted areas that you should look at, since rietveld has picked up all
the changes from trunk since I started.

I remember that ekr asked me to get review from Bob, so I'll do that in
parallel.  There isn't any rush here.

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

https://codereview.appspot.com/284710043/diff/40001/lib/ssl/ssl.h#newcode817
lib/ssl/ssl.h:817: const SSLExtraServerCertData *data, unsigned int data_len);
Note: I've added a length, so that we can check that the data provided is
correct length.

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

https://codereview.appspot.com/284710043/diff/40001/lib/ssl/ssl3con.c#newcode329
lib/ssl/ssl3con.c:329: {kea_ecdh_ecdsa,     ssl_kea_ecdh, ssl_sign_null,
ssl_auth_ecdh, PR_FALSE,   0, PR_FALSE, PR_FALSE, SEC_OID_TLS_ECDH_ECDSA},
These suites don't sign, so ssl_sign_null is correct.

https://codereview.appspot.com/284710043/diff/40001/lib/ssl/ssl3con.c#newcode334
lib/ssl/ssl3con.c:334: {kea_ecdhe_psk,      ssl_kea_ecdh, ssl_sign_psk,
ssl_auth_psk, PR_FALSE, 0, PR_FALSE, PR_TRUE, SEC_OID_TLS_ECDHE_PSK}
Note: I will change this to ssl_kea_ecdhe_psk so that we have a distinct KEA
type (I just forgot...)

https://codereview.appspot.com/284710043/diff/40001/lib/ssl/ssl3con.c#newcode501
lib/ssl/ssl3con.c:501: CKM_ECDH1_DERIVE /* ssl_auth_ecdh_psk */
This (and the above) are new.

https://codereview.appspot.com/284710043/diff/40001/lib/ssl/ssl3con.c#newcode...
lib/ssl/ssl3con.c:8311: /* Select a cipher suite.
This block is all ekr, who forgot to run clang-format.

https://codereview.appspot.com/284710043/diff/40001/lib/ssl/ssl3con.c#newcode...
lib/ssl/ssl3con.c:8545: if (cert->slot.authType != kea_def->authKeyType) {
This block changed based on review comments.

https://codereview.appspot.com/284710043/diff/40001/lib/ssl/ssl3con.c#newcode...
lib/ssl/ssl3con.c:9074: }
This is new.  Rather than break badly when we can't find the cert, this checks
that the cert is present before allowing resumption.

https://codereview.appspot.com/284710043/diff/40001/lib/ssl/ssl3con.c#newcode...
lib/ssl/ssl3con.c:9534: }
I had to add this.

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

https://codereview.appspot.com/284710043/diff/40001/lib/ssl/ssl3ecc.c#newcode...
lib/ssl/ssl3ecc.c:1125: if (!ecdhCert || !ecdhCert->serverCert) {
Changed based on review.

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

https://codereview.appspot.com/284710043/diff/40001/lib/ssl/ssl3ext.c#newcode...
lib/ssl/ssl3ext.c:1403: PORT_Assert(aes_key);
I restored these asserts, because the PSK patch removed them accidentally.

https://codereview.appspot.com/284710043/diff/40001/lib/ssl/ssl3ext.c#newcode...
lib/ssl/ssl3ext.c:1549: /* Generic ticket processing code, common to TLS 1.0-1.2
and
Most of this is TLS 1.3 PSK, but I'll mark the bits that are relevant.

https://codereview.appspot.com/284710043/diff/40001/lib/ssl/ssl3ext.c#newcode...
lib/ssl/ssl3ext.c:1815: /* Read certificate slot */
This bit.

https://codereview.appspot.com/284710043/diff/40001/lib/ssl/ssl3ext.c#newcode...
lib/ssl/ssl3ext.c:1937: sizeof(sslServerCertSlot));
This bit.

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

https://codereview.appspot.com/284710043/diff/40001/lib/ssl/sslinfo.c#newcode165
lib/ssl/sslinfo.c:165: #define K_ECDHE_PSK "ECDHE-PSK", ssl_kea_ecdh_psk
Changed this...

https://codereview.appspot.com/284710043/diff/40001/lib/ssl/sslinfo.c#newcode257
lib/ssl/sslinfo.c:257: { 0, CS(ECDHE_PSK_WITH_AES_128_GCM_SHA256), S_PSK,
K_ECDHE_PSK, C_AESGCM, B_128, M_AEAD_128, 1, 0, 0 },
The new one.

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

https://codereview.appspot.com/284710043/diff/40001/lib/ssl/sslt.h#newcode46
lib/ssl/sslt.h:46: ssl_kea_ecdh_psk = 5,
new

https://codereview.appspot.com/284710043/diff/40001/lib/ssl/sslt.h#newcode104
lib/ssl/sslt.h:104: ssl_auth_psk = 8,
new

https://codereview.appspot.com/284710043/diff/40001/lib/ssl/sslt.h#newcode250
lib/ssl/sslt.h:250: SSLAuthType authType;
Interesting change since last time.

What I did here was report the same boring values as before in `authAlgorithm`
above.  This one reports the updated and correct values.

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

https://codereview.appspot.com/284710043/diff/40001/lib/ssl/tls13con.c#newcod...
lib/ssl/tls13con.c:363: }
New addition to new code.

https://codereview.appspot.com/284710043/diff/40001/lib/ssl/tls13con.c#newcod...
lib/ssl/tls13con.c:494: }
I rewrote this block, it was originally a little odd:

         PRBool resumeOK = PR_FALSE;

        do {
            if (sid->version != ss->version) {
                break;
            }
            resumeOK = PR_TRUE;
        } while(0);

        if (!resumeOK) {

The change I've made matches similar logic in ssl3con.c.

https://codereview.appspot.com/284710043/diff/40001/lib/ssl/tls13con.c#newcod...
lib/ssl/tls13con.c:870: (ss->ssl3.hs.kea_def->signKeyType == ssl_sign_psk));
I've changed throughout to ssl_auth_psk rather than ssl_sign_psk.

https://codereview.appspot.com/284710043/diff/40001/lib/ssl/tls13con.c#newcod...
lib/ssl/tls13con.c:1786: if (ss->ssl3.hs.kea_def->signKeyType == ssl_sign_psk) {
probably authType as well I guess

[ekr-rietveld, 2016-03-23 18:17:10]

This patchset doesn't compile for me when applied to master

ssl3con.c:8298:18: error: no member named 'authType' in 'ssl3KEADef'
    if (kea_def->authType == ssl_sign_dsa) {
        ~~~~~~~  ^
ssl3con.c:11985:13: error: implicit declaration of function
      'ssl3_KEAAllowsSessionTicket' is invalid in C99
      [-Werror,-Wimplicit-function-declaration]
            ssl3_KEAAllowsSessionTicket(ss->ssl3.hs.suite_def->key_excha...
            ^
2 errors generated.

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

https://codereview.appspot.com/284710043/diff/20001/lib/ssl/sslcert.c#newcode157
lib/ssl/sslcert.c:157: }
I'm not sure either at this point.

https://codereview.appspot.com/284710043/diff/20001/lib/ssl/sslcert.c#newcode202
lib/ssl/sslcert.c:202: }
On 2016/03/07 12:12:54, mt wrote:
> On 2016/03/05 21:31:48, ekr-webrtc wrote:
> > Not a big fan of this populate idiom. Seems to me that you can just
> > free all the contents and then populate unconditionally
> 
> I am not a fan either, but it is necessary to maintain bugward compatibility
> with the existing stuff.  My original code just made a new server cert and
> stuffed values into it, but then I found that the tests failed.  They were
> putting OCSP responses in place before the certs and expecting the two piece
of
> info to land in the same place.

OK

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

https://codereview.appspot.com/284710043/diff/20001/lib/ssl/sslt.h#newcode106
lib/ssl/sslt.h:106: #define ssl_auth_rsa ssl_auth_rsa_sign
On 2016/03/07 12:12:55, mt wrote:
> On 2016/03/05 21:31:49, ekr-webrtc wrote:
> > Please scrub for every use of this in the .c files. I only saw it one place
> > but...
> 
> You did?  Because I compiled without this line here.  I'll try again.

May have only been in a comment.

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

https://codereview.appspot.com/284710043/diff/40002/lib/ssl/sslinfo.c#newcode157
lib/ssl/sslinfo.c:157: #define A_EXP ssl_auth_null
This seems like it is likely to create problems....

https://codereview.appspot.com/284710043/diff/40002/lib/ssl/sslinfo.c#newcode206
lib/ssl/sslinfo.c:206: { 0, CS(RSA_WITH_AES_128_GCM_SHA256), S_RSA, K_RSA,
C_AESGCM, B_128, M_AEAD_128, F_FIPS_STD, A_RSAD },
I am going to assume this is correct. Did you use a script?

[ekr-rietveld, 2016-04-01 02:26:56]

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

https://codereview.appspot.com/284710043/diff/20001/lib/ssl/ssl3con.c#newcode887
lib/ssl/ssl3con.c:887: if (cipher_alg != calg_null &&
!PK11_TokenExists(cipher_mech)) {
On 2016/03/07 12:12:53, mt wrote:
> On 2016/03/05 21:31:47, ekr-webrtc wrote:
> > Can we ever have calg_null?
> 
> Well, I didn't really think that one through, but maybe we had mac-only cipher
> suites at one point.

Or it was future-proofing. Maybe an assert is in order?

https://codereview.appspot.com/284710043/diff/20001/lib/ssl/ssl3con.c#newcode...
lib/ssl/ssl3con.c:8222: if (kea_def->is_limited && kea_def->authKeyType ==
ssl_auth_rsa_sign) {
On 2016/03/07 12:12:53, mt wrote:
> On 2016/03/05 21:31:47, ekr-webrtc wrote:
> > PORT_Assert that is_limited ==> ssl_auth_rsa_sign. instaead.
> 
> Acknowledged.

Doesn't seem to have happened

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

https://codereview.appspot.com/284710043/diff/20001/lib/ssl/ssl3ecc.c#newcode...
lib/ssl/ssl3ecc.c:1116: }
On 2016/03/07 12:12:54, mt wrote:
> On 2016/03/05 21:31:47, ekr-webrtc wrote:
> > Can you refactor this into ssl3_SelectServerCert?
> 
> I don't want to.  ssl3_SelectServerCert looks at curves.  This doesn't.  All
> this does is act in support of the function below.
> 
> Sure, this isn't a great way to do this.  We can negotiate an EC cipher suite
> and then discover that we don't have curves in common and then fail, when RSA
is
> perfectly good.  But that's not a problem I'm making worse.

Can't you just pass the "i don't care" curve?

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

https://codereview.appspot.com/284710043/diff/20001/lib/ssl/sslsnce.c#newcode639
lib/ssl/sslsnce.c:639: break;
On 2016/03/07 12:12:55, mt wrote:
> On 2016/03/05 21:31:49, ekr-webrtc wrote:
> > Can't you just memcpy?
> 
> I am fairly sure that these are different sizes.

certSlot

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

https://codereview.appspot.com/284710043/diff/40002/lib/ssl/sslinfo.c#newcode157
lib/ssl/sslinfo.c:157: #define A_EXP ssl_auth_null
On 2016/03/23 18:17:10, ekr-webrtc wrote:
> This seems like it is likely to create problems....

[mt, 2016-04-02 13:15:32]

I'll have to answer on the other one as well before I'm done.

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

https://codereview.appspot.com/284710043/diff/20001/lib/ssl/ssl3con.c#newcode879
lib/ssl/ssl3con.c:879: !PK11_TokenExists(auth_alg_defs[authType])) {
On 2016/03/05 21:31:46, ekr-webrtc wrote:
> Let's break this last test into a separate clause.

Done.

https://codereview.appspot.com/284710043/diff/20001/lib/ssl/ssl3con.c#newcode887
lib/ssl/ssl3con.c:887: if (cipher_alg != calg_null &&
!PK11_TokenExists(cipher_mech)) {
On 2016/04/01 02:26:55, ekr-webrtc wrote:
> On 2016/03/07 12:12:53, mt wrote:
> > On 2016/03/05 21:31:47, ekr-webrtc wrote:
> > > Can we ever have calg_null?
> > 
> > Well, I didn't really think that one through, but maybe we had mac-only
cipher
> > suites at one point.
> 
> Or it was future-proofing. Maybe an assert is in order?

Turns out that we want to report TLS_NULL_WITH_NULL_NULL as present.  Not that
it actually matters.

https://codereview.appspot.com/284710043/diff/20001/lib/ssl/ssl3con.c#newcode...
lib/ssl/ssl3con.c:8222: if (kea_def->is_limited && kea_def->authKeyType ==
ssl_auth_rsa_sign) {
On 2016/04/01 02:26:55, ekr-webrtc wrote:
> On 2016/03/07 12:12:53, mt wrote:
> > On 2016/03/05 21:31:47, ekr-webrtc wrote:
> > > PORT_Assert that is_limited ==> ssl_auth_rsa_sign. instaead.
> > 
> > Acknowledged.
> 
> Doesn't seem to have happened

Yep, missed it.  Done.

https://codereview.appspot.com/284710043/diff/20001/lib/ssl/ssl3con.c#newcode...
lib/ssl/ssl3con.c:8305: * they are configured in. */
On 2016/03/05 21:31:47, ekr-webrtc wrote:
> Nit: please be consistent about whether . has one space or two after.

Done.

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

https://codereview.appspot.com/284710043/diff/20001/lib/ssl/ssl3ecc.c#newcode...
lib/ssl/ssl3ecc.c:1116: }
On 2016/04/01 02:26:55, ekr-webrtc wrote:
> On 2016/03/07 12:12:54, mt wrote:
> > On 2016/03/05 21:31:47, ekr-webrtc wrote:
> > > Can you refactor this into ssl3_SelectServerCert?
> > 
> > I don't want to.  ssl3_SelectServerCert looks at curves.  This doesn't.  All
> > this does is act in support of the function below.
> > 
> > Sure, this isn't a great way to do this.  We can negotiate an EC cipher
suite
> > and then discover that we don't have curves in common and then fail, when
RSA
> is
> > perfectly good.  But that's not a problem I'm making worse.
> 
> Can't you just pass the "i don't care" curve?

Yes, that's what I do now.

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

https://codereview.appspot.com/284710043/diff/20001/lib/ssl/sslcert.c#newcode157
lib/ssl/sslcert.c:157: }
On 2016/03/23 18:17:10, ekr-webrtc wrote:
> I'm not sure either at this point.

Acknowledged.

https://codereview.appspot.com/284710043/diff/20001/lib/ssl/sslcert.c#newcode233
lib/ssl/sslcert.c:233: * set in *data, we just need to work out any extra
information. */
On 2016/03/05 21:31:48, ekr-webrtc wrote:
> |*data|

Done.

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

https://codereview.appspot.com/284710043/diff/20001/lib/ssl/sslsnce.c#newcode639
lib/ssl/sslsnce.c:639: break;
On 2016/04/01 02:26:55, ekr-webrtc wrote:
> On 2016/03/07 12:12:55, mt wrote:
> > On 2016/03/05 21:31:49, ekr-webrtc wrote:
> > > Can't you just memcpy?
> > 
> > I am fairly sure that these are different sizes.
> 
> certSlot

from doesn't have a certSlot.

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

https://codereview.appspot.com/284710043/diff/40002/lib/ssl/sslinfo.c#newcode157
lib/ssl/sslinfo.c:157: #define A_EXP ssl_auth_null
On 2016/03/23 18:17:10, ekr-webrtc wrote:
> This seems like it is likely to create problems....

Yep.  But we're fixing that hopefully.  I could make this ssl_auth_rsa_x if that
makes you happier...

https://codereview.appspot.com/284710043/diff/40002/lib/ssl/sslinfo.c#newcode206
lib/ssl/sslinfo.c:206: { 0, CS(RSA_WITH_AES_128_GCM_SHA256), S_RSA, K_RSA,
C_AESGCM, B_128, M_AEAD_128, F_FIPS_STD, A_RSAD },
On 2016/03/23 18:17:10, ekr-webrtc wrote:
> I am going to assume this is correct. Did you use a script?

Yes.  Well, a simple search and replace.  If you dig, you will see that some of
the entries are probably wrong.

[mt, 2016-04-15 04:56:31]

Updated based on Bob's review, PTAL

[ekr-rietveld, 2016-04-16 23:02:44]

See below for comments about what confuses me.

https://codereview.appspot.com/284710043/diff/90001/lib/ssl/sslcert.c
File lib/ssl/sslcert.c (right):

https://codereview.appspot.com/284710043/diff/90001/lib/ssl/sslcert.c#newcode203
lib/ssl/sslcert.c:203: PORT_SetError(SEC_ERROR_INVALID_ARGS); /* zero bits! */
Why did you remove this? Does SECKEY_Pub..() PORT_SetError()

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

https://codereview.appspot.com/284710043/diff/110001/external_tests/ssl_gtest...
external_tests/ssl_gtest/ssl_loopback_unittest.cc:343:
EXPECT_FALSE(SECITEM_ItemsAreEqual(&cert1->derCert, &cert2->derCert));
What happens if you swap types? Do we need a test?

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

https://codereview.appspot.com/284710043/diff/110001/external_tests/ssl_gtest...
external_tests/ssl_gtest/tls_agent.cc:108: if (!priv.get()) return false;  //
Leak cert.
I wonder if we should wrap this in a helper since it now appears twice.

Also, we don't leak the certa ny more.

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

https://codereview.appspot.com/284710043/diff/110001/external_tests/ssl_gtest...
external_tests/ssl_gtest/tls_connect.cc:227:
client_->DisableCiphersByKeyExchange(ssl_kea_ecdh);
Ugh. What could possibly go wrong here.

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

https://codereview.appspot.com/284710043/diff/110001/lib/ssl/ssl3con.c#newcod...
lib/ssl/ssl3con.c:8556: cert->certType.u.namedCurve)) {
This is confusing. ECDH_ECDSA certs potentially have two curves:
 
1. The one used to sign the certificate.
2. The one of the key.

Which one are you talking about? I believe it's #2. Do you ever check #1?

https://codereview.appspot.com/284710043/diff/110001/lib/ssl/sslcert.c
File lib/ssl/sslcert.c (right):

https://codereview.appspot.com/284710043/diff/110001/lib/ssl/sslcert.c#newcod...
lib/ssl/sslcert.c:531: if (data) {
So data can be NULL?

https://codereview.appspot.com/284710043/diff/110001/lib/ssl/sslcert.c#newcod...
lib/ssl/sslcert.c:675: ssl_PopulateCertType(&sc->certType, authType, cert,
keyPair);
What's the convention on who calls PORT_SetError()

https://codereview.appspot.com/284710043/diff/110001/lib/ssl/sslcert.c#newcod...
lib/ssl/sslcert.c:738: break;
This doesn't seem right. This cert can't be both RSA and ECDSA.

https://codereview.appspot.com/284710043/diff/110001/lib/ssl/sslcert.c#newcod...
lib/ssl/sslcert.c:742: PORT_SetError(SEC_ERROR_INVALID_ARGS);
Ooh... I think I would be happier if rv was uninitalized and you set it here and
then the compiler uninit warnings would catch it.

[mt, 2016-04-18 01:38:26]

PTAL

[ekr-rietveld, 2016-04-18 01:43:19]

MT, can you respond to my comments, pls, so I have context.

[mt, 2016-04-18 01:50:58]

https://codereview.appspot.com/284710043/diff/90001/lib/ssl/sslcert.c
File lib/ssl/sslcert.c (right):

https://codereview.appspot.com/284710043/diff/90001/lib/ssl/sslcert.c#newcode203
lib/ssl/sslcert.c:203: PORT_SetError(SEC_ERROR_INVALID_ARGS); /* zero bits! */
On 2016/04/16 23:02:43, ekr-webrtc wrote:
> Why did you remove this? Does SECKEY_Pub..() PORT_SetError()

I just moved it.  None of these functions called PORT_SetError(), their caller
does it instead.

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

https://codereview.appspot.com/284710043/diff/110001/external_tests/ssl_gtest...
external_tests/ssl_gtest/ssl_loopback_unittest.cc:343:
EXPECT_FALSE(SECITEM_ItemsAreEqual(&cert1->derCert, &cert2->derCert));
On 2016/04/16 23:02:43, ekr-webrtc wrote:
> What happens if you swap types? Do we need a test?

Hmm, yes.  The thing here is that you can't.  Switching types doesn't have any
effect because libssl commits to a cipher suite before doing the SNI checks and
won't change as a result.

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

https://codereview.appspot.com/284710043/diff/110001/external_tests/ssl_gtest...
external_tests/ssl_gtest/tls_agent.cc:108: if (!priv.get()) return false;  //
Leak cert.
On 2016/04/16 23:02:43, ekr-webrtc wrote:
> I wonder if we should wrap this in a helper since it now appears twice.
> 
> Also, we don't leak the certa ny more.

Done and done.

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

https://codereview.appspot.com/284710043/diff/110001/external_tests/ssl_gtest...
external_tests/ssl_gtest/tls_connect.cc:227:
client_->DisableCiphersByKeyExchange(ssl_kea_ecdh);
On 2016/04/16 23:02:43, ekr-webrtc wrote:
> Ugh. What could possibly go wrong here.

Made an EnableSingleCipher() function instead.

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

https://codereview.appspot.com/284710043/diff/110001/lib/ssl/ssl3con.c#newcod...
lib/ssl/ssl3con.c:8556: cert->certType.u.namedCurve)) {
On 2016/04/16 23:02:43, ekr-webrtc wrote:
> This is confusing. ECDH_ECDSA certs potentially have two curves:
>  
> 1. The one used to sign the certificate.
> 2. The one of the key.
> 
> Which one are you talking about? I believe it's #2. Do you ever check #1?

It's always #2.  Frankly, why anyone ever cared about #1 is a mystery to me. 
TLS shouldn't have ever cared about what the certificate is signed with.  I have
no problem carrying a separate signal for that purpose, in the same way that we
carry the entire certificate chain, but something so intimately coupled to the
operation as a cipher suite it a bit much.

I never even considered the possibility that could be confusing.  I will add a
comment in the header though.

https://codereview.appspot.com/284710043/diff/110001/lib/ssl/sslcert.c
File lib/ssl/sslcert.c (right):

https://codereview.appspot.com/284710043/diff/110001/lib/ssl/sslcert.c#newcod...
lib/ssl/sslcert.c:531: if (data) {
On 2016/04/16 23:02:44, ekr-webrtc wrote:
> So data can be NULL?

That's the idea.  I originally commented that this was "optional", though that
was incorrect because it wasn't possible to omit it.  Now it is.  I think that
makes sense for the sorts of things we're doing in WebRTC, for instance.

https://codereview.appspot.com/284710043/diff/110001/lib/ssl/sslcert.c#newcod...
lib/ssl/sslcert.c:675: ssl_PopulateCertType(&sc->certType, authType, cert,
keyPair);
On 2016/04/16 23:02:43, ekr-webrtc wrote:
> What's the convention on who calls PORT_SetError()

Ahh, yes, this code needs a few calls.

https://codereview.appspot.com/284710043/diff/110001/lib/ssl/sslcert.c#newcod...
lib/ssl/sslcert.c:738: break;
On 2016/04/16 23:02:43, ekr-webrtc wrote:
> This doesn't seem right. This cert can't be both RSA and ECDSA.

Hmm, that was lazy (and wrong) on my part.

https://codereview.appspot.com/284710043/diff/110001/lib/ssl/sslcert.c#newcod...
lib/ssl/sslcert.c:742: PORT_SetError(SEC_ERROR_INVALID_ARGS);
On 2016/04/16 23:02:43, ekr-webrtc wrote:
> Ooh... I think I would be happier if rv was uninitalized and you set it here
and
> then the compiler uninit warnings would catch it.

Done.

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

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

https://codereview.appspot.com/284710043/diff/110001/external_tests/ssl_gtest...
external_tests/ssl_gtest/ssl_loopback_unittest.cc:343:
EXPECT_FALSE(SECITEM_ItemsAreEqual(&cert1->derCert, &cert2->derCert));
On 2016/04/18 01:50:57, mt wrote:
> On 2016/04/16 23:02:43, ekr-webrtc wrote:
> > What happens if you swap types? Do we need a test?
> 
> Hmm, yes.  The thing here is that you can't.  Switching types doesn't have any
> effect because libssl commits to a cipher suite before doing the SNI checks
and
> won't change as a result.

I know. That's why I wanted a test.

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

https://codereview.appspot.com/284710043/diff/110001/external_tests/ssl_gtest...
external_tests/ssl_gtest/tls_connect.cc:227:
client_->DisableCiphersByKeyExchange(ssl_kea_ecdh);
On 2016/04/18 01:50:57, mt wrote:
> On 2016/04/16 23:02:43, ekr-webrtc wrote:
> > Ugh. What could possibly go wrong here.
> 
> Made an EnableSingleCipher() function instead.

Thanks

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

https://codereview.appspot.com/284710043/diff/110001/lib/ssl/ssl3con.c#newcod...
lib/ssl/ssl3con.c:8556: cert->certType.u.namedCurve)) {
On 2016/04/18 01:50:57, mt wrote:
> On 2016/04/16 23:02:43, ekr-webrtc wrote:
> > This is confusing. ECDH_ECDSA certs potentially have two curves:
> >  
> > 1. The one used to sign the certificate.
> > 2. The one of the key.
> > 
> > Which one are you talking about? I believe it's #2. Do you ever check #1?
> 
> It's always #2.  Frankly, why anyone ever cared about #1 is a mystery to me. 
> TLS shouldn't have ever cared about what the certificate is signed with.  I
have
> no problem carrying a separate signal for that purpose, in the same way that
we
> carry the entire certificate chain, but something so intimately coupled to the
> operation as a cipher suite it a bit much.
> 
> I never even considered the possibility that could be confusing.  I will add a
> comment in the header though.

Well, it's not the cipher suite. And SignatureAlgorithms *does* apply to the
whole chain, though surprisingly in 4492, SupportedCurves does not.

https://codereview.appspot.com/284710043/diff/110001/lib/ssl/ssl3con.c#newcod...
lib/ssl/ssl3con.c:8556: cert->certType.u.namedCurve)) {
On 2016/04/18 01:50:57, mt wrote:
> On 2016/04/16 23:02:43, ekr-webrtc wrote:
> > This is confusing. ECDH_ECDSA certs potentially have two curves:
> >  
> > 1. The one used to sign the certificate.
> > 2. The one of the key.
> > 
> > Which one are you talking about? I believe it's #2. Do you ever check #1?
> 
> It's always #2.  Frankly, why anyone ever cared about #1 is a mystery to me. 
> TLS shouldn't have ever cared about what the certificate is signed with.  I
have
> no problem carrying a separate signal for that purpose, in the same way that
we
> carry the entire certificate chain, but something so intimately coupled to the
> operation as a cipher suite it a bit much.
> 
> I never even considered the possibility that could be confusing.  I will add a
> comment in the header though.

Well, it's not the cipher suite. And SignatureAlgorithms *does* apply to the
whole chain, though surprisingly in 4492, SupportedCurves does not.

https://codereview.appspot.com/284710043/diff/110001/lib/ssl/sslcert.c
File lib/ssl/sslcert.c (right):

https://codereview.appspot.com/284710043/diff/110001/lib/ssl/sslcert.c#newcod...
lib/ssl/sslcert.c:531: if (data) {
On 2016/04/18 01:50:57, mt wrote:
> On 2016/04/16 23:02:44, ekr-webrtc wrote:
> > So data can be NULL?
> 
> That's the idea.  I originally commented that this was "optional", though that
> was incorrect because it wasn't possible to omit it.  Now it is.  I think that
> makes sense for the sorts of things we're doing in WebRTC, for instance.

Why?

https://codereview.appspot.com/284710043/diff/110001/lib/ssl/sslcert.c#newcod...
lib/ssl/sslcert.c:738: break;
On 2016/04/18 01:50:57, mt wrote:
> On 2016/04/16 23:02:43, ekr-webrtc wrote:
> > This doesn't seem right. This cert can't be both RSA and ECDSA.
> 
> Hmm, that was lazy (and wrong) on my part.

Much better.

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

https://codereview.appspot.com/284710043/diff/130001/external_tests/ssl_gtest...
external_tests/ssl_gtest/ssl_loopback_unittest.cc:311: if (!ok) return
SSL_SNI_SEND_ALERT;
Why don't you want to update the key bits here?

https://codereview.appspot.com/284710043/diff/130001/external_tests/ssl_gtest...
external_tests/ssl_gtest/ssl_loopback_unittest.cc:343:
server_->SetSniCallback(SwitchCertificates);
Well, the impact is that you now have two certs, right?

https://codereview.appspot.com/284710043/diff/130001/lib/ssl/sslcert.c
File lib/ssl/sslcert.c (right):

https://codereview.appspot.com/284710043/diff/130001/lib/ssl/sslcert.c#newcod...
lib/ssl/sslcert.c:904: PORT_SetError(SEC_ERROR_INVALID_ARGS);
Not necessary. SeE:
https://dxr.mozilla.org/mozilla-central/source/security/nss/lib/ssl/sslsock.c...

[mt, 2016-04-19 00:20:42]

Small correctness updates

[rrelyea, 2016-04-21 21:02:23]

Looks good now r+ rrelyea. 

I have a few very minor comments. none serious enough to prevent the patch from
being checked in as is.

https://codereview.appspot.com/284710043/diff/150001/lib/pk11wrap/pk11auth.c
File lib/pk11wrap/pk11auth.c (right):

https://codereview.appspot.com/284710043/diff/150001/lib/pk11wrap/pk11auth.c#...
lib/pk11wrap/pk11auth.c:316: }
I'm not sure why this was necessary, But it is a fine change.

bob

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

https://codereview.appspot.com/284710043/diff/150001/lib/ssl/ssl3con.c#newcod...
lib/ssl/ssl3con.c:484: 0x80000000L, /* ssl_auth_null */
probably CKM_INVALID_MECHANISM would be better

https://codereview.appspot.com/284710043/diff/150001/lib/ssl/ssl3con.c#newcod...
lib/ssl/ssl3con.c:487: 0x80000000L, /* ssl_auth_kea (unused) */
See above

https://codereview.appspot.com/284710043/diff/150001/lib/ssl/sslcert.c
File lib/ssl/sslcert.c (right):

https://codereview.appspot.com/284710043/diff/150001/lib/ssl/sslcert.c#newcod...
lib/ssl/sslcert.c:456: }
Yes, this is much clearer! (and correct now):).