Add new ParsedCertificate class, move TrustStore to own file.

components/cast_certificate/cast_cert_validator.cc

 // Copyright 2014 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 "components/cast_certificate/cast_cert_validator.h"
 
 #include <stddef.h>
 #include <stdint.h>
 
 #include <algorithm>
 #include <memory>
 #include <utility>
 
 #include "base/memory/ptr_util.h"
 #include "base/memory/singleton.h"
 #include "net/cert/internal/certificate_policies.h"
 #include "net/cert/internal/extended_key_usage.h"
 #include "net/cert/internal/parse_certificate.h"
 #include "net/cert/internal/parse_name.h"
+#include "net/cert/internal/parsed_certificate.h"
 #include "net/cert/internal/signature_algorithm.h"
 #include "net/cert/internal/signature_policy.h"
+#include "net/cert/internal/trust_store.h"
 #include "net/cert/internal/verify_certificate_chain.h"
 #include "net/cert/internal/verify_signed_data.h"
 #include "net/der/input.h"
 
 namespace cast_certificate {
 namespace {
 
 // -------------------------------------------------------------------------
 // Cast trust anchors.
 // -------------------------------------------------------------------------
@@ skipping 16 matching lines @@
                            base::LeakySingletonTraits<CastTrustStore>>::get();
   }
 
   static net::TrustStore& Get() { return GetInstance()->store_; }
 
  private:
   friend struct base::DefaultSingletonTraits<CastTrustStore>;
 
   CastTrustStore() {
     // Initialize the trust store with two root certificates.
-    CHECK(store_.AddTrustedCertificateWithoutCopying(kCastRootCaDer,
-                                                     sizeof(kCastRootCaDer)));
-    CHECK(store_.AddTrustedCertificateWithoutCopying(kEurekaRootCaDer,
-                                                     sizeof(kEurekaRootCaDer)));
+    scoped_refptr<net::ParsedCertificate> root;
+    CHECK(root = net::ParsedCertificate::CreateFromCertificateData(
+              kCastRootCaDer, sizeof(kCastRootCaDer),
+              net::ParsedCertificate::DataSource::EXTERNAL_REFERENCE));
+    store_.AddTrustedCertificate(std::move(root));

[eroman, 2016/05/12 18:12:29]

Rather than needing "root" temporary, how about moving the CHECK into
AddTrustCertificate instead?

AddTrustCertificate(x) {
  CHECK(x);
  ...
}

I think we generally want to enforce that anyway to be safe. Also std::move() of
refptr feels strange.

[Ryan Sleevi, 2016/05/12 20:41:46]

Why? That's the recommended style.

[mattm, 2016/05/13 02:17:36]

I think in this case the CHECK makes sense since it's baked in data that should
never fail. In other cases we would want to more gracefully handle a failure to
parse, so having the CHECK in AddTrustedCertificate is sort of extraneous.
Style-wise it would probably be a DCHECK rather than a CHECK anyway.
 
Does feel strange if you're used to only seeing it on scoped_ptr, but this
allows you to pass a reference directly instead of doing an unnecessary
ref/unref cycle. Obviously not that important in this case, but I think it's
good to get in the habit of doing it.

+    CHECK(root = net::ParsedCertificate::CreateFromCertificateData(
+              kEurekaRootCaDer, sizeof(kEurekaRootCaDer),
+              net::ParsedCertificate::DataSource::EXTERNAL_REFERENCE));
+    store_.AddTrustedCertificate(std::move(root));
   }
 
   net::TrustStore store_;
   DISALLOW_COPY_AND_ASSIGN(CastTrustStore);
 };
 
 using ExtensionsMap = std::map<net::der::Input, net::ParsedExtension>;
 
 // Helper that looks up an extension by OID given a map of extensions.
 bool GetExtensionValue(const ExtensionsMap& extensions,
@@ skipping 92 matching lines @@
   return false;
 }
 
 // Checks properties on the target certificate.
 //
 //   * The Key Usage must include Digital Signature
 //   * THe Extended Key Usage must includ TLS Client Auth
 //   * May have the policy 1.3.6.1.4.1.11129.2.5.2 to indicate it
 //     is an audio-only device.
 WARN_UNUSED_RESULT bool CheckTargetCertificate(
-    const net::der::Input& cert_der,
+    const net::ParsedCertificate* cert,
     std::unique_ptr<CertVerificationContext>* context,
     CastDeviceCertPolicy* policy) {
-  // TODO(eroman): Simplify this. The certificate chain verification

[eroman, 2016/05/12 18:12:29]

nice, much better!

-  // function already parses this stuff, awkward to re-do it here.
-
-  net::der::Input tbs_certificate_tlv;
-  net::der::Input signature_algorithm_tlv;
-  net::der::BitString signature_value;
-  if (!net::ParseCertificate(cert_der, &tbs_certificate_tlv,
-                             &signature_algorithm_tlv, &signature_value))
-    return false;
-
-  net::ParsedTbsCertificate tbs;
-  if (!net::ParseTbsCertificate(tbs_certificate_tlv, &tbs))
-    return false;
-
-  // Get the extensions.
-  if (!tbs.has_extensions)
-    return false;
-  ExtensionsMap extensions;
-  if (!net::ParseExtensions(tbs.extensions_tlv, &extensions))
-    return false;
-
-  net::der::Input extension_value;
-
   // Get the Key Usage extension.
-  if (!GetExtensionValue(extensions, net::KeyUsageOid(), &extension_value))
-    return false;
-  net::der::BitString key_usage;
-  if (!net::ParseKeyUsage(extension_value, &key_usage))
+  if (!cert->has_key_usage())
     return false;
 
   // Ensure Key Usage contains digitalSignature.
-  if (!key_usage.AssertsBit(net::KEY_USAGE_BIT_DIGITAL_SIGNATURE))
+  if (!cert->key_usage().AssertsBit(net::KEY_USAGE_BIT_DIGITAL_SIGNATURE))
     return false;
 
   // Get the Extended Key Usage extension.
-  if (!GetExtensionValue(extensions, net::ExtKeyUsageOid(), &extension_value))
+  net::der::Input extension_value;
+  if (!GetExtensionValue(cert->unconsumed_extensions(), net::ExtKeyUsageOid(),

[eroman, 2016/05/12 18:12:29]

This doesn't seem right, are you sure?

I believe we have a direct accessor now for ext key usage which will need to be
checked instead.

[mattm, 2016/05/13 02:17:35]

ParsedCertificate extracts KeyUsage extension, but not ExtKeyUsage.

I did wonder about this in general though, if we add more extension handling
natively to ParsedCertificate, any existing code which was pulling that
extension from unconsumed_extensions() would break unless it was updated to use
the accessor for that specific extension. I guess as long as the users are just
in chromium that isn't too hard to handle. But perhaps we could add a method to
DCHECK that the extension wasn't handled by ParsedCertificate, so that any code
that would be broken gets a more obvious failure? 
Something like
DCHECK(ParsedCertificate::DoesNotConsumeExtension(net::ExtKeyUsageOid())
Would be even better if it could be a compile failure, don't know if that would
be possible..

Or could just keep a separate full list of the extensions (without the parsed
ones removed), and then such code would continue working even if it does
unnecessary re-parsing..

+                         &extension_value))
     return false;
   std::vector<net::der::Input> ekus;
   if (!net::ParseEKUExtension(extension_value, &ekus))
     return false;
 
   // Ensure Extended Key Usage contains client auth.
   if (!HasClientAuth(ekus))
     return false;
 
   // Check for an optional audio-only policy extension.
   *policy = CastDeviceCertPolicy::NONE;
-  if (GetExtensionValue(extensions, net::CertificatePoliciesOid(),
-                        &extension_value)) {
+  if (GetExtensionValue(cert->unconsumed_extensions(),

[eroman, 2016/05/12 18:12:29]

Same here.

[mattm, 2016/05/13 02:17:35]

CertificatePolicies isn't currently consumed by ParsedCertificate

+                        net::CertificatePoliciesOid(), &extension_value)) {
     std::vector<net::der::Input> policies;
     if (!net::ParseCertificatePoliciesExtension(extension_value, &policies))
       return false;
 
     // Look for an audio-only policy. Disregard any other policy found.
     if (std::find(policies.begin(), policies.end(), AudioOnlyPolicyOid()) !=
         policies.end()) {
       *policy = CastDeviceCertPolicy::AUDIO_ONLY;
     }
   }
 
   // Get the Common Name for the certificate.
   std::string common_name;
-  if (!GetCommonNameFromSubject(tbs.subject_tlv, &common_name))
+  if (!GetCommonNameFromSubject(cert->parsed_tbs().subject_tlv, &common_name))
     return false;
 
-  context->reset(new CertVerificationContextImpl(tbs.spki_tlv, common_name));
+  context->reset(new CertVerificationContextImpl(cert->parsed_tbs().spki_tlv,
+                                                 common_name));
   return true;
 }
 
 // Converts a base::Time::Exploded to a net::der::GeneralizedTime.
 net::der::GeneralizedTime ConvertExplodedTime(
     const base::Time::Exploded& exploded) {
   net::der::GeneralizedTime result;
   result.year = exploded.year;
   result.month = exploded.month;
   result.day = exploded.day_of_month;
   result.hours = exploded.hour;
   result.minutes = exploded.minute;
   result.seconds = exploded.second;
   return result;
 }
 
 }  // namespace
 
 bool VerifyDeviceCert(const std::vector<std::string>& certs,
                       const base::Time::Exploded& time,
                       std::unique_ptr<CertVerificationContext>* context,
                       CastDeviceCertPolicy* policy) {
   // The underlying verification function expects a sequence of
-  // der::Input, so wrap the data in it (cheap).
-  std::vector<net::der::Input> input_chain;
-  for (const auto& cert : certs)
-    input_chain.push_back(net::der::Input(&cert));
+  // ParsedCertificate.
+  std::vector<scoped_refptr<net::ParsedCertificate>> input_chain;
+  for (const std::string& cert_der : certs) {

[Ryan Sleevi, 2016/05/12 20:41:46]

fwiw, const auto& is also fine here

[mattm, 2016/05/13 02:17:35]

Done.

+    // No reference to the ParsedCertificate is kept past the end of this
+    // function, so using EXTERNAL_REFERENCE here is safe.
+    scoped_refptr<net::ParsedCertificate> cert(
+        net::ParsedCertificate::CreateFromCertificateData(
+            reinterpret_cast<const uint8_t*>(cert_der.data()), cert_der.size(),
+            net::ParsedCertificate::DataSource::EXTERNAL_REFERENCE));
+    if (!cert)
+      return false;
+    input_chain.push_back(std::move(cert));
+  }
 
   // Use a signature policy compatible with Cast's PKI.
   auto signature_policy = CreateCastSignaturePolicy();
 
   // Do RFC 5280 compatible certificate verification using the two Cast
   // trust anchors and Cast signature policy.
   if (!net::VerifyCertificateChain(input_chain, CastTrustStore::Get(),
                                    signature_policy.get(),
                                    ConvertExplodedTime(time))) {
     return false;
   }
 
   // Check properties of the leaf certificate (key usage, policy), and construct
   // a CertVerificationContext that uses its public key.
-  return CheckTargetCertificate(input_chain[0], context, policy);
+  return CheckTargetCertificate(input_chain[0].get(), context, policy);
 }
 
 std::unique_ptr<CertVerificationContext> CertVerificationContextImplForTest(
     const base::StringPiece& spki) {
   // Use a bogus CommonName, since this is just exposed for testing signature
   // verification by unittests.
   return base::WrapUnique(
       new CertVerificationContextImpl(net::der::Input(spki), "CommonName"));
 }
 
 bool AddTrustAnchorForTest(const uint8_t* data, size_t length) {
-  return CastTrustStore::Get().AddTrustedCertificateWithoutCopying(data,
-                                                                   length);
+  scoped_refptr<net::ParsedCertificate> anchor(
+      net::ParsedCertificate::CreateFromCertificateData(
+          kCastRootCaDer, sizeof(kCastRootCaDer),
+          net::ParsedCertificate::DataSource::INTERNAL_COPY));

[eroman, 2016/05/12 18:12:29]

EXTERNAL_REFERENCE to match previous code.

[Ryan Sleevi, 2016/05/12 20:41:46]

Why would EXTERNAL_REFERENCE be safe here? |anchor| is kept past the end of this
function.

[eroman, 2016/05/12 20:55:55]

It is safe because the function mandates those lifetimes:

https://code.google.com/p/chromium/codesearch#chromium/src/components/cast_ce...

We don't actually use this function in Chrome code, but Ryan (from Chromecast)
wanted it for some cast tests they have. The lifetime expectations are confirmed
in this code-review comment:

https://codereview.chromium.org/1888913005/diff/80001/extensions/common/cast/...

[mattm, 2016/05/13 02:17:35]

Done.

+  if (!anchor)
+    return false;
+  CastTrustStore::Get().AddTrustedCertificate(std::move(anchor));
+  return true;
 }
 
 }  // namespace cast_certificate