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..e64932d397d2fbead47e07f894f7e6bfe36b4355 100644
--- a/blimp/client/session/assignment_source.cc
+++ b/blimp/client/session/assignment_source.cc
@@ -5,57 +5,89 @@
 #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"

[Ryan Sleevi, 2016/02/19 22:56:08]

DESIGN: This is not intended to be used outside of //net

Please use the supported //net interface of
X509Certificate::CreateFromBytes(..., ...,
X509Certificate::FORMAT_PEM_SEQUENCE);

(The existing uses are bugs that should be fixed - filed
https://bugs.chromium.org/p/chromium/issues/detail?id=588298 for that )

[Kevin M, 2016/02/23 00:28:09]

Done.

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

[Ryan Sleevi, 2016/02/19 22:56:08]

DESIGN: It seems counter to Chromium practices to CHECK() on user-input.

[Kevin M, 2016/02/22 22:53:31]

Do command line parameters count as user input, as is the case here?

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

For code which users will invoke via the command-line, yes. :)

In this case it's a developer-facing command-line interface, so we should go
with whatever mechanism is easiest for a developer to work with.

+      << "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;
-    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|.
+// If the parameter was not found, |output| is set to 0.
+// CHECK()s that |params| decodes to a valid IP port number.
+void GetUint16Parameter(const std::string& param, uint16_t* output) {
+  *output = 0;
+  std::string param_str =
+      base::CommandLine::ForCurrentProcess()->GetSwitchValueASCII(param);
+  if (param_str.empty()) {
+    return;
+  }
+
+  uint param_parsed = 0;

[Ryan Sleevi, 2016/02/19 22:56:07]

BUG: C++ does not define "uint". Please use one of the standard types from the
language or stddef.h.

In this case, your API contract is "unsigned".

[Kevin M, 2016/02/22 22:53:31]

Done.

+  bool is_valid = base::StringToUint(param_str, &param_parsed) &&
+                  param_parsed > 0 && param_parsed <= 65535;
+  CHECK(is_valid) << "Invalid range for parameter " << param;

[Ryan Sleevi, 2016/02/19 22:56:08]

DESIGN: Please use //base/numerics/safe_conversions.h checked_cast for
converting "unsigned" to uint16_t

[Kevin M, 2016/02/22 22:53:31]

Done. Should ParseHostAndPort be made to return |port| as an uint16_t* instead
of an int*?

+  *output = param_parsed;
+}
+
+// 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(cert_str);
+  DCHECK(!cert_str->empty());
+
+  net::PEMTokenizer pem_tokenizer(*cert_str, {"CERTIFICATE"});

[Ryan Sleevi, 2016/02/19 22:56:07]

STYLE: Uniform initialization syntax is explicitly forbidden in Chromium - see
https://chromium-cpp.appspot.com/#core-blacklist

[Kevin M, 2016/02/22 22:53:31]

Done. (FYI, other PEMTokenizer clients do the same thing and should be clened up
- though, they shouldn't be using that class in the first place?)

[Kevin M, 2016/02/23 00:28:09]

PEMTokenizer was removed and replaced with CreateCertificateListFromBytes.

+  while (pem_tokenizer.GetNext()) {
+    CHECK(!assignment->cert) << "More than one CERTIFICATE entries provided.";
+    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 +95,40 @@ AssignmentSource::AssignmentSource(
 
 AssignmentSource::~AssignmentSource() {}
 
+scoped_refptr<base::SingleThreadTaskRunner>

[Ryan Sleevi, 2016/02/19 22:56:08]

DESIGN: Your API contract does not require the use of a SingleThreadTaskRunner;
you could return a better type, such as a SequencedTaskRunner or, ideally, a
TaskRunner, which accomplishes your goal/use case.

[Kevin M, 2016/02/22 22:53:31]

The URLRequestContextGetter (now integrated on trunk, see the rebased file)
needs a SingleThreadTaskRunner.

I'll store it in a field as a plain ol' TaskRunner for
PostBackAndReplyWithResult.

+AssignmentSource::GetIOTaskRunner() {
+  if (!io_thread_) {
+    io_thread_.reset(new base::Thread("CertFileThread"));
+    base::Thread::Options options(base::MessageLoop::TYPE_IO, 0);
+    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);
+  GetUint16Parameter(switches::kEngineSSLPort, &assignment->ssl_port);
+  if (assignment->ssl_port > 0) {
+    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);
+    std::string* cert_str_ptr = cert_str.get();

[Ryan Sleevi, 2016/02/19 22:56:08]

PostTaskAndReplyWithResult explicitly exists to avoid needing this level of heap
allocation gymanstics

Change your signature to

std::string ReadFromDisk(const base::FilePath& path) to accomodate this.

[Kevin M, 2016/02/22 22:53:31]

Thanks, done. I switched GetCustomAssignment to use PostTaskAndReplyWithResult,
and it's a nice improvement.

+    GetIOTaskRunner()->PostTaskAndReply(

[Ryan Sleevi, 2016/02/19 22:56:07]

BUG/DESIGN: This seems improperly named, or at least, likely to confuse any
Chromie that works on this, as the IO thread is *not* allowed to do file access
(that's handled by a file thread).

Nor is it necessary for your use case to use a MessageLoop::TYPE_IO file thread
for this, IIRC ( I believe all our FILE threads are just normal MessageLoops)

[Kevin M, 2016/02/22 22:53:31]

Question: how do we create a FILE thread using base::Thread?

The client doesn't depend on content/ so we don't have access to the usual
BrowserThreads.

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

+1 on Ryan's comment; it was very confusing to me to see references to "the IO
thread" everywhere.

You don't need to use the FILE thread if you can't depend on content/ (in fact,
even if you could, you should probably use BrowserThread::GetBlockingPool()
instead), just don't refer to the thread as the IO thread or task runner, and
remove the MessageLoop::TYPE_IO.

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

In the latest patch, you'll notice that we take a task runner from the object
owner for network and file tasks. That would be considered an IO thread,
wouldn't it?

Re: using BrowserThread::GetBlockingPool, we have a "-content" DEPS exclusion
rule on the Android client to keep us honest about not pulling in content/.

+        FROM_HERE, base::Bind(&ReadFromDisk, cert_path, cert_str_ptr),
+        base::Bind(&ParseCertForAssignment, base::Passed(std::move(cert_str)),
+                   base::Passed(std::move(assignment)), callback));

[Ryan Sleevi, 2016/02/19 22:56:07]

DESIGN: While this is only one level, I find myself frequently discouraging
people from this pattern, because it makes it difficult to follow the code flow
and execution state machine, and makes it easy to introduce bugs later on, as
time has repeatedly borne out.

Our encouragement from //net is to consider explicit state machines to make this
clear - but this is up to you and your team, who will ultimately maintain this.

[Kevin M, 2016/02/22 22:53:31]

Ack.

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