diff --git a/client.go b/client.go index aafb7d30..92f7d14d 100644 --- a/client.go +++ b/client.go @@ -151,8 +151,16 @@ func setupClientWithConfig(ctx context.Context, config *ClientConfig) (c *APICli MaxIdleConns: defaultTransport.MaxIdleConns, IdleConnTimeout: defaultTransport.IdleConnTimeout, ExpectContinueTimeout: defaultTransport.ExpectContinueTimeout, - TLSClientConfig: defaultTransport.TLSClientConfig, - TLSHandshakeTimeout: time.Duration(config.TLSHandshakeTimeout) * time.Second, + // Clone rather than share the pointer. The default transport's config + // belongs to the whole process, and both the InsecureSkipVerify write + // below and net/http's HTTP/2 setup write into whatever config a + // transport holds. + TLSClientConfig: defaultTransport.TLSClientConfig.Clone(), + TLSHandshakeTimeout: time.Duration(config.TLSHandshakeTimeout) * time.Second, + // Without this, Go disables HTTP/2 negotiation whenever TLSClientConfig + // or DialContext is set. Setting it re-enables ALPN negotiation, so the + // protocol is still picked during the TLS handshake. + ForceAttemptHTTP2: true, } config.HTTPClient = &http.Client{Transport: transport} @@ -172,9 +180,14 @@ func setupClientWithConfig(ctx context.Context, config *ClientConfig) (c *APICli MinVersion: tls.VersionTLS12, } } - transport.TLSClientConfig.InsecureSkipVerify = config.Insecure + transport.TLSClientConfig.InsecureSkipVerify = true } + // Without this, Go disables HTTP/2 negotiation whenever TLSClientConfig + // or DialContext is set. Setting it re-enables ALPN negotiation, so the + // protocol is still picked during the TLS handshake. + transport.ForceAttemptHTTP2 = true + if config.ReuseConnections { transport.DisableKeepAlives = false transport.IdleConnTimeout = 1 * time.Minute diff --git a/client_test.go b/client_test.go index 490a82b8..182f862d 100644 --- a/client_test.go +++ b/client_test.go @@ -17,6 +17,8 @@ import ( "math/big" "net/http" "net/http/httptest" + "sync" + "sync/atomic" "testing" "time" @@ -277,3 +279,105 @@ func TestCertHashMonitoring(t *testing.T) { } } } + +// newServiceRootTLSServer starts an HTTPS server with a self-signed +// certificate that answers every request with an empty service root. +func newServiceRootTLSServer(t *testing.T) *httptest.Server { + t.Helper() + ts := httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.Write([]byte(`{}`)) //nolint + })) + t.Cleanup(ts.Close) + return ts +} + +// setDefaultTransportTLSConfig puts cfg on http.DefaultTransport until the +// test ends. A process has a non-nil config there after its first HTTPS +// request through the default transport, or when it installs its own. The +// config is process-wide, so tests that call this must not run in parallel. +func setDefaultTransportTLSConfig(t *testing.T, cfg *tls.Config) { + t.Helper() + defaultTransport := http.DefaultTransport.(*http.Transport) + original := defaultTransport.TLSClientConfig + defaultTransport.TLSClientConfig = cfg + t.Cleanup(func() { + defaultTransport.CloseIdleConnections() + defaultTransport.TLSClientConfig = original + }) +} + +// TestConnectInsecureLeavesDefaultTransportTLSConfig checks that an Insecure +// connect skips certificate checks only on the transport gofish builds, and +// that other requests through http.DefaultTransport still verify certificates. +func TestConnectInsecureLeavesDefaultTransportTLSConfig(t *testing.T) { + ts := newServiceRootTLSServer(t) + shared := &tls.Config{MinVersion: tls.VersionTLS12} + setDefaultTransportTLSConfig(t, shared) + + // No HTTPClient, so gofish builds its transport from http.DefaultTransport. + if _, err := Connect(ClientConfig{Endpoint: ts.URL, Insecure: true}); err != nil { + t.Fatalf("insecure connect to a self-signed server failed: %v", err) + } + + if shared.InsecureSkipVerify { + t.Error("connect set InsecureSkipVerify on http.DefaultTransport's tls.Config") + } + if len(shared.NextProtos) != 0 { + t.Errorf("connect changed NextProtos on http.DefaultTransport's tls.Config: %v", shared.NextProtos) + } + + req, err := http.NewRequestWithContext(context.Background(), http.MethodGet, ts.URL, http.NoBody) + if err != nil { + t.Fatal(err) + } + resp, err := http.DefaultClient.Do(req) + if err == nil { + resp.Body.Close() + t.Fatal("http.DefaultClient accepted a self-signed certificate after an insecure connect") + } + var certErr *tls.CertificateVerificationError + if !errors.As(err, &certErr) { + t.Errorf("expected a certificate verification error, got: %v", err) + } +} + +// TestConnectInsecureConcurrent runs Insecure connects in parallel while +// http.DefaultTransport holds a tls.Config. Each connect needs its own copy of +// that config, or go test -race reports the writes to the shared one. +func TestConnectInsecureConcurrent(t *testing.T) { + ts := newServiceRootTLSServer(t) + setDefaultTransportTLSConfig(t, &tls.Config{MinVersion: tls.VersionTLS12}) + + var wg sync.WaitGroup + for i := 0; i < 8; i++ { + wg.Add(1) + go func() { + defer wg.Done() + if _, err := Connect(ClientConfig{Endpoint: ts.URL, Insecure: true}); err != nil { + t.Errorf("concurrent insecure connect failed: %v", err) + } + }() + } + wg.Wait() +} + +// TestConnectNegotiatesHTTP2 checks that the transport gofish builds still +// offers HTTP/2. net/http turns HTTP/2 off on a transport with its own +// TLSClientConfig or DialContext unless ForceAttemptHTTP2 is set. +func TestConnectNegotiatesHTTP2(t *testing.T) { + var proto atomic.Value + ts := httptest.NewUnstartedServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + proto.Store(r.Proto) + w.Write([]byte(`{}`)) //nolint + })) + ts.EnableHTTP2 = true + ts.StartTLS() + t.Cleanup(ts.Close) + + if _, err := Connect(ClientConfig{Endpoint: ts.URL, Insecure: true}); err != nil { + t.Fatalf("insecure connect failed: %v", err) + } + if got := proto.Load(); got != "HTTP/2.0" { + t.Errorf("expected the connect to negotiate HTTP/2.0, got %v", got) + } +}