Pass in a non-null CertVerifier into SSLClientSocket.

Pass in a non-null CertVerifier into SSLClientSocket.

SslHmacChannelAuthenticator passes in a null one which crashes but the
IsAllowedBadCert check, as well as inconsistent ability to use X509Certificate
in the sandbox masks the issue most of the time.

This also fixes FakeStreamSocket to propogate disconnects to the peer, which is
needed to add a test for this case. (If SSLClientSocket doesn't like a
certificate, it just ceremoniously disconnects the connection right after the
handshake.) This test crashed before this CL outside the sandbox. (Inside the
sandbox, it's possible that it worked on some platforms due to the sandbox
breaking net::X509Certificate. I didn't do a survey.)

BUG=none

Committed: https://crrev.com/9bbf3290520788bfe634a286a30965cd43f4dd78
Cr-Commit-Position: refs/heads/master@{#326886}

[davidben, 2015-04-23 19:38:14]

davidben@chromium.org changed reviewers:
+ rsleevi@chromium.org, sergeyu@chromium.org

[davidben, 2015-04-23 19:38:15]

https://codereview.chromium.org/1080593003/diff/20001/remoting/protocol/fake_...
File remoting/protocol/fake_stream_socket.h (right):

https://codereview.chromium.org/1080593003/diff/20001/remoting/protocol/fake_...
remoting/protocol/fake_stream_socket.h:50: void set_next_write_error(int error)
{ next_write_error_ = error; }
It'd probably be nice if this also let you interrupt the current write, but I
just left it alone for now.

[Ryan Sleevi, 2015-04-23 20:25:14]

//net LGTM

[Sergey Ulanov, 2015-04-24 00:34:45]

https://codereview.chromium.org/1080593003/diff/20001/remoting/protocol/fake_...
File remoting/protocol/fake_stream_socket.cc (right):

https://codereview.chromium.org/1080593003/diff/20001/remoting/protocol/fake_...
remoting/protocol/fake_stream_socket.cc:55: // Complete pending ead if any.
typo: ead

https://codereview.chromium.org/1080593003/diff/20001/remoting/protocol/ssl_h...
File remoting/protocol/ssl_hmac_channel_authenticator.cc (right):

https://codereview.chromium.org/1080593003/diff/20001/remoting/protocol/ssl_h...
remoting/protocol/ssl_hmac_channel_authenticator.cc:129:
cert_verifier_.reset(new FailingCertVerifier);
On the client side we actually don't care about the cert received from the host.
SSL here is used for encryption/key exchange, but not authentication.
Authentication is based on the extra layer that exchanges HMAC tokens based on
the keys extracted from the negotiated SSL master key and a shared key (which
exchanged prior to establishing SSL connection). So instead of
FailingCertVerifier we need a CertVerifier that accepts any certs.

Currently it works by adding remote_cert_ to ssl_config.allowed_bad_certs. The
(ugly) reason it was implemented this way instead of creating a special
CertVerifier is because net::X509Certificate* can't be created in the renderer
sandbox (because it depends on system APIs that don't work in sandbox) and this
code runs in sandbox. In other words CertVerifier is not useful here because
X509Certificate creation normally fails - that's the reason we never bothered
creating CertVerifier here. It will not be a problem in the future after we
finished migrating to PNaCl (net::X509Certificate works properly in PNaCl
sandbox as it doesn't depend on system APIs in that case), at which point it
will be possible to remove the allowed_bad_certs hack, but we will want
CertVerifier that accept all certs for that.

Also we are planning to migrate to QUIC, and so this file will be removed
completely in the future.

https://codereview.chromium.org/1080593003/diff/20001/remoting/protocol/ssl_h...
File remoting/protocol/ssl_hmac_channel_authenticator_unittest.cc (right):

https://codereview.chromium.org/1080593003/diff/20001/remoting/protocol/ssl_h...
remoting/protocol/ssl_hmac_channel_authenticator_unittest.cc:183:
TEST_F(SslHmacChannelAuthenticatorTest, InvalidCertificate) {
This test is not useful - see my previous comment. The security guarantees
provided by this class do not depend on the cert used by the host.

[Sergey Ulanov, 2015-04-24 20:03:22]

LGTM after offline discussion

[davidben, 2015-04-24 20:52:44]

https://codereview.chromium.org/1080593003/diff/20001/remoting/protocol/fake_...
File remoting/protocol/fake_stream_socket.cc (right):

https://codereview.chromium.org/1080593003/diff/20001/remoting/protocol/fake_...
remoting/protocol/fake_stream_socket.cc:55: // Complete pending ead if any.
On 2015/04/24 00:34:45, Sergey Ulanov wrote:
> typo: ead

Done.

https://codereview.chromium.org/1080593003/diff/20001/remoting/protocol/ssl_h...
File remoting/protocol/ssl_hmac_channel_authenticator.cc (right):

https://codereview.chromium.org/1080593003/diff/20001/remoting/protocol/ssl_h...
remoting/protocol/ssl_hmac_channel_authenticator.cc:129:
cert_verifier_.reset(new FailingCertVerifier);
Leaving as-is per offline discussion.

https://codereview.chromium.org/1080593003/diff/20001/remoting/protocol/ssl_h...
File remoting/protocol/ssl_hmac_channel_authenticator_unittest.cc (right):

https://codereview.chromium.org/1080593003/diff/20001/remoting/protocol/ssl_h...
remoting/protocol/ssl_hmac_channel_authenticator_unittest.cc:183:
TEST_F(SslHmacChannelAuthenticatorTest, InvalidCertificate) {
Leaving as-is per offline discussion.

[davidben, 2015-04-24 20:52:49]

The CQ bit was checked by davidben@chromium.org

[davidben, 2015-04-24 20:52:50]

The patchset sent to the CQ was uploaded after l-g-t-m from
sergeyu@chromium.org, rsleevi@chromium.org
Link to the patchset: https://codereview.chromium.org/1080593003/#ps40001
(title: "fix comment typo")

[davidben, 2015-04-24 20:53:15]

The CQ bit was unchecked by davidben@chromium.org

[davidben, 2015-04-24 20:53:43]

The CQ bit was checked by davidben@chromium.org

[commit-bot: I haz the power, 2015-04-24 20:54:47]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1080593003/40001

[commit-bot: I haz the power, 2015-04-24 21:50:15]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-04-24 21:51:03]

Patchset 3 (id:??) landed as
https://crrev.com/9bbf3290520788bfe634a286a30965cd43f4dd78
Cr-Commit-Position: refs/heads/master@{#326886}