From 126713648e3a9ef2a3275c4aef1c0cc7e9844272 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Mon, 3 Feb 2025 20:20:07 +0100 Subject: [PATCH 1/5] cli/connhelper/ssh: TestParseURL: various improvements - use designated example domains as example value - swap "expected" and "actual" values in assertions - add doc / name for each test - add test-cases for remote commands - also test the Spec that's produced, not just the args - merge two test-cases that could be combined Signed-off-by: Sebastiaan van Stijn --- cli/connhelper/ssh/ssh_test.go | 95 +++++++++++++++++++++++++++------- 1 file changed, 77 insertions(+), 18 deletions(-) diff --git a/cli/connhelper/ssh/ssh_test.go b/cli/connhelper/ssh/ssh_test.go index 4b6d2c341d..cefe45eda7 100644 --- a/cli/connhelper/ssh/ssh_test.go +++ b/cli/connhelper/ssh/ssh_test.go @@ -9,59 +9,118 @@ 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", + doc: "ssh URL with username, port, and path", + url: "ssh://me@example.com:10022/var/run/docker.sock", + expectedArgs: []string{ + "-l", "me", + "-p", "10022", + "--", "example.com", + }, + expectedSpec: Spec{ + User: "me", + Host: "example.com", + Port: "10022", + Path: "/var/run/docker.sock", + }, + }, + { + doc: "invalid URL with password", + url: "ssh://me:passw0rd@example.com", expectedError: "plain-text password is not supported", }, { - url: "ssh://foo/bar", - expectedArgs: []string{ - "--", "foo", - }, - }, - { - url: "ssh://foo?bar", + doc: "invalid URL with query parameter", + url: "ssh://example.com?bar", expectedError: `extra query after the host: "bar"`, }, { - url: "ssh://foo#bar", + doc: "invalid URL with fragment", + url: "ssh://example.com#bar", expectedError: `extra fragment after the host: "bar"`, }, { + doc: "invalid URL without hostname", url: "ssh://", expectedError: "no host specified", }, { - url: "foo://bar", - expectedError: `expected scheme ssh, got "foo"`, + doc: "invalid URL with unsupported scheme", + url: "https://example.com", + expectedError: `expected scheme ssh, got "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)) } }) } From 6ca9766897a9578a95834ecb13f84819cad005a5 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Tue, 4 Feb 2025 11:09:31 +0100 Subject: [PATCH 2/5] cli/connhelper/ssh: improve GoDoc for ParseURL Signed-off-by: Sebastiaan van Stijn --- cli/connhelper/ssh/ssh.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/cli/connhelper/ssh/ssh.go b/cli/connhelper/ssh/ssh.go index fb4c911105..61b95b9ff4 100644 --- a/cli/connhelper/ssh/ssh.go +++ b/cli/connhelper/ssh/ssh.go @@ -7,7 +7,9 @@ import ( "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 { From 8c0c1db67923e8d6f439188f097487ad7851d49b Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Tue, 4 Feb 2025 11:55:13 +0100 Subject: [PATCH 3/5] cli/connhelper/ssh: use stdlib errors and improve errors Signed-off-by: Sebastiaan van Stijn --- cli/connhelper/ssh/ssh.go | 32 ++++++++++++++++++++++++-------- cli/connhelper/ssh/ssh_test.go | 26 ++++++++++++++++++++------ 2 files changed, 44 insertions(+), 14 deletions(-) diff --git a/cli/connhelper/ssh/ssh.go b/cli/connhelper/ssh/ssh.go index 61b95b9ff4..a2552b82b5 100644 --- a/cli/connhelper/ssh/ssh.go +++ b/cli/connhelper/ssh/ssh.go @@ -2,9 +2,9 @@ package ssh import ( + "errors" + "fmt" "net/url" - - "github.com/pkg/errors" ) // ParseURL creates a [Spec] from the given ssh URL. It returns an error if @@ -13,10 +13,25 @@ import ( 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 @@ -29,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 cefe45eda7..198d5a0802 100644 --- a/cli/connhelper/ssh/ssh_test.go +++ b/cli/connhelper/ssh/ssh_test.go @@ -84,30 +84,44 @@ func TestParseURL(t *testing.T) { Path: "/var/run/docker.sock", }, }, + { + doc: "malformed URL", + url: "malformed %%url", + expectedError: `invalid ssh URL: invalid URL escape "%%u"`, + }, + { + 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: "plain-text password is not supported", + expectedError: "invalid ssh URL: plain-text password is not supported", }, { doc: "invalid URL with query parameter", - url: "ssh://example.com?bar", - expectedError: `extra query after the host: "bar"`, + 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: `extra fragment after the host: "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: "ssh:///no-hostname", + expectedError: "invalid ssh URL: hostname is empty", }, { doc: "invalid URL with unsupported scheme", url: "https://example.com", - expectedError: `expected scheme ssh, got "https"`, + expectedError: `invalid ssh URL: incorrect scheme: https`, }, } for _, tc := range testCases { From 2c24fb2bcdb78ce45294f8361636691e3b6dae8e Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Tue, 4 Feb 2025 15:39:11 +0100 Subject: [PATCH 4/5] cli/connhelper/commandcon: use stdlib errors Signed-off-by: Sebastiaan van Stijn --- cli/connhelper/commandconn/commandconn.go | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) 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 { From c77159623ba3a87df6d6c8fb0b5ad820c395efa0 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Tue, 4 Feb 2025 15:39:28 +0100 Subject: [PATCH 5/5] cli/connhelper: use stdlib errors Signed-off-by: Sebastiaan van Stijn --- cli/connhelper/connhelper.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) 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)