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

Add a way to customize the gpg program used for signing #2072

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

gregoire-mullvad
Copy link

Add a constructor to make a SigningMechanism with a custom program. This
can be used to sign images with a gpg wrapper (e.g. qubes-gpg-client-wrapper)
or an alternative implementation (e.g. sequoia-chameleon-gnupg).

Related to containers/podman#15838

Add a constructor to make a SigningMechanism with a custom program. This
can be used to sign images with a gpg wrapper (e.g. qubes-gpg-client-wrapper)
or an alternative implementation (e.g. sequoia-chameleon-gnupg).

Signed-off-by: Grégoire Détrez <gregoire@mullvad.net>
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.

Thanks!

  • This seems not to be the right API, assuming this targets podman push / skopeo copy and the like. Those things are now configurable in a somewhat-extensible way via c/image/copy.Options.Signers, with the caller calling c/image/signature/simplesigning.NewSigner. So, I think the user-facing API needs to be something like simplesigning.WithGPGProgram.
    • We don’t, strictly speaking, need to expose a NewGPGSigningMechanism — and without the new simplesigning option it would, AFAICS, not directly help, unless you have a completely different use case in mind.
    • The public SigningMechanism API is already not ideally fit for purpose, see the private signingMechanismWithPassphrase extension we need. So I don’t think adding a SigningMechanism-focused public API is very valuable any more.
    • and if we did add an API, it should probably be extensible using something like the functional option mechanism, so that we don’t motivate a combinatoric explosion of functions.
    • Overall, my guess at a clean approach (which I haven’t prototyped to make sure that works) is:
      • Move most of the SigningMechanism code (the two implementations + the shared facade) into signature/internal (or is that signature/internal/mechanism?) Then we don’t need to design a long-term public API. E.g. we can have a single newGPGSigningMechanism without the existing per-option functions in that private API.
      • Keep the existing public API in signature, just forward to the new location.
      • Port signature/simplesigning to the private API
      • Add the new option to the internal implementation, and expose it in signature/simplesigning.
  • This does not compile with the containers_image_openpgp build tag. The two build alternatives need to expose the same API — though it’s fine to report an “unsupported” error message; e.g. the openpgp backend refuses to sign things.
  • Within the signature subpackage, we want as high unit test code coverage as reasonably possible. In this case, at least a basic test that points at a shell script and verifies that the shell script has been executed seems appropriate.

@mtrmac mtrmac added the kind/feature A request for, or a PR adding, new functionality label Aug 4, 2023
@gregoire-mullvad
Copy link
Author

@mtrmac thanks for the review. I was indeed a bit unsure about the different APIs, and decided that a good starting point was to make this work with podman image sign, and if I'm not mistaken that command uses the SigningMechanism directly. (Since the simplesigning API builds on top of signingMechanism this seemed like a natural starting point.)
I think doing it the way you described would then require refactoring podman image sign to use this other API. Should I try to do that?

@mtrmac
Copy link
Collaborator

mtrmac commented Aug 11, 2023

@gregoire-mullvad I’m afraid I don’t much think about podman image sign, that command is very tempting to use in insecure ways (compared to podman push --sign-by when done by the trusted build system or the like). So first a gentle reminder to try using podman push --sign-by or skopeo copy --sign-by when copying from a trusted to a potentially-untrusted system instead — though there certainly are ways to use podman image sign securely.


Right now, signature/signer.Signer is basically completely opaque and only consumable by copy.Image; that’s not useful for podman image sign as is.

To make that work, signature/internal.signer.SignImageManifest would need to be made public. I wouldn’t mind that — it was made private originally just so that we didn’t have to commit to a public API without a user. As long as we don’t add interfaces that might need changing or extending to the public API, adding a public function which we can possibly redirect to a different internal abstraction should be fine.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/feature A request for, or a PR adding, new functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants