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

Modernize attest statements #620

Merged
merged 6 commits into from
Jan 22, 2025
Merged

Modernize attest statements #620

merged 6 commits into from
Jan 22, 2025

Conversation

TomHennen
Copy link
Contributor

Summary

attest now generates 'modern' in-toto attestations using the in-toto attestation go bindings and dropped usage of the cosign library for statement generation.

This lets us:

  1. Use recommended gitCommit as the digest type (which is what we were doing anyways)
  2. Use the correct _type value for modern in-toto statements

In the future it will let us add annotations on the subject.

Note that this also changes the user behavior in that it:

  1. Changes the things mentioned above.
  2. Doesn't let users use the convenience attestation types from the command line. They must instead specify the full predicate type.

refs #611

Release Note

  • attest produces in-toto compliant statements

Documentation

No documentation updates needed as the attest command is not referenced in the current documentation. https://docs.sigstore.dev/cosign/signing/gitsign/

attest no longer uses the cosign library to generate the Statement
used in the attestation.

This lets us:

1. Use `gitCommit` as the digest type (which is what we were doing anyways)
2. Use the correct `_type` value for modern in-toto statements

In the future it will let us add annotations on the subject.

Note that this also changes the user behavior in that it:
1. Changes the things mentioned above.
2. Doesn't let users use the convenience attestation types from the command
   line.  They must instead specify the full predicate type.

refs sigstore#611

Signed-off-by: Tom Hennen <[email protected]>
Signed-off-by: Tom Hennen <[email protected]>
@TomHennen
Copy link
Contributor Author

Hmm, these tests worked yesterday, perhaps there's some non-determinism in how the statement gets marshaled. Let me check on that.

The prior setup needed the contents to be exactly equal.
Unfortunately protojson explicitly doesn't maintain
byte-for-byte equality. To address that the test now
deeply examines the 'attestation' (.sig) file produced
parses the payload JSON into a struct and then compares
that.

This provides somewhat easier to understand error diffs
when things go wrong, but most importantly it appears
stable even if field order or spacing change.

Signed-off-by: Tom Hennen <[email protected]>
@TomHennen
Copy link
Contributor Author

Ok, I believe the tests have been fixed. I'm not sure if folks would prefer this had been handled some other way. Please let me know.

@wlynch wlynch self-requested a review January 22, 2025 15:38
Copy link
Member

@wlynch wlynch left a comment

Choose a reason for hiding this comment

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

Thanks!

wlynch
wlynch previously approved these changes Jan 22, 2025
internal/attest/attest.go Outdated Show resolved Hide resolved
@TomHennen TomHennen requested a review from wlynch January 22, 2025 20:52
@@ -46,7 +46,7 @@ type options struct {
func (o *options) AddFlags(cmd *cobra.Command) {
cmd.Flags().StringVar(&o.FlagObjectType, "objtype", FlagObjectTypeCommit, "[commit | tree] - Git object type to attest")
cmd.Flags().StringVarP(&o.FlagPath, "filepath", "f", "", "attestation filepath")
cmd.Flags().StringVar(&o.FlagAttestationType, "type", "", `specify a predicate type (slsaprovenance|link|spdx|spdxjson|cyclonedx|vuln|custom) or an URI (default "custom")`)
Copy link
Member

@wlynch wlynch Jan 22, 2025

Choose a reason for hiding this comment

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

(no action needed)

After responding to #623 (comment) this made me realize there was a good reason to have this, which was to support making it easy to add attestations for arbitrary files that don't fit neatly into an in-toto type, e.g. being able to do something like gitsign attest -f test-results.xml --type custom and have that work auto-magically.

That said, I think there's more benefits to getting this working with the new in-toto libraries at the moment so happy to proceed with this for now, but this will likely be something we'll look to add back in some form (either as this flag or another flag/command).

@wlynch wlynch enabled auto-merge (squash) January 22, 2025 21:26
@wlynch wlynch merged commit 6e07836 into sigstore:main Jan 22, 2025
7 checks passed
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.

2 participants