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 a83c66f4ac1b7ec63fb5883b679806d269677a3b..e585caf5a1e06c39b549354a13f2775842b7a325 100644
--- a/blimp/client/session/assignment_source.cc
+++ b/blimp/client/session/assignment_source.cc
@@ -5,28 +5,32 @@
 #include "blimp/client/session/assignment_source.h"
 
 #include "base/bind.h"
+#include "base/callback.h"
 #include "base/command_line.h"
+#include "base/files/file_util.h"
 #include "base/location.h"
 #include "base/numerics/safe_conversions.h"
 #include "base/strings/string_number_conversions.h"
 #include "blimp/client/app/blimp_client_switches.h"
+#include "net/base/hash_value.h"
 #include "net/base/ip_address.h"
 #include "net/base/ip_endpoint.h"
+#include "net/cert/pem_tokenizer.h"
 
 namespace blimp {
+namespace client {
 namespace {
 
 // TODO(kmarshall): Take values from configuration data.
 const char kDummyClientToken[] = "MyVoiceIsMyPassport";
 const std::string kDefaultBlimpletIPAddress = "127.0.0.1";
-const uint16_t kDefaultBlimpletTCPPort = 25467;
 
 net::IPAddress GetBlimpletIPAddress() {
   std::string host;
   if (base::CommandLine::ForCurrentProcess()->HasSwitch(
-          switches::kBlimpletHost)) {
+          switches::kEngineHost)) {
     host = base::CommandLine::ForCurrentProcess()->GetSwitchValueASCII(
-        switches::kBlimpletHost);
+        switches::kEngineHost);

[Wez, 2016/02/18 00:40:50]

This is super verbose; can't you just say:

string host = GetSwitchValueASCII(...);
if (host.empty())
  host = kDefaultBlimpletIPAddress;

[Kevin M, 2016/02/18 23:35:46]

Done.

   } else {
     host = kDefaultBlimpletIPAddress;
   }
@@ -36,26 +40,59 @@ net::IPAddress GetBlimpletIPAddress() {
   return ip_address;
 }
 
-uint16_t GetBlimpletTCPPort() {
-  if (base::CommandLine::ForCurrentProcess()->HasSwitch(
-          switches::kBlimpletTCPPort)) {
-    std::string port_str =
-        base::CommandLine::ForCurrentProcess()->GetSwitchValueASCII(
-            switches::kBlimpletTCPPort);
-    uint port_64t;
-    if (!base::StringToUint(port_str, &port_64t) ||
-        !base::IsValueInRangeForNumericType<uint16_t>(port_64t)) {
-      CHECK(false) << "Invalid BlimpletAssignment port " << port_str;
-    }
-    return base::checked_cast<uint16_t>(port_64t);
-  } else {
-    return kDefaultBlimpletTCPPort;
+// Puts the value of the command line parameter |param| in |output|.
+// Returns true if the parameter was provided.

[Wez, 2016/02/18 00:40:50]

This doesn't make clear what it does if the parameter isn't provided.

Wouldn't it be cleaner to use the invalid zero value to indicate that no port
number was provided?

[Kevin M, 2016/02/18 23:35:46]

Done.

+// CHECK-fails if the parameter was provided but invalid (must be numeric and
+// within the range [1, 65535]

[Wez, 2016/02/18 00:40:50]

Suggest: "CHECK()s that |param| decodes to a valid IP port-number."

[Kevin M, 2016/02/18 23:35:46]

Done.

+bool GetUint16Parameter(const std::string& param, uint16_t* output) {
+  std::string param_str =
+      base::CommandLine::ForCurrentProcess()->GetSwitchValueASCII(param);
+  if (param_str.empty()) {
+    return false;
+  }
+
+  uint param_parsed = 0;
+  bool is_valid = base::StringToUint(param_str, &param_parsed) &&
+                  base::IsValueInRangeForNumericType<uint16_t>(param_parsed) &&
+                  param_parsed > 0;
+  CHECK(is_valid) << "Invalid range for parameter " << param;

[Wez, 2016/02/18 00:40:50]

nit: Misleading; may also not be a number.

nit: Why use IsValueInRangeForNumericType, rather than comparing against 65535
explicitly, since that is the maximum valid IP port?

[Kevin M, 2016/02/18 23:35:46]

Done.

+  *output = base::checked_cast<uint16_t>(param_parsed);

[Wez, 2016/02/18 00:40:50]

No need to use checked_cast<> here, since |param_parsed| can never be
out-of-range at this point.

[Kevin M, 2016/02/18 23:35:46]

Done.

+  return true;
+}
+
+// Reads the contents of |path| into |output|.
+void ReadFromDisk(const base::FilePath& path, std::string* output) {
+  DCHECK(output);
+  CHECK(base::ReadFileToString(path, output)) << "Couldn't read from file: "
+                                              << path.LossyDisplayName();
+}
+
+// Parses a certificate from PEM-encoded |cert_str| and attaches it to
+// |assignment|. Returns the populated assignment object via |callback|.
+void ParseCertForAssignment(
+    scoped_ptr<std::string> cert_str,
+    scoped_ptr<Assignment> assignment,
+    const AssignmentSource::AssignmentCallback& callback) {
+  DCHECK(assignment);

[Wez, 2016/02/18 00:40:50]

nit: This code dereferences |assignment| directly, so there isn't really any
need to DCHECK it here.

[Kevin M, 2016/02/18 23:35:46]

Done.

+  DCHECK(cert_str);
+  DCHECK(!cert_str->empty());
+
+  net::PEMTokenizer pem_tokenizer(*cert_str, {"CERTIFICATE"});
+  while (pem_tokenizer.GetNext()) {
+    CHECK(!assignment->cert.get());

[Wez, 2016/02/18 00:40:50]

nit: No need for .get() when testing a scoped[ref]ptr.

Suggest adding a message to the CHECK(...) << "Multiple CERTIFICATE entries
provided."

[Kevin M, 2016/02/18 23:35:47]

Done.

+    assignment->cert = net::X509Certificate::CreateFromBytes(
+        pem_tokenizer.data().data(), pem_tokenizer.data().length());
+    CHECK(assignment->cert) << "Couldn't parse CERTIFICATE entry.";
   }
+
+  callback.Run(*assignment);
 }
 
 }  // namespace
 
-namespace client {
+Assignment::Assignment() {}
+
+Assignment::~Assignment() {}
 
 AssignmentSource::AssignmentSource(
     const scoped_refptr<base::SingleThreadTaskRunner>& main_task_runner)
@@ -63,13 +100,39 @@ AssignmentSource::AssignmentSource(
 
 AssignmentSource::~AssignmentSource() {}
 
+scoped_refptr<base::SingleThreadTaskRunner>
+AssignmentSource::GetIOTaskRunner() {
+  if (!io_thread_) {
+    io_thread_.reset(new base::Thread("CertFileThrad"));
+    base::Thread::Options options;
+    options.message_loop_type = base::MessageLoop::TYPE_IO;

[Wez, 2016/02/18 00:40:50]

nit: Why not use the type+size constructor with the "use default" size of zero?

[Kevin M, 2016/02/18 23:35:47]

Done.

+    io_thread_->StartWithOptions(options);
+  }
+  return io_thread_->task_runner();
+}
+
 void AssignmentSource::GetAssignment(const AssignmentCallback& callback) {
   DCHECK(main_task_runner_->BelongsToCurrentThread());
-  Assignment assignment;
-  assignment.ip_endpoint =
-      net::IPEndPoint(GetBlimpletIPAddress(), GetBlimpletTCPPort());
-  assignment.client_token = kDummyClientToken;
-  main_task_runner_->PostTask(FROM_HERE, base::Bind(callback, assignment));
+
+  scoped_ptr<Assignment> assignment(new Assignment);
+  assignment->client_token = kDummyClientToken;
+  assignment->ip_addresses.push_back(GetBlimpletIPAddress());
+  GetUint16Parameter(switches::kEngineTCPPort, &assignment->tcp_port);
+  if (GetUint16Parameter(switches::kEngineSSLPort, &assignment->ssl_port)) {
+    base::FilePath cert_path =
+        base::CommandLine::ForCurrentProcess()->GetSwitchValuePath(
+            switches::kEngineCertPath);
+    CHECK(!cert_path.empty()) << "Missing required parameter --"
+                              << switches::kEngineCertPath << ".";
+
+    scoped_ptr<std::string> cert_str(new std::string);
+    GetIOTaskRunner()->PostTaskAndReply(
+        FROM_HERE, base::Bind(&ReadFromDisk, cert_path, cert_str.get()),
+        base::Bind(&ParseCertForAssignment, base::Passed(std::move(cert_str)),
+                   base::Passed(std::move(assignment)), callback));

[Wez, 2016/02/18 00:40:50]

Eeek; you'll need to take the bare pointer to |cert_str| before the
PostTaskAndReply() call - the compiler is at liberty to evaluated parameters in
whatever order makes most sense for the platform, so you must never a.get() and
std::move(a) in the same parameter sequence.

[Kevin M, 2016/02/18 23:35:47]

Good catch, thanks! Done

+  } else {
+    callback.Run(*assignment);
+  }
 }
 
 }  // namespace client