Call CheckPathAfterVerification in deadline/iteration limit case

For symmetry with the CheckPathAfterVerification calls that happen
when partial paths are found during pathbuilding.

Also removes the DebugLog after VerifyCertificateChain, as the
CheckPathAfterVerification is already called there, the delegate can
do any logging it wants there.

Bug: chromium:634484
Change-Id: I93e472b6ec813536b1808f1459a3a477b1b0c876
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/64667
Commit-Queue: David Benjamin <davidben@google.com>
Auto-Submit: Matt Mueller <mattm@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: Matt Mueller <mattm@google.com>

pki/path_builder.cc

@@ -818,6 +818,12 @@ CertPathBuilder::Result CertPathBuilder::Run() {
           result_path->errors.GetOtherErrors()->AddError(
               cert_errors::kInternalError);
         }
+
+        // Allow the delegate to do any processing or logging of the partial
+        // path. (This is for symmetry for the other CheckPathAfterVerification
+        // which also gets called on partial paths.)
+        delegate_->CheckPathAfterVerification(*this, result_path.get());
+
         AddResultPath(std::move(result_path));
       }
       out_result_.iteration_count = iteration_count;
@@ -840,11 +846,6 @@ CertPathBuilder::Result CertPathBuilder::Run() {
           &result_path->user_constrained_policy_set, &result_path->errors);
     }
 
-    if (delegate_->IsDebugLogEnabled()) {
-      delegate_->DebugLog("CertPathBuilder VerifyCertificateChain errors:\n" +
-                 result_path->errors.ToDebugString(result_path->certs));
-    }
-
     // Give the delegate a chance to add errors to the path.
     delegate_->CheckPathAfterVerification(*this, result_path.get());
 

pki/path_builder.h

@@ -88,10 +88,13 @@ struct OPENSSL_EXPORT CertPathBuilderResultPath {
 class OPENSSL_EXPORT CertPathBuilderDelegate
     : public VerifyCertificateChainDelegate {
  public:
-  // This is called during path building on candidate paths which have already
-  // been run through RFC 5280 verification. |path| may already have errors
-  // and warnings set on it. Delegates can "reject" a candidate path from path
-  // building by adding high severity errors.
+  // This is called during path building on candidate paths. These are either
+  // paths which have already been run through RFC 5280 verification, or
+  // partial paths that the path builder cannot continue either due to not
+  // finding a matching issuer or reaching a configured pathbuilding limit.
+  // |path| may already have errors and warnings set on it. Delegates can
+  // "reject" a candidate path from path building by adding high severity
+  // errors.
   virtual void CheckPathAfterVerification(const CertPathBuilder &path_builder,
                                           CertPathBuilderResultPath *path) = 0;
 

pki/path_builder_unittest.cc

@@ -60,6 +60,24 @@ class TestPathBuilderDelegate : public SimplePathBuilderDelegate {
   MockSignatureVerifyCache cache_;
 };
 
+class CertPathBuilderDelegateBase : public SimplePathBuilderDelegate {
+ public:
+  CertPathBuilderDelegateBase()
+      : SimplePathBuilderDelegate(
+            1024, SimplePathBuilderDelegate::DigestPolicy::kWeakAllowSha1) {}
+  void CheckPathAfterVerification(const CertPathBuilder &path_builder,
+                                  CertPathBuilderResultPath *path) override {
+    ADD_FAILURE() << "Tests must override this";
+  }
+};
+
+class MockPathBuilderDelegate : public CertPathBuilderDelegateBase {
+ public:
+  MOCK_METHOD2(CheckPathAfterVerification,
+               void(const CertPathBuilder &path_builder,
+                    CertPathBuilderResultPath *path));
+};
+
 // AsyncCertIssuerSourceStatic always returns its certs asynchronously.
 class AsyncCertIssuerSourceStatic : public CertIssuerSource {
  public:
@@ -628,8 +646,13 @@ TEST_F(PathBuilderMultiRootTest, TestIterationLimit) {
   for (const bool insufficient_limit : {true, false}) {
     SCOPED_TRACE(insufficient_limit);
 
+    StrictMock<MockPathBuilderDelegate> mock_delegate;
+    // The CheckPathAfterVerification delegate should be called regardless if
+    // the iteration limit is reached.
+    EXPECT_CALL(mock_delegate, CheckPathAfterVerification(_, _));
+
     CertPathBuilder path_builder(
-        a_by_b_, &trust_store, &delegate_, time_, KeyPurpose::ANY_EKU,
+        a_by_b_, &trust_store, &mock_delegate, time_, KeyPurpose::ANY_EKU,
         initial_explicit_policy_, user_initial_policy_set_,
         initial_policy_mapping_inhibit_, initial_any_policy_inhibit_);
     path_builder.AddCertIssuerSource(&sync_certs);
@@ -1872,24 +1895,6 @@ TEST_F(PathBuilderDistrustTest, TargetIntermediateRoot) {
 class PathBuilderCheckPathAfterVerificationTest
     : public PathBuilderSimpleChainTest {};
 
-class CertPathBuilderDelegateBase : public SimplePathBuilderDelegate {
- public:
-  CertPathBuilderDelegateBase()
-      : SimplePathBuilderDelegate(
-            1024, SimplePathBuilderDelegate::DigestPolicy::kWeakAllowSha1) {}
-  void CheckPathAfterVerification(const CertPathBuilder &path_builder,
-                                  CertPathBuilderResultPath *path) override {
-    ADD_FAILURE() << "Tests must override this";
-  }
-};
-
-class MockPathBuilderDelegate : public CertPathBuilderDelegateBase {
- public:
-  MOCK_METHOD2(CheckPathAfterVerification,
-               void(const CertPathBuilder &path_builder,
-                    CertPathBuilderResultPath *path));
-};
-
 TEST_F(PathBuilderCheckPathAfterVerificationTest, NoOpToValidPath) {
   StrictMock<MockPathBuilderDelegate> delegate;
   // Just verify that the hook is called.