From e7678f3a37e49533366e4a669f102df70ea1116c Mon Sep 17 00:00:00 2001 From: Stephen J Day Date: Wed, 21 Sep 2016 19:16:44 -0700 Subject: [PATCH] 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 --- client.go | 15 ++++++++------- client_test.go | 31 ++++++++++++++++++++++++++++++- hijack.go | 7 +------ request.go | 8 ++------ transport.go | 20 +++++++------------- 5 files changed, 48 insertions(+), 33 deletions(-) diff --git a/client.go b/client.go index deccb4ab74..ff9efa5700 100644 --- a/client.go +++ b/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{ diff --git a/client_test.go b/client_test.go index 60e44dc299..eaac339658 100644 --- a/client_test.go +++ b/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) } } diff --git a/hijack.go b/hijack.go index f3461ecf78..dededb7af2 100644 --- a/hijack.go +++ b/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?") diff --git a/request.go b/request.go index f5c239bf25..07a12657a4 100644 --- a/request.go +++ b/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) } diff --git a/transport.go b/transport.go index 43a667272d..771d76f06b 100644 --- a/transport.go +++ b/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" }