-
Notifications
You must be signed in to change notification settings - Fork 548
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
Require a payload to be provided with a signature #2785
Conversation
Codecov Report
@@ Coverage Diff @@
## main #2785 +/- ##
==========================================
- Coverage 29.47% 29.42% -0.06%
==========================================
Files 151 152 +1
Lines 9678 9712 +34
==========================================
+ Hits 2853 2858 +5
- Misses 6386 6414 +28
- Partials 439 440 +1
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
c69a550
to
22af56b
Compare
When verifying, we need some link between the artifact that's being verified and the provided hash. Otherwise, users are just providing a hash and signature, which might not link to the image they want to verify. If the concern is changes to the marshaller, we may want to pick something like canonicalized JSON (like what we do for Rekor) to have a guaranteed order. We would need to transition over and still support verification with previously generated signatures. I'm curious the motivation for this PR. Have there been changes to Golang's marshaller in the past? For a PR specific comment, we should avoid breaking changes right now, as we just shipped a new major version of Cosign. Any changes should be additive. We can put a deprecation warning in, but shouldn't break users until we have a roadmap for a new major release. |
Such things are not unheard of ( golang/go#14135 ), though that’s admittedly more a pedantic concern than something I would think is very likely.
The description says
My motivation is handling other data, not just the digest, in the payload. That’s conceptually been always possible, both In particular, I’d like to change the default payload contents, per #2047 (comment) . (See https://github.com/mtrmac/cosign/tree/signed-identity for a WIP version if you are curious.) That would definitely break the consumers that throw away the payload; hence this PR first, with
Works for me. |
Updated: The generation of implied payload is now restored, consolidated into |
How is the blob or container used for verification? I'm still not sold on users providing the hash instead of the container/blob, because you get into cases where a user wants to verify X but are given a hash/sig to Y. If the issue is that additional metadata/annotations are not bundled alongside the blob/container, should we instead be finding ways to persist those? https://github.com/sigstore/protobuf-specs might be the answer to that, attaching the additional metadata alongside the artifact. |
This PR changes absolutely nothing in that respect.
I’m not sure what this refers to; nothing like that happens, and I’m not proposing adding nothing like that either, AFAICS. Where exactly are users “providing the hash”? Sure, I guess, users running (It is a risk that users verify bar@sha256… and then carelessly run maybe bar:v1.2.3, but that’s not a risk that can be fixed with the
That’s exactly what this PR is doing! The data is already been generated during signing, so this adds a new option to persist the data on signing, and to use that data on verification, where those options were missing. (And, as a reminder, nothing of this is user-relevant in the ~typical scenarios of |
Just to be explicit, and to see if I’m completely wrong about some basic assumption (and to possibly agree on the terminology), the container signing design right now is:
This PR only affects uses where a “signature bundle” is not involved at all, i.e. when the signatures are not stored on container registries, and nevertheless we are signing containers. |
Ah, I misunderstood, thanks for your explanation. I had thought payload was referring to just a hash that’s signed over, hence my reservation. Now I see this is meant to be a set of values as additional metadata. I’ll take another look tomorrow! |
I'd just like to +1 the general notion of preferring to include the signed payload with the signature instead of relying on implicit payload generation, particularly if canonicalization is involved. The fewer dependencies needed for the authentication step of consuming attestations, the better. This was one of the design motivations for DSSE when we were comparing it against existing signature enveloping schemes, like In-Toto's. Here's one relevant comment in the broader thread: secure-systems-lab/dsse#1 (comment) |
... to replace SEVEN individual parameters. This will make it easier to add more parameters in the future. Should not change behavior. Signed-off-by: Miloslav Trmač <[email protected]>
--output-signature makes no sense without --output-payload; otherwise users would need to somehow deterministically regenerate the payload to be able to verify the signature. Signed-off-by: Miloslav Trmač <[email protected]>
Signed-off-by: Miloslav Trmač <[email protected]>
The signature signs the payload; it makes no sense for the user to provide the signature but not the payload - it would effectively force cosign to generate a byte-for-byte identical (and, currently, undesirable) payload forever. Still, for compatibility, continue to accept such invocations, but trigger a warning. Signed-off-by: Miloslav Trmač <[email protected]>
…ation ... and warn if that's not the case. The signature signs the payload; it makes no sense for the user to provide the signature but not the payload - it would effectively force cosign to generate a byte-for-byte identical (and, currently, undesirable) payload forever. Still, for compatibility, continue to accept such invocations, but trigger a warning. Signed-off-by: Miloslav Trmač <[email protected]>
Rebased, PTAL. |
Thanks @mtrmac for the PR and the detailed explanation. I'm also in favor of avoiding implicit payload generation. A minor point:
FWIW we can break users after at least 6 months. So I think a warning if --payload is omitted for verify is okay for now. |
Given that we know we want to integrate with the sigstore bundle format defined in https://github.com/sigstore/protobuf-specs/, should we hold off in this PR and instead add payload to the protobuf (if it's not already there, sorry, I don't recall)? I'd prefer we don't add another option to Cosign, especially given the feedback we've received so far with other flags that we added. |
I don’t think that’s directly relevant: The existing “signature bundle” format already contains a payload, and this PR changes nothing in the use cases and code paths where a “signature bundle” is used. It’s the non-bundle commands and option combinations that currently make a deterministic payload assumption; and that’s what this PR is proposing to change. If the project were moving towards new data structure formats[1], and isn’t interested in keeping feature parity in the non-bundle use cases, fine; I can work on the payload change proposals just as well without this PR. In that case, though, I think it could be useful to at least warn users if they invoke the old non-bundle commands with the deterministic payload assumption, even if we don’t add the new options to make payloads explicit. Would you be interested in a variant of this PR that adds the warnings, but not the new options?
I don’t really feel comfortable reverse-engineering a design just from a protobuf; with that caveat, the DSSE format does seem to have a payload of some kind, the I'd prefer we don't add another option to Cosign, especially given the feedback we've received so far with other flags that we added. [1] (I’m a bit unhappy about the extra work keeping up with changes to the data representation would cause us…) |
Taking some time to sort through this finally, appreciate your patience. I think that the right thing to do long-term is definitely to fold this into the protobuf Signature bundle format. This is intended to be an opaque blob that gets passed around containing all of the material needed for verification. We might do that by adding a "PayloadAndSignature" That said, in the short term this does need to get done. I agree generally that more flags is unpleasant, but we can get rid of it after we fold the bundles in (since we have to get rid of a bunch of flags anyway). I'm okay with the PR as-is (and talking to @haydentherapper offline he is too). |
Summary
In near future, I plan to propose changing the contents of the “signature payload” (the blob that is signed by the raw cryptographic signature.
That’s currently not cleanly possible because various code paths assume that the payload can generated afresh just from the image name/digest. Because the cryptographic signature signs that payload, that essentially forces us to freeze the payload contents forever, and assumes that Golang’s JSON marshaler will always produce byte-for-byte identical output to its current version. That’s not a reasonable thing to assume long-term.
So, make it explicit that that’s not how signing workflows should work, and wherever the user is providing a signature, force the user to also provide the payload signed by that signature. This, I think, mostly affects low-level users that manipulate the contents of the signature bundle as individual components, but not most users that operate with the signature bundle as a whole (or don’t think about the bundle at all).
(Alternatively, we could not require the payload to be provided now, and assume the current content for compatibility, with a warning. But that risks breaking the users at some inopportune time not under anyone’s control, maybe with a Go security update, instead of being able to manage that transition within Cosign. Based on the unquantified guess that this is not likely to affect most users, I think just breaking these uses might be acceptable.)
See just below for the CLI changes.
See individual commit messages for a bit more detail.
Release Note
--output-payload
option tocosign sign
, to be used by users of--output-signature
.cosign attach signature
now warns if the--payload
option is not used along with--signature
.--payload
option tocosign verify
,cosign dockerfile verify
andcosign manifest verify
; these commands now warn if--payload
is not used along with--signature
.Documentation
sigstore/docs#84