Fix storing URLs without scheme #72

thaJeztah · 2017-06-14T10:54:12Z

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.

thaJeztah · 2017-06-14T10:55:54Z

osxkeychain/osxkeychain_darwin_test.go

+		{"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?


Wasn't sure about this one

This one looks good to me

thaJeztah · 2017-06-14T10:56:01Z

osxkeychain/osxkeychain_darwin_test.go

+		{"https://foobar.docker.io:1234", "https://foobar.docker.io:5678"},
+
+		// non-matching ports
+		// TODO is this desired behavior? The other way round does work


Not sure here either, I'd drop this test.

thaJeztah · 2017-06-14T10:57:11Z

osxkeychain/osxkeychain_darwin.go

+// 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) {


Splitted this to a separate function, so that we can test independently 😄

thaJeztah · 2017-06-14T10:58:05Z

osxkeychain/osxkeychain_darwin.go

+		return nil, errors.New("no hostname in URL")
+	}
+
+	u.RawQuery = ""


Explicitly strip query-string, because we're not storing it, but let me know if you don't want this 👍

Very good catch!

thaJeztah · 2017-06-14T11:17:17Z

osxkeychain/osxkeychain_darwin_test.go

+		{"https://foobar.docker.io:2376", "foobar.docker.io"},
+
+		// stored with path, retrieved without
+		// TODO is this the desired behavior?


Wasn't sure about this either

Path is not kept in the keychain, this is the desired behavior AAIK 👍

n4ss · 2017-06-14T11:35:42Z

osxkeychain/osxkeychain_darwin.go

+// 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) {


nit: can we add a comment to know what this regex checks?

Yup, makes sense

n4ss · 2017-06-14T11:37:12Z

osxkeychain/osxkeychain_darwin.go

+		return nil, errors.New("no hostname in URL")
+	}
+
+	u.RawQuery = ""


Very good catch!

n4ss · 2017-06-14T11:37:49Z

osxkeychain/osxkeychain_darwin.go

 	}

 	return &C.struct_Server{
 		proto: C.SecProtocolType(proto),
-		host:  C.CString(host),
+		host:  C.CString(u.Hostname()),


Is u.Hostname() different from u.Host ?

u.Hostname() strips the port; https://golang.org/src/net/url/url.go?s=28154:28185#L997

n4ss · 2017-06-14T11:42:17Z

osxkeychain/osxkeychain_darwin_test.go

+		{"https://foobar.docker.io:2376", "foobar.docker.io"},
+
+		// stored with path, retrieved without
+		// TODO is this the desired behavior?


Path is not kept in the keychain, this is the desired behavior AAIK 👍

n4ss · 2017-06-14T11:52:46Z

osxkeychain/osxkeychain_darwin_test.go

+			continue
+		}
+		if te.err != nil && err == nil {
+			t.Errorf("Error: expected parsing to fail for URL %q", te.url)


Can we include the expected error message of te.err?

n4ss · 2017-06-14T11:58:39Z

osxkeychain/osxkeychain_darwin_test.go

+		{"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?


This one looks good to me

n4ss · 2017-06-14T11:59:05Z

osxkeychain/osxkeychain_darwin_test.go

+		{"https://foobar.docker.io:1234", "https://foobar.docker.io:5678"},
+
+		// non-matching ports
+		// TODO is this desired behavior? The other way round does work


Not sure here either, I'd drop this test.

n4ss · 2017-06-14T12:02:47Z

osxkeychain/osxkeychain_darwin_test.go

+			Secret:    fmt.Sprintf("secret-%d", i),
+		}
+
+		if err := helper.Add(c); err != nil {


helper.Delete before testing and defer helper.Delete in order to remove all the added credentials, otherwise it generates false positives/negatives with creds from previous tests.

I did comment backward for this one sorry, added references to other spots where it should be done too.

For future reference; this was addressed through this part above;

// Clean store before testing. for _, te := range tests { helper.Delete(te.url) }

n4ss · 2017-06-14T12:03:21Z

osxkeychain/osxkeychain_darwin_test.go

+	helper := Osxkeychain{}
+	for _, te := range tests {
+		c := &credentials.Credentials{ServerURL: te.storeURL, Username: "hello", Secret: "world"}
+		if err := helper.Add(c); err != nil {


Same thing here for the Delete -> Add -> defer Delete

n4ss · 2017-06-14T12:03:29Z

osxkeychain/osxkeychain_darwin_test.go

+	helper := Osxkeychain{}
+	for _, te := range tests {
+		c := &credentials.Credentials{ServerURL: te.storeURL, Username: "hello", Secret: "world"}
+		if err := helper.Add(c); err != nil {


Same thing here for the Delete -> Add -> defer Delete

n4ss · 2017-06-14T12:52:21Z

LGTM! travisCI is failing because it compiles with Golang 1.6, I don't have the rights to update I think.

@vdemeester can you review this please? ^_^

n4ss · 2017-06-14T13:16:02Z

Follow up issue on this on the CLI side: docker/cli#190

We now need to fix the behavior of stripping the protocol when a user adds it to the URL.

Since we'll default to HTTPS, they'll have explicitly specify protocol: docker login http://... to use an insecure connection.

vdemeester

LGTM 👍

thaJeztah · 2017-06-14T16:46:25Z

Hm, looks like it fails on Go 1.7 as well; were these options added in 1.8?

ok  	github.com/docker/docker-credential-helpers/credentials	0.224s
# github.com/docker/docker-credential-helpers/osxkeychain
osxkeychain/osxkeychain_darwin.go:148: u.Port undefined (type *url.URL has no field or method Port)
osxkeychain/osxkeychain_darwin.go:149: u.Port undefined (type *url.URL has no field or method Port)
osxkeychain/osxkeychain_darwin.go:157: u.Hostname undefined (type *url.URL has no field or method Hostname)
osxkeychain/osxkeychain_darwin.go:188: u.Hostname undefined (type *url.URL has no field or method Hostname)
# github.com/docker/docker-credential-helpers/osxkeychain
osxkeychain/osxkeychain_darwin.go:148: u.Port undefined (type *url.URL has no field or method Port)
osxkeychain/osxkeychain_darwin.go:149: u.Port undefined (type *url.URL has no field or method Port)
osxkeychain/osxkeychain_darwin.go:157: u.Hostname undefined (type *url.URL has no field or method Hostname)
osxkeychain/osxkeychain_darwin.go:188: u.Hostname undefined (type *url.URL has no field or method Hostname)
FAIL	github.com/docker/docker-credential-helpers/osxkeychain [build failed]
make: *** [test] Error 2

thaJeztah · 2017-06-14T16:48:38Z

Ugh looks like it was added in Go 1.8 https://golang.org/doc/go1.8

dnephin · 2017-06-14T21:46:05Z

osxkeychain/osxkeychain_darwin.go

+	}
+
+	u.RawQuery = ""
+	return u, nil


Instead of striping the query, and checking the scheme above what do you think about returning a different struct here instead of url.Url. It seems we're not really using much of the url.

type url struct { proto string host string port string path string }

We're using the Port() and Hostname() methods

Right, but those values could be written to this new struct before returning. Either way works fine.

I was thinking of that first, but decided to keep it simple

dnephin · 2017-06-14T21:46:07Z

osxkeychain/osxkeychain_darwin.go

+	// Without a scheme, the hostname is seen as "path" by `url.Parse()`
+	if !regexp.MustCompile(`^([a-zA-Z][-+.a-zA-Z0-9]+:)?//`).MatchString(serverURL) {
+		serverURL = "//" + serverURL
+	}


I think this could call url.Parse() and if u.Scheme == "", then prepend // and call it again on the new string.

Oh, funny, that was my original proposal; #55 (comment) / https://play.golang.org/p/uSIHiJ55ir

tiborvass · 2017-06-14T21:48:30Z

osxkeychain/osxkeychain_darwin.go

+func parseURL(serverURL string) (*url.URL, error) {
+	// Check if serverURL has a valid scheme, otherwise add `//` as scheme.
+	// Without a scheme, the hostname is seen as "path" by `url.Parse()`
+	if !regexp.MustCompile(`^([a-zA-Z][-+.a-zA-Z0-9]+:)?//`).MatchString(serverURL) {


no biggie, but if you update the PR, you could put this mustcompile in a package level var

tiborvass · 2017-06-14T21:53:01Z

osxkeychain/osxkeychain_darwin.go

 	var port int
-	if len(hostAndPort) == 2 {
-		p, err := strconv.Atoi(hostAndPort[1])
+	if u.Port() != "" {


These go1.8 Port() and Hostname() methods are only useful for supporting ipv6, based on what I've read from their code. We could argue it's out of scope for this PR. Or we should provide pre-1.8 functions with !go1.8 buildtags

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>

Signed-off-by: Tibor Vass <teabee89@gmail.com>

tiborvass · 2017-06-14T22:46:57Z

ping @dnephin @thaJeztah

thaJeztah · 2017-06-14T23:00:28Z

osxkeychain/osxkeychain_darwin.go

+	}
+	// Check if serverURL has a valid scheme, otherwise add `//` as scheme.
+	// Without a scheme, the hostname is seen as "path" by `url.Parse()`
+	if u.Scheme == "" {


This may not actually work; localhost:443 will be parsed as scheme: localhost, host: 443; https://play.golang.org/p/QRE7pqxjx1

To simplify, we can use

if !strings.Contains(serverURL, "://") && !strings.HasPrefix(serverURL, "//") { serverURL = "//" + serverURL }

Sounds good

tiborvass · 2017-06-15T00:21:34Z

Another note: this should be a fix for all OSes and not specific to OSX. Fixing.

n4ss · 2017-06-15T07:09:36Z

@tiborvass at the wrapper level? Then, we'll have to warn every folk implementing a credential helper.

Signed-off-by: Nassim 'Nass' Eddequiouaq <eddequiouaq.nassim@gmail.com>

tiborvass · 2017-06-15T08:36:24Z

LGTM

thaJeztah · 2017-06-15T08:40:01Z

osxkeychain/osxkeychain_darwin.go

 	"unsafe"

 	"github.com/docker/docker-credential-helpers/credentials"
+	"strings"


Is this correct?

According to gofmt it is :/

Ah, I guess it's because docker/docker follows the goimport conventions, in which case they are grouped, and sorted, but gofmt won't complain if they were not grouped.

gofmt follows a simple rule, which is to sort each group of imports,
where different groups are separated by a blank line.

goimports also follows a simple rule, which is to put stdlib imports
in one group and non-stdlib imports in a different group.

thaJeztah · 2017-06-15T08:41:42Z

osxkeychain/osxkeychain_darwin.go

+// 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 valid scheme, otherwise add `//` as scheme.


can you remove "valid" here in the comment? This is just a detection is a "possible" scheme is present

Signed-off-by: Nassim 'Nass' Eddequiouaq <eddequiouaq.nassim@gmail.com>

thaJeztah · 2017-06-15T08:49:50Z

osxkeychain/osxkeychain_darwin_test.go

@@ -1,6 +1,8 @@
 package osxkeychain

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


hm, not sure what formatting is used in this repo, but ideally would be;

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

Just a nit, so not a blocker

I'll investigate that for a next PR :)

thaJeztah

one tiny nit, but LGTM

dnephin · 2017-06-15T18:11:14Z

Someone just linked me this: https://github.com/goware/urlx

I think this is exactly what we want for the url parsing code.

thaJeztah · 2017-06-15T19:31:44Z

@dnephin that looks neat indeed. Only issue I see is that it defaults to http, but perhaps we can make a proposal to make it configurable, or have a function that allows passing in defaults for missing components

thaJeztah mentioned this pull request Jun 14, 2017

Force serverURL to have https protocol #55

Closed

thaJeztah commented Jun 14, 2017

View reviewed changes

thaJeztah force-pushed the fix-missing-scheme branch from 3e8a573 to de83797 Compare June 14, 2017 11:16

thaJeztah commented Jun 14, 2017

View reviewed changes

vdemeester requested review from vdemeester and n4ss June 14, 2017 11:23

n4ss reviewed Jun 14, 2017

View reviewed changes

thaJeztah force-pushed the fix-missing-scheme branch 2 times, most recently from 8d249de to 42d4782 Compare June 14, 2017 12:46

vdemeester approved these changes Jun 14, 2017

View reviewed changes

thaJeztah force-pushed the fix-missing-scheme branch from 42d4782 to 9fc60eb Compare June 14, 2017 15:33

thaJeztah force-pushed the fix-missing-scheme branch from 9fc60eb to 181227e Compare June 14, 2017 20:09

dnephin reviewed Jun 14, 2017

View reviewed changes

tiborvass reviewed Jun 14, 2017

View reviewed changes

thaJeztah and others added 2 commits June 14, 2017 15:45

Have pre go1.8 compiler able to compile

7cfc1fc

Signed-off-by: Tibor Vass <teabee89@gmail.com>

tiborvass force-pushed the fix-missing-scheme branch from 934f22e to 7cfc1fc Compare June 14, 2017 22:45

thaJeztah commented Jun 14, 2017

View reviewed changes

Fix URL parsing with port and no scheme

add4510

Signed-off-by: Nassim 'Nass' Eddequiouaq <eddequiouaq.nassim@gmail.com>

thaJeztah commented Jun 15, 2017

View reviewed changes

Improve parseURL comment

992e545

Signed-off-by: Nassim 'Nass' Eddequiouaq <eddequiouaq.nassim@gmail.com>

thaJeztah commented Jun 15, 2017

View reviewed changes

vdemeester approved these changes Jun 15, 2017

View reviewed changes

thaJeztah commented Jun 15, 2017

View reviewed changes

n4ss merged commit e1d4c01 into docker:master Jun 15, 2017

thaJeztah deleted the fix-missing-scheme branch June 15, 2017 09:15

This was referenced Jun 16, 2017

Docker registry Keychain credentials have weird "Where" value docker/for-mac#1721

Closed

OSX Keychain storage behavior in Keychain Access is strange #14

Closed

Mac 0.5.1 fails to parse host that begin with a number if port is specified #71

Closed

jonjohnsonjr mentioned this pull request May 22, 2019

Add fallback Authorization without protocol google/go-containerregistry#456

Closed

thaJeztah mentioned this pull request Apr 11, 2023

Inconsistent auth keys on macOS, Windows, Linux #256

Open

Fix storing URLs without scheme #72

Fix storing URLs without scheme #72

Conversation

thaJeztah commented Jun 14, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

n4ss Jun 14, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

n4ss commented Jun 14, 2017

n4ss commented Jun 14, 2017

vdemeester left a comment

Choose a reason for hiding this comment

thaJeztah commented Jun 14, 2017

thaJeztah commented Jun 14, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dnephin Jun 14, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tiborvass commented Jun 14, 2017

Choose a reason for hiding this comment

thaJeztah Jun 14, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tiborvass commented Jun 15, 2017

n4ss commented Jun 15, 2017

tiborvass commented Jun 15, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thaJeztah left a comment

Choose a reason for hiding this comment

dnephin commented Jun 15, 2017

thaJeztah commented Jun 15, 2017

n4ss Jun 14, 2017 •

edited

Loading

dnephin Jun 14, 2017 •

edited

Loading

thaJeztah Jun 14, 2017 •

edited

Loading