Move SSL layer initialization into ChannelAuthenticator implementations.

remoting/protocol/simple_client_channel_authenticator.cc

Index: remoting/protocol/simple_client_channel_authenticator.cc
diff --git a/remoting/protocol/simple_client_channel_authenticator.cc b/remoting/protocol/simple_client_channel_authenticator.cc
new file mode 100644
index 0000000000000000000000000000000000000000..6af4186bf1b7b0205f56586b695f24335bec7a1c
--- /dev/null
+++ b/remoting/protocol/simple_client_channel_authenticator.cc
@@ -0,0 +1,146 @@
+// Copyright (c) 2011 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "remoting/protocol/simple_client_channel_authenticator.h"
+
+#include "net/base/cert_verifier.h"
+#include "net/base/host_port_pair.h"
+#include "net/base/io_buffer.h"
+#include "net/base/net_errors.h"
+#include "net/base/ssl_config_service.h"
+#include "net/socket/client_socket_factory.h"
+#include "net/socket/ssl_client_socket.h"
+#include "remoting/protocol/auth_util.h"
+
+namespace remoting {
+namespace protocol {
+
+namespace {
+
+// Helper method to create a SSL client socket.
+net::SSLClientSocket* CreateSSLClientSocket(

[Wez, 2011/11/22 22:29:48]

Is it really worth having this a separate method from SecureAndAuthenticate?

[Sergey Ulanov, 2011/11/23 01:23:42]

Done.

+    net::StreamSocket* socket, const std::string& der_cert,
+    net::CertVerifier* cert_verifier) {
+  net::SSLConfig ssl_config;
+
+  // Certificate provided by the host doesn't need authority.

[Wez, 2011/11/22 22:29:48]

Isn't this also to do with being self-signed, as below?

[Sergey Ulanov, 2011/11/23 01:23:42]

Done.

+  net::SSLConfig::CertAndStatus cert_and_status;
+  cert_and_status.cert_status = net::CERT_STATUS_AUTHORITY_INVALID;
+  cert_and_status.der_cert = der_cert;
+  ssl_config.allowed_bad_certs.push_back(cert_and_status);
+
+  // Revocation checking is not needed because we use self-signed
+  // certs. Disable it so that SSL layer doesn't try to initialize

[Wez, 2011/11/22 22:29:48]

nit: that _the_ SSL layer

[Sergey Ulanov, 2011/11/23 01:23:42]

Done.

+  // OCSP (OCSP works only on IO thread).

[Wez, 2011/11/22 22:29:48]

nit: on _the_ IO thread

[Sergey Ulanov, 2011/11/23 01:23:42]

Done.

+  ssl_config.rev_checking_enabled = false;
+
+  // SSLClientSocket takes ownership of the adapter.

[Wez, 2011/11/22 22:29:48]

nit: What adapter?

[Sergey Ulanov, 2011/11/23 01:23:42]

Done.

+  net::HostPortPair host_and_port(kSslFakeHostName, 0);
+  net::SSLClientSocketContext context;
+  context.cert_verifier = cert_verifier;
+  net::SSLClientSocket* ssl_socket =
+      net::ClientSocketFactory::GetDefaultFactory()->CreateSSLClientSocket(
+          socket, host_and_port, ssl_config, NULL, context);
+  return ssl_socket;
+}
+
+}  // namespace
+
+SimpleClientChannelAuthenticator::SimpleClientChannelAuthenticator(
+    const std::string& remote_cert,

[Wez, 2011/11/22 22:29:48]

nit: |remote_cert| -> |peer_cert|?

[Sergey Ulanov, 2011/11/23 01:23:42]

Changed it to host_cert as it is more specific.

+    const std::string& shared_secret)
+    : remote_cert_(remote_cert),
+      shared_secret_(shared_secret),
+      socket_(NULL),
+      ALLOW_THIS_IN_INITIALIZER_LIST(connect_callback_(
+          this, &SimpleClientChannelAuthenticator::OnConnected)),
+      ALLOW_THIS_IN_INITIALIZER_LIST(auth_write_callback_(
+          this, &SimpleClientChannelAuthenticator::OnAuthBytesWritten)) {
+}
+
+SimpleClientChannelAuthenticator::~SimpleClientChannelAuthenticator() {
+}
+
+void SimpleClientChannelAuthenticator::SecureAndAuthenticate(
+    net::StreamSocket* socket, const DoneCallback& done_callback) {
+  DCHECK(CalledOnValidThread());
+
+  done_callback_ = done_callback;
+
+  cert_verifier_.reset(new net::CertVerifier());

[Wez, 2011/11/22 22:29:48]

Why do we pass in a standard CertVerifier if we never verify certificates?

[Sergey Ulanov, 2011/11/23 01:23:42]

This is because SSLClientSocket still expects to get CertVerifier instance :(
Certificate verification will fail even if we get CA-signed cert because this
code runs in sandbox and X509Certificate work in sandbox. 

Ideally CertVerifier should be an interface, and we would have our own
implementation that accepts only one cert - hopefully I'll fix this some day.

+  socket_.reset(CreateSSLClientSocket(
+      socket, remote_cert_, cert_verifier_.get()));
+
+  int result = socket_->Connect(&connect_callback_);

[Wez, 2011/11/22 22:29:48]

It seems strange for connecting the SteramSocket to be done here; it's neither
securing nor authenticating?

[Sergey Ulanov, 2011/11/23 01:23:42]

This connects SSL layer, not the socket itself. This method is expected to be
called with a connected socket - added DCHECK for this.

+  if (result == net::ERR_IO_PENDING) {
+    return;
+  }
+  OnConnected(result);
+}
+
+void SimpleClientChannelAuthenticator::OnConnected(int result) {
+  if (result != net::OK) {
+    LOG(ERROR) << "Failed to establish SSL connection";

[Wez, 2011/11/22 22:29:48]

nit: I don't think this needs logging here; calling code can decide what to log?

[Sergey Ulanov, 2011/11/23 01:23:42]

This log message should be very rare, and I think it's better to log it here so
we can distinguish between connection and read errors.

+    done_callback_.Run(static_cast<net::Error>(result), NULL);
+  }
+
+  unsigned char key_material[kAuthDigestLength];
+  int export_result = socket_->ExportKeyingMaterial(
+      kClientAuthSslExporterLabel, "", key_material, kAuthDigestLength);
+  if (export_result != net::OK) {
+    LOG(ERROR) << "Error fetching keying material: " << export_result;

[Wez, 2011/11/22 22:29:48]

nit: See above.

[Sergey Ulanov, 2011/11/23 01:23:42]

same here

+    done_callback_.Run(static_cast<net::Error>(export_result), NULL);
+    return;
+  }
+
+  std::string auth_bytes;
+  if (!GetAuthBytes(shared_secret_,
+                    std::string(key_material, key_material + kAuthDigestLength),

[Wez, 2011/11/22 22:29:48]

Why not just std::string(key_material, kAuthDigestLength)?

[Sergey Ulanov, 2011/11/23 01:23:42]

key_material is unsigned int, so std::string(key_material, kAuthDigestLength)
would be ambiguous. Changed it to
std::string(reinterpret_cast<char*>(key_material), kAuthDigestLength).

+                    &auth_bytes)) {
+    done_callback_.Run(net::ERR_FAILED, NULL);
+    return;
+  }
+
+  // Allocate a buffer to write the authentication digest.

[Wez, 2011/11/22 22:29:48]

nit: This function doesn't have comments on earlier code blocks, which arguably
would benefit more from explanation than this one!

[Sergey Ulanov, 2011/11/23 01:23:42]

Done.

+  auth_write_buf_ = new net::DrainableIOBuffer(
+      new net::StringIOBuffer(auth_bytes), auth_bytes.size());
+  DoAuthWrite();
+}
+
+void SimpleClientChannelAuthenticator::DoAuthWrite() {

[Wez, 2011/11/22 22:29:48]

nit: DoAuthWrite -> WriteAuthenticationBytes() or similar?

[Sergey Ulanov, 2011/11/23 01:23:42]

Done.

+  while (true) {
+    int result = socket_->Write(auth_write_buf_,
+                                auth_write_buf_->BytesRemaining(),
+                                &auth_write_callback_);
+    if (result == net::ERR_IO_PENDING)
+      break;
+    if (!HandleAuthBytesWritten(result))
+      break;
+  }
+}
+
+void SimpleClientChannelAuthenticator::OnAuthBytesWritten(int result) {
+  DCHECK(CalledOnValidThread());
+
+  if (HandleAuthBytesWritten(result))
+    DoAuthWrite();
+}
+
+bool SimpleClientChannelAuthenticator::HandleAuthBytesWritten(int result) {
+  if (result <= 0) {
+    LOG(ERROR) << "Error writing authentication: " << result;
+    done_callback_.Run(static_cast<net::Error>(result), NULL);
+    return false;
+  }
+
+  auth_write_buf_->DidConsume(result);
+  if (auth_write_buf_->BytesRemaining() > 0)
+    return true;
+
+  done_callback_.Run(net::OK, socket_.release());
+  return false;
+}
+
+}  // namespace protocol
+}  // namespace remoting