Wire up SSL client authentication for OpenSSL/Android through the net/ stack

net/socket/ssl_client_socket_openssl_unittest.cc

+// Copyright (c) 2012 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 "net/socket/ssl_client_socket.h"
+
+#include <errno.h>
+#include <string.h>
+
+#include <openssl/bn.h>
+#include <openssl/evp.h>
+#include <openssl/pem.h>
+#include <openssl/rsa.h>
+
+#include "base/file_util.h"
+#include "base/memory/ref_counted.h"
+#include "base/memory/scoped_handle.h"
+#include "base/values.h"
+#include "crypto/openssl_util.h"
+#include "net/base/address_list.h"
+#include "net/base/cert_test_util.h"
+#include "net/base/host_resolver.h"
+#include "net/base/io_buffer.h"
+#include "net/base/mock_cert_verifier.h"
+#include "net/base/net_errors.h"
+#include "net/base/net_log.h"
+#include "net/base/net_log_unittest.h"
+#include "net/base/openssl_private_key_store.h"
+#include "net/base/ssl_cert_request_info.h"
+#include "net/base/ssl_config_service.h"
+#include "net/base/test_completion_callback.h"
+#include "net/base/test_data_directory.h"
+#include "net/base/test_root_certs.h"
+#include "net/socket/client_socket_factory.h"
+#include "net/socket/client_socket_handle.h"
+#include "net/socket/socket_test_util.h"
+#include "net/socket/tcp_client_socket.h"
+#include "net/test/test_server.h"
+#include "testing/gtest/include/gtest/gtest.h"
+#include "testing/platform_test.h"
+
+namespace {
+
+typedef net::OpenSSLPrivateKeyStore::ScopedEVP_PKEY ScopedEVP_PKEY;

[Ryan Sleevi, 2013/02/14 07:54:01]

When you do the namespace move suggested below, please make sure to remove all
net:: from this file.

[digit1, 2013/02/25 14:26:22]

Done.

+
+typedef crypto::ScopedOpenSSL<RSA, RSA_free> ScopedRSA;
+typedef crypto::ScopedOpenSSL<BIGNUM, BN_free> ScopedBIGNUM;
+
+}  // namespace
+
+namespace net {

[Ryan Sleevi, 2013/02/14 07:54:01]

Move this to line 41 - that is

namespace net {
namespace {

As originally proposed. This will also eliminate the need to declare these
functions static.

[digit1, 2013/02/25 14:26:22]

Done.

+
+// The following is needed to construct paths to certificates passed as
+// |client_authorities| in server SSLOptions. Current implementation of
+// RemoteTestServer (used on Android) expects relative paths, as opposed to
+// LocalTestServer, which expects absolute paths (what to fix?).
+static base::FilePath CertDirectory() {
+#ifdef OS_ANDROID
+  return net::GetTestCertsDirectoryRelative();
+#else
+  return net::GetTestCertsDirectory();
+#endif
+}
+
+// Loads a PEM-encoded private key file into a scoped EVP_PKEY object.
+// |filepath| is the private key file path.
+// |*pkey| is reset to the new EVP_PKEY on success, untouched otherwise.
+// Returns true on success, false on failure.
+static bool LoadPrivateKeyOpenSSL(
+    const base::FilePath& filepath,
+    net::OpenSSLPrivateKeyStore::ScopedEVP_PKEY* pkey) {
+  ScopedStdioHandle file(file_util::OpenFile(filepath, "rb"));
+  if (!file.get()) {
+    LOG(ERROR) << "Could not open private key file: "
+               << filepath.value() << ": " << strerror(errno);
+    return false;
+  }
+  EVP_PKEY* result = PEM_read_PrivateKey(file.get(), NULL, NULL, NULL);
+  if (result == NULL) {
+    LOG(ERROR) << "Could not read private key file: "
+               << filepath.value();
+    return false;
+  }
+  pkey->reset(result);
+  return true;
+}
+
+// LogContainsSSLConnectEndEvent returns true if the given index in the given
+// log is an SSL connect end event. The NSS sockets will cork in an attempt to

[Ryan Sleevi, 2013/02/15 23:53:26]

You've still got this comment referring to NSS.

Does any of this even apply to OpenSSL? Did you just copy it verbatim?

[digit1, 2013/02/25 14:26:22]

This is no longer needed (see comment below), so has been removed.

+// merge the first application data record with the Finished message when false
+// starting. However, in order to avoid the server timing out the handshake,
+// they'll give up waiting for application data and send the Finished after a
+// timeout. This means that an SSL connect end event may appear as a socket
+// write.
+static bool LogContainsSSLConnectEndEvent(
+    const net::CapturingNetLog::CapturedEntryList& log, int i) {
+  return net::LogContainsEndEvent(log, i, net::NetLog::TYPE_SSL_CONNECT) ||
+         net::LogContainsEvent(log, i, net::NetLog::TYPE_SOCKET_BYTES_SENT,
+                                net::NetLog::PHASE_NONE);
+};
+
+static const net::SSLConfig kDefaultSSLConfig;
+
+class SSLClientSocketOpenSSLClientAuthTest : public PlatformTest {
+ public:
+  SSLClientSocketOpenSSLClientAuthTest()
+      : socket_factory_(net::ClientSocketFactory::GetDefaultFactory()),
+        cert_verifier_(new net::MockCertVerifier) {
+    cert_verifier_->set_default_result(net::OK);
+    context_.cert_verifier = cert_verifier_.get();
+    key_store_ = net::OpenSSLPrivateKeyStore::GetInstance();
+  }
+
+ protected:
+  virtual void TearDown() {
+    key_store_->Flush();
+  }

[Ryan Sleevi, 2013/02/15 23:53:26]

See previous remarks re: SetUp and TearDown using instead constructors and
destructors.

[digit1, 2013/02/25 14:26:22]

I've changed it to use SetUp/TearDown exclusively.

+
+  net::SSLClientSocket* CreateSSLClientSocket(
+      net::StreamSocket* transport_socket,
+      const net::HostPortPair& host_and_port,
+      const net::SSLConfig& ssl_config) {
+    return socket_factory_->CreateSSLClientSocket(transport_socket,
+                                                  host_and_port,
+                                                  ssl_config,
+                                                  context_);
+  }
+
+  // Connect to a HTTPS test server.
+  bool ConnectToTestServer(net::TestServer::SSLOptions& ssl_options) {
+    test_server_.reset(new net::TestServer(net::TestServer::TYPE_HTTPS,
+                                           ssl_options,
+                                           base::FilePath()));
+    if (!test_server_.get()) {
+      LOG(ERROR) << "Could not create new TestServer";
+      return false;
+    }

[Ryan Sleevi, 2013/02/15 23:53:26]

This is an unnecessary check, at least following Chromium patterns. This should
only happen if OOM - and we intentionally don't check those conditions.

[digit1, 2013/02/25 14:26:22]

Done.

+    if (!test_server_->Start()) {
+      LOG(ERROR) << "Could not start TestServer";
+      return false;
+    }
+
+    if (!test_server_->GetAddressList(&addr_)) {
+      LOG(ERROR) << "Could not get TestServer address list";
+      return false;
+    }
+
+    transport_.reset(new net::TCPClientSocket(
+        addr_, &log_, net::NetLog::Source()));
+    int rv = transport_->Connect(callback_.callback());

[Ryan Sleevi, 2013/02/14 08:51:15]

int rv = callback.GetResult(transport_->Connect(callback_.callback());

This allows you to remove lines 151 & 152.

Same below ~lines 186/187 & 177.

[digit1, 2013/02/25 14:26:22]

Good to know, thanks.

+    if (rv == net::ERR_IO_PENDING)
+      rv = callback_.WaitForResult();
+    if (rv != net::OK) {
+      LOG(ERROR) << "Could not connect to TestServer";
+      return false;
+    }
+    return true;
+  }
+
+  bool RecordPrivateKey(net::SSLConfig& ssl_config,
+                        EVP_PKEY* private_key) {
+    return key_store_->RecordClientCertPrivateKey(
+        ssl_config.client_cert.get(), private_key);
+  }
+
+  bool CreateAndConnectSSLClientSocket(net::SSLConfig& ssl_config,

[Ryan Sleevi, 2013/02/15 23:53:26]

comment this function.

[digit1, 2013/02/25 14:26:22]

Done.

+                                       int* result) {
+    sock_.reset(CreateSSLClientSocket(transport_.release(),
+                                      test_server_->host_port_pair(),
+                                      ssl_config));
+
+    if (sock_->IsConnected()) {
+      LOG(ERROR) << "SSL Socket prematurely connected";
+      return false;
+    }
+
+    int rv = sock_->Connect(callback_.callback());
+
+    net::CapturingNetLog::CapturedEntryList entries;
+    log_.GetEntries(&entries);
+    if (!net::LogContainsBeginEvent(
+            entries, 5, net::NetLog::TYPE_SSL_CONNECT)) {
+      LOG(ERROR) << "SSL connection not started in logs";
+      return false;
+    }
+    if (rv == net::ERR_IO_PENDING)
+      rv = callback_.WaitForResult();
+
+    *result = rv;
+    return true;
+  }
+
+
+  bool CheckSSLClientSocketSentCert() {
+    net::CapturingNetLog::CapturedEntryList entries;
+    log_.GetEntries(&entries);
+    if (!LogContainsSSLConnectEndEvent(entries, -1)) {

[Ryan Sleevi, 2013/02/15 23:53:26]

Are you sure this needs to be checked?

[digit1, 2013/02/25 14:26:22]

Not really, this came from ssl_client_socket_unittest.cc, I've removed it.

+      LOG(ERROR) << "!LogContainsSSLConnectEndEvent()";
+      return false;
+    }
+
+    // Check that the client certificate was sent.
+    net::SSLInfo ssl_info;
+    sock_->GetSSLInfo(&ssl_info);
+    return ssl_info.client_cert_sent;
+  }
+
+  net::ClientSocketFactory* socket_factory_;
+  scoped_ptr<net::MockCertVerifier> cert_verifier_;
+  net::SSLClientSocketContext context_;
+  net::OpenSSLPrivateKeyStore* key_store_;
+  scoped_ptr<net::TestServer> test_server_;
+  net::AddressList addr_;
+  net::TestCompletionCallback callback_;
+  net::CapturingNetLog log_;
+  scoped_ptr<net::StreamSocket> transport_;
+  scoped_ptr<net::SSLClientSocket> sock_;
+};
+
+// Connect to a server requesting client authentication, do not send
+// any client certificates. It should refuse the connection.
+TEST_F(SSLClientSocketOpenSSLClientAuthTest, NoCert) {
+  net::TestServer::SSLOptions ssl_options;
+  ssl_options.request_client_certificate = true;
+
+  ASSERT_TRUE(ConnectToTestServer(ssl_options));
+
+  base::FilePath certs_dir = net::GetTestCertsDirectory();
+  net::SSLConfig ssl_config = kDefaultSSLConfig;
+
+  int rv;
+  ASSERT_TRUE(CreateAndConnectSSLClientSocket(ssl_config, &rv));
+
+  EXPECT_EQ(net::ERR_SSL_CLIENT_AUTH_CERT_NEEDED, rv);
+  EXPECT_FALSE(sock_->IsConnected());
+}
+
+// Connect to a server requesting client authentication, and send it
+// an empty certificate. It should refuse the connection.
+TEST_F(SSLClientSocketOpenSSLClientAuthTest, SendEmptyCert) {
+  net::TestServer::SSLOptions ssl_options;
+  ssl_options.request_client_certificate = true;
+
+  ASSERT_TRUE(ConnectToTestServer(ssl_options));
+
+  base::FilePath certs_dir = net::GetTestCertsDirectory();
+  net::SSLConfig ssl_config = kDefaultSSLConfig;
+  ssl_config.send_client_cert = true;
+  ssl_config.client_cert = NULL;
+
+  int rv;
+  ASSERT_TRUE(CreateAndConnectSSLClientSocket(ssl_config, &rv));
+
+  EXPECT_EQ(net::OK, rv);
+  EXPECT_TRUE(sock_->IsConnected());
+}
+
+// Connect to a server requesting client authentication. Send it a
+// matching certificate. It should allow the connection.
+TEST_F(SSLClientSocketOpenSSLClientAuthTest, SendGoodCert) {
+  net::TestServer::SSLOptions ssl_options;
+  ssl_options.request_client_certificate = true;
+  ssl_options.client_authorities.push_back(
+      CertDirectory().AppendASCII("client_1_root.pem"));
+
+  ASSERT_TRUE(ConnectToTestServer(ssl_options));
+
+  base::FilePath certs_dir = net::GetTestCertsDirectory();
+  net::SSLConfig ssl_config = kDefaultSSLConfig;
+  ssl_config.send_client_cert = true;
+  ssl_config.client_cert = net::ImportCertFromFile(certs_dir,
+                                                   "client_1.pem");
+
+  // This is required to ensure that signing works with the client
+  // certificate's private key.
+  net::OpenSSLPrivateKeyStore::ScopedEVP_PKEY client_private_key;
+  ASSERT_TRUE(LoadPrivateKeyOpenSSL(certs_dir.AppendASCII("client_1.key"),
+                                    &client_private_key));
+  EXPECT_TRUE(RecordPrivateKey(ssl_config, client_private_key.get()));
+
+  int rv;
+  ASSERT_TRUE(CreateAndConnectSSLClientSocket(ssl_config, &rv));
+
+  EXPECT_EQ(net::OK, rv);
+  EXPECT_TRUE(sock_->IsConnected());
+
+  EXPECT_TRUE(CheckSSLClientSocketSentCert());
+
+  sock_->Disconnect();
+  EXPECT_FALSE(sock_->IsConnected());
+}
+
+// Connect to a server requesting client authentication. Send it a
+// non-matching certificate. It should not allow the connection.
+// NOTE: Disabled because our TestServer never verifies that the client
+//       certificate matches the required CA authorities. Thus is always
+//       accepts the connection.
+TEST_F(SSLClientSocketOpenSSLClientAuthTest, DISABLED_SendBadCert) {

[Ryan Sleevi, 2013/02/15 23:53:26]

Unless you have a bug # and plan to be working on this, there's no value in
adding a DISABLED_ test here.

[digit1, 2013/02/25 14:26:22]

Ok, I've removed this test.

+  net::TestServer::SSLOptions ssl_options;
+  ssl_options.request_client_certificate = true;
+  ssl_options.client_authorities.push_back(
+      CertDirectory().AppendASCII("client_1_root.pem"));
+
+  ASSERT_TRUE(ConnectToTestServer(ssl_options));
+
+  base::FilePath certs_dir = net::GetTestCertsDirectory();
+  net::SSLConfig ssl_config = kDefaultSSLConfig;
+  ssl_config.send_client_cert = true;
+  ssl_config.client_cert = net::ImportCertFromFile(certs_dir,
+                                                   "client_2.pem");
+
+  net::OpenSSLPrivateKeyStore::ScopedEVP_PKEY client_private_key;
+  ASSERT_TRUE(LoadPrivateKeyOpenSSL(certs_dir.AppendASCII("client_2.key"),
+                                    &client_private_key));
+  EXPECT_TRUE(RecordPrivateKey(ssl_config, client_private_key.get()));
+
+  int rv;
+  ASSERT_TRUE(CreateAndConnectSSLClientSocket(ssl_config, &rv));
+
+  EXPECT_EQ(net::ERR_BAD_SSL_CLIENT_AUTH_CERT, rv);
+  EXPECT_FALSE(sock_->IsConnected());
+
+  EXPECT_TRUE(CheckSSLClientSocketSentCert());
+}
+
+}  // namespace net