8 Commits

Author SHA1 Message Date
Scott Blum 5f2d0c2dfc remove old SA1019 lint:ignore (#574) 2026-07-27 15:38:17 -04:00
Bram 6be36ba632 Fix path traversal attack in -proto-out-dir (#573)
* Fix path traversal in `-proto-out-dir`

A malicious file descriptor name (e.g. from an untrusted protoset or
server reflection) could escape the output directory through ../
sequences, writing attacker-controlled content to arbitrary paths.

Confine all writes to an `os.Root` opened on the output directory.

* assert

* refactor

---------

Co-authored-by: Scott Blum <dragonsinth@gmail.com>
2026-07-27 15:30:28 -04:00
Bram 353acfef45 Fix flaky TLS test by waiting for dial outcome before closing connection (#572)
* Fix flaky TLS test by waiting for dial outcome before closing connection

PRs #564 and #567 introduced errSignalingConn to surface TLS errors
(especially TLS 1.3 post-handshake alerts) to BlockingDial callers.
This caused a flaky test failure (reported in: https://github.com/fullstorydev/grpcurl/pull/564#issuecomment-4916344087):

```
TestBrokenTLS_RequireClientCertButNonePresented:
expecting a TLS certificate error, got: read tcp ...: use of closed
network connection
```

The race works as follows:

1. With TLS 1.3, the server rejects the connection *after* the handshake
   completes (e.g. missing client cert) by sending a TLS alert, then
   closing the connection.
2. Simultaneously, the client's Write fails (broken pipe) because the server already closed its side.
3. grpc-go's `NewHTTP2Client` returns the Write error and calls `t.Close()`
   to tear down the transport, which calls `errSignalingConn.Close()` ->
   `c.Conn.Close()`, closing the connection.
4. The reader goroutine's pending Read (which should have returned the
   TLS alert (e.g. "certificate required")) instead returns
   `use of closed network connection` because the connection was
   closed underneath it.
5. `writeResult` surfaces the "closed network connection" error to the caller instead of the meaningful TLS alert.

The fix: delay Close on the errSignalingConn until the outcome of the
dial is known, with a 50ms timeout. This gives the reader a chance to
surface the TLS alert.

Demonstrating this by stressing the test:

**Without the fix**

```
alasia :: github/fullstorydev/grpcurl ‹master› » go test -c -o /tmp/grpcurl.test . && GOMAXPROCS=2 $HOME/go/bin/stress -p 16 -count 5000 -ignore 'assign requested address|context deadline exceeded' /tmp/grpcurl.test -test.run 'TestBrokenTLS'

/var/folders/7l/0tdjxwg912j9gqh446yk3ll40000gn/T/go-stress-20260724T113423-1710148011
--- FAIL: TestBrokenTLS_RequireClientCertButNonePresented (0.00s)
    tls_settings_test.go:332: expecting a TLS certificate error, got: read tcp 127.0.0.1:52850->127.0.0.1:52848: use of closed network connection
FAIL

ERROR: exit status 1

[...]

5s: 1580 runs so far, 5 failures (0.32%), 13 active
```

**With the fix**

```
alasia :: github/fullstorydev/grpcurl ‹surface-post-dial-conn-errors› » go test -c -o /tmp/grpcurl.test . && GOMAXPROCS=2 $HOME/go/bin/stress -p 16 -count 5000 -ignore 'assign requested address|context deadline exceeded' /tmp/grpcurl.test -test.run 'TestBrokenTLS'
5s: 1594 runs so far, 0 failures, 16 active
10s: 3386 runs so far, 0 failures, 16 active
15s: 4999 runs so far, 0 failures, 1 active
15s: 5000 runs total, 0 failures
```

* Implement review comment
2026-07-24 15:41:33 -04:00
max 04bf23be33 docs: fix typo "foor" -> "foo" in README example (#571)
The JSON example in the stdin section had a typo: "foor" instead of
"foo". Surrounding examples in the same section use "foo" consistently.

Co-authored-by: maxtaran2010 <ocotifuzo727@gmail.com>
2026-07-08 17:41:29 +00:00
dependabot[bot] 1a48043e8a Bump golang.org/x/net in the go_modules group across 1 directory (#569)
Bumps the go_modules group with 1 update in the / directory: [golang.org/x/net](https://github.com/golang/net).


Updates `golang.org/x/net` from 0.49.0 to 0.55.0
- [Commits](https://github.com/golang/net/compare/v0.49.0...v0.55.0)

---
updated-dependencies:
- dependency-name: golang.org/x/net
  dependency-version: 0.55.0
  dependency-type: indirect
  dependency-group: go_modules
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
2026-07-05 09:33:41 -04:00
Erik Engberg afea969b8a 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>
2026-06-15 11:10:27 -04:00
Bram 4ea1554ec7 Fast-fail on connection errors (#567)
Avoid hanging on connection errors; fail fast by propagating connection
errors.

This also speeds up tests a lot (from >20s to about 3s), since the tests
included some connection timeouts.

This expands on https://github.com/fullstorydev/grpcurl/pull/564 ; it
does the same for plaintext connections.

Fixes #387
2026-06-13 11:55:47 -04:00
Bram 0521a49a6a 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.
2026-06-10 13:57:10 -04:00
13 changed files with 174 additions and 116 deletions
+1
View File
@@ -1,4 +1,5 @@
dist/ dist/
.claude/
.idea/ .idea/
VERSION VERSION
.tmp/ .tmp/
+1 -1
View File
@@ -146,7 +146,7 @@ grpcurl -d @ grpc.server.com:443 my.custom.server.Service/Method <<EOM
{ {
"id": 1234, "id": 1234,
"tags": [ "tags": [
"foor", "foo",
"bar" "bar"
] ]
} }
+1 -1
View File
@@ -15,7 +15,7 @@ import (
"strings" "strings"
"time" "time"
"github.com/jhump/protoreflect/desc" //lint:ignore SA1019 required to use APIs in other grpcurl package "github.com/jhump/protoreflect/desc"
"github.com/jhump/protoreflect/grpcreflect" "github.com/jhump/protoreflect/grpcreflect"
"google.golang.org/grpc" "google.golang.org/grpc"
"google.golang.org/grpc/codes" "google.golang.org/grpc/codes"
+24 -11
View File
@@ -9,11 +9,11 @@ import (
"path/filepath" "path/filepath"
"sync" "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/golang/protobuf/proto"
"github.com/jhump/protoreflect/desc" //lint:ignore SA1019 same as above "github.com/jhump/protoreflect/desc"
"github.com/jhump/protoreflect/desc/protoparse" //lint:ignore SA1019 same as above "github.com/jhump/protoreflect/desc/protoparse"
"github.com/jhump/protoreflect/desc/protoprint" "github.com/jhump/protoreflect/desc/protoprint"
"github.com/jhump/protoreflect/dynamic" //lint:ignore SA1019 same as above "github.com/jhump/protoreflect/dynamic"
"github.com/jhump/protoreflect/grpcreflect" "github.com/jhump/protoreflect/grpcreflect"
"google.golang.org/grpc/codes" "google.golang.org/grpc/codes"
"google.golang.org/grpc/status" "google.golang.org/grpc/status"
@@ -309,24 +309,37 @@ func WriteProtoFiles(outProtoDirPath string, descSource DescriptorSource, symbol
for _, filename := range filenames { for _, filename := range filenames {
allFileDescriptors = addFilesToFileDescriptorList(allFileDescriptors, expandedFiles, fds[filename]) allFileDescriptors = addFilesToFileDescriptorList(allFileDescriptors, expandedFiles, fds[filename])
} }
return writeProtoFiles(outProtoDirPath, allFileDescriptors)
}
func writeProtoFiles(outProtoDirPath string, allFileDescriptors []*desc.FileDescriptor) error {
if err := os.MkdirAll(outProtoDirPath, 0755); err != nil {
return fmt.Errorf("failed to create output directory %q: %w", outProtoDirPath, err)
}
root, err := os.OpenRoot(outProtoDirPath)
if err != nil {
return fmt.Errorf("failed to open output directory %q: %w", outProtoDirPath, err)
}
defer root.Close()
pr := protoprint.Printer{} pr := protoprint.Printer{}
// now we can serialize to files // now we can serialize to files
for i := range allFileDescriptors { for i := range allFileDescriptors {
if err := writeProtoFile(outProtoDirPath, allFileDescriptors[i], &pr); err != nil { if err := writeProtoFile(root, allFileDescriptors[i], &pr); err != nil {
return err return err
} }
} }
return nil return nil
} }
func writeProtoFile(outProtoDirPath string, fd *desc.FileDescriptor, pr *protoprint.Printer) error { func writeProtoFile(root *os.Root, fd *desc.FileDescriptor, pr *protoprint.Printer) error {
outFile := filepath.Join(outProtoDirPath, fd.GetFullyQualifiedName()) // root confines all path operations to the output directory, so a
outDir := filepath.Dir(outFile) // malicious file name (e.g. "../escape.proto") cannot write outside it
if err := os.MkdirAll(outDir, 0777); err != nil { outFile := fd.GetFullyQualifiedName()
return fmt.Errorf("failed to create directory %q: %w", outDir, err) if err := root.MkdirAll(filepath.Dir(outFile), 0755); err != nil {
return fmt.Errorf("failed to create directory for %q: %w", outFile, err)
} }
f, err := os.Create(outFile) f, err := root.Create(outFile)
if err != nil { if err != nil {
return fmt.Errorf("failed to create proto file %q: %w", outFile, err) return fmt.Errorf("failed to create proto file %q: %w", outFile, err)
} }
+47 -1
View File
@@ -3,9 +3,11 @@ package grpcurl
import ( import (
"bytes" "bytes"
"os" "os"
"path/filepath"
"testing" "testing"
"github.com/golang/protobuf/proto" //lint:ignore SA1019 we have to import this because it appears in exported API "github.com/golang/protobuf/proto"
"github.com/jhump/protoreflect/desc"
"google.golang.org/protobuf/types/descriptorpb" "google.golang.org/protobuf/types/descriptorpb"
) )
@@ -60,3 +62,47 @@ func checkWriteProtoset(t *testing.T, descSrc DescriptorSource, protoset *descri
t.Fatalf("written protoset not equal to input:\nExpecting: %s\nActual: %s", protoset, &result) t.Fatalf("written protoset not equal to input:\nExpecting: %s\nActual: %s", protoset, &result)
} }
} }
func writeProtoFileForTest(t *testing.T, dir, fdName string) error {
t.Helper()
fd, err := desc.CreateFileDescriptor(&descriptorpb.FileDescriptorProto{
Name: proto.String(fdName),
Syntax: proto.String("proto3"),
})
if err != nil {
t.Fatalf("failed to create file descriptor: %v", err)
}
return writeProtoFiles(dir, []*desc.FileDescriptor{fd})
}
func TestWriteProtoFile_NormalPath(t *testing.T) {
dir := t.TempDir()
if err := writeProtoFileForTest(t, dir, "foo/bar.proto"); err != nil {
t.Fatalf("unexpected error: %v", err)
}
if _, err := os.Stat(filepath.Join(dir, "foo", "bar.proto")); err != nil {
t.Fatalf("expected output file not created: %v", err)
}
}
func TestWriteProtoFile_RejectsPathTraversal(t *testing.T) {
dir := t.TempDir()
if err := writeProtoFileForTest(t, dir, "../escape.proto"); err == nil {
t.Fatal("expected error for path-traversing descriptor name, got nil")
}
escapePath := filepath.Join(dir, "..", "escape.proto")
if _, err := os.Stat(escapePath); err == nil {
t.Fatalf("file was created outside output directory at %q", escapePath)
}
}
func TestWriteProtoFile_RejectsDeepPathTraversal(t *testing.T) {
dir := t.TempDir()
if err := writeProtoFileForTest(t, dir, "foo/../../../escape.proto"); err == nil {
t.Fatal("expected error for path-traversing descriptor name, got nil")
}
escapePath := filepath.Join(dir, "foo", "..", "..", "..", "escape.proto")
if _, err := os.Stat(escapePath); err == nil {
t.Fatalf("file was created outside output directory at %q", escapePath)
}
}
+4 -5
View File
@@ -11,10 +11,10 @@ import (
"strings" "strings"
"sync" "sync"
"github.com/golang/protobuf/jsonpb" //lint:ignore SA1019 we have to import these because some of their types appear in exported API "github.com/golang/protobuf/jsonpb"
"github.com/golang/protobuf/proto" //lint:ignore SA1019 same as above "github.com/golang/protobuf/proto"
"github.com/jhump/protoreflect/desc" //lint:ignore SA1019 same as above "github.com/jhump/protoreflect/desc"
"github.com/jhump/protoreflect/dynamic" //lint:ignore SA1019 same as above "github.com/jhump/protoreflect/dynamic"
"google.golang.org/grpc/codes" "google.golang.org/grpc/codes"
"google.golang.org/grpc/metadata" "google.golang.org/grpc/metadata"
"google.golang.org/grpc/status" "google.golang.org/grpc/status"
@@ -330,7 +330,6 @@ func (r anyResolverWithFallback) Resolve(typeUrl string) (proto.Message, error)
if slash := strings.LastIndex(mname, "/"); slash >= 0 { if slash := strings.LastIndex(mname, "/"); slash >= 0 {
mname = mname[slash+1:] mname = mname[slash+1:]
} }
//lint:ignore SA1019 new non-deprecated API requires other code changes; deferring...
mt := proto.MessageType(mname) mt := proto.MessageType(mname)
if mt != nil { if mt != nil {
return reflect.New(mt.Elem()).Interface().(proto.Message), nil return reflect.New(mt.Elem()).Interface().(proto.Message), nil
+3 -3
View File
@@ -7,9 +7,9 @@ import (
"strings" "strings"
"testing" "testing"
"github.com/golang/protobuf/jsonpb" //lint:ignore SA1019 we have to import these because some of their types appear in exported API "github.com/golang/protobuf/jsonpb"
"github.com/golang/protobuf/proto" //lint:ignore SA1019 same as above "github.com/golang/protobuf/proto"
"github.com/jhump/protoreflect/desc" //lint:ignore SA1019 same as above "github.com/jhump/protoreflect/desc"
"google.golang.org/grpc/metadata" "google.golang.org/grpc/metadata"
"google.golang.org/protobuf/types/known/structpb" "google.golang.org/protobuf/types/known/structpb"
) )
+5 -7
View File
@@ -1,8 +1,6 @@
module github.com/fullstorydev/grpcurl module github.com/fullstorydev/grpcurl
go 1.24.0 go 1.25.0
toolchain go1.24.1
require ( require (
github.com/golang/protobuf v1.5.4 github.com/golang/protobuf v1.5.4
@@ -24,11 +22,11 @@ require (
github.com/planetscale/vtprotobuf v0.6.1-0.20240319094008-0393e58bdf10 // indirect github.com/planetscale/vtprotobuf v0.6.1-0.20240319094008-0393e58bdf10 // indirect
github.com/spiffe/go-spiffe/v2 v2.6.0 // indirect github.com/spiffe/go-spiffe/v2 v2.6.0 // indirect
github.com/stretchr/testify v1.11.1 // indirect github.com/stretchr/testify v1.11.1 // indirect
golang.org/x/net v0.49.0 // indirect golang.org/x/net v0.55.0 // indirect
golang.org/x/oauth2 v0.34.0 // indirect golang.org/x/oauth2 v0.34.0 // indirect
golang.org/x/sync v0.19.0 // indirect golang.org/x/sync v0.20.0 // indirect
golang.org/x/sys v0.40.0 // indirect golang.org/x/sys v0.45.0 // indirect
golang.org/x/text v0.33.0 // indirect golang.org/x/text v0.37.0 // indirect
google.golang.org/genproto/googleapis/api v0.0.0-20260120221211-b8f7ae30c516 // indirect google.golang.org/genproto/googleapis/api v0.0.0-20260120221211-b8f7ae30c516 // indirect
google.golang.org/genproto/googleapis/rpc v0.0.0-20260120221211-b8f7ae30c516 // indirect google.golang.org/genproto/googleapis/rpc v0.0.0-20260120221211-b8f7ae30c516 // indirect
) )
+8 -8
View File
@@ -56,16 +56,16 @@ go.opentelemetry.io/otel/sdk/metric v1.39.0 h1:cXMVVFVgsIf2YL6QkRF4Urbr/aMInf+2W
go.opentelemetry.io/otel/sdk/metric v1.39.0/go.mod h1:xq9HEVH7qeX69/JnwEfp6fVq5wosJsY1mt4lLfYdVew= go.opentelemetry.io/otel/sdk/metric v1.39.0/go.mod h1:xq9HEVH7qeX69/JnwEfp6fVq5wosJsY1mt4lLfYdVew=
go.opentelemetry.io/otel/trace v1.39.0 h1:2d2vfpEDmCJ5zVYz7ijaJdOF59xLomrvj7bjt6/qCJI= go.opentelemetry.io/otel/trace v1.39.0 h1:2d2vfpEDmCJ5zVYz7ijaJdOF59xLomrvj7bjt6/qCJI=
go.opentelemetry.io/otel/trace v1.39.0/go.mod h1:88w4/PnZSazkGzz/w84VHpQafiU4EtqqlVdxWy+rNOA= go.opentelemetry.io/otel/trace v1.39.0/go.mod h1:88w4/PnZSazkGzz/w84VHpQafiU4EtqqlVdxWy+rNOA=
golang.org/x/net v0.49.0 h1:eeHFmOGUTtaaPSGNmjBKpbng9MulQsJURQUAfUwY++o= golang.org/x/net v0.55.0 h1:bcvxaJn3e1U6InsFWt1JUq1aSjnRxLzT2rtD2KfkDF8=
golang.org/x/net v0.49.0/go.mod h1:/ysNB2EvaqvesRkuLAyjI1ycPZlQHM3q01F02UY/MV8= golang.org/x/net v0.55.0/go.mod h1:L5U2KuzuOe1lY7Z+aWVIKK6qEeJXnXV9yzGA+WCHJww=
golang.org/x/oauth2 v0.34.0 h1:hqK/t4AKgbqWkdkcAeI8XLmbK+4m4G5YeQRrmiotGlw= golang.org/x/oauth2 v0.34.0 h1:hqK/t4AKgbqWkdkcAeI8XLmbK+4m4G5YeQRrmiotGlw=
golang.org/x/oauth2 v0.34.0/go.mod h1:lzm5WQJQwKZ3nwavOZ3IS5Aulzxi68dUSgRHujetwEA= golang.org/x/oauth2 v0.34.0/go.mod h1:lzm5WQJQwKZ3nwavOZ3IS5Aulzxi68dUSgRHujetwEA=
golang.org/x/sync v0.19.0 h1:vV+1eWNmZ5geRlYjzm2adRgW2/mcpevXNg50YZtPCE4= golang.org/x/sync v0.20.0 h1:e0PTpb7pjO8GAtTs2dQ6jYa5BWYlMuX047Dco/pItO4=
golang.org/x/sync v0.19.0/go.mod h1:9KTHXmSnoGruLpwFjVSX0lNNA75CykiMECbovNTZqGI= golang.org/x/sync v0.20.0/go.mod h1:9xrNwdLfx4jkKbNva9FpL6vEN7evnE43NNNJQ2LF3+0=
golang.org/x/sys v0.40.0 h1:DBZZqJ2Rkml6QMQsZywtnjnnGvHza6BTfYFWY9kjEWQ= golang.org/x/sys v0.45.0 h1:dO4czNzziLiiXplLQgBCEpCvXQ3dnkn0SdaZSYdQ+FY=
golang.org/x/sys v0.40.0/go.mod h1:OgkHotnGiDImocRcuBABYBEXf8A9a87e/uXjp9XT3ks= golang.org/x/sys v0.45.0/go.mod h1:4GL1E5IUh+htKOUEOaiffhrAeqysfVGipDYzABqnCmw=
golang.org/x/text v0.33.0 h1:B3njUFyqtHDUI5jMn1YIr5B0IE2U0qck04r6d4KPAxE= golang.org/x/text v0.37.0 h1:Cqjiwd9eSg8e0QAkyCaQTNHFIIzWtidPahFWR83rTrc=
golang.org/x/text v0.33.0/go.mod h1:LuMebE6+rBincTi9+xWTY8TztLzKHc/9C1uBCG27+q8= golang.org/x/text v0.37.0/go.mod h1:a5sjxXGs9hsn/AJVwuElvCAo9v8QYLzvavO5z2PiM38=
gonum.org/v1/gonum v0.17.0 h1:VbpOemQlsSMrYmn7T2OUvQ4dqxQXU+ouZFQsZOx50z4= gonum.org/v1/gonum v0.17.0 h1:VbpOemQlsSMrYmn7T2OUvQ4dqxQXU+ouZFQsZOx50z4=
gonum.org/v1/gonum v0.17.0/go.mod h1:El3tOrEuMpv2UdMrbNlKEh9vd86bmQ6vqIcDwxEOc1E= gonum.org/v1/gonum v0.17.0/go.mod h1:El3tOrEuMpv2UdMrbNlKEh9vd86bmQ6vqIcDwxEOc1E=
google.golang.org/genproto/googleapis/api v0.0.0-20260120221211-b8f7ae30c516 h1:vmC/ws+pLzWjj/gzApyoZuSVrDtF1aod4u/+bbj8hgM= google.golang.org/genproto/googleapis/api v0.0.0-20260120221211-b8f7ae30c516 h1:vmC/ws+pLzWjj/gzApyoZuSVrDtF1aod4u/+bbj8hgM=
+62 -20
View File
@@ -21,11 +21,12 @@ import (
"sort" "sort"
"strings" "strings"
"sync" "sync"
"time"
"github.com/golang/protobuf/proto" //lint:ignore SA1019 we have to import these because some of their types appear in exported API "github.com/golang/protobuf/proto"
"github.com/jhump/protoreflect/desc" //lint:ignore SA1019 same as above "github.com/jhump/protoreflect/desc"
"github.com/jhump/protoreflect/desc/protoprint" "github.com/jhump/protoreflect/desc/protoprint"
"github.com/jhump/protoreflect/dynamic" //lint:ignore SA1019 same as above "github.com/jhump/protoreflect/dynamic"
"google.golang.org/grpc" "google.golang.org/grpc"
"google.golang.org/grpc/connectivity" "google.golang.org/grpc/connectivity"
"google.golang.org/grpc/credentials" "google.golang.org/grpc/credentials"
@@ -615,8 +616,8 @@ func BlockingDial(ctx context.Context, network, address string, creds credential
} }
var err error 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 // target. The relevant credentials will be automatically pulled from the GRPC_XDS_BOOTSTRAP or
// GRPC_XDS_BOOTSTRAP_CONFIG env vars. // GRPC_XDS_BOOTSTRAP_CONFIG env vars.
creds, err = xdsCredentials.NewClientCredentials(xdsCredentials.ClientOptions{FallbackCreds: creds}) creds, err = xdsCredentials.NewClientCredentials(xdsCredentials.ClientOptions{FallbackCreds: creds})
@@ -630,7 +631,16 @@ func BlockingDial(ctx context.Context, network, address string, creds credential
// custom dialer that can provide that info. That means we manage the TLS handshake. // custom dialer that can provide that info. That means we manage the TLS handshake.
result := make(chan interface{}, 1) result := make(chan interface{}, 1)
// dialCompleted is closed once the outcome of the dial (ready connection
// or error) is known, so that connection teardown no longer needs to wait
// for a pending TLS alert to be read.
dialCompleted := make(chan struct{})
var dialCompletedOnce sync.Once
completeDial := func() { dialCompletedOnce.Do(func() { close(dialCompleted) }) }
defer completeDial()
writeResult := func(res interface{}) { writeResult := func(res interface{}) {
completeDial()
// non-blocking write: we only need the first result // non-blocking write: we only need the first result
select { select {
case result <- res: case result <- res:
@@ -643,6 +653,7 @@ func BlockingDial(ctx context.Context, network, address string, creds credential
creds = &errSignalingCreds{ creds = &errSignalingCreds{
TransportCredentials: creds, TransportCredentials: creds,
writeResult: writeResult, writeResult: writeResult,
dialCompleted: dialCompleted,
} }
switch network { switch network {
@@ -726,6 +737,7 @@ func BlockingDial(ctx context.Context, network, address string, creds credential
type errSignalingCreds struct { type errSignalingCreds struct {
credentials.TransportCredentials credentials.TransportCredentials
writeResult func(res interface{}) writeResult func(res interface{})
dialCompleted <-chan struct{}
} }
func (c *errSignalingCreds) ClientHandshake(ctx context.Context, addr string, rawConn net.Conn) (net.Conn, credentials.AuthInfo, error) { func (c *errSignalingCreds) ClientHandshake(ctx context.Context, addr string, rawConn net.Conn) (net.Conn, credentials.AuthInfo, error) {
@@ -734,32 +746,62 @@ func (c *errSignalingCreds) ClientHandshake(ctx context.Context, addr string, ra
c.writeResult(err) c.writeResult(err)
return conn, auth, 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, dialCompleted: c.dialCompleted}, 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 // maxTLSAlertWait is an upper bound on how long a failing dial's connection
// report it via writeResult. This allows BlockingDial to surface post-handshake // teardown waits for a pending TLS alert to be read. It is a heuristic: long
// errors. // enough for the reader goroutine to be scheduled even on a loaded machine,
// short enough to not noticeably delay a failing dial.
const maxTLSAlertWait = 50 * time.Millisecond
// errSignalingConn wraps a net.Conn to capture read errors and report
// them via writeResult. Close is delayed until the dial has completed to
// give the reader a chance to read a pending TLS alert before the
// connection is closed.
type errSignalingConn struct { type errSignalingConn struct {
net.Conn net.Conn
writeResult func(res interface{}) writeResult func(res interface{})
once sync.Once dialCompleted <-chan struct{}
} }
func (c *errSignalingConn) Read(b []byte) (int, error) { func (c *errSignalingConn) Read(b []byte) (int, error) {
n, err := c.Conn.Read(b) n, err := c.Conn.Read(b)
if err != nil { if err != nil {
c.once.Do(func() {
c.writeResult(err) c.writeResult(err)
})
} }
return n, err return n, err
} }
func (c *errSignalingConn) Close() error {
// With TLS 1.3, the server may reject the connection *after* the
// handshake completes (e.g. a missing client cert), by sending an alert.
// If the transport closes the connection before that alert is read, the
// caller sees a less useful error (e.g. "use of closed network
// connection") instead of the TLS alert. So while the outcome of the
// dial is still undecided, give the reader a brief window to read a
// pending alert before closing the connection.
select {
case <-c.dialCompleted:
case <-time.After(maxTLSAlertWait):
}
return c.Conn.Close()
}
// 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
}
+3 -3
View File
@@ -12,9 +12,9 @@ import (
"testing" "testing"
"time" "time"
"github.com/golang/protobuf/jsonpb" //lint:ignore SA1019 we have to import these because some of their types appear in exported API "github.com/golang/protobuf/jsonpb"
"github.com/golang/protobuf/proto" //lint:ignore SA1019 same as above "github.com/golang/protobuf/proto"
"github.com/jhump/protoreflect/desc" //lint:ignore SA1019 same as above "github.com/jhump/protoreflect/desc"
"github.com/jhump/protoreflect/grpcreflect" "github.com/jhump/protoreflect/grpcreflect"
"google.golang.org/grpc" "google.golang.org/grpc"
"google.golang.org/grpc/codes" "google.golang.org/grpc/codes"
+4 -4
View File
@@ -9,10 +9,10 @@ import (
"sync" "sync"
"sync/atomic" "sync/atomic"
"github.com/golang/protobuf/jsonpb" //lint:ignore SA1019 we have to import these because some of their types appear in exported API "github.com/golang/protobuf/jsonpb"
"github.com/golang/protobuf/proto" //lint:ignore SA1019 same as above "github.com/golang/protobuf/proto"
"github.com/jhump/protoreflect/desc" //lint:ignore SA1019 same as above "github.com/jhump/protoreflect/desc"
"github.com/jhump/protoreflect/dynamic" //lint:ignore SA1019 same as above "github.com/jhump/protoreflect/dynamic"
"github.com/jhump/protoreflect/dynamic/grpcdynamic" "github.com/jhump/protoreflect/dynamic/grpcdynamic"
"github.com/jhump/protoreflect/grpcreflect" "github.com/jhump/protoreflect/grpcreflect"
"google.golang.org/grpc" "google.golang.org/grpc"
+8 -49
View File
@@ -171,58 +171,17 @@ func TestBrokenTLS_ClientPlainText(t *testing.T) {
t.Fatalf("failed to create server creds: %v", err) 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 { 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(), "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)
} }
} }