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>
diff --git a/pki/path_builder.cc b/pki/path_builder.cc
index c629550..f89576e 100644
--- a/pki/path_builder.cc
+++ b/pki/path_builder.cc
@@ -818,6 +818,12 @@
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 @@
&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());
diff --git a/pki/path_builder.h b/pki/path_builder.h
index 59cfc24..325477b 100644
--- a/pki/path_builder.h
+++ b/pki/path_builder.h
@@ -88,10 +88,13 @@
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;
diff --git a/pki/path_builder_unittest.cc b/pki/path_builder_unittest.cc
index 2c0781c..c76670d 100644
--- a/pki/path_builder_unittest.cc
+++ b/pki/path_builder_unittest.cc
@@ -60,6 +60,24 @@
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 @@
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 @@
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.
