From 0521a49a6a72cc83892163db8387e9c8caeb09c4 Mon Sep 17 00:00:00 2001 From: Bram Date: Wed, 10 Jun 2026 19:57:10 +0200 Subject: [PATCH] Add support for TLS 1.3 (#564) * Add support for TLS 1.3 This PR allows TLS 1.3, by removing the MaxVersion in the client config. This would silently swallow errors, so e.g. a client without cert dialing a server that requires client certs would lead to an error which gets ignored, leading to retries until timeout. In this PR, we wrap the connection and if an error occurs we send it to the existing `result` channel. I think this matches @jhump's comment in https://github.com/fullstorydev/grpcurl/issues/387#issuecomment-1517098394 **Testing** ```console # Start the test server (in another tab) go run ./internal/testing/cmd/testserver \ -cert internal/testing/tls/server.crt \ -key internal/testing/tls/server.key \ -cacert internal/testing/tls/ca.crt \ -requirecert -p 9999 # Old behavior $ grpcurl -cacert internal/testing/tls/ca.crt \ localhost:9999 list Failed to dial target host "localhost:9999": context deadline exceeded # New behavior $ go run ./cmd/grpcurl -cacert internal/testing/tls/ca.crt \ localhost:9999 list Failed to dial target host "localhost:9999": remote error: tls: certificate required exit status 1 ``` The old behavior is to hang until we hit the deadline. The new behavior is to return immediately with an error. Fixes #563 * Implement review comments Removing unnecessary sync.Once, as the channel will already drop repeated calls. --- grpcurl.go | 30 +++++++++++++-- tls_settings_test.go | 88 +++++++++++++++++++++++++++++++++++++++----- 2 files changed, 104 insertions(+), 14 deletions(-) diff --git a/grpcurl.go b/grpcurl.go index 777e3c1..089fc5e 100644 --- a/grpcurl.go +++ b/grpcurl.go @@ -568,9 +568,6 @@ func ClientTLSConfig(insecureSkipVerify bool, cacertFile, clientCertFile, client // client certs. The serverCertFile and serverKeyFile must both not be blank. func ServerTransportCredentials(cacertFile, serverCertFile, serverKeyFile string, requireClientCerts bool) (credentials.TransportCredentials, error) { var tlsConf tls.Config - // TODO(jh): Remove this line once https://github.com/golang/go/issues/28779 is fixed - // in Go tip. Until then, the recently merged TLS 1.3 support breaks the TLS tests. - tlsConf.MaxVersion = tls.VersionTLS12 // Load the server certificates from disk certificate, err := tls.LoadX509KeyPair(serverCertFile, serverKeyFile) @@ -734,6 +731,31 @@ func (c *errSignalingCreds) ClientHandshake(ctx context.Context, addr string, ra conn, auth, err := c.TransportCredentials.ClientHandshake(ctx, addr, rawConn) if err != nil { c.writeResult(err) + return conn, auth, err } - return conn, auth, err + // Wrap TLS connections to capture post-handshake errors. With TLS 1.3, + // client certificate rejection by the server happens after the client + // considers the handshake complete. The server's TLS alert surfaces on the + // first Read from the connection. Only TLS connections need this (plaintext + // 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 +// errors. +type errSignalingConn struct { + net.Conn + writeResult func(res interface{}) +} + +func (c *errSignalingConn) Read(b []byte) (int, error) { + n, err := c.Conn.Read(b) + if err != nil { + c.writeResult(err) + } + return n, err } diff --git a/tls_settings_test.go b/tls_settings_test.go index ad8958f..9c569dc 100644 --- a/tls_settings_test.go +++ b/tls_settings_test.go @@ -2,6 +2,7 @@ package grpcurl_test import ( "context" + "crypto/tls" "fmt" "net" "strings" @@ -10,6 +11,7 @@ import ( "google.golang.org/grpc" "google.golang.org/grpc/credentials" + "google.golang.org/grpc/peer" . "github.com/fullstorydev/grpcurl" grpcurl_testing "github.com/fullstorydev/grpcurl/internal/testing" @@ -101,6 +103,68 @@ func TestRequireClientCertTLS(t *testing.T) { simpleTest(t, e.cc) } +func TestTLS12(t *testing.T) { + serverCreds, err := ServerTransportCredentials("", "internal/testing/tls/server.crt", "internal/testing/tls/server.key", false) + if err != nil { + t.Fatalf("failed to create server creds: %v", err) + } + tlsConf, err := ClientTLSConfig(false, "internal/testing/tls/ca.crt", "", "") + if err != nil { + t.Fatalf("failed to create client TLS config: %v", err) + } + tlsConf.MaxVersion = tls.VersionTLS12 + + e, err := createTestServerAndClient(serverCreds, credentials.NewTLS(tlsConf)) + if err != nil { + t.Fatalf("failed to setup server and client: %v", err) + } + defer e.Close() + + tlsVersion := negotiatedTLSVersion(t, e.cc) + if tlsVersion != tls.VersionTLS12 { + t.Errorf("expected TLS 1.2, got 0x%04x", tlsVersion) + } +} + +func TestTLS13(t *testing.T) { + serverCreds, err := ServerTransportCredentials("", "internal/testing/tls/server.crt", "internal/testing/tls/server.key", false) + if err != nil { + t.Fatalf("failed to create server creds: %v", err) + } + clientCreds, err := ClientTransportCredentials(false, "internal/testing/tls/ca.crt", "", "") + if err != nil { + t.Fatalf("failed to create client creds: %v", err) + } + + e, err := createTestServerAndClient(serverCreds, clientCreds) + if err != nil { + t.Fatalf("failed to setup server and client: %v", err) + } + defer e.Close() + + tlsVersion := negotiatedTLSVersion(t, e.cc) + if tlsVersion != tls.VersionTLS13 { + t.Errorf("expected TLS 1.3, got 0x%04x", tlsVersion) + } +} + +func negotiatedTLSVersion(t *testing.T, cc *grpc.ClientConn) uint16 { + t.Helper() + cl := grpcurl_testing.NewTestServiceClient(cc) + ctx, cancel := context.WithTimeout(context.Background(), 3*time.Second) + defer cancel() + var p peer.Peer + _, err := cl.UnaryCall(ctx, &grpcurl_testing.SimpleRequest{}, grpc.WaitForReady(true), grpc.Peer(&p)) + if err != nil { + t.Fatalf("RPC failed: %v", err) + } + tlsInfo, ok := p.AuthInfo.(credentials.TLSInfo) + if !ok { + t.Fatalf("expected TLS auth info, got %T", p.AuthInfo) + } + return tlsInfo.State.Version +} + func TestBrokenTLS_ClientPlainText(t *testing.T) { serverCreds, err := ServerTransportCredentials("", "internal/testing/tls/server.crt", "internal/testing/tls/server.key", false) if err != nil { @@ -253,12 +317,14 @@ func TestBrokenTLS_ClientNotTrusted(t *testing.T) { e.Close() t.Fatal("expecting TLS failure setting up server and client") } - // Check for either the old error (Go <=1.24) or the new one (Go 1.25+) - // Go 1.24: "bad certificate" - // Go 1.25: "handshake failure" + // The exact TLS alert varies by Go version and TLS version negotiated: + // - TLS 1.2: "bad certificate" (Go <=1.24) or "handshake failure" (Go 1.25+) + // - TLS 1.3: "certificate required" (server rejects after handshake) errMsg := err.Error() - if !strings.Contains(errMsg, "bad certificate") && !strings.Contains(errMsg, "handshake failure") { - t.Fatalf("expecting a specific TLS certificate or handshake error, got: %v", err) + if !strings.Contains(errMsg, "bad certificate") && + !strings.Contains(errMsg, "handshake failure") && + !strings.Contains(errMsg, "certificate required") { + t.Fatalf("expecting a TLS certificate error, got: %v", err) } } @@ -297,12 +363,14 @@ func TestBrokenTLS_RequireClientCertButNonePresented(t *testing.T) { e.Close() t.Fatal("expecting TLS failure setting up server and client") } - // Check for either the old error (Go <=1.24) or the new one (Go 1.25+) - // Go 1.24: "bad certificate" - // Go 1.25: "handshake failure" + // The exact TLS alert varies by Go version and TLS version negotiated: + // - TLS 1.2: "bad certificate" (Go <=1.24) or "handshake failure" (Go 1.25+) + // - TLS 1.3: "certificate required" (server rejects after handshake) errMsg := err.Error() - if !strings.Contains(errMsg, "bad certificate") && !strings.Contains(errMsg, "handshake failure") { - t.Fatalf("expecting a specific TLS certificate or handshake error, got: %v", err) + if !strings.Contains(errMsg, "bad certificate") && + !strings.Contains(errMsg, "handshake failure") && + !strings.Contains(errMsg, "certificate required") { + t.Fatalf("expecting a TLS certificate error, got: %v", err) } }