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

GPGME: support passphrase for prompt-less signing #1446

Merged
merged 2 commits into from
Jan 25, 2022

Conversation

vrothberg
Copy link
Member

@vrothberg vrothberg commented Jan 19, 2022

To support signing images via gpgme without user prompt, allow for
providing a passphrase via the copy options. Add a new *WithOptions API
to the signature package and extend its interface.

To prevent breaking the API, extend the signature API with an internal
type as has already been done for other types and interfaces in c/image.

Signed-off-by: Valentin Rothberg rothberg@redhat.com

@vrothberg
Copy link
Member Author

Skopeo PR: containers/skopeo#1540

Copy link
Collaborator

@mtrmac mtrmac left a comment

Choose a reason for hiding this comment

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

A process-wide environment variable shouldn’t be the only way to expose this (e.g. it is unusable from any multi-threaded server managing multiple private keys); a caller should be able to just pass a Go string to a NewGPGSigningMechanism$differentiator constructor. (Assuming $differentiator is WithOptions, should that just be a struct, or “functional options” with all that boilerplate? I don’t have a strong opinion at this point.)

Similarly, up the stack, the passphrase should come from copy.Options.

Afterwards, if passing this through the environment is a good idea (AFAIK it’s not unsafe but it might be unergonomic — notably the way containers/skopeo#1540 indiscriminately makes that available to all child processes immediately makes me pause), we can add a function to pkg/cli/environment.

Copy link
Collaborator

@mtrmac mtrmac left a comment

Choose a reason for hiding this comment

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

See also the previous review — this should be configurable in Go.

signature/mechanism_gpgme.go Outdated Show resolved Hide resolved
signature/mechanism_gpgme.go Outdated Show resolved Hide resolved
signature/mechanism_gpgme.go Outdated Show resolved Hide resolved
@vrothberg vrothberg force-pushed the passphrase branch 3 times, most recently from b6b7545 to 65f47bf Compare January 20, 2022 15:36
@vrothberg vrothberg changed the title WIP - GPGME: support passphrase env variable GPGME: support passphrase env variable Jan 20, 2022
@vrothberg
Copy link
Member Author

Good to go from my side. Had to do the usual *WithOptions dance.

Copy link
Member

@TomSweeneyRedHat TomSweeneyRedHat left a comment

Choose a reason for hiding this comment

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

LGTM
Documentation targeted towards somewhere in Podman and/or Buildah?

@vrothberg
Copy link
Member Author

LGTM Documentation targeted towards somewhere in Podman and/or Buildah?

Skopeo for now. If needed, we can add the --sign-passphrase-file flag to Podman/Buildah as well. Would require some plumbing in c/common/libimage.

copy/copy.go Outdated Show resolved Hide resolved
copy/copy.go Outdated Show resolved Hide resolved
copy/sign.go Outdated Show resolved Hide resolved
signature/mechanism.go Outdated Show resolved Hide resolved
signature/mechanism.go Outdated Show resolved Hide resolved
signature/mechanism_gpgme.go Outdated Show resolved Hide resolved
signature/mechanism_gpgme.go Outdated Show resolved Hide resolved
signature/mechanism_gpgme.go Outdated Show resolved Hide resolved
signature/mechanism_gpgme.go Show resolved Hide resolved
signature/signature.go Outdated Show resolved Hide resolved
@vrothberg vrothberg changed the title GPGME: support passphrase env variable GPGME: support passphrase for prompt-less signing Jan 24, 2022
@vrothberg
Copy link
Member Author

@mtrmac, PTanotherL:

  • We're passing a passphrase only (no path) around
  • pkg/cli received a convenience function for parsing a passphrase file
  • added a new signature/internal/signature package to extend the interface without breaking the API

Copy link
Collaborator

@mtrmac mtrmac left a comment

Choose a reason for hiding this comment

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

ACK to the overall approach.

signature/internal/signature/mechanism.go Outdated Show resolved Hide resolved
signature/mechanism_openpgp.go Outdated Show resolved Hide resolved
signature/mechanism_gpgme.go Outdated Show resolved Hide resolved
signature/mechanism_gpgme.go Outdated Show resolved Hide resolved
signature/signature.go Show resolved Hide resolved
signature/signature.go Outdated Show resolved Hide resolved
pkg/cli/parse.go Outdated Show resolved Hide resolved
@mtrmac's fork was needed to allow for building on RHEL 7 which we do
not target anymore.  Moving to upstream allows for making use of more
recent features and avoids diverging in the future.

Signed-off-by: Valentin Rothberg <rothberg@redhat.com>
@mtrmac
Copy link
Collaborator

mtrmac commented Jan 25, 2022

@vrothberg I’d really like to have a test for the gpgmeSigningMechanism.SignWithPassphrase code. Are you working on one, or should I take that on?

@vrothberg vrothberg force-pushed the passphrase branch 2 times, most recently from e1aa665 to df2414e Compare January 25, 2022 12:18
signature/docker_test.go Outdated Show resolved Hide resolved
signature/docker_test.go Outdated Show resolved Hide resolved
@vrothberg vrothberg force-pushed the passphrase branch 3 times, most recently from 82bd697 to 2c9ff81 Compare January 25, 2022 13:10
signature/docker_test.go Show resolved Hide resolved
signature/docker_test.go Outdated Show resolved Hide resolved
@vrothberg vrothberg force-pushed the passphrase branch 3 times, most recently from 0557579 to 03ca304 Compare January 25, 2022 13:48
signature/docker_test.go Outdated Show resolved Hide resolved
signature/docker_test.go Outdated Show resolved Hide resolved
signature/docker_test.go Outdated Show resolved Hide resolved
signature/docker_test.go Show resolved Hide resolved
To support signing images via gpgme without user prompt, allow for
providing a passphrase via the copy options.  Add a new *WithOptions API
to the `signature` package and extend its interface.

To prevent breaking the API, extend the signature API with an internal
type as has already been done for other types and interfaces.

Signed-off-by: Valentin Rothberg <rothberg@redhat.com>
@mtrmac mtrmac merged commit bd97b58 into containers:main Jan 25, 2022
@vrothberg vrothberg deleted the passphrase branch January 25, 2022 14:32
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.

3 participants