Implement LoadTemporaryRoot for Windows

net/base/test_root_certs.cc

+// Copyright (c) 2010 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/test_root_certs.h"
+
+#include <string>
+
+#include "base/file_path.h"
+#include "base/file_util.h"
+#include "base/logging.h"
+#include "net/base/x509_certificate.h"
+
+namespace net {
+
+namespace {
+
+bool g_has_instance = false;
+
+CertificateList LoadCertificates(const FilePath& filename) {
+  using ::operator<<;
+  std::string raw_cert;
+  if (!file_util::ReadFileToString(filename, &raw_cert)) {
+    LOG(ERROR) << "Can't load certificate " << filename.value();
+    return CertificateList();
+  }
+
+  return X509Certificate::CreateCertificateListFromBytes(
+      raw_cert.data(), raw_cert.length(), X509Certificate::FORMAT_AUTO);
+}
+
+}  // namespace
+
+// static
+TestRootCerts* TestRootCerts::GetInstance() {
+  g_has_instance = true;
+  return Singleton<TestRootCerts>::get();
+}
+
+bool TestRootCerts::HasInstance() {
+  return g_has_instance;

[joth, 2010/11/22 13:05:40]

this might give some valgrind warnings as g_has_instance could be written to
repeatedly from multiple threads whilst others are reading it.
Setting g_has_instance from TestRootCerts c'tor should ix this (so it just
happens once) however that pushes it into each of the platform files.

Perhaps as well to make it an explicit getter/setter pair:

bool g_test_mode_enabled = false;

void EnableTestMode() {  // Called once e.g. in test Init()
  g_test_mode_enabled = true;
}

bool TestModeEnabled() {
  return g_test_mode_enabled;
}


TestRootCerts::GetInstance() {
  CHECK(TestModeEnabled()); 
  ...
}

[bulach, 2010/11/22 14:36:50]

I like this idea, just some minor suggestions on top of it:

1. check that EnableTestMode() is only defined for UNIT_TEST (this macro is
defined when depending on gtest, so I think it should be available for tests,
but not for production code):
http://codesearch.google.com/codesearch/p?hl=en#OAMlx_jo-ck/src/testing/gtest...

2. add a message on the CHECK for GetInstance():

#if defined(UNIT_TEST)
void EnableTestMode() {  // Called once e.g. in test Init()
  g_test_mode_enabled = true;
  GetInstance();
}
#endif  // UNIT_TEST

TestRootCerts::GetInstance() {
  CHECK(g_test_mode_enabled) << "Can't access TestRootCerts without enabling it
for tests";
  return Singleton<TestRootCerts>::get();
}

[Ryan Sleevi, 2010/12/03 03:28:06]

joth: I pushed the platform-specific implementation into an explicit Init()
method, while the platform-agnostic details, specifically g_has_instance, is now
handled in the ctor in test_root_certs.cc

I'm not sure I understand the desire to provide the explicit getter/setter pair,
perhaps you could elaborate?

[Ryan Sleevi, 2010/12/03 03:28:06]

bulach: Could you explain the reasoning behind this, as I'm not sure I really
understand why the desire for the UNIT_TEST check. 

My understanding of why you were requesting HasInstance() was because the
constructors were doing non-trivial work (CertOpenStore in the case of Windows,
CFMutableArrayCreate() in the case of Mac).

The reason why I went with HasInstance() as such was because it may be able to
be optimized out by an aggressive enough optimizer (g_has_instance will always
be false in "normal" binaries, so all of the TestRootCerts code can be
eliminated and the branch in X509Certificate removed)

As far as thread-safety, the underlying TestRootCerts isn't thread-safe (and
hasn't been, historically). There's definitely a race between HasInstance() and
GetInstance(), even still, but there's also implicit races between GetInstance()
and the verify code, so I'm not sure the benefit of addressing one without the
other.

[joth, 2010/12/03 10:40:59]

My thinking was to put the "g_has_instance = true;" into it's own method
("TestRootCerts::EnableTestMode") which is called explicitly by the test code,
so it's impossible for the production code to accidentally enable test mode as a
side effect of an inappropriate call to GetInstance. (as per bulach's idea, if
TestRootCerts::EnableTestMode is only visible to the test code under a UNIT_TEST
guard, it gets really very impossible for production code to accidentally enable
it).

I'm also OK with it as you have it now though, as it will fix the main concern
around valgrind problems. (assuming the first GetInstance call does not race
with HasInstance, which it shouldn't as it will occur in test-setup phase
anyway)

[bulach, 2010/12/03 10:58:27]

sorry, I wasn't clear :) let me try to explain my thoughts.. 

the reason I suggested "HasInstance()" was to separate the test path from the
real production code, i.e., we'd only go to a different path in production code
"if (TestRootCerts::HasInstance())".


however, that doesn't entirely prevent production code from (accidentally)
instantiating TestRootCerts.

I liked joth's idea of "EnableTestMode()", and then to ensure this would only
ever be called by test code, I suggested guarding it with the UNIT_TEST check..

I guess my suggestions are about:
1. on production code mark it clear that we're following a different path for
tests.

2. make sure only test code can inject this. 

it would also clarify the HasInstance() / GetInstance() / EnableTest()
threading, i.e., HasInstance() can be called any time, GetInstance() only ever
after HasInstance(), and EnableTest() must be called before doing anything else.

I'm probably being overzealous though :) I'm happy with the current solution
(but would be happier would the EnableTestMode()).

+}
+
+bool TestRootCerts::AddFromFile(const FilePath& file) {
+  CertificateList root_certs = LoadCertificates(file);
+  if (root_certs.empty() || root_certs.size() > 1)
+    return false;
+
+  return Add(root_certs.front());
+}
+
+}  // namespace net