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 '--cert-identity' flag to support subject alternate names for ver… #2278

Merged
merged 8 commits into from
Oct 11, 2022

Conversation

kpk47
Copy link
Contributor

@kpk47 kpk47 commented Sep 23, 2022

…ification.

Summary

This PR adds the '--cert-identity' flag for supporting SANs during verification. This change is working towards #2056, but does not fix it completely.

Fixes #1964

Release Note

Added support for certificate Subject Alternate Names during verification flow.

Documentation

N/A, though this flag will need documenting if/when it completely replaces --cert-email.

…ification.

Signed-off-by: kpk47 <kkris@google.com>
@kpk47 kpk47 marked this pull request as ready for review September 23, 2022 21:37
@codecov-commenter
Copy link

codecov-commenter commented Sep 24, 2022

Codecov Report

Merging #2278 (b89ba96) into main (04b96dd) will increase coverage by 0.12%.
The diff coverage is 72.22%.

@@            Coverage Diff             @@
##             main    #2278      +/-   ##
==========================================
+ Coverage   29.02%   29.15%   +0.12%     
==========================================
  Files         131      131              
  Lines        7872     7899      +27     
==========================================
+ Hits         2285     2303      +18     
- Misses       5275     5284       +9     
  Partials      312      312              
Impacted Files Coverage Δ
cmd/cosign/cli/commands.go 0.00% <0.00%> (ø)
cmd/cosign/cli/options/certificate.go 0.00% <0.00%> (ø)
cmd/cosign/cli/verify.go 0.00% <0.00%> (ø)
cmd/cosign/cli/verify/verify.go 19.27% <0.00%> (-0.08%) ⬇️
cmd/cosign/cli/verify/verify_attestation.go 0.00% <0.00%> (ø)
cmd/cosign/cli/verify/verify_blob.go 45.20% <100.00%> (+0.11%) ⬆️
pkg/cosign/verify.go 33.70% <100.00%> (+1.62%) ⬆️

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

@maxking
Copy link

maxking commented Sep 25, 2022

You probably need to run make docgen to regenerate the markdown documentation for the CLI. It seems to be causing CI to fail.

znewman01
znewman01 previously approved these changes Sep 26, 2022
cmd/cosign/cli/options/certificate.go Outdated Show resolved Hide resolved
Signed-off-by: kpk47 <kkris@google.com>
znewman01
znewman01 previously approved these changes Sep 26, 2022
Copy link
Contributor

@znewman01 znewman01 left a comment

Choose a reason for hiding this comment

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

LGTM, thank you so much!

(I don't actually have CODEOWNERs here but someone with CODEOWNERs will swing by soon)

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.

very nice!

@dlorenc
Copy link
Member

dlorenc commented Sep 28, 2022

Looks like you missed a few changes in the e2e_test, otherwise LGTM!

@znewman01
Copy link
Contributor

@kpk47 can you add the DCO to your last commit?

Signed-off-by: kpk47 <kkris@google.com>
znewman01
znewman01 previously approved these changes Oct 3, 2022
haydentherapper
haydentherapper previously approved these changes Oct 3, 2022
…ification.

Signed-off-by: kpk47 <kkris@google.com>
Signed-off-by: kpk47 <kkris@google.com>
Signed-off-by: kpk47 <kkris@google.com>
@kpk47 kpk47 dismissed stale reviews from haydentherapper and znewman01 via 5d7e321 October 3, 2022 20:33
…-identity

Signed-off-by: kpk47 <kkris@google.com>
@haydentherapper
Copy link
Contributor

@kpk47 You'll need to sign the DCO again.

Signed-off-by: kpk47 <kkris@google.com>
znewman01 added a commit to znewman01/community that referenced this pull request Oct 11, 2022
I do a [whole bunch of reviewing for Cosign](https://github.com/sigstore/cosign/pulls?q=is%3Apr++commenter%3Aznewman01) and get CC'd in [pretty frequently](https://github.com/sigstore/cosign/pulls?q=is%3Apr+mentions%3Aznewman01) but wind up needing a second reviewer in every case. Would be nice to avoid that (so contributors [don't get stuck](sigstore/cosign#2278)).

Signed-off-by: Zack Newman <zjn@chainguard.dev>
priyawadhwa pushed a commit to sigstore/community that referenced this pull request Oct 11, 2022
I do a [whole bunch of reviewing for Cosign](https://github.com/sigstore/cosign/pulls?q=is%3Apr++commenter%3Aznewman01) and get CC'd in [pretty frequently](https://github.com/sigstore/cosign/pulls?q=is%3Apr+mentions%3Aznewman01) but wind up needing a second reviewer in every case. Would be nice to avoid that (so contributors [don't get stuck](sigstore/cosign#2278)).

Signed-off-by: Zack Newman <zjn@chainguard.dev>

Signed-off-by: Zack Newman <zjn@chainguard.dev>
@znewman01 znewman01 merged commit d795dcb into sigstore:main Oct 11, 2022
@github-actions github-actions bot added this to the v1.14.0 milestone Oct 11, 2022
@kpk47 kpk47 deleted the verify-identity branch November 2, 2022 19:23
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.

CLI: cosign verify --cert-email should be aliased as --cert-identity (or similar)
6 participants