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

feat: support keyless verification for verify-blob-attestation #2525

Merged
merged 2 commits into from
Jan 5, 2023

Conversation

asraa
Copy link
Contributor

@asraa asraa commented Dec 7, 2022

Signed-off-by: Asra Ali asraa@google.com

  • feat: supports keyless verification for verify-blob-attestation. this includes loading from a bundle & cert verification options
  • cleanup: removes DSSE support from verify-blob

Summary

Release Note

Documentation

@asraa asraa force-pushed the verify-blob-attestation-keyless branch from 3400674 to 3a74a5d Compare December 7, 2022 22:47
@codecov-commenter
Copy link

codecov-commenter commented Dec 7, 2022

Codecov Report

Merging #2525 (914bc31) into main (0081e1a) will decrease coverage by 0.04%.
The diff coverage is 27.16%.

@@            Coverage Diff             @@
##             main    #2525      +/-   ##
==========================================
- Coverage   30.09%   30.05%   -0.05%     
==========================================
  Files         146      146              
  Lines        9113     9245     +132     
==========================================
+ Hits         2743     2779      +36     
- Misses       5961     6040      +79     
- Partials      409      426      +17     
Impacted Files Coverage Δ
cmd/cosign/cli/options/verify.go 0.00% <0.00%> (ø)
cmd/cosign/cli/verify.go 0.00% <0.00%> (ø)
pkg/cosign/verify.go 39.50% <0.00%> (+0.09%) ⬆️
cmd/cosign/cli/verify/verify_blob_attestation.go 33.64% <31.81%> (+6.27%) ⬆️
cmd/cosign/cli/verify/verify_blob.go 49.33% <50.00%> (-0.26%) ⬇️

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

@asraa
Copy link
Contributor Author

asraa commented Dec 7, 2022

IDK what's up with windows regression

@haydentherapper
Copy link
Contributor

    testing.go:1097: TempDir RemoveAll cleanup: remove C:\Users\RUNNER~1\AppData\Local\Temp\TestVerifyBlobCmdWithBundle3198511928\001\attestation.txt: The process cannot access the file because it is being used by another process.

A file not being closed?

@asraa
Copy link
Contributor Author

asraa commented Dec 8, 2022

A file not being closed?

Yeah, but everything is using temp Mkdir, which should cleanup after every test run

@dlorenc
Copy link
Member

dlorenc commented Dec 8, 2022

Yeah, but everything is using temp Mkdir, which should cleanup after every test run

I think we've seen bugs with this on Windows...

@asraa
Copy link
Contributor Author

asraa commented Dec 8, 2022

I think we've seen bugs with this on Windows...

Ah got it, I'll add in manual removals then and see if that'll work ok

@asraa asraa force-pushed the verify-blob-attestation-keyless branch 4 times, most recently from e0bd392 to 9e16353 Compare December 8, 2022 18:00
@asraa
Copy link
Contributor Author

asraa commented Dec 8, 2022

Nope it was my fault, I found a missing file close.

PredicateType string
// TODO: Add policies

SignaturePath string // Path to the signature
}

// Exec runs the verification command
func (c *VerifyBlobAttestationCommand) Exec(ctx context.Context, artifactPath string) error {
Copy link
Contributor

Choose a reason for hiding this comment

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

To reduce duplication, should we refactor verify_blob.go to provide an internal function that does everything but calling cosign.VerifyBlob? Then Exec could call that to do all of the setup, then call cosign.VerifyBlobAttestation and whatever else after. Is there anything else that differs between blob and blob attestation verification?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There's subject claim setting and validating the predicate type. There's not that much I can do here that wouldn't degrade clarity (for simple stuff I think it's better to not obscure behind helpers that work slightly different), except for simplifying some options we can clean up generally across all 4 verification funcs. Stuff like:

  • SetFulcioCheckOpts
  • SetRekorCheckOpts

over full black-box helpers

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm fine to punt this refactor to another time, it doesn't seem like it'll add much now

@haydentherapper haydentherapper mentioned this pull request Dec 12, 2022
3 tasks
@asraa asraa force-pushed the verify-blob-attestation-keyless branch 2 times, most recently from dfebeda to 390461f Compare December 28, 2022 16:22
Signed-off-by: Asra Ali <asraa@google.com>

fix

Signed-off-by: Asra Ali <asraa@google.com>

fix failure

Signed-off-by: Asra Ali <asraa@google.com>

fix windows

Signed-off-by: Asra Ali <asraa@google.com>

update cli options

Signed-off-by: Asra Ali <asraa@google.com>

docgen

Signed-off-by: Asra Ali <asraa@google.com>

add close

Signed-off-by: Asra Ali <asraa@google.com>

fix test

Signed-off-by: Asra Ali <asraa@google.com>
@asraa asraa force-pushed the verify-blob-attestation-keyless branch from 390461f to 3de1b6d Compare January 4, 2023 20:45
haydentherapper
haydentherapper previously approved these changes Jan 4, 2023
Copy link
Contributor

@haydentherapper haydentherapper left a comment

Choose a reason for hiding this comment

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

Looks great! Just a failing e2e test

Signed-off-by: Asra Ali <asraa@google.com>
@asraa asraa merged commit bdb8ea6 into sigstore:main Jan 5, 2023
@github-actions github-actions bot added this to the v1.14.0 milestone Jan 5, 2023
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.

4 participants