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

chrome/browser/chromeos/policy/network_configuration_updater.cc

 // Copyright (c) 2012 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 "chrome/browser/chromeos/policy/network_configuration_updater.h"
 
 #include <string>
 
 #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)
     : policy_change_registrar_(
           policy_service, PolicyNamespace(POLICY_DOMAIN_CHROME, std::string())),
       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,
       base::Bind(&NetworkConfigurationUpdater::OnPolicyChanged,
                  base::Unretained(this),
                  chromeos::onc::ONC_SOURCE_DEVICE_POLICY));
   policy_change_registrar_.Observe(
       key::kOpenNetworkConfiguration,
       base::Bind(&NetworkConfigurationUpdater::OnPolicyChanged,
                  base::Unretained(this),
                  chromeos::onc::ONC_SOURCE_USER_POLICY));
 
   network_library_->AddNetworkProfileObserver(this);
 
   // Apply the current policies immediately.
   ApplyNetworkConfigurations();
 }
 
 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() {
   VLOG(1) << "Network profile list changed, applying policies.";
   ApplyNetworkConfigurations();
 }
 
 void NetworkConfigurationUpdater::OnUserPolicyInitialized() {
   VLOG(1) << "User policy initialized, applying policies.";
   user_policy_initialized_ = true;
   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,
     const base::Value* current) {
   VLOG(1) << "Policy for ONC source "
           << chromeos::onc::GetSourceAsString(onc_source) << " changed.";
   ApplyNetworkConfigurations();
 }
 
 void NetworkConfigurationUpdater::ApplyNetworkConfigurations() {
@@ skipping 23 matching lines @@
                    << chromeos::onc::GetSourceAsString(onc_source)
                    << " is not a string value.";
     }
   }
 
   // An empty string is not a valid ONC and generates warnings and
   // errors. Replace by a valid empty configuration.
   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