diff --git a/include/grpc/grpc_security.h b/include/grpc/grpc_security.h index c81b81a207b90..a09daa6259075 100644 --- a/include/grpc/grpc_security.h +++ b/include/grpc/grpc_security.h @@ -1074,9 +1074,12 @@ typedef struct grpc_tls_certificate_verifier_external { * request: request information exposed to the function implementer. It will * be the same request object that was passed to verify(), and it * tells the cancel() which request to cancel. + * code: status code for the cancellation. + * error_details: status message for the cancellation. */ void (*cancel)(void* user_data, - grpc_tls_custom_verification_check_request* request); + grpc_tls_custom_verification_check_request* request, + grpc_status_code code, const char* error_details); /** * A function pointer that does some additional destruction work when the * verifier is destroyed. This is used when the caller wants to associate some @@ -1186,7 +1189,8 @@ int grpc_tls_certificate_verifier_verify( */ void grpc_tls_certificate_verifier_cancel( grpc_tls_certificate_verifier* verifier, - grpc_tls_custom_verification_check_request* request); + grpc_tls_custom_verification_check_request* request, grpc_status_code code, + const char* error_details); /** * EXPERIMENTAL API - Subject to change diff --git a/include/grpcpp/security/tls_certificate_verifier.h b/include/grpcpp/security/tls_certificate_verifier.h index 61c0acb0fb30b..cc28314877321 100644 --- a/include/grpcpp/security/tls_certificate_verifier.h +++ b/include/grpcpp/security/tls_certificate_verifier.h @@ -23,6 +23,8 @@ #include #include +#include "absl/status/status.h" + #include #include #include @@ -116,7 +118,8 @@ class CertificateVerifier { // verification request is pending. // // request: the verification information associated with this request - void Cancel(TlsCustomVerificationCheckRequest* request); + void Cancel(TlsCustomVerificationCheckRequest* request, + const absl::Status& status); // Gets the core verifier used internally. grpc_tls_certificate_verifier* c_verifier() { return verifier_; } @@ -181,7 +184,8 @@ class ExternalCertificateVerifier { // passed to Verify() with a status indicating the cancellation. // // request: the verification information associated with this request - virtual void Cancel(TlsCustomVerificationCheckRequest* request) = 0; + virtual void Cancel(TlsCustomVerificationCheckRequest* request, + const absl::Status& status) = 0; protected: ExternalCertificateVerifier(); @@ -207,7 +211,8 @@ class ExternalCertificateVerifier { char** sync_error_details); static void CancelInCoreExternalVerifier( - void* user_data, grpc_tls_custom_verification_check_request* request); + void* user_data, grpc_tls_custom_verification_check_request* request, + grpc_status_code code, const char* error_details); static void DestructInCoreExternalVerifier(void* user_data); diff --git a/src/core/lib/security/credentials/tls/grpc_tls_certificate_verifier.cc b/src/core/lib/security/credentials/tls/grpc_tls_certificate_verifier.cc index 79c6cc2b028bd..bd9607df2f801 100644 --- a/src/core/lib/security/credentials/tls/grpc_tls_certificate_verifier.cc +++ b/src/core/lib/security/credentials/tls/grpc_tls_certificate_verifier.cc @@ -23,13 +23,13 @@ #include #include +#include "absl/status/status.h" #include "absl/strings/string_view.h" #include #include #include -#include "src/core/lib/debug/trace.h" #include "src/core/lib/gprpp/host_port.h" #include "src/core/lib/iomgr/exec_ctx.h" #include "src/core/lib/security/credentials/tls/tls_utils.h" @@ -210,9 +210,11 @@ int grpc_tls_certificate_verifier_verify( void grpc_tls_certificate_verifier_cancel( grpc_tls_certificate_verifier* verifier, - grpc_tls_custom_verification_check_request* request) { + grpc_tls_custom_verification_check_request* request, grpc_status_code code, + const char* error_details) { grpc_core::ExecCtx exec_ctx; - verifier->Cancel(request); + verifier->Cancel(request, absl::Status(static_cast(code), + error_details ? error_details : "")); } grpc_tls_certificate_verifier* grpc_tls_certificate_verifier_external_create( diff --git a/src/core/lib/security/credentials/tls/grpc_tls_certificate_verifier.h b/src/core/lib/security/credentials/tls/grpc_tls_certificate_verifier.h index e100f852cff59..d675e0708cb58 100644 --- a/src/core/lib/security/credentials/tls/grpc_tls_certificate_verifier.h +++ b/src/core/lib/security/credentials/tls/grpc_tls_certificate_verifier.h @@ -50,9 +50,8 @@ struct grpc_tls_certificate_verifier absl::Status* sync_status) = 0; // Operations that will be performed when a request is cancelled. // This is only needed when in async mode. - // TODO(roth): This needs to take an absl::Status argument so that we - // can pass the cancellation status through to the check_peer callback. - virtual void Cancel(grpc_tls_custom_verification_check_request* request) = 0; + virtual void Cancel(grpc_tls_custom_verification_check_request* request, + const absl::Status& status) = 0; // Compares this grpc_tls_certificate_verifier object with \a other. // If this method returns 0, it means that gRPC can treat the two certificate @@ -97,8 +96,11 @@ class ExternalCertificateVerifier : public grpc_tls_certificate_verifier { std::function callback, absl::Status* sync_status) override; - void Cancel(grpc_tls_custom_verification_check_request* request) override { - external_verifier_->cancel(external_verifier_->user_data, request); + void Cancel(grpc_tls_custom_verification_check_request* request, + const absl::Status& status) override { + external_verifier_->cancel(external_verifier_->user_data, request, + static_cast(status.code()), + std::string(status.message()).c_str()); } UniqueTypeName type() const override; @@ -133,7 +135,8 @@ class NoOpCertificateVerifier : public grpc_tls_certificate_verifier { std::function, absl::Status*) override { return true; // synchronous check }; - void Cancel(grpc_tls_custom_verification_check_request*) override {} + void Cancel(grpc_tls_custom_verification_check_request*, + const absl::Status&) override {} UniqueTypeName type() const override; @@ -152,7 +155,8 @@ class HostNameCertificateVerifier : public grpc_tls_certificate_verifier { bool Verify(grpc_tls_custom_verification_check_request* request, std::function callback, absl::Status* sync_status) override; - void Cancel(grpc_tls_custom_verification_check_request*) override {} + void Cancel(grpc_tls_custom_verification_check_request*, + const absl::Status&) override {} UniqueTypeName type() const override; diff --git a/src/core/lib/security/credentials/xds/xds_credentials.cc b/src/core/lib/security/credentials/xds/xds_credentials.cc index e7e4d11088b3d..d39fc8c3edba2 100644 --- a/src/core/lib/security/credentials/xds/xds_credentials.cc +++ b/src/core/lib/security/credentials/xds/xds_credentials.cc @@ -100,8 +100,8 @@ bool XdsCertificateVerifier::Verify( return true; // synchronous check } -void XdsCertificateVerifier::Cancel( - grpc_tls_custom_verification_check_request*) {} +void XdsCertificateVerifier::Cancel(grpc_tls_custom_verification_check_request*, + const absl::Status&) {} int XdsCertificateVerifier::CompareImpl( const grpc_tls_certificate_verifier* other) const { diff --git a/src/core/lib/security/credentials/xds/xds_credentials.h b/src/core/lib/security/credentials/xds/xds_credentials.h index 7b8ce1531a4ce..bdaf646c0eeb8 100644 --- a/src/core/lib/security/credentials/xds/xds_credentials.h +++ b/src/core/lib/security/credentials/xds/xds_credentials.h @@ -52,7 +52,8 @@ class XdsCertificateVerifier : public grpc_tls_certificate_verifier { bool Verify(grpc_tls_custom_verification_check_request* request, std::function, absl::Status* sync_status) override; - void Cancel(grpc_tls_custom_verification_check_request*) override; + void Cancel(grpc_tls_custom_verification_check_request*, + const absl::Status&) override; UniqueTypeName type() const override; diff --git a/src/core/lib/security/security_connector/tls/tls_security_connector.cc b/src/core/lib/security/security_connector/tls/tls_security_connector.cc index 69fb13c6d465b..6e1a7628f3d3e 100644 --- a/src/core/lib/security/security_connector/tls/tls_security_connector.cc +++ b/src/core/lib/security/security_connector/tls/tls_security_connector.cc @@ -389,7 +389,7 @@ void TlsChannelSecurityConnector::check_peer( } void TlsChannelSecurityConnector::cancel_check_peer( - grpc_closure* on_peer_checked, grpc_error_handle /*error*/) { + grpc_closure* on_peer_checked, grpc_error_handle error) { auto* verifier = options_->certificate_verifier(); if (verifier != nullptr) { grpc_tls_custom_verification_check_request* pending_verifier_request = @@ -406,7 +406,7 @@ void TlsChannelSecurityConnector::cancel_check_peer( } } if (pending_verifier_request != nullptr) { - verifier->Cancel(pending_verifier_request); + verifier->Cancel(pending_verifier_request, error); } } } @@ -668,7 +668,7 @@ void TlsServerSecurityConnector::check_peer( } void TlsServerSecurityConnector::cancel_check_peer( - grpc_closure* on_peer_checked, grpc_error_handle /*error*/) { + grpc_closure* on_peer_checked, grpc_error_handle error) { auto* verifier = options_->certificate_verifier(); if (verifier != nullptr) { grpc_tls_custom_verification_check_request* pending_verifier_request = @@ -685,7 +685,7 @@ void TlsServerSecurityConnector::cancel_check_peer( } } if (pending_verifier_request != nullptr) { - verifier->Cancel(pending_verifier_request); + verifier->Cancel(pending_verifier_request, error); } } } diff --git a/src/cpp/common/tls_certificate_verifier.cc b/src/cpp/common/tls_certificate_verifier.cc index 093062a6202b0..5cc0ca517951b 100644 --- a/src/cpp/common/tls_certificate_verifier.cc +++ b/src/cpp/common/tls_certificate_verifier.cc @@ -141,10 +141,14 @@ bool CertificateVerifier::Verify(TlsCustomVerificationCheckRequest* request, return is_done; } -void CertificateVerifier::Cancel(TlsCustomVerificationCheckRequest* request) { - GPR_ASSERT(request != nullptr); - GPR_ASSERT(request->c_request() != nullptr); - grpc_tls_certificate_verifier_cancel(verifier_, request->c_request()); +void CertificateVerifier::Cancel(TlsCustomVerificationCheckRequest* request, + const absl::Status& status) { + // A copy of the status message has to be made since the API takes const + // char*. + grpc_tls_certificate_verifier_cancel( + verifier_, request->c_request(), + static_cast(status.code()), + std::string(status.message()).c_str()); } void CertificateVerifier::AsyncCheckDone( @@ -229,7 +233,8 @@ int ExternalCertificateVerifier::VerifyInCoreExternalVerifier( } void ExternalCertificateVerifier::CancelInCoreExternalVerifier( - void* user_data, grpc_tls_custom_verification_check_request* request) { + void* user_data, grpc_tls_custom_verification_check_request* request, + grpc_status_code code, const char* error_details) { auto* self = static_cast(user_data); TlsCustomVerificationCheckRequest* cpp_request = nullptr; { @@ -240,7 +245,8 @@ void ExternalCertificateVerifier::CancelInCoreExternalVerifier( } } if (cpp_request != nullptr) { - self->Cancel(cpp_request); + self->Cancel(cpp_request, absl::Status(static_cast(code), + error_details ? error_details : "")); } } diff --git a/test/core/util/tls_utils.h b/test/core/util/tls_utils.h index 24dd6f9f1365f..8b49f6093e786 100644 --- a/test/core/util/tls_utils.h +++ b/test/core/util/tls_utils.h @@ -81,7 +81,8 @@ class SyncExternalVerifier { void* callback_arg, grpc_status_code* sync_status, char** sync_error_details); - static void Cancel(void*, grpc_tls_custom_verification_check_request*) {} + static void Cancel(void*, grpc_tls_custom_verification_check_request*, + grpc_status_code, const char*) {} static void Destruct(void* user_data); @@ -130,7 +131,8 @@ class AsyncExternalVerifier { void* callback_arg, grpc_status_code* sync_status, char** sync_error_details); - static void Cancel(void*, grpc_tls_custom_verification_check_request*) {} + static void Cancel(void*, grpc_tls_custom_verification_check_request*, + grpc_status_code, const char*) {} static void Destruct(void* user_data); @@ -168,7 +170,8 @@ class PeerPropertyExternalVerifier { void* callback_arg, grpc_status_code* sync_status, char** sync_error_details); - static void Cancel(void*, grpc_tls_custom_verification_check_request*) {} + static void Cancel(void*, grpc_tls_custom_verification_check_request*, + grpc_status_code, const char*) {} static void Destruct(void* user_data); diff --git a/test/cpp/security/tls_certificate_verifier_test.cc b/test/cpp/security/tls_certificate_verifier_test.cc index e86df6c725554..e069647ccd29f 100644 --- a/test/cpp/security/tls_certificate_verifier_test.cc +++ b/test/cpp/security/tls_certificate_verifier_test.cc @@ -24,8 +24,6 @@ #include #include -#include "src/cpp/client/secure_credentials.h" -#include "test/core/util/port.h" #include "test/core/util/test_config.h" #include "test/cpp/util/tls_test_utils.h" @@ -90,6 +88,19 @@ TEST(TlsCertificateVerifierTest, AsyncCertificateVerifierFails) { EXPECT_FALSE(verifier->Verify(&cpp_request, callback, &sync_status)); } +TEST(TlsCertificateVerifierTest, AsyncCertificateVerifierCancelled) { + absl::Status status; + grpc_tls_custom_verification_check_request request; + auto verifier = + ExternalCertificateVerifier::Create(&status); + TlsCustomVerificationCheckRequest cpp_request(&request); + grpc::Status sync_status; + EXPECT_FALSE( + verifier->Verify(&cpp_request, /*callback=*/nullptr, &sync_status)); + verifier->Cancel(&cpp_request, absl::CancelledError("cancelled")); + EXPECT_EQ(status, absl::CancelledError("cancelled")); +} + TEST(TlsCertificateVerifierTest, NoOpCertificateVerifierSucceeds) { grpc_tls_custom_verification_check_request request; memset(&request, 0, sizeof(request)); diff --git a/test/cpp/util/tls_test_utils.cc b/test/cpp/util/tls_test_utils.cc index 3a8ab04b5a8da..12cddc55cc3dd 100644 --- a/test/cpp/util/tls_test_utils.cc +++ b/test/cpp/util/tls_test_utils.cc @@ -16,10 +16,7 @@ #include "test/cpp/util/tls_test_utils.h" -#include - #include "src/core/lib/gprpp/thd.h" -#include "test/core/util/port.h" #include "test/core/util/test_config.h" using ::grpc::experimental::TlsCustomVerificationCheckRequest; @@ -43,6 +40,7 @@ AsyncCertificateVerifier::AsyncCertificateVerifier(bool success) : success_(success), thread_("AsyncCertificateVerifierWorkerThread", WorkerThread, this) { thread_.Start(); + thread_started_ = true; } AsyncCertificateVerifier::~AsyncCertificateVerifier() { @@ -51,8 +49,10 @@ AsyncCertificateVerifier::~AsyncCertificateVerifier() { internal::MutexLock lock(&mu_); queue_.push_back(Request{nullptr, nullptr, true}); } - // Wait for thread to exit. - thread_.Join(); + if (thread_started_) { + // Wait for thread to exit. + thread_.Join(); + } } bool AsyncCertificateVerifier::Verify( @@ -63,6 +63,11 @@ bool AsyncCertificateVerifier::Verify( return false; // Asynchronous call } +void AsyncCertificateVerifier::Cancel(TlsCustomVerificationCheckRequest*, + const absl::Status& status) { + *status_from_cancellation_ = status; +} + void AsyncCertificateVerifier::WorkerThread(void* arg) { auto* self = static_cast(arg); while (true) { diff --git a/test/cpp/util/tls_test_utils.h b/test/cpp/util/tls_test_utils.h index 6f7d84acaa92a..d1d5a67def7c1 100644 --- a/test/cpp/util/tls_test_utils.h +++ b/test/cpp/util/tls_test_utils.h @@ -39,8 +39,8 @@ class SyncCertificateVerifier std::function callback, grpc::Status* sync_status) override; - void Cancel(grpc::experimental::TlsCustomVerificationCheckRequest*) override { - } + void Cancel(grpc::experimental::TlsCustomVerificationCheckRequest*, + const absl::Status&) override {} private: bool success_ = false; @@ -51,14 +51,18 @@ class AsyncCertificateVerifier public: explicit AsyncCertificateVerifier(bool success); + // This is used to test Cancel() and the worker thread will not start. + explicit AsyncCertificateVerifier(absl::Status* status_from_cancellation) + : status_from_cancellation_(status_from_cancellation) {} + ~AsyncCertificateVerifier() override; bool Verify(grpc::experimental::TlsCustomVerificationCheckRequest* request, std::function callback, grpc::Status* sync_status) override; - void Cancel(grpc::experimental::TlsCustomVerificationCheckRequest*) override { - } + void Cancel(grpc::experimental::TlsCustomVerificationCheckRequest*, + const absl::Status&) override; private: // A request to pass to the worker thread. @@ -72,8 +76,11 @@ class AsyncCertificateVerifier bool success_ = false; grpc_core::Thread thread_; + bool thread_started_ = false; grpc::internal::Mutex mu_; std::deque queue_ ABSL_GUARDED_BY(mu_); + // Not owned. Stores the status given from the latest cancellation. + absl::Status* status_from_cancellation_; }; class VerifiedRootCertSubjectVerifier @@ -88,8 +95,8 @@ class VerifiedRootCertSubjectVerifier std::function callback, grpc::Status* sync_status) override; - void Cancel(grpc::experimental::TlsCustomVerificationCheckRequest*) override { - } + void Cancel(grpc::experimental::TlsCustomVerificationCheckRequest*, + const absl::Status&) override {} private: std::string expected_subject_;