Skip to content
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

Feature: Allow cosign to sign digests before they are uploaded. #2959

Merged
merged 2 commits into from
Jun 2, 2023

Conversation

mattmoor
Copy link
Member

@mattmoor mattmoor commented May 7, 2023

🎁 This feature allows cosign to sign a digest that doesn't exist yet, to support scenarios such as signing an image prior to pushing it.

This adapts the ideas from the two prior approaches, which were closed by stale-bot.

Fixes: #1905

/kind feature

Release Note

  • cosign can now be used to sign digests before the image has been pushed to a registry (this only supports shallow signing of digests, so tag-based signing and recursive signing are not supported because both need the image structure available)

Documentation

@codecov
Copy link

codecov bot commented May 7, 2023

Codecov Report

Merging #2959 (1968ec2) into main (3121a17) will increase coverage by 0.54%.
The diff coverage is 81.31%.

@@            Coverage Diff             @@
##             main    #2959      +/-   ##
==========================================
+ Coverage   30.26%   30.81%   +0.54%     
==========================================
  Files         151      152       +1     
  Lines        9469     9552      +83     
==========================================
+ Hits         2866     2943      +77     
- Misses       6158     6163       +5     
- Partials      445      446       +1     
Impacted Files Coverage Δ
cmd/cosign/cli/attest/attest.go 0.00% <0.00%> (ø)
cmd/cosign/cli/sign/sign.go 14.56% <0.00%> (-0.08%) ⬇️
pkg/oci/remote/remote.go 38.25% <0.00%> (ø)
pkg/oci/mutate/mutate.go 83.82% <83.33%> (+2.54%) ⬆️
pkg/oci/remote/unknown.go 100.00% <100.00%> (ø)

hectorj2f
hectorj2f previously approved these changes May 7, 2023
Copy link
Contributor

@hectorj2f hectorj2f left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@mattmoor
Copy link
Member Author

mattmoor commented May 7, 2023

I changed it to a hardcoded digest since the macos leg doesn't have sha256sum.

@mattmoor
Copy link
Member Author

mattmoor commented May 7, 2023

I'm going to fix cosign attest too while I'm in here, since it has the same problem.

@mattmoor
Copy link
Member Author

mattmoor commented May 8, 2023

Ok, so the CI failure is a legit failure, and I'm glad it caught this because there's a subtle bug with this approach!

Essentially the ociempty approach is subtly flawed in that it doesn't allow the signing/attesting logic to access pre-existing signatures and attestations, so sign and attest always overwrite the existing signatures instead of augmenting them.

I think this will require some changes more integrated into ociremote.SignedEntity itself, which needs some thought.

@mattmoor
Copy link
Member Author

mattmoor commented May 8, 2023

Ok, this no longer uses ociempty and I introduced an ociremote.SignedUnknown that we can explicitly fallback on (e.g. when we see the new ErrEntityNotFound) to support these cases.

It felt wrong to automatically do this in SignedEntity since it would break existing walks that assume a SignedEntity must be one of oci.Signed{Entity,Image}, but I actually like this a lot more than the prior approach because this is digestable (we know the digest), but it doesn't pretend to be a v1.Image (or index).

@mattmoor
Copy link
Member Author

mattmoor commented May 8, 2023

Blah, it still needs some work to pass tests.

🎁 This feature allows `cosign` to sign and attest a digest that doesn't exist yet, to support scenarios such as signing an image prior to pushing it.

This adapts the ideas from the two prior approaches, which were closed by stale-bot.

Fixes: sigstore#1905

/kind feature

Signed-off-by: Matt Moore <mattmoor@chainguard.dev>
@mattmoor
Copy link
Member Author

mattmoor commented May 8, 2023

This is RFAL!

hectorj2f
hectorj2f previously approved these changes May 8, 2023
"foo": "bar"
}
EOF
./cosign attest --key ${signing_key} --type custom --predicate "${PREDICATE_FILE}" "$img3@sha256:17b14220441083f55dfa21e1deb3720457d3c2d571219801d629b43c53b99627"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you attest something else and check that both are present, to guard against the overwriting behavior you discovered?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's already a Go test that checks exactly that (which is what caught the bug!), and I'm not sure doing it for the "unknown" case (or in bash) adds much value 😅

}
// We don't actually need to access the remote entity to attach things to it
// so we use a placeholder here.
se := ociremote.SignedUnknown(digest)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

WDYT about s/Unknown/Digest/ everywhere? Unknown doesn't feel quite right to me.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I worry that Digest isn't clear enough, since it is effectively what SignedEntity is (especially after my PR to add Digest to oci.SignedEntity).

That said, I don't feel particularly strongly about the naming here :bikeshed:

pkg/oci/remote/unknown.go Outdated Show resolved Hide resolved
pkg/oci/remote/unknown_test.go Outdated Show resolved Hide resolved
@@ -179,7 +179,9 @@ func SignCmd(ro *options.RootOptions, ko options.KeyOpts, signOpts options.SignO

if digest, ok := ref.(name.Digest); ok && !signOpts.Recursive {
se, err := ociremote.SignedEntity(ref, opts...)
if err != nil {
if err == ociremote.ErrEntityNotFound {
se = ociremote.SignedUnknown(digest)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you sure we always want to do this on 404?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think yes, but do you have an alternative in mind?

Co-authored-by: Jon Johnson <jonjohnsonjr@gmail.com>
Signed-off-by: Matt Moore <mattomata@gmail.com>
@imjasonh
Copy link
Member

Ping. I think this looks good (I have no power to approve), and I'm not sure any of the remaining comments are blocking. I don't want to lose momentum on this long-standing feature request. 🤞

Copy link
Member

@cpanato cpanato left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This lgtm
Will wait for @jonjohnsonjr because made some comments here

@jonjohnsonjr
Copy link
Contributor

This is not great because OCI referrers stuff expects a whole descriptor (digest+size+mediaType). It would be better if this took a Descriptor, assuming we want to adopt the OCI referrers stuff down the road.

@imjasonh
Copy link
Member

This is not great because OCI referrers stuff expects a whole descriptor (digest+size+mediaType). It would be better if this took a Descriptor, assuming we want to adopt the OCI referrers stuff down the road.

Good point. However, in that case, I think we can just set the subject without a size and maybe with some default mediaType, to denote "I'm attaching to something I don't know anything else about".

"subject": {
  "digest": "sha256:abcdef...",
  "size": 0,
  "mediaType": "application/octet-stream"
}

We can either establish this convention and use it, or try to specify this better in OCI image-spec. Supporting this case has been a goal of OCI referrers, and if implementers like cosign don't know how to achieve it then OCI should help them. Or if they want to solve it with this convention, that works too, we just need registries not to validate that the size+mediaType matches reality.

@mattmoor
Copy link
Member Author

This is not great because OCI referrers stuff expects a whole descriptor (digest+size+mediaType). It would be better if this took a Descriptor, assuming we want to adopt the OCI referrers stuff down the road.

Digests need to be fully self-describing, the absence of this property was (as you know) a CVE.

@jonjohnsonjr
Copy link
Contributor

Digests need to be fully self-describing, the absence of this property was (as you know) a CVE.

Sure, but it's nice to know the size prior to decoding the payload, which you can do once you have an entrypoint into the dag. If you want to go against the grain, that's fine, but I'd file an issue with OCI.

@imjasonh
Copy link
Member

imjasonh commented Jun 2, 2023

Ping. I think this looks good (I have no power to approve), and I'm not sure any of the remaining comments are blocking. I don't want to lose momentum on this long-standing feature request. 🤞

@priyawadhwa
Copy link
Contributor

I'll go ahead and merge since it seems like next steps are to file an issue with OCI (sorry if I'm reading this thread wrong!).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Signing images by digest that don't exist yet
6 participants