Disk Based Certificate Cache Implementation

net/http/disk_based_cert_cache_unittest.cc

Index: net/http/disk_based_cert_cache_unittest.cc
diff --git a/net/http/disk_based_cert_cache_unittest.cc b/net/http/disk_based_cert_cache_unittest.cc
new file mode 100644
index 0000000000000000000000000000000000000000..b1d93f7f1c6ba6b24660992b8a1965467244e764
--- /dev/null
+++ b/net/http/disk_based_cert_cache_unittest.cc
@@ -0,0 +1,270 @@
+// Copyright (c) 2014 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/http/disk_based_cert_cache.h"
+
+#include "base/bind.h"
+#include "base/callback_helpers.h"
+#include "net/base/completion_callback.h"
+#include "net/base/net_errors.h"
+#include "net/base/test_completion_callback.h"
+#include "net/base/test_data_directory.h"
+#include "net/disk_cache/memory/mem_backend_impl.h"
+#include "net/http/mock_http_cache.h"
+#include "net/test/cert_test_util.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace net {
+
+namespace {
+
+const char kCertTransaction1Key[] =
+    "cert:4C005EF1CF45F80D4A5A2BCFB00D4F198121E8D4";
+const char kCertTransaction2Key[] =
+    "cert:9174C7CB9E4919604E7B1BFC430E4929DA45F65F";
+
+// MockTransactions are required to use the MockDiskCache backend.
+// This transaction corresponds to "root_ca_cert.pem" in
+// GetTestCertsDirectory().
+const MockTransaction kCertTransaction1 = {
+    kCertTransaction1Key, "", base::Time(),     "",   LOAD_NORMAL, "", "",
+    base::Time(),         "", TEST_MODE_NORMAL, NULL, 0,           OK};
+
+// This transaction corresponds to "ok_cert.pem" in GetTestCertsDirectory().
+const MockTransaction kCertTransaction2 = {
+    kCertTransaction2Key, "", base::Time(),     "",   LOAD_NORMAL, "", "",
+    base::Time(),         "", TEST_MODE_NORMAL, NULL, 0,           OK};
+
+// MockCertCache is used so that results from the DiskBasedCertCache can be
+// received using TestCompletionCallback::GetResult.
+class MockCertCache {
+ public:
+  MockCertCache()
+      : backend(new MockDiskCache()),
+        cert_cache_(new DiskBasedCertCache(backend.get())) {}
+
+  int Set(X509Certificate::OSCertHandle cert_handle,
+          std::string* key,
+          const CompletionCallback& callback) {
+    cert_cache_->Set(cert_handle,
+                     base::Bind(&MockCertCache::OnSetComplete,
+                                base::Unretained(this),
+                                key,
+                                callback));
+    return ERR_IO_PENDING;
+  }
+
+  int Get(const std::string& key,
+          X509Certificate::OSCertHandle* cert_handle,
+          const CompletionCallback& callback) {
+    cert_cache_->Get(key,
+                     base::Bind(&MockCertCache::OnGetComplete,
+                                base::Unretained(this),
+                                cert_handle,
+                                callback));
+    return ERR_IO_PENDING;
+  }
+
+  void OnSetComplete(std::string* key_return,
+                     CompletionCallback callback,
+                     const std::string& key_retrieved) {
+    *key_return = key_retrieved;
+    callback.Run(OK);
+  }
+
+  void OnGetComplete(X509Certificate::OSCertHandle* handle_return,
+                     CompletionCallback callback,
+                     const X509Certificate::OSCertHandle handle_retrieved) {
+    *handle_return = handle_retrieved;
+    callback.Run(OK);
+  }
+
+  void DeleteCertCache() { cert_cache_.reset(); }
+
+ private:
+  scoped_ptr<disk_cache::Backend> backend;
+  scoped_ptr<DiskBasedCertCache> cert_cache_;
+};
+
+}  // namespace
+
+// ----------------------------------------------------------------------------
+
+// Tests that a certificate can be stored in the cache.
+TEST(DiskBasedCertCache, SetCert) {
+  AddMockTransaction(&kCertTransaction1);

[Ryan Sleevi, 2014/06/24 22:01:00]

Notes from discussion/review: You want to use ScopedMockTransaction, as
otherwise this leaves global state around.

+  MockCertCache user;
+  scoped_refptr<X509Certificate> cert(
+      ImportCertFromFile(GetTestCertsDirectory(), "root_ca_cert.pem"));
+  ASSERT_TRUE(cert.get());
+
+  TestCompletionCallback set_callback;
+  std::string key;
+
+  int rv =
+      user.Set(cert.get()->os_cert_handle(), &key, set_callback.callback());
+  rv = set_callback.GetResult(rv);
+  EXPECT_EQ(OK, rv);
+  EXPECT_FALSE(key.empty());

[Ryan Sleevi, 2014/06/24 22:00:59]

Notes from discussion/review:

You should also verify:
1) That the key was the expected key (kCertTransaction1Key)
2) That the contents for key in backend_ (user.backend_) actually match the
expected certificate contents

+}
+
+// Tests that attempting to retrieve a cert that is not in the cache will
+// return NULL.
+TEST(DiskBasedCertCache, GetUncachedCert) {
+  AddMockTransaction(&kCertTransaction1);
+  MockCertCache user;
+  TestCompletionCallback get_callback;
+  X509Certificate::OSCertHandle cert_handle = NULL;
+
+  int rv =
+      user.Get(kCertTransaction1Key, &cert_handle, get_callback.callback());
+  rv = get_callback.GetResult(rv);
+  EXPECT_EQ(OK, rv);
+  EXPECT_EQ(NULL, cert_handle);

[Ryan Sleevi, 2014/06/24 22:01:00]

Notes from discussion/review:

BUG: cert_handle has been invalidated (the cache's callback ran when your
get_callback completed), and so it's not safe to operate on.

+}
+
+// Issues two requests to store a certificate in the cache
+// (simultaneously), and checks that the DiskBasedCertCache stores the
+// certificate to the cache (in one write rather than two).
+TEST(DiskBasedCertCache, SetMultiple) {
+  AddMockTransaction(&kCertTransaction1);
+  MockCertCache user;
+
+  scoped_refptr<X509Certificate> cert(
+      ImportCertFromFile(GetTestCertsDirectory(), "root_ca_cert.pem"));
+  ASSERT_TRUE(cert.get());
+
+  TestCompletionCallback set_callback1;
+  TestCompletionCallback set_callback2;
+
+  std::string key1, key2;
+
+  int rv1 =
+      user.Set(cert.get()->os_cert_handle(), &key1, set_callback1.callback());

[Ryan Sleevi, 2014/06/24 22:01:00]

Notes from discussion/review:

No need to use "cert.get()->" - just use "cert->" for all places you find the
pattern.

+  int rv2 =
+      user.Set(cert.get()->os_cert_handle(), &key2, set_callback2.callback());

[Ryan Sleevi, 2014/06/24 22:01:00]

Notes from discussion/review:

This isn't matching the description on 127-129

The first value (rv1) may have completed synchronously before you invoke the
second set (rv2), so you may have caused two writes, rather than 1

You want to ensure that your test harness ensures that user.Set(key1) won't have
completed before you user.Set(key2)

+
+  rv1 = set_callback1.GetResult(rv1);
+  EXPECT_EQ(OK, rv1);
+  rv2 = set_callback2.GetResult(rv2);
+  EXPECT_EQ(OK, rv2);
+  EXPECT_EQ(key1, key2);
+}
+
+// Stores a certificate in the DiskBasedCertCache, then retrieves it
+// and makes sure it was retrieved successfully.
+TEST(DiskBasedCertCache, SimpleSetAndGet) {
+  AddMockTransaction(&kCertTransaction1);
+  MockCertCache user;
+
+  scoped_refptr<X509Certificate> cert(
+      ImportCertFromFile(GetTestCertsDirectory(), "root_ca_cert.pem"));
+  ASSERT_TRUE(cert.get());
+
+  TestCompletionCallback set_callback;
+  TestCompletionCallback get_callback;
+  std::string key;
+
+  X509Certificate::OSCertHandle retrieved_cert_handle = NULL;
+  int rv =
+      user.Set(cert.get()->os_cert_handle(), &key, set_callback.callback());
+  rv = set_callback.GetResult(rv);
+  EXPECT_EQ(OK, rv);
+  rv = user.Get(key, &retrieved_cert_handle, get_callback.callback());
+  rv = get_callback.GetResult(rv);
+  EXPECT_EQ(OK, rv);
+  EXPECT_TRUE(X509Certificate::IsSameOSCert(retrieved_cert_handle,
+                                            cert.get()->os_cert_handle()));

[Ryan Sleevi, 2014/06/24 22:01:00]

Notes from discussion/review:

BUG: As noted, |retrieved_cert_handle| is invalidated by the time you make this
check. The reason it doesn't crash is that it happens to be returning the same
os_cert_handle as |cert|, and thus because |cert| is kept alive (161), it
doesn't crash.

However, this isn't part of the API contract of the Dup/Free cycle, so you need
to actually be sure to dup/free using your own helper (as discussed)

+}
+

[Ryan Sleevi, 2014/06/24 22:00:59]

TEST: You want to have a test where a Get() from a "populated" cache succeeds.

The best way to test this is to setup your MockCertCache, then manually use
OpenEntry/WriteData/CloseEntry (as part of your test), and then ensure that
Get() retrieves that value.

[Ryan Sleevi, 2014/06/24 22:01:00]

TEST: You want to have a test where a Get() from a "corrupted" cache fails.

This is similar to the success case, except you'd just mess up the data, so that
X509Certificate::CreateOSCertHandleFromData will fail.

+// Tests some basic functionality of the DiskBasedCertCache, with multiple
+// set and get operations.
+TEST(DiskBasedCertCache, BasicUsage) {
+  AddMockTransaction(&kCertTransaction1);
+  AddMockTransaction(&kCertTransaction2);
+  MockCertCache user;
+
+  scoped_refptr<X509Certificate> cert1(
+      ImportCertFromFile(GetTestCertsDirectory(), "root_ca_cert.pem"));
+  scoped_refptr<X509Certificate> cert2(
+      ImportCertFromFile(GetTestCertsDirectory(), "ok_cert.pem"));
+  ASSERT_TRUE(cert1.get());
+  ASSERT_TRUE(cert2.get());
+  ASSERT_FALSE(X509Certificate::IsSameOSCert(cert1->os_cert_handle(),
+                                             cert2->os_cert_handle()));
+  TestCompletionCallback set_callback1, set_callback2;
+  std::string key1, key2;
+
+  int rv1 =
+      user.Set(cert1.get()->os_cert_handle(), &key1, set_callback1.callback());
+  int rv2 =
+      user.Set(cert2.get()->os_cert_handle(), &key2, set_callback2.callback());
+
+  rv1 = set_callback1.GetResult(rv1);
+  EXPECT_EQ(OK, rv1);
+  rv2 = set_callback2.GetResult(rv2);
+  EXPECT_EQ(OK, rv2);
+
+  TestCompletionCallback get_callback1, get_callback2;
+  X509Certificate::OSCertHandle cert_handle1, cert_handle2;
+
+  rv1 = user.Get(key1, &cert_handle1, get_callback1.callback());
+  rv2 = user.Get(key2, &cert_handle2, get_callback2.callback());
+
+  rv1 = get_callback1.GetResult(rv1);
+  EXPECT_EQ(OK, rv1);
+  rv2 = get_callback2.GetResult(rv2);
+  EXPECT_EQ(OK, rv2);
+
+  EXPECT_TRUE(
+      X509Certificate::IsSameOSCert(cert1->os_cert_handle(), cert_handle1));
+  EXPECT_TRUE(
+      X509Certificate::IsSameOSCert(cert2->os_cert_handle(), cert_handle2));
+}
+
+// Test the result of simultaneous requests to store and retrieve a
+// certificate from the cache. Since it is unknown whether the set or get
+// operation will complete first, this test is just to make sure that it
+// does not crash.

[Ryan Sleevi, 2014/06/24 22:01:00]

Notes from discussion:

You'll want your test harness to deterministically handle whether the Set or Get
operation completes first.

I believe you may be able to use MockDiskEntry::IgnoreCallbacks(true) to cause
the disk cache to suspend all the callbacks, call Set then Get, and then run
MockDiskEntry::IgnoreCallbacks(false), which will order the callbacks such that
Set happens before Get.

Better yet, however, is
MockDiskEntry::IgnoreCallbacks(true);
base::ScopedClosureRunner runner(base::Bind(&MockDiskEntry::IgnoreCallbacks,
false));

You can also set your MockTransaction to use

TEST_MODE_SYNC_CACHE_START / _READ / _WRITE to order determinism (eg: if you do
SYNC_CACHE_START, the Set() will immediately succeed with
OpenEntry()/CreateEntry(), without even waiting for the Callback.

+TEST(DiskBasedCertCache, SimultaneousSetGet) {
+  AddMockTransaction(&kCertTransaction1);
+  MockCertCache user;
+  TestCompletionCallback set_callback, get_callback;
+  X509Certificate::OSCertHandle cert_handle;
+  std::string key("cert:4C005EF1CF45F80D4A5A2BCFB00D4F198121E8D4");

[Ryan Sleevi, 2014/06/24 22:00:59]

Reuse transaction key?

+
+  scoped_refptr<X509Certificate> cert(
+      ImportCertFromFile(GetTestCertsDirectory(), "root_ca_cert.pem"));
+  ASSERT_TRUE(cert.get());
+
+  int rv1 =
+      user.Set(cert.get()->os_cert_handle(), &key, set_callback.callback());
+  int rv2 = user.Get(key, &cert_handle, get_callback.callback());
+  rv1 = get_callback.GetResult(rv1);
+  EXPECT_EQ(OK, rv1);
+  rv2 = set_callback.GetResult(rv2);
+  EXPECT_EQ(OK, rv2);
+}
+

[Ryan Sleevi, 2014/06/24 22:01:00]

TEST: Use the MockTransactions to alter the sync/async behaviour to ensure
things work out well.

For example, does your test cache handle synchronous completion for START / READ
/ WRITE?

+// Tests that the DiskBasedCertCache can be deleted without issues when
+// there are pending operations in the disk cache.
+TEST(DiskBasedCertCache, DeletedCertCache) {
+  AddMockTransaction(&kCertTransaction1);
+  MockCertCache user;
+  std::string key;
+  scoped_refptr<X509Certificate> cert(
+      ImportCertFromFile(GetTestCertsDirectory(), "root_ca_cert.pem"));
+  ASSERT_TRUE(cert.get());
+
+  TestCompletionCallback set_callback;
+  int rv =
+      user.Set(cert.get()->os_cert_handle(), &key, set_callback.callback());
+
+  user.DeleteCertCache();
+  rv = set_callback.GetResult(rv);
+  EXPECT_EQ(OK, rv);
+  EXPECT_EQ(std::string(), key);
+}
+
+}  // namespace net