Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reorder priority for retrieving auth from Docker config (~/.docker/config.json) #1958

Merged
merged 4 commits into from
Sep 9, 2019

Conversation

chanseokoh
Copy link
Member

@chanseokoh chanseokoh commented Sep 5, 2019

Fixes #1819.

This is actually more than that. The priority is now credHelpers --> credsStore --> auths. According to moby/moby#25851 (and assuming it wasn't reverted), looks like at least credHelpers should take precedence over credsStore:

This change adds a "credentialHelpers" member to the config file where users can specify credential helpers on a per-registry basis. This does not over-load the "credsStore" field, which now acts as the 'default' credential store in the event that one is not explicitly specified for the registry in question.

Also, I've lifted the restriction that there should be a matching auths registry entry for credsStore to be returned. The practice to define an empty auths entry is documented in here (but it dropped the credsStore support in 2.0 anyways.) My guess is that the previous practice is attributed to a quick workaround to prevent a failure in certain circumstances: awslabs/amazon-ecr-credential-helper#9 (comment) Nonetheless, it seems to me the workaround is no longer relevant anymore with this change in recent Docker versions where Docker queries the credential helper to populate a list of supported registries instead of using the registries in the auths section. This is also explained in awslabs/amazon-ecr-credential-helper#9 (comment)

Will update CHANGELOG soon.

Copy link
Member

@loosebazooka loosebazooka left a comment

Choose a reason for hiding this comment

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

Looks good, just hope reordering this doesn't break anyone.

@chanseokoh
Copy link
Member Author

I don't have high hopes. It's a breaking change. Although I think we can justify this reordering and people will be able to make it right and better: auths is discouraged as you put passwords as plain text, and credsStore is old enough to ditch and no real reason to use.

jib-core/CHANGELOG.md Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Prioritize credhelpers block in docker config.json
4 participants