Skip to content

Fix docker-credential-osxkeychain list behaviour in case of missing entry in keychain - #143

Merged
gtardif merged 1 commit into
docker:masterfrom
pgayvallet:osx-list-no-error-on-missing-key
Apr 29, 2019
Merged

Fix docker-credential-osxkeychain list behaviour in case of missing entry in keychain#143
gtardif merged 1 commit into
docker:masterfrom
pgayvallet:osx-list-no-error-on-missing-key

Conversation

@pgayvallet

Copy link
Copy Markdown
Contributor

Currently, docker-credential-osxkeychain list returns an error if the keychain entry is missing.

However, docker logout actually clears the Docker Credentials keychain entry. Meaning that after performing the logout, the calls to docker-credential-osxkeychain list returns an error.

When building images (at least with docker-compose build, did not try directly with docker build) calls to docker-credential-osxkeychain list are performed, resulting in build failure as the credential helpers returns a non-0 return code.

This is probably not the expected behaviour. Also, this is not the case on windows, as the used implementation from danieljoos/wincred correctly handles the case:

func List() ([]*Credential, error) {
	creds, err := nativeCredEnumerate("", true)
	if err != nil && err.Error() == naERROR_NOT_FOUND {
		// Ignore ERROR_NOT_FOUND and return an empty list instead
		creds = []*Credential{}
		err = nil
	}
	return creds, err
}

This PR changes the docker-credential-osxkeychain list behaviour to return an empty map if the keychain entry is missing.

@GordonTheTurtle

Copy link
Copy Markdown

Please sign your commits following these rules:
https://github.com/moby/moby/blob/master/CONTRIBUTING.md#sign-your-work
The easiest way to do this is to amend the last commit:

$ git clone -b "osx-list-no-error-on-missing-key" git@github.com:pgayvallet/docker-credential-helpers.git somewhere
$ cd somewhere
$ git commit --amend -s --no-edit
$ git push -f

Amending updates the existing PR. You DO NOT need to open a new one.

@pgayvallet
pgayvallet force-pushed the osx-list-no-error-on-missing-key branch from 2a86b82 to 3201db6 Compare April 18, 2019 14:17
Signed-off-by: pgayvallet <pierre.gayvallet@gmail.com>
@pgayvallet
pgayvallet force-pushed the osx-list-no-error-on-missing-key branch from 3201db6 to 1546024 Compare April 26, 2019 16:35

@silvin-lubecki silvin-lubecki left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Seems legit 😸
Could you add a comment in the code with a short explanation ? (just a summary of the one you made in the PR)
Also is there any simple way to make this behavior explicit now in a unit test?

@gtardif gtardif left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Also checked the resulting binary on mac, docker-compose up --build works fine now

@gtardif
gtardif merged commit 74636a1 into docker:master Apr 29, 2019
thaJeztah added a commit to thaJeztah/cli that referenced this pull request Jun 11, 2019
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>
docker-jenkins pushed a commit to docker-archive/docker-ce that referenced this pull request Jun 20, 2019
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
thaJeztah added a commit to thaJeztah/cli that referenced this pull request Jul 4, 2019
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>
thaJeztah added a commit to thaJeztah/cli that referenced this pull request Jul 4, 2019
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>
thaJeztah added a commit to thaJeztah/cli that referenced this pull request Jul 23, 2019
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>
thaJeztah added a commit to thaJeztah/cli that referenced this pull request Aug 8, 2019
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>
docker-jenkins pushed a commit to docker-archive/docker-ce that referenced this pull request Aug 8, 2019
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
docker-jenkins pushed a commit to docker-archive/docker-ce that referenced this pull request Aug 8, 2019
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants