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

Add verify-blob-attestation command and tests #2337

Merged
merged 1 commit into from
Oct 14, 2022

Conversation

priyawadhwa
Copy link
Contributor

Summary

Release Note

Documentation

@priyawadhwa priyawadhwa force-pushed the verify-attest-blob branch 2 times, most recently from 864be03 to 0873f39 Compare October 13, 2022 22:23
@codecov-commenter
Copy link

codecov-commenter commented Oct 13, 2022

Codecov Report

Merging #2337 (d7bd9d0) into main (712f279) will decrease coverage by 0.19%.
The diff coverage is 18.04%.

@@            Coverage Diff             @@
##             main    #2337      +/-   ##
==========================================
- Coverage   29.99%   29.80%   -0.20%     
==========================================
  Files         133      134       +1     
  Lines        8142     8275     +133     
==========================================
+ Hits         2442     2466      +24     
- Misses       5376     5479     +103     
- Partials      324      330       +6     
Impacted Files Coverage Δ
cmd/cosign/cli/commands.go 0.00% <0.00%> (ø)
cmd/cosign/cli/options/verify.go 0.00% <0.00%> (ø)
cmd/cosign/cli/verify.go 0.00% <0.00%> (ø)
cmd/cosign/cli/verify/verify_blob_attestation.go 25.80% <25.80%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

cpanato
cpanato previously approved these changes Oct 13, 2022
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.

lgtm

Signed-off-by: Priya Wadhwa <priya@chainguard.dev>
if statement.Subject == nil {
return fmt.Errorf("no subject in intoto statement")
}
if len(statement.Subject) != 1 {
Copy link
Contributor

Choose a reason for hiding this comment

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

should this definitely be a requirement? if the attestation signed over say, a jar file and a pom file, the attestation should verify against the jar file, right?

Copy link
Contributor

Choose a reason for hiding this comment

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

it's sad that IntotoSubjectClaimVerifier logic needs refactoring to be reused..

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah I added this in because attest-blob only allows one subject at the moment. I can remove this requirement in a follow-up PR though!

@asraa
Copy link
Contributor

asraa commented Oct 14, 2022

I just realized: this will deprecate verify-blob when used with attestations once experimental support is added!

yay! that had some problems anyway

@priyawadhwa priyawadhwa merged commit 797033c into sigstore:main Oct 14, 2022
@priyawadhwa priyawadhwa deleted the verify-attest-blob branch October 14, 2022 17:37
@github-actions github-actions bot added this to the v1.14.0 milestone Oct 14, 2022
vaikas added a commit to vaikas/cosign that referenced this pull request Oct 14, 2022
Signed-off-by: Ville Aikas <vaikas@chainguard.dev>
Copy link
Contributor

@vaikas vaikas left a comment

Choose a reason for hiding this comment

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

Couple of tiny nits, I'll fix.

@@ -154,3 +154,23 @@ func (o *VerifyDockerfileOptions) AddFlags(cmd *cobra.Command) {
cmd.Flags().BoolVar(&o.BaseImageOnly, "base-image-only", false,
"only verify the base image (the last FROM image in the Dockerfile)")
}

// VerifyBlobOptions is the top level wrapper for the `verify blob` command.
Copy link
Contributor

Choose a reason for hiding this comment

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

nit, this seems like cut&paste?
VerifyBlobAttestationOptions is ... verify-blob-attestation command.


The signature may be specified as a path to a file or a base64 encoded string.
The blob may be specified as a path to a file.`,
Example: ` cosign verify-blob-attestastion (--key <key path>|<key url>|<kms uri>) --signature <sig> [path to BLOB]
Copy link
Contributor

Choose a reason for hiding this comment

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

nit verify-blob-attestation

Example: ` cosign verify-blob-attestastion (--key <key path>|<key url>|<kms uri>) --signature <sig> [path to BLOB]

# Verify a simple blob attestation with a DSSE style signature
cosign verify-blob-attestastion --key cosign.pub (--signature <sig path>|<sig url>)[path to BLOB]
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto.

@vaikas vaikas mentioned this pull request Oct 14, 2022
vaikas added a commit that referenced this pull request Oct 14, 2022
* Nits for #2337

Signed-off-by: Ville Aikas <vaikas@chainguard.dev>

* run docgen.

Signed-off-by: Ville Aikas <vaikas@chainguard.dev>

Signed-off-by: Ville Aikas <vaikas@chainguard.dev>
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.

None yet

6 participants