Skip to content

Allow absolute path in credential store/helpers #6104

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

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

hamnis
Copy link

@hamnis hamnis commented May 22, 2025

Implements #6103

Allows users to use a credential helper that does not live in path. The underlying credentials helper library uses os/exec exec.Command so this should be safe to do.

I should add a test, please advice for the best location to do that.

- What I did
Checks if the path is absolute, then return that instead of prepending docker-credential-

- How I did it
uses filepath to check if the path is absolute

- How to verify it
use an absolute path to a binary file which implements the docker-credential protocol.

- Human readable description for the release notes

Allows users to use a credential helper that does not live in path.

This would allow users to use a helper that does not live in path. The underlying credentials helper library uses os/exec exec.Command so this should be safe to do.

I should add a test, please advice for the best location to do that.

Signed-off-by: Erlend Hamnaberg <[email protected]>
Copy link
Collaborator

@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.

Just a nit:

Co-authored-by: Laura Brehm <[email protected]>
Signed-off-by: Erlend Hamnaberg <[email protected]>
@hamnis hamnis force-pushed the allow-credential-full-path branch from a7c17c1 to e5ff2f8 Compare May 25, 2025 18:10
@hamnis
Copy link
Author

hamnis commented May 25, 2025

Thanks @laurazard. I am not really a go programmer

@codecov-commenter
Copy link

codecov-commenter commented May 26, 2025

Codecov Report

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

Project coverage is 55.02%. Comparing base (b2c4452) to head (e5ff2f8).

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #6104      +/-   ##
==========================================
- Coverage   55.04%   55.02%   -0.03%     
==========================================
  Files         361      361              
  Lines       30142    30145       +3     
==========================================
- Hits        16591    16586       -5     
- Misses      12594    12601       +7     
- Partials      957      958       +1     
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@thaJeztah
Copy link
Member

In general, I wonder if you could provide more context on why you need this change; ultimately, the CLI would look for binaries in PATH, so it would already be possible to update your PATH if you want custom locations to take precedence (which .. is basically what PATH was designed for).

That's not a "hard reject" for this change, but we're also working on various changes around authentication (and credential stores); I want to avoid introducing more complexity if it can get in the way of those changes (especially if it's for a niche-case that already could have workarounds (like PATH mentioned above)).

@hamnis
Copy link
Author

hamnis commented Jun 5, 2025

Currently i am using docker in a setting where I need to generate the configuration file and set credentials to docker repos as part of this generation.

Since I have to then also generate a shell script to piggyback to my own tooling and manipulate the path is annoying, but not a showstopper. Since I cant enforce how my users set their PATH variables, I have to build a shim on top of docker to set the PATH correctly.

This would help by making this interaction a bit less annoying.

I also think users would like to be able to specify absolute paths to these authentication tools.

@hamnis hamnis force-pushed the allow-credential-full-path branch from f66b6ce to b198fa4 Compare June 23, 2025 06:40
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