security handshaker: include security connector type in error message (#30046)

* weighted_target and RLS: delegate to child picker on error

* security handshaker: include security connector type in error message

* update test

* fix sanity

* fix crash

* Automated change: Fix sanity tests

Co-authored-by: markdroth <markdroth@users.noreply.github.com>

src/core/lib/security/security_connector/security_connector.cc

@@ -34,14 +34,6 @@
 grpc_core::DebugOnlyTraceFlag grpc_trace_security_connector_refcount(
     false, "security_connector_refcount");
 
-grpc_server_security_connector::grpc_server_security_connector(
-    absl::string_view url_scheme,
-    grpc_core::RefCountedPtr<grpc_server_credentials> server_creds)
-    : grpc_security_connector(url_scheme),
-      server_creds_(std::move(server_creds)) {}
-
-grpc_server_security_connector::~grpc_server_security_connector() = default;
-
 grpc_channel_security_connector::grpc_channel_security_connector(
     absl::string_view url_scheme,
     grpc_core::RefCountedPtr<grpc_channel_credentials> channel_creds,
@@ -50,8 +42,6 @@ grpc_channel_security_connector::grpc_channel_security_connector(
       channel_creds_(std::move(channel_creds)),
       request_metadata_creds_(std::move(request_metadata_creds)) {}
 
-grpc_channel_security_connector::~grpc_channel_security_connector() {}
-
 int grpc_channel_security_connector::channel_security_connector_cmp(
     const grpc_channel_security_connector* other) const {
   const grpc_channel_security_connector* other_sc =
@@ -64,6 +54,16 @@ int grpc_channel_security_connector::channel_security_connector_cmp(
                                  other_sc->request_metadata_creds());
 }
 
+grpc_core::UniqueTypeName grpc_channel_security_connector::type() const {
+  return channel_creds_->type();
+}
+
+grpc_server_security_connector::grpc_server_security_connector(
+    absl::string_view url_scheme,
+    grpc_core::RefCountedPtr<grpc_server_credentials> server_creds)
+    : grpc_security_connector(url_scheme),
+      server_creds_(std::move(server_creds)) {}
+
 int grpc_server_security_connector::server_security_connector_cmp(
     const grpc_server_security_connector* other) const {
   const grpc_server_security_connector* other_sc =
@@ -73,6 +73,10 @@ int grpc_server_security_connector::server_security_connector_cmp(
   return grpc_core::QsortCompare(server_creds(), other_sc->server_creds());
 }
 
+grpc_core::UniqueTypeName grpc_server_security_connector::type() const {
+  return server_creds_->type();
+}
+
 static void connector_arg_destroy(void* p) {
   if (p == nullptr) return;
   static_cast<grpc_security_connector*>(p)->Unref(DEBUG_LOCATION,

src/core/lib/security/security_connector/security_connector.h

@@ -33,6 +33,7 @@
 #include "src/core/lib/debug/trace.h"
 #include "src/core/lib/gprpp/ref_counted.h"
 #include "src/core/lib/gprpp/ref_counted_ptr.h"
+#include "src/core/lib/gprpp/unique_type_name.h"
 #include "src/core/lib/iomgr/closure.h"
 #include "src/core/lib/iomgr/endpoint.h"
 #include "src/core/lib/iomgr/error.h"
@@ -66,7 +67,6 @@ class grpc_security_connector
                 ? "security_connector_refcount"
                 : nullptr),
         url_scheme_(url_scheme) {}
-  ~grpc_security_connector() override = default;
 
   static absl::string_view ChannelArgName() {
     return GRPC_ARG_SECURITY_CONNECTOR;
@@ -94,6 +94,8 @@ class grpc_security_connector
 
   absl::string_view url_scheme() const { return url_scheme_; }
 
+  virtual grpc_core::UniqueTypeName type() const = 0;
+
  private:
   absl::string_view url_scheme_;
 };
@@ -119,7 +121,6 @@ class grpc_channel_security_connector : public grpc_security_connector {
       absl::string_view url_scheme,
       grpc_core::RefCountedPtr<grpc_channel_credentials> channel_creds,
       grpc_core::RefCountedPtr<grpc_call_credentials> request_metadata_creds);
-  ~grpc_channel_security_connector() override;
 
   /// Checks that the host that will be set for a call is acceptable.
   /// Returns ok if the host is acceptable, otherwise returns an error.
@@ -144,6 +145,8 @@ class grpc_channel_security_connector : public grpc_security_connector {
     return request_metadata_creds_.get();
   }
 
+  grpc_core::UniqueTypeName type() const override;
+
  protected:
   // Helper methods to be used in subclasses.
   int channel_security_connector_cmp(
@@ -170,7 +173,6 @@ class grpc_server_security_connector : public grpc_security_connector {
   grpc_server_security_connector(
       absl::string_view url_scheme,
       grpc_core::RefCountedPtr<grpc_server_credentials> server_creds);
-  ~grpc_server_security_connector() override;
 
   virtual void add_handshakers(const grpc_channel_args* args,
                                grpc_pollset_set* interested_parties,
@@ -183,6 +185,8 @@ class grpc_server_security_connector : public grpc_security_connector {
     return server_creds_.get();
   }
 
+  grpc_core::UniqueTypeName type() const override;
+
  protected:
   // Helper methods to be used in subclasses.
   int server_security_connector_cmp(

src/core/lib/security/transport/security_handshaker.cc

@@ -31,6 +31,8 @@
 #include "absl/base/attributes.h"
 #include "absl/container/inlined_vector.h"
 #include "absl/memory/memory.h"
+#include "absl/strings/str_cat.h"
+#include "absl/strings/string_view.h"
 #include "absl/types/optional.h"
 
 #include <grpc/grpc_security.h>
@@ -46,6 +48,7 @@
 #include "src/core/lib/gprpp/debug_location.h"
 #include "src/core/lib/gprpp/ref_counted_ptr.h"
 #include "src/core/lib/gprpp/sync.h"
+#include "src/core/lib/gprpp/unique_type_name.h"
 #include "src/core/lib/iomgr/closure.h"
 #include "src/core/lib/iomgr/endpoint.h"
 #include "src/core/lib/iomgr/error.h"
@@ -395,8 +398,16 @@ grpc_error_handle SecurityHandshaker::OnHandshakeNextDoneLocked(
     return error;
   }
   if (result != TSI_OK) {
+    auto* security_connector =
+        grpc_security_connector_find_in_args(args_->args);
+    absl::string_view connector_type = "<unknown>";
+    if (security_connector != nullptr) {
+      connector_type = security_connector->type().name();
+    }
     return grpc_set_tsi_error_result(
-        GRPC_ERROR_CREATE_FROM_STATIC_STRING("Handshake failed"), result);
+        GRPC_ERROR_CREATE_FROM_CPP_STRING(
+            absl::StrCat(connector_type, " handshake failed")),
+        result);
   }
   // Update handshaker result.
   if (handshaker_result != nullptr) {

test/cpp/end2end/xds/xds_end2end_test.cc

@@ -383,7 +383,7 @@ class XdsSecurityTest : public XdsEnd2endTest {
                     "connections to all backends failing; last error: "
                     "(UNKNOWN: Failed to connect to remote host: Connection "
                     "(refused|reset by peer)|UNAVAILABLE: Failed to connect "
-                    "to remote host: FD shutdown|UNKNOWN: Handshake failed|"
+                    "to remote host: FD shutdown|UNKNOWN: Tls handshake failed|"
                     "UNAVAILABLE: Socket closed)"));
           }
         });