add status information to cancel

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

include/grpcpp/security/tls_certificate_verifier.h

@@ -23,6 +23,8 @@
 #include <utility>
 #include <vector>
 
+#include "absl/status/status.h"
+
 #include <grpc/grpc_security_constants.h>
 #include <grpc/status.h>
 #include <grpc/support/log.h>
@@ -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);
 

src/core/lib/security/credentials/tls/grpc_tls_certificate_verifier.cc

@@ -23,13 +23,13 @@
 #include <string>
 #include <utility>
 
+#include "absl/status/status.h"
 #include "absl/strings/string_view.h"
 
 #include <grpc/support/alloc.h>
 #include <grpc/support/log.h>
 #include <grpc/support/string_util.h>
 
-#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<absl::StatusCode>(code),
+                                         error_details ? error_details : ""));
 }
 
 grpc_tls_certificate_verifier* grpc_tls_certificate_verifier_external_create(

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<void(absl::Status)> 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<grpc_status_code>(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<void(absl::Status)>, 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<void(absl::Status)> 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;
 

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 {

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<void(absl::Status)>,
               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;
 

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);
     }
   }
 }

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<grpc_status_code>(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<ExternalCertificateVerifier*>(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<absl::StatusCode>(code),
+                                           error_details ? error_details : ""));
   }
 }
 

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);
 

test/cpp/security/tls_certificate_verifier_test.cc

@@ -24,8 +24,6 @@
 #include <grpcpp/security/server_credentials.h>
 #include <grpcpp/security/tls_credentials_options.h>
 
-#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<AsyncCertificateVerifier>(&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));

test/cpp/util/tls_test_utils.cc

@@ -16,10 +16,7 @@
 
 #include "test/cpp/util/tls_test_utils.h"
 
-#include <memory>
-
 #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<AsyncCertificateVerifier*>(arg);
   while (true) {

test/cpp/util/tls_test_utils.h

@@ -39,8 +39,8 @@ class SyncCertificateVerifier
               std::function<void(grpc::Status)> 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<void(grpc::Status)> 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<Request> 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<void(grpc::Status)> 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_;