Fix SSLClientSocketOpenSSL error-handling for Channel ID.

Fix SSLClientSocketOpenSSL error-handling for Channel ID.

The old logic did not propogate asynchronous errors forward and potentially may
trigger two GetOrCreateDomainBoundCert calls in parallel if DoHandshake were
called while a Channel ID request were in flight.

Add a test to ensure this case behaves as expected.

BUG=386276
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=278947

[davidben, 2014-06-19 17:56:20]

asvitkine for histograms.xml.

I made this add new states to SSL_do_handshake since it looks like returning
something in the callback is equivalent to waiting for OpenSSL to tell us it
needs something and calling SSL_set1_tls_channel_id and that's how the NSS
socket does it.

I have another version that fixes it by keeping the callback and maintaining
state so the callback can internally distinguish between:
- No channel ID request in flight; fire one off.
- Channel ID request in flight but not yet completed; continue to return
ERR_IO_PENDING.
- Channel ID request completed but not yet consumed by OpenSSL; report the key
or the error.
But this avoids channel_id_request_return_value_ so it seems cleaner.

histograms.xml generated with tools/metrics/histograms/update_net_error_codes.py
which is why it includes other error codes. Forgot to email net-dev@ about that
one... will go do that...

[Alexei Svitkine (slow), 2014-06-19 18:28:35]

histograms lgtm

[wtc, 2014-06-19 22:00:11]

Patch set 2 LGTM.

https://codereview.chromium.org/338093012/diff/20001/net/base/net_error_list.h
File net/base/net_error_list.h (right):

https://codereview.chromium.org/338093012/diff/20001/net/base/net_error_list....
net/base/net_error_list.h:736: NET_ERROR(CHANNEL_ID_IMPORT_FAILED, -715)

Should we change ssl_client_socket_nss.cc to use this error code? Right now it
is mapping the NSS error code:

  SECItem cert_item;
  cert_item.data = (unsigned char*) domain_bound_cert_.data();
  cert_item.len = domain_bound_cert_.size();
  ScopedCERTCertificate cert(CERT_NewTempCertificate(CERT_GetDefaultCertDB(),
                                                     &cert_item,
                                                     NULL,
                                                     PR_FALSE,
                                                     PR_TRUE));
  if (cert == NULL)
    return MapNSSError(PORT_GetError());

and

  crypto::ScopedPK11Slot slot(PK11_GetInternalSlot());
  // Set the private key.
  if (!crypto::ECPrivateKey::ImportFromEncryptedPrivateKeyInfo(
          slot.get(),
          ServerBoundCertService::kEPKIPassword,
          reinterpret_cast<const unsigned char*>(
              domain_bound_private_key_.data()),
          domain_bound_private_key_.size(),
          &cert->subjectPublicKeyInfo,
          false,
          false,
          key,
          public_key)) {
    int error = MapNSSError(PORT_GetError());
    return error;
  }

https://codereview.chromium.org/338093012/diff/20001/net/socket/ssl_client_so...
File net/socket/ssl_client_socket_openssl.cc (right):

https://codereview.chromium.org/338093012/diff/20001/net/socket/ssl_client_so...
net/socket/ssl_client_socket_openssl.cc:829: } else {

Nit: remove the "else" because now the "if" branch ends with a return statement.

https://codereview.chromium.org/338093012/diff/20001/net/socket/ssl_client_so...
net/socket/ssl_client_socket_openssl.cc:849: channel_id_xtn_negotiated_ = true;

Nit: this probably can be moved to line 827.

https://codereview.chromium.org/338093012/diff/20001/net/socket/ssl_client_so...
net/socket/ssl_client_socket_openssl.cc:879: subject_public_key_info));

Nit: indentation is wrong.

https://codereview.chromium.org/338093012/diff/20001/net/socket/ssl_client_so...
net/socket/ssl_client_socket_openssl.cc:885: SSL_set1_tls_channel_id(ssl_,
ec_private_key->key());

I assume SSL_set1_tls_channel_id will call EVP_PKEY_dup on its second argument?

https://codereview.chromium.org/338093012/diff/20001/net/socket/ssl_client_so...
net/socket/ssl_client_socket_openssl.cc:1044: DCHECK(rv == OK);

Please also change this to DCHECK_EQ(OK, rv) for consistency.

https://codereview.chromium.org/338093012/diff/20001/net/socket/ssl_client_so...
File net/socket/ssl_client_socket_unittest.cc (right):

https://codereview.chromium.org/338093012/diff/20001/net/socket/ssl_client_so...
net/socket/ssl_client_socket_unittest.cc:2658: // Connect to a server using
channel id but failing to query the store. It

Nit: "failing to query the store" sounds like we don't query the store. Perhaps
"failing to look up the Channel ID" would be better?

https://codereview.chromium.org/338093012/diff/20001/net/ssl/server_bound_cer...
File net/ssl/server_bound_cert_service.cc (right):

https://codereview.chromium.org/338093012/diff/20001/net/ssl/server_bound_cer...
net/ssl/server_bound_cert_service.cc:519: // Async lookup failed. If lookup gave
an error or failed to find a valid cert

What does "lookup gave an error" mean? It seems to be referring to the test "err
!= ERR_FILE_NOT_FOUND", but I don't understand why "err != ERR_FILE_NOT_FOUND"
means lookup gave an error. Is ERR_FILE_NOT_FOUND not considered an error? Or
does ERR_FILE_NOT_FOUND actually mean "cert not found"?

[davidben, 2014-06-19 22:36:38]

https://codereview.chromium.org/338093012/diff/20001/net/base/net_error_list.h
File net/base/net_error_list.h (right):

https://codereview.chromium.org/338093012/diff/20001/net/base/net_error_list....
net/base/net_error_list.h:736: NET_ERROR(CHANNEL_ID_IMPORT_FAILED, -715)
On 2014/06/19 22:00:11, wtc wrote:
> 
> Should we change ssl_client_socket_nss.cc to use this error code? Right now it
> is mapping the NSS error code:
> 
>   SECItem cert_item;
>   cert_item.data = (unsigned char*) domain_bound_cert_.data();
>   cert_item.len = domain_bound_cert_.size();
>   ScopedCERTCertificate cert(CERT_NewTempCertificate(CERT_GetDefaultCertDB(),
>                                                      &cert_item,
>                                                      NULL,
>                                                      PR_FALSE,
>                                                      PR_TRUE));
>   if (cert == NULL)
>     return MapNSSError(PORT_GetError());
> 
> and
> 
>   crypto::ScopedPK11Slot slot(PK11_GetInternalSlot());
>   // Set the private key.
>   if (!crypto::ECPrivateKey::ImportFromEncryptedPrivateKeyInfo(
>           slot.get(),
>           ServerBoundCertService::kEPKIPassword,
>           reinterpret_cast<const unsigned char*>(
>               domain_bound_private_key_.data()),
>           domain_bound_private_key_.size(),
>           &cert->subjectPublicKeyInfo,
>           false,
>           false,
>           key,
>           public_key)) {
>     int error = MapNSSError(PORT_GetError());
>     return error;
>   }

We could. Although, unlike the function OpenSSL is using, the NSS one preserves
the NSS error code. In theory it could get a more detailed error if we mapped
the error code. I don't have preferences as to whether we should switch to this
one.

https://codereview.chromium.org/338093012/diff/20001/net/socket/ssl_client_so...
File net/socket/ssl_client_socket_openssl.cc (right):

https://codereview.chromium.org/338093012/diff/20001/net/socket/ssl_client_so...
net/socket/ssl_client_socket_openssl.cc:829: } else {
On 2014/06/19 22:00:11, wtc wrote:
> 
> Nit: remove the "else" because now the "if" branch ends with a return
statement.

Done.

https://codereview.chromium.org/338093012/diff/20001/net/socket/ssl_client_so...
net/socket/ssl_client_socket_openssl.cc:849: channel_id_xtn_negotiated_ = true;
On 2014/06/19 22:00:11, wtc wrote:
> 
> Nit: this probably can be moved to line 827.

Done.

https://codereview.chromium.org/338093012/diff/20001/net/socket/ssl_client_so...
net/socket/ssl_client_socket_openssl.cc:879: subject_public_key_info));
On 2014/06/19 22:00:11, wtc wrote:
> 
> Nit: indentation is wrong.

Done.

https://codereview.chromium.org/338093012/diff/20001/net/socket/ssl_client_so...
net/socket/ssl_client_socket_openssl.cc:885: SSL_set1_tls_channel_id(ssl_,
ec_private_key->key());
On 2014/06/19 22:00:11, wtc wrote:
> 
> I assume SSL_set1_tls_channel_id will call EVP_PKEY_dup on its second
argument?

Yeah. I think set1 vs set may be the OpenSSL convention for "takes a reference"?
Anyway, it hits this function:
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/openss...

For completeness, this is equivalent to what we were doing before because
tlsext_channel_id_enabled was already set by SSL_enable_tls_channel_id:

https://code.google.com/p/chromium/codesearch#chromium/src/third_party/openss...

And this is the code that calls the callback:

https://code.google.com/p/chromium/codesearch#chromium/src/third_party/openss...

I suppose for completeness, we really should be checking the return value there,
in case OpenSSL rejects it for being the wrong size. Fixed.

https://codereview.chromium.org/338093012/diff/20001/net/socket/ssl_client_so...
net/socket/ssl_client_socket_openssl.cc:1044: DCHECK(rv == OK);
On 2014/06/19 22:00:11, wtc wrote:
> 
> Please also change this to DCHECK_EQ(OK, rv) for consistency.

Done.

https://codereview.chromium.org/338093012/diff/20001/net/socket/ssl_client_so...
File net/socket/ssl_client_socket_unittest.cc (right):

https://codereview.chromium.org/338093012/diff/20001/net/socket/ssl_client_so...
net/socket/ssl_client_socket_unittest.cc:2658: // Connect to a server using
channel id but failing to query the store. It
On 2014/06/19 22:00:11, wtc wrote:
> 
> Nit: "failing to query the store" sounds like we don't query the store.
Perhaps
> "failing to look up the Channel ID" would be better?

Done.

https://codereview.chromium.org/338093012/diff/20001/net/ssl/server_bound_cer...
File net/ssl/server_bound_cert_service.cc (right):

https://codereview.chromium.org/338093012/diff/20001/net/ssl/server_bound_cer...
net/ssl/server_bound_cert_service.cc:519: // Async lookup failed. If lookup gave
an error or failed to find a valid cert
On 2014/06/19 22:00:11, wtc wrote:
> 
> What does "lookup gave an error" mean? It seems to be referring to the test
"err
> != ERR_FILE_NOT_FOUND", but I don't understand why "err != ERR_FILE_NOT_FOUND"
> means lookup gave an error. Is ERR_FILE_NOT_FOUND not considered an error? Or
> does ERR_FILE_NOT_FOUND actually mean "cert not found"?

Oh yeah, I meant to explain this here. Yeah, ERR_FILE_NOT_FOUND actually means
"cert not found" here. This is to mirror the synchronous codepath:

https://code.google.com/p/chromium/codesearch#chromium/src/net/ssl/server_bou...

which handles ERR_FILE_NOT_FOUND as a signal to create a new key, but otherwise
returns the error.

DefaultServerBoundCertStore returns ERR_FILE_NOT_FOUND if the cert wasn't found:
https://code.google.com/p/chromium/codesearch#chromium/src/net/ssl/default_se...

In fact, it doesn't look like DefaultServerBoundCertStore ever returns anything
other than OK and ERR_FILE_NOT_FOUND. The
DefaultServerBoundCertStore::PersistentStore interface has no way to signal a
load error. If we get a load error in the sqlite implementation, we just drop
the database and return nothing. So this change doesn't do anything outside of
test code.

Within test code, we can only interpose on ServerBoundCertStore and not
ServerBoundCertService. Without this change, it sees the fake error and just
generates a new one. This makes the (totally unused in real code)
ServerBoundCertStore error-handling match between the synchronous and
asynchronous codepaths so we can asynchronously fail at the
ServerBoundCertService level.

Note that although asynchronously failing in ServerBoundCertStore is not
possible in our real code, it IS possible to asynchronously fail in
ServerBoundCertService if GenerateCert fails for some reason. But there isn't a
convenient way to interpose there, so I just went with this one.

---

Anyway, rephrased that comment to be clearer.

[wtc, 2014-06-20 00:21:55]

Patch set 3 LGTM. Thanks.

https://codereview.chromium.org/338093012/diff/20001/net/base/net_error_list.h
File net/base/net_error_list.h (right):

https://codereview.chromium.org/338093012/diff/20001/net/base/net_error_list....
net/base/net_error_list.h:736: NET_ERROR(CHANNEL_ID_IMPORT_FAILED, -715)

OK, let's leave ssl_client_socket_nss.cc unchanged. I will use this new error
code in my QUIC Channel ID code.

https://codereview.chromium.org/338093012/diff/40001/net/ssl/server_bound_cer...
File net/ssl/server_bound_cert_service.cc (right):

https://codereview.chromium.org/338093012/diff/40001/net/ssl/server_bound_cer...
net/ssl/server_bound_cert_service.cc:429: if (err == ERR_FILE_NOT_FOUND) {

We set create_if_missing to true on line 421 and pass that to
LookupDomainBoundCert(). Is this why we don't need to check (the equivalent of)
j->second->CreateIfMissing() here?

https://codereview.chromium.org/338093012/diff/40001/net/ssl/server_bound_cer...
net/ssl/server_bound_cert_service.cc:519: // Async lookup failed or was missing.
Return the error directly, unless the

Nit: add "the certificate" after "or".

[davidben, 2014-06-20 20:05:57]

https://codereview.chromium.org/338093012/diff/40001/net/ssl/server_bound_cer...
File net/ssl/server_bound_cert_service.cc (right):

https://codereview.chromium.org/338093012/diff/40001/net/ssl/server_bound_cer...
net/ssl/server_bound_cert_service.cc:429: if (err == ERR_FILE_NOT_FOUND) {
On 2014/06/20 00:21:54, wtc wrote:
> 
> We set create_if_missing to true on line 421 and pass that to
> LookupDomainBoundCert(). Is this why we don't need to check (the equivalent
of)
> j->second->CreateIfMissing() here?

Yeah, GetOrCreateDomainBoundCert does a create_if_missing request and
GetDomainBoundCert does not. j->second->CreateIfMissing() is I think if any
request attached to the job had create_if_missing set.

https://codereview.chromium.org/338093012/diff/40001/net/ssl/server_bound_cer...
net/ssl/server_bound_cert_service.cc:519: // Async lookup failed or was missing.
Return the error directly, unless the
On 2014/06/20 00:21:54, wtc wrote:
> 
> Nit: add "the certificate" after "or".

Done.

[davidben, 2014-06-20 22:59:04]

The CQ bit was checked by davidben@chromium.org

[commit-bot: I haz the power, 2014-06-20 23:00:05]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/davidben@chromium.org/338093012/60001

[commit-bot: I haz the power, 2014-06-21 06:13:50]

Change committed as 278947