diff --git a/src/core/tsi/ssl_transport_security.cc b/src/core/tsi/ssl_transport_security.cc index 1ee9faf284a..17ad9077393 100644 --- a/src/core/tsi/ssl_transport_security.cc +++ b/src/core/tsi/ssl_transport_security.cc @@ -1045,10 +1045,12 @@ static bool ValidateCrl(X509* cert, X509* issuer, X509_CRL* crl) { // 6.3.3b verify issuer and scope valid = grpc_core::VerifyCrlCertIssuerNamesMatch(crl, cert); if (!valid) { + gpr_log(GPR_DEBUG, "CRL and cert issuer names mismatched."); return valid; } valid = grpc_core::HasCrlSignBit(issuer); if (!valid) { + gpr_log(GPR_DEBUG, "CRL issuer not allowed to sign CRLs."); return valid; } // 6.3.3c Not supporting deltas @@ -1058,6 +1060,9 @@ static bool ValidateCrl(X509* cert, X509* issuer, X509_CRL* crl) { // same. // 6.3.3g Verify CRL Signature valid = grpc_core::VerifyCrlSignature(crl, issuer); + if (!valid) { + gpr_log(GPR_DEBUG, "Crl signature check failed."); + } return valid; } @@ -1146,6 +1151,7 @@ static int CheckChainRevocation( static int CustomVerificationFunction(X509_STORE_CTX* ctx, void* arg) { int ret = X509_verify_cert(ctx); if (ret <= 0) { + gpr_log(GPR_DEBUG, "Failed to verify cert chain."); // Verification failed. We shouldn't expect to have a verified chain, so // there is no need to attempt to extract the root cert from it, check for // revocation, or check anything else. @@ -1154,10 +1160,10 @@ static int CustomVerificationFunction(X509_STORE_CTX* ctx, void* arg) { grpc_core::experimental::CrlProvider* provider = GetCrlProvider(ctx); if (provider != nullptr) { ret = CheckChainRevocation(ctx, provider); - } - if (ret <= 0) { - // Something has failed return the failure - return ret; + if (ret <= 0) { + gpr_log(GPR_DEBUG, "The chain failed revocation checks."); + return ret; + } } return RootCertExtractCallback(ctx, arg); } diff --git a/src/core/tsi/ssl_transport_security_utils.cc b/src/core/tsi/ssl_transport_security_utils.cc index 020e88fa0ac..1c05de9293d 100644 --- a/src/core/tsi/ssl_transport_security_utils.cc +++ b/src/core/tsi/ssl_transport_security_utils.cc @@ -259,12 +259,19 @@ bool VerifyCrlSignature(X509_CRL* crl, X509* issuer) { if (ikey == nullptr) { // Can't verify signature because we couldn't get the pubkey, fail the // check. + gpr_log(GPR_DEBUG, "Could not public key from certificate."); EVP_PKEY_free(ikey); return false; } - bool ret = X509_CRL_verify(crl, ikey) == 1; + int ret = X509_CRL_verify(crl, ikey); + if (ret < 0) { + gpr_log(GPR_DEBUG, + "There was an unexpected problem checking the CRL signature."); + } else if (ret == 0) { + gpr_log(GPR_DEBUG, "CRL failed verification."); + } EVP_PKEY_free(ikey); - return ret; + return ret == 1; } bool VerifyCrlCertIssuerNamesMatch(X509_CRL* crl, X509* cert) { diff --git a/test/core/tsi/BUILD b/test/core/tsi/BUILD index a4854f4dedf..fcf97c83eca 100644 --- a/test/core/tsi/BUILD +++ b/test/core/tsi/BUILD @@ -71,10 +71,10 @@ grpc_cc_test( "//test/core/tsi/test_creds/crl_data:evil_ca.pem", "//test/core/tsi/test_creds/crl_data:intermediate_ca.pem", "//test/core/tsi/test_creds/crl_data:leaf_signed_by_intermediate.pem", + "//test/core/tsi/test_creds/crl_data/bad_crls:invalid_content.crl", + "//test/core/tsi/test_creds/crl_data/bad_crls:invalid_signature.crl", "//test/core/tsi/test_creds/crl_data/crls:current.crl", "//test/core/tsi/test_creds/crl_data/crls:intermediate.crl", - "//test/core/tsi/test_creds/crl_data/crls:invalid_content.crl", - "//test/core/tsi/test_creds/crl_data/crls:invalid_signature.crl", ], external_deps = ["gtest"], language = "C++", @@ -136,13 +136,13 @@ grpc_cc_test( "//test/core/tsi/test_creds/crl_data:revoked.pem", "//test/core/tsi/test_creds/crl_data:valid.key", "//test/core/tsi/test_creds/crl_data:valid.pem", + "//test/core/tsi/test_creds/crl_data/bad_crls:evil.crl", + "//test/core/tsi/test_creds/crl_data/bad_crls:invalid_content.crl", + "//test/core/tsi/test_creds/crl_data/bad_crls:invalid_signature.crl", "//test/core/tsi/test_creds/crl_data/crls:ab06acdd.r0", "//test/core/tsi/test_creds/crl_data/crls:b9322cac.r0", "//test/core/tsi/test_creds/crl_data/crls:current.crl", - "//test/core/tsi/test_creds/crl_data/crls:evil.crl", "//test/core/tsi/test_creds/crl_data/crls:intermediate.crl", - "//test/core/tsi/test_creds/crl_data/crls:invalid_content.crl", - "//test/core/tsi/test_creds/crl_data/crls:invalid_signature.crl", "//test/core/tsi/test_creds/crl_data/crls_missing_intermediate:ab06acdd.r0", "//test/core/tsi/test_creds/crl_data/crls_missing_root:b9322cac.r0", ], diff --git a/test/core/tsi/crl_ssl_transport_security_test.cc b/test/core/tsi/crl_ssl_transport_security_test.cc index da873e80956..93018a818d0 100644 --- a/test/core/tsi/crl_ssl_transport_security_test.cc +++ b/test/core/tsi/crl_ssl_transport_security_test.cc @@ -69,10 +69,11 @@ const char* kRootCrlPath = "test/core/tsi/test_creds/crl_data/crls/current.crl"; const char* kIntermediateCrlPath = "test/core/tsi/test_creds/crl_data/crls/intermediate.crl"; const char* kModifiedSignaturePath = - "test/core/tsi/test_creds/crl_data/crls/invalid_signature.crl"; + "test/core/tsi/test_creds/crl_data/bad_crls/invalid_signature.crl"; const char* kModifiedContentPath = - "test/core/tsi/test_creds/crl_data/crls/invalid_content.crl"; -const char* kEvilCrlPath = "test/core/tsi/test_creds/crl_data/crls/evil.crl"; + "test/core/tsi/test_creds/crl_data/bad_crls/invalid_content.crl"; +const char* kEvilCrlPath = + "test/core/tsi/test_creds/crl_data/bad_crls/evil.crl"; class CrlSslTransportSecurityTest : public testing::TestWithParam { diff --git a/test/core/tsi/ssl_transport_security_utils_test.cc b/test/core/tsi/ssl_transport_security_utils_test.cc index 54e705d7574..2b7b6dfb36f 100644 --- a/test/core/tsi/ssl_transport_security_utils_test.cc +++ b/test/core/tsi/ssl_transport_security_utils_test.cc @@ -46,9 +46,9 @@ namespace testing { const char* kValidCrl = "test/core/tsi/test_creds/crl_data/crls/current.crl"; const char* kCrlIssuer = "test/core/tsi/test_creds/crl_data/ca.pem"; const char* kModifiedSignature = - "test/core/tsi/test_creds/crl_data/crls/invalid_signature.crl"; + "test/core/tsi/test_creds/crl_data/bad_crls/invalid_signature.crl"; const char* kModifiedContent = - "test/core/tsi/test_creds/crl_data/crls/invalid_content.crl"; + "test/core/tsi/test_creds/crl_data/bad_crls/invalid_content.crl"; const char* kIntermediateCrl = "test/core/tsi/test_creds/crl_data/crls/intermediate.crl"; const char* kIntermediateCrlIssuer = diff --git a/test/core/tsi/test_creds/crl_data/README b/test/core/tsi/test_creds/crl_data/README index f69fd1b75b3..3e5cfab739f 100644 --- a/test/core/tsi/test_creds/crl_data/README +++ b/test/core/tsi/test_creds/crl_data/README @@ -57,7 +57,8 @@ Run `ca_with_akid_gen.sh` from the `test/core/tsi/test_creds/crl_data` directory Create CRLs with modified signatures and content ---------------------------------------------------------------------------- -Make two copies of `test/core/tsi/test_creds/crl_data/crls/current.crl` named `test/core/tsi/test_creds/crl_data/crls/invalid_content.crl` and `test/core/tsi/test_creds/crl_data/crls/invalid_signature.crl`. +Make a directory `test/core/tsi/test_creds/crl_data/bad_crls +Make two copies of `test/core/tsi/test_creds/crl_data/crls/current.crl` in the bad_crls directory named `test/core/tsi/test_creds/crl_data/bad_crls/invalid_content.crl` and `test/core/tsi/test_creds/crl_data/bad_crls/invalid_signature.crl`. In `invalid_content.crl`, change the first letter on the second line. In `invalid_signature.crl`, change the last letter before the `=` on the second to last line. diff --git a/test/core/tsi/test_creds/crl_data/bad_crls/BUILD b/test/core/tsi/test_creds/crl_data/bad_crls/BUILD new file mode 100644 index 00000000000..f74dabcf8a0 --- /dev/null +++ b/test/core/tsi/test_creds/crl_data/bad_crls/BUILD @@ -0,0 +1,21 @@ +# Copyright 2021 gRPC authors. +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +licenses(["notice"]) + +exports_files([ + "evil.crl", + "invalid_content.crl", + "invalid_signature.crl", +]) diff --git a/test/core/tsi/test_creds/crl_data/crls/evil.crl b/test/core/tsi/test_creds/crl_data/bad_crls/evil.crl similarity index 100% rename from test/core/tsi/test_creds/crl_data/crls/evil.crl rename to test/core/tsi/test_creds/crl_data/bad_crls/evil.crl diff --git a/test/core/tsi/test_creds/crl_data/crls/invalid_content.crl b/test/core/tsi/test_creds/crl_data/bad_crls/invalid_content.crl similarity index 100% rename from test/core/tsi/test_creds/crl_data/crls/invalid_content.crl rename to test/core/tsi/test_creds/crl_data/bad_crls/invalid_content.crl diff --git a/test/core/tsi/test_creds/crl_data/crls/invalid_signature.crl b/test/core/tsi/test_creds/crl_data/bad_crls/invalid_signature.crl similarity index 100% rename from test/core/tsi/test_creds/crl_data/crls/invalid_signature.crl rename to test/core/tsi/test_creds/crl_data/bad_crls/invalid_signature.crl diff --git a/test/core/tsi/test_creds/crl_data/crls/BUILD b/test/core/tsi/test_creds/crl_data/crls/BUILD index e1a36f42c8f..922d279aa6a 100644 --- a/test/core/tsi/test_creds/crl_data/crls/BUILD +++ b/test/core/tsi/test_creds/crl_data/crls/BUILD @@ -19,7 +19,4 @@ exports_files([ "b9322cac.r0", "current.crl", "intermediate.crl", - "invalid_signature.crl", - "invalid_content.crl", - "evil.crl", ]) diff --git a/test/cpp/end2end/crl_provider_test.cc b/test/cpp/end2end/crl_provider_test.cc index 6902fd7443e..28dabae37d3 100644 --- a/test/cpp/end2end/crl_provider_test.cc +++ b/test/cpp/end2end/crl_provider_test.cc @@ -61,7 +61,8 @@ const char* kRevokedCertPath = "test/core/tsi/test_creds/crl_data/revoked.pem"; const char* kValidKeyPath = "test/core/tsi/test_creds/crl_data/valid.key"; const char* kValidCertPath = "test/core/tsi/test_creds/crl_data/valid.pem"; const char* kRootCrlPath = "test/core/tsi/test_creds/crl_data/crls/current.crl"; -const char* kCrlDirectoryPath = "test/core/tsi/test_creds/crl_data/crls/"; +const char* kCrlDirectoryPath = + "test/core/tsi/test_creds/crl_data/crl_provider_test_dir/"; constexpr char kMessage[] = "Hello"; // This test must be at the top of the file because the