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

Deprecate --certificate-email flag. Make --certificate-identity and -… #2411

Merged
merged 51 commits into from
Dec 21, 2022

Conversation

kpk47
Copy link
Contributor

@kpk47 kpk47 commented Nov 3, 2022

…-certificate-oidc-issuer required for verification.

--certificate-email is now an alias for --certificate-identity

Signed-off-by: kpk47 [email protected]

Summary

Closes #2056

Release Note

Documentation

…-certificate-oidc-issuer required for verification.

--certificate-email is now an alias for --certificate-identity. Also removed some redundant subject/issuer checking.

Signed-off-by: kpk47 <[email protected]>
@codecov-commenter
Copy link

codecov-commenter commented Nov 3, 2022

Codecov Report

Merging #2411 (820bf93) into main (77206c4) will decrease coverage by 0.62%.
The diff coverage is 50.00%.

@@            Coverage Diff             @@
##             main    #2411      +/-   ##
==========================================
- Coverage   30.04%   29.41%   -0.63%     
==========================================
  Files         140      140              
  Lines        8671     8654      -17     
==========================================
- Hits         2605     2546      -59     
- Misses       5687     5745      +58     
+ Partials      379      363      -16     
Impacted Files Coverage Δ
cmd/cosign/cli/dockerfile.go 0.00% <0.00%> (ø)
cmd/cosign/cli/manifest.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 22.49% <85.71%> (+3.72%) ⬆️
cmd/cosign/cli/verify/verify_attestation.go 3.43% <85.71%> (+3.43%) ⬆️
cmd/cosign/cli/verify/verify_blob.go 49.59% <100.00%> (+1.04%) ⬆️
pkg/cosign/verify.go 38.56% <100.00%> (-2.31%) ⬇️
pkg/oci/layout/signatures.go 0.00% <0.00%> (-53.85%) ⬇️
pkg/oci/layout/write.go 0.00% <0.00%> (-37.50%) ⬇️
... and 4 more

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

@kpk47 kpk47 marked this pull request as ready for review November 3, 2022 19:33
Signed-off-by: kpk47 <[email protected]>
Signed-off-by: kpk47 <[email protected]>
Signed-off-by: kpk47 <[email protected]>
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.

Overall LGTM. Needs a rebase before we can merge.

I'd be interested in putting this validation all in the same place. Maybe factor out some of these arguments into a new options.VerifyOpts and have a function to parse them into cosign/verify.CheckOpts. But I'm okay doing that later.

znewman01
znewman01 previously approved these changes Nov 7, 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.

adding approving review so tests run

Signed-off-by: kpk47 <[email protected]>
znewman01
znewman01 previously approved these changes Nov 7, 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.

approving for tests

@znewman01
Copy link
Contributor

test failure looks related!

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

Just need to rerun make docgen, this looks great!

@haydentherapper
Copy link
Contributor

haydentherapper commented Dec 21, 2022

@kpk47 Can you update test/testdata/test_blob_cert.pem with the following:

-----BEGIN CERTIFICATE-----
MIIBdDCCARqgAwIBAgIUZw7gQ6T/IgmiMD1AWB2OTIIVH1owCgYIKoZIzj0EAwIw
ADAeFw0yMjEyMjEwMDIwNThaFw0zMjEyMTgwMDIwNThaMAAwWTATBgcqhkjOPQIB
BggqhkjOPQMBBwNCAAR1Q4hB1jtagrdsVxygtDa/rli00U7n/1I/NSw8yoMRQ+MO
AjRhg3gtcV0tha34L6150qJirQHbfocsao8X6wFmo3IwcDAdBgNVHQ4EFgQUx3Wb
0LwCWoGsl0FUpeQb3M4MukkwHwYDVR0jBBgwFoAUx3Wb0LwCWoGsl0FUpeQb3M4M
ukkwEgYDVR0TAQH/BAgwBgEB/wIBATAaBgNVHREEEzARgQ9mb29AZXhhbXBsZS5j
b20wCgYIKoZIzj0EAwIDSAAwRQIhALXG7XS5TIFLp+jLSxjuRk1Tj5MfE+y9x92Z
YPMbi9GZAiAmfEe0+q5l3PnI6zliOG5kG6EcS80QQgQmPcFvRZWOvw==
-----END CERTIFICATE-----

And update https://github.com/sigstore/cosign/blob/main/test/testdata/README.md, the openssl command, to be :

openssl req -key test/testdata/test_blob_private_key -x509 -days 3650 -out cert.pem -new -nodes -subj "/" -addext "subjectAltName = email:[email protected]"

You can remove all the output below the command too, -subj "/" handles that so you aren't prompted.

You may need to update tests to check for this example SAN. Feel free to rerun the command too if you have a preferred email value.

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.

LGTM, thanks so much for working on this!

@haydentherapper
Copy link
Contributor

@znewman01 once tests finish, do you want to merge?

@znewman01
Copy link
Contributor

E2e tests are failing, once we get those fixed I can merge (will take a quick skim today too)

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.

test failure is unrelated

@znewman01 znewman01 merged commit ed437f1 into sigstore:main Dec 21, 2022
@github-actions github-actions bot added this to the v1.14.0 milestone Dec 21, 2022
@dmitris
Copy link
Contributor

dmitris commented Apr 26, 2023

@kpk47 do you know if the docs on https://docs.sigstore.dev/cosign/keyless/ were updated for this change or not?

@haydentherapper
Copy link
Contributor

Docs are being actively updated for cosign 2.0. Keyless will likely be removed as it’s not duplicated with the verify doc, which has been updated.

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.

Discourage (disallow?) naked cosign verify invocations
6 participants