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;
+}
+
+}
+
+// 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();
+  }
+
+  virtual void TearDown() OVERRIDE {
+    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.
+  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.
+  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.
+  }

[Ryan Sleevi, 2013/02/13 23:25:55]

As mentioned on previous reviews, such scope blocks are unnecessary and
generally discouraged.

[digit1, 2013/02/14 06:23:50]

In this case it is intentionally used to release pkey2 and decrease the
reference count on the priv_key, which is later tested after the call to
Flush(). That's why I added a comment on line 119 to make this explicit.

The point is to check that store->Flush() actually releases the reference it
holds on the key. It is possible to keep pkey2 alive until the end of the
function and check that the final reference count is 2, instead of 1, but I
think this version of the code is less ambiguous / more expressive about its
intent. Do you still prefer that I remove the scope block?

[Ryan Sleevi, 2013/02/14 07:15:00]

I actually find that preferable, in that when reading this, I missed the
significance of line 119 as it relates to the test.

Alternatively, rather than using the scope block, using .release() would also be
at least internally consistent with how such unit tests have historically been
used, as part of the general preference to minimize unnecessary indentation
(such as how error handling is structured).

Something like:

ASSERT_EQ(3, EVP_PKEY_get_refcount(priv_key.get()));
pkey2.release();
ASSERT_EQ(2, EVP_PKEY_get_refcount(priv_key.get()));
// Flush the store ...
store_->Flush();
ASSERT_EQ(1, EVP_PKEY_get_refcount(priv_key.get()));

Making clear the sequences.

+
+  // 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