-
Notifications
You must be signed in to change notification settings - Fork 2.1k
cli/config/credentials: skip saving config-file if credentials didn't change #5553
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
Conversation
Codecov ReportAttention: Patch coverage is
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 |
|
/cc @derekpovah FYI 😄 |
|
I just realised that this patch is not yet fully addressing the issue, because cli/cli/config/credentials/file_store.go Lines 31 to 35 in 062eecf
|
… 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 <[email protected]>
51a7ea0 to
d3f6867
Compare
Fixed in the last push; https://github.com/docker/cli/compare/51a7ea0e112fe318a07eb7b7ec4f02b6c069b502..d3f6867e4d7f5018ae4c0fbc709934893f0e95a2 |
laurazard
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
vvoland
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
…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 <[email protected]>
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.jsonis a symlink to a write-protected file;Which causes
docker loginto 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 linkThis 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:
- How to verify it
- Description for the changelog
- A picture of a cute animal (not mandatory but encouraged)