Add CertErrors* parameter to the main Certificate parsing functions.

components/cast_certificate/cast_crl.cc

Index: components/cast_certificate/cast_crl.cc
diff --git a/components/cast_certificate/cast_crl.cc b/components/cast_certificate/cast_crl.cc
index 25be1d251c072ae43ee46db4827037d4671ba6a5..b6227b789795ad9147b9f868eb4db16b4e020477 100644
--- a/components/cast_certificate/cast_crl.cc
+++ b/components/cast_certificate/cast_crl.cc
@@ -12,6 +12,7 @@
 #include "base/memory/singleton.h"
 #include "components/cast_certificate/proto/revocation.pb.h"
 #include "crypto/sha2.h"
+#include "net/cert/internal/cert_errors.h"
 #include "net/cert/internal/parse_certificate.h"
 #include "net/cert/internal/parsed_certificate.h"
 #include "net/cert/internal/path_builder.h"
@@ -23,8 +24,8 @@
 #include "net/cert/x509_certificate.h"
 #include "net/der/encode_values.h"
 #include "net/der/input.h"
-#include "net/der/parser.h"
 #include "net/der/parse_values.h"
+#include "net/der/parser.h"
 
 namespace cast_certificate {
 namespace {
@@ -62,11 +63,11 @@ class CastCRLTrustStore {
 
   CastCRLTrustStore() {
     // Initialize the trust store with the root certificate.
+    net::CertErrors errors;
     scoped_refptr<net::ParsedCertificate> cert =
-        net::ParsedCertificate::CreateFromCertificateData(
-            kCastCRLRootCaDer, sizeof(kCastCRLRootCaDer),
-            net::ParsedCertificate::DataSource::EXTERNAL_REFERENCE, {});
-    CHECK(cert);
+        net::ParsedCertificate::CreateWithoutCopyingUnsafe(
+            kCastCRLRootCaDer, sizeof(kCastCRLRootCaDer), {}, &errors);
+    CHECK(cert) << errors.ToDebugString();
     // Enforce pathlen constraints and policies defined on the root certificate.
     scoped_refptr<net::TrustAnchor> anchor =
         net::TrustAnchor::CreateFromCertificateWithConstraints(std::move(cert));
@@ -107,13 +108,12 @@ bool VerifyCRL(const Crl& crl,
                net::TrustStore* trust_store,
                net::der::GeneralizedTime* overall_not_after) {
   // Verify the trust of the CRL authority.
+  net::CertErrors errors;
   scoped_refptr<net::ParsedCertificate> parsed_cert =
-      net::ParsedCertificate::CreateFromCertificateData(
-          reinterpret_cast<const uint8_t*>(crl.signer_cert().data()),
-          crl.signer_cert().size(),
-          net::ParsedCertificate::DataSource::EXTERNAL_REFERENCE, {});
+      net::ParsedCertificate::Create(crl.signer_cert().data(), {}, &errors);
   if (parsed_cert == nullptr) {
-    VLOG(2) << "CRL - Issuer certificate parsing failed.";
+    VLOG(2) << "CRL - Issuer certificate parsing failed:\n"
+            << errors.ToDebugString();
     return false;
   }
 
@@ -125,13 +125,13 @@ bool VerifyCRL(const Crl& crl,
   auto signature_policy = CreateCastSignaturePolicy();
   std::unique_ptr<net::SignatureAlgorithm> signature_algorithm_type =
       net::SignatureAlgorithm::CreateRsaPkcs1(net::DigestAlgorithm::Sha256);
-  net::CertErrors errors;
   if (!VerifySignedData(*signature_algorithm_type,
                         net::der::Input(&crl.tbs_crl()),
                         signature_value_bit_string, parsed_cert->tbs().spki_tlv,
                         signature_policy.get(), &errors)) {

[mattm, 2016/09/12 23:29:27]

Should it use a separate errors object just in case there is anything left over
from the ParsedCertificate call?

[eroman, 2016/09/12 23:42:59]

Good question.

I debated this a bit when writing, and my thinking was:
 (a) If there was an error, the first call would return
 (b) If there was a warning from the parsing, and then VerifySignedData failed,
that warning would leak through here.

In case (b) it isn't necessarily harmful to show that warning here, maybe it is
even relevant. (Logging the warnings each time might be overkill though)

WDYT if I add a ".Clear()" method on CertErrors?
Having two different named "errors" seems a bit weird, so I could either use
.Clear() or use a short lived instance for each call.

[mattm, 2016/09/13 22:27:38]

I worry a bit about a Clear() method since it seems it could get weird if you
used it in the presence of scopers. I guess you could add rules about when it's
allowed to be called, but seems easier just to not have it.

I suppose there is little chance that parsing and verifying would use the same
error ids, so it wouldn't be overly confusing if it did happen. So I guess just
leaving it could be okay too.

[eroman, 2016/09/13 22:47:30]

I went ahead and created 2 different error objects: |parse_errors| and
|verify_errors| to remove the ambiguity.

(I decided against enclosing them in smaller scopes since I found it hurt
readability.)

I'll see what the Cast reviewer thinks.

     // TODO(634443): Dump the error information.
-    VLOG(2) << "CRL - Signature verification failed.";
+    VLOG(2) << "CRL - Signature verification failed:\n"
+            << errors.ToDebugString();
     return false;
   }