diff --git a/osxkeychain/osxkeychain_darwin.go b/osxkeychain/osxkeychain_darwin.go index 43912676..d015182f 100644 --- a/osxkeychain/osxkeychain_darwin.go +++ b/osxkeychain/osxkeychain_darwin.go @@ -10,9 +10,8 @@ package osxkeychain import "C" import ( "errors" - "net/url" + "github.com/docker/docker-credential-helpers/registryurl" "strconv" - "strings" "unsafe" "github.com/docker/docker-credential-helpers/credentials" @@ -135,7 +134,7 @@ func (h Osxkeychain) List() (map[string]string, error) { } func splitServer(serverURL string) (*C.struct_Server, error) { - u, err := parseURL(serverURL) + u, err := registryurl.Parse(serverURL) if err != nil { return nil, err } @@ -145,7 +144,7 @@ func splitServer(serverURL string) (*C.struct_Server, error) { proto = C.kSecProtocolTypeHTTP } var port int - p := getPort(u) + p := registryurl.GetPort(u) if p != "" { port, err = strconv.Atoi(p) if err != nil { @@ -155,7 +154,7 @@ func splitServer(serverURL string) (*C.struct_Server, error) { return &C.struct_Server{ proto: C.SecProtocolType(proto), - host: C.CString(getHostname(u)), + host: C.CString(registryurl.GetHostname(u)), port: C.uint(port), path: C.CString(u.Path), }, nil @@ -165,32 +164,3 @@ 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 b74927cc..dce0d1f2 100644 --- a/osxkeychain/osxkeychain_darwin_test.go +++ b/osxkeychain/osxkeychain_darwin_test.go @@ -1,7 +1,6 @@ package osxkeychain import ( - "errors" "fmt" "github.com/docker/docker-credential-helpers/credentials" "testing" @@ -56,46 +55,6 @@ 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) { diff --git a/osxkeychain/url_go18.go b/osxkeychain/url_go18.go deleted file mode 100644 index 0b7297d2..00000000 --- a/osxkeychain/url_go18.go +++ /dev/null @@ -1,13 +0,0 @@ -//+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/registryurl/parse.go b/registryurl/parse.go new file mode 100644 index 00000000..76171328 --- /dev/null +++ b/registryurl/parse.go @@ -0,0 +1,37 @@ +package registryurl + +import ( + "errors" + "net/url" + "strings" +) + +// Parse 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 Parse(registryURL string) (*url.URL, error) { + // Check if registryURL has a scheme, otherwise add `//` as scheme. + if !strings.Contains(registryURL, "://") && !strings.HasPrefix(registryURL, "//") { + registryURL = "//" + registryURL + } + + u, err := url.Parse(registryURL) + 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/registryurl/parse_test.go b/registryurl/parse_test.go new file mode 100644 index 00000000..9b1e0d54 --- /dev/null +++ b/registryurl/parse_test.go @@ -0,0 +1,46 @@ +package registryurl + +import ( + "errors" + "testing" +) + +// TestHelperParseURL verifies that a // "scheme" is added to URLs, +// and that invalid URLs produce an error. +func TestHelperParseURL(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 := Parse(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) + } + } +} \ No newline at end of file diff --git a/registryurl/url_go18.go b/registryurl/url_go18.go new file mode 100644 index 00000000..dbfbfa1a --- /dev/null +++ b/registryurl/url_go18.go @@ -0,0 +1,15 @@ +//+build go1.8 + +package registryurl + +import ( + url "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/registryurl/url_non_go18.go similarity index 85% rename from osxkeychain/url_non_go18.go rename to registryurl/url_non_go18.go index bdf9b7b0..ee187ab4 100644 --- a/osxkeychain/url_non_go18.go +++ b/registryurl/url_non_go18.go @@ -1,17 +1,17 @@ //+build !go1.8 -package osxkeychain +package registryurl import ( - "net/url" "strings" + url "net/url" ) -func getHostname(u *url.URL) string { +func GetHostname(u *url.URL) string { return stripPort(u.Host) } -func getPort(u *url.URL) string { +func GetPort(u *url.URL) string { return portOnly(u.Host) } diff --git a/wincred/wincred_windows.go b/wincred/wincred_windows.go index 8c3941c5..1a95e004 100644 --- a/wincred/wincred_windows.go +++ b/wincred/wincred_windows.go @@ -2,6 +2,8 @@ package wincred import ( "bytes" + "github.com/docker/docker-credential-helpers/registryurl" + "net/url" "strings" winc "github.com/danieljoos/wincred" @@ -37,10 +39,18 @@ func (h Wincred) Delete(serverURL string) error { // Get retrieves credentials from the windows credentials manager. func (h Wincred) Get(serverURL string) (string, string, error) { - g, _ := winc.GetGenericCredential(serverURL) + target, err := getTarget(serverURL) + if err != nil { + return "", "", err + } else if target == "" { + return "", "", credentials.NewErrCredentialsNotFound() + } + + g, _ := winc.GetGenericCredential(target) if g == nil { return "", "", credentials.NewErrCredentialsNotFound() } + for _, attr := range g.Attributes { if strings.Compare(attr.Keyword, "label") == 0 && bytes.Compare(attr.Value, []byte(credentials.CredsLabel)) == 0 { @@ -51,6 +61,72 @@ func (h Wincred) Get(serverURL string) (string, string, error) { return "", "", credentials.NewErrCredentialsNotFound() } +func getTarget(serverURL string) (string, error) { + s, err := registryurl.Parse(serverURL) + if err != nil { + return serverURL, nil + } + + creds, err := winc.List() + if err != nil { + return "", err + } + + targets := make([]string, 0) + for i := range creds { + attrs := creds[i].Attributes + for _, attr := range attrs { + if strings.Compare(attr.Keyword, "label") == 0 && + bytes.Compare(attr.Value, []byte(credentials.CredsLabel)) == 0 { + targets = append(targets, creds[i].TargetName) + } + } + } + + if target, found := findMatch(s, targets, exactMatch); found { + return target, nil + } + + if target, found := findMatch(s, targets, approximateMatch); found { + return target, nil + } + + return "", nil +} + +func findMatch(serverUrl *url.URL, targets []string, matches func(url.URL, url.URL) bool) (string, bool) { + for _, target := range targets { + tURL, err := registryurl.Parse(target) + if err != nil { + continue + } + if matches(*serverUrl, *tURL) { + return target, true + } + } + return "", false +} + +func exactMatch(serverURL, target url.URL) bool { + return serverURL.String() == target.String() +} + +func approximateMatch(serverURL, target url.URL) bool { + //if scheme is missing assume it is the same as target + if serverURL.Scheme == "" { + serverURL.Scheme = target.Scheme + } + //if port is missing assume it is the same as target + if serverURL.Port() == "" && target.Port() != "" { + serverURL.Host = serverURL.Host + ":" + target.Port() + } + //if path is missing assume it is the same as target + if serverURL.Path == "" { + serverURL.Path = target.Path + } + return serverURL.String() == target.String() +} + // List returns the stored URLs and corresponding usernames for a given credentials label. func (h Wincred) List() (map[string]string, error) { creds, err := winc.List() diff --git a/wincred/wincred_windows_test.go b/wincred/wincred_windows_test.go index 4421fb1e..68d293af 100644 --- a/wincred/wincred_windows_test.go +++ b/wincred/wincred_windows_test.go @@ -1,6 +1,7 @@ package wincred import ( + "fmt" "strings" "testing" @@ -86,6 +87,154 @@ func TestWinCredHelper(t *testing.T) { } } +// TestWinCredHelperRetrieveAliases verifies that secrets can be accessed +// through variations on the URL +func TestWinCredHelperRetrieveAliases(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", "foobar.docker.io"}, + + // stored with path, retrieved without + {"https://foobar.docker.io/one/two", "https://foobar.docker.io"}, + } + + helper := Wincred{} + 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) + } +} + +// TestWinCredHelperRetrieveStrict verifies that only matching secrets are +// returned. +func TestWinCredHelperRetrieveStrict(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 := Wincred{} + 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) + } +} + +// TestWinCredHelperStoreRetrieve verifies that secrets stored in the +// the keychain can be read back using the URL that was used to store them. +func TestWinCredHelperStoreRetrieve(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 := Wincred{} + 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 := Wincred{} _, _, err := helper.Get("https://adsfasdf.wrewerwer.com/asdfsdddd")