Refactor client cert flow with ClientCertificateResponder mojo interface

The problem this CL addresses is that several semantically-different
messages are currently jammed into the response from
OnCertificateRequested. For example, using nullptr for certificate and
private key to signify that no certificate was selected.

This CL eliminates the response from OnCertificateRequested and moves
it into several different messages within the new
ClientCertificateResponder interface.

Bug: 946742
Change-Id: I49c63bdc0732a438336a02e558d4cca4d1202ff6
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1589995Reviewed-by: Matt Menke <mmenke@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Reviewed-by: Antoine Labour <piman@chromium.org>
Reviewed-by: David Benjamin <davidben@chromium.org>
Commit-Queue: Daniel Cheng <dcheng@chromium.org>
Cr-Commit-Position: refs/heads/master@{#664537}

content/browser/network_service_client.h

@@ -5,6 +5,10 @@
 #ifndef CONTENT_BROWSER_NETWORK_SERVICE_IMPL_H_
 #define CONTENT_BROWSER_NETWORK_SERVICE_IMPL_H_
 
+#include <memory>
+#include <string>
+#include <vector>
+
 #include "base/macros.h"
 #include "base/memory/memory_pressure_listener.h"
 #include "build/build_config.h"
@@ -50,8 +54,7 @@ class CONTENT_EXPORT NetworkServiceClient
       uint32_t routing_id,
       uint32_t request_id,
       const scoped_refptr<net::SSLCertRequestInfo>& cert_info,
-      network::mojom::NetworkServiceClient::OnCertificateRequestedCallback
-          callback) override;
+      network::mojom::ClientCertificateResponderPtr cert_responder) override;
   void OnSSLCertificateError(uint32_t process_id,
                              uint32_t routing_id,
                              const GURL& url,

services/network/public/mojom/network_service.mojom

@@ -49,6 +49,37 @@ interface AuthChallengeResponder {
    OnAuthCredentials(AuthCredentials? credentials);
 };
 
+// This interface enables the UI to send client certificate selections back to
+// the network service.
+//
+// Defining an interface for this purpose, rather than using a union in the
+// response of OnCertificateRequested, enables the NetworkServiceClient to learn
+// of the URLLoader destruction via the connection error handler.
+interface ClientCertificateResponder {
+  // Use the selected certificate and continue the URLRequest.
+  //
+  // - |provider_name| corresponds to the return value of
+  //   net::SSLPrivateKey::GetProviderName().
+  // - |algorithm_preferences| corresponds to the return value of
+  //   net::SSLPrivateKey::GetAlgorithmPreferences().
+  ContinueWithCertificate(network.mojom.X509Certificate x509_certificate,
+                          string provider_name,
+                          array<uint16> algorithm_preferences,
+                          SSLPrivateKey ssl_private_key);
+
+  // Affirmatively select no certificate (this is cached and can affect future
+  // URLRequests). Does not cancel the URLRequest.
+  //
+  // The connection is continued with no client cert.
+  // net::URLRequest::ContinueWithCertificate(nullptr, nullptr) needs to be
+  // called.
+  ContinueWithoutCertificate();
+
+  // Cancel the URLRequest. The request is aborted.
+  // net::URLRequest::CancelWithError() needs to be called.
+  CancelRequest();
+};
+
 struct LoadInfo {
   uint32 process_id;
   uint32 routing_id;
@@ -73,34 +104,22 @@ interface NetworkServiceClient {
                  AuthChallengeInfo auth_info,
                  URLResponseHead? head,
                  AuthChallengeResponder auth_challenge_responder);
+
   // Called when an SSL certificate requested message is received for client
   // authentication.
-  // The |provider_name| parameter corresponses to the return value of
-  // net::SSLPrivateKey::GetProviderName().
-  // The |algorithm_preferences| parameter corresponds to the return value
-  // of net::SSLPrivateKey::GetAlgorithmPreferences().
-  // The |cancel_certificate_selection| parameter is used to distinguish
-  // between the following two cases because the |x509_certificate| will be
-  // nullptr in both cases:
-  // 1. The connection is continued with no client cert,
-  // net::URLRequest::ContinueWithCertificate(nullptr, nullptr) needs to be
-  // called.
-  // 2. The request is aborted, net::URLRequest::CancelWithError() needs to be
-  // called.
   //
-  // |window_id| or else |process_id| and |routing_id| indicates
-  // the frame making the request, see
-  // network::ResourceRequest::fetch_window_id.
+  // Rather than using one response for multiple purposes, the caller expects
+  // exactly one response (or disconnect) to be sent back via |cert_responder|.
+  //
+  // |window_id| or else |process_id| and |routing_id| indicates the frame
+  // making the request, see network::ResourceRequest::fetch_window_id.
   OnCertificateRequested(mojo_base.mojom.UnguessableToken? window_id,
                          uint32 process_id,
                          uint32 routing_id,
                          uint32 request_id,
-                         network.mojom.SSLCertRequestInfo cert_info) => (
-                             network.mojom.X509Certificate x509_certificate,
-                             string provider_name,
-                             array<uint16> algorithm_preferences,
-                             SSLPrivateKey ssl_private_key,
-                             bool cancel_certificate_selection);
+                         network.mojom.SSLCertRequestInfo cert_info,
+                         ClientCertificateResponder cert_responder);
+
   // Called when an SSL certificate is encountered.
   // The callback argument is a net::ERROR value. If it's net::OK, then the
   // request is resumed. Otherwise it's cancelled with the given error.

services/network/test/test_network_service_client.cc

@@ -4,6 +4,8 @@
 
 #include "services/network/test/test_network_service_client.h"
 
+#include <utility>
+
 #include "base/optional.h"
 #include "base/task/post_task.h"
 #include "base/unguessable_token.h"
@@ -36,7 +38,7 @@ void TestNetworkServiceClient::OnCertificateRequested(
     uint32_t routing_id,
     uint32_t request_id,
     const scoped_refptr<net::SSLCertRequestInfo>& cert_info,
-    mojom::NetworkServiceClient::OnCertificateRequestedCallback callback) {
+    mojom::ClientCertificateResponderPtr client_cert_responder) {
   NOTREACHED();
 }
 

services/network/test/test_network_service_client.h

@@ -5,6 +5,9 @@
 #ifndef SERVICES_NETWORK_TEST_TEST_NETWORK_SERVICE_CLIENT_H_
 #define SERVICES_NETWORK_TEST_TEST_NETWORK_SERVICE_CLIENT_H_
 
+#include <string>
+#include <vector>
+
 #include "build/build_config.h"
 #include "mojo/public/cpp/bindings/binding.h"
 #include "services/network/public/mojom/network_service.mojom.h"
@@ -43,8 +46,7 @@ class TestNetworkServiceClient : public network::mojom::NetworkServiceClient {
       uint32_t routing_id,
       uint32_t request_id,
       const scoped_refptr<net::SSLCertRequestInfo>& cert_info,
-      mojom::NetworkServiceClient::OnCertificateRequestedCallback callback)
-      override;
+      mojom::ClientCertificateResponderPtr client_cert_responder) override;
   void OnSSLCertificateError(uint32_t process_id,
                              uint32_t routing_id,
                              const GURL& url,

services/network/url_loader.cc

@@ -332,6 +332,7 @@ URLLoader::URLLoader(
       do_not_prompt_for_login_(request.do_not_prompt_for_login),
       binding_(this, std::move(url_loader_request)),
       auth_challenge_responder_binding_(this),
+      client_cert_responder_binding_(this),
       url_loader_client_(std::move(url_loader_client)),
       writable_handle_watcher_(FROM_HERE,
                                mojo::SimpleWatcher::ArmingPolicy::MANUAL,
@@ -776,33 +777,40 @@ void URLLoader::OnAuthRequired(net::URLRequest* url_request,
 
 void URLLoader::OnCertificateRequested(net::URLRequest* unused,
                                        net::SSLCertRequestInfo* cert_info) {
+  DCHECK(!client_cert_responder_binding_.is_bound());
+
   if (base::CommandLine::ForCurrentProcess()->HasSwitch(
           switches::kIgnoreUrlFetcherCertRequests) &&
       factory_params_->process_id == 0 &&
       render_frame_id_ == MSG_ROUTING_NONE) {
-    url_request_->ContinueWithCertificate(nullptr, nullptr);
+    ContinueWithoutCertificate();
     return;
   }
 
   if (!network_service_client_) {
-    OnCertificateRequestedResponse(nullptr, std::string(),
-                                   std::vector<uint16_t>(), nullptr,
-                                   true /* cancel_certificate_selection */);
+    ContinueWithoutCertificate();
     return;
   }
 
+  // Set up mojo endpoints for ClientCertificateResponder and bind to the
+  // InterfaceRequest. This enables us to receive messages regarding the client
+  // certificate selection.
+  mojom::ClientCertificateResponderPtr client_cert_responder;
+  auto client_cert_responder_request =
+      mojo::MakeRequest(&client_cert_responder);
+  client_cert_responder_binding_.Bind(std::move(client_cert_responder_request));
+  client_cert_responder_binding_.set_connection_error_handler(
+      base::BindOnce(&URLLoader::CancelRequest, base::Unretained(this)));
+
   if (fetch_window_id_) {
     network_service_client_->OnCertificateRequested(
         fetch_window_id_, -1 /* process_id */, -1 /* routing_id */, request_id_,
-        cert_info,
-        base::BindOnce(&URLLoader::OnCertificateRequestedResponse,
-                       weak_ptr_factory_.GetWeakPtr()));
+        cert_info, std::move(client_cert_responder));
   } else {
     network_service_client_->OnCertificateRequested(
         base::nullopt /* window_id */, factory_params_->process_id,
         render_frame_id_, request_id_, cert_info,
-        base::BindOnce(&URLLoader::OnCertificateRequestedResponse,
-                       weak_ptr_factory_.GetWeakPtr()));
+        std::move(client_cert_responder));
   }
 }
 
@@ -1177,6 +1185,28 @@ void URLLoader::OnAuthCredentials(
   }
 }
 
+void URLLoader::ContinueWithCertificate(
+    const scoped_refptr<net::X509Certificate>& x509_certificate,
+    const std::string& provider_name,
+    const std::vector<uint16_t>& algorithm_preferences,
+    mojom::SSLPrivateKeyPtr ssl_private_key) {
+  client_cert_responder_binding_.Close();
+  auto key = base::MakeRefCounted<SSLPrivateKeyInternal>(
+      provider_name, algorithm_preferences, std::move(ssl_private_key));
+  url_request_->ContinueWithCertificate(std::move(x509_certificate),
+                                        std::move(key));
+}
+
+void URLLoader::ContinueWithoutCertificate() {
+  client_cert_responder_binding_.Close();
+  url_request_->ContinueWithCertificate(nullptr, nullptr);
+}
+
+void URLLoader::CancelRequest() {
+  client_cert_responder_binding_.Close();
+  url_request_->CancelWithError(net::ERR_SSL_CLIENT_AUTH_CERT_NEEDED);
+}
+
 void URLLoader::NotifyCompleted(int error_code) {
   // Ensure sending the final upload progress message here, since
   // OnResponseCompleted can be called without OnResponseStarted on cancellation
@@ -1315,26 +1345,6 @@ void URLLoader::OnSSLCertificateErrorResponse(const net::SSLInfo& ssl_info,
   url_request_->CancelWithSSLError(net_error, ssl_info);
 }
 
-void URLLoader::OnCertificateRequestedResponse(
-    const scoped_refptr<net::X509Certificate>& x509_certificate,
-    const std::string& provider_name,
-    const std::vector<uint16_t>& algorithm_preferences,
-    mojom::SSLPrivateKeyPtr ssl_private_key,
-    bool cancel_certificate_selection) {
-  if (cancel_certificate_selection) {
-    url_request_->CancelWithError(net::ERR_SSL_CLIENT_AUTH_CERT_NEEDED);
-  } else {
-    if (x509_certificate) {
-      auto key = base::MakeRefCounted<SSLPrivateKeyInternal>(
-          provider_name, algorithm_preferences, std::move(ssl_private_key));
-      url_request_->ContinueWithCertificate(std::move(x509_certificate),
-                                            std::move(key));
-    } else {
-      url_request_->ContinueWithCertificate(nullptr, nullptr);
-    }
-  }
-}
-
 bool URLLoader::HasDataPipe() const {
   return pending_write_ || response_body_stream_.is_valid();
 }

services/network/url_loader.h

@@ -8,6 +8,7 @@
 #include <stdint.h>
 
 #include <memory>
+#include <string>
 #include <vector>
 
 #include "base/callback.h"
@@ -51,7 +52,8 @@ class ScopedThrottlingToken;
 class COMPONENT_EXPORT(NETWORK_SERVICE) URLLoader
     : public mojom::URLLoader,
       public net::URLRequest::Delegate,
-      public mojom::AuthChallengeResponder {
+      public mojom::AuthChallengeResponder,
+      public mojom::ClientCertificateResponder {
  public:
   using DeleteCallback = base::OnceCallback<void(mojom::URLLoader* loader)>;
 
@@ -113,6 +115,15 @@ class COMPONENT_EXPORT(NETWORK_SERVICE) URLLoader
   void OnAuthCredentials(
       const base::Optional<net::AuthCredentials>& credentials) override;
 
+  // mojom::ClientCertificateResponder:
+  void ContinueWithCertificate(
+      const scoped_refptr<net::X509Certificate>& x509_certificate,
+      const std::string& provider_name,
+      const std::vector<uint16_t>& algorithm_preferences,
+      mojom::SSLPrivateKeyPtr ssl_private_key) override;
+  void ContinueWithoutCertificate() override;
+  void CancelRequest() override;
+
   net::LoadState GetLoadStateForTesting() const;
 
   uint32_t GetRenderFrameId() const;
@@ -190,12 +201,6 @@ class COMPONENT_EXPORT(NETWORK_SERVICE) URLLoader
   void OnUploadProgressACK();
   void OnSSLCertificateErrorResponse(const net::SSLInfo& ssl_info,
                                      int net_error);
-  void OnCertificateRequestedResponse(
-      const scoped_refptr<net::X509Certificate>& x509_certificate,
-      const std::string& provider_name,
-      const std::vector<uint16_t>& algorithm_preferences,
-      mojom::SSLPrivateKeyPtr ssl_private_key,
-      bool cancel_certificate_selection);
   bool HasDataPipe() const;
   void RecordBodyReadFromNetBeforePausedIfNeeded();
   void ResumeStart();
@@ -247,6 +252,8 @@ class COMPONENT_EXPORT(NETWORK_SERVICE) URLLoader
   mojo::Binding<mojom::URLLoader> binding_;
   mojo::Binding<mojom::AuthChallengeResponder>
       auth_challenge_responder_binding_;
+  mojo::Binding<mojom::ClientCertificateResponder>
+      client_cert_responder_binding_;
   mojom::URLLoaderClientPtr url_loader_client_;
   int64_t total_written_bytes_ = 0;
 

services/network/url_loader_unittest.cc

@@ -33,6 +33,7 @@
 #include "mojo/public/cpp/bindings/strong_binding.h"
 #include "mojo/public/cpp/system/data_pipe_utils.h"
 #include "mojo/public/cpp/system/wait.h"
+#include "net/base/escape.h"
 #include "net/base/io_buffer.h"
 #include "net/base/load_flags.h"
 #include "net/base/mime_sniffer.h"
@@ -2131,6 +2132,7 @@ class MockNetworkServiceClient : public TestNetworkServiceClient {
     NULL_CERTIFICATE,
     VALID_CERTIFICATE_SIGNATURE,
     INVALID_CERTIFICATE_SIGNATURE,
+    DESTROY_CLIENT_CERT_RESPONDER,
   };
 
   // mojom::NetworkServiceClient:
@@ -2170,8 +2172,7 @@ class MockNetworkServiceClient : public TestNetworkServiceClient {
       uint32_t routing_id,
       uint32_t request_id,
       const scoped_refptr<net::SSLCertRequestInfo>& cert_info,
-      mojom::NetworkServiceClient::OnCertificateRequestedCallback callback)
-      override {
+      mojom::ClientCertificateResponderPtr client_cert_responder) override {
     switch (certificate_response_) {
       case CertificateResponse::INVALID:
         NOTREACHED();
@@ -2181,21 +2182,19 @@ class MockNetworkServiceClient : public TestNetworkServiceClient {
         url_loader_ptr_->reset();
         break;
       case CertificateResponse::CANCEL_CERTIFICATE_SELECTION:
-        std::move(callback).Run(nullptr, std::string(), std::vector<uint16_t>(),
-                                nullptr,
-                                true /* cancel_certificate_selection */);
+        client_cert_responder->CancelRequest();
         break;
       case CertificateResponse::NULL_CERTIFICATE:
-        std::move(callback).Run(nullptr, std::string(), std::vector<uint16_t>(),
-                                nullptr,
-                                false /* cancel_certificate_selection */);
+        client_cert_responder->ContinueWithoutCertificate();
         break;
       case CertificateResponse::VALID_CERTIFICATE_SIGNATURE:
       case CertificateResponse::INVALID_CERTIFICATE_SIGNATURE:
-        std::move(callback).Run(std::move(certificate_), provider_name_,
-                                algorithm_preferences_,
-                                std::move(ssl_private_key_ptr_),
-                                false /* cancel_certificate_selection */);
+        client_cert_responder->ContinueWithCertificate(
+            std::move(certificate_), provider_name_, algorithm_preferences_,
+            std::move(ssl_private_key_ptr_));
+        break;
+      case CertificateResponse::DESTROY_CLIENT_CERT_RESPONDER:
+        // Send no response and let the local variable be destroyed.
         break;
     }
     ++on_certificate_requested_counter_;
@@ -2641,6 +2640,119 @@ class TestSSLPrivateKey : public net::SSLPrivateKey {
 };
 
 #if !defined(OS_IOS)
+TEST_F(URLLoaderTest, ClientAuthRespondTwice) {
+  // This tests that one URLLoader can handle two client cert requests.
+
+  net::SSLServerConfig ssl_config;
+  ssl_config.client_cert_type =
+      net::SSLServerConfig::ClientCertType::REQUIRE_CLIENT_CERT;
+
+  net::EmbeddedTestServer test_server_1(net::EmbeddedTestServer::TYPE_HTTPS);
+  test_server_1.SetSSLConfig(net::EmbeddedTestServer::CERT_OK, ssl_config);
+  test_server_1.AddDefaultHandlers(
+      base::FilePath(FILE_PATH_LITERAL("services/test/data")));
+  ASSERT_TRUE(test_server_1.Start());
+
+  net::EmbeddedTestServer test_server_2(net::EmbeddedTestServer::TYPE_HTTPS);
+  test_server_2.SetSSLConfig(net::EmbeddedTestServer::CERT_OK, ssl_config);
+  test_server_2.AddDefaultHandlers(
+      base::FilePath(FILE_PATH_LITERAL("services/test/data")));
+  ASSERT_TRUE(test_server_2.Start());
+
+  std::unique_ptr<net::FakeClientCertIdentity> identity =
+      net::FakeClientCertIdentity::CreateFromCertAndKeyFiles(
+          net::GetTestCertsDirectory(), "client_1.pem", "client_1.pk8");
+  ASSERT_TRUE(identity);
+  scoped_refptr<TestSSLPrivateKey> private_key =
+      base::MakeRefCounted<TestSSLPrivateKey>(identity->ssl_private_key());
+
+  MockNetworkServiceClient network_service_client;
+  network_service_client.set_certificate_response(
+      MockNetworkServiceClient::CertificateResponse::
+          VALID_CERTIFICATE_SIGNATURE);
+  network_service_client.set_private_key(private_key);
+  network_service_client.set_certificate(identity->certificate());
+
+  // Create a request to server_1 that will redirect to server_2
+  ResourceRequest request = CreateResourceRequest(
+      "GET",
+      test_server_1.GetURL("/server-redirect-307?" +
+                           net::EscapeQueryParamValue(
+                               test_server_2.GetURL("/echo").spec(), true)));
+
+  base::RunLoop delete_run_loop;
+  mojom::URLLoaderPtr loader;
+  std::unique_ptr<URLLoader> url_loader;
+  mojom::URLLoaderFactoryParams params;
+  params.process_id = kProcessId;
+  params.is_corb_enabled = false;
+  url_loader = std::make_unique<URLLoader>(
+      context(), &network_service_client,
+      DeleteLoaderCallback(&delete_run_loop, &url_loader),
+      mojo::MakeRequest(&loader), mojom::kURLLoadOptionNone, request,
+      client()->CreateInterfacePtr(), TRAFFIC_ANNOTATION_FOR_TESTS, &params,
+      0 /* request_id */, resource_scheduler_client(), nullptr,
+      nullptr /* network_usage_accumulator */, nullptr /* header_client */);
+
+  EXPECT_EQ(0, network_service_client.on_certificate_requested_counter());
+  EXPECT_EQ(0, private_key->sign_count());
+
+  client()->RunUntilRedirectReceived();
+  loader->FollowRedirect({}, {}, base::nullopt);
+  // MockNetworkServiceClient gives away the private key when it invokes
+  // ContinueWithCertificate, so we have to give it the key again.
+  network_service_client.set_private_key(private_key);
+  client()->RunUntilComplete();
+  delete_run_loop.Run();
+
+  EXPECT_EQ(net::OK, client()->completion_status().error_code);
+  EXPECT_EQ(2, network_service_client.on_certificate_requested_counter());
+  EXPECT_EQ(2, private_key->sign_count());
+}
+
+TEST_F(URLLoaderTest, ClientAuthDestroyResponder) {
+  // When URLLoader receives no message from the ClientCertificateResponder and
+  // its connection errors out, we expect the request to be canceled rather than
+  // just hang.
+  net::EmbeddedTestServer test_server(net::EmbeddedTestServer::TYPE_HTTPS);
+  net::SSLServerConfig ssl_config;
+  ssl_config.client_cert_type =
+      net::SSLServerConfig::ClientCertType::REQUIRE_CLIENT_CERT;
+  test_server.SetSSLConfig(net::EmbeddedTestServer::CERT_OK, ssl_config);
+  test_server.AddDefaultHandlers(
+      base::FilePath(FILE_PATH_LITERAL("services/test/data")));
+  ASSERT_TRUE(test_server.Start());
+
+  MockNetworkServiceClient network_service_client;
+  network_service_client.set_certificate_response(
+      MockNetworkServiceClient::CertificateResponse::
+          DESTROY_CLIENT_CERT_RESPONDER);
+
+  ResourceRequest request =
+      CreateResourceRequest("GET", test_server.GetURL("/defaultresponse"));
+  base::RunLoop delete_run_loop;
+  mojom::URLLoaderPtr loader;
+  mojom::URLLoaderFactoryParams params;
+  params.process_id = kProcessId;
+  params.is_corb_enabled = false;
+  std::unique_ptr<URLLoader> url_loader = std::make_unique<URLLoader>(
+      context(), &network_service_client,
+      DeleteLoaderCallback(&delete_run_loop, &url_loader),
+      mojo::MakeRequest(&loader), 0, request, client()->CreateInterfacePtr(),
+      TRAFFIC_ANNOTATION_FOR_TESTS, &params, 0 /* request_id */,
+      resource_scheduler_client(), nullptr,
+      nullptr /* network_usage_accumulator */, nullptr /* header_client */);
+  network_service_client.set_url_loader_ptr(&loader);
+
+  RunUntilIdle();
+  ASSERT_TRUE(url_loader);
+
+  client()->RunUntilComplete();
+
+  EXPECT_EQ(net::ERR_SSL_CLIENT_AUTH_CERT_NEEDED,
+            client()->completion_status().error_code);
+}
+
 TEST_F(URLLoaderTest, ClientAuthCancelConnection) {
   net::EmbeddedTestServer test_server(net::EmbeddedTestServer::TYPE_HTTPS);
   net::SSLServerConfig ssl_config;