-
Notifications
You must be signed in to change notification settings - Fork 24
Test cleartext signature support in gpgme and sequoia backends #307
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
base: main
Are you sure you want to change the base?
Conversation
I only tested the gpgme backend, but AIUI I think CI will test the other backends. (Right?) |
d2f8d3b
to
a4a1c23
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I only tested the gpgme backend, but AIUI I think CI will test the other backends. (Right?)
Yes, and it is failing in the openpgp
implementation, as suspected.
This then is implying that this is a supported use case.
I’m somewhat tempted to make it a tested/supported but not very documented use case . Well, that’s what this PR is doing anyway, by adding a test and not extending the signature format documentation. :)
The added test LGTM. If the OpenPGP mechanism will need extending, we might need to add extra unit tests if the existing tests don’t exercise all lines.
Yup, that's fine with me! :) Happy to add a marker anywhere else as needed to signal this.
Hmm, right. I do see https://pkg.go.dev/golang.org/x/crypto@v0.41.0/openpgp/clearsign and could try to get that working. Though given golang/go#44226, I wonder if it'd also be reasonable to just... not support it there? |
That would make the clear-signed variant a clearly-unsupported format. That might be fine? To an extent, what matters is the consumers that are relevant to your workflow. The OTOH, for some uses, third-party SBOM / policy checkers and image audit workflows can use ~arbitrary implementations, perhaps not even c/image, and in that case it would be hard to argue that a format not universally accepted even by c/image should be supported by all such software. |
a4a1c23
to
310afd4
Compare
OK added support for it in the openpgp backend as well! |
Anything else on this one? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I’m afraid there’s a rather major snag with this plan: the CheckDetachedSignature
code path is not enforcing everything the existing code does.
In particular, the SigLifetimeSecs
check is, AFAICS, impossible to do with CheckDetachedSignature
because the function does not return the signature packet.
(Ultimately, for confidence, a lot of TestGPGSigningMechanismVerify
should be duplicated with clear-signed signatures, to ensure parity. Purely by code review, it seems mostly fine — but the signature expiration is a clear divergence.)
I don’t know what’s an easy path forward here, given the deprecated/frozen status of x/crypto/openpgp
. Moving away from that backend, maybe to one of its forks, seems possible (e.g., from a random look which is by no means a recommendation, ProtonMail/go-crypto
seems to have code that both returns the data, and enforces validity automatically), but that’s a somewhat different project / PR.
image/signature/mechanism_openpgp.go
Outdated
// or the mechanism should implement signingMechanismWithVerificationIdentityLookup. | ||
func (m *openpgpSigningMechanism) Verify(unverifiedSignature []byte) (contents []byte, keyIdentity string, err error) { | ||
// First, try to decode it as a clearsigned message. | ||
block, _ := clearsign.Decode(unverifiedSignature) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Purely aesthetically, I’d prefer the expected code path, i.e. a non-clearsigned signature, to be tried first. Ultimately that makes no difference to security.)
image/signature/mechanism_openpgp.go
Outdated
// It is a clearsigned message. | ||
signer, err := openpgp.CheckDetachedSignature(m.keyring, bytes.NewReader(block.Bytes), block.ArmoredSignature.Body) | ||
if err != nil { | ||
return nil, "", err |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This failure path does not have test coverage.
Ahh, tough. So let me narrow this down a bit more. My main goal here is to make sure signature verification of update payloads in FCOS clients don't break in the future before we switch over to Sigstore. So I'm most particularly interested in the paths relevant for Fedora, which I think today is the gpgme backend (but possibly in the future Sequoia?), and those are covered. I.e. going back to this conversation:
Yeah, I'm personally OK with us not claiming full support for it and with the third-party caveat. What I mostly want to avoid is a change that lands here that breaks the FCOS workflow and only finding out when we're about to do a release and fail update testing. |
Per the above, I think this should work — assuming there are no relevant consumers of these signatures choosing to use OpenPGP:
That would ensure that we’d see blocking test failure if this broke (which would be an API break of GPGME / Sequoia, and I think very unlikely), and in that unlikely case we could discuss what to do. |
Currently, simple signatures are expected to be in the binary GPG format, and that's what e.g. `podman push --sign-by` produces as well. But the code for all backends work today with cleartext signatures. Add a new test to cover this case. But only in the GPGME and Sequoia backends since the OpenPGP backend does not support it and cannot easily be supported (see also [1]). The reason why I'm interested in this is that I'd like to make use of it for signing Fedora CoreOS container images. The end-goal is to move to Sigstore signing, but until that's ready, we'd like to use GPG signing. We use Robosignatory, the Fedora signing service, which only supports detached signatures, and while it's theoretically possible to convert the detached signatures we get back into inline binary signatures, it's much less cumbersome and error-prone to convert it to cleartext signatures. It's worth noting that while Fedora's signing server (Sigul) does support container image signing, Robosignatory does not surface it yet (see https://pagure.io/robosignatory/issue/22). Fixing that wouldn't be too hard I think, but all this is ideally short-term anyway until we can move to Sigstore signing + Konflux. There's work in progress on that (see e.g. https://discussion.fedoraproject.org/t/148999). The primary goal here is just ensuring that this keeps working until we move off of it. Signed-off-by: Jonathan Lebon <jonathan@jlebon.com> [1]: containers#307 (review)
310afd4
to
44e128f
Compare
Updated to drop the "supported" wording, the openpgp implementation, and moved the tests to the gpgme and sequoia backends! |
Hmm, the Sequoia CI issue I think is unrelated. Might just be infra flake. I did test the sequoia backend locally and tests passed. |
Just a quick note now,
is a drift in the expected test message caused by differing version of Podman-sequoia, and IIRC we have updated for that. Can you try rebasing, first? |
Currently, simple signatures are expected to be in the binary GPG format, and that's what e.g. `podman push --sign-by` produces as well. But the code for all backends work today with cleartext signatures. Add a new test to cover this case. But only in the GPGME and Sequoia backends since the OpenPGP backend does not support it and cannot easily be supported (see also [1]). The reason why I'm interested in this is that I'd like to make use of it for signing Fedora CoreOS container images. The end-goal is to move to Sigstore signing, but until that's ready, we'd like to use GPG signing. We use Robosignatory, the Fedora signing service, which only supports detached signatures, and while it's theoretically possible to convert the detached signatures we get back into inline binary signatures, it's much less cumbersome and error-prone to convert it to cleartext signatures. It's worth noting that while Fedora's signing server (Sigul) does support container image signing, Robosignatory does not surface it yet (see https://pagure.io/robosignatory/issue/22). Fixing that wouldn't be too hard I think, but all this is ideally short-term anyway until we can move to Sigstore signing + Konflux. There's work in progress on that (see e.g. https://discussion.fedoraproject.org/t/148999). The primary goal here is just ensuring that this keeps working until we move off of it. Signed-off-by: Jonathan Lebon <jonathan@jlebon.com> [1]: containers#307 (review)
44e128f
to
40ee327
Compare
Currently, simple signatures are expected to be in the binary GPG format, and that's what e.g.
podman push --sign-by
produces as well. But the code for all backends work today with cleartext signatures.Add a new test to cover this case. But only in the GPGME and Sequoia backends since the OpenPGP backend does not support it and cannot easily be supported (see also 1).
The reason why I'm interested in this is that I'd like to make use of it for signing Fedora CoreOS container images. The end-goal is to move to Sigstore signing, but until that's ready, we'd like to use GPG signing.
We use Robosignatory, the Fedora signing service, which only supports detached signatures, and while it's theoretically possible to convert the detached signatures we get back into inline binary signatures, it's much less cumbersome and error-prone to convert it to cleartext signatures.
It's worth noting that while Fedora's signing server (Sigul) does support container image signing, Robosignatory does not surface it yet (see https://pagure.io/robosignatory/issue/22).
Fixing that wouldn't be too hard I think, but all this is ideally short-term anyway until we can move to Sigstore signing + Konflux. There's work in progress on that (see e.g. https://discussion.fedoraproject.org/t/148999).
The primary goal here is just ensuring that this keeps working until we move off of it.