Blimp: add support for SSL connections.

blimp/client/session/assignment_source.h

 // 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.
 
 #ifndef BLIMP_CLIENT_SESSION_ASSIGNMENT_SOURCE_H_
 #define BLIMP_CLIENT_SESSION_ASSIGNMENT_SOURCE_H_
 
 #include <string>
+#include <vector>
 
 #include "base/callback.h"
+#include "base/files/file_path.h"
+#include "base/threading/thread.h"
 #include "blimp/client/blimp_client_export.h"
-#include "net/base/ip_endpoint.h"
+#include "net/base/address_list.h"
+#include "net/base/hash_value.h"
+#include "net/cert/x509_certificate.h"
 
 namespace base {
 class SingleThreadTaskRunner;
 }
 
 namespace blimp {
 namespace client {
 
 // An Assignment contains the configuration data needed for a client
 // to connect to the engine.
 struct BLIMP_CLIENT_EXPORT Assignment {
-  net::IPEndPoint ip_endpoint;
+  Assignment();
+  ~Assignment();

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

IIUC you only need these because the struct is no longer purely POD?

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

Correct

+
+  std::vector<net::IPAddress> ip_addresses;
+  int16_t tcp_port;
+  int16_t ssl_port;

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

Can you inline initialize these?

These need to be uint16_t, otherwise they can't hold the full potential port
range; your command-line parsing code uses uint16_t internally, so you're
implicitly reinterpreting when you assign from one to the other - I'm a little
surprising the compiler didn't complain!

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

vOn 2016/02/12 02:31:13, Wez wrote:
Done
Hmm, yes - why no warning? <_<

Done.

   std::string client_token;
+  scoped_refptr<net::X509Certificate> cert;
 };
 
 // AssignmentSource provides functionality to find out how a client should
 // connect to an engine.
 class BLIMP_CLIENT_EXPORT AssignmentSource {

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

It seems strange to be bundling the dev-mode file-based assignment fetch into
this class - I'd assumed we'd have two discrete implementations, one for
dev-mode and one for production. It looks like you're planning on having this
class implement both, though?

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

I think the boundary between "dev mode" and "production" is blurry - we might
want to run a release APK on an local engine instance, for example.

  public:
   typedef const base::Callback<void(const Assignment&)> AssignmentCallback;
 
   // The |main_task_runner| should be the task runner for the UI thread because
   // this will in some cases be used to trigger user interaction on the UI
   // thread.
   AssignmentSource(
       const scoped_refptr<base::SingleThreadTaskRunner>& main_task_runner);
   virtual ~AssignmentSource();

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

Why is this dtor virtual?

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

Done.

 
   // Retrieves a valid assignment for the client and posts the result to the
   // given callback.
   void GetAssignment(const AssignmentCallback& callback);
 
  private:
+  // Reads the contents of |path| into a string.
+  // Sends the contents to |callback| on the IO thread.

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

This comment looks to be incorrect?

As far as I can tell there is no reason for either of these functions to even
exist in this class, so having them here just increases the risk of future code
changes breaking things, with no net benefit.

You're just trying to turn a GetAssignment call into a file-read on the
IO-thread followed by a decode of the PEM file (does that also have to be on the
IO thread?), so all you need is for GetAssignment to PostTask a static function
with all the necessary data, including the calling task-runner and callback,
bound to it.

Better still, you could use PostTaskAndReply to do some of the thread-hopping
for you.

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

Done.

+  void ReadFromDisk(base::FilePath path,

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

|path| should be const& here

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

Done.

+                    const base::Callback<void(const std::string&)>& callback);
+
+  // Parses a certificate from PEM-encoded |cert_str| and attaches it to
+  // |assignment|. Returns the populated assignment object via |callback|.
+  void ParseCertForAssignment(Assignment assignment,

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

nit: It's really strange to be passing Assignment by-value; it'll end up being
copied (expensive, since it's non-POD) and then be mutable by the recipient.

As noted, though, it doesn't look like you actually need this method at all?

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

Passing around Assignments as scoped_ptr now.

+                              const AssignmentCallback& callback,
+                              const std::string& cert_str);
+
   scoped_refptr<base::SingleThreadTaskRunner> main_task_runner_;
 
+  // Thread for executing cert file reads.
+  // TODO(kmarshall): Use BlimpClientSession IO thread task runner.
+  // dtrainor@ will land a change that moves AssignmentSource into the
+  // BlimpClientSession, which manages its own IO thread.
+  base::Thread io_thread_;
+
   DISALLOW_COPY_AND_ASSIGN(AssignmentSource);
 };
 
 }  // namespace client
 }  // namespace blimp
 
 #endif  // BLIMP_CLIENT_SESSION_ASSIGNMENT_SOURCE_H_