client: do not modify user-provided HTTP client

The http.Client passed into client.WithHTTPClient() is modified by the
constructor in-place: the value of its Transport field is mutated and
wrapped in an OpenTelemetry decorator. This can lead to very surprising
behaviour when a second client is constructed reusing the same
http.Client value. If the http.Client is configured for TLS, the second
client will fail to detect that and will incorrectly dial the Engine API
socket as cleartext HTTP. Copy the provided http.Client so our
modifications don't leak out to unexpected places.

Signed-off-by: Cory Snider <csnider@mirantis.com>

.golangci.yml

@@ -293,6 +293,11 @@ linters:
         linters:
           - gosec
 
+      - text: "^G402: " # Look for bad TLS connection settings
+        source: "cmpopts\\.Ignore"
+        linters:
+          - gosec
+
         # FIXME: ignoring unused assigns to ctx for now; too many hits in libnetwork/xxx functions that setup traces
       - text: "assigned to ctx, but never used afterwards"
         linters:

client/client_options.go

@@ -149,7 +149,16 @@ func WithHostFromEnv() Opt {
 func WithHTTPClient(client *http.Client) Opt {
 	return func(c *clientConfig) error {
 		if client != nil {
-			c.client = client
+			// Make a clone of client so modifications do not affect
+			// the caller's client. Clone here instead of in New()
+			// as other options (WithHost) also mutate c.client.
+			// Cloned clients share the same CookieJar as the
+			// original.
+			hc := *client
+			if ht, ok := hc.Transport.(*http.Transport); ok {
+				hc.Transport = ht.Clone()
+			}
+			c.client = &hc
 		}
 		return nil
 	}

client/client_options_test.go

@@ -1,12 +1,15 @@
 package client
 
 import (
+	"crypto/tls"
 	"net/http"
+	"net/http/cookiejar"
 	"runtime"
 	"strings"
 	"testing"
 	"time"
 
+	"github.com/google/go-cmp/cmp/cmpopts"
 	"gotest.tools/v3/assert"
 	is "gotest.tools/v3/assert/cmp"
 )
@@ -367,3 +370,23 @@ func TestWithUserAgent(t *testing.T) {
 		assert.NilError(t, c.Close())
 	})
 }
+
+func TestWithHTTPClient(t *testing.T) {
+	cookieJar, err := cookiejar.New(nil)
+	assert.NilError(t, err)
+	pristineHTTPClient := func() *http.Client {
+		return &http.Client{
+			Timeout: 42 * time.Second,
+			Jar:     cookieJar,
+			Transport: &http.Transport{
+				TLSClientConfig: &tls.Config{ServerName: "example.com", MinVersion: tls.VersionTLS12},
+			},
+		}
+	}
+	hc := pristineHTTPClient()
+	_, err = New(WithHTTPClient(hc), WithHost("tcp://example.com:443"))
+	assert.NilError(t, err)
+	assert.DeepEqual(t, hc, pristineHTTPClient(),
+		cmpopts.IgnoreUnexported(http.Transport{}, tls.Config{}),
+		cmpopts.EquateComparable(&cookiejar.Jar{}))
+}

vendor/github.com/moby/moby/client/client_options.go

@@ -149,7 +149,16 @@ func WithHostFromEnv() Opt {
 func WithHTTPClient(client *http.Client) Opt {
 	return func(c *clientConfig) error {
 		if client != nil {
-			c.client = client
+			// Make a clone of client so modifications do not affect
+			// the caller's client. Clone here instead of in New()
+			// as other options (WithHost) also mutate c.client.
+			// Cloned clients share the same CookieJar as the
+			// original.
+			hc := *client
+			if ht, ok := hc.Transport.(*http.Transport); ok {
+				hc.Transport = ht.Clone()
+			}
+			c.client = &hc
 		}
 		return nil
 	}