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

signature: add OpenPGP signing mechanism based on Sequoia #2569

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

Conversation

ueno
Copy link

@ueno ueno commented Sep 17, 2024

This adds a new OpenPGP signing mechanism based Sequoia[1]. As Sequoia
is written in Rust and doesn't provide a stable C FFI, this
integration uses a minimal shared library as a "point solution".

To build, first follow the instruction at [2] and install
libpodman_sequoia.so* into the library path, and then build with the
following command from the top-level directory:

  $ make BUILDTAGS="btrfs_noversion libdm_no_deferred_remove containers_image_sequoia"

Note also that, for testing on Fedora, crypto-policies settings might
need to be modified to explictly allow SHA-1 and 1024 bit RSA, as the
testing keys in signature/fixtures are using those legacy algorithms.

  1. https://sequoia-pgp.org/
  2. https://github.com/ueno/podman-sequoia

@ueno ueno force-pushed the wip/signature-sequoia branch 5 times, most recently from 0f4142d to 5003e75 Compare September 17, 2024 10:24
@cgwalters
Copy link
Contributor

(I'm not an "official" reviewer here, FWIW)

I skimmed your code and I have to say it looks generally great.

But very similarly to your PR in ostreedev/ostree#3278 it'd be quite nice I think to list out why we're making this change. Especially questions like: Do we still need the openpgp backend too going forward?

https://github.com/ueno/podman-sequoia

I do wonder whether it makes sense for such a thing to live separately from this repository. Would it have a stable API/ABI? Of course adding it to this repository suddenly grows the scope of things here from "Go" to "Go, Rust and C" which is not at all a small thing.

But, I personally also do like the idea of opening up to thinking about how we use Rust code in this stack too. (To be clear, I am not speaking in any way for the other people who have done 99% of the work in this repository that are not me)

//go:build !containers_image_openpgp
// +build !containers_image_openpgp
//go:build !containers_image_openpgp && !containers_image_sequoia
// +build !containers_image_openpgp,!containers_image_sequoia
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we default to sequoia or not?


Eventually the build tag, whichever tag it ends up being, should be documented in README.md.

signature/mechanism_sequoia.go Outdated Show resolved Hide resolved
@mtrmac
Copy link
Collaborator

mtrmac commented Sep 17, 2024

Hey @ueno , I’m very sorry I couldn’t get back to this discussion since … April?, due to a different RHEL priority (… possibly relevant here: sigstore signatures, using the Go standard-library crypto implementation).

The code in this PR is all good.


We do need to figure out packaging, and RH package ownership.

Do we still need the openpgp backend too going forward?

There’s a product discussion motivating the inclusion of this — and I assume product concerns would drive the continued existence of OpenPGP for a number of years. The default … should be kept up to date, but also needs to be somewhat practical to use.

Of course adding it to this repository suddenly grows the scope of things here from "Go" to "Go, Rust and C" which is not at all a small thing.

This repository is currently consumed by referencing it in go.mod in Podman & friends, and using go mod vendor; i.e. (apart from some configuration files), it mostly does not directly participate in builds.

*shrug* That means that adding an entirely new built artifact into this repo would probably not hurt any existing packaging.

But one other implication is that different versions of “Podman & friends” can be concurrently installed and include different versions of c/image, i.e. the C-API shared library (in a single system-wide location) will have to maintain a stable ABI to support all of those versions; directly including the Rust+C part in the c/image repo would not allow us to develop the server and client in immediate lockstep, so there’s no direct benefit of using one repo for all.

One thing I’m unsure about is assumptions of the Go tooling about repository contents — we have a go.mod at the top level of this Git repo, it’s conceivable that compilers/linters/… are going to assume that subdirectories primarily (exclusively?) contain Go code.


Podman etc. developers who don’t directly work on signing do need some reasonably convenient way to get an environment which doesn’t panic during process start. For GPGME, we tell them to install existing packages, on both Linux and macOS: https://github.com/containers/image/tree/main?tab=readme-ov-file#building . Something at about that level of hassle (or less) should exist for https://github.com/containers/image/tree/main?tab=readme-ov-file#building , or those developers are going to start sticking containers_image_openpgp in Makefiles.

@mtrmac mtrmac added the kind/feature A request for, or a PR adding, new functionality label Sep 17, 2024
@ueno
Copy link
Author

ueno commented Sep 18, 2024

[...] it'd be quite nice I think to list out why we're making this change. Especially questions like: Do we still need the openpgp backend too going forward?

In the discussion in April, a couple of motivations raised are:

As for the future of the openpgp backend, I agree with @mtrmac that it still has some value to maintain it as a legacy/standalone backend, even if sigstore signatures are preferred.

Podman etc. developers who don’t directly work on signing do need some reasonably convenient way to get an environment which doesn’t panic during process start. For GPGME, we tell them to install existing packages, on both Linux and macOS: https://github.com/containers/image/tree/main?tab=readme-ov-file#building . Something at about that level of hassle (or less) should exist for https://github.com/containers/image/tree/main?tab=readme-ov-file#building , or those developers are going to start sticking containers_image_openpgp in Makefiles.

Not sure if this is an ideal solution, but one option might be to merge mechanism_sequoia.go into mechanism_gpgme.go (rename it accordingly), and make it use the existing GPGME backend as a fallback if it fails to load libpodman_sequoia.so.0. The current Go binding uses dlwrap to enable this kind of run-time detection of Sequoia through sequoia.Init.

@ueno ueno force-pushed the wip/signature-sequoia branch 2 times, most recently from e7c0777 to fa356ee Compare September 19, 2024 01:50
This adds a new OpenPGP signing mechanism based Sequoia[1]. As Sequoia
is written in Rust and doesn't provide a stable C FFI, this
integration uses a minimal shared library as a "point solution".

To build, first follow the instruction at [2] and install
`libpodman_sequoia.so*` into the library path, and then build with the
following command from the top-level directory:

  $ make BUILDTAGS="btrfs_noversion libdm_no_deferred_remove containers_image_sequoia"

Note also that, for testing on Fedora, crypto-policies settings might
need to be modified to explictly allow SHA-1 and 1024 bit RSA, as the
testing keys in signature/fixtures are using those legacy algorithms.

1. https://sequoia-pgp.org/
2. https://github.com/ueno/podman-sequoia

Signed-off-by: Daiki Ueno <[email protected]>
@mtrmac
Copy link
Collaborator

mtrmac commented Sep 19, 2024

Not sure if this is an ideal solution, but one option might be to merge mechanism_sequoia.go into mechanism_gpgme.go (rename it accordingly), and make it use the existing GPGME backend as a fallback if it fails to load libpodman_sequoia.so.0.

That’s interesting … that would certainly eliminate concerns about developers not being able to work on other things. OTOH we’d then need some other mode to run in tests, and in production, to ensure we are using the right backend.

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.

3 participants