Blimp: add support for SSL connections.

blimp/client/session/assignment_source.cc

 // 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.
 
 #include "blimp/client/session/assignment_source.h"
 
 #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"
 #include "net/base/net_errors.h"
 #include "net/base/url_util.h"

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

Do you need this include any more?

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

Done.

 #include "net/http/http_status_code.h"
 #include "net/proxy/proxy_config_service.h"
 #include "net/proxy/proxy_service.h"
 #include "net/url_request/url_fetcher.h"
 #include "net/url_request/url_request_context.h"
 #include "net/url_request/url_request_context_builder.h"
 #include "net/url_request/url_request_context_getter.h"
 
 namespace blimp {
 namespace client {
 
 namespace {
 
 // Assignment request JSON keys.
 const char kProtocolVersionKey[] = "protocol_version";
 
 // Assignment response JSON keys.
 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.
   return GURL(kDefaultAssignerURL);
 }
 
 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_)) {}
 
   // net::URLRequestContextGetter implementation.
   net::URLRequestContext* GetURLRequestContext() override {
     if (!url_request_context_) {
       net::URLRequestContextBuilder builder;
       builder.set_proxy_config_service(std::move(proxy_config_service_));
       builder.DisableHttpCache();
@@ skipping 16 matching lines @@
 
   // Temporary storage for the ProxyConfigService, which needs to be created on
   // the main thread but cleared on the IO thread.  This will be built in the
   // constructor and cleared on the IO thread.  Due to the usage of this class
   // this is safe.
   scoped_ptr<net::ProxyConfigService> proxy_config_service_;
 
   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_)
         .Run(AssignmentSource::Result::RESULT_SERVER_INTERRUPTED, Assignment());
   }

[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;
   }
 
   // Call out to the network for a real assignment.  Build the network request

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

nit: Suggest keeping the network-assignment code as a separate
GetAssignmentFromNetwork(), that can be called directly from GetAssignment if no
--engine-cert was provided.

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

Done.

   // to hit the assigner.
   url_fetcher_ = net::URLFetcher::Create(GetBlimpAssignerURL(),
                                          net::URLFetcher::POST, this);
   url_fetcher_->SetRequestContext(url_request_context_.get());
   url_fetcher_->SetLoadFlags(net::LOAD_DO_NOT_SAVE_COOKIES |
                              net::LOAD_DO_NOT_SEND_COOKIES);
   url_fetcher_->AddExtraRequestHeader("Authorization: Bearer " +
                                       client_auth_token);
 
   // Write the JSON for the request data.
   base::DictionaryValue dictionary;
   dictionary.SetString(kProtocolVersionKey, blimp::kEngineVersion);
   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);
 
   if (!source->GetStatus().is_success()) {
     DVLOG(1) << "Assignment request failed due to network error: "
              << net::ErrorToString(source->GetStatus().error());
     base::ResetAndReturn(&callback_)
         .Run(AssignmentSource::Result::RESULT_NETWORK_FAILURE, Assignment());
     return;
   }
@@ skipping 36 matching lines @@
   DCHECK_EQ(net::HTTP_OK, url_fetcher_->GetResponseCode());
 
   // Grab the response from the assigner request.
   std::string response;
   if (!url_fetcher_->GetResponseAsString(&response)) {
     base::ResetAndReturn(&callback_)
         .Run(AssignmentSource::Result::RESULT_BAD_RESPONSE, Assignment());
     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_)
         .Run(AssignmentSource::Result::RESULT_BAD_RESPONSE, Assignment());
     return;
   }
 
   // Validate that all the expected fields are present.
   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;
   }
 
   net::IPAddress ip_address;
   if (!ip_address.AssignFromIPLiteral(host)) {
     base::ResetAndReturn(&callback_)
         .Run(AssignmentSource::Result::RESULT_BAD_RESPONSE, Assignment());
     return;
   }
 
   if (!base::IsValueInRangeForNumericType<uint16_t>(port)) {
     base::ResetAndReturn(&callback_)
         .Run(AssignmentSource::Result::RESULT_BAD_RESPONSE, Assignment());
     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