From e1d4c012bc6e2219e3807f474b198c8bbed3b54a Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Thu, 15 Jun 2017 10:58:17 +0200 Subject: [PATCH] Fix storing URLs without scheme (#72) * Fix storing URLs without scheme If secrets are stored without specifying a scheme (https://), the keychain-helper would interpret the hostname as _path_, causing lookup of secrets to fail. This patch makes sure that a scheme is added (if missing). If no scheme is specified, https:// is used as a default. Signed-off-by: Sebastiaan van Stijn * Have pre go1.8 compiler able to compile Signed-off-by: Tibor Vass * Fix URL parsing with port and no scheme Signed-off-by: Nassim 'Nass' Eddequiouaq * Improve parseURL comment Signed-off-by: Nassim 'Nass' Eddequiouaq --- credentials/credentials_test.go | 8 +- credentials/error.go | 5 +- osxkeychain/osxkeychain_darwin.go | 50 +++++-- osxkeychain/osxkeychain_darwin_test.go | 190 +++++++++++++++++++++++++ osxkeychain/url_go18.go | 13 ++ osxkeychain/url_non_go18.go | 41 ++++++ wincred/wincred_windows_test.go | 4 +- 7 files changed, 289 insertions(+), 22 deletions(-) create mode 100644 osxkeychain/url_go18.go create mode 100644 osxkeychain/url_non_go18.go diff --git a/credentials/credentials_test.go b/credentials/credentials_test.go index e7ffa69..9c0f1d7 100644 --- a/credentials/credentials_test.go +++ b/credentials/credentials_test.go @@ -76,8 +76,8 @@ func TestStore(t *testing.T) { func TestStoreMissingServerURL(t *testing.T) { creds := &Credentials{ ServerURL: "", - Username: "foo", - Secret: "bar", + Username: "foo", + Secret: "bar", } b, err := json.Marshal(creds) @@ -96,8 +96,8 @@ func TestStoreMissingServerURL(t *testing.T) { func TestStoreMissingUsername(t *testing.T) { creds := &Credentials{ ServerURL: "https://index.docker.io/v1/", - Username: "", - Secret: "bar", + Username: "", + Secret: "bar", } b, err := json.Marshal(creds) diff --git a/credentials/error.go b/credentials/error.go index 588d4a8..fe6a5ae 100644 --- a/credentials/error.go +++ b/credentials/error.go @@ -8,7 +8,7 @@ const ( // ErrCredentialsMissingServerURL and ErrCredentialsMissingUsername standardize // invalid credentials or credentials management operations errCredentialsMissingServerURLMessage = "no credentials server URL" - errCredentialsMissingUsernameMessage = "no credentials username" + errCredentialsMissingUsernameMessage = "no credentials username" ) // errCredentialsNotFound represents an error @@ -43,7 +43,6 @@ func IsErrCredentialsNotFoundMessage(err string) bool { return err == errCredentialsNotFoundMessage } - // errCredentialsMissingServerURL represents an error raised // when the credentials object has no server URL or when no // server URL is provided to a credentials operation requiring @@ -64,7 +63,6 @@ func (errCredentialsMissingUsername) Error() string { return errCredentialsMissingUsernameMessage } - // NewErrCredentialsMissingServerURL creates a new error for // errCredentialsMissingServerURL. func NewErrCredentialsMissingServerURL() error { @@ -77,7 +75,6 @@ func NewErrCredentialsMissingUsername() error { return errCredentialsMissingUsername{} } - // IsCredentialsMissingServerURL returns true if the error // was an errCredentialsMissingServerURL. func IsCredentialsMissingServerURL(err error) bool { diff --git a/osxkeychain/osxkeychain_darwin.go b/osxkeychain/osxkeychain_darwin.go index a3f5da8..4391267 100644 --- a/osxkeychain/osxkeychain_darwin.go +++ b/osxkeychain/osxkeychain_darwin.go @@ -135,30 +135,27 @@ func (h Osxkeychain) List() (map[string]string, error) { } func splitServer(serverURL string) (*C.struct_Server, error) { - u, err := url.Parse(serverURL) + u, err := parseURL(serverURL) if err != nil { return nil, err } - hostAndPort := strings.Split(u.Host, ":") - host := hostAndPort[0] + proto := C.kSecProtocolTypeHTTPS + if u.Scheme == "http" { + proto = C.kSecProtocolTypeHTTP + } var port int - if len(hostAndPort) == 2 { - p, err := strconv.Atoi(hostAndPort[1]) + p := getPort(u) + if p != "" { + port, err = strconv.Atoi(p) if err != nil { return nil, err } - port = p - } - - proto := C.kSecProtocolTypeHTTPS - if u.Scheme != "https" { - proto = C.kSecProtocolTypeHTTP } return &C.struct_Server{ proto: C.SecProtocolType(proto), - host: C.CString(host), + host: C.CString(getHostname(u)), port: C.uint(port), path: C.CString(u.Path), }, nil @@ -168,3 +165,32 @@ func freeServer(s *C.struct_Server) { C.free(unsafe.Pointer(s.host)) C.free(unsafe.Pointer(s.path)) } + +// parseURL parses and validates a given serverURL to an url.URL, and +// returns an error if validation failed. Querystring parameters are +// omitted in the resulting URL, because they are not used in the helper. +// +// If serverURL does not have a valid scheme, `//` is used as scheme +// before parsing. This prevents the hostname being used as path, +// and the credentials being stored without host. +func parseURL(serverURL string) (*url.URL, error) { + // Check if serverURL has a scheme, otherwise add `//` as scheme. + if !strings.Contains(serverURL, "://") && !strings.HasPrefix(serverURL, "//") { + serverURL = "//" + serverURL + } + + u, err := url.Parse(serverURL) + if err != nil { + return nil, err + } + + if u.Scheme != "" && u.Scheme != "https" && u.Scheme != "http" { + return nil, errors.New("unsupported scheme: " + u.Scheme) + } + if getHostname(u) == "" { + return nil, errors.New("no hostname in URL") + } + + u.RawQuery = "" + return u, nil +} diff --git a/osxkeychain/osxkeychain_darwin_test.go b/osxkeychain/osxkeychain_darwin_test.go index 406fe9b..b74927c 100644 --- a/osxkeychain/osxkeychain_darwin_test.go +++ b/osxkeychain/osxkeychain_darwin_test.go @@ -1,6 +1,8 @@ package osxkeychain import ( + "errors" + "fmt" "github.com/docker/docker-credential-helpers/credentials" "testing" ) @@ -54,6 +56,194 @@ func TestOSXKeychainHelper(t *testing.T) { } } +// TestOSXKeychainHelperParseURL verifies that a // "scheme" is added to URLs, +// and that invalid URLs produce an error. +func TestOSXKeychainHelperParseURL(t *testing.T) { + tests := []struct { + url string + expectedURL string + err error + }{ + {url: "foobar.docker.io", expectedURL: "//foobar.docker.io"}, + {url: "foobar.docker.io:2376", expectedURL: "//foobar.docker.io:2376"}, + {url: "//foobar.docker.io:2376", expectedURL: "//foobar.docker.io:2376"}, + {url: "http://foobar.docker.io:2376", expectedURL: "http://foobar.docker.io:2376"}, + {url: "https://foobar.docker.io:2376", expectedURL: "https://foobar.docker.io:2376"}, + {url: "https://foobar.docker.io:2376/some/path", expectedURL: "https://foobar.docker.io:2376/some/path"}, + {url: "https://foobar.docker.io:2376/some/other/path?foo=bar", expectedURL: "https://foobar.docker.io:2376/some/other/path"}, + {url: "/foobar.docker.io", err: errors.New("no hostname in URL")}, + {url: "ftp://foobar.docker.io:2376", err: errors.New("unsupported scheme: ftp")}, + } + + for _, te := range tests { + u, err := parseURL(te.url) + + if te.err == nil && err != nil { + t.Errorf("Error: failed to parse URL %q: %s", te.url, err) + continue + } + if te.err != nil && err == nil { + t.Errorf("Error: expected error %q, got none when parsing URL %q", te.err, te.url) + continue + } + if te.err != nil && err.Error() != te.err.Error() { + t.Errorf("Error: expected error %q, got %q when parsing URL %q", te.err, err, te.url) + continue + } + if u != nil && u.String() != te.expectedURL { + t.Errorf("Error: expected URL: %q, but got %q for URL: %q", te.expectedURL, u.String(), te.url) + } + } +} + +// TestOSXKeychainHelperRetrieveAliases verifies that secrets can be accessed +// through variations on the URL +func TestOSXKeychainHelperRetrieveAliases(t *testing.T) { + tests := []struct { + storeURL string + readURL string + }{ + // stored with port, retrieved without + {"https://foobar.docker.io:2376", "https://foobar.docker.io"}, + + // stored as https, retrieved without scheme + {"https://foobar.docker.io:2376", "foobar.docker.io"}, + + // stored with path, retrieved without + {"https://foobar.docker.io:1234/one/two", "https://foobar.docker.io:1234"}, + } + + helper := Osxkeychain{} + defer func() { + for _, te := range tests { + helper.Delete(te.storeURL) + } + }() + + // Clean store before testing. + for _, te := range tests { + helper.Delete(te.storeURL) + } + + for _, te := range tests { + c := &credentials.Credentials{ServerURL: te.storeURL, Username: "hello", Secret: "world"} + if err := helper.Add(c); err != nil { + t.Errorf("Error: failed to store secret for URL %q: %s", te.storeURL, err) + continue + } + if _, _, err := helper.Get(te.readURL); err != nil { + t.Errorf("Error: failed to read secret for URL %q using %q", te.storeURL, te.readURL) + } + helper.Delete(te.storeURL) + } +} + +// TestOSXKeychainHelperRetrieveStrict verifies that only matching secrets are +// returned. +func TestOSXKeychainHelperRetrieveStrict(t *testing.T) { + tests := []struct { + storeURL string + readURL string + }{ + // stored as https, retrieved using http + {"https://foobar.docker.io:2376", "http://foobar.docker.io:2376"}, + + // stored as http, retrieved using https + {"http://foobar.docker.io:2376", "https://foobar.docker.io:2376"}, + + // same: stored as http, retrieved without a scheme specified (hence, using the default https://) + {"http://foobar.docker.io", "foobar.docker.io:5678"}, + + // non-matching ports + {"https://foobar.docker.io:1234", "https://foobar.docker.io:5678"}, + + // non-matching ports TODO is this desired behavior? The other way round does work + //{"https://foobar.docker.io", "https://foobar.docker.io:5678"}, + + // non-matching paths + {"https://foobar.docker.io:1234/one/two", "https://foobar.docker.io:1234/five/six"}, + } + + helper := Osxkeychain{} + defer func() { + for _, te := range tests { + helper.Delete(te.storeURL) + } + }() + + // Clean store before testing. + for _, te := range tests { + helper.Delete(te.storeURL) + } + + for _, te := range tests { + c := &credentials.Credentials{ServerURL: te.storeURL, Username: "hello", Secret: "world"} + if err := helper.Add(c); err != nil { + t.Errorf("Error: failed to store secret for URL %q: %s", te.storeURL, err) + continue + } + if _, _, err := helper.Get(te.readURL); err == nil { + t.Errorf("Error: managed to read secret for URL %q using %q, but should not be able to", te.storeURL, te.readURL) + } + helper.Delete(te.storeURL) + } +} + +// TestOSXKeychainHelperStoreRetrieve verifies that secrets stored in the +// the keychain can be read back using the URL that was used to store them. +func TestOSXKeychainHelperStoreRetrieve(t *testing.T) { + tests := []struct { + url string + }{ + {url: "foobar.docker.io"}, + {url: "foobar.docker.io:2376"}, + {url: "//foobar.docker.io:2376"}, + {url: "https://foobar.docker.io:2376"}, + {url: "http://foobar.docker.io:2376"}, + {url: "https://foobar.docker.io:2376/some/path"}, + {url: "https://foobar.docker.io:2376/some/other/path"}, + {url: "https://foobar.docker.io:2376/some/other/path?foo=bar"}, + } + + helper := Osxkeychain{} + defer func() { + for _, te := range tests { + helper.Delete(te.url) + } + }() + + // Clean store before testing. + for _, te := range tests { + helper.Delete(te.url) + } + + // Note that we don't delete between individual tests here, to verify that + // subsequent stores/overwrites don't affect storing / retrieving secrets. + for i, te := range tests { + c := &credentials.Credentials{ + ServerURL: te.url, + Username: fmt.Sprintf("user-%d", i), + Secret: fmt.Sprintf("secret-%d", i), + } + + if err := helper.Add(c); err != nil { + t.Errorf("Error: failed to store secret for URL: %s: %s", te.url, err) + continue + } + user, secret, err := helper.Get(te.url) + if err != nil { + t.Errorf("Error: failed to read secret for URL %q: %s", te.url, err) + continue + } + if user != c.Username { + t.Errorf("Error: expected username %s, got username %s for URL: %s", c.Username, user, te.url) + } + if secret != c.Secret { + t.Errorf("Error: expected secret %s, got secret %s for URL: %s", c.Secret, secret, te.url) + } + } +} + func TestMissingCredentials(t *testing.T) { helper := Osxkeychain{} _, _, err := helper.Get("https://adsfasdf.wrewerwer.com/asdfsdddd") diff --git a/osxkeychain/url_go18.go b/osxkeychain/url_go18.go new file mode 100644 index 0000000..0b7297d --- /dev/null +++ b/osxkeychain/url_go18.go @@ -0,0 +1,13 @@ +//+build go1.8 + +package osxkeychain + +import "net/url" + +func getHostname(u *url.URL) string { + return u.Hostname() +} + +func getPort(u *url.URL) string { + return u.Port() +} diff --git a/osxkeychain/url_non_go18.go b/osxkeychain/url_non_go18.go new file mode 100644 index 0000000..bdf9b7b --- /dev/null +++ b/osxkeychain/url_non_go18.go @@ -0,0 +1,41 @@ +//+build !go1.8 + +package osxkeychain + +import ( + "net/url" + "strings" +) + +func getHostname(u *url.URL) string { + return stripPort(u.Host) +} + +func getPort(u *url.URL) string { + return portOnly(u.Host) +} + +func stripPort(hostport string) string { + colon := strings.IndexByte(hostport, ':') + if colon == -1 { + return hostport + } + if i := strings.IndexByte(hostport, ']'); i != -1 { + return strings.TrimPrefix(hostport[:i], "[") + } + return hostport[:colon] +} + +func portOnly(hostport string) string { + colon := strings.IndexByte(hostport, ':') + if colon == -1 { + return "" + } + if i := strings.Index(hostport, "]:"); i != -1 { + return hostport[i+len("]:"):] + } + if strings.Contains(hostport, "]") { + return "" + } + return hostport[colon+len(":"):] +} diff --git a/wincred/wincred_windows_test.go b/wincred/wincred_windows_test.go index 557bcbe..4421fb1 100644 --- a/wincred/wincred_windows_test.go +++ b/wincred/wincred_windows_test.go @@ -1,8 +1,8 @@ package wincred import ( - "testing" "strings" + "testing" "github.com/docker/docker-credential-helpers/credentials" ) @@ -63,7 +63,7 @@ func TestWinCredHelper(t *testing.T) { } auths, err := helper.List() - if err != nil || len(auths) - len(oldauths) != 1 { + if err != nil || len(auths)-len(oldauths) != 1 { t.Fatal(err) }