Add errors per ResultPath for CertPathBuilder.

BUG=634443

Review-Url: https://codereview.chromium.org/2292333002
Cr-Commit-Position: refs/heads/master@{#416296}

components/cast_certificate/cast_cert_validator.cc

@@ -299,8 +299,10 @@ bool VerifyDeviceCert(const std::vector<std::string>& certs,
   path_builder.AddCertIssuerSource(&intermediate_cert_issuer_source);
   net::CompletionStatus rv = path_builder.Run(base::Closure());
   DCHECK_EQ(rv, net::CompletionStatus::SYNC);
-  if (!result.is_success())
+  if (!result.HasValidPath()) {
+    // TODO(crbug.com/634443): Log error information.
     return false;
+  }
 
   // Check properties of the leaf certificate (key usage, policy), and construct
   // a CertVerificationContext that uses its public key.
@@ -313,12 +315,7 @@ bool VerifyDeviceCert(const std::vector<std::string>& certs,
       return false;
     }
   } else {
-    if (result.paths.empty() ||
-        !result.paths[result.best_result_index]->is_success())
-      return false;
-
-    if (!crl->CheckRevocation(result.paths[result.best_result_index]->path,
-                              time)) {
+    if (!crl->CheckRevocation(result.GetBestValidPath()->path, time)) {
       return false;
     }
   }

components/cast_certificate/cast_crl.cc

@@ -145,9 +145,9 @@ bool VerifyCRL(const Crl& crl,
                                     &result);
   net::CompletionStatus rv = path_builder.Run(base::Closure());
   DCHECK_EQ(rv, net::CompletionStatus::SYNC);
-  if (!result.is_success() || result.paths.empty() ||
-      !result.paths[result.best_result_index]->is_success()) {
+  if (!result.HasValidPath()) {
     VLOG(2) << "CRL - Issuer certificate verification failed.";
+    // TODO(crbug.com/634443): Log the error information.
     return false;
   }
   // There are no requirements placed on the leaf certificate having any
@@ -174,7 +174,7 @@ bool VerifyCRL(const Crl& crl,
   // "expiration" of the trust anchor is handled instead by its
   // presence in the trust store.
   *overall_not_after = not_after;
-  for (const auto& cert : result.paths[result.best_result_index]->path.certs) {
+  for (const auto& cert : result.GetBestValidPath()->path.certs) {
     net::der::GeneralizedTime cert_not_after = cert->tbs().validity_not_after;
     if (cert_not_after < *overall_not_after)
       *overall_not_after = cert_not_after;

net/cert/internal/path_builder.cc

@@ -621,6 +621,25 @@ CertPathBuilder::ResultPath::~ResultPath() = default;
 CertPathBuilder::Result::Result() = default;
 CertPathBuilder::Result::~Result() = default;
 
+const CertPathBuilder::ResultPath* CertPathBuilder::Result::GetBestValidPath()
+    const {
+  DCHECK((paths.empty() && best_result_index == 0) ||
+         best_result_index < paths.size());
+
+  if (best_result_index >= paths.size())
+    return nullptr;
+
+  const ResultPath* result_path = paths[best_result_index].get();
+  if (result_path->valid)
+    return result_path;
+
+  return nullptr;
+}
+
+bool CertPathBuilder::Result::HasValidPath() const {
+  return GetBestValidPath() != nullptr;
+}
+
 CertPathBuilder::CertPathBuilder(scoped_refptr<ParsedCertificate> cert,
                                  const TrustStore* trust_store,
                                  const SignaturePolicy* signature_policy,
@@ -695,15 +714,16 @@ CompletionStatus CertPathBuilder::DoGetNextPathComplete() {
     return CompletionStatus::SYNC;
   }
 
-  //  TODO(crbug.com/634443): Expose CertErrors on ResultPath.
-  CertErrors errors;
+  // Verify the entire certificate chain.
+  auto result_path = base::MakeUnique<ResultPath>();
   bool verify_result =
-      next_path_.trust_anchor.get() &&
       VerifyCertificateChain(next_path_.certs, next_path_.trust_anchor.get(),
-                             signature_policy_, time_, &errors);
+                             signature_policy_, time_, &result_path->errors);
   DVLOG(1) << "CertPathBuilder VerifyCertificateChain result = "
-           << verify_result;
-  AddResultPath(next_path_, verify_result);
+           << result_path->valid;
+  result_path->path = next_path_;
+  result_path->valid = verify_result;
+  AddResultPath(std::move(result_path));
 
   if (verify_result) {
     // Found a valid path, return immediately.
@@ -719,15 +739,11 @@ CompletionStatus CertPathBuilder::DoGetNextPathComplete() {
   return CompletionStatus::SYNC;
 }
 
-void CertPathBuilder::AddResultPath(const CertPath& path, bool is_success) {
-  std::unique_ptr<ResultPath> result_path(new ResultPath());
-  // TODO(mattm): better error reporting.
-  result_path->error = is_success ? OK : ERR_CERT_AUTHORITY_INVALID;
+void CertPathBuilder::AddResultPath(std::unique_ptr<ResultPath> result_path) {
   // TODO(mattm): set best_result_index based on number or severity of errors.
-  if (result_path->error == OK)
+  if (result_path->valid)
     out_result_->best_result_index = out_result_->paths.size();
   // TODO(mattm): add flag to only return a single path or all attempted paths?
-  result_path->path = path;
   out_result_->paths.push_back(std::move(result_path));
 }
 

net/cert/internal/path_builder.h

@@ -11,8 +11,8 @@
 
 #include "base/callback.h"
 #include "net/base/completion_callback.h"
-#include "net/base/net_errors.h"
 #include "net/base/net_export.h"
+#include "net/cert/internal/cert_errors.h"
 #include "net/cert/internal/completion_status.h"
 #include "net/cert/internal/parsed_certificate.h"
 #include "net/cert/internal/trust_store.h"
@@ -64,17 +64,19 @@ class NET_EXPORT CertPathBuilder {
     ResultPath();
     ~ResultPath();
 
-    // Returns true if this path was successfully verified.
-    bool is_success() const { return error == OK; }
-
-    // The (possibly partial) certificate path. In the case of an
-    // error path.trust_anchor may be nullptr.
+    // The (possibly partial) certificate path. Consumers must always test
+    // |valid| before using |path|. When |!valid| path.trust_anchor may be
+    // nullptr, and the path may be otherwise incomplete/invalid.
     CertPath path;
 
-    // A net error code result of attempting to verify this path.
-    // TODO(mattm): may want to have an independent result enum, which caller
-    // can map to a net error if they want.
-    int error = ERR_UNEXPECTED;
+    // The errors/warnings from this path. Note that the list of errors is
+    // independent of whether the path was |valid| (a valid path may
+    // contain errors/warnings, and vice versa an invalid path may not have
+    // logged any errors).
+    CertErrors errors;
+
+    // True if |path| is a correct verified certificate chain.
+    bool valid = false;
   };
 
   // Provides the overall result of path building. This includes the paths that
@@ -84,21 +86,18 @@ class NET_EXPORT CertPathBuilder {
     ~Result();
 
     // Returns true if there was a valid path.
-    bool is_success() const { return error() == OK; }
+    bool HasValidPath() const;
 
-    // Returns the net error code of the overall best result.
-    int error() const {
-      if (paths.empty())
-        return ERR_CERT_AUTHORITY_INVALID;
-      return paths[best_result_index]->error;
-    }
+    // Returns the ResultPath for the best valid path, or nullptr if there
+    // was none.
+    const ResultPath* GetBestValidPath() const;
 
     // List of paths that were attempted and the result for each.
     std::vector<std::unique_ptr<ResultPath>> paths;
 
     // Index into |paths|. Before use, |paths.empty()| must be checked.
-    // NOTE: currently the definition of "best" is fairly limited. Successful is
-    // better than unsuccessful, but otherwise nothing is guaranteed.
+    // NOTE: currently the definition of "best" is fairly limited. Valid is
+    // better than invalid, but otherwise nothing is guaranteed.
     size_t best_result_index = 0;
 
    private:
@@ -162,7 +161,7 @@ class NET_EXPORT CertPathBuilder {
   void HandleGotNextPath();
   CompletionStatus DoGetNextPathComplete();
 
-  void AddResultPath(const CertPath& path, bool is_success);
+  void AddResultPath(std::unique_ptr<ResultPath> result_path);
 
   base::Closure callback_;
 

net/cert/internal/path_builder_pkits_unittest.cc

@@ -92,7 +92,7 @@ class PathBuilderPkitsTestDelegate {
     CompletionStatus rv = path_builder.Run(base::Closure());
     EXPECT_EQ(CompletionStatus::SYNC, rv);
 
-    return result.is_success();
+    return result.HasValidPath();
   }
 };
 

net/cert/internal/path_builder_verify_certificate_chain_unittest.cc

@@ -39,7 +39,7 @@ class PathBuilderDelegate {
     CompletionStatus rv = path_builder.Run(base::Closure());
     EXPECT_EQ(CompletionStatus::SYNC, rv);
 
-    EXPECT_EQ(expected_result, result.is_success());
+    EXPECT_EQ(expected_result, result.HasValidPath());
   }
 };
 

net/tools/cert_verify_tool/verify_using_path_builder.cc

@@ -10,7 +10,6 @@
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/string_util.h"
 #include "crypto/sha2.h"
-#include "net/base/net_errors.h"
 #include "net/base/test_completion_callback.h"
 #include "net/cert/internal/cert_issuer_source_aia.h"
 #include "net/cert/internal/cert_issuer_source_static.h"
@@ -114,6 +113,48 @@ std::string SubjectFromTrustAnchor(const net::TrustAnchor* trust_anchor) {
   return SubjectToString(parsed_subject);
 }
 
+void PrintCertErrors(const net::CertErrors& errors) {
+  // TODO(crbug.com/634443): Include more detailed error information. Also this
+  // should likely be extracted to a common location and used by unit-tests and
+  // other debugging needs.
+  for (const auto& error : errors.errors()) {
+    std::cout << " " << error.type;
+  }
+}
+
+// Dumps a ResultPath to std::cout.
+void PrintResultPath(const net::CertPathBuilder::ResultPath* result_path,
+                     size_t index,
+                     bool is_best) {
+  std::cout << "path " << index << " "
+            << (result_path->valid ? "valid" : "invalid")
+            << (is_best ? " (best)" : "") << "\n";
+
+  // Print the certificate chain.
+  for (const auto& cert : result_path->path.certs) {
+    std::cout << " " << FingerPrintParsedCertificate(cert.get()) << " "
+              << SubjectFromParsedCertificate(cert.get()) << "\n";
+  }
+
+  // Print the trust anchor (if there was one).
+  const auto& trust_anchor = result_path->path.trust_anchor;
+  if (trust_anchor) {
+    std::string trust_anchor_cert_fingerprint = "<no cert>";
+    if (trust_anchor->cert()) {
+      trust_anchor_cert_fingerprint =
+          FingerPrintParsedCertificate(trust_anchor->cert().get());
+    }
+    std::cout << " " << trust_anchor_cert_fingerprint << " "
+              << SubjectFromTrustAnchor(trust_anchor.get()) << "\n";
+  }
+
+  // Print the errors.
+  if (result_path->errors.errors().empty()) {
+    std::cout << "Errors:\n";
+    PrintCertErrors(result_path->errors);
+  }
+}
+
 }  // namespace
 
 // Verifies |target_der_cert| using CertPathBuilder.
@@ -197,29 +238,14 @@ bool VerifyUsingPathBuilder(
     DVLOG(1) << "async completed.";
   }
 
-  // TODO(crbug.com/634443): Display the full error information.
-  std::cout << "CertPathBuilder best result: "
-            << net::ErrorToShortString(result.error()) << "\n";
+  // TODO(crbug.com/634443): Display any errors/warnings associated with path
+  //                         building that were not part of a particular
+  //                         PathResult.
+  std::cout << "CertPathBuilder result: "
+            << (result.HasValidPath() ? "SUCCESS" : "FAILURE") << "\n";
 
   for (size_t i = 0; i < result.paths.size(); ++i) {
-    std::cout << "path " << i << " "
-              << net::ErrorToShortString(result.paths[i]->error)
-              << ((result.best_result_index == i) ? " (best)" : "") << "\n";
-    for (const auto& cert : result.paths[i]->path.certs) {
-      std::cout << " " << FingerPrintParsedCertificate(cert.get()) << " "
-                << SubjectFromParsedCertificate(cert.get()) << "\n";
-    }
-
-    const auto& trust_anchor = result.paths[i]->path.trust_anchor;
-    if (trust_anchor) {
-      std::string trust_anchor_cert_fingerprint = "<no cert>";
-      if (trust_anchor->cert()) {
-        trust_anchor_cert_fingerprint =
-            FingerPrintParsedCertificate(trust_anchor->cert().get());
-      }
-      std::cout << " " << trust_anchor_cert_fingerprint << " "
-                << SubjectFromTrustAnchor(trust_anchor.get()) << "\n";
-    }
+    PrintResultPath(result.paths[i].get(), i, i == result.best_result_index);
   }
 
   // TODO(mattm): add flag to dump all paths, not just the final one?
@@ -232,5 +258,5 @@ bool VerifyUsingPathBuilder(
     }
   }
 
-  return result.error() == net::OK;
+  return result.HasValidPath();
 }