Split SSLKeyLogger into an interface and implementation.

Someday in the future, the network service will be sandboxed, at which
point file access won't be allowed. We still need to deal with the
SSLClientContext mess but we can do the interface split now. This also
removes an OS_NACL special-case.

Bug: 838910
Cq-Include-Trybots: master.tryserver.chromium.android:android_cronet_tester;master.tryserver.chromium.mac:ios-simulator-cronet
Change-Id: I607f18b26a357de56b13aa2453caa2f40f2ecacb
Reviewed-on: https://chromium-review.googlesource.com/1128189
Reviewed-by: Matt Menke <mmenke@chromium.org>
Reviewed-by: Helen Li <xunjieli@chromium.org>
Reviewed-by: Sami Kyöstilä <skyostil@chromium.org>
Commit-Queue: David Benjamin <davidben@chromium.org>
Cr-Commit-Position: refs/heads/master@{#574212}

chrome/browser/io_thread.cc

@@ -80,6 +80,7 @@
 #include "net/proxy_resolution/proxy_resolution_service.h"
 #include "net/quic/chromium/quic_utils_chromium.h"
 #include "net/socket/ssl_client_socket.h"
+#include "net/ssl/ssl_key_logger_impl.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"
@@ -334,8 +335,10 @@ void IOThread::Init() {
 
   // Export ssl keys if log file specified.
   base::FilePath ssl_keylog_file = GetSSLKeyLogFile(command_line);
-  if (!ssl_keylog_file.empty())
-    net::SSLClientSocket::SetSSLKeyLogFile(ssl_keylog_file);
+  if (!ssl_keylog_file.empty()) {
+    net::SSLClientSocket::SetSSLKeyLogger(
+        std::make_unique<net::SSLKeyLoggerImpl>(ssl_keylog_file));
+  }
 
   DCHECK(!globals_);
   globals_ = new Globals;

components/cronet/ios/cronet_environment.mm

@@ -44,6 +44,7 @@
 #include "net/proxy_resolution/proxy_resolution_service.h"
 #include "net/socket/ssl_client_socket.h"
 #include "net/ssl/channel_id_service.h"
+#include "net/ssl/ssl_key_logger_impl.h"
 #include "net/third_party/quic/core/quic_versions.h"
 #include "net/url_request/http_user_agent_settings.h"
 #include "net/url_request/url_request_context.h"
@@ -311,7 +312,8 @@ bool IsNetLogPathValid(const base::FilePath& path) {
   if (!ssl_key_log_file_set && !ssl_key_log_file_name_.empty()) {
     ssl_key_log_file_set = true;
     base::FilePath ssl_key_log_file(ssl_key_log_file_name_);
-    net::SSLClientSocket::SetSSLKeyLogFile(ssl_key_log_file);
+    net::SSLClientSocket::SetSSLKeyLogger(
+        std::make_unique<net::SSLKeyLoggerImpl>(ssl_key_log_file));
   }
 
   if (user_agent_partial_)

components/cronet/url_request_context_config.cc

@@ -31,6 +31,7 @@
 #include "net/quic/chromium/quic_utils_chromium.h"
 #include "net/reporting/reporting_policy.h"
 #include "net/socket/ssl_client_socket.h"
+#include "net/ssl/ssl_key_logger_impl.h"
 #include "net/third_party/quic/core/quic_packets.h"
 #include "net/url_request/url_request_context_builder.h"
 
@@ -456,12 +457,13 @@ void URLRequestContextConfig::ParseAndSetExperimentalOptions(
         base::FilePath ssl_key_log_file(
             base::FilePath::FromUTF8Unsafe(ssl_key_log_file_string));
         if (!ssl_key_log_file.empty()) {
-          // SetSSLKeyLogFile is only safe to call before any SSLClientSockets
+          // SetSSLKeyLogger is only safe to call before any SSLClientSockets
           // are created. This should not be used if there are multiple
           // CronetEngine.
           // TODO(xunjieli): Expose this as a stable API after crbug.com/458365
           // is resolved.
-          net::SSLClientSocket::SetSSLKeyLogFile(ssl_key_log_file);
+          net::SSLClientSocket::SetSSLKeyLogger(
+              std::make_unique<net::SSLKeyLoggerImpl>(ssl_key_log_file));
         }
       }
     } else if (it.key() == kNetworkQualityEstimatorFieldTrialName) {

headless/app/headless_shell.cc

@@ -41,6 +41,7 @@
 #include "net/base/net_errors.h"
 #include "net/http/http_util.h"
 #include "net/socket/ssl_client_socket.h"
+#include "net/ssl/ssl_key_logger_impl.h"
 #include "services/network/public/cpp/network_switches.h"
 #include "ui/base/ui_base_switches.h"
 #include "ui/gfx/geometry/size.h"
@@ -163,8 +164,10 @@ void HeadlessShell::OnStart(HeadlessBrowser* browser) {
   // are created via DevTools later.
   base::FilePath ssl_keylog_file =
       GetSSLKeyLogFile(base::CommandLine::ForCurrentProcess());
-  if (!ssl_keylog_file.empty())
-    net::SSLClientSocket::SetSSLKeyLogFile(ssl_keylog_file);
+  if (!ssl_keylog_file.empty()) {
+    net::SSLClientSocket::SetSSLKeyLogger(
+        std::make_unique<net::SSLKeyLoggerImpl>(ssl_keylog_file));
+  }
 
   if (base::CommandLine::ForCurrentProcess()->HasSwitch(::switches::kLang)) {
     context_builder.SetAcceptLanguage(

net/BUILD.gn

@@ -355,6 +355,7 @@ component("net") {
     "ssl/ssl_connection_status_flags.h",
     "ssl/ssl_info.cc",
     "ssl/ssl_info.h",
+    "ssl/ssl_key_logger.h",
     "ssl/ssl_private_key.cc",
     "ssl/ssl_private_key.h",
     "ssl/ssl_server_config.cc",
@@ -1102,8 +1103,8 @@ component("net") {
       "ssl/client_cert_store_win.h",
       "ssl/ssl_config_service_defaults.cc",
       "ssl/ssl_config_service_defaults.h",
-      "ssl/ssl_key_logger.cc",
-      "ssl/ssl_key_logger.h",
+      "ssl/ssl_key_logger_impl.cc",
+      "ssl/ssl_key_logger_impl.h",
       "ssl/ssl_platform_key_android.cc",
       "ssl/ssl_platform_key_android.h",
       "ssl/ssl_platform_key_mac.cc",

net/socket/ssl_client_socket.cc

@@ -7,12 +7,12 @@
 #include "base/metrics/histogram_macros.h"
 #include "base/metrics/sparse_histogram.h"
 #include "base/strings/string_util.h"
-#include "build/build_config.h"
 #include "crypto/ec_private_key.h"
 #include "net/base/net_errors.h"
 #include "net/socket/ssl_client_socket_impl.h"
 #include "net/ssl/channel_id_service.h"
 #include "net/ssl/ssl_config_service.h"
+#include "net/ssl/ssl_key_logger.h"
 
 namespace net {
 
@@ -21,12 +21,8 @@ SSLClientSocket::SSLClientSocket()
       stapled_ocsp_response_received_(false) {}
 
 // static
-void SSLClientSocket::SetSSLKeyLogFile(const base::FilePath& path) {
-#if !defined(OS_NACL)
-  SSLClientSocketImpl::SetSSLKeyLogFile(path);
-#else
-  NOTIMPLEMENTED();
-#endif
+void SSLClientSocket::SetSSLKeyLogger(std::unique_ptr<SSLKeyLogger> logger) {
+  SSLClientSocketImpl::SetSSLKeyLogger(std::move(logger));
 }
 
 bool SSLClientSocket::IgnoreCertError(int error, int load_flags) {

net/socket/ssl_client_socket.h

@@ -20,16 +20,13 @@
 #include "net/socket/stream_socket.h"
 #include "net/ssl/token_binding.h"
 
-namespace base {
-class FilePath;
-}
-
 namespace net {
 
 class CTPolicyEnforcer;
 class CertVerifier;
 class ChannelIDService;
 class CTVerifier;
+class SSLKeyLogger;
 class TransportSecurityState;
 
 // This struct groups together several fields which are used by various
@@ -70,14 +67,12 @@ class NET_EXPORT SSLClientSocket : public SSLSocket {
  public:
   SSLClientSocket();
 
-  // Log SSL key material to |path|. Must be called before any
+  // Log SSL key material to |logger|. Must be called before any
   // SSLClientSockets are created.
   //
   // TODO(davidben): Switch this to a parameter on the SSLClientSocketContext
-  // once https://crbug.com/458365 is resolved. To avoid a dependency from
-  // OS_NACL to file I/O logic, this will require splitting SSLKeyLogger into an
-  // interface, built with OS_NACL and a non-NaCl SSLKeyLoggerImpl.
-  static void SetSSLKeyLogFile(const base::FilePath& path);
+  // once https://crbug.com/458365 is resolved.
+  static void SetSSLKeyLogger(std::unique_ptr<SSLKeyLogger> logger);
 
   // Returns true if |error| is OK or |load_flags| ignores certificate errors
   // and |error| is a certificate error.

net/socket/ssl_client_socket_impl.cc

@@ -51,6 +51,7 @@
 #include "net/ssl/ssl_client_session_cache.h"
 #include "net/ssl/ssl_connection_status_flags.h"
 #include "net/ssl/ssl_info.h"
+#include "net/ssl/ssl_key_logger.h"
 #include "net/ssl/ssl_private_key.h"
 #include "net/ssl/token_binding.h"
 #include "net/traffic_annotation/network_traffic_annotation.h"
@@ -61,10 +62,6 @@
 #include "third_party/boringssl/src/include/openssl/mem.h"
 #include "third_party/boringssl/src/include/openssl/ssl.h"
 
-#if !defined(OS_NACL)
-#include "net/ssl/ssl_key_logger.h"
-#endif
-
 #if !defined(NET_DISABLE_BROTLI)
 #include "third_party/brotli/include/brotli/decode.h"
 #endif
@@ -315,13 +312,11 @@ class SSLClientSocketImpl::SSLContext {
     return SSL_set_ex_data(ssl, ssl_socket_data_index_, socket) != 0;
   }
 
-#if !defined(OS_NACL)
-  void SetSSLKeyLogFile(const base::FilePath& path) {
+  void SetSSLKeyLogger(std::unique_ptr<SSLKeyLogger> logger) {
     DCHECK(!ssl_key_logger_);
-    ssl_key_logger_.reset(new SSLKeyLogger(path));
+    ssl_key_logger_ = std::move(logger);
     SSL_CTX_set_keylog_callback(ssl_ctx_.get(), KeyLogCallback);
   }
-#endif
 
   static const SSL_PRIVATE_KEY_METHOD kPrivateKeyMethod;
 
@@ -397,11 +392,9 @@ class SSLClientSocketImpl::SSLContext {
     return socket->PrivateKeyCompleteCallback(out, out_len, max_out);
   }
 
-#if !defined(OS_NACL)
   static void KeyLogCallback(const SSL* ssl, const char* line) {
     GetInstance()->ssl_key_logger_->WriteLine(line);
   }
-#endif
 
   static void InfoCallback(const SSL* ssl, int type, int value) {
     SSLClientSocketImpl* socket = GetInstance()->GetClientSocketFromSSL(ssl);
@@ -425,9 +418,7 @@ class SSLClientSocketImpl::SSLContext {
 
   bssl::UniquePtr<SSL_CTX> ssl_ctx_;
 
-#if !defined(OS_NACL)
   std::unique_ptr<SSLKeyLogger> ssl_key_logger_;
-#endif
 
   // TODO(davidben): Use a separate cache per URLRequestContext.
   // https://crbug.com/458365
@@ -491,12 +482,10 @@ SSLClientSocketImpl::~SSLClientSocketImpl() {
   Disconnect();
 }
 
-#if !defined(OS_NACL)
-void SSLClientSocketImpl::SetSSLKeyLogFile(
-    const base::FilePath& ssl_keylog_file) {
-  SSLContext::GetInstance()->SetSSLKeyLogFile(ssl_keylog_file);
+void SSLClientSocketImpl::SetSSLKeyLogger(
+    std::unique_ptr<SSLKeyLogger> logger) {
+  SSLContext::GetInstance()->SetSSLKeyLogger(std::move(logger));
 }
-#endif
 
 int SSLClientSocketImpl::ExportKeyingMaterial(const base::StringPiece& label,
                                               bool has_context,

net/socket/ssl_client_socket_impl.h

@@ -17,7 +17,6 @@
 #include "base/macros.h"
 #include "base/memory/ref_counted.h"
 #include "base/memory/weak_ptr.h"
-#include "build/build_config.h"
 #include "net/base/completion_callback.h"
 #include "net/base/io_buffer.h"
 #include "net/cert/cert_verifier.h"
@@ -37,7 +36,6 @@
 #include "third_party/boringssl/src/include/openssl/ssl.h"
 
 namespace base {
-class FilePath;
 namespace trace_event {
 class ProcessMemoryDump;
 }
@@ -53,6 +51,7 @@ class CertVerifier;
 class CTVerifier;
 class SSLCertRequestInfo;
 class SSLInfo;
+class SSLKeyLogger;
 
 using TokenBindingSignatureMap =
     base::MRUCache<std::pair<TokenBindingType, std::string>,
@@ -76,11 +75,9 @@ class SSLClientSocketImpl : public SSLClientSocket,
     return ssl_session_cache_shard_;
   }
 
-#if !defined(OS_NACL)
-  // Log SSL key material to |path|. Must be called before any SSLClientSockets
-  // are created.
-  static void SetSSLKeyLogFile(const base::FilePath& path);
-#endif
+  // Log SSL key material to |logger|. Must be called before any
+  // SSLClientSockets are created.
+  static void SetSSLKeyLogger(std::unique_ptr<SSLKeyLogger> logger);
 
   // SSLSocket implementation.
   int ExportKeyingMaterial(const base::StringPiece& label,

net/ssl/ssl_key_logger.h

@@ -5,43 +5,24 @@
 #ifndef NET_SSL_SSL_KEY_LOGGER_H_
 #define NET_SSL_SSL_KEY_LOGGER_H_
 
-#include <memory>
 #include <string>
 
-#include "base/macros.h"
-#include "base/memory/ref_counted.h"
-
-namespace base {
-class FilePath;
-class SequencedTaskRunner;
-}
+#include "net/base/net_export.h"
 
 namespace net {
 
 // SSLKeyLogger logs SSL key material for debugging purposes. This should only
 // be used when requested by the user, typically via the SSLKEYLOGFILE
 // environment variable. See also
 // https://developer.mozilla.org/en-US/docs/Mozilla/Projects/NSS/Key_Log_Format.
-class SSLKeyLogger {
+class NET_EXPORT SSLKeyLogger {
  public:
-  // Creates a new SSLKeyLogger which writes to |path|, scheduling write
-  // operations in the background.
-  explicit SSLKeyLogger(const base::FilePath& path);
-  ~SSLKeyLogger();
+  virtual ~SSLKeyLogger() {}
 
   // Writes |line| followed by a newline. This may be called by multiple threads
   // simultaneously. If two calls race, the order of the lines is undefined, but
   // each line will be written atomically.
-  void WriteLine(const std::string& line);
-
- private:
-  class Core;
-
-  scoped_refptr<base::SequencedTaskRunner> task_runner_;
-  // Destroyed on |task_runner_|.
-  std::unique_ptr<Core> core_;
-
-  DISALLOW_COPY_AND_ASSIGN(SSLKeyLogger);
+  virtual void WriteLine(const std::string& line) = 0;
 };
 
 }  // namespace net

net/ssl/ssl_key_logger.cc → net/ssl/ssl_key_logger_impl.cc

@@ -2,7 +2,7 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#include "net/ssl/ssl_key_logger.h"
+#include "net/ssl/ssl_key_logger_impl.h"
 
 #include <stdio.h>
 
@@ -21,7 +21,7 @@ namespace net {
 
 // An object which lives on the background SequencedTaskRunner and performs the
 // blocking file operations.
-class SSLKeyLogger::Core {
+class SSLKeyLoggerImpl::Core {
  public:
   Core() { DETACH_FROM_SEQUENCE(sequence_checker_); }
   ~Core() { DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_); }
@@ -49,24 +49,25 @@ class SSLKeyLogger::Core {
   DISALLOW_COPY_AND_ASSIGN(Core);
 };
 
-SSLKeyLogger::SSLKeyLogger(const base::FilePath& path) : core_(new Core) {
+SSLKeyLoggerImpl::SSLKeyLoggerImpl(const base::FilePath& path)
+    : core_(new Core) {
   // The user explicitly asked for debugging information, so these tasks block
   // shutdown to avoid dropping some log entries.
   task_runner_ = base::CreateSequencedTaskRunnerWithTraits(
       {base::MayBlock(), base::TaskShutdownBehavior::BLOCK_SHUTDOWN});
   task_runner_->PostTask(
       FROM_HERE,
-      base::Bind(&Core::OpenFile, base::Unretained(core_.get()), path));
+      base::BindOnce(&Core::OpenFile, base::Unretained(core_.get()), path));
 }
 
-SSLKeyLogger::~SSLKeyLogger() {
+SSLKeyLoggerImpl::~SSLKeyLoggerImpl() {
   task_runner_->DeleteSoon(FROM_HERE, core_.release());
 }
 
-void SSLKeyLogger::WriteLine(const std::string& line) {
+void SSLKeyLoggerImpl::WriteLine(const std::string& line) {
   task_runner_->PostTask(
       FROM_HERE,
-      base::Bind(&Core::WriteLine, base::Unretained(core_.get()), line));
+      base::BindOnce(&Core::WriteLine, base::Unretained(core_.get()), line));
 }
 
 }  // namespace net