fix xds credentials being silently ignored (#566)

* fix xds credentials being silently ignored

Fixes #565

* Apply suggestion from @dragonsinth

Co-authored-by: Scott Blum <dragonsinth@gmail.com>

---------

Co-authored-by: Scott Blum <dragonsinth@gmail.com>

grpcurl.go

@@ -20,7 +20,6 @@ import (
 	"slices"
 	"sort"
 	"strings"
-	"sync"
 
 	"github.com/golang/protobuf/proto"   //lint:ignore SA1019 we have to import these because some of their types appear in exported API
 	"github.com/jhump/protoreflect/desc" //lint:ignore SA1019 same as above
@@ -615,8 +614,8 @@ func BlockingDial(ctx context.Context, network, address string, creds credential
 	}
 
 	var err error
-	if strings.HasPrefix(address, "xds:///") {
+	if strings.HasPrefix(address, "xds://") {
-		// The xds:/// prefix is used to signal to the gRPC client to use an xDS server to resolve the
+		// The xds:// prefix is used to signal to the gRPC client to use an xDS server to resolve the
 		// target. The relevant credentials will be automatically pulled from the GRPC_XDS_BOOTSTRAP or
 		// GRPC_XDS_BOOTSTRAP_CONFIG env vars.
 		creds, err = xdsCredentials.NewClientCredentials(xdsCredentials.ClientOptions{FallbackCreds: creds})
@@ -734,32 +733,38 @@ func (c *errSignalingCreds) ClientHandshake(ctx context.Context, addr string, ra
 		c.writeResult(err)
 		return conn, auth, err
 	}
-	// Wrap TLS connections to capture post-handshake errors. With TLS 1.3,
+	// Wrap the connection to capture post-handshake errors, e.g.:
-	// client certificate rejection by the server happens after the client
+	// - TLS 1.3 client cert rejection (server sends alert after handshake)
-	// considers the handshake complete. The server's TLS alert surfaces on the
+	// - Plaintext client to TLS server (server closes conn immediately)
-	// first Read from the connection. Only TLS connections need this (plaintext
+	return &errSignalingConn{Conn: conn, writeResult: c.writeResult}, auth, nil
-	// connections don't have post-handshake alerts).
-	if _, isTLS := auth.(credentials.TLSInfo); isTLS {
-		conn = &errSignalingConn{Conn: conn, writeResult: c.writeResult}
-	}
-	return conn, auth, nil
 }
 
 // errSignalingConn wraps a net.Conn to capture the first read error and
-// report it via writeResult. This allows BlockingDial to surface post-handshake
+// report it via writeResult.
-// errors.
 type errSignalingConn struct {
 	net.Conn
 	writeResult func(res interface{})
-	once        sync.Once
 }
 
 func (c *errSignalingConn) Read(b []byte) (int, error) {
 	n, err := c.Conn.Read(b)
 	if err != nil {
-		c.once.Do(func() {
 		c.writeResult(err)
-		})
 	}
 	return n, err
 }
+
+// UsesXDS forwards the optional UsesXDS marker of the wrapped credentials. The
+// xDS credentials returned for "xds://" targets implement this method, and
+// grpc-go's cds balancer relies on a type assertion for it to decide whether to
+// apply the security configuration (e.g. UpstreamTlsContext) delivered by the
+// management server. Because errSignalingCreds embeds the TransportCredentials
+// interface, that extra method is not promoted automatically, so we forward it
+// explicitly. Without this, xDS-supplied mTLS is silently ignored and the
+// connection falls back to the plain credentials.
+func (c *errSignalingCreds) UsesXDS() bool {
+	if x, ok := c.TransportCredentials.(interface{ UsesXDS() bool }); ok {
+		return x.UsesXDS()
+	}
+	return false
+}

tls_settings_test.go

@@ -171,58 +171,17 @@ func TestBrokenTLS_ClientPlainText(t *testing.T) {
 		t.Fatalf("failed to create server creds: %v", err)
 	}
 
-	// client connection (usually) succeeds since client is not waiting for TLS handshake
+	// Plaintext client to TLS server: the server expects a TLS handshake,
-	// (we try several times, but if we never get a connection and the error message is
+	// gets an HTTP/2 preface instead, and closes the connection.
-	// a known/expected possibility, we'll just bail)
+	e, err := createTestServerAndClient(serverCreds, nil)
-	var e testEnv
-	failCount := 0
-	for {
-		e, err = createTestServerAndClient(serverCreds, nil)
 	if err == nil {
-			// success!
+		e.Close()
-			defer e.Close()
+		t.Fatal("expecting failure when connecting plaintext to TLS server")
-			break
 	}
-
+	if !strings.Contains(err.Error(), "EOF") &&
-		if strings.Contains(err.Error(), "deadline exceeded") ||
-			strings.Contains(err.Error(), "use of closed network connection") {
-			// It is possible that the connection never becomes healthy:
-			//   1) grpc connects successfully
-			//   2) grpc client tries to send HTTP/2 preface and settings frame
-			//   3) server, expecting handshake, closes the connection
-			//   4) in the client, the write fails, so the connection never
-			//      becomes ready
-			// The client will attempt to reconnect on transient errors, so
-			// may eventually bump into the connect time limit. This used to
-			// result in a "deadline exceeded" error, but more recent versions
-			// of the grpc library report any underlying I/O error instead, so
-			// we also check for "use of closed network connection".
-			failCount++
-			if failCount > 5 {
-				return // bail...
-			}
-			// we'll try again
-
-		} else {
-			// some other error occurred, so we'll consider that a test failure
-			t.Fatalf("failed to setup server and client: %v", err)
-		}
-	}
-
-	// but request fails because server closes connection upon seeing request
-	// bytes that are not a TLS handshake
-	cl := grpcurl_testing.NewTestServiceClient(e.cc)
-	_, err = cl.UnaryCall(context.Background(), &grpcurl_testing.SimpleRequest{})
-	if err == nil {
-		t.Fatal("expecting failure")
-	}
-	// various errors possible when server closes connection
-	if !strings.Contains(err.Error(), "transport is closing") &&
-		!strings.Contains(err.Error(), "connection is unavailable") &&
 		!strings.Contains(err.Error(), "use of closed network connection") &&
-		!strings.Contains(err.Error(), "all SubConns are in TransientFailure") {
+		!strings.Contains(err.Error(), "connection reset by peer") {
-
+		t.Fatalf("expecting connection closed error, got: %v", err)
-		t.Fatalf("expecting transport failure, got: %v", err)
 	}
 }