client: pedantic checking of tlsconfig

Under the convoluted code path for the transport configuration,
TLSConfig was being set even though the socket type is unix. This caused
other code detecting the TLSConfig to assume https, rather than using
the http scheme. This led to a situation where if `DOCKER_CERT_PATH` is
set, unix sockets start reverting to https. There is other odd behavior
from go-connections that is also reproduced here.

For the most part, we try to reproduce the side-effecting behavior from
go-connections to retain the current docker behavior. This whole mess
needs to ripped out and fixed, as this pile spaghetti is unnacceptable.

This code is way to convoluted for an http client. We'll need to fix
this but the Go API will break to do it.

Signed-off-by: Stephen J Day <stephen.day@docker.com>

client/client.go

@@ -86,15 +86,16 @@ func NewClient(host string, version string, client *http.Client, httpHeaders map
 		return nil, err
 	}
 
-	if client == nil {
-		client = &http.Client{}
-	}
-
-	if client.Transport == nil {
-		// setup the transport, if not already present
+	if client != nil {
+		if _, ok := client.Transport.(*http.Transport); !ok {
+			return nil, fmt.Errorf("unable to verify TLS configuration, invalid transport %v", client.Transport)
+		}
+	} else {
 		transport := new(http.Transport)
 		sockets.ConfigureTransport(transport, proto, addr)
-		client.Transport = transport
+		client = &http.Client{
+			Transport: transport,
+		}
 	}
 
 	return &Client{

client/client_test.go

@@ -40,6 +40,20 @@ func TestNewEnvClient(t *testing.T) {
 			},
 			expectedVersion: DefaultVersion,
 		},
+		{
+			envs: map[string]string{
+				"DOCKER_CERT_PATH":  "testdata/",
+				"DOCKER_TLS_VERIFY": "1",
+			},
+			expectedVersion: DefaultVersion,
+		},
+		{
+			envs: map[string]string{
+				"DOCKER_CERT_PATH": "testdata/",
+				"DOCKER_HOST":      "https://notaunixsocket",
+			},
+			expectedVersion: DefaultVersion,
+		},
 		{
 			envs: map[string]string{
 				"DOCKER_HOST": "host",
@@ -69,7 +83,9 @@ func TestNewEnvClient(t *testing.T) {
 		recoverEnvs := setupEnvs(t, c.envs)
 		apiclient, err := NewEnvClient()
 		if c.expectedError != "" {
-			if err == nil || err.Error() != c.expectedError {
+			if err == nil {
+				t.Errorf("expected an error for %v", c)
+			} else if err.Error() != c.expectedError {
 				t.Errorf("expected an error %s, got %s, for %v", c.expectedError, err.Error(), c)
 			}
 		} else {
@@ -81,6 +97,19 @@ func TestNewEnvClient(t *testing.T) {
 				t.Errorf("expected %s, got %s, for %v", c.expectedVersion, version, c)
 			}
 		}
+
+		if c.envs["DOCKER_TLS_VERIFY"] != "" {
+			// pedantic checking that this is handled correctly
+			tr := apiclient.client.Transport.(*http.Transport)
+			if tr.TLSClientConfig == nil {
+				t.Errorf("no tls config found when DOCKER_TLS_VERIFY enabled")
+			}
+
+			if tr.TLSClientConfig.InsecureSkipVerify {
+				t.Errorf("tls verification should be enabled")
+			}
+		}
+
 		recoverEnvs(t)
 	}
 }

client/hijack.go

@@ -47,12 +47,7 @@ func (cli *Client) postHijacked(ctx context.Context, path string, query url.Valu
 	req.Header.Set("Connection", "Upgrade")
 	req.Header.Set("Upgrade", "tcp")
 
-	tlsConfig, err := resolveTLSConfig(cli.client.Transport)
-	if err != nil {
-		return types.HijackedResponse{}, err
-	}
-
-	conn, err := dial(cli.proto, cli.addr, tlsConfig)
+	conn, err := dial(cli.proto, cli.addr, resolveTLSConfig(cli.client.Transport))
 	if err != nil {
 		if strings.Contains(err.Error(), "connection refused") {
 			return types.HijackedResponse{}, fmt.Errorf("Cannot connect to the Docker daemon. Is 'docker daemon' running on this host?")

client/request.go

@@ -99,10 +99,7 @@ func (cli *Client) sendClientRequest(ctx context.Context, method, path string, q
 		req.Host = "docker"
 	}
 
-	scheme, err := resolveScheme(cli.client.Transport)
-	if err != nil {
-		return serverResp, err
-	}
+	scheme := resolveScheme(cli.client.Transport)
 
 	req.URL.Host = cli.addr
 	req.URL.Scheme = scheme
@@ -113,8 +110,7 @@ func (cli *Client) sendClientRequest(ctx context.Context, method, path string, q
 
 	resp, err := ctxhttp.Do(ctx, cli.client, req)
 	if err != nil {
-
-		if scheme == "https" && strings.Contains(err.Error(), "malformed HTTP response") {
+		if scheme != "https" && strings.Contains(err.Error(), "malformed HTTP response") {
 			return serverResp, fmt.Errorf("%v.\n* Are you trying to connect to a TLS-enabled daemon without TLS?", err)
 		}
 

client/transport.go

@@ -18,14 +18,12 @@ func (tf transportFunc) RoundTrip(req *http.Request) (*http.Response, error) {
 
 // resolveTLSConfig attempts to resolve the tls configuration from the
 // RoundTripper.
-func resolveTLSConfig(transport http.RoundTripper) (*tls.Config, error) {
+func resolveTLSConfig(transport http.RoundTripper) *tls.Config {
 	switch tr := transport.(type) {
 	case *http.Transport:
-		return tr.TLSClientConfig, nil
-	case transportFunc:
-		return nil, nil // detect this type for testing.
+		return tr.TLSClientConfig
 	default:
-		return nil, errTLSConfigUnavailable
+		return nil
 	}
 }
 
@@ -37,15 +35,11 @@ func resolveTLSConfig(transport http.RoundTripper) (*tls.Config, error) {
 // Unfortunately, the model of having a host-ish/url-thingy as the connection
 // string has us confusing protocol and transport layers. We continue doing
 // this to avoid breaking existing clients but this should be addressed.
-func resolveScheme(transport http.RoundTripper) (string, error) {
-	c, err := resolveTLSConfig(transport)
-	if err != nil {
-		return "", err
-	}
-
+func resolveScheme(transport http.RoundTripper) string {
+	c := resolveTLSConfig(transport)
 	if c != nil {
-		return "https", nil
+		return "https"
 	}
 
-	return "http", nil
+	return "http"
 }

integration-cli/docker_cli_run_test.go

@@ -2628,7 +2628,7 @@ func (s *DockerSuite) TestRunModeUTSHost(c *check.C) {
 	c.Assert(out, checker.Contains, runconfig.ErrConflictUTSHostname.Error())
 }
 
-func (s *DockerSuite) TestRunTLSverify(c *check.C) {
+func (s *DockerSuite) TestRunTLSVerify(c *check.C) {
 	// Remote daemons use TLS and this test is not applicable when TLS is required.
 	testRequires(c, SameHostDaemon)
 	if out, code, err := dockerCmdWithError("ps"); err != nil || code != 0 {