sigstore-bundle: add sigstore bundle image verification #567
Conversation
p5
changed the title
|
✅ A new PR has been created in buildah to vendor these changes: containers/buildah#6613 |
|
Packit jobs failed. @containers/packit-build please check. |
p5
force-pushed
the
dev/robertsturla/sigstore-bundle-verification
branch
from
a285903 to
1f0c308
Compare
p5
force-pushed
the
dev/robertsturla/sigstore-bundle-verification
branch
from
1f0c308 to
98629c5
Compare
github-actions
bot
added
storage
p5
force-pushed
the
dev/robertsturla/sigstore-bundle-verification
branch
8 times, most recently
from
374a72c to
2f9c022
Compare
Update the minimum Go version requirement to 1.25.0 in go.work and image/go.mod to support sigstore dependencies that require Go 1.25.0. Signed-off-by: Robert Sturla <rsturla@redhat.com>
Signed-off-by: Robert Sturla <rsturla@redhat.com>
p5
force-pushed
the
dev/robertsturla/sigstore-bundle-verification
branch
from
2f9c022 to
07102dd
Compare
Signed-off-by: Robert Sturla <rsturla@redhat.com>
Signed-off-by: Robert Sturla <rsturla@redhat.com>
p5
force-pushed
the
dev/robertsturla/sigstore-bundle-verification
branch
from
07102dd to
7dd6b21
Compare
…atures Signed-off-by: Robert Sturla <rsturla@redhat.com>
Signed-off-by: Robert Sturla <rsturla@redhat.com>
p5
force-pushed
the
dev/robertsturla/sigstore-bundle-verification
branch
from
7dd6b21 to
b5c927b
Compare
mtrmac
left a comment
mtrmac
left a comment
There was a problem hiding this comment.
Thanks!
I’m very interested in this feature set, and this is very welcome.
For now, just an extremely brief skim:
- It’s unclear that we can update to Go 1.25 until Fedora 42 goes EOL (scheduled for May)
- We will also need support for creating signatures, although that’s of course not at all a blocker for this PR.
- The signature verification code paths in
signature/…, being so security critical, will need as close to 100% test coverage as possible. (Premature to discuss now when I haven’t studied the new format, just as a FYI for planning the future.) - I feel very strongly that signature verification must also enforce image identity as a component of the signature. If DSSE can’t do that, we’ll need to figure out some path forward.
- There’s a balance to be struck on external dependencies … I’d generally prefer to share code, but the signature verification code has no business making internet connections, so the
go-openapipart suggests we might want to do something differently. - Similarly, I’d prefer to avoid TUF and its complexity; I’m not sure whether that is possible.
Assisted by: …
(Generally it’s the projects’ position that the submitter is responsible for the contents of the PR in its entirety.)
The bunch of review comments is just a mishmash of immediate thoughts, I didn’t want to now take the time the time to organize or prioritize.
|
|
||
| msg "Executing tests as $ROOTLESS_USER" | ||
| showrun ssh $ROOTLESS_USER@localhost make -C $GOSRC test "BUILDTAGS='$BUILDTAGS'" "TESTFLAGS=-v" "REKOR_SERVER_URL='http://127.0.0.1:3000'" | ||
| showrun ssh $ROOTLESS_USER@localhost "GOTOOLCHAIN=go1.25.0 GOSUMDB=sum.golang.org make -C $GOSRC test BUILDTAGS='$BUILDTAGS' TESTFLAGS=-v REKOR_SERVER_URL='http://127.0.0.1:3000'" |
There was a problem hiding this comment.
Similarly to the cirrus changes, this was necessary due to the remote CI runner pre-installing Go 1.24.X, and therefore compilation failing to meet dependency requirements. These changes to the runners/CI scripts were the interesting hacks needed to get a newer Go version to work.
I wanted to use auto here to set this dynamically based on the mod file but that didn't include all the tools needed (the coverage tool was missing?).
Ideally once this lands, the remote machine would have a 1.25.x version of Go preinstalled and these wouldn't be needed.
There was a problem hiding this comment.
Ideally once this lands, the remote machine would have a 1.25.x version of Go preinstalled and these wouldn't be needed.
Yes.
| // It filters for common sigstore artifact types and returns the matching descriptors. | ||
| // Returns nil if the referrers API is not supported or no sigstore signatures exist. | ||
| func (c *dockerClient) getSigstoreReferrers(ctx context.Context, ref dockerReference, manifestDigest digest.Digest) ([]imgspecv1.Descriptor, error) { | ||
| // First try without artifact type filter to get all referrers |
There was a problem hiding this comment.
There is no “second try” AFAICS… so the artifact filtering code is unused, isn’t it?
There was a problem hiding this comment.
Agree that the comment could be better - "First try" was in the context of the caller before it was split out, but I'm not quite following the claim that the filtering it's unused.
There was a problem hiding this comment.
The only caller of getReferrers sets artifactType to "".
| for _, tc := range testCases { | ||
| t.Run(tc.digest, func(t *testing.T) { | ||
| // This mimics the conversion in getReferrersFromTag | ||
| tag := replaceColonWithDash(tc.digest) |
There was a problem hiding this comment.
Careful with the AI … this is not testing anything at all, this function only exists in the tests!
(When I mention “as close to 100% coverage as possible” for the verification path, that means carefully designed test coverage, not this.)
There was a problem hiding this comment.
This is completely fair - I've been mostly focused on the non-test code and wanted to get early thoughts before spending much more time on it.
I'm sure there will be a few more AI-isms in the tests that slipped through.
| // isSigstoreReferrer Tests | ||
| // ============================================================================= | ||
|
|
||
| func TestIsSigstoreReferrer(t *testing.T) { |
There was a problem hiding this comment.
This can be a table-driven test, almost certainly much more concise.
| return err | ||
| } | ||
|
|
||
| // Try OCI 1.1 referrers API first (Cosign v3 / sigstore bundle format) |
There was a problem hiding this comment.
I wonder whether we might need an opt-in, like we have useSigstoreAttachments. (I was on the fence with useSigstoreAttachments, and it turned out to be immediately necessary for interoperability with a slightly unusual registry. That’s of course just an anecdote. OTOH an opt-in is a hassle for users.)
There was a problem hiding this comment.
I was more thinking users shouldn't need to know what type of signatures are being used, it should "just work". Even I don't actually know definitively what to call the new/old versions of the formats.
There was a problem hiding this comment.
I agree having things “just work” is ideal.
Compare #568 .
| return sarRejected, internal.NewInvalidSignatureError("bundle does not contain a transparency log entry required for Fulcio verification") | ||
| } | ||
|
|
||
| integratedTime, err := bundle.GetIntegratedTime() |
There was a problem hiding this comment.
(I suspect other comments from verifyMessageSignatureBundle apply to this function.)
| // Now verify the DSSE envelope signature using the verified public key | ||
| result, err := internal.VerifyBundle(bundle.RawBytes(), internal.BundleVerifyOptions{ | ||
| PublicKeys: []crypto.PublicKey{pk}, | ||
| SkipTlogVerification: true, | ||
| }) | ||
| if err != nil { | ||
| return sarRejected, err | ||
| } | ||
|
|
||
| return pr.validateDSSEPayload(ctx, image, result.EnvelopePayload) |
There was a problem hiding this comment.
The code duplication across case branches would be better factored out, to minimize risk of drift.
| // The payload is typically an in-toto attestation or a simple signing payload. | ||
| // The DSSE signature has already been verified, so we just need to validate the content. | ||
| func (pr *prSigstoreSigned) validateDSSEPayload(ctx context.Context, image private.UnparsedImage, payload []byte) (signatureAcceptanceResult, error) { | ||
| // Try to parse as a simple signing payload first |
There was a problem hiding this comment.
Note to self: “Try to parse” worries me. Check whether that is necessary.
| // validateParsedPayload validates a parsed simple signing payload against the image. | ||
| func (pr *prSigstoreSigned) validateParsedPayload(ctx context.Context, image private.UnparsedImage, payload *internal.UntrustedSigstorePayload) error { | ||
| // Validate the docker reference | ||
| if !pr.SignedIdentity.matchesDockerReference(image, payload.UntrustedDockerReference()) { |
There was a problem hiding this comment.
See elsewhere about centralizing this kind of enforcement.
| } | ||
|
|
||
| // validateInTotoStatement validates an in-toto statement against the image. | ||
| func (pr *prSigstoreSigned) validateInTotoStatement(ctx context.Context, image private.UnparsedImage, statement *internal.InTotoStatement) error { |
There was a problem hiding this comment.
pr.SignedIdentity this is unacceptable. (Yes, I realize that might affect interoperability. TBD.)
|
Thanks for the review! I was aware it wouldn't be near ready, but this is quite a bit more underbaked than I thought. This was just after the "get it working" stage and a small bit of cleaning up. The purpose was primarily to make sure I'm somewhat along the right lines before I spend longer getting it ready. Hence the CI hacks to simply get the tests to run.
The introduction of a new external dependency was one of the things I was most worried about, as both it changes the Go version requirements, but generally keeping the dependency chain small is ideal. I thought in this case it was fine since it's deferring sigstore handling to sigstore's package.
I think I'll call bankruptcy on the current tests, and start fresh. My previous experiences have been decent with regards to generated tests, but in this case, they seem to be more decorative than useful. The Fulcio/Rekor/non-" |
|
On the dependencies — in principle I would prefer to use as much of the Structurally this does hit all the relevant places roughly correctly. I need to actually study the v3 formats in detail to have a more specific opinion… e.g. it’s possible that we might want to use different |
3 participants
Fixes #388
This is a very early stage implementation of the new sigstore bundle format verification for use in podman, skopeo and co.
Cosign v3 was released a few months ago with a change to the default format they use for signatures. The new format is not compatible with this library, and therefore verification fails on any image pushed using the default settings in cosign v3.
This does NOT implement pushing new signatures - purely reading and verifying.
I expect to iterate over this PR in draft for a while. Just raised this here for some early feedback. Fulcio is completely new to me and have not yet been able to test this use-case, yet.
Test cases were pretty heavily implemented by Claude Code with
closeguidance from myself.Assisted by: Claude Opus 4.5 via Cursor