Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: TCP+TLS docker daemon connection #1629

Merged
merged 2 commits into from
Mar 16, 2023
Merged

fix: TCP+TLS docker daemon connection #1629

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
eb0853d
fix: TCP+TLS docker daemon connection
matejvasek Mar 15, 2023
4b385c8
fixup: skip tls verify if DOCKER_TLS_VERIFY is falsy
matejvasek Mar 16, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
83 changes: 82 additions & 1 deletion pkg/docker/docker_client.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,16 +1,24 @@
package docker

import (
"crypto/tls"
"crypto/x509"
"encoding/json"
"errors"
"io"
"net"
"net/http"
"net/url"
"os"
"os/exec"
"path/filepath"
"runtime"
"strconv"
"time"

"github.com/docker/cli/cli/config"
"github.com/docker/docker/client"
"github.com/docker/go-connections/tlsconfig"
"golang.org/x/crypto/ssh"

fnssh "knative.dev/func/pkg/ssh"
Expand Down Expand Up @@ -89,7 +97,13 @@ func NewClient(defaultHost string) (dockerClient client.CommonAPIClient, dockerH
}

if !isSSH {
dockerClient, err = client.NewClientWithOpts(client.FromEnv, client.WithAPIVersionNegotiation(), client.WithHost(dockerHost))
opts := []client.Opt{client.FromEnv}
if isTCP {
if httpClient := newHttpClient(); httpClient != nil {
opts = append(opts, client.WithHTTPClient(httpClient))
}
}
dockerClient, err = client.NewClientWithOpts(opts...)
dockerClient = &closeGuardingClient{pimpl: dockerClient}
return
}
Expand Down Expand Up @@ -132,6 +146,73 @@ func NewClient(defaultHost string) (dockerClient client.CommonAPIClient, dockerH
return dockerClient, dockerHostInRemote, err
}

// If the DOCKER_TLS_VERIFY environment variable is set
// this function returns HTTP client with appropriately configured TLS config.
// Otherwise, nil is returned.
func newHttpClient() *http.Client {
tlsVerifyStr, tlsVerifyChanged := os.LookupEnv("DOCKER_TLS_VERIFY")

if !tlsVerifyChanged {
return nil
}

var tlsOpts []func(*tls.Config)

tlsVerify := true
if b, err := strconv.ParseBool(tlsVerifyStr); err == nil {
tlsVerify = b
}

if !tlsVerify {
tlsOpts = append(tlsOpts, func(t *tls.Config) {
t.InsecureSkipVerify = true

Check failure

Code scanning / CodeQL

Disabled TLS certificate check

InsecureSkipVerify should not be used in production code.
Copy link

@riverar riverar Mar 16, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would delete this.

Copy link
Contributor Author

@matejvasek matejvasek Mar 16, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But docker CLI does this too.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Without this DOCKER_TLS_VERIFY=0 wouldn't have desired effect, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lance @evankanderson wdyt?

Copy link
Contributor Author

@matejvasek matejvasek Mar 16, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also:
https://github.com/docker/cli/blob/82427d1a078e89d9bd10dd397e5ee7736a8086f9/cli/context/docker/load.go#L82-L86
Depends what sets c.SkipTLSVerify maybe CLI only set is via flag?

Copy link
Contributor Author

@matejvasek matejvasek Mar 16, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@riverar you are right about behaviour of the envvar -- it only matter if its set/empty.
The effect that I described (encryption+non_verify_cert) can be achieved by --tlsverify=false flag.

So I immodestly dare to say that my implementation here is little bit better.
The thing is that we do not have --tlsverify flag, so to achieve similar effect we can use the envvar (even if that deviates from what docker CLI does).

Do you agree?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed. (I continue to be extremely disappointed with Docker, esp. its impl. on Windows.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@riverar Just try out docker --tlsverify=false image ls it will use https but will skip cert verification.

Copy link

@riverar riverar Mar 16, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@matejvasek Ah ha! It does indeed. (That's a miserable mismatch in behavior, imo.)

})
}

dockerCertPath := os.Getenv("DOCKER_CERT_PATH")
if dockerCertPath == "" {
dockerCertPath = config.Dir()
}

// Set root CA.
caData, err := os.ReadFile(filepath.Join(dockerCertPath, "ca.pem"))
if err == nil {
certPool := x509.NewCertPool()
if certPool.AppendCertsFromPEM(caData) {
tlsOpts = append(tlsOpts, func(t *tls.Config) {
t.RootCAs = certPool
})
}
}

// Set client certificate.
certData, certErr := os.ReadFile(filepath.Join(dockerCertPath, "cert.pem"))
keyData, keyErr := os.ReadFile(filepath.Join(dockerCertPath, "key.pem"))
if certErr == nil && keyErr == nil {
cliCert, err := tls.X509KeyPair(certData, keyData)
if err == nil {
tlsOpts = append(tlsOpts, func(cfg *tls.Config) {
cfg.Certificates = []tls.Certificate{cliCert}
})
}
}

dialer := &net.Dialer{
KeepAlive: 30 * time.Second,
Timeout: 30 * time.Second,
}

tlsConfig := tlsconfig.ClientDefault(tlsOpts...)

return &http.Client{
Transport: &http.Transport{
TLSClientConfig: tlsConfig,
DialContext: dialer.DialContext,
},
CheckRedirect: client.CheckRedirect,
}
}

// tries to get connection to default podman machine
func tryGetPodmanRemoteConn() (uri string, identity string) {
cmd := exec.Command("podman", "system", "connection", "list", "--format=json")
Expand Down