Skip to content
New issue

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

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

Already on GitHub? # to your account

pass: return correct error, and ignore empty stores on list #321

Merged
merged 1 commit into from
May 9, 2024

Conversation

thaJeztah
Copy link
Member

commit 2fc2313 changed the errors returned by the pass credentials-helper to use a errCredentialsNotFound. This error string is used in the client to distinguish a "not found" error from other errors. (see client.Get).

However, there were additional second code-paths that returned a custom error, which would not be detected as a "not found" error, resulting in an error when logging out;

Removing login credentials for https://index.docker.io/v1/
WARNING: could not erase credentials:
https://index.docker.io/v1/: error erasing credentials - err: exit status 1, out: `error getting credentials - err: exit status 1, out: `no usernames for https://index.docker.io/v1/``

This patch:

  • updates Pass.Get() to return a errCredentialsNotFound if no credentials were found
  • updates Pass.List() to not return an error if any of the domains had no credentials stored.

- Description for the changelog

Fix "could not erase credentials" when trying to log out and no credentials were present

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

@codecov-commenter
Copy link

codecov-commenter commented May 9, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 51.46%. Comparing base (73b9e5d) to head (1bb9aa3).
Report is 21 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #321      +/-   ##
==========================================
- Coverage   54.10%   51.46%   -2.65%     
==========================================
  Files           9        8       -1     
  Lines         621      513     -108     
==========================================
- Hits          336      264      -72     
+ Misses        238      212      -26     
+ Partials       47       37      -10     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@thaJeztah thaJeztah force-pushed the fix_pass_errors branch 4 times, most recently from 85a678b to bed185c Compare May 9, 2024 10:18
commit 2fc2313 changed the errors returned
by the pass credentials-helper to use a errCredentialsNotFound. This error
string is used in the client to distinguish a "not found" error from other
errors. (see [client.Get][1]).

However, there were additional second code-paths that returned a custom error,
which would not be detected as a "not found" error, resulting in an error when
logging out;

    Removing login credentials for https://index.docker.io/v1/
    WARNING: could not erase credentials:
    https://index.docker.io/v1/: error erasing credentials - err: exit status 1, out: `error getting credentials - err: exit status 1, out: `no usernames for https://index.docker.io/v1/``

This patch:

- updates Pass.Get() to return a errCredentialsNotFound if no credentials
  were found
- updates Pass.List() to not return an error if any of the domains had no
  credentials stored.

[1]: https://github.com/docker/docker-credential-helpers/blob/73b9e5d51f8dc9f598e08a0f2171c5d5a828e76b/client/client.go#L51-L55

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
@thaJeztah thaJeztah requested review from nicks and crazy-max May 9, 2024 10:23
@thaJeztah thaJeztah self-assigned this May 9, 2024
@thaJeztah
Copy link
Member Author

FWIW, we should probably consider using the PASSWORD_STORE_DIR env-var in tests (use t.TempDir() to create a temporary location and use that as PASSWORD_STORE_DIR (t.SetEnv()). That would prevent tests from interfering with other tests (or with an "actual" store on the user's machine)

But this also requires the pass-store to be initialised in that directory, so that required more work;

func getPassDir() string {
if passDir := os.Getenv("PASSWORD_STORE_DIR"); passDir != "" {
return passDir
}

Copy link

@krissetto krissetto 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
Contributor

@nicks nicks left a comment

Choose a reason for hiding this comment

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

LGTM! thanks from all linux users 🐧 !

@thaJeztah
Copy link
Member Author

Let me bring this one in; thanks!

I saw some small bits that could use a cleanup after this; will also discuss doing a new patch release 👍

@thaJeztah thaJeztah merged commit f64d6b1 into docker:master May 9, 2024
11 checks passed
@thaJeztah thaJeztah deleted the fix_pass_errors branch May 9, 2024 14:44
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants