Implement LoadTemporaryRoot for Windows

net/base/x509_certificate_mac.cc

Index: net/base/x509_certificate_mac.cc
diff --git a/net/base/x509_certificate_mac.cc b/net/base/x509_certificate_mac.cc
index a2a0eeaa676b1b43c254185c83c64f87563ed076..eb3eb06f00e9520c5ec2b64b661f193d4280186a 100644
--- a/net/base/x509_certificate_mac.cc
+++ b/net/base/x509_certificate_mac.cc
@@ -15,68 +15,13 @@
 #include "net/base/cert_status_flags.h"
 #include "net/base/cert_verify_result.h"
 #include "net/base/net_errors.h"
+#include "net/base/test_root_certs.h"
 
 using base::mac::ScopedCFTypeRef;
 using base::Time;
 
 namespace net {
 
-class MacTrustedCertificates {
- public:
-  // Sets the trusted root certificate used by tests. Call with |cert| set
-  // to NULL to clear the test certificate.
-  void SetTestCertificate(X509Certificate* cert) {
-    AutoLock lock(lock_);
-    test_certificate_ = cert;
-  }
-
-  // Returns an array containing the trusted certificates for use with
-  // SecTrustSetAnchorCertificates(). Returns NULL if the system-supplied
-  // list of trust anchors is acceptable (that is, there is not test
-  // certificate available). Ownership follows the Create Rule (caller
-  // is responsible for calling CFRelease on the non-NULL result).
-  CFArrayRef CopyTrustedCertificateArray() {
-    AutoLock lock(lock_);
-
-    if (!test_certificate_)
-      return NULL;
-
-    // Failure to copy the anchor certificates or add the test certificate
-    // is non-fatal; SecTrustEvaluate() will use the system anchors instead.
-    CFArrayRef anchor_array;
-    OSStatus status = SecTrustCopyAnchorCertificates(&anchor_array);
-    if (status)
-      return NULL;
-    ScopedCFTypeRef<CFArrayRef> scoped_anchor_array(anchor_array);
-    CFMutableArrayRef merged_array = CFArrayCreateMutableCopy(
-        kCFAllocatorDefault, 0, anchor_array);
-    if (!merged_array)
-      return NULL;
-    CFArrayAppendValue(merged_array, test_certificate_->os_cert_handle());
-
-    return merged_array;
-  }
- private:
-  friend struct DefaultSingletonTraits<MacTrustedCertificates>;
-
-  // Obtain an instance of MacTrustedCertificates via the singleton
-  // interface.
-  MacTrustedCertificates() : test_certificate_(NULL) { }
-
-  // An X509Certificate object that may be appended to the list of
-  // system trusted anchors.
-  scoped_refptr<X509Certificate> test_certificate_;
-
-  // The trusted cache may be accessed from multiple threads.
-  mutable Lock lock_;
-
-  DISALLOW_COPY_AND_ASSIGN(MacTrustedCertificates);
-};
-
-void SetMacTestCertificate(X509Certificate* cert) {
-  Singleton<MacTrustedCertificates>::get()->SetTestCertificate(cert);
-}
-
 namespace {
 
 typedef OSStatus (*SecTrustCopyExtendedResultFuncPtr)(SecTrustRef,
@@ -542,16 +487,10 @@ int X509Certificate::Verify(const std::string& hostname, int flags,
     return NetErrorFromOSStatus(status);
   ScopedCFTypeRef<SecTrustRef> scoped_trust_ref(trust_ref);
 
-  // Set the trusted anchor certificates for the SecTrustRef by merging the
-  // system trust anchors and the test root certificate.
-  CFArrayRef anchor_array =
-      Singleton<MacTrustedCertificates>::get()->CopyTrustedCertificateArray();
-  ScopedCFTypeRef<CFArrayRef> scoped_anchor_array(anchor_array);
-  if (anchor_array) {
-    status = SecTrustSetAnchorCertificates(trust_ref, anchor_array);
-    if (status)
-      return NetErrorFromOSStatus(status);
-  }
+  TestRootCerts* root_certs = TestRootCerts::GetInstance();
+  status = root_certs->SetAnchorCertificates(trust_ref);

[bulach, 2010/11/17 17:17:30]

I know SetAnchorCertificates already checks, but I think at this point it'd be
clearer to comment that this is only for test code.

TestRootCerts* root_certs = TestRootCerts::GetInstance();
if (!root_certs->IsEmpty()) {
  // We're running tests, set the anchor certificates.
  status = root_certs->SetAnchorCertificates(trust_ref);
  if (status)
    return NetErrorFromOSStatus(status);
}

it may be even clearer to have this under a 
if (TestRootCerts::HasInstance()) {
  ...
}

and require that tests that needs this would initialize the singleton
beforehand. thoughts?

[wtc, 2010/11/18 02:12:49]

Nit: I find this harder to understand because this looks
like an operation on root_certs, but this actually acts
on trust_ref.  The original code shows that more clearly.

Two possible solutions:
1. Rename the method:
  status = root_certs->SetAnchorCertificatesOnSecTrust(trust_ref);
2. Go back to the style of the original code:
  CFArrayRef anchor_array = root_certs->CopyRootCertArray();
  status = SecTrustSetAnchorCertificates(trust_ref, anchor_array);

[Ryan Sleevi, 2010/11/18 05:31:58]

re: IsEmpty(), it seems unnecessary to put the check in two places. I've updated
the documentation of SetAnchorCertificates to better describe the behaviour,
especially with respect to IsEmpty(). Does that work for you?

re: HasInstance: How do you see something like that working? I don't see
anything that can check-without-create on Singleton<> or LazyInstance<>.

[Ryan Sleevi, 2010/11/18 05:31:58]

I went with 3, which renamed it to FixupSecTrustRef to match FixupChainContext
on Windows. Does that sufficiently disambiguate?

[bulach, 2010/11/18 12:42:12]

nit: I'd rename root_certs to test_root_certs to make it even more explicit this
is only used for tests.

this works for me, but just to clarify re:singleton:
since the tests would have to initialize the singleton on startup before the
actual test hits this, a I think a simple static bool on TestRootCerts
initialized to false and toggled on the singleton ctor would be sufficient..

your current code is clear to me now, but perhaps we'd like to avoid
instantiating the TestRootCerts object in production code and only initialize it
in test code?

+  if (status)
+    return NetErrorFromOSStatus(status);
 
   if (flags & VERIFY_REV_CHECKING_ENABLED) {
     // When called with VERIFY_REV_CHECKING_ENABLED, we ask SecTrustEvaluate()