diff --git a/cli/connhelper/commandconn/commandconn.go b/cli/connhelper/commandconn/commandconn.go index 52888a9100..4b04f8b39f 100644 --- a/cli/connhelper/commandconn/commandconn.go +++ b/cli/connhelper/commandconn/commandconn.go @@ -28,7 +28,6 @@ import ( "syscall" "time" - "github.com/pkg/errors" "github.com/sirupsen/logrus" ) @@ -149,7 +148,7 @@ func (c *commandConn) handleEOF(err error) error { c.stderrMu.Lock() stderr := c.stderr.String() c.stderrMu.Unlock() - return errors.Errorf("command %v did not exit after %v: stderr=%q", c.cmd.Args, err, stderr) + return fmt.Errorf("command %v did not exit after %v: stderr=%q", c.cmd.Args, err, stderr) } } @@ -159,7 +158,7 @@ func (c *commandConn) handleEOF(err error) error { c.stderrMu.Lock() stderr := c.stderr.String() c.stderrMu.Unlock() - return errors.Errorf("command %v has exited with %v, make sure the URL is valid, and Docker 18.09 or later is installed on the remote host: stderr=%s", c.cmd.Args, werr, stderr) + return fmt.Errorf("command %v has exited with %v, make sure the URL is valid, and Docker 18.09 or later is installed on the remote host: stderr=%s", c.cmd.Args, werr, stderr) } func ignorableCloseError(err error) bool { diff --git a/cli/connhelper/connhelper.go b/cli/connhelper/connhelper.go index 152d3e2953..9e3347c84c 100644 --- a/cli/connhelper/connhelper.go +++ b/cli/connhelper/connhelper.go @@ -3,13 +3,13 @@ package connhelper import ( "context" + "fmt" "net" "net/url" "strings" "github.com/docker/cli/cli/connhelper/commandconn" "github.com/docker/cli/cli/connhelper/ssh" - "github.com/pkg/errors" ) // ConnectionHelper allows to connect to a remote host with custom stream provider binary. @@ -43,7 +43,7 @@ func getConnectionHelper(daemonURL string, sshFlags []string) (*ConnectionHelper if u.Scheme == "ssh" { sp, err := ssh.ParseURL(daemonURL) if err != nil { - return nil, errors.Wrap(err, "ssh host connection is not valid") + return nil, fmt.Errorf("ssh host connection is not valid: %w", err) } sshFlags = addSSHTimeout(sshFlags) sshFlags = disablePseudoTerminalAllocation(sshFlags) diff --git a/cli/connhelper/ssh/ssh.go b/cli/connhelper/ssh/ssh.go index fb4c911105..a2552b82b5 100644 --- a/cli/connhelper/ssh/ssh.go +++ b/cli/connhelper/ssh/ssh.go @@ -2,19 +2,36 @@ package ssh import ( + "errors" + "fmt" "net/url" - - "github.com/pkg/errors" ) -// ParseURL parses URL +// ParseURL creates a [Spec] from the given ssh URL. It returns an error if +// the URL is using the wrong scheme, contains fragments, query-parameters, +// or contains a password. func ParseURL(daemonURL string) (*Spec, error) { u, err := url.Parse(daemonURL) if err != nil { - return nil, err + var urlErr *url.Error + if errors.As(err, &urlErr) { + err = urlErr.Unwrap() + } + return nil, fmt.Errorf("invalid ssh URL: %w", err) + } + s, err := newSpec(u) + if err != nil { + return nil, fmt.Errorf("invalid ssh URL: %w", err) + } + return s, nil +} + +func newSpec(u *url.URL) (*Spec, error) { + if u.Scheme == "" { + return nil, errors.New("no scheme provided") } if u.Scheme != "ssh" { - return nil, errors.Errorf("expected scheme ssh, got %q", u.Scheme) + return nil, errors.New("incorrect scheme: " + u.Scheme) } var sp Spec @@ -27,17 +44,18 @@ func ParseURL(daemonURL string) (*Spec, error) { } sp.Host = u.Hostname() if sp.Host == "" { - return nil, errors.Errorf("no host specified") + return nil, errors.New("hostname is empty") } sp.Port = u.Port() sp.Path = u.Path if u.RawQuery != "" { - return nil, errors.Errorf("extra query after the host: %q", u.RawQuery) + return nil, fmt.Errorf("query parameters are not allowed: %q", u.RawQuery) } if u.Fragment != "" { - return nil, errors.Errorf("extra fragment after the host: %q", u.Fragment) + return nil, fmt.Errorf("fragments are not allowed: %q", u.Fragment) } - return &sp, err + + return &sp, nil } // Spec of SSH URL diff --git a/cli/connhelper/ssh/ssh_test.go b/cli/connhelper/ssh/ssh_test.go index 4b6d2c341d..198d5a0802 100644 --- a/cli/connhelper/ssh/ssh_test.go +++ b/cli/connhelper/ssh/ssh_test.go @@ -9,59 +9,132 @@ import ( func TestParseURL(t *testing.T) { testCases := []struct { + doc string url string + remoteCommand []string expectedArgs []string expectedError string + expectedSpec Spec }{ { - url: "ssh://foo", + doc: "bare ssh URL", + url: "ssh://example.com", expectedArgs: []string{ - "--", "foo", + "--", "example.com", + }, + expectedSpec: Spec{ + Host: "example.com", }, }, { - url: "ssh://me@foo:10022", + doc: "bare ssh URL and remote command", + url: "ssh://example.com", + remoteCommand: []string{ + "docker", "system", "dial-stdio", + }, + expectedArgs: []string{ + "--", "example.com", + "docker", "system", "dial-stdio", + }, + expectedSpec: Spec{ + Host: "example.com", + }, + }, + { + doc: "ssh URL with path and remote command and flag", + url: "ssh://example.com/var/run/docker.sock", + remoteCommand: []string{ + "docker", "--host", "unix:///var/run/docker.sock", "system", "dial-stdio", + }, + expectedArgs: []string{ + "--", "example.com", + "docker", "--host", "unix:///var/run/docker.sock", "system", "dial-stdio", + }, + expectedSpec: Spec{ + Host: "example.com", + Path: "/var/run/docker.sock", + }, + }, + { + doc: "ssh URL with username and port", + url: "ssh://me@example.com:10022", expectedArgs: []string{ "-l", "me", "-p", "10022", - "--", "foo", + "--", "example.com", + }, + expectedSpec: Spec{ + User: "me", + Host: "example.com", + Port: "10022", }, }, { - url: "ssh://me:passw0rd@foo", - expectedError: "plain-text password is not supported", - }, - { - url: "ssh://foo/bar", + doc: "ssh URL with username, port, and path", + url: "ssh://me@example.com:10022/var/run/docker.sock", expectedArgs: []string{ - "--", "foo", + "-l", "me", + "-p", "10022", + "--", "example.com", + }, + expectedSpec: Spec{ + User: "me", + Host: "example.com", + Port: "10022", + Path: "/var/run/docker.sock", }, }, { - url: "ssh://foo?bar", - expectedError: `extra query after the host: "bar"`, + doc: "malformed URL", + url: "malformed %%url", + expectedError: `invalid ssh URL: invalid URL escape "%%u"`, }, { - url: "ssh://foo#bar", - expectedError: `extra fragment after the host: "bar"`, + doc: "URL missing scheme", + url: "no-scheme.example.com", + expectedError: "invalid ssh URL: no scheme provided", }, { + doc: "invalid URL with password", + url: "ssh://me:passw0rd@example.com", + expectedError: "invalid ssh URL: plain-text password is not supported", + }, + { + doc: "invalid URL with query parameter", + url: "ssh://example.com?foo=bar&bar=baz", + expectedError: `invalid ssh URL: query parameters are not allowed: "foo=bar&bar=baz"`, + }, + { + doc: "invalid URL with fragment", + url: "ssh://example.com#bar", + expectedError: `invalid ssh URL: fragments are not allowed: "bar"`, + }, + { + doc: "invalid URL without hostname", url: "ssh://", - expectedError: "no host specified", + expectedError: "invalid ssh URL: hostname is empty", }, { - url: "foo://bar", - expectedError: `expected scheme ssh, got "foo"`, + url: "ssh:///no-hostname", + expectedError: "invalid ssh URL: hostname is empty", + }, + { + doc: "invalid URL with unsupported scheme", + url: "https://example.com", + expectedError: `invalid ssh URL: incorrect scheme: https`, }, } for _, tc := range testCases { - t.Run(tc.url, func(t *testing.T) { + t.Run(tc.doc, func(t *testing.T) { sp, err := ParseURL(tc.url) if tc.expectedError == "" { assert.NilError(t, err) - assert.Check(t, is.DeepEqual(tc.expectedArgs, sp.Args())) + actualArgs := sp.Args(tc.remoteCommand...) + assert.Check(t, is.DeepEqual(actualArgs, tc.expectedArgs)) + assert.Check(t, is.Equal(*sp, tc.expectedSpec)) } else { - assert.ErrorContains(t, err, tc.expectedError) + assert.Check(t, is.Error(err, tc.expectedError)) + assert.Check(t, is.Nil(sp)) } }) }