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.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 {
 
 // TODO(kmarshall): Take values from configuration data.
 const char kDummyClientToken[] = "MyVoiceIsMyPassport";
 const std::string kDefaultBlimpletIPAddress = "127.0.0.1";
-const uint16_t kDefaultBlimpletTCPPort = 25467;
+const uint16_t kNoPort = 0;

[Wez, 2016/02/12 02:31:13]

nit: kPortNotSet or kInvalidPort?

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

Done.

 
 net::IPAddress GetBlimpletIPAddress() {
   std::string host;
   if (base::CommandLine::ForCurrentProcess()->HasSwitch(
           switches::kBlimpletHost)) {
     host = base::CommandLine::ForCurrentProcess()->GetSwitchValueASCII(
         switches::kBlimpletHost);
   } else {
     host = kDefaultBlimpletIPAddress;
   }
   net::IPAddress ip_address;
   if (!ip_address.AssignFromIPLiteral(host))
     CHECK(false) << "Invalid BlimpletAssignment host " << host;
   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;

[Wez, 2016/02/12 02:31:13]

nit: This needs to be explicitly initialized.

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

Done.

     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;
+    return kNoPort;
   }
 }
 
+uint16_t GetBlimpletSSLPort() {

[Wez, 2016/02/12 02:31:13]

This is basically identical to the TCP port one above - why not have a more
generic GetUint16Parameter(const& switch_name) and have the CHECK include the
name?

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

Done.

+  if (base::CommandLine::ForCurrentProcess()->HasSwitch(
+          switches::kBlimpletSSLPort)) {

[Wez, 2016/02/12 02:31:13]

GetSwitchValueASCII returns an empty string if the switch isn't present, so you
don't really need to explicitly check for the switch beforehand - just
early-exit if it's empty.

(The existing e.g. IPAddress switch code could be simplified similarly)

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

Done.

+    std::string port_str =
+        base::CommandLine::ForCurrentProcess()->GetSwitchValueASCII(
+            switches::kBlimpletSSLPort);
+    uint port_64t;

[Wez, 2016/02/12 02:31:13]

nit: This needs to be explicitly initialized.

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

Done.

+    if (!base::StringToUint(port_str, &port_64t) ||
+        !base::IsValueInRangeForNumericType<uint16_t>(port_64t)) {
+      CHECK(false) << "Invalid SSL port " << port_str;

[Wez, 2016/02/12 02:31:13]

nit: Save the result of StringToUint || IsValue.. into a bool and then
CHECK(bool).

Also note that range-checking the value isn't sufficient, since 0 is not a valid
port number.

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

Done.

+    }
+    return base::checked_cast<uint16_t>(port_64t);
+  } else {
+    return kNoPort;
+  }
+}
+
+base::FilePath GetBlimpletCertPath() {

[Wez, 2016/02/12 02:31:13]

This function is redundant; GetSwitchValuePath() already returns an empty
FilePath() if the switch is missing.

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

Done.

+  if (base::CommandLine::ForCurrentProcess()->HasSwitch(
+          switches::kBlimpletCertPath)) {
+    return base::CommandLine::ForCurrentProcess()->GetSwitchValuePath(
+        switches::kBlimpletCertPath);
+  }
+  return base::FilePath();
+}
+
 }  // namespace
 
 namespace client {
 
+Assignment::Assignment() {}

[Wez, 2016/02/12 02:31:13]

See above re inline init; you need to make sure to initialize the port numbers,
otherwise naive instantiation of an Assignment instance has it in a weird state.

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

Done.

+
+Assignment::~Assignment() {}
+
 AssignmentSource::AssignmentSource(
     const scoped_refptr<base::SingleThreadTaskRunner>& main_task_runner)
-    : main_task_runner_(main_task_runner) {}
+    : main_task_runner_(main_task_runner), io_thread_("CertFileThread") {
+  base::Thread::Options options;
+  options.message_loop_type = base::MessageLoop::TYPE_IO;
+  io_thread_.StartWithOptions(options);

[Wez, 2016/02/12 02:31:13]

Cleaner to avoid creating this thread until you actually need it?

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

Done.

+}
 
 AssignmentSource::~AssignmentSource() {}
 
+void AssignmentSource::ReadFromDisk(

[Wez, 2016/02/12 02:31:13]

As noted elsewhere, this function can be a static that you Bind() any necessary
data into.

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

Done.

+    base::FilePath path,
+    const base::Callback<void(const std::string&)>& callback) {
+  if (!io_thread_.task_runner()->RunsTasksOnCurrentThread()) {
+    // Execute file reads on the I/O thread.
+    io_thread_.task_runner()->PostTask(
+        FROM_HERE, base::Bind(&AssignmentSource::ReadFromDisk,
+                              base::Unretained(this), path, callback));
+    return;

[Wez, 2016/02/12 02:31:13]

ICK!  This sort of trampolining is safe while the calling object owns the
Thread, but will break the moment some bright spark switches it to use some
other IO thread that is owned externally, or re-orders this object's members so
that the Thread member isn't deleted (and therefore joined) before all the
others!

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

Done.

+  }
+
+  std::string output;
+  if (!base::ReadFileToString(path, &output)) {
+    LOG(ERROR) << "Couldn't read cert from file: " << path.LossyDisplayName();

[Wez, 2016/02/12 02:31:13]

Why LOG(ERROR) when we CHECK for everything else?

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

Done.

+  }
+
+  main_task_runner_->PostTask(FROM_HERE, base::Bind(callback, output));

[Wez, 2016/02/12 02:31:13]

This doesn't match the API comment.

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

Acknowledged.

+}
+
+void AssignmentSource::ParseCertForAssignment(
+    Assignment assignment,
+    const AssignmentCallback& callback,
+    const std::string& cert_str) {
+  if (!cert_str.empty()) {

[Wez, 2016/02/12 02:31:13]

Surely you can't reach here if it's empty, so this should be a DCHECK?

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

Done.

+    scoped_refptr<net::X509Certificate> cert;

[Wez, 2016/02/12 02:31:13]

Doesn't look like you use this?

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

Done.

+    net::PEMTokenizer pem_tokenizer(cert_str, {"CERTIFICATE"});
+    int num_certs = 0;

[Wez, 2016/02/12 02:31:13]

Rather than counting the certs, you can surely just CHECK() that there is no
assignment.cert before you CreateFromBytes - the developer can go and check
their PEM file and see how many certs they accidentally provided!

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

Done.

+    for (; pem_tokenizer.GetNext(); ++num_certs) {
+      assignment.cert = net::X509Certificate::CreateFromBytes(
+          pem_tokenizer.data().data(), pem_tokenizer.data().length());
+      if (!assignment.cert) {
+        LOG(ERROR) << "Couldn't parse cert!";
+      } else {
+        DVLOG(1) << "Parsed X509 cert.";
+      }
+    }
+    if (num_certs != 1) {
+      LOG(ERROR) << "Expected exactly 1 CERTIFICATE block, but found "
+                 << num_certs << " blocks instead.";
+      assignment.cert = nullptr;
+    }
+  }
+
+  main_task_runner_->PostTask(FROM_HERE, base::Bind(callback, assignment));

[Wez, 2016/02/12 02:31:13]

You're already on the main task runner, with a clean stack - why are you posting
a task to run this?

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

This line is from Tommy's initial impl, I was thinking he might have had a good
reason.

+}
+
 void AssignmentSource::GetAssignment(const AssignmentCallback& callback) {
   DCHECK(main_task_runner_->BelongsToCurrentThread());
   Assignment assignment;
-  assignment.ip_endpoint =
-      net::IPEndPoint(GetBlimpletIPAddress(), GetBlimpletTCPPort());
+  assignment.ip_addresses.push_back(GetBlimpletIPAddress());
+  if (GetBlimpletTCPPort() > 0) {

[Wez, 2016/02/12 02:31:13]

This if() is always true, since the Get*Port() method returns a uint16_t.

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

Done.

+    assignment.tcp_port = GetBlimpletTCPPort();
+  }
+  if (GetBlimpletSSLPort() > 0) {
+    assignment.ssl_port = GetBlimpletSSLPort();
+  }
   assignment.client_token = kDummyClientToken;
-  main_task_runner_->PostTask(FROM_HERE, base::Bind(callback, assignment));
+
+  base::FilePath cert_path = GetBlimpletCertPath();
+  if (!cert_path.empty()) {
+    ReadFromDisk(cert_path,
+                 base::Bind(&AssignmentSource::ParseCertForAssignment,
+                            base::Unretained(this), assignment, callback));

[Wez, 2016/02/12 02:31:13]

See notes above re combining file-read and cert-parse into a single static
function using PostTaskAndReply.

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

Done.

+  } else {
+    main_task_runner_->PostTask(FROM_HERE, base::Bind(callback, assignment));

[Wez, 2016/02/12 02:31:13]

Assert that the SSL port is kNoPort in this case?

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

Done.

+  }
 }
 
 }  // namespace client
 }  // namespace blimp