Blimp: add support for SSL connections.

blimp/net/ssl_client_transport.h

+// Copyright 2016 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.
+
+#ifndef BLIMP_NET_SSL_CLIENT_TRANSPORT_H_
+#define BLIMP_NET_SSL_CLIENT_TRANSPORT_H_
+
+#include <string>
+
+#include "base/callback_forward.h"
+#include "base/macros.h"
+#include "base/memory/scoped_ptr.h"
+#include "blimp/net/blimp_cert_verifier.h"
+#include "blimp/net/blimp_net_export.h"
+#include "blimp/net/blimp_transport.h"
+#include "net/base/address_list.h"
+#include "net/base/net_errors.h"
+#include "net/http/transport_security_state.h"
+
+namespace net {
+class ClientSocketFactory;
+class NetLog;
+class StreamSocket;
+class TCPClientSocket;
+class TransportSecurityState;
+}  // namespace net
+
+namespace blimp {
+
+class BlimpConnection;
+
+// Creates and connects SSL socket connections to Blimplets.
+class BLIMP_NET_EXPORT SSLClientTransport : public BlimpTransport {

[Wez, 2016/02/12 21:42:38]

TCP connection logic already exists in TCPClientTransport, so you could build
SSLClientTransport as a subclass:

- move OnTCPConnectComplete to a protected member, OnConnectComplete.
- have a new OnTCPConnectComplete that is a protected virtual.
- have TCPClientTransport::OnTCPConnectComplete simple call OnConnectComplete.
- have SSLClientTransport override OnTCPConnectComplete by wrapping |socket_| w/
a new SSLStreamSocket and kicking off the SSL handshake.
- on SSL handshake completion call down to OnConnectComplete as before.

[Kevin M, 2016/02/13 00:44:18]

Done.

+ public:
+  // |addresses|: a list of addresses to connect to.
+  // |assigned_cert|: the certificate required from the remote peer.
+  //     SSL connections that use different certificates are rejected.
+  // |net_log|: the socket event log (optional).
+  SSLClientTransport(const net::AddressList& addresses,
+                     scoped_refptr<net::X509Certificate> assigned_cert,

[Wez, 2016/02/12 21:42:38]

nit: Why is "assigned" relevant here? At this level all the transport cares
about is that it is the certificate that the peer must present.

[Kevin M, 2016/02/13 00:44:18]

Done.

+                     net::NetLog* net_log);
+
+  ~SSLClientTransport() override;
+
+  // BlimpTransport implementation.
+  void Connect(const net::CompletionCallback& callback) override;
+  scoped_ptr<BlimpConnection> TakeConnection() override;
+  const std::string GetName() const override;
+
+ private:
+  friend class SSLClientTransportTest;
+
+  void SetClientSocketFactoryForTest(net::ClientSocketFactory* factory);

[Wez, 2016/02/12 21:42:38]

Does this need to be private? I thought we had a presubmit check that prevents
non-test code from calling *ForTest methods, so we can leave it public?

[Kevin M, 2016/02/13 00:44:18]

Done.

+
+  // Callback method after TCPClientSocket::Connect finishes.
+  void OnTCPConnectComplete(int result);
+
+  // Callback method after SSLClientSocket::Connect finishes.
+  void OnSSLConnectComplete(int result);
+
+  net::AddressList addresses_;
+  net::NetLog* net_log_;

[Wez, 2016/02/12 21:42:38]

nit: Suggest inline init this to nullptr, and socket_factory_ below.

[Kevin M, 2016/02/13 00:44:18]

Done.

+  scoped_ptr<net::StreamSocket> tcp_socket_;
+  scoped_ptr<net::StreamSocket> ssl_socket_;
+  net::CompletionCallback connect_callback_;
+  scoped_ptr<BlimpCertVerifier> cert_verifier_;

[Wez, 2016/02/12 21:42:38]

nit: Looks like this could be a direct member rather than allocating separately
on heap.

[Kevin M, 2016/02/13 00:44:18]

Done.

+  net::TransportSecurityState transport_security_state_;
+  net::ClientSocketFactory* socket_factory_;
+
+  DISALLOW_COPY_AND_ASSIGN(SSLClientTransport);
+};
+
+}  // namespace blimp
+
+#endif  // BLIMP_NET_SSL_CLIENT_TRANSPORT_H_