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

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

Merged
merged 1 commit into from
Oct 21, 2024

Conversation

thaJeztah
Copy link
Member

@thaJeztah thaJeztah commented Oct 19, 2024

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
Copy link

codecov-commenter commented Oct 19, 2024

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
Member Author

/cc @derekpovah FYI 😄

@thaJeztah thaJeztah requested a review from laurazard October 19, 2024 12:09
@thaJeztah
Copy link
Member Author

thaJeztah commented Oct 20, 2024

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(serverAddress string) error {
delete(c.file.GetAuthConfigs(), serverAddress)
return c.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 <[email protected]>
@thaJeztah
Copy link
Member Author

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

Copy link
Member

@laurazard laurazard left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Collaborator

@vvoland vvoland left a comment

Choose a reason for hiding this comment

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

LGTM

Sign up for free to 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