docker / docker-credential-helpers

Programs to keep Docker login credentials safe by storing in platform keystores
MIT License
1.05k stars 166 forks source link

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

Closed thaJeztah closed 2 months ago

thaJeztah commented 2 months ago

commit 2fc2313bb1a9608195bb2a7624983b52901d4c73 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:

- 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 commented 2 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 51.46%. Comparing base (73b9e5d) to head (1bb9aa3).

Additional details and impacted files ```diff @@ 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 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

thaJeztah commented 2 months ago

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; https://github.com/docker/docker-credential-helpers/blob/73b9e5d51f8dc9f598e08a0f2171c5d5a828e76b/pass/pass.go#L107-L110

thaJeztah commented 2 months ago

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 👍