Don't add extra 'informational' errors in the delegate
So when IsPublicKeyAcceptable returns false, this unambigiously
adds the UnacceptablePublicKey error to the certificate.
However the delegate currently adds an Additional error to the
certificate, to add some information about what was there.
This causes problems with tests in chrome that care about
seeing the unacceptable public key as such, without the
certificate being tagged as just having multiple errors and
therefore generically invalid.
Instead of adding an extra error for information, add
this as a warning, then there's no problem.
Change-Id: Id24e913df17f6c8bbc63c19b31df3f80e59f638b
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/67888
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: Bob Beck <bbe@google.com>
diff --git a/pki/simple_path_builder_delegate.cc b/pki/simple_path_builder_delegate.cc
index 822019b..89000e3 100644
--- a/pki/simple_path_builder_delegate.cc
+++ b/pki/simple_path_builder_delegate.cc
@@ -97,7 +97,7 @@
unsigned int modulus_length_bits = RSA_bits(rsa);
if (modulus_length_bits < min_rsa_modulus_length_bits_) {
- errors->AddError(
+ errors->AddWarning(
kRsaModulusTooSmall,
CreateCertErrorParams2SizeT("actual", modulus_length_bits, "minimum",
min_rsa_modulus_length_bits_));
@@ -116,7 +116,7 @@
int curve_nid = EC_GROUP_get_curve_name(EC_KEY_get0_group(ec));
if (!IsAcceptableCurveForEcdsa(curve_nid)) {
- errors->AddError(kUnacceptableCurveForEcdsa);
+ errors->AddWarning(kUnacceptableCurveForEcdsa);
return false;
}
diff --git a/pki/testdata/verify_certificate_chain_unittest/target-has-512bit-rsa-key/main.test b/pki/testdata/verify_certificate_chain_unittest/target-has-512bit-rsa-key/main.test
index 8b3e869..dfd7ff0 100644
--- a/pki/testdata/verify_certificate_chain_unittest/target-has-512bit-rsa-key/main.test
+++ b/pki/testdata/verify_certificate_chain_unittest/target-has-512bit-rsa-key/main.test
@@ -4,7 +4,7 @@
key_purpose: SERVER_AUTH
expected_errors:
----- Certificate i=0 (CN=Target) -----
-ERROR: RSA modulus too small
+WARNING: RSA modulus too small
actual: 512
minimum: 1024
ERROR: Unacceptable public key
diff --git a/pki/testdata/verify_certificate_chain_unittest/target-signed-by-512bit-rsa/main.test b/pki/testdata/verify_certificate_chain_unittest/target-signed-by-512bit-rsa/main.test
index 6af47f5..d94fd2b 100644
--- a/pki/testdata/verify_certificate_chain_unittest/target-signed-by-512bit-rsa/main.test
+++ b/pki/testdata/verify_certificate_chain_unittest/target-signed-by-512bit-rsa/main.test
@@ -4,7 +4,7 @@
key_purpose: SERVER_AUTH
expected_errors:
----- Certificate i=1 (CN=Intermediate) -----
-ERROR: RSA modulus too small
+WARNING: RSA modulus too small
actual: 512
minimum: 1024
ERROR: Unacceptable public key
diff --git a/pki/verify_certificate_chain.h b/pki/verify_certificate_chain.h
index 9510fa9..4603ace 100644
--- a/pki/verify_certificate_chain.h
+++ b/pki/verify_certificate_chain.h
@@ -56,18 +56,22 @@
class OPENSSL_EXPORT VerifyCertificateChainDelegate {
public:
// Implementations should return true if |signature_algorithm| is allowed for
- // certificate signing, false otherwise. When returning false implementations
- // can optionally add high-severity errors to |errors| with details on why it
- // was rejected.
+ // certificate signing, false otherwise. When false is returned, the caller
+ // will add a high severity error of kUnacceptableSignatureAlgorithm to
+ // |errors|. When returning false, implementations can optionally add warnings
+ // to errors to |errors| with details on why it was rejected. Implementations
+ // may add any further details on why the signature algorithm was deemed
+ // unacceptable by adding warnings to |errors|.
virtual bool IsSignatureAlgorithmAcceptable(
SignatureAlgorithm signature_algorithm, CertErrors *errors) = 0;
- // Implementations should return true if |public_key| is acceptable. This is
- // called for each certificate in the chain, including the target certificate.
- // When returning false implementations can optionally add high-severity
- // errors to |errors| with details on why it was rejected.
- //
- // |public_key| can be assumed to be non-null.
+ // Implementations should return true if |public_key| is acceptable, false
+ // otherwise. This is called for each certificate in the chain, including the
+ // target certificate. When false is returned, the caller will add a high
+ // severity error of kUnacceptablePublicKey to |errors|. When returning false,
+ // implementations may add any further details on why the public key was
+ // deemed unacceptable by adding warnings to |errors|. |public_key| can be
+ // assumed to be non-null.
virtual bool IsPublicKeyAcceptable(EVP_PKEY *public_key,
CertErrors *errors) = 0;
