Skip to content

Commit

Permalink
Fix storing URLs without scheme
Browse files Browse the repository at this point in the history 
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 <github@gone.nl>
  • Loading branch information
@thaJeztah
thaJeztah committed Jun 14, 2017
1 parent d8f57a1 commit 3e8a573
Show file tree
Hide file tree
Showing 2 changed files with 184 additions and 13 deletions.
44 changes: 31 additions & 13 deletions osxkeychain/osxkeychain_darwin.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,8 @@ import "C"
import (
"errors"
"net/url"
"regexp"
"strconv"
"strings"
"unsafe"

"github.com/docker/docker-credential-helpers/credentials"
Expand Down Expand Up @@ -135,30 +135,26 @@ 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])
if u.Port() != "" {
port, err = strconv.Atoi(u.Port())
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(u.Hostname()),
port: C.uint(port),
path: C.CString(u.Path),
}, nil
Expand All @@ -168,3 +164,25 @@ 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
func parseURL(serverURL string) (*url.URL, error) {
if !regexp.MustCompile(`^([a-zA-Z][-+.a-zA-Z0-9]+:)?//`).MatchString(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 u.Hostname() == "" {
return nil, errors.New("no hostname in URL")
}

u.RawQuery = ""
return u, nil
}
153 changes: 153 additions & 0 deletions osxkeychain/osxkeychain_darwin_test.go
View file
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
package osxkeychain

import (
"errors"
"fmt"
"github.com/docker/docker-credential-helpers/credentials"
"testing"
)
Expand Down Expand Up @@ -54,6 +56,157 @@ 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 parsing to fail for URL %q", 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"},
}

helper := Osxkeychain{}
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. Secrets
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://)
// TODO is this desired behavior?
{"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{}
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{}
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)
}
}
for _, te := range tests {
helper.Delete(te.url)
}
}

func TestMissingCredentials(t *testing.T) {
helper := Osxkeychain{}
_, _, err := helper.Get("https://adsfasdf.wrewerwer.com/asdfsdddd")
Expand Down

0 comments on commit 3e8a573

Please sign in to comment.