Added a PolicyCertVerifier that uses the trust anchors from the ONC policies.

chrome/browser/chromeos/policy/network_configuration_updater.cc

Index: chrome/browser/chromeos/policy/network_configuration_updater.cc
diff --git a/chrome/browser/chromeos/policy/network_configuration_updater.cc b/chrome/browser/chromeos/policy/network_configuration_updater.cc
index 1eb60290b273a2d042b127c5c2f59bb69ea46064..1b6125292998080238d2d869c2013417bd9cb94b 100644
--- a/chrome/browser/chromeos/policy/network_configuration_updater.cc
+++ b/chrome/browser/chromeos/policy/network_configuration_updater.cc
@@ -8,14 +8,48 @@
 
 #include "base/bind.h"
 #include "base/bind_helpers.h"
+#include "base/command_line.h"
+#include "base/logging.h"
+#include "base/memory/scoped_ptr.h"
 #include "chrome/browser/chromeos/cros/network_library.h"
 #include "chrome/browser/policy/policy_map.h"
+#include "chrome/common/chrome_switches.h"
 #include "chromeos/network/onc/onc_constants.h"
 #include "chromeos/network/onc/onc_utils.h"
+#include "content/public/browser/browser_thread.h"
+#include "net/base/cert_trust_anchor_provider.h"
+#include "net/base/x509_certificate.h"
 #include "policy/policy_constants.h"
 
+using content::BrowserThread;
+
 namespace policy {
 
+// A simple implementation of net::CertTrustAnchorProvider that returns a list
+// of certificates that can be set by the owner of this object.
+class NetworkConfigurationUpdater::CertTrustAnchorProviderImpl

[Ryan Sleevi, 2013/03/25 21:09:53]

*mostly* a nit, but I'm inclined to suggest you just make this an
implementation-local thing (eg: don't make it a private class of NCU)

namespace {

class CrosTrustAnchorProvider : public net::CertTrustAnchorProvider {

};

}

Then just use a cast the one place you need to - line 166.

Within the class, you'd just use a CertTrustAnchorProvider* pointer.

[Mostly, this is an aesthetic nit against "Impl" and in favour of better naming]

[Joao da Silva, 2013/03/31 19:22:14]

Done.

+    : public net::CertTrustAnchorProvider {
+ public:
+  CertTrustAnchorProviderImpl() {}
+  virtual ~CertTrustAnchorProviderImpl() {}
+
+  // CertTrustAnchorProvider:

[pneubeck (no reviews), 2013/03/26 10:01:25]

in the header the syntax
  Cert..Provider overrides.
is used

[Joao da Silva, 2013/03/31 19:22:14]

Done.

+  virtual const net::CertificateList& GetAdditionalTrustAnchors() OVERRIDE {
+    DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
+    return trust_anchors_;
+  }
+
+  void SetTrustAnchors(scoped_ptr<net::CertificateList> trust_anchors) {

[Ryan Sleevi, 2013/03/25 21:09:53]

Why are you passing a pointer? Just use a
"const net::CertificateList& trust_anchors"

What's the concern here?

[Joao da Silva, 2013/03/31 19:22:14]

The list is passed from the UI thread to the IO thread, so passing a pointer
avoids copying the vector.

It's a vector of scoped_refptrs so it's not that expensive, but it still
involves a lock/unlock per entry; I'd keep as is.

+    DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
+    trust_anchors_.swap(*trust_anchors);
+  }
+
+ private:
+  net::CertificateList trust_anchors_;
+
+  DISALLOW_COPY_AND_ASSIGN(CertTrustAnchorProviderImpl);
+};
+
 NetworkConfigurationUpdater::NetworkConfigurationUpdater(
     PolicyService* policy_service,
     chromeos::NetworkLibrary* network_library)
@@ -24,7 +58,8 @@ NetworkConfigurationUpdater::NetworkConfigurationUpdater(
       network_library_(network_library),
       user_policy_initialized_(false),
       allow_web_trust_(false),
-      policy_service_(policy_service) {
+      policy_service_(policy_service),
+      cert_trust_provider_(NULL) {

[pneubeck (no reviews), 2013/03/26 10:01:25]

Just create it here.
Makes the code below a bit simpler.
Performance diff is insignificant. We will always create it with the first
ApplyNetw...Configurations() call.

[Joao da Silva, 2013/03/31 19:22:14]

Done.

   DCHECK(network_library_);
   policy_change_registrar_.Observe(
       key::kDeviceOpenNetworkConfiguration,
@@ -45,6 +80,12 @@ NetworkConfigurationUpdater::NetworkConfigurationUpdater(
 
 NetworkConfigurationUpdater::~NetworkConfigurationUpdater() {
   network_library_->RemoveNetworkProfileObserver(this);
+  if (cert_trust_provider_) {
+    bool posted = BrowserThread::DeleteSoon(
+        BrowserThread::IO, FROM_HERE, cert_trust_provider_);
+    if (!posted)

[pneubeck (no reviews), 2013/03/26 10:01:25]

is that an error/problematic case? if so should we log a message?

[Joao da Silva, 2013/03/31 19:22:14]

This occurs when shutdown happens very early, before the IO thread was started.
It's not a real issue, but the memory bots catch that (there's a test that
triggers an early shutdown). We had a similar issue with shutdown of the policy
providers' backends on the FILE thread.

+      delete cert_trust_provider_;
+  }
 }
 
 void NetworkConfigurationUpdater::OnProfileListChanged() {
@@ -58,6 +99,14 @@ void NetworkConfigurationUpdater::OnUserPolicyInitialized() {
   ApplyNetworkConfigurations();
 }
 
+net::CertTrustAnchorProvider*
+    NetworkConfigurationUpdater::GetCertTrustAnchorProvider() {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  if (!cert_trust_provider_)
+    cert_trust_provider_ = new CertTrustAnchorProviderImpl();

[Ryan Sleevi, 2013/03/25 21:09:53]

Why the lazy-load?

[Joao da Silva, 2013/03/31 19:22:14]

No good reason, removed.

+  return cert_trust_provider_;
+}
+
 void NetworkConfigurationUpdater::OnPolicyChanged(
     chromeos::onc::ONCSource onc_source,
     const base::Value* previous,
@@ -101,10 +150,23 @@ void NetworkConfigurationUpdater::ApplyNetworkConfiguration(
   if (new_network_config.empty())
     new_network_config = chromeos::onc::kEmptyUnencryptedConfiguration;
 
+  scoped_ptr<net::CertificateList> web_trust_certs(new net::CertificateList());
   if (!network_library_->LoadOncNetworks(new_network_config, "", onc_source,
-                                         allow_web_trust_)) {
+                                         web_trust_certs.get())) {
     LOG(ERROR) << "Errors occurred during the ONC policy application.";
   }
+
+  CommandLine* command_line = CommandLine::ForCurrentProcess();
+  if (onc_source == chromeos::onc::ONC_SOURCE_USER_POLICY &&
+      allow_web_trust_ &&
+      command_line->HasSwitch(switches::kEnableWebTrustCerts)) {
+    GetCertTrustAnchorProvider();  // Make sure it's there.
+    BrowserThread::PostTask(
+        BrowserThread::IO, FROM_HERE,
+        base::Bind(&CertTrustAnchorProviderImpl::SetTrustAnchors,
+                   base::Unretained(cert_trust_provider_),
+                   base::Passed(&web_trust_certs)));

[Ryan Sleevi, 2013/03/25 21:09:53]

In the larger context, this highlights an interesting part of your design:
You're currently using a cache key based on the additional trust anchors (which
requires constantly re-calculating hashes for fingerprints).

If you instead adopted a design where the entire cache was explicitly flushed
whenever a new CTAP was set on a CertVerifier, you'd avoid the per-verification
overhead involved - thus amortizing the cost.

If you go the wrapping of a CertVerifier, then you can easily know whenever the
CTAP has changed, and thus flush the delegate CertVerifier.

[Joao da Silva, 2013/03/31 19:22:14]

SGTM; I'll leave that for another CL.

That requires a public ClearCache() call at the MTCV. It's also possible that a
Verify() sends a job to the worker thread and then a ClearCache() comes before
the result is ready; the cache is already cleared when the result comes back,
and then it's inserted into the cache but was computed using the trust anchors.

I'd solve that by storing the creation time in the CertVerifierWorker, passing
it to HandleResult, and then only store it in the cache if that creation time is
after the most recent ClearCache.

Another concern is the API of MTCV, which can now be misused if the trust
anchors provided change but ClearCache() isn't invoked. I suggest also caching
the list of trust anchors: if that list is updated at the
trust_anchors_provider, then ClearCache() must be called to flush the cache and
fetch the updated list. The previous problem can also be solved by caching the
fingerprints and copying them into each job; at HandleResult the cache is
updated only if the fingerprints from the job still match the cached
fingerprints.

WDYT?

[Ryan Sleevi, 2013/04/02 19:16:06]

I'll have to think more about this one. We can address it in follow-up though.

+  }
 }
 
 }  // namespace policy