Blimp: add support for SSL connections.

blimp/client/session/assignment_source.cc

Index: blimp/client/session/assignment_source.cc
diff --git a/blimp/client/session/assignment_source.cc b/blimp/client/session/assignment_source.cc
index 242d7839122af23edbbeef36cf87feeedd5821d2..2b7b88391d1f4ba483ed310bb1b5097e1203c417 100644
--- a/blimp/client/session/assignment_source.cc
+++ b/blimp/client/session/assignment_source.cc
@@ -7,14 +7,18 @@
 #include "base/bind.h"
 #include "base/callback_helpers.h"
 #include "base/command_line.h"
+#include "base/files/file_util.h"
 #include "base/json/json_reader.h"
 #include "base/json/json_writer.h"
 #include "base/location.h"
+#include "base/memory/ref_counted.h"
 #include "base/numerics/safe_conversions.h"
 #include "base/strings/string_number_conversions.h"
+#include "base/task_runner_util.h"
 #include "base/values.h"
 #include "blimp/client/app/blimp_client_switches.h"
 #include "blimp/common/protocol_version.h"
+#include "components/safe_json/safe_json_parser.h"
 #include "net/base/ip_address.h"
 #include "net/base/ip_endpoint.h"
 #include "net/base/load_flags.h"
@@ -40,55 +44,12 @@ const char kProtocolVersionKey[] = "protocol_version";
 const char kClientTokenKey[] = "clientToken";
 const char kHostKey[] = "host";
 const char kPortKey[] = "port";
-const char kCertificateFingerprintKey[] = "certificateFingerprint";
 const char kCertificateKey[] = "certificate";
 
 // URL scheme constants for custom assignments.  See the '--blimplet-endpoint'
 // documentation in blimp_client_switches.cc for details.

[Wez, 2016/03/01 00:23:56]

This comment is now out-of-date.

[Kevin M, 2016/03/01 18:23:17]

Done.

-const char kCustomSSLScheme[] = "ssl";
-const char kCustomTCPScheme[] = "tcp";
-const char kCustomQUICScheme[] = "quic";
-
-Assignment GetCustomBlimpletAssignment() {
-  GURL url(base::CommandLine::ForCurrentProcess()->GetSwitchValueASCII(
-      switches::kBlimpletEndpoint));
-
-  std::string host;
-  int port;
-  if (url.is_empty() || !url.is_valid() || !url.has_scheme() ||
-      !net::ParseHostAndPort(url.path(), &host, &port)) {
-    return Assignment();
-  }
-
-  net::IPAddress ip_address;
-  if (!ip_address.AssignFromIPLiteral(host)) {
-    CHECK(false) << "Invalid BlimpletAssignment host " << host;
-  }
-
-  if (!base::IsValueInRangeForNumericType<uint16_t>(port)) {
-    CHECK(false) << "Invalid BlimpletAssignment port " << port;
-  }
-
-  Assignment::TransportProtocol protocol =
-      Assignment::TransportProtocol::UNKNOWN;
-  if (url.has_scheme()) {
-    if (url.SchemeIs(kCustomSSLScheme)) {
-      protocol = Assignment::TransportProtocol::SSL;
-    } else if (url.SchemeIs(kCustomTCPScheme)) {
-      protocol = Assignment::TransportProtocol::TCP;
-    } else if (url.SchemeIs(kCustomQUICScheme)) {
-      protocol = Assignment::TransportProtocol::QUIC;
-    } else {
-      CHECK(false) << "Invalid BlimpletAssignment scheme " << url.scheme();
-    }
-  }
-
-  Assignment assignment;
-  assignment.transport_protocol = protocol;
-  assignment.ip_endpoint = net::IPEndPoint(ip_address, port);
-  assignment.client_token = kDummyClientToken;
-  return assignment;
-}
+const char kSSLTransport[] = "ssl";
+const char kTCPTransport[] = "tcp";

[Wez, 2016/03/01 00:23:55]

nit: Suggest kSslTransportValue and kTcpTransportValue, since these are
potential values of the --engine-transport parameter.

[Kevin M, 2016/03/01 18:23:17]

Done.

 
 GURL GetBlimpAssignerURL() {
   // TODO(dtrainor): Add a way to specify another assigner.
@@ -98,8 +59,8 @@ GURL GetBlimpAssignerURL() {
 class SimpleURLRequestContextGetter : public net::URLRequestContextGetter {
  public:
   SimpleURLRequestContextGetter(
-      const scoped_refptr<base::SingleThreadTaskRunner>& io_loop_task_runner)
-      : io_loop_task_runner_(io_loop_task_runner),
+      scoped_refptr<base::SingleThreadTaskRunner> io_loop_task_runner)
+      : io_loop_task_runner_(std::move(io_loop_task_runner)),
         proxy_config_service_(net::ProxyService::CreateSystemProxyConfigService(
             io_loop_task_runner_,
             io_loop_task_runner_)) {}
@@ -136,29 +97,115 @@ class SimpleURLRequestContextGetter : public net::URLRequestContextGetter {
   DISALLOW_COPY_AND_ASSIGN(SimpleURLRequestContextGetter);
 };
 
+// Populates an Assignment using command-line parameters, if provided.
+// Returns a null Assignment if no parameters were set.
+// Must be called on an IO thread.

[Wez, 2016/03/01 00:23:55]

nit: Suggest being explicit as to why e.g. "Must be called on a thread suitable
for file-IO."

[Kevin M, 2016/03/01 18:23:17]

Done.

[Kevin M, 2016/03/01 18:23:17]

Done.

+Assignment GetAssignmentFromCommandLine() {
+  std::string ip_str =
+      base::CommandLine::ForCurrentProcess()->GetSwitchValueASCII(
+          switches::kEngineIP);
+  std::string port_str =
+      base::CommandLine::ForCurrentProcess()->GetSwitchValueASCII(
+          switches::kEnginePort);
+  std::string transport_str =
+      base::CommandLine::ForCurrentProcess()->GetSwitchValueASCII(
+          switches::kEngineTransport);

[Wez, 2016/03/01 00:23:55]

Don't bother pulling these out of the command-line until you are ready to parse
them, below - in general you want to declare local variables as close to their
first use as possible.

[Kevin M, 2016/03/01 18:23:17]

Done.

+
+  if (ip_str.empty() && port_str.empty() && transport_str.empty()) {
+    return Assignment();

[Wez, 2016/03/01 00:23:55]

This early-exit and the subsequent test to see that everything's either set or
un-set is equivalent to decoding each field individually into an Assignment and
then returning it only if it IsValid().

I'd therefore suggest creating an empty Assignment here, and parsing each
non-empty command-line parameter into it, below, and finally checking IsValid()
before returning it.

[Kevin M, 2016/03/01 18:23:17]

Done.

+  }
+
+  if (ip_str.empty() != port_str.empty() ||
+      ip_str.empty() != transport_str.empty()) {

[Wez, 2016/03/01 00:23:56]

nit: If you keep the early-exit, above, then this becomes a check that they are
all not-empty(), but see my comment above.

[Kevin M, 2016/03/01 18:23:17]

Done.

+    DLOG(FATAL)
+        << "--engine-ip, --engine-port, and --engine-transport must be set "
+           "together.";
+    return Assignment();
+  }
+
+  unsigned port_parsed;
+  uint16_t port;

[Wez, 2016/03/01 00:23:55]

nit: Always initialize PoD types, even if they're about to be overwritten by
some function call out-parameter.

[Kevin M, 2016/03/01 18:23:17]

Done.

+  if (!base::StringToUint(port_str, &port_parsed) || port_parsed <= 0 ||
+      port_parsed > 65535) {
+    DLOG(FATAL) << "--engine-port must be a value between 0 and 65535.";

[Wez, 2016/03/01 00:23:55]

It's not valid for the port # to be zero, so this comment, and the range-check,
are not correct.  Also consider writing & using a trivial helper
IsValidTcpPortNumber() or something.

[Kevin M, 2016/03/01 18:23:17]

Done.

[Kevin M, 2016/03/01 18:23:17]

Done.

+    return Assignment();
+  }
+  port = base::checked_cast<uint16_t>(port_parsed);
+
+  net::IPAddress ip_address;
+  if (!ip_address.AssignFromIPLiteral(ip_str)) {
+    DLOG(FATAL) << "Invalid engine IP " << ip_str;
+    return Assignment();
+  }
+
+  Assignment::TransportProtocol transport =
+      Assignment::TransportProtocol::UNKNOWN;
+  if (transport_str == kSSLTransport) {
+    transport = Assignment::TransportProtocol::SSL;
+  } else if (transport_str == kTCPTransport) {
+    transport = Assignment::TransportProtocol::TCP;
+  } else {
+    DLOG(FATAL) << "Invalid engine transport " << transport;
+    return Assignment();
+  }
+
+  scoped_refptr<net::X509Certificate> cert;
+  if (transport == Assignment::TransportProtocol::SSL) {
+    base::FilePath cert_path =
+        base::CommandLine::ForCurrentProcess()->GetSwitchValuePath(
+            switches::kEngineCertPath);
+    if (cert_path.empty()) {
+      DLOG(FATAL) << "Missing required parameter --"
+                  << switches::kEngineCertPath << ".";
+      return Assignment();
+    }
+    std::string cert_str;
+    if (!base::ReadFileToString(cert_path, &cert_str)) {
+      DLOG(FATAL) << "Couldn't read from file: "
+                  << cert_path.LossyDisplayName();
+      return Assignment();
+    }
+    net::CertificateList cert_list =
+        net::X509Certificate::CreateCertificateListFromBytes(
+            cert_str.data(), cert_str.size(),
+            net::X509Certificate::FORMAT_PEM_CERT_SEQUENCE);
+    DLOG_IF(FATAL, (cert_list.size() != 1u))
+        << "Only one cert is allowed in PEM cert list.";
+    cert = std::move(cert_list[0]);
+  }
+
+  Assignment assignment;
+  assignment.transport_protocol = transport;
+  assignment.engine_endpoint =
+      net::IPEndPoint(ip_address, base::checked_cast<uint16_t>(port));

[Wez, 2016/03/01 00:23:56]

You have already checked_cast<> when assigning to |port|, above.

[Kevin M, 2016/03/01 18:23:17]

Done.

+  assignment.client_token = kDummyClientToken;
+  assignment.cert = std::move(cert);
+  return assignment;
+}
+
 }  // namespace
 
 Assignment::Assignment() : transport_protocol(TransportProtocol::UNKNOWN) {}
 
 Assignment::~Assignment() {}
 
-bool Assignment::is_null() const {
-  return ip_endpoint.address().empty() || ip_endpoint.port() == 0 ||
-         transport_protocol == TransportProtocol::UNKNOWN;
+bool Assignment::IsValid() const {
+  return !engine_endpoint.address().empty() && engine_endpoint.port() != 0 &&
+         transport_protocol != TransportProtocol::UNKNOWN;
 }
 
 AssignmentSource::AssignmentSource(
-    const scoped_refptr<base::SingleThreadTaskRunner>& main_task_runner,
-    const scoped_refptr<base::SingleThreadTaskRunner>& io_task_runner)
-    : main_task_runner_(main_task_runner),
-      url_request_context_(new SimpleURLRequestContextGetter(io_task_runner)) {}
+    scoped_refptr<base::SingleThreadTaskRunner> network_task_runner,
+    scoped_refptr<base::SingleThreadTaskRunner> file_task_runner)
+    : file_task_runner_(std::move(file_task_runner)),
+      url_request_context_(
+          new SimpleURLRequestContextGetter(network_task_runner)),
+      weak_factory_(this) {}
 
 AssignmentSource::~AssignmentSource() {}
 
 void AssignmentSource::GetAssignment(const std::string& client_auth_token,
                                      const AssignmentCallback& callback) {
-  DCHECK(main_task_runner_->BelongsToCurrentThread());
-
   // Cancel any outstanding callback.
   if (!callback_.is_null()) {
     base::ResetAndReturn(&callback_)
@@ -166,12 +213,23 @@ void AssignmentSource::GetAssignment(const std::string& client_auth_token,
   }

[Wez, 2016/03/01 00:23:55]

Follow-up: This looks dubious:

1. the caller should know whether they requested an assignment, so know not to
call again until they see that earlier request responded to.

2. If there is a concern about requests stalling, and therefore needing to be
cancelled then I'd suggest an explicit CancelGetAssignment() method.

3. With this code as-is, the |weak_factory_| is not being invalidated, so an
earlier assignment request will still complete and we'll try to call |callback_|
twice, once for the old and once for the new request.

[Kevin M, 2016/03/01 18:23:17]

Hey +dtrainor@chromium.org, is there a specific reason why this code is the way
it is? Because I'd rather just have dupe callbacks be a DCHECKable error
condition instead of something we handle.

[Wez, 2016/03/02 02:26:44]

Acknowledged.

   callback_ = AssignmentCallback(callback);
 
-  Assignment assignment = GetCustomBlimpletAssignment();
-  if (!assignment.is_null()) {
-    // Post the result so that the behavior of this function is consistent.
-    main_task_runner_->PostTask(
-        FROM_HERE, base::Bind(base::ResetAndReturn(&callback_),
-                              AssignmentSource::Result::RESULT_OK, assignment));
+  // Try to get a custom assignment from command lines first.
+  PostTaskAndReplyWithResult(
+      file_task_runner_.get(), FROM_HERE,
+      base::Bind(&GetAssignmentFromCommandLine),

[Wez, 2016/03/01 00:23:55]

Wouldn't it be cleaner to only GetAssignmentFromCommandLine() if there is an
--engine-host parameter? (The function itself can check for the other parameters
and barf if necessary), and otherwise move on to calling
GetAssignmentFromNetwork() directly?

Alternatively keep a call to GetAssignmentFromCommandLine here, but push the
PostTaskAndReplyWithResult() call inside it, so that it's just the actual
file-IO we do on the file thread. Or is there other code in there that needs the
file-IO thread?

[Kevin M, 2016/03/02 18:05:33]

(Done on previous patch)

+      base::Bind(&AssignmentSource::OnGetAssignmentFromCommandLineDone,
+                 weak_factory_.GetWeakPtr(), client_auth_token));
+}
+
+void AssignmentSource::OnGetAssignmentFromCommandLineDone(
+    const std::string& client_auth_token,
+    Assignment parsed_assignment) {
+  // If GetAssignmentFromCommandLine succeeded, then return the custom

[Wez, 2016/03/01 00:23:56]

nit: Not "custom" assignment any more.

[Wez, 2016/03/02 02:26:44]

nit: Ping!

[Kevin M, 2016/03/02 18:05:33]

Done.

+  // assignment
+  // directly.
+  if (parsed_assignment.IsValid()) {
+    base::ResetAndReturn(&callback_)
+        .Run(AssignmentSource::RESULT_OK, parsed_assignment);
     return;
   }
 
@@ -191,12 +249,10 @@ void AssignmentSource::GetAssignment(const std::string& client_auth_token,
   std::string json;
   base::JSONWriter::Write(dictionary, &json);
   url_fetcher_->SetUploadData("application/json", json);
-
   url_fetcher_->Start();
 }
 
 void AssignmentSource::OnURLFetchComplete(const net::URLFetcher* source) {
-  DCHECK(main_task_runner_->BelongsToCurrentThread());
   DCHECK(!callback_.is_null());
   DCHECK_EQ(url_fetcher_.get(), source);
 
@@ -253,14 +309,14 @@ void AssignmentSource::ParseAssignerResponse() {
     return;
   }
 
-  // Attempt to interpret the response as JSON and treat it as a dictionary.
-  scoped_ptr<base::Value> json = base::JSONReader::Read(response);
-  if (!json) {
-    base::ResetAndReturn(&callback_)
-        .Run(AssignmentSource::Result::RESULT_BAD_RESPONSE, Assignment());
-    return;
-  }
+  safe_json::SafeJsonParser::Parse(
+      response,
+      base::Bind(&AssignmentSource::OnJSONParsed, weak_factory_.GetWeakPtr()),
+      base::Bind(&AssignmentSource::OnJSONParseError,
+                 weak_factory_.GetWeakPtr()));
+}
 
+void AssignmentSource::OnJSONParsed(scoped_ptr<base::Value> json) {
   const base::DictionaryValue* dict;
   if (!json->GetAsDictionary(&dict)) {
     base::ResetAndReturn(&callback_)
@@ -272,12 +328,10 @@ void AssignmentSource::ParseAssignerResponse() {
   std::string client_token;
   std::string host;
   int port;
-  std::string cert_fingerprint;
-  std::string cert;
+  std::string cert_str;
   if (!(dict->GetString(kClientTokenKey, &client_token) &&
         dict->GetString(kHostKey, &host) && dict->GetInteger(kPortKey, &port) &&
-        dict->GetString(kCertificateFingerprintKey, &cert_fingerprint) &&
-        dict->GetString(kCertificateKey, &cert))) {
+        dict->GetString(kCertificateKey, &cert_str))) {
     base::ResetAndReturn(&callback_)
         .Run(AssignmentSource::Result::RESULT_BAD_RESPONSE, Assignment());
     return;
@@ -296,18 +350,33 @@ void AssignmentSource::ParseAssignerResponse() {
     return;
   }
 
-  Assignment assignment;
+  net::CertificateList cert_list =
+      net::X509Certificate::CreateCertificateListFromBytes(
+          cert_str.data(), cert_str.size(),
+          net::X509Certificate::FORMAT_PEM_CERT_SEQUENCE);
+  if (cert_list.size() != 1) {
+    base::ResetAndReturn(&callback_)
+        .Run(AssignmentSource::Result::RESULT_INVALID_CERT, Assignment());
+    return;
+  }
+
   // The assigner assumes SSL-only and all engines it assigns only communicate
   // over SSL.
+  Assignment assignment;
   assignment.transport_protocol = Assignment::TransportProtocol::SSL;
-  assignment.ip_endpoint = net::IPEndPoint(ip_address, port);
+  assignment.engine_endpoint = net::IPEndPoint(ip_address, port);
   assignment.client_token = client_token;
-  assignment.certificate = cert;
-  assignment.certificate_fingerprint = cert_fingerprint;
+  assignment.cert = std::move(cert_list[0]);
 
   base::ResetAndReturn(&callback_)
       .Run(AssignmentSource::Result::RESULT_OK, assignment);
 }
 
+void AssignmentSource::OnJSONParseError(const std::string& error) {
+  DLOG(ERROR) << "Error while parsing assigner JSON: " << error;
+  base::ResetAndReturn(&callback_)
+      .Run(AssignmentSource::Result::RESULT_BAD_RESPONSE, Assignment());
+}
+
 }  // namespace client
 }  // namespace blimp