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

net/base/openssl_private_key_store_unittest.cc

Index: net/base/openssl_private_key_store_unittest.cc
diff --git a/net/base/openssl_private_key_store_unittest.cc b/net/base/openssl_private_key_store_unittest.cc
new file mode 100644
index 0000000000000000000000000000000000000000..863031383d814e81c61434b4104983be9114cb7d
--- /dev/null
+++ b/net/base/openssl_private_key_store_unittest.cc
@@ -0,0 +1,175 @@
+// 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.
+
+#include "net/base/openssl_private_key_store.h"
+
+#include "base/memory/ref_counted.h"
+#include "net/base/cert_test_util.h"
+#include "net/base/test_data_directory.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace net {
+
+namespace {
+
+typedef OpenSSLPrivateKeyStore::ScopedEVP_PKEY ScopedEVP_PKEY;
+
+// Return the internal reference count of a given EVP_PKEY.
+int EVP_PKEY_get_refcount(EVP_PKEY* pkey) {
+  return pkey->references;
+}
+
+}

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

}  // namespace

You can and should *minimally* tend this namespace to over the fixture (eg: to
line 40), but realistically, to the whole file.

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

Done.

+
+// A common test class to ensure that the store is flushed after
+// each test case.
+class OpenSSLPrivateKeyStoreTest : public ::testing::Test {
+ protected:
+  virtual void SetUp() OVERRIDE {
+    store_ = net::OpenSSLPrivateKeyStore::GetInstance();

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

you do not need net::

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

Done.

+  }
+
+  virtual void TearDown() OVERRIDE {

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

Do not place OVERRIDE on testing:: overrides (SetUp/TearDown). Keep virtual.

That said, you can and should just use the constructor and destructor here.
There's no need for this deferred initialization to SetUp/TearDown

OpenSSLPrivateKeyStoreTest()
  : store_(OpenSSLPrivateKeyStore::GetInstance()) {
}
virtual ~OpenSSLPrivateKeyStoreTest() {
  if (store_)
    store_->Flush();
}

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

Done.

[Ryan Sleevi, 2013/02/25 19:51:07]

I'm not sure why you marked this DONE - it's still an issue.

[digit1, 2013/02/26 11:03:13]

Can you clarify? I've removed SetUp/TearDown in Patch set 7 just like you asked.
See https://codereview.chromium.org/12220104/patch/29001/29012

+    if (store_)
+      store_->Flush();
+  }
+
+  OpenSSLPrivateKeyStore* store_;
+};
+
+// Check that GetInstance() returns non-null
+TEST_F(OpenSSLPrivateKeyStoreTest, GetInstance) {
+  ASSERT_TRUE(store_);
+}
+
+// Check that Flush() works correctly.
+TEST_F(OpenSSLPrivateKeyStoreTest, Flush) {
+  ASSERT_TRUE(store_);
+
+  scoped_refptr<X509Certificate> cert_1(
+      ImportCertFromFile(GetTestCertsDirectory(), "client_1.pem"));
+  ASSERT_TRUE(cert_1);
+
+  ScopedEVP_PKEY priv_key(EVP_PKEY_new());
+  ASSERT_TRUE(priv_key.get());
+
+  ASSERT_TRUE(store_->RecordClientCertPrivateKey(cert_1.get(),
+                                                 priv_key.get()));
+
+  store_->Flush();
+
+  // Retrieve the private key now. This shall fail.

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

s/now//
s/shall/should/

// Retrieve the private key. This should fail because the
// store was flushed.

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

Done.

+  ScopedEVP_PKEY pkey;
+  ASSERT_FALSE(store_->FetchClientCertPrivateKey(cert_1.get(), &pkey));
+  ASSERT_FALSE(pkey.get());
+}
+
+// Check that trying to retrieve the private key of an unknown certificate
+// simply fails by returning null.
+TEST_F(OpenSSLPrivateKeyStoreTest, FetchEmptyPrivateKey) {
+  ASSERT_TRUE(store_);
+
+  scoped_refptr<X509Certificate> cert_1(
+      ImportCertFromFile(GetTestCertsDirectory(), "client_1.pem"));
+  ASSERT_TRUE(cert_1);
+
+  // Retrieve the private key now. This shall fail.

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

Same sort of rewording concerns as above (and throughout the rest of this file).

It's more important to explain why/how it will fail, rather than "This shall
fail" (which is clear from the use of ASSERT_FALSE that "this shall fail")

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

Done.

+  ScopedEVP_PKEY pkey;
+  ASSERT_FALSE(store_->FetchClientCertPrivateKey(cert_1.get(), &pkey));
+  ASSERT_FALSE(pkey.get());
+}
+
+// Check that any private key recorded through RecordClientCertPrivateKey
+// can be retrieved with FetchClientCertPrivateKey.
+TEST_F(OpenSSLPrivateKeyStoreTest, RecordAndFetchPrivateKey) {
+  ASSERT_TRUE(store_);
+
+  // Any certificate / key pair will do, the store is not supposed to
+  // check that the private and certificate public keys match. This is
+  // by design since the private EVP_PKEY could be a wrapper around a
+  // JNI reference, with no way to access the real private key bits.
+  scoped_refptr<X509Certificate> cert_1(
+      ImportCertFromFile(GetTestCertsDirectory(), "client_1.pem"));
+  ASSERT_TRUE(cert_1);
+
+  ScopedEVP_PKEY priv_key(EVP_PKEY_new());
+  ASSERT_TRUE(priv_key.get());
+  ASSERT_EQ(1, EVP_PKEY_get_refcount(priv_key.get()));
+
+  // Add the key a first time, this shall succeed, and increment the
+  // reference count.
+  ASSERT_TRUE(store_->RecordClientCertPrivateKey(cert_1.get(),
+                                                 priv_key.get()));
+  ASSERT_EQ(2, EVP_PKEY_get_refcount(priv_key.get()));
+
+  // Two successive calls with the same certificate / private key shall
+  // also succeed, but the key's reference count won't be incremented.
+  ASSERT_TRUE(store_->RecordClientCertPrivateKey(cert_1.get(),
+                                                 priv_key.get()));
+  ASSERT_EQ(2, EVP_PKEY_get_refcount(priv_key.get()));
+
+  // Retrieve the private key now. This shall succeed and increment
+  // the private key's reference count.
+  {
+    ScopedEVP_PKEY pkey2;
+    ASSERT_TRUE(store_->FetchClientCertPrivateKey(cert_1.get(), &pkey2));
+    ASSERT_EQ(pkey2.get(), priv_key.get());
+    ASSERT_EQ(3, EVP_PKEY_get_refcount(priv_key.get()));
+    // Release pkey2 here.
+  }
+
+  // Flush the store explicitely now, and check that this decrements
+  // the private key's reference count.
+  store_->Flush();
+  ASSERT_EQ(1, EVP_PKEY_get_refcount(priv_key.get()));
+}
+
+// Same test, but with two certificates / private keys.
+TEST_F(OpenSSLPrivateKeyStoreTest, RecordAndFetchTwoPrivateKeys) {
+  scoped_refptr<X509Certificate> cert_1(
+      ImportCertFromFile(GetTestCertsDirectory(), "client_1.pem"));
+  ASSERT_TRUE(cert_1);
+
+  scoped_refptr<X509Certificate> cert_2(
+      ImportCertFromFile(GetTestCertsDirectory(), "client_2.pem"));
+  ASSERT_TRUE(cert_2);
+
+  ScopedEVP_PKEY priv_key1(EVP_PKEY_new());
+  ASSERT_TRUE(priv_key1.get());
+  ASSERT_EQ(1, EVP_PKEY_get_refcount(priv_key1.get()));
+
+  ScopedEVP_PKEY priv_key2(EVP_PKEY_new());
+  ASSERT_TRUE(priv_key2.get());
+  ASSERT_EQ(1, EVP_PKEY_get_refcount(priv_key2.get()));
+
+  ASSERT_NE(priv_key1.get(), priv_key2.get());
+
+  // Add the key a first time, this shall succeed, and increment the
+  // reference count.
+  EXPECT_TRUE(store_->RecordClientCertPrivateKey(cert_1.get(),
+                                                 priv_key1.get()));
+  EXPECT_TRUE(store_->RecordClientCertPrivateKey(cert_2.get(),
+                                                 priv_key2.get()));
+  EXPECT_EQ(2, EVP_PKEY_get_refcount(priv_key1.get()));
+  EXPECT_EQ(2, EVP_PKEY_get_refcount(priv_key2.get()));
+
+  // Retrieve the private key now. This shall succeed and increment
+  // the private key's reference count.
+  ScopedEVP_PKEY fetch_key1;
+  ASSERT_TRUE(store_->FetchClientCertPrivateKey(cert_1.get(),
+                                                &fetch_key1));
+  ScopedEVP_PKEY fetch_key2;
+  ASSERT_TRUE(store_->FetchClientCertPrivateKey(cert_2.get(),
+                                                &fetch_key2));
+  EXPECT_TRUE(fetch_key1.get());
+  EXPECT_TRUE(fetch_key2.get());
+
+  EXPECT_EQ(fetch_key1.get(), priv_key1.get());
+  EXPECT_EQ(fetch_key2.get(), priv_key2.get());
+
+  EXPECT_EQ(3, EVP_PKEY_get_refcount(priv_key1.get()));
+  EXPECT_EQ(3, EVP_PKEY_get_refcount(priv_key2.get()));
+}
+
+}  // namespace net