docker / docker-credential-helpers

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

feat: add support for gopass as a credential store #268

Open sudoforge opened 1 year ago

sudoforge commented 1 year ago

This change adds support for gopass as a credential store, based on the pass implementation.

Closes #138 Closes #166

codecov-commenter commented 1 year ago

Codecov Report

Attention: Patch coverage is 54.88722% with 60 lines in your changes missing coverage. Please review.

Project coverage is 55.21%. Comparing base (6b9df3e) to head (43efb72).

Files Patch % Lines
gopass/gopass.go 54.88% 42 Missing and 18 partials :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #268 +/- ## ========================================== - Coverage 55.28% 55.21% -0.08% ========================================== Files 9 10 +1 Lines 624 757 +133 ========================================== + Hits 345 418 +73 - Misses 234 276 +42 - Partials 45 63 +18 ```

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

crazy-max commented 1 year ago

Thanks for your contrib, please check CI issues.

@sudoforge ^

sudoforge commented 1 year ago

Thanks for your contrib, please check CI issues.

@sudoforge ^

Yep, I'm aware of this and have a WIP solution that should resolve the build/test matrice failures.

crazy-max commented 1 year ago

sandboxed tests look good. For native ones on GHA we might need https://github.com/docker/docker-credential-helpers/pull/289 first.

sudoforge commented 1 year ago

sandboxed tests look good. For native ones on GHA we might need #289 first.

i'm not sure #289 resolves the issue we're seeing in the GHA tests. it looks like the pass and gopass tests are cross-pollinating the password store. i think this was due to not initializing gopass with a separate password store like what was being done in the sandboxed tests (the tests that run as part of the container build).

this should be addressed with sudoforge/docker-credential-helpers@4e78793 (the latest push as of this edit)

sudoforge commented 1 year ago

FWIW, i find the lack of a sandboxed test environment for all of the tests a little frustrating: running the test suite locally populates commits in my host machine's password store. this should probably be addressed separately. i'm firmly of the opinion that tests shouldn't have side effects.

sudoforge commented 1 year ago

rebased on top of c842499594dd00887c9ad798e3ccf6e96cb18c9a (the latest docker/docker-credential-helpers:master as of this writing).

sudoforge commented 1 year ago

Thanks for running the test pipeline. Looks like initialization is failing on ubuntu and macos, and the windows test is failing to initialize for a reason related to the pubkey. I'll take a closer look later today.

jmacdonald commented 6 months ago

It seems like this feature lost steam trying to get the Windows tests passing, but for what it's worth, I'd love to see this merged; gopass support would be a really nice addition!

sudoforge commented 6 months ago

actually, it lost steam to real life, as things seem to often do -- but i'd still love to get this merged as well! i can revisit it this weekend.

sudoforge commented 6 months ago

sudoforge/docker-credential-helpers@0ebbeb842e1a8fa19df77eff0d8b0f6448379d7c is a rebase on top of docker/docker-credential-helpers@097f945536afd1bbda30527501e62a4a1aab06f2; this is not expected to pass the entire pipeline -- i'm expecting it to fail on the test (windows-2022) workflow. this will be useful, though, as the logs for the last run in this tree (for sudoforge/docker-credential-helpers@5f7addcd22fde66a316a72d4b9d0c76860544280) have since expired, preventing me from being able to pick up debugging the error (which i cannot remember).

sudoforge commented 6 months ago

@crazy-max @thaJeztah would it be possible to get the pipelines kicked off for this PR some time this next week? i should only need a maximum of two more runs as far as i can tell:

i'd greatly appreciate your attention to this, and if there's anything i can do to make this a bit smoother, please let me know.

sudoforge commented 5 months ago

:wave: checking in again. can we get the tests executed for this tree?

sudoforge commented 4 months ago

@thaJeztah @crazy-max checking in again.

sudoforge commented 3 months ago

I have created a commit which restricts pipeline steps that upload artifacts and/or consume secrets to this upstream repository, allowing me to enable actions in my fork, so that I can iterate on this freely.

sudoforge commented 3 months ago

The Windows pipeline now passes: https://github.com/sudoforge/docker-credential-helpers/actions/runs/9899993331/job/27349937852

This tree will now pass all of the pipelines. It has (since the last time it was reviewed) received a few changes:

sudoforge commented 3 months ago

Due to the lack of response from any maintainer, I feel compelled to let people know that they are able to build from my fork and use it, if they desire gopass support. I will keep it up-to-date, and I've enabled issues in it. You are welcome to open issues for gopass, or requests to rebase on top of upstream, explicitly.

I will accept no other contributions to the fork at this point in time.

jmacdonald commented 3 months ago

@sudoforge thank you for chasing this! It's disappointing that it may not get merged in, but I can definitely use your fork. Appreciate the work you've put in here. 🍻