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

Make sure a cert passed in via --cert matches the bundle cert #2652

Merged
merged 2 commits into from
Jan 25, 2023

Conversation

priyawadhwa
Copy link
Contributor

This is part of #2632, and is one of the smaller action items listed in #2632 (comment).

Signed-off-by: Priya Wadhwa [email protected]

Release Note

Documentation

@codecov-commenter
Copy link

codecov-commenter commented Jan 25, 2023

Codecov Report

Merging #2652 (3c0afc7) into main (35bf1fe) will decrease coverage by 0.01%.
The diff coverage is 40.00%.

@@            Coverage Diff             @@
##             main    #2652      +/-   ##
==========================================
- Coverage   30.01%   30.01%   -0.01%     
==========================================
  Files         146      146              
  Lines        9288     9296       +8     
==========================================
+ Hits         2788     2790       +2     
- Misses       6070     6074       +4     
- Partials      430      432       +2     
Impacted Files Coverage Δ
cmd/cosign/cli/verify/verify_blob.go 48.91% <40.00%> (-0.43%) ⬇️
cmd/cosign/cli/verify/verify_blob_attestation.go 33.48% <40.00%> (-0.16%) ⬇️

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

Signed-off-by: Priya Wadhwa <[email protected]>
Copy link
Contributor

@asraa asraa left a comment

Choose a reason for hiding this comment

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

This doesn't handle the OCI image case, right? Since in that case the bundle is on-disk and only gets read during the internal verification call.

@znewman01
Copy link
Contributor

This doesn't handle the OCI image case, right? Since in that case the bundle is on-disk and only gets read during the internal verification call.

That's a good catch. I think long-run, we're going to want:

  1. An inner verify method that doesn't know anything about the bundle
  2. An outer verifyBundle method that takes in certificate, bundle, etc. and unpacks them into a verify call

Then, all of verify, verify-blob, verify-blob-attestation, etc. would go through verifyBundle; this check would happen in (2).

As it stands, I haven't been able to figure out whether this actually happens in verify (and if so, where). Given that users probably shouldn't be doing this anyway, I'm comfortable with just fixing it here.

@znewman01 znewman01 merged commit 1cf1dff into sigstore:main Jan 25, 2023
@github-actions github-actions bot added this to the v1.14.0 milestone Jan 25, 2023
@asraa
Copy link
Contributor

asraa commented Jan 25, 2023

As it stands, I haven't been able to figure out whether this actually happens in verify (and if so, where). Given that users probably shouldn't be doing this anyway, I'm comfortable with just fixing it here.

I think this was the source of the original issue though, right? From #2632 is an OCI image.

@znewman01
Copy link
Contributor

I think #2633 is more important for that, this was just a side issue and I think it's more obviously a problem for verify-blob

@priyawadhwa priyawadhwa deleted the match-bundle-cert branch January 25, 2023 18:13
uralsemih pushed a commit to uralsemih/cosign that referenced this pull request Jan 29, 2023
…re#2652)

* Make sure a cert passed in via --cert matches the bundle cert

Signed-off-by: Priya Wadhwa <[email protected]>

* Use cert.Equal for comparison

Signed-off-by: Priya Wadhwa <[email protected]>

Signed-off-by: Priya Wadhwa <[email protected]>
dmitris pushed a commit to dmitris/cosign that referenced this pull request Mar 24, 2023
…re#2652)

* Make sure a cert passed in via --cert matches the bundle cert

Signed-off-by: Priya Wadhwa <[email protected]>

* Use cert.Equal for comparison

Signed-off-by: Priya Wadhwa <[email protected]>

Signed-off-by: Priya Wadhwa <[email protected]>
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.

5 participants