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.

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

make docker-credential-wincred work like docker-credential-osxkeychain #139

Merged
merged 3 commits into from
Apr 30, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
38 changes: 4 additions & 34 deletions osxkeychain/osxkeychain_darwin.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,12 +10,11 @@ package osxkeychain
import "C"
import (
"errors"
"net/url"
"strconv"
"strings"
"unsafe"

"github.com/docker/docker-credential-helpers/credentials"
"github.com/docker/docker-credential-helpers/registryurl"
)

// errCredentialsNotFound is the specific error message returned by OS X
Expand Down Expand Up @@ -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
}
Expand All @@ -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 {
Expand All @@ -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
Expand All @@ -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
}
44 changes: 2 additions & 42 deletions osxkeychain/osxkeychain_darwin_test.go
Original file line number Diff line number Diff line change
@@ -1,10 +1,10 @@
package osxkeychain

import (
"errors"
"fmt"
"github.com/docker/docker-credential-helpers/credentials"
"testing"

"github.com/docker/docker-credential-helpers/credentials"
)

func TestOSXKeychainHelper(t *testing.T) {
Expand Down Expand Up @@ -56,46 +56,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) {
Expand Down
13 changes: 0 additions & 13 deletions osxkeychain/url_go18.go

This file was deleted.

37 changes: 37 additions & 0 deletions registryurl/parse.go
Original file line number Diff line number Diff line change
@@ -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
}
46 changes: 46 additions & 0 deletions registryurl/parse_test.go
Original file line number Diff line number Diff line change
@@ -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)
}
}
}
15 changes: 15 additions & 0 deletions registryurl/url_go18.go
Original file line number Diff line number Diff line change
@@ -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()
}
8 changes: 4 additions & 4 deletions osxkeychain/url_non_go18.go → registryurl/url_non_go18.go
Original file line number Diff line number Diff line change
@@ -1,17 +1,17 @@
//+build !go1.8

package osxkeychain
package registryurl

import (
"net/url"
url "net/url"
"strings"
)

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)
}

Expand Down
77 changes: 76 additions & 1 deletion wincred/wincred_windows.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,12 @@ package wincred

import (
"bytes"
"net/url"
"strings"

winc "github.com/danieljoos/wincred"
"github.com/docker/docker-credential-helpers/credentials"
"github.com/docker/docker-credential-helpers/registryurl"
)

// Wincred handles secrets using the Windows credential service.
Expand Down Expand Up @@ -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 {
Expand All @@ -51,6 +61,71 @@ 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
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: return empty string here instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We returned the serverURL here b/c before we made our changes there was never a requirement that the url had to parse successfully (you could still fetch an exact match from the cred store). Happy to change this if you want to make this a requirement.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh right read it too fast, you return a nil error. That's fine then 👍

}

creds, err := winc.List()
if err != nil {
return "", err
}

var targets []string
for i := range creds {
attrs := creds[i].Attributes
for _, attr := range attrs {
if attr.Keyword == "label" && bytes.Equal(attr.Value, []byte(credentials.CredsLabel)) {
targets = append(targets, creds[i].TargetName)
}
}
}

if target, found := findMatch(s, targets, exactMatch); found {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this really needed? Can't we just match with approximation? What is the targets length ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We assumed that if there were both an exact and an approximate match in the cred store, the exact match would be preferable. We can take the exact match out if you disagree.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds good 😄 Ok then 👍

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()
Expand Down
Loading