Implement LoadTemporaryRoot for Windows

net/base/temporary_root_certs.h

Index: net/base/temporary_root_certs.h
diff --git a/net/base/temporary_root_certs.h b/net/base/temporary_root_certs.h
new file mode 100644
index 0000000000000000000000000000000000000000..f03fbcc1263586b05675a1e698876531086b02ae
--- /dev/null
+++ b/net/base/temporary_root_certs.h
@@ -0,0 +1,85 @@
+// 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.
+
+#ifndef NET_BASE_TEMPORARY_ROOT_CERTS_H_
+#define NET_BASE_TEMPORARY_ROOT_CERTS_H_
+#pragma once
+
+#include "base/singleton.h"

[wtc, 2010/11/16 23:24:01]

Nit: list "base/singleton.h" after all the other headers,
to conform to the Style Guide's recommendation as much as
we can.

Nit: include <windows.h> before <wincrypt.h>.

+#include "build/build_config.h"
+
+#if defined(OS_MACOSX)
+#include <CoreFoundation/CoreFoundation.h>
+#include "base/mac/scoped_cftyperef.h"
+#elif defined(OS_WIN)
+#include <wincrypt.h>
+#elif defined(USE_NSS)
+#include <map>
+#include <string>
+#include "net/base/x509_cert_types.h"
+#endif
+
+class FilePath;
+
+namespace net {
+
+class X509Certificate;
+
+class TemporaryRootCerts {

[bulach, 2010/11/09 16:21:09]

this is only ever going to be used for tests, right?
the previous version under "cert_test_util" made it slightly clearer.
perhaps we could name these files / classes as TestRootCerts,
TemporaryRootCertsForTests, or something along this line?

[wtc, 2010/11/16 23:24:01]

I agree with bulach's suggestion of naming this class
TestRootCerts to make it clear this class is used for tests
only.

Please add a concise block comment to describe what this class
does.

+ public:
+  // Obtain the Singleton instance to the trusted certificates.

[wtc, 2010/11/16 23:24:01]

When documenting methods in a header file, please use
descriptive ("Obtains") rather than imperative ("Obtain").

See the Style Guide:
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi...

+  static TemporaryRootCerts* GetInstance();
+
+  // Mark |certificate| as trusted for X509Certificate::Verify(). Returns
+  // false if the certificate could not be marked trusted.
+  bool Add(X509Certificate* certificate);
+
+  // Mark all the certificates in |file| as trusted. Returns false if the
+  // file could not be read or the certificates could not be marked trusted.
+  bool AddFromFile(const FilePath& file);

[wtc, 2010/11/16 23:24:01]

I commented on this issue before: it is better to only support
one certificate per file.  This comment also applies to
RemoveFromFile below, and the LoadCertificates method in
temporary_root_certs.cc.

+
+  // Clear the trusted status from |certificate|, reverting it to its original
+  // trust state prior to Add(). If |certificate| was already trusted, this
+  // does nothing.
+  void Remove(X509Certificate* certificate);

[wtc, 2010/11/16 23:24:01]

Perhaps all we need is a Clear/RemoveAll method that removes
all test root certs?

+
+  // Clear the trusted status from all the certificates in |file|.
+  void RemoveFromFile(const FilePath& file);
+
+#if defined(OS_MACOSX)
+  bool IsEmpty() const;

[wtc, 2010/11/16 23:24:01]

It is strange that IsEmpty() is defined for only some platforms.

+
+  CFArrayRef temporary_roots() const { return temporary_roots_; }
+#elif defined(OS_WIN)
+  bool IsEmpty() const { return cert_count_ == 0; }
+
+  HCERTSTORE temporary_roots() const { return temporary_roots_; }
+#endif
+
+ private:
+  friend struct DefaultSingletonTraits<TemporaryRootCerts>;
+
+  TemporaryRootCerts();
+  ~TemporaryRootCerts();
+
+#if defined(OS_MACOSX)
+  base::mac::ScopedCFTypeRef<CFMutableArrayRef> temporary_roots_;
+#elif defined(OS_WIN)
+  HCERTSTORE temporary_roots_;
+
+  // The number of certificates added to |temporary_roots_|.
+  size_t cert_count_;
+#elif defined(USE_NSS)
+  class TrustEntry;
+  typedef std::map<SHA1Fingerprint, TrustEntry,
+                   SHA1FingerprintLessThan> CertTrustMap;
+  CertTrustMap cert_trust_map_;

[wtc, 2010/11/16 23:24:01]

You should document what you store in cert_trust_map_ is
the original trust.  This was not obvious until I read
temporary_root_certs_nss.cc.

+#endif
+
+  DISALLOW_COPY_AND_ASSIGN(TemporaryRootCerts);
+};
+
+}  // namespace net
+
+#endif  // NET_BASE_TEMPORARY_ROOT_CERTS_H_