-
Notifications
You must be signed in to change notification settings - Fork 545
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
Add COSIGN_OCI_EXPERIMENTAL, push .sig/.sbom using OCI 1.1+ digest tag #2684
Conversation
Amazing! |
Signed-off-by: Josh Dolitsky <[email protected]>
bba1b7f
to
d118235
Compare
Codecov Report
📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more @@ Coverage Diff @@
## main #2684 +/- ##
==========================================
- Coverage 30.11% 29.68% -0.43%
==========================================
Files 150 151 +1
Lines 9483 9670 +187
==========================================
+ Hits 2856 2871 +15
- Misses 6190 6360 +170
- Partials 437 439 +2
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
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.
Super excited about this change!
When COSIGN_OCI_EXPERIMENTAL=1
, we should search for SBOMs etc in both the OCI fallback tag, and the legacy cosign <digest>.sbom
tag, since folks may have uploaded an SBOM using an old version of cosign, or without the env set.
) | ||
|
||
// Referrers fetches references using registry options. | ||
func Referrers(d name.Digest, artifactType string, opts ...Option) (*v1.IndexManifest, error) { |
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.
Can we move this to an internal package? I don't think we want folks outside of cosign to use this.
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 wasn't able to figure out a way to make this method non-public. It is used outside of this package, and unfortunately the options parser (makeOptions) is unavailable outside this package... so this will not reliably pass along registry options etc.
I'm open to other solutions, maybe even panicking if the method is called and the env var is unset 🤷
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.
The rot runs deep.
I think I'm okay with a "this is subject to change, don't use this" in the docstring
Yes, it does this! In the PR description I mention it: "If the digest tag is not found, download/verify commands will fallback to try the cosign-defined tags ( In the code you will see that if an err is returned from one of these experimental methods on fetch, it just prints something to the effect of "Unable to locate attachment using digest tag, trying older scheme" |
FYI - once this PR is in, we just need a go.mod bump to start using the Referrers API endpoint: google/go-containerregistry#1546 |
…-tag Signed-off-by: Josh Dolitsky <[email protected]>
Signed-off-by: Josh Dolitsky <[email protected]>
12e5542
to
42868b1
Compare
After pulling in the latest GGCR, and running a local instance of zot, the SBOM and signature flows above work using the referrers API! No digest tag, no .sbom/.sig tags, nothing! |
Signed-off-by: Josh Dolitsky <[email protected]>
|
||
// WriteSignaturesExperimentalOCI publishes the signatures attached to the given entity | ||
// into the provided repository (using OCI 1.1 methods). | ||
func WriteSignaturesExperimentalOCI(d name.Digest, se oci.SignedEntity, opts ...Option) error { |
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 method also suffers the same problem as Referrers, needing to use makeOptions
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 is awesome, thanks for your patience with the review.
One big question I have: is there a fallback story?
I'd expect there to be an awkward transition period where during this transition"
- to write, we attempt to figure out if a repository supports the Referrers API, and use that if so. otherwise use the old trick
- to read, we attempt to read from Referrers. if that doesn't exist (regardless of whether the repo supports it, we try the old trick
So maybe instead of a boolean UseReferrers
we have a ReferencesMode
enum with values REFERRERS_ONLY
, TAG_HACK_ONLY
, REFERRERS_WITH_FALLBACK
.
The other thing is...there seems to be some missing abstraction here. I see a lot of logic duplicated between SBOMs, signatures, and attachments. Maybe that would hurt more than it helps though.
pkg/cosign/verify.go
Outdated
@@ -461,6 +464,14 @@ func (fos *fakeOCISignatures) Get() ([]oci.Signature, error) { | |||
// VerifyImageSignatures does all the main cosign checks in a loop, returning the verified signatures. | |||
// If there were no valid signatures, we return an error. | |||
func VerifyImageSignatures(ctx context.Context, signedImgRef name.Reference, co *CheckOpts) (checkedSignatures []oci.Signature, bundleVerified bool, err error) { | |||
if b, err := strconv.ParseBool(env.Getenv(env.VariableOCIExperimental)); err == nil && b { |
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.
My preference is actually to make experimental solely a CLI concern. Then, we can add a field OciReferrers: bool
or something to CheckOpts
and rely on that.
Or even better to oci.Options
(aka co.RegistryClientOpts
)!
cmd/cosign/cli/sign/sign.go
Outdated
@@ -311,6 +313,11 @@ func signDigest(ctx context.Context, digest name.Digest, payload []byte, ko opti | |||
ui.Infof(ctx, "Pushing signature to: %s", repo.RepositoryStr()) | |||
} | |||
|
|||
// Publish the signatures associated with this entity (using OCI 1.1+ behavior) | |||
if b, err := strconv.ParseBool(env.Getenv(env.VariableOCIExperimental)); err == nil && b { |
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.
Use exprimental.EnableOciWhatever()
?
) | ||
|
||
// Referrers fetches references using registry options. | ||
func Referrers(d name.Digest, artifactType string, opts ...Option) (*v1.IndexManifest, error) { |
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.
The rot runs deep.
I think I'm okay with a "this is subject to change, don't use this" in the docstring
pkg/oci/remote/remote.go
Outdated
@@ -146,6 +149,13 @@ func attestations(digestable digestable, o *options) (oci.Signatures, error) { | |||
|
|||
// attachment is a shared implementation of the oci.Signed* Attachment method. | |||
func attachment(digestable digestable, attName string, o *options) (oci.File, error) { | |||
if b, err := strconv.ParseBool(env.Getenv(env.VariableOCIExperimental)); err == nil && b { |
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.
Same comment: I'd prefer nothing in pkg/
to know about experimental features, so this should just be an option (in options
?)
pkg/cosign/verify.go
Outdated
return nil, false, fmt.Errorf("unable to locate reference with artifactType %s", artifactType) | ||
} else if numResults > 1 { | ||
// TODO: if there is more than 1 result.. what does that even mean? | ||
fmt.Printf("WARNING: there were a total of %d references with artifactType %s\n", numResults, artifactType) |
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.
Maybe: "expected only 1" in the warning?
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 isn't a terribly actionable warning right now sadly. I guess because we don't know why the heck it would happen, ha
@@ -60,6 +72,67 @@ func SBOMCmd(ctx context.Context, regOpts options.RegistryOptions, sbomRef strin | |||
return remote.Write(dstRef, img, regOpts.GetRegistryClientOpts(ctx)...) | |||
} | |||
|
|||
func sbomCmdOCIExperimental(ctx context.Context, regOpts options.RegistryOptions, sbomRef string, sbomType ocitypes.MediaType, imageRef string) error { |
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 is a bit long for the cmd/
package. Can we move some of it into pkg/
?
There's also a fair bit of overlap with SBOMCmd
. I bet if we reorder things you can actually stick the if EnableOCIExperimental()
at the end of SBOMCmd
and call this something like writeSBOM
.
I still need to give this a test drive, so forgive the uninformed suggestion. I'd recommend consuming both the old and new format regardless of the experimental flag, without any warnings. Make the experimental flag change how the signatures are produced. That results in a smoother transition for orgs with older clients that don't want to simultaneously set the experimental flag on every consumer. |
Signed-off-by: Josh Dolitsky <[email protected]>
Signed-off-by: Josh Dolitsky <[email protected]>
Signed-off-by: Josh Dolitsky <[email protected]>
Signed-off-by: Josh Dolitsky <[email protected]>
Signed-off-by: Josh Dolitsky <[email protected]>
Signed-off-by: Josh Dolitsky <[email protected]>
e2e tests failing, otherwise LGTM |
Signed-off-by: Josh Dolitsky <[email protected]>
sigstore#2684) * Add COSIGN_OCI_EXPERIMENTAL, push .sig/.sbom using OCI 1.1+ digest tag Signed-off-by: Josh Dolitsky <[email protected]> * upgrade to latest ggcr with referrers API support Signed-off-by: Josh Dolitsky <[email protected]> * unexport methods whereever possible Signed-off-by: Josh Dolitsky <[email protected]> * Use ui logger wherever possible Signed-off-by: Josh Dolitsky <[email protected]> * add TODO about using created annotations Signed-off-by: Josh Dolitsky <[email protected]> * remove logging about unable to to find new type of attachment Signed-off-by: Josh Dolitsky <[email protected]> * fixes from review Signed-off-by: Josh Dolitsky <[email protected]> * push the config blob Signed-off-by: Josh Dolitsky <[email protected]> * run docgen Signed-off-by: Josh Dolitsky <[email protected]> * fix for e2e tests Signed-off-by: Josh Dolitsky <[email protected]> --------- Signed-off-by: Josh Dolitsky <[email protected]>
Please see https://github.com/opencontainers/distribution-spec/blob/main/spec.md#referrers-tag-schema
This begins publishing/fetching/verifying signatures and SBOM attachments using the proper OCI defined referrers tag schema, if
COSIGN_OCI_EXPERIMENTAL=1
is set in the environment. If the digest tag is not found, download/verify commands will fallback to try the cosign-defined tags (sha256-<hash>.sig
/sha256-<hash>.sbom
).This means all attached entities will be able to be located via the tag
sha256-<hash>
, which should be formatted as an OCI image index. Manifests defining signatures and SBOMs also add thesubject
field also newly defined by OCI to point to a reference.This also follows guidance by OCI to start using unique mediatypes for config.mediaType to surface as an artifactType:
application/vnd.dev.cosign.artifact.sbom.v1+json
application/vnd.dev.cosign.artifact.sig.v1+json
Setup
Note: I've made the repo
ghcr.io/jdolitsky/cosign-pr-demo
public if anybody wants to inspect the tags, manifests, etc.Get a base image to start attaching to:
Set the new experimental env var:
SBOM flow
Create a fake SBOM:
Attach it:
View repo tags (notice no
.sbom
tag):View the digest tag:
View the manifest for the SBOM artifact:
Fetch the raw SBOM contents:
Signature flow
Sign the image (keyless):
View repo tags (notice no
.sig
tag):View the digest tag (now updated with and second manifest):
View the manifest for the signature artifact:
Verify the signature: