Prevent invalid credentials: no missing server URL or username #62

n4ss · 2017-05-22T09:45:19Z

We should not do operations on credential stores with invalid (empty) server URLs or usernames.

This causes credentials management issues in some platforms' helpers depending on the credentials management system implementation.

This needs to be merged ASAP as next docker version is probably released tomorrow (05/23) morning CET.

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

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

thaJeztah · 2017-05-22T10:32:24Z

credentials/error.go

+// was caused by not having a credentials server URL when
+// required.
+func IsCredentialsMissingServerURLMessage(err string) bool {
+	return err == errCredentialsMissingServerURLMessage


Is there a use for this utility function?

(and the other Is..Message functions

You're right, I forgot to add the error checks in the output logs from credentials store client.

Done in : b69480e

thaJeztah · 2017-05-22T10:32:41Z

credentials/error.go

+// required.
+func IsCredentialsMissingUsernameMessage(err string) bool {
+	return err == errCredentialsMissingUsernameMessage
+}


nit: missing newline at end of file

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

n4ss · 2017-05-22T14:42:23Z

/cc @vdemeester 😇

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

aaronlehmann · 2017-05-22T16:39:30Z

Are empty strings the only value that can trigger this issue? I know that the OS X keychain API returns the first "matching" credential, but I'm not sure whether "matching" means an exact match for all non-empty strings, or if prefix matching is done in some cases.

aaronlehmann · 2017-05-22T16:47:16Z

client/client.go

+// isValidCredsMessage checks if 'msg' contains invalid credentials error message.
+// It returns whether the logs are free of invalid credentials errors and the error if it isn't.
+// error values can be errCredentialsMissingServerURL or errCredentialsMissingUsername.
+func isValidCredsMessage(msg string) (bool, error) {


Can we use a return code, or something similar, to convey this information? I really don't like the idea of baking an exact error string into the "protocol" here.

That was being done before this PR.

You're totally right, I'd certainly prefer a return code too but this should go in another PR imho as I prefer not to alter anything that was here before and just add additional checks to fix the issue asap.

The helpers should use errors from credentials package so I guess that makes sense in the meantime.

we probably could just have this function only return an error instead of (bool, error) - I don't see a use-case for them not matching

n4ss · 2017-05-22T18:32:05Z

@aaronlehmann Regarding your question, when trying to do get creds by the prefix of the stored credentials' serverURL, I got nothing. There is no documentation on the way the attributes are used to match existing keychain objects.

vdemeester

LGTM 🐯

riyazdf · 2017-05-23T21:03:20Z

client/client.go

+// isValidCredsMessage checks if 'msg' contains invalid credentials error message.
+// It returns whether the logs are free of invalid credentials errors and the error if it isn't.
+// error values can be errCredentialsMissingServerURL or errCredentialsMissingUsername.
+func isValidCredsMessage(msg string) (bool, error) {


we probably could just have this function only return an error instead of (bool, error) - I don't see a use-case for them not matching

riyazdf · 2017-05-23T21:18:11Z

client/client.go

 	cmd := program("store")

 	buffer := new(bytes.Buffer)
-	if err := json.NewEncoder(buffer).Encode(credentials); err != nil {
+	if err := json.NewEncoder(buffer).Encode(creds); err != nil {
 		return err
 	}
 	cmd.Input(buffer)

 	out, err := cmd.Output()
 	if err != nil {
 		t := strings.TrimSpace(string(out))


for a followup PR: it might be useful to combine the cmd.Output(), strings.TrimSpace, and isValidCredsMessage parsing into a shared function that all of these exported functions can use.
The same functionality seems to be replicated across Get, Store, Erase, etc.

Indeed, added to my todo list :)

riyazdf · 2017-05-23T21:19:40Z

credentials/credentials.go

@@ -100,13 +120,17 @@ func Get(helper Helper, reader io.Reader, writer io.Writer) error {
 	}

 	serverURL := strings.TrimSpace(buffer.String())
+	if len(serverURL) == 0 {


instead of this individual check, should we instead call resp.isValid() after forming it below? That way we can also verify the username

the Get operation on a credentials store only needs a server URL, username is not needed, as the operation is the following:

I have a repository foobar.io, get me the credentials for this repo so I can authenticate on it

got it. But potentially, the username could create an invalid Credentials struct in the code below right? I would prefer to validate so that any invalid Credentials struct is rejected, in case the semantics of the Get operation change.

This can be a followup since I acknowledge the importance of this fix and that this is not an issue at present - I can help with the PR.

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

thaJeztah · 2017-05-24T10:50:03Z

client/client.go

 		return err
 	}
 	cmd.Input(buffer)

 	out, err := cmd.Output()
 	if err != nil {
 		t := strings.TrimSpace(string(out))
+
+		if err := isValidCredsMessage(t); err != nil {
+			return err


It the error is one of these "special cases", only the bare message is returned, whereas other errors will show "error storing credentials - err: ..." Without that it may be less apparent in what stage the error occurred (storing or retrieving the credentials)

Is there a reason doing so? If we need to have access to the plain error message, perhaps we should consider using errors.Wrap()?

I was actually following the "CredentialsNotFound" error check behavior, a follow-up PR fixing this is ready.

I prefer to have it separately though and not modify existing code.

Are you ok with an additional parameter on errCredentialsMissingUsername and errCredentialsMissingServerURL errors that would include the operation in the error message?

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

thaJeztah

LGTM

ping @vdemeester PTAL if it still LGTY

Also, can you squash some commits @n4ss ?

vdemeester

still LGTM 👼

ashfall

Thanks for giving me enough context to review. :) Looks good to me!

n4ss · 2017-05-29T08:21:34Z

I'm squashing and merging, this will go in 17.06. Thanks all for reviewing.

Prevent invalid credentials: no missing server URL or username

a7bd772

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

n4ss requested review from vdemeester and aaronlehmann May 22, 2017 09:45

thaJeztah reviewed May 22, 2017

View reviewed changes

n4ss added 2 commits May 22, 2017 13:32

Add missing username/serverURL error checks in credstore client

b69480e

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

Clean up doc on invalid creds errors and client's error checks

3b01cf8

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

Add tests for missing ServerURL/Username

bd25f56

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

aaronlehmann reviewed May 22, 2017

View reviewed changes

vdemeester approved these changes May 23, 2017

View reviewed changes

riyazdf reviewed May 23, 2017

View reviewed changes

Clean isValidCredsMessage prototype

8ffd3bd

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

thaJeztah reviewed May 24, 2017

View reviewed changes

Add test for missing server URL and more detailed error msg

a3f2dfd

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

thaJeztah approved these changes May 24, 2017

View reviewed changes

vdemeester approved these changes May 24, 2017

View reviewed changes

ashfall approved these changes May 24, 2017

View reviewed changes

n4ss merged commit ed11c58 into docker:master May 29, 2017

thaJeztah mentioned this pull request May 29, 2017

17.06.0 RC2 tracker docker/for-linux#2

Closed

23 tasks

thaJeztah mentioned this pull request Jun 7, 2017

17.06.0 RC3 tracker docker/for-linux#8

Closed

40 tasks

andrewhsu mentioned this pull request Jun 13, 2017

17.06.0 RC4 tracker docker/for-linux#14

Closed

2 tasks

shin- mentioned this pull request Jun 13, 2017

Error when using registry: first path segment in URL cannot contain colon docker/compose#4924

Closed

andrewhsu mentioned this pull request Jun 19, 2017

17.06.0 RC5 tracker docker/for-linux#37

Closed

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Prevent invalid credentials: no missing server URL or username #62

Prevent invalid credentials: no missing server URL or username #62

n4ss commented May 22, 2017

thaJeztah May 22, 2017 •

edited

Loading

thaJeztah May 22, 2017

n4ss May 22, 2017

thaJeztah May 22, 2017

n4ss May 22, 2017

n4ss commented May 22, 2017

aaronlehmann commented May 22, 2017

aaronlehmann May 22, 2017

n4ss May 22, 2017 •

edited

Loading

riyazdf May 23, 2017

n4ss commented May 22, 2017

vdemeester left a comment

riyazdf May 23, 2017

riyazdf May 23, 2017

n4ss May 24, 2017

riyazdf May 23, 2017

n4ss May 24, 2017

riyazdf May 24, 2017

thaJeztah May 24, 2017

n4ss May 24, 2017

n4ss May 24, 2017

thaJeztah left a comment

vdemeester left a comment

ashfall left a comment

n4ss commented May 29, 2017

Prevent invalid credentials: no missing server URL or username #62

Prevent invalid credentials: no missing server URL or username #62

Conversation

n4ss commented May 22, 2017

thaJeztah May 22, 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 May 22, 2017

aaronlehmann commented May 22, 2017

Choose a reason for hiding this comment

n4ss May 22, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

n4ss commented May 22, 2017

vdemeester left a 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

thaJeztah left a comment

Choose a reason for hiding this comment

vdemeester left a comment

Choose a reason for hiding this comment

ashfall left a comment

Choose a reason for hiding this comment

n4ss commented May 29, 2017

thaJeztah May 22, 2017 •

edited

Loading

n4ss May 22, 2017 •

edited

Loading