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

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

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

Conversation

sudoforge
Copy link

@sudoforge sudoforge commented May 13, 2023

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

Closes #138
Closes #166

@sudoforge sudoforge force-pushed the master branch 2 times, most recently from 540cce2 to f4090b4 Compare May 13, 2023 22:33
@sudoforge sudoforge force-pushed the master branch 3 times, most recently from 043a423 to fc40f5a Compare May 27, 2023 13:12
Dockerfile Outdated Show resolved Hide resolved
Copy link
Member

@crazy-max crazy-max left a comment

Choose a reason for hiding this comment

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

Thanks for your contrib, please check CI issues.

Also missing build-gopass make target in build-linux, build-darwin and build-windows stages in the Dockerfile.

@codecov-commenter
Copy link

codecov-commenter commented May 28, 2023

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 ⚠️
Additional details and impacted files
@@            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     

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

.github/workflows/build.yml Outdated Show resolved Hide resolved
Dockerfile Show resolved Hide resolved
Dockerfile Show resolved Hide resolved
@sudoforge sudoforge force-pushed the master branch 3 times, most recently from 8c1d14a to bc197e4 Compare May 28, 2023 00:31
.github/workflows/build.yml Outdated Show resolved Hide resolved
Dockerfile Outdated Show resolved Hide resolved
Dockerfile Outdated Show resolved Hide resolved
@crazy-max
Copy link
Member

Thanks for your contrib, please check CI issues.

@sudoforge ^

@sudoforge
Copy link
Author

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.

@sudoforge
Copy link
Author

sudoforge commented May 29, 2023

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

sudoforge commented May 29, 2023

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

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

@sudoforge
Copy link
Author

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

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

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

sudoforge/docker-credential-helpers@0ebbeb8 is a rebase on top of 097f945; 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@5f7addc) have since expired, preventing me from being able to pick up debugging the error (which i cannot remember).

@sudoforge sudoforge requested a review from crazy-max April 8, 2024 03:59
@sudoforge
Copy link
Author

sudoforge commented Apr 14, 2024

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

  • one to show me any errors that exist after rebasing, so that i can fix them
  • one after fixing them to show that they all pass

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

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

@sudoforge
Copy link
Author

@thaJeztah @crazy-max checking in again.

@sudoforge sudoforge force-pushed the master branch 3 times, most recently from e5dc19b to 5002a10 Compare July 11, 2024 23:30
…elpers

This change adds conditional expressions to restrict the execution of
pipeline steps that consume secrets, such as uploading artifacts to
remote stores, from being executed unless they are being executed in the
context of the upstream `docker/docker-credential-helpers` repository.

With this change, downstream, external contributors (users who have
forked this repository, and have that fork on GitHub) can enable GitHub
Actions in their fork, in order to iterate and validate their changes
without waiting on the upstream maintainers.

This is extremely helpful to all contributors, because the repository
requires maintainer approval in order to execute pipelines, which is
burdensome on the maintainers, and due to this restrictive gatekeeping,
contributors have an excessively long feedback loop.

Signed-off-by: sudoforge <no-reply@sudoforge.com>
@sudoforge
Copy link
Author

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.

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

Closes: docker#138
Closes: docker#166
Signed-off-by: sudoforge <no-reply@sudoforge.com>
@sudoforge
Copy link
Author

sudoforge commented Jul 11, 2024

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:

  • A second commit was added that restricts the execution of several pipeline steps to this repository (docker/docker-credentials-helper). The commit message contains the full context for why this was done, and given the fact that worklows require maintainer approval in this repository, I think it is something that should make it in-tree. Please let me know if you'd rather have this submitted in a second PR (I'd greatly prefer this if you plan to squash merge this tree; this is a logically independent commit and should not be squashed with the other).

  • The commit adding support for gopass also touches the workflow file, namely, enabling the import of the GPG key for the Windows pipeline. This is necessary for the gopass pipeline to succeed.

@sudoforge
Copy link
Author

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

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add a credential helper for gopass
5 participants