feat: attach: attestation: allow passing multiple payloads #2085

Dentrax · 2022-07-20T21:30:41Z

Signed-off-by: Furkan furkan.turkal@trendyol.com
Co-authored-by: Batuhan batuhan.apaydin@trendyol.com

Fixes #2052

Summary

Release Note

Allow passing multiple attestation payloads to "attach attestation" command

Documentation

Dentrax · 2022-07-20T21:35:27Z

$ ./cosign attach attestation --attestation at1.json --attestation at2.json furkanturkal/busybox:0.1.0

Using payload from: at1.json (0205d4c0d9d12706c126515b1ef452e3c7460de647aeec33d1ed01bd172e98de)
Using payload from: at2.json (d2a9efb2c1621823ecb3dc270a74d5ded58be7219080875be0349b625bae7206)

{
  "schemaVersion": 2,
  "mediaType": "application/vnd.oci.image.manifest.v1+json",
  "config": {
    "mediaType": "application/vnd.oci.image.config.v1+json",
    "size": 342,
    "digest": "sha256:9641fd9d3b1b7e18ab7ed6b7fe6c232e4969043d188902aa192843cb9c3eec84"
  },
  "layers": [
    {
      "mediaType": "application/vnd.dsse.envelope.v1+json",
      "size": 4939,
      "digest": "sha256:0205d4c0d9d12706c126515b1ef452e3c7460de647aeec33d1ed01bd172e98de",
      "annotations": {
        "dev.cosignproject.cosign/signature": ""
      }
    },
    {
      "mediaType": "application/vnd.dsse.envelope.v1+json",
      "size": 5999,
      "digest": "sha256:d2a9efb2c1621823ecb3dc270a74d5ded58be7219080875be0349b625bae7206",
      "annotations": {
        "dev.cosignproject.cosign/signature": ""
      }
    }
  ]
}

TomHennen · 2022-07-21T12:43:12Z

Thanks for working on this!

Looking at the example and code it looks like this doesn't follow the intent of #2052 which wants the attach command to support reading multiple attestations from a single file in the in-toto bundle format.

The expected behavior is that a single file would have multiple attestations contained within it (one per line) and that attach should read and attach all of them.

e.g.

{ "payloadType": "application/vnd.in-toto+json", "payload": "a...", "signatures": [w...] }
{ "payloadType": "application/vnd.in-toto+json", "payload": "b...", "signatures": [w...] }

The behavior here, letting the user specify multiple files, does seem useful though. I wouldn't be surprised if some users would like this capability, it's just not quite what I was looking for in #2052.

WDYT?

developer-guy · 2022-07-21T13:54:01Z

Oh, thanks, @TomHennen, we couldn't understand the issue at first, but now we are okay with the idea and will fix it soon, but @dlorenc, what do you think about this PR? It seems useful for people who might want to add multiple attestation files.

codecov-commenter · 2022-07-27T06:04:45Z

Codecov Report

Merging #2085 (0fae11e) into main (8f2c36d) will decrease coverage by 0.70%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##             main    #2085      +/-   ##
==========================================
- Coverage   26.33%   25.62%   -0.71%     
==========================================
  Files         129      130       +1     
  Lines        7564     7597      +33     
==========================================
- Hits         1992     1947      -45     
- Misses       5317     5412      +95     
+ Partials      255      238      -17

Impacted Files	Coverage Δ
cmd/cosign/cli/attach.go	`0.00% <0.00%> (ø)`
cmd/cosign/cli/options/attach.go	`0.00% <0.00%> (ø)`
pkg/oci/layout/signatures.go	`0.00% <0.00%> (-53.85%)`	⬇️
pkg/oci/layout/write.go	`0.00% <0.00%> (-37.50%)`	⬇️
pkg/oci/layout/index.go	`0.00% <0.00%> (-28.58%)`	⬇️
pkg/blob/load.go	`68.57% <0.00%> (-5.72%)`	⬇️
pkg/oci/static/file.go	`64.70% <0.00%> (-5.30%)`	⬇️
pkg/oci/mutate/signatures.go	`33.33% <0.00%> (-0.67%)`	⬇️
pkg/policy/eval.go	`78.94% <0.00%> (-0.54%)`	⬇️
pkg/cosign/rego/rego.go	`70.96% <0.00%> (-0.47%)`	⬇️
... and 6 more

Help us with your feedback. Take ten seconds to tell us how you rate us.

dlorenc · 2022-07-27T09:30:27Z

Seems fine. When we support bundles, would we just detect them in the same file/flag or would that be different?

Fixed sigstore#2052 Signed-off-by: Furkan <furkan.turkal@trendyol.com> Co-authored-by: Batuhan <batuhan.apaydin@trendyol.com> Signed-off-by: Batuhan Apaydın <batuhan.apaydin@trendyol.com>

developer-guy · 2022-07-27T20:49:44Z

Seems fine. When we support bundles, would we just detect them in the same file/flag or would that be different?

In this PR, UX will remain the same, just add knowledge about the jsonlines file for attestation bundles.

TomHennen

Thanks, this seems to work well! :)

TomHennen · 2022-07-27T21:05:45Z

cmd/cosign/cli/attach.go

-		Args:    cobra.ExactArgs(1),
+		Use:   "attestation",
+		Short: "Attach attestation to the supplied container image",
+		Example: `  cosign attach attestation --attestation <payload path> <image uri>


suggestion: instead of <payload path>, <attestation file path>

TomHennen · 2022-07-27T21:07:45Z

cmd/cosign/cli/attach.go

+		Short: "Attach attestation to the supplied container image",
+		Example: `  cosign attach attestation --attestation <payload path> <image uri>
+
+  # attach multiple attestations to a container image


suggestion:

"attach attestations from multiple files to a container image"

TomHennen · 2022-07-27T21:12:14Z

cmd/cosign/cli/attach.go

-		Example: "  cosign attach attestation <image uri>",
-		Args:    cobra.ExactArgs(1),
+		Use:   "attestation",
+		Short: "Attach attestation to the supplied container image",


Is there a place to add a note that "attach attestation supports files using the in-toto bundle format" or something like that?

cmd/cosign/cli/attach/attach.go

TomHennen · 2022-07-27T21:18:20Z

cmd/cosign/cli/attach/attach.go

+	"github.com/sigstore/cosign/pkg/types"
+)
+
+func AttestationCmd(ctx context.Context, regOpts options.RegistryOptions, signedPayloads []string, imageRef string) error {


nit, instead of 'signedPayloads', should this be 'filePaths' ?

thank you, updated.

cmd/cosign/cli/attach/attach.go

TomHennen · 2022-07-27T21:20:32Z

cmd/cosign/cli/attach/attach.go

+	return nil
+}
+
+func attachAttestation(remoteOpts []ociremote.Option, signedPayload, imageRef string) error {


nit: instead of 'signedPayload' should this be 'filePath' ?

thank you, updated.

Signed-off-by: Batuhan Apaydın <batuhan.apaydin@trendyol.com>

Dentrax force-pushed the feature/multi-payload-for-attach branch from 7731763 to 6865430 Compare July 20, 2022 21:42

Dentrax mentioned this pull request Jul 20, 2022

attach attestation should support Attestation Bundles #2052

Closed

developer-guy requested a review from dlorenc July 27, 2022 06:28

developer-guy assigned Dentrax and developer-guy Jul 27, 2022

developer-guy requested a review from cpanato July 27, 2022 06:28

feat: attach: allow passing multiple payloads

8ceb65c

Fixed sigstore#2052 Signed-off-by: Furkan <furkan.turkal@trendyol.com> Co-authored-by: Batuhan <batuhan.apaydin@trendyol.com> Signed-off-by: Batuhan Apaydın <batuhan.apaydin@trendyol.com>

developer-guy force-pushed the feature/multi-payload-for-attach branch from 171ef35 to 8ceb65c Compare July 27, 2022 19:47

TomHennen suggested changes Jul 27, 2022

View reviewed changes

developer-guy force-pushed the feature/multi-payload-for-attach branch 2 times, most recently from 550524b to 28f7030 Compare August 1, 2022 08:41

fix: reviews

0fae11e

Signed-off-by: Batuhan Apaydın <batuhan.apaydin@trendyol.com>

developer-guy force-pushed the feature/multi-payload-for-attach branch from 28f7030 to 0fae11e Compare August 1, 2022 08:49

dlorenc approved these changes Aug 1, 2022

View reviewed changes

dlorenc merged commit 938ad43 into sigstore:main Aug 1, 2022

github-actions bot added this to the v1.11.0 milestone Aug 1, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: attach: attestation: allow passing multiple payloads #2085

feat: attach: attestation: allow passing multiple payloads #2085

Dentrax commented Jul 20, 2022 •

edited by developer-guy

Loading

Dentrax commented Jul 20, 2022 •

edited

Loading

TomHennen commented Jul 21, 2022

developer-guy commented Jul 21, 2022

codecov-commenter commented Jul 27, 2022 •

edited

Loading

dlorenc commented Jul 27, 2022

developer-guy commented Jul 27, 2022

TomHennen left a comment

TomHennen Jul 27, 2022

developer-guy Aug 1, 2022

TomHennen Jul 27, 2022

developer-guy Aug 1, 2022

TomHennen Jul 27, 2022

developer-guy Aug 1, 2022

TomHennen Jul 27, 2022

developer-guy Aug 1, 2022

TomHennen Jul 27, 2022

developer-guy Aug 1, 2022

feat: attach: attestation: allow passing multiple payloads #2085

feat: attach: attestation: allow passing multiple payloads #2085

Conversation

Dentrax commented Jul 20, 2022 • edited by developer-guy Loading

Summary

Release Note

Documentation

Dentrax commented Jul 20, 2022 • edited Loading

TomHennen commented Jul 21, 2022

developer-guy commented Jul 21, 2022

codecov-commenter commented Jul 27, 2022 • edited Loading

Codecov Report

dlorenc commented Jul 27, 2022

developer-guy commented Jul 27, 2022

TomHennen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Dentrax commented Jul 20, 2022 •

edited by developer-guy

Loading

Dentrax commented Jul 20, 2022 •

edited

Loading

codecov-commenter commented Jul 27, 2022 •

edited

Loading