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..352b92ef03409b4a43d9890c81efac3016acbd77 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,13 @@ 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.
-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";
+const char kQUICTransport[] = "quic";
 
 GURL GetBlimpAssignerURL() {
   // TODO(dtrainor): Add a way to specify another assigner.
@@ -98,8 +60,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 +98,103 @@ 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 the IO thread.
+
+Assignment GetCustomAssignment() {
+  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);
+
+  if (ip_str.empty() && port_str.empty() && transport_str.empty()) {
+    return Assignment();
+  }
+
+  CHECK(ip_str.empty() == port_str.empty() &&

[Bernhard Bauer, 2016/02/26 16:26:30]

You mention in the review that failing with an error message is useful for
developers, but is CHECKing really what you want? For example, a crash due to a
CHECK will produce a crash report that is uploaded to our servers, which does
not seem quite right for something that could legitimately happen due to a user
error. It also means you can't actually test the behavior if invalid switches
are passed in (unless you use death tests, but those are kinda yuck IMO).

[Kevin M, 2016/02/26 19:57:23]

Good point about the reporting noise.

I'll switch to DCHECKs/DLOGs with graceful fallback behavior for release builds.

+        ip_str.empty() == transport_str.empty())
+      << "--engine-ip, --engine-port, and --engine-transport must be set "
+         "together.";
+
+  unsigned port_parsed;
+  uint16_t port;
+  CHECK(base::StringToUint(port_str, &port_parsed) && port_parsed > 0 &&
+        port_parsed <= 65535)
+      << "--engine-port must be a value between 0 and 65535.";
+  port = base::checked_cast<uint16_t>(port_parsed);
+
+  net::IPAddress ip_address;
+  CHECK(ip_address.AssignFromIPLiteral(ip_str)) << "Invalid engine IP "
+                                                << ip_str;
+
+  Assignment::TransportProtocol transport =
+      Assignment::TransportProtocol::UNKNOWN;
+  if (transport_str == kSSLTransport) {
+    transport = Assignment::TransportProtocol::SSL;
+  } else if (transport_str == kTCPTransport) {
+    transport = Assignment::TransportProtocol::TCP;
+  } else if (transport_str == kQUICTransport) {
+    transport = Assignment::TransportProtocol::QUIC;
+  } else {
+    LOG(FATAL) << "Invalid engine transport " << transport;
+  }
+
+  scoped_refptr<net::X509Certificate> cert;
+  if (transport == Assignment::TransportProtocol::SSL ||
+      transport == Assignment::TransportProtocol::QUIC) {
+    base::FilePath cert_path =
+        base::CommandLine::ForCurrentProcess()->GetSwitchValuePath(
+            switches::kEngineCertPath);
+    CHECK(!cert_path.empty()) << "Missing required parameter --"
+                              << switches::kEngineCertPath << ".";
+    std::string cert_str;
+    CHECK(base::ReadFileToString(cert_path, &cert_str))
+        << "Couldn't read from file: " << cert_path.LossyDisplayName();
+    net::CertificateList cert_list =
+        net::X509Certificate::CreateCertificateListFromBytes(
+            cert_str.data(), cert_str.size(),
+            net::X509Certificate::FORMAT_PEM_CERT_SEQUENCE);
+    CHECK_EQ(1u, cert_list.size())
+        << "Only one cert is allowed in PEM cert list.";
+    cert = std::move(cert_list[0]);
+  }
+
+  Assignment assignment;
+  assignment.transport_protocol = transport;
+  assignment.ip_endpoint =
+      net::IPEndPoint(ip_address, base::checked_cast<uint16_t>(port));
+  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 !ip_endpoint.address().empty() && ip_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> io_task_runner)
+    : io_task_runner_(std::move(io_task_runner)),
+      url_request_context_(new SimpleURLRequestContextGetter(io_task_runner_)),

[Bernhard Bauer, 2016/02/26 16:26:30]

Actually, I'm still confused. You use this thread for the network request, but
also to synchronously read files? Wouldn't that block any network communication?
Or is just that you don't care because those things happen sequentially?

(TBH, I would still use two different threads here and set
ThreadRestrictions::SetIOAllowed() on the network thread, just to make it clear
in the future which thread should be used for what, but it's up to you, I guess.
Blimp is crazy, I tell ya! 😉)

[Kevin M, 2016/02/26 19:57:23]

I added a separate task runner for file access. The operations are executed
sequentially, and the file accesses happen once on application start, so it's OK
that they point to the same task runner refptr for the time being, but this
allows for the caller to run the operations on separate threads if needed.

+      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 +202,21 @@ void AssignmentSource::GetAssignment(const std::string& client_auth_token,
   }
   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 on the IO thread first.
+  PostTaskAndReplyWithResult(
+      io_task_runner_.get(), FROM_HERE, base::Bind(&GetCustomAssignment),
+      base::Bind(&AssignmentSource::OnGetCustomAssignmentDone,
+                 weak_factory_.GetWeakPtr(), client_auth_token));
+}
+
+void AssignmentSource::OnGetCustomAssignmentDone(
+    const std::string& client_auth_token,
+    Assignment custom_assignment) {
+  // If GetCustomAssignment succeeded, then return the custom assignment
+  // directly.
+  if (custom_assignment.IsValid()) {
+    base::ResetAndReturn(&callback_)
+        .Run(AssignmentSource::RESULT_OK, custom_assignment);
     return;
   }
 
@@ -191,12 +236,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 +296,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::AssignerJSONParseOK,
+                           weak_factory_.GetWeakPtr()),
+      base::Bind(&AssignmentSource::AssignerJSONParseError,
+                 weak_factory_.GetWeakPtr()));
+}
 
+void AssignmentSource::AssignerJSONParseOK(scoped_ptr<base::Value> json) {
   const base::DictionaryValue* dict;
   if (!json->GetAsDictionary(&dict)) {
     base::ResetAndReturn(&callback_)
@@ -272,12 +315,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 +337,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.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::AssignerJSONParseError(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