Allow for option in cosign attest and attest-blob to upload attestation as supported in Rekor

[aalsabag]

Resolves sigstore/rekor#1928

Summary
This allows for attestation uploads to S3 if --store-attestation is set. This is only enabled for those running private instances of Rekor.

Release Note
A --store-attestation flag is added to attest and attest-blob to upload the attestation to a third party store (i.e s3) if enabled in a private rekor instance

Documentation
NONE

[codecov]

Codecov Report

Attention: 22 lines in your changes are missing coverage. Please review.

Comparison is base (e678426) 40.09% compared to head (43081c4) 40.07%.
Report is 10 commits behind head on main.

Files	Patch %	Lines
cmd/cosign/cli/attest/attest.go	0.00%	8 Missing ⚠️
cmd/cosign/cli/attest/attest_blob.go	33.33%	6 Missing ⚠️
cmd/cosign/cli/options/attest.go	0.00%	3 Missing ⚠️
cmd/cosign/cli/options/attest_blob.go	0.00%	3 Missing ⚠️
cmd/cosign/cli/attest.go	0.00%	1 Missing ⚠️
cmd/cosign/cli/attest_blob.go	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3466      +/-   ##
==========================================
- Coverage   40.09%   40.07%   -0.02%     
==========================================
  Files         155      155              
  Lines        9982    10040      +58     
==========================================
+ Hits         4002     4024      +22     
- Misses       5494     5530      +36     
  Partials      486      486              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[nkreiger]

hey @aalsabag long time no see. Do we think StoreAttestation is too generic? Maybe StoreInTotoAttestation?

Or how about AttestationType, and it defaults to dsse? I feel like the current name would make me think I need to set the flag when running cosign attest, because I want the attestation stored in rekor, which could lead to a lot of people setting the flag unneccessarily when sending to the public good instance. Thoughts?

[bobcallaway]

A --store-attestation boolean infers that the client has complete control over this, when in reality both the Rekor type cosign uploads (intoto may store it vs dsse never will) and the server side setting can effect whether this is successful.

IMO, this PR should really be about setting the Rekor type (intoto vs dsse) vs having a carveout for a specific endpoint (rekor.sigstore.dev)

[aalsabag]

Big dog @nkreiger !

Yeah that's a good point!

To combine both you and @bobcallaway 's comments, I'm thinking of

1. Changing the flag's name from store-attestation to store-intoto-attestation.
2. Adding an additional conditional to check to see that the type is "intoto" effectively checking to see that the flag was set, we are uploading an intoto type, and we are pointing to a private rekor instance.
3. Expanding on the help comment to indicate that the flag "is for those running private instances of rekor with an attestation store backend"

What do y'all think of this?

[haydentherapper]

To @bobcallaway's point, we should avoid referencing storing an attestation in the flag name, because there's no guarantee that an instance of Rekor, public or private, will support uploading the attestations. Even with the proposed documentation for the flag, there would be an expectation from the user that this controls server-side behavior.

I think the flag should be whether to use dsse vs intoto for the Rekor attestation type, and whether or not the log backend stores the attestation is up to the log deployer.

I am in support of restricting it to just private instances though, as this is forward looking to disabling these types in the public instance.

[bobcallaway]

2. Adding an additional conditional to check to see that the type is "intoto" effectively checking to see that the flag was set, we are uploading an intoto type, and we are pointing to a private rekor instance.

I don't think you need to check what instance you are pointing to at all. It does make me wonder if Rekor should set a response header if it does not store the attestation.

[haydentherapper]

It depends if the intoto type will be deprecated in favor of the newer type, or if storage would just be disabled for these types. I'm fine with leaving this check out though as we don't have any other examples of a check for private vs public in the code.

The response header would be a nice feature.

[aalsabag]

I think the flag should be whether to use dsse vs intoto for the Rekor attestation type, and whether or not the log backend stores the attestation is up to the log deployer.

That's fine, but if you guys recall, my original suggestion was to check the type flag which I was told was a bad idea and abandoned in favor of a "new" flag. Are we deprecating this flag entirely? What's its purpose with regards to our attest command?

Anyway, not important at the moment.
Here are some options for a new flag name that doesn't include a reference to storing attestations.

--attestation-type
--upload-type

The type would either be intoto or dsse.

Also, I am with @haydentherapper with regards to keeping the rekor server check for the time being at least until we get some functionality around the response header suggestion, but happy to abandon that too.

[bobcallaway]

I'd include `rekor` in the flag name so that it is clear that the value is specific to the Rekor transaction, e.g. it would be ignored if a user was publishing the attestation to a private container registry and didn't want to upload to Rekor at all. This was my previous concern with overloading the existing type flag in that the meaning would be coupled vs a `--rekor-entry-type` flag which feels clearer to me.

…

On Tue, Jan 9, 2024 at 1:58 AM aalsabag ***@***.***> wrote: I think the flag should be whether to use dsse vs intoto for the Rekor attestation type, and whether or not the log backend stores the attestation is up to the log deployer. That's fine, but if you guys recall, my original suggestion was to check the type flag which I was told was a bad idea and abandoned in favor of a "new" flag. Are we deprecating this flag entirely? What's its purpose with regards to our attest command? Anyway, not important at the moment. Here are some options for a new flag name that doesn't include a reference to storing attestations. --attestation-type --upload-type The type would either be intoto or dsse. Also, I am with @haydentherapper <https://github.com/haydentherapper> with regards to keeping the rekor server check for the time being at least until we get some functionality around the response header suggestion, but happy to abandon that too. — Reply to this email directly, view it on GitHub <#3466 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAVWTJKZYNZOMU2QVDFZUY3YNTTABAVCNFSM6AAAAABBKV7OVSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQOBSGUYDQMRTHA> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[aalsabag]

I've gone ahead and made the update @bobcallaway

[haydentherapper]

PTAL at failing tests

[aalsabag]

PTAL at failing tests

I think I've fixed them all now.