Add new ParsedCertificate class, move TrustStore to own file.

components/cast_certificate/cast_cert_validator.cc

Index: components/cast_certificate/cast_cert_validator.cc
diff --git a/components/cast_certificate/cast_cert_validator.cc b/components/cast_certificate/cast_cert_validator.cc
index b83ad541174068c04236a0af64260beda24bc4c5..16a8e28a058d3eec1c87e6f95b8aa6e60fd9e678 100644
--- a/components/cast_certificate/cast_cert_validator.cc
+++ b/components/cast_certificate/cast_cert_validator.cc
@@ -17,8 +17,10 @@
 #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"
@@ -55,10 +57,15 @@ class 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_;
@@ -171,45 +178,21 @@ bool HasClientAuth(const std::vector<net::der::Input>& ekus) {
 //   * 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))
@@ -221,8 +204,8 @@ WARN_UNUSED_RESULT bool CheckTargetCertificate(
 
   // 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;
@@ -236,10 +219,11 @@ WARN_UNUSED_RESULT bool CheckTargetCertificate(
 
   // 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;
 }
 
@@ -263,10 +247,19 @@ bool VerifyDeviceCert(const std::vector<std::string>& certs,
                       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();
@@ -281,7 +274,7 @@ bool VerifyDeviceCert(const std::vector<std::string>& certs,
 
   // 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(
@@ -293,8 +286,14 @@ std::unique_ptr<CertVerificationContext> CertVerificationContextImplForTest(
 }
 
 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