signature: add OpenPGP signing mechanism based on Sequoia #2569
Conversation
0f4142d to
5003e75
Compare
|
(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? 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) |
|
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 The code in this PR is all good. We do need to figure out packaging, and RH package ownership.
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.
This repository is currently consumed by referencing it in *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 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 |
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.
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 |
fa356ee to
23fec2d
Compare
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. |
23fec2d to
995e8aa
Compare
|
@mtrmac I've reworked this and incorporated all the required code into the same tree, instead of vendoring an external repository. The rust code is now hosted in signatures/sequoia/rust, which can be packaged something like rust-rpm-sequoia; we (RHEL Crypto team) are happy to own the downstream package. As for the CI coverage, the base CI image needs to be updated to include the Rust toolchain and OpenSSL development files. Could you help with that? |
995e8aa to
f8df8c8
Compare
f8df8c8 to
bdfcafc
Compare
|
To test it locally, I currently do the following on Fedora 41: cd signature/sequoia/rust
PREFIX=/usr LIBDIR="\${prefix}/lib64" cargo build --release
cd -
sudo update-crypto-policies --set LEGACY
LD_LIBRARY_PATH=$PWD/signature/sequoia/rust/target/release make BUILDTAGS="btrfs_noversion containers_image_sequoia" |
bdfcafc to
344c8ea
Compare
|
Cc: @jnovy, I’ll probably come asking questions. |
| for _, keyring := range []string{"pubring.gpg", "secring.gpg"} { | ||
| blob, err := os.ReadFile(path.Join(gpgHome, keyring)) |
There was a problem hiding this comment.
Would this be copying all keys from ~/.gnupg into ~/.local/share/sequoia, on every single podman push --sign-by? That seems unexpected.
I don’t know what to do here… at a first glance, maybe, if we find such keys, we need to load them only into an ephemeral store.
Or maybe this is fine, for products where users / product owners opt in — probably implying that Sequoia wouldn’t be the default. Well, it isn’t a default in this PR.
There was a problem hiding this comment.
It turns out ( https://www.gnupg.org/faq/whats-new-in-2.1.html#nosecring ) that recent GPG implementations are not writing to secring.gpg at all, so this would not necessarily import all relevant keys.
And after I generated a new ed25519 key pair using the default settings of GnuPG 2.4.8, the sequoia backend imported it successfully, but using it failed with Malformed MPI: Details omitted, parsing secret; that seems to be a message that applies also when an incorrect passphrase is used ?!
There was a problem hiding this comment.
… a rsa3072 key works.
Is there something non-obvious I need to do to support the GnuPG-default ed25519 keys?
There was a problem hiding this comment.
Filed https://gitlab.com/sequoia-pgp/sequoia/-/issues/1194 with a reproducer.
There was a problem hiding this comment.
For the record, Sequoia was able to sign using, and verify signatures against, a different GnuPG-generated ed25519 key (which wasn’t protected by a passphrase), so this seems to be, at least, not a systemic interoperability problem.
There was a problem hiding this comment.
So, in #2876 (“WIP: With sequoia, still use GPGME for existing signing, and add a new Signer API”), I am prototyping a design where
- There is a new
signature/simplesequoia.NewSignerAPI (suitable forskopeo copyandpodman push, but perhaps not for the discouragedpodman image sign) that allows signing using a pre-existing Sequoia directory (but not, yet, automatically, the default one — signature: add OpenPGP signing mechanism based on Sequoia #2569 (comment) might be relevant). Notably, in that API, callers would be providing a Sequoia home directory, not a GPG one, and there is no expectation of interaction withgpg-agent. newEphemeralGPGSigningMechanism(used for enforcingpolicy.json) always uses Sequoia.newGPGSigningMechanismInDirectory(only used in tests, and forpodman image sign) always uses GPGME.
I haven’t yet hooked it up to a CLI to provide an end-to-end PoC, but there is an automated test of the signature creation+verification, so I feel fairly confident in the basics.
@ueno I’d appreciate your thoughts on that draft.
There was a problem hiding this comment.
Sorry for the delay; was swamped with end-of-quarter tasks. The new API looks nice, and I left only a couple of comments there.
I'm still not sure if this separation of signing-only vs verification per backend is desired (given gpg-agent protocol is agnostic to OpenPGP format nor GnuPG internals; e.g., sequoia-chameleon-gnupg handles this well), though if the typical workflow is going to be fully stateless, then not assuming a home directory with the new backend makes much more sense.
There was a problem hiding this comment.
I think we need a new Sequoia-specific API for Sequoia-specific keys, in any case. That makes migration of GnuPG-owned keys to Sequoia home directories not very attractive, especially if there is an opportunity to eventually access the gpg-agent data via Sequoia. So, we will have distinct “keys-in-Sequoia” and “keys-in-GnuPG” APIs, and I think that’s correct both in that it is ~simpler to implement, and in that it more correctly models the world.
Now, replacing the GPGME implementation with the Sequoia compatibility layer is an option, and it would be nicer than the dependency on both implementations that I’m proposing now. But I think it’s very clearly not the first thing to work on, vs. the blocker of packaging the podman-sequoia shared library in RPMs/distributions, CI in our projects, and probably Homebrew.
(I’m also still somewhat curious about https://gitlab.com/sequoia-pgp/sequoia/-/issues/1194 — AFAICT that is an interoperability issue only for private keys, the corresponding public key works fine, but, still, I’d be more comfortable if the situation were investigated and positively known not to be an issue in the Sequoia-calls-gpg-agent workflow.)
If we can finish the Sequoia-calls-gpg-agent implementation in time for the first release, and have enough confidence in it, sure, that would be great; but for me personally, that can’t be a thing I’ll be working on this week. And if we don’t ship it immediately, the assumed level of compatibility should mean that we can do that any time later; the only cost is increased size of OS/VM images in the meantime.
signature/sequoia/sequoia_test.go
Outdated
| @@ -0,0 +1,228 @@ | |||
| //go:build containers_image_sequoia | |||
There was a problem hiding this comment.
FIXME: I didn’t read this yet.
There was a problem hiding this comment.
Looks good.
In #2876 , I’ll add a test for the typical workflow, using a non-ephemeral mechanism to sign, + an ephemeral one to verify.
Reminder to self: Eventually, we should aim at ~100% test coverage, but only after the feature set is settled.
macOS: (cd signature/internal/sequoia/rust; DYLD_FALLBACK_LIBRARY_PATH="$(xcode-select --print-path)/Toolchains/XcodeDefault.xctoolchain/usr/lib/" LIBDIR="$(pwd)" cargo build --release)where … and more modifications are required in the caller. |
|
A high-level note: during a personal discussion at DevConf.CZ, it was agreed that the preferred design is to use Sequoia for all signature verifications; but for signing, to expose signing using GnuPG-maintained secret keys, and Sequoia-maintained secret keys, as two separate features in the UI. That way, existing GnuPG keys, and existing |
|
@nwalfield pointed out that, |
|
@ueno I think that might well make sense to do, but not as the first step. I don’t think full transparency is achievable: if we want to support PQC algorithms using the Sequoia implementations, users should be manually generating keys using Sequoia directly, without GnuPG. So, users will need to use So, I think the best UI will be to add new CLI features ( In the first version of this, it would be less work to keep the existing And afterwards, after this time-sensitive work is time-sensitive is done, we can consider replacing the @ueno I can work on the Go / c/image-side work to introduce the new Sequoia-only signing features, and handle the other small comments from the earlier review. (I’d be also happy to work on improvements to the Rust side, with the disclaimer that I’m very new to Rust and completely unfamiliar with the Sequoia codebase and API.) The primary thing I’d need help with is for you and @jnovy to come to an agreement on packaging / ownership. Does the offer that the RHEL Crypto team could own and maintain the Rust-side package still stand? In that case, it would probably make most sense for the Rust code to live in its own repository (with c/image containing just a static copy of the generated headers and CGo stubs); and I think it would be simpler for Podman/Buildah/Skopeo to link directly to a shared library, without |
344c8ea to
1e352d0
Compare
That would be great! I just pushed some fixes (not all) to the issues pointed in the review, FWIW.
Yes, it does.
That would be simpler, though I actually chose this |
I don’t know. @jnovy ? |
1e352d0 to
19cc16f
Compare
This adds a new OpenPGP signing mechanism based Sequoia-PGP[1]. As Sequoia-PGP is written in Rust and doesn't provide a stable C FFI, this integration includes a minimal shared library as a "point solution". To build, first follow the instruction in signature/sequoia/README.md and install `libimage_sequoia.so*` into the library path, and then build with the following command from the top-level directory: $ make BUILDTAGS="btrfs_noversion 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/ Signed-off-by: Daiki Ueno <dueno@redhat.com>
19cc16f to
a83a195
Compare
| let primary_key_handle: KeyHandle = key_handle.parse()?; | ||
| let certs = self | ||
| .certstore | ||
| .lookup_by_cert_or_subkey(&primary_key_handle) |
There was a problem hiding this comment.
If you know you have a primary key handle, then you should use lookup_by_cert instead of lookup_by_cert_or_subkey.
There was a problem hiding this comment.
WIP in ueno/podman-sequoia#4 .
|
|
||
| let mut signing_key_handles: Vec<KeyHandle> = vec![]; | ||
| for cert in certs { | ||
| for ka in cert.keys().with_policy(&self.policy, None).for_signing() { |
There was a problem hiding this comment.
You need to check:
- The cert is not revoked
- The cert is live (not expired)
- The subkey is not revoked
- The subkey is live (not expired)
There was a problem hiding this comment.
Note to self: Do we need more test coverage?
The primary caller does verification in a ~stateless manner, given a signature and a set of acceptable public keys. That input might contain top-level revocations, but that seems unlikely. Expired keys are definitely a possibility, it’s a bit hard to infer user intent in that case.
Also there’s some earlier history in containers/podman#16406 , which says subkeys don’t work with GPGME at all — and some speculation how key revocation / expiration without a trusted timestamp can be problematic.
There was a problem hiding this comment.
Note parallel (coincidental?) #2874 .
There was a problem hiding this comment.
The signing-side subkey filtering is WIP in ueno/podman-sequoia#4 .
But, also, Sequoia does set up a subkey structure, and, by default, signing happens with a subkey. So, for interoperability, we’ll probably need to accept that in the other implementations :|
| MessageLayer::SignatureGroup { ref results } => { | ||
| let result = results.iter().find(|r| r.is_ok()); | ||
| if let Some(result) = result { | ||
| self.signer = Some(result.as_ref().unwrap().ka.cert().to_owned()); |
There was a problem hiding this comment.
As above, you need to check that the certificate and subkey are not revoked and live.
There was a problem hiding this comment.
@nwalfield Isn’t this automatically done by Verifier? https://docs.rs/sequoia-openpgp/latest/sequoia_openpgp/parse/stream/struct.GoodChecksum.html claims this is implied by VerificationResult::Ok, and I read https://gitlab.com/sequoia-pgp/sequoia/-/blob/main/openpgp/src/parse/stream.rs?ref_type=heads#L2844 and following to implement both expiration and revocation of the keys.
There was a problem hiding this comment.
Indeed, you're right. The streaming verifier takes care of that.
That’s not a blocker but fairly inconvenient. I tried to avoid that by regenerating keys in #2875 ; that works with gpgme, but fails for v3 signature packets, I’m still looking into why. |
My fault, the signatures use SHA1. |
If the primary goal is to work around the CI issues with legacy fixture data, it might be simpler to override it with the |
I have updated #2875 , re-generating signatures to use SHA256; now the Sequoia code is passing tests on macOS with the default (= built-in) policy and no modifications. |
| } | ||
|
|
||
| // Migrate GnuPG keyrings if exist. | ||
| for _, keyring := range []string{"pubring.gpg", "secring.gpg"} { |
There was a problem hiding this comment.
If you are only interested in support for gpg v1, this is enough. Otherwise, you also need to also consider pubring.kbx, keybox.db, and all the files under private-keys-v1.d.
There was a problem hiding this comment.
Thanks!
The current plan ( #2569 (comment) ) makes this migration unnecessary; I have removed it in #2876 .
This adds a new OpenPGP signing mechanism based Sequoia-PGP[1]. As
Sequoia-PGP is written in Rust and doesn't provide a stable C FFI,
this integration includes a minimal shared library as a "point
solution".
To build, first follow the instruction in signature/sequoia/README.md
and install
libimage_sequoia.so*into the library path, and thenbuild with the following command from the top-level directory:
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.