Skip to content

cli/config/credentials: skip saving config-file if credentials didn't change - #5553

Merged
vvoland merged 1 commit into
docker:masterfrom
thaJeztah:login_idempotent
Oct 21, 2024
Merged

cli/config/credentials: skip saving config-file if credentials didn't change#5553
vvoland merged 1 commit into
docker:masterfrom
thaJeztah:login_idempotent

Conversation

@thaJeztah

@thaJeztahthaJeztah commented Oct 19, 2024

Copy link
Copy Markdown
Member

Before this change, the config-file was always updated, even if there were no changes to save. This could cause issues when the config-file already had credentials set and was read-only for the current user.

For example, on NixOS, this poses a problem because config.json is a symlink to a write-protected file;

$ readlink ~/.docker/config.json
/home/username/.config/sops-nix/secrets/ghcr_auth
$ readlink -f ~/.docker/config.json
/run/user/1000/secrets.d/28/ghcr_auth

Which causes docker login to fail, even if no changes were to be made;

Error saving credentials: rename /home/derek/.docker/config.json2180380217 /home/username/.config/sops-nix/secrets/ghcr_auth: invalid cross-device link

This patch updates the code to only update the config file if changes were detected. It there's nothing to save, it skips updating the file, as well as skips printing the warning about credentials being stored insecurely.

With this patch applied:

$ docker login -u yourname
Password:
WARNING! Your credentials are stored unencrypted in'/root/.docker/config.json'.
Configure a credential helper to remove this warning. See
https://docs.docker.com/go/credential-store/
Login Succeeded
$ docker login -u yourname
Password:
Login Succeeded

- How to verify it

- Description for the changelog

The `docker login` and `docker logout` command no longer update the configuration file if the credentials didn't change.

- A picture of a cute animal (not mandatory but encouraged)

@codecov-commenter

codecov-commenter commented Oct 19, 2024

Copy link
Copy Markdown

Codecov Report

Attention: Patch coverage is 0% with 8 lines in your changes missing coverage. Please review.

Project coverage is 59.56%. Comparing base (8a7c5ae) to head (d3f6867).
Report is 25 commits behind head on master.

Additional details and impacted files
@@ Coverage Diff @@## master #5553 +/- ##
==========================================
- Coverage 59.57% 59.56% -0.02% 
==========================================
Files 345 345 Lines 29088 29096 +8 ==========================================
Hits 17330 17330 - Misses 10788 10794 +6 - Partials 970 972 +2 

@thaJeztah

Copy link
Copy Markdown
MemberAuthor

/cc @derekpovah FYI 😄

@thaJeztah

thaJeztah commented Oct 20, 2024

Copy link
Copy Markdown
MemberAuthor

I just realised that this patch is not yet fully addressing the issue, because docker logout also mutates the config-file, even if no credentials were found in the config file, it does a write.

// Erase removes the given credentials from the file store.
func (c*fileStore) Erase(serverAddressstring) error {
delete(c.file.GetAuthConfigs(), serverAddress)
returnc.file.Save()
}

… change
Before this change, the config-file was always updated, even if there
were no changes to save. This could cause issues when the config-file
already had credentials set and was read-only for the current user.
For example, on NixOS, this poses a problem because `config.json` is a
symlink to a write-protected file;
$ readlink ~/.docker/config.json
/home/username/.config/sops-nix/secrets/ghcr_auth
$ readlink -f ~/.docker/config.json
/run/user/1000/secrets.d/28/ghcr_auth
Which causes `docker login` to fail, even if no changes were to be made;
Error saving credentials: rename /home/derek/.docker/config.json2180380217 /home/username/.config/sops-nix/secrets/ghcr_auth: invalid cross-device link
This patch updates the code to only update the config file if changes
were detected. It there's nothing to save, it skips updating the file,
as well as skips printing the warning about credentials being stored
insecurely.
With this patch applied:
$ docker login -u yourname
Password:
WARNING! Your credentials are stored unencrypted in '/root/.docker/config.json'.
Configure a credential helper to remove this warning. See
https://docs.docker.com/go/credential-store/
Login Succeeded
$ docker login -u yourname
Password:
Login Succeeded
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
@thaJeztah

Copy link
Copy Markdown
MemberAuthor

I just realised that this patch is not yet fully addressing the issue, because docker logout also mutates the config-file, even if no credentials were found in the config file, it does a write.

Fixed in the last push; https://github.com/docker/cli/compare/51a7ea0e112fe318a07eb7b7ec4f02b6c069b502..d3f6867e4d7f5018ae4c0fbc709934893f0e95a2

@laurazardlaurazard left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

@vvolandvvoland left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

LGTM

@vvoland
vvoland merged commit d2b87a0 into docker:masterOct 21, 2024
@thaJeztah
thaJeztah deleted the login_idempotent branch October 21, 2024 13:35
D3-LucaPiombino added a commit to CodeCoil/container-desktop that referenced this pull request Jun 2, 2025
…uildx: `0.24.0`) (#10)
## Summary of the Pull Request
Update docker (and related tooling) to the latest version.
## Detailed Description of the Pull Request / Additional comments
The core motivation is to have a version of the docker client that does
not attempt to mutate/rewrite the config
(docker/cli#5553).
This is in preparation of a future enhancement to share the same client
configuration of the windows host and to provide a seamless experience.
This could include also the auth via credential helpers running on the
host (e.g. a basic scenario is to at least use the windows credential
manager to avoid storing static credentials in plain in the config
file).
I did not push anything yet because i am waiting to see if
docker/cli#5948
get merged as it would probably provide a better and simpler out of the box experience.
Co-authored-by: Luca Piombino <luca.piombino@codecoil.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@thaJeztah@codecov-commenter@vvoland@laurazard