make docker-credential-wincred work like docker-credential-osxkeychain by ekcasey · Pull Request #139 · docker/docker-credential-helpers

ekcasey · 2019-04-03T20:47:35Z

fetch credentials for server with matching hostname if scheme, path, or port are not provided
if the credential request includes specific scheme, path, or port that does not match entry, don't return
extract url helpers into a package

Signed-off-by: Danny Joyce djoyce@pivotal.io
Signed-off-by: Emily Casey ecasey@pivotal.io

ekcasey · 2019-04-03T20:50:32Z

This fixes issues fetching docker credentials in libraries like https://github.com/google/go-containerregistry that resolve Docker Hub registry url differently (https://index.docker.io vs https://index.docker.io/v1)

ekcasey · 2019-04-04T14:29:20Z

@vdemeester It looks like the Travis build is broken for reasons unrelated to our changes. If that is not a correct diagnosis, let @djoyahoy and I know so we fix it :)

guillaumerose · 2019-04-27T07:52:41Z

CI is now fixed. Can you rebase ? Thanks

* fetch credentials for server with matching hostname if scheme, path, or port are not provided * if the credential request includes specific scheme, path, or port that does not match entry, don't return * extract url helpers into a package Signed-off-by: Emily Casey <ecasey@pivotal.io> Signed-off-by: Danny Joyce <djoyce@pivotal.io>

ekcasey · 2019-04-29T13:31:56Z

@guillaumerose done!

silvin-lubecki · 2019-04-29T13:42:00Z

 import (
 	"errors"
-	"net/url"
+	"github.com/docker/docker-credential-helpers/registryurl"


nit picking: Can you re-order the imports? goimports should do that for you 😺

silvin-lubecki · 2019-04-29T14:01:30Z


 import (
 	"bytes"
+	"github.com/docker/docker-credential-helpers/registryurl"


nit: please re-organize the imports here too

silvin-lubecki · 2019-04-29T14:03:32Z

+func getTarget(serverURL string) (string, error) {
+	s, err := registryurl.Parse(serverURL)
+	if err != nil {
+		return serverURL, nil


nit: return empty string here instead?

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.

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

Signed-off-by: Emily Casey <ecasey@pivotal.io>

silvin-lubecki · 2019-04-29T14:04:55Z

+		return "", err
+	}
+
+	targets := make([]string, 0)


nit: var targets []string

silvin-lubecki · 2019-04-29T14:10:47Z

+	for i := range creds {
+		attrs := creds[i].Attributes
+		for _, attr := range attrs {
+			if strings.Compare(attr.Keyword, "label") == 0 &&


From the documentation: https://golang.org/pkg/strings/#Compare

Compare is included only for symmetry with package bytes. It is usually clearer and always faster to use the built-in string comparison operators ==, <, >, and so on.

So I guess if attr.Keyword == "label" is simpler 😸

silvin-lubecki · 2019-04-29T14:16:11Z

+		attrs := creds[i].Attributes
+		for _, attr := range attrs {
+			if strings.Compare(attr.Keyword, "label") == 0 &&
+				bytes.Compare(attr.Value, []byte(credentials.CredsLabel)) == 0 {


You should use bytes.Equal instead.
What is the real type behind attr.Value? Is it an arbitrary byte slice? Or always a string encoded into bytes? My point is: even if the result is the same, I think it's more clearer to cast attr.Value as string instead.

This piece of code existed before our PR so weren't sure if attr.Value needed to be a byte slice for a specific reason, happy to make the changes though.

silvin-lubecki · 2019-04-29T14:23:22Z

+		}
+	}
+
+	if target, found := findMatch(s, targets, exactMatch); found {


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

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.

Sounds good 😄 Ok then 👍

Signed-off-by: Emily Casey <ecasey@pivotal.io> Signed-off-by: Danny Joyce <djoyce@pivotal.io>

silvin-lubecki

LGTM, thank you @ekcasey for the fixes 👍

vdemeester

LGTM 🐅

ekcasey · 2019-04-30T13:19:40Z

Thanks everybody!

full diff: docker/docker-credential-helpers@5241b46...8a9f93a includes: - docker/docker-credential-helpers#29 C.free(unsafe.Pointer(err)) -> C.g_error_free(err) - docker/docker-credential-helpers#124 pass: changed the way for checking if password-store is initalized - addresses docker/docker-credential-helpers#133 docker-credential-pass commits about 10 times every time I run a docker command - docker/docker-credential-helpers#143 Fix docker-credential-osxkeychain list behaviour in case of missing entry in keychain - docker/docker-credential-helpers#139 make docker-credential-wincred work like docker-credential-osxkeychain Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

full diff: docker/docker-credential-helpers@5241b46...8a9f93a includes: - docker/docker-credential-helpers#29 C.free(unsafe.Pointer(err)) -> C.g_error_free(err) - docker/docker-credential-helpers#124 pass: changed the way for checking if password-store is initalized - addresses docker/docker-credential-helpers#133 docker-credential-pass commits about 10 times every time I run a docker command - docker/docker-credential-helpers#143 Fix docker-credential-osxkeychain list behaviour in case of missing entry in keychain - docker/docker-credential-helpers#139 make docker-credential-wincred work like docker-credential-osxkeychain Signed-off-by: Sebastiaan van Stijn <github@gone.nl> Upstream-commit: f6a4c76fbb6d594f4874dea5059e82eadfae867a Component: cli

full diff: docker/docker-credential-helpers@5241b46...8a9f93a includes: - docker/docker-credential-helpers#29 C.free(unsafe.Pointer(err)) -> C.g_error_free(err) - docker/docker-credential-helpers#124 pass: changed the way for checking if password-store is initalized - addresses docker/docker-credential-helpers#133 docker-credential-pass commits about 10 times every time I run a docker command - docker/docker-credential-helpers#143 Fix docker-credential-osxkeychain list behaviour in case of missing entry in keychain - docker/docker-credential-helpers#139 make docker-credential-wincred work like docker-credential-osxkeychain Signed-off-by: Sebastiaan van Stijn <github@gone.nl> (cherry picked from commit f6a4c76) Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

full diff: docker/docker-credential-helpers@5241b46...8a9f93a includes: - docker/docker-credential-helpers#29 C.free(unsafe.Pointer(err)) -> C.g_error_free(err) - docker/docker-credential-helpers#124 pass: changed the way for checking if password-store is initalized - addresses docker/docker-credential-helpers#133 docker-credential-pass commits about 10 times every time I run a docker command - docker/docker-credential-helpers#143 Fix docker-credential-osxkeychain list behaviour in case of missing entry in keychain - docker/docker-credential-helpers#139 make docker-credential-wincred work like docker-credential-osxkeychain Signed-off-by: Sebastiaan van Stijn <github@gone.nl> (cherry picked from commit f6a4c76fbb6d594f4874dea5059e82eadfae867a) Signed-off-by: Sebastiaan van Stijn <github@gone.nl> Upstream-commit: 24dcc56123634e048dc0c7d8fa2eeff65b97a33a Component: cli

full diff: docker/docker-credential-helpers@5241b46...8a9f93a includes: - docker/docker-credential-helpers#29 C.free(unsafe.Pointer(err)) -> C.g_error_free(err) - docker/docker-credential-helpers#124 pass: changed the way for checking if password-store is initalized - addresses docker/docker-credential-helpers#133 docker-credential-pass commits about 10 times every time I run a docker command - docker/docker-credential-helpers#143 Fix docker-credential-osxkeychain list behaviour in case of missing entry in keychain - docker/docker-credential-helpers#139 make docker-credential-wincred work like docker-credential-osxkeychain Signed-off-by: Sebastiaan van Stijn <github@gone.nl> (cherry picked from commit f6a4c76fbb6d594f4874dea5059e82eadfae867a) Signed-off-by: Sebastiaan van Stijn <github@gone.nl> Upstream-commit: 11b15544c503d91d8ea3ba782d30d2070a24d84e Component: cli

ekcasey mentioned this pull request Apr 3, 2019

Unable to --publish to private registry on Windows buildpacks/pack#109

Closed

ekcasey force-pushed the server-alias-wincred branch from fe0ee73 to 6f4b0a7 Compare April 29, 2019 13:27

silvin-lubecki reviewed Apr 29, 2019

View reviewed changes

Fix imports

a3c1b5b

Signed-off-by: Emily Casey <ecasey@pivotal.io>

silvin-lubecki reviewed Apr 29, 2019

View reviewed changes

Style fixes - incorporates PR feedback

77e30bd

Signed-off-by: Emily Casey <ecasey@pivotal.io> Signed-off-by: Danny Joyce <djoyce@pivotal.io>

silvin-lubecki approved these changes Apr 29, 2019

View reviewed changes

vdemeester approved these changes Apr 30, 2019

View reviewed changes

vdemeester merged commit 152d643 into docker:master Apr 30, 2019

jonjohnsonjr mentioned this pull request May 22, 2019

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

Closed

thaJeztah mentioned this pull request Jun 11, 2019

bump docker-credential-helpers v0.6.2 docker/cli#1935

Merged

This was referenced Jul 4, 2019

[19.03 backport] bump docker-credential-helpers v0.6.3 docker/cli#1983

Merged

[18.09 backport] bump docker-credential-helpers v0.6.3 docker/cli#1984

Merged

Uh oh!

Conversation

ekcasey commented Apr 3, 2019

Uh oh!

ekcasey commented Apr 3, 2019

Uh oh!

ekcasey commented Apr 4, 2019

Uh oh!

guillaumerose commented Apr 27, 2019

Uh oh!

ekcasey commented Apr 29, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

silvin-lubecki left a comment

Choose a reason for hiding this comment

Uh oh!

vdemeester left a comment

Choose a reason for hiding this comment

Uh oh!

ekcasey commented Apr 30, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants