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 "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"
 #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.
 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;
-}
-
 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),
@@ skipping 38 matching lines @@
 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;
 }
 
 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)) {}
+    : io_task_runner_(io_task_runner),
+      url_request_context_(new SimpleURLRequestContextGetter(io_task_runner)),
+      file_reader_(new FileReader) {}
 
 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());
   }
   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(&AssignmentSource::GetCustomAssignment,
+                 base::Unretained(this)),
+      base::Bind(&AssignmentSource::OnGetCustomAssignmentDone,
+                 base::Unretained(this), client_auth_token));
+}
+
+scoped_ptr<Assignment> AssignmentSource::GetCustomAssignment() {
+  DCHECK(io_task_runner_->RunsTasksOnCurrentThread());

[Ryan Sleevi, 2016/02/23 00:49:45]

It makes me deeply nervous to have an object that lives on multiple threads, as
that's a ripe source of bugs. Also, the use of base::Unretained(this) [lines
131-134] are definitely a warning sign that "Here is code we can no longer
reason about"

From examining your interface visibility, it does not seem like this needs to be
a member method - that is, it seems (and perhaps I missed something) that this
could be entirely accomplished within an unnamed namespace in your .cc file -
which not only hides your abstraction more, but helps reason about the classes.

Separately, I would question if 133-134 should be using base::Unretained(this) -
my instinct is htat you should be using WeakPtrs to handle the AssignmentSource
going away mid-operation.

[Kevin M, 2016/02/23 01:58:25]

Made it an anonymous namespaced function and added WeakPtrs.

+  scoped_ptr<Assignment> assignment(new Assignment);
+  GURL url(base::CommandLine::ForCurrentProcess()->GetSwitchValueASCII(
+      switches::kEngineEndpoint));
+
+  std::string host;
+  int port;
+  if (url.is_empty() || !url.is_valid() || !url.has_scheme() ||
+      !net::ParseHostAndPort(url.path(), &host, &port)) {
+    return nullptr;
+  }
+
+  net::IPAddress ip_address;
+  CHECK(ip_address.AssignFromIPLiteral(host)) << "Invalid Assignment host "
+                                              << host;
+  CHECK(port > 0 && port < 65535) << "Invalid IP port number: " << port;

[Ryan Sleevi, 2016/02/23 00:49:45]

Why do you check this, given url.is_valid()?

[Kevin M, 2016/02/23 01:58:25]

Done.

+
+  Assignment::TransportProtocol protocol =
+      Assignment::TransportProtocol::UNKNOWN;
+  if (url.has_scheme()) {

[Ryan Sleevi, 2016/02/23 00:49:45]

Why do you check this, given line 145?

[Kevin M, 2016/02/23 01:58:25]

Done.

+    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 engine protocol scheme " << url.scheme();
+    }
+  }
+
+  scoped_refptr<net::X509Certificate> cert;
+  if (protocol == Assignment::TransportProtocol::SSL ||
+      protocol == 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(file_reader_->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 = cert_list[0];
+  }
+
+  assignment->transport_protocol = protocol;
+  assignment->ip_endpoint =
+      net::IPEndPoint(ip_address, base::checked_cast<uint16_t>(port));

[Ryan Sleevi, 2016/02/23 00:49:45]

Why do you do this, given line 153?

[Kevin M, 2016/02/23 01:58:25]

In the previous patch you recommended using checked_cast to downcast an int to
an uint16_t. IPEndPoint takes a uint16_t in its ctor.

+  assignment->client_token = kDummyClientToken;
+  assignment->cert = cert;
+  return assignment;
+}
+
+void AssignmentSource::OnGetCustomAssignmentDone(
+    const std::string& client_auth_token,
+    scoped_ptr<Assignment> custom_assignment) {
+  // If GetCustomAssignment succeeded, then return the custom assignment
+  // directly.
+  if (custom_assignment && !custom_assignment->is_null()) {
+    base::ResetAndReturn(&callback_)
+        .Run(AssignmentSource::RESULT_OK, *custom_assignment);

[Ryan Sleevi, 2016/02/23 00:49:45]

The API contract for using a heap-allocated custom_assignment seems... weird. Is
that just to curry whether or not a custom assignment was used? It feels weird
when it gets to line 239 & friends.

[Kevin M, 2016/02/23 01:58:25]

Done.

     return;
   }
 
   // Call out to the network for a real assignment.  Build the network request
   // 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::SetFileReaderForTest(scoped_ptr<FileReader> reader) {
+  file_reader_ = std::move(reader);
+}
+
 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 37 matching lines @@
 
   // 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);

[Ryan Sleevi, 2016/02/23 00:49:45]

SECURITY: In Chrome code, we never process JSON in a trusted process, we use the
utility process for this. This is true regardless of it coming from a 'trusted
source'

[Kevin M, 2016/02/23 01:58:25]

Interesting. Is there a Codesearch link or a doc that I can use as a reference
for doing that?

+dtrainor@chromium.org FYI

[Kevin M, 2016/02/23 20:38:15]

Filed bug https://bugs.chromium.org/p/chromium/issues/detail?id=589202 to track
this.

[David Trainor- moved to gerrit, 2016/02/23 21:34:29]

Hmm it looks like there are multiple instances of this happening in the browser
process currently (GaiaAuthFetcher?  I tracked it back to the
AccountFetcherService which looks like it's running in the Browser process, but
I'm not 100% sure).

Should we update json_reader.h to say something along these lines in the header?

[Kevin M, 2016/02/23 22:01:49]

Issue is resolved. I modified the JSON parsing code to use safe_json instead.

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

So, uh, I hate to be the bearer of bad news here, but we do parse JSON from a
trusted source in the browser in a _lot_ of places. So far the guidance I've
received from the security team has been that we only need to parse _untrusted_
JSON out-of-process.

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

Is output from googleapis.com, as in this case, considered a "trusted source"?

   if (!json) {
     base::ResetAndReturn(&callback_)
         .Run(AssignmentSource::Result::RESULT_BAD_RESPONSE, Assignment());
     return;
   }
 
   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)) {

[Ryan Sleevi, 2016/02/23 00:49:45]

Why do you soft-fail these errors, but CHECK on the others? (Line 191 for
example)

[Kevin M, 2016/02/23 01:58:25]

This code handles responses received over the network so errors should be
handled gracefully.

Lines 191 et al parse command line arguments that are provided by developers, so
abrupt termination w/a stack trace is a nice thing to have.

     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_)

[Ryan Sleevi, 2016/02/23 00:49:44]

The heavy use of base::ResetAndReturn(...) makes me think that callback_ can
invalidate the present object. Is that the case? If so, this does seem like a
rather dangerous API.

[Kevin M, 2016/02/23 01:58:25]

No, it's a safety practice my team has been using to turn quietly buggy callback
reuse into segfaults.

+        .Run(AssignmentSource::Result::RESULT_INVALID_CERT, Assignment());
+    return;
+  }
+  scoped_refptr<net::X509Certificate> cert = std::move(cert_list[0]);

[Ryan Sleevi, 2016/02/23 00:49:45]

Why do you std::move() here, only to copy-assign on line 346?

[Kevin M, 2016/02/23 01:58:25]

:P Good point.

+
   // 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 = cert;
 
   base::ResetAndReturn(&callback_)
       .Run(AssignmentSource::Result::RESULT_OK, assignment);
 }
 
+FileReader::FileReader() {}
+
+FileReader::~FileReader() {}
+
+bool FileReader::ReadFileToString(const base::FilePath& path,
+                                  std::string* output) {
+  return base::ReadFileToString(path, output);
+}
+
 }  // namespace client
 }  // namespace blimp