An implementation of chrome.socket.secure().

chrome/browser/extensions/api/socket/tls_socket.h

+// Copyright (c) 2013 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 CHROME_BROWSER_EXTENSIONS_API_SOCKET_TLS_SOCKET_H_
+#define CHROME_BROWSER_EXTENSIONS_API_SOCKET_TLS_SOCKET_H_
+
+#include <string>
+
+#include "chrome/browser/extensions/api/socket/socket.h"
+#include "chrome/browser/extensions/api/socket/socket_api.h"
+#include "chrome/browser/extensions/api/socket/tcp_socket.h"
+
+namespace net {
+class Socket;
+}
+
+namespace extensions {
+class TLSSocket;
+typedef base::Callback<void(TLSSocket*, int)> SecureCallback;
+
+// TLS Sockets from the chrome.socket API.  A regular TCPSocket is converted
+// to a TLSSocket via chrome.socket.secure() or chrome.sockets.tcp.secure().
+// The inheritance here is for interface API compatibility, not for
+// implementation that comes with it.  We override methods to prevent
+// behavioral leakage from the underlying implementation.

[Ryan Sleevi, 2013/12/17 00:37:40]

style nit: single space after a period, not double. Applies throughout.

style nit: Pronouns in comments considered harmful -
https://groups.google.com/a/chromium.org/d/msg/chromium-dev/NH-S6KCkr2M/yv0z3...

For what it's worth, I'm still confused on that last sentence.

[lally, 2014/01/09 18:47:16]

Fixed, with a re-written last sentence.

+class TLSSocket : public ResumableTCPSocket {
+ public:
+  TLSSocket(net::StreamSocket* tls_socket,
+            const std::string& owner_extension_id);
+
+  virtual ~TLSSocket();
+
+  // Most of these methods either fail or forward the method call on to the
+  // inner net::StreamSocket.  The remaining few do actual TLS work.
+
+  // Fails.
+  virtual void Connect(const std::string& address,
+                       int port,
+                       const CompletionCallback& callback) OVERRIDE;

[Ryan Sleevi, 2013/12/17 00:37:40]

This still heavily rankles my design sentiments, because this clearly violates
the is-a inheritance model being suggested here.

I'd like to be able to find a solution that doesn't require this. If we can't,
we've got a design issue with the sockets API in general. Same for every other
"Fails" below.

[lally, 2014/01/09 18:47:16]

I don't know how to get around this issue without rewriting the sockets API
implementation.  The Fails/Forwards comments are part of an attempt to at least
document the impact of that design decision as it impacts TLSSocket.

+  // Forwards.
+  virtual void Disconnect() OVERRIDE;
+
+  // The |callback| will be called with the number of bytes read into the
+  // buffer, or a negative number if an error occurred.  Does TLS work.
+  virtual void Read(int count,
+                    const ReadCompletionCallback& callback) OVERRIDE;
+
+  // Fails.  This should have been called on the TCP socket before secure() was
+  // invoked.
+  virtual bool SetKeepAlive(bool enable, int delay) OVERRIDE;
+
+  // Fails.  This should have been called on the TCP socket before secure() was
+  // invoked.
+  virtual bool SetNoDelay(bool no_delay) OVERRIDE;
+
+  // Fails.  TLSSocket is only a client.
+  virtual int Listen(const std::string& address, int port, int backlog,
+                     std::string* error_msg) OVERRIDE;
+  // Fails.  TLSSocket is only a client.
+  virtual void Accept(const AcceptCompletionCallback &callback) OVERRIDE;
+
+  // Forwards.
+  virtual bool IsConnected() OVERRIDE;
+
+  // Forwards.
+  virtual bool GetPeerAddress(net::IPEndPoint* address) OVERRIDE;
+  // Forwards.
+  virtual bool GetLocalAddress(net::IPEndPoint* address) OVERRIDE;
+
+  // Returns TYPE_TLS.
+  virtual SocketType GetSocketType() const OVERRIDE;
+
+  // Convert the given |socket| to a TLS socket.  Works only for TCP sockets.
+  // Must be invoked in the IO thread.  |callback| will always be invoked.
+  // Note: |callback| may get invoked before SecureTCPSocket returns.
+  static void SecureTCPSocket(
+    Socket* socket,
+    Profile* profile,
+    net::URLRequestContextGetter* url_request_getter,
+    const std::string& extension_id,
+    api::socket::SecureOptions* options,
+    SecureCallback callback);

[Ryan Sleevi, 2013/12/17 00:37:40]

style nit: four space indent

comment nit: Explain the lifetime requirements for |profile|

comment/style nit: Is ownership of |socket| transferred? If so, it should be
scoped_ptr<Socket> and use our C++11 emulation of scoped_ptr<T>::Pass()

style nit: "SecureCallback" -> "const SecureCallback&"

naming nit: SecureTCPSocket is arguably ambiguous with respect to "Is it a verb
- eg: secure this TCP socket - or a noun - eg: this is a constructor for a
secure TCP socket". Perhaps "UpgradeSocketToSecureSocket" or something equally
descriptive and unambiguously verb-ified.

[lally, 2014/01/09 18:47:16]

I chose UpgradeSocketToTLS, to avoid having Socket in there twice.

+
+ private:
+  virtual int WriteImpl(net::IOBuffer* io_buffer, int io_buffer_size,
+                        const net::CompletionCallback& callback) OVERRIDE;

[Ryan Sleevi, 2013/12/17 00:37:40]

style: line breaks -
http://www.chromium.org/developers/coding-style#Code_Formatting

[lally, 2014/01/09 18:47:16]

Done.

+  void OnReadComplete(scoped_refptr<net::IOBuffer> io_buffer, int result);

[Ryan Sleevi, 2013/12/17 00:37:40]

style nit: "scoped_refptr<net::IOBuffer>" -> "const scoped_refptr<IOBuffer>&"

[lally, 2014/01/09 18:47:16]

Done.

+
+  scoped_ptr<net::StreamSocket> tls_socket_;
+
+  ReadCompletionCallback read_callback_;
+};
+
+}  // namespace extensions
+
+#endif  // CHROME_BROWSER_EXTENSIONS_API_SOCKET_TLS_SOCKET_H_