Browse code

Merge pull request #26804 from stevvooe/clear-tlsconfig-unix-socket

client: pedantic checking of tlsconfig

Aaron Lehmann authored on 2016/10/12 07:47:47
Showing 6 changed files
... ...
@@ -131,15 +131,16 @@ func NewClient(host string, version string, client *http.Client, httpHeaders map
131 131
 		return nil, err
132 132
 	}
133 133
 
134
-	if client == nil {
135
-		client = &http.Client{}
136
-	}
137
-
138
-	if client.Transport == nil {
139
-		// setup the transport, if not already present
134
+	if client != nil {
135
+		if _, ok := client.Transport.(*http.Transport); !ok {
136
+			return nil, fmt.Errorf("unable to verify TLS configuration, invalid transport %v", client.Transport)
137
+		}
138
+	} else {
140 139
 		transport := new(http.Transport)
141 140
 		sockets.ConfigureTransport(transport, proto, addr)
142
-		client.Transport = transport
141
+		client = &http.Client{
142
+			Transport: transport,
143
+		}
143 144
 	}
144 145
 
145 146
 	return &Client{
... ...
@@ -42,6 +42,20 @@ func TestNewEnvClient(t *testing.T) {
42 42
 		},
43 43
 		{
44 44
 			envs: map[string]string{
45
+				"DOCKER_CERT_PATH":  "testdata/",
46
+				"DOCKER_TLS_VERIFY": "1",
47
+			},
48
+			expectedVersion: DefaultVersion,
49
+		},
50
+		{
51
+			envs: map[string]string{
52
+				"DOCKER_CERT_PATH": "testdata/",
53
+				"DOCKER_HOST":      "https://notaunixsocket",
54
+			},
55
+			expectedVersion: DefaultVersion,
56
+		},
57
+		{
58
+			envs: map[string]string{
45 59
 				"DOCKER_HOST": "host",
46 60
 			},
47 61
 			expectedError: "unable to parse docker host `host`",
... ...
@@ -69,7 +83,9 @@ func TestNewEnvClient(t *testing.T) {
69 69
 		recoverEnvs := setupEnvs(t, c.envs)
70 70
 		apiclient, err := NewEnvClient()
71 71
 		if c.expectedError != "" {
72
-			if err == nil || err.Error() != c.expectedError {
72
+			if err == nil {
73
+				t.Errorf("expected an error for %v", c)
74
+			} else if err.Error() != c.expectedError {
73 75
 				t.Errorf("expected an error %s, got %s, for %v", c.expectedError, err.Error(), c)
74 76
 			}
75 77
 		} else {
... ...
@@ -81,6 +97,19 @@ func TestNewEnvClient(t *testing.T) {
81 81
 				t.Errorf("expected %s, got %s, for %v", c.expectedVersion, version, c)
82 82
 			}
83 83
 		}
84
+
85
+		if c.envs["DOCKER_TLS_VERIFY"] != "" {
86
+			// pedantic checking that this is handled correctly
87
+			tr := apiclient.client.Transport.(*http.Transport)
88
+			if tr.TLSClientConfig == nil {
89
+				t.Errorf("no tls config found when DOCKER_TLS_VERIFY enabled")
90
+			}
91
+
92
+			if tr.TLSClientConfig.InsecureSkipVerify {
93
+				t.Errorf("tls verification should be enabled")
94
+			}
95
+		}
96
+
84 97
 		recoverEnvs(t)
85 98
 	}
86 99
 }
... ...
@@ -47,12 +47,7 @@ func (cli *Client) postHijacked(ctx context.Context, path string, query url.Valu
47 47
 	req.Header.Set("Connection", "Upgrade")
48 48
 	req.Header.Set("Upgrade", "tcp")
49 49
 
50
-	tlsConfig, err := resolveTLSConfig(cli.client.Transport)
51
-	if err != nil {
52
-		return types.HijackedResponse{}, err
53
-	}
54
-
55
-	conn, err := dial(cli.proto, cli.addr, tlsConfig)
50
+	conn, err := dial(cli.proto, cli.addr, resolveTLSConfig(cli.client.Transport))
56 51
 	if err != nil {
57 52
 		if strings.Contains(err.Error(), "connection refused") {
58 53
 			return types.HijackedResponse{}, fmt.Errorf("Cannot connect to the Docker daemon. Is 'docker daemon' running on this host?")
... ...
@@ -100,10 +100,7 @@ func (cli *Client) sendClientRequest(ctx context.Context, method, path string, q
100 100
 		req.Host = "docker"
101 101
 	}
102 102
 
103
-	scheme, err := resolveScheme(cli.client.Transport)
104
-	if err != nil {
105
-		return serverResp, err
106
-	}
103
+	scheme := resolveScheme(cli.client.Transport)
107 104
 
108 105
 	req.URL.Host = cli.addr
109 106
 	req.URL.Scheme = scheme
... ...
@@ -114,8 +111,7 @@ func (cli *Client) sendClientRequest(ctx context.Context, method, path string, q
114 114
 
115 115
 	resp, err := ctxhttp.Do(ctx, cli.client, req)
116 116
 	if err != nil {
117
-
118
-		if scheme == "https" && strings.Contains(err.Error(), "malformed HTTP response") {
117
+		if scheme != "https" && strings.Contains(err.Error(), "malformed HTTP response") {
119 118
 			return serverResp, fmt.Errorf("%v.\n* Are you trying to connect to a TLS-enabled daemon without TLS?", err)
120 119
 		}
121 120
 
... ...
@@ -18,14 +18,12 @@ func (tf transportFunc) RoundTrip(req *http.Request) (*http.Response, error) {
18 18
 
19 19
 // resolveTLSConfig attempts to resolve the tls configuration from the
20 20
 // RoundTripper.
21
-func resolveTLSConfig(transport http.RoundTripper) (*tls.Config, error) {
21
+func resolveTLSConfig(transport http.RoundTripper) *tls.Config {
22 22
 	switch tr := transport.(type) {
23 23
 	case *http.Transport:
24
-		return tr.TLSClientConfig, nil
25
-	case transportFunc:
26
-		return nil, nil // detect this type for testing.
24
+		return tr.TLSClientConfig
27 25
 	default:
28
-		return nil, errTLSConfigUnavailable
26
+		return nil
29 27
 	}
30 28
 }
31 29
 
... ...
@@ -37,15 +35,11 @@ func resolveTLSConfig(transport http.RoundTripper) (*tls.Config, error) {
37 37
 // Unfortunately, the model of having a host-ish/url-thingy as the connection
38 38
 // string has us confusing protocol and transport layers. We continue doing
39 39
 // this to avoid breaking existing clients but this should be addressed.
40
-func resolveScheme(transport http.RoundTripper) (string, error) {
41
-	c, err := resolveTLSConfig(transport)
42
-	if err != nil {
43
-		return "", err
44
-	}
45
-
40
+func resolveScheme(transport http.RoundTripper) string {
41
+	c := resolveTLSConfig(transport)
46 42
 	if c != nil {
47
-		return "https", nil
43
+		return "https"
48 44
 	}
49 45
 
50
-	return "http", nil
46
+	return "http"
51 47
 }
... ...
@@ -2623,7 +2623,7 @@ func (s *DockerSuite) TestRunModeUTSHost(c *check.C) {
2623 2623
 	c.Assert(out, checker.Contains, runconfig.ErrConflictUTSHostname.Error())
2624 2624
 }
2625 2625
 
2626
-func (s *DockerSuite) TestRunTLSverify(c *check.C) {
2626
+func (s *DockerSuite) TestRunTLSVerify(c *check.C) {
2627 2627
 	// Remote daemons use TLS and this test is not applicable when TLS is required.
2628 2628
 	testRequires(c, SameHostDaemon)
2629 2629
 	if out, code, err := dockerCmdWithError("ps"); err != nil || code != 0 {