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 --ca-roots and --ca-intermediates flags to 'cosign verify' #3464

Merged
merged 13 commits into from
Jul 1, 2024

Conversation

dmitris
Copy link
Contributor

@dmitris dmitris commented Jan 2, 2024

Summary

Add new --ca-roots and --ca-intermediates flags to allow pass a certificate bundle PEM file with multiple CA roots and optionally a file with the intermediate certificates. Related to issue #3462. Current commit adds the two flags to verify the CLI options. There is a new functional test file test/e2e_tsa_certbundle.sh for the new flags to cosign verify.

Release Note

  • New features and improvements, including behavioural changes, UI changes and CLI changes
    added --ca-roots and --ca-intermediates flags to take a certificate bundle PEM file with one or multiple CA roots and optionally a PEM file with intermediate certificates. Both flags are exclusive with --certificate-chain.

Documentation

sigstore/docs#291

Copy link

codecov bot commented Jan 2, 2024

Codecov Report

Attention: Patch coverage is 6.89655% with 54 lines in your changes missing coverage. Please review.

Project coverage is 36.44%. Comparing base (2ef6022) to head (f1be17d).
Report is 139 commits behind head on main.

Files Patch % Lines
cmd/cosign/cli/verify/verify.go 0.00% 38 Missing ⚠️
cmd/cosign/cli/options/certificate.go 0.00% 14 Missing ⚠️
cmd/cosign/cli/verify.go 66.66% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3464      +/-   ##
==========================================
- Coverage   40.10%   36.44%   -3.66%     
==========================================
  Files         155      200      +45     
  Lines       10044    12282    +2238     
==========================================
+ Hits         4028     4476     +448     
- Misses       5530     7260    +1730     
- Partials      486      546      +60     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@dmitris dmitris changed the title add --certificate-bundle flag to 'cosign verify' add --ca-roots flag to 'cosign verify' Jan 9, 2024
@haydentherapper
Copy link
Contributor

@dmitris Is this ready for review? Or were you looking for more initial feedback for moving it out from draft?

@dmitris
Copy link
Contributor Author

dmitris commented Jan 23, 2024

@dmitris Is this ready for review? Or were you looking for more initial feedback for moving it out from draft?

I was waiting for the initial feedback - I think I have it now (in #3462), thanks! - I'll try to add the unit tests shortly and mark it as "ready to review".

@dmitris dmitris force-pushed the certbundle branch 3 times, most recently from 8fb3141 to fca88dc Compare January 29, 2024 08:41
@dmitris dmitris marked this pull request as ready for review January 29, 2024 08:42
@dmitris
Copy link
Contributor Author

dmitris commented Jan 29, 2024

@haydentherapper marked as "ready for review" now. I thought about adding unit tests - but I believe the current implementation with the "big" VerifyCommand.Exec() method https://github.com/sigstore/cosign/blob/main/cmd/cosign/cli/verify/verify.go#L85-L316 doesn't make it easy to cover that code with unit tests, could be a good idea to refactor and split that. I added a new functional test test/e2e_tsa_certbundle.sh modelled after and expanding on the test/e2e_tsa_mtls.sh code - it works with the PR code and (unsurprisingly) fails with the trunk version (since it tests the newly added --ca-roots command-line flag). We also are running this version internally in some CI pipelines.

@dmitris dmitris force-pushed the certbundle branch 7 times, most recently from e770d38 to e7375fa Compare February 5, 2024 17:52
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.

Given this is a more sensitive change, cc'ing a few others for thoughts - @woodruffw @kommendorkapten PTAL.

cmd/cosign/cli/verify/verify.go Outdated Show resolved Hide resolved
cmd/cosign/cli/options/certificate.go Outdated Show resolved Hide resolved
cmd/cosign/cli/verify/verify.go Outdated Show resolved Hide resolved
// ValidateAndUnpackCertWithCertPools creates a Verifier from a certificate. Verifies that the certificate
// chains up to the provided root. CheckOpts should contain a pool of CA Roots and optionally the Intermediates.
// Optionally verifies the subject and issuer of the certificate.
func ValidateAndUnpackCertWithCertPools(cert *x509.Certificate, co *CheckOpts) (signature.Verifier, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Given this is only used in one place, can we just call ValidateAndUnpackCert directly? I believe if no roots are provided, verification will fail so we don't need an explicit check, though it would be good to have a unit test to confirm.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

[FYI] I tried to "combine" the two functions but was getting test failures with cosign attach flows - likely I have missed some code paths. Backed up to the state that you reviewed for now to get into the green build zone - will try again "with smaller steps."

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I got rid of the "special" function ValidateAndUnpackCertWithCertPools in 44e0ff5.
Do you think I should remove this check?
https://github.com/sigstore/cosign/pull/3464/files#diff-fc9a26a4caa1d8684a4bdd9102f187f44e1f7fce7c06a19f4185035f13317cc8R296-R298

                               if co.RootCerts == nil {
                                       return errors.New("no CA roots provided to validate certificate")
                               }

I would think it can be good to fail earlier with possibly more explicit error what have gone wrong, to make for easier troubleshooting (if it ever gets triggered). Let me know if you prefer this yanked @haydentherapper. I will also look into adding or expanding the unit tests, as you mentioned above.

cmd/cosign/cli/options/certificate.go Show resolved Hide resolved
cmd/cosign/cli/verify/verify.go Show resolved Hide resolved
cmd/cosign/cli/verify/verify.go Show resolved Hide resolved
@dmitris dmitris force-pushed the certbundle branch 2 times, most recently from 78c7400 to 1c1deda Compare June 20, 2024 16:28
dmitris and others added 11 commits June 26, 2024 10:31
Related to issue sigstore#3462.  Current commit adds the flag
to verify the CLI options.  The new flag doesn't have
any effect yet (will add in follow-up PRs).

Signed-off-by: Dmitry S <[email protected]>
Add --ca-roots command-line flag for 'cosign verify'
to enable verifying cosign signatures using PEM bundles
of CA roots. Whether to also add --ca-intermediates flag
is TBD.  Unit tests will be added in the next commit(s).

Fixes sigstore#3462.

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

@dmitris, thanks for pushing this through. This looks good, and from comparing to the last review, reusing ValidateAndUnpackCert keeps this much cleaner. And the tests look great. I pointed out a few duplicate tests that are in another e2e_test file, can they be removed?

Can you add the duplicated CLI options for verify-blob, verify-attestation, and verify-blob-attestation now?

Comment on lines +80 to +90
cmd.Flags().StringVar(&o.CAIntermediates, "ca-intermediates", "",
"path to a file of intermediate CA certificates in PEM format which will be needed "+
"when building the certificate chains for the signing certificate. "+
"The flag is optional and must be used together with --ca-roots, conflicts with "+
"--certificate-chain.")
_ = cmd.Flags().SetAnnotation("ca-intermediates", cobra.BashCompFilenameExt, []string{"cert"})
cmd.Flags().StringVar(&o.CARoots, "ca-roots", "",
"path to a bundle file of CA certificates in PEM format which will be needed "+
"when building the certificate chains for the signing certificate. Conflicts with --certificate-chain.")
_ = cmd.Flags().SetAnnotation("ca-roots", cobra.BashCompFilenameExt, []string{"cert"})

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry for raising this issue back up, but to summarize how I'm looking at this:

  • We know --trusted-root is the route we eventually want to take. We are slowly adding sigstore-go dependencies in Cosign. Once we add support for trusted-root, we will deprecate all other options for providing trust root information, including the one proposed here. With that said, deprecation will take time.
  • The current trust root spec supports chains rather than pools, as noted in this issue. We need to add support for pools to match this PR.

I'm fine moving forward with this because it unblocks @dmitris's use-case. While it is adding more complexity around how roots are provided, there are already a number of ways that adding one more doesn't add much more complexity. We should concurrently discuss moving sigstore/protobuf-specs#249 forward.

cmd/cosign/cli/verify/verify.go Outdated Show resolved Hide resolved
test/helpers.go Outdated Show resolved Hide resolved
test/helpers.go Outdated Show resolved Hide resolved
test/e2e_test.go Outdated Show resolved Hide resolved
test/e2e_test.go Outdated Show resolved Hide resolved
test/e2e_test.go Outdated Show resolved Hide resolved
@haydentherapper
Copy link
Contributor

Chatted offline with @dmitris who will follow up in a second PR with the updates to the other verify-* commands.

@haydentherapper haydentherapper merged commit 40fc15f into sigstore:main Jul 1, 2024
21 checks passed
@github-actions github-actions bot added this to the v2.3.0 milestone Jul 1, 2024
@dmitris dmitris deleted the certbundle branch July 1, 2024 20:33
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