Add trusted-root create helper command #3876

steiza · 2024-09-11T13:15:40Z

Summary

We want to help cosign users move from providing disparate verification material to a single file that contains the needed verification material.

Using a trusted root file makes it easier for users to rotate key material and specify what time period different keys were valid.

Once you create your trusted root with cosign trusted-root create ... you can then supply the output to verification commands that support it with cosign verify* --trusted-root ...

Here's an example, note that some flags can be specified more than once (for instance, if your certificate chain has rotated, or if you are supporting public good and private instance). You can specify as few or as much content as you'd like. You'll need to create or obtain certificate chains and public key files as needed:

$ go run cmd/cosign/main.go trusted-root create --certificate-chain fulcio-chain-public.pem --certificate-chain fulcio-chain-private.pem --timestamp-certificate-chain timestamp.pem --ctfe-key ctfe.pub --rekor-key rekor.pub

Release Note

Added cosign trusted-root create command to assist users in creating trusted root files to use in cosign verification commands

Documentation

sigstore/docs#327

To help cosign users move from providing disparate verification material to a single file that contains the needed verification material. This makes it easier for users to rotate key material and specify what time period different keys were valid. Signed-off-by: Zach Steindler <steiza@github.com>

codecov · 2024-09-11T13:17:40Z

Codecov Report

Attention: Patch coverage is 31.25000% with 132 lines in your changes missing coverage. Please review.

Project coverage is 36.55%. Comparing base (2ef6022) to head (1bd2b08).
Report is 231 commits behind head on main.

Files with missing lines	Patch %	Lines
cmd/cosign/cli/trustedroot/trustedroot.go	31.25%	76 Missing and 12 partials ⚠️
cmd/cosign/cli/options/trustedroot.go	0.00%	28 Missing ⚠️
cmd/cosign/cli/trustedroot.go	54.28%	15 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3876      +/-   ##
==========================================
- Coverage   40.10%   36.55%   -3.56%     
==========================================
  Files         155      206      +51     
  Lines       10044    13003    +2959     
==========================================
+ Hits         4028     4753     +725     
- Misses       5530     7654    +2124     
- Partials      486      596     +110

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

Signed-off-by: Zach Steindler <steiza@github.com>

To go with sigstore/cosign#3876 Signed-off-by: Zach Steindler <steiza@github.com>

cmurphy

Love this! Generating a trusted root has been a pain for me for the last few weeks.

cmurphy · 2024-09-11T23:21:21Z

cmd/cosign/cli/options/trustedroot.go

+
+	cmd.MarkFlagsMutuallyExclusive("ca-roots", "certificate-chain")
+	cmd.MarkFlagsMutuallyExclusive("ca-intermediates", "certificate-chain")
+


I'm not seeing a way to provide keys for the CT log service, is that intentional?

Sort of - I was trying to mirror the flags of commands like cosign verify-blob ... as much as possible, and I didn't see a flag for specifying the public key of the CT log. If there is one, and I missed it, I'm happy to add it!

It is overridden via an environment variable

cosign/pkg/cosign/env/env.go

Line 57 in 780780b

VariableSigstoreCTLogPublicKeyFile Variable = "SIGSTORE_CT_LOG_PUBLIC_KEY_FILE"

but by default it is fetched from the TUF targets. I don't think there is a flag to override it.

The CTFE key is used for signing, so unfortunately I don't think the flags for verify-blob are a good analog for what needs to be included in the trusted root.

This is a little tricky because of the diversity of private deployments of Sigstore. I added a --ignore-sct so that if the private deployment is using long-lived keys instead of a private Fulcio we don't try to add CT Log keys to their trusted root; otherwise we call cosign.GetCTLogPubs() which will make use of the environment variable or TUF target.

cmurphy · 2024-09-11T23:22:04Z

cmd/cosign/cli/options/trustedroot.go

+	cmd.Flags().StringVar(&o.Out, "out", "",
+		"path to output trusted root")
+
+	cmd.Flags().StringVar(&o.RekorURL, "rekor-url", "",


What is the rationale for using the rekor URL here instead of an on-disk artifact? How do we know we can trust the key being served at this URL?

Another great question, with a similar answer - as much as possible I'm trying to mirror the flags and behavior of the cosign verify* ... commands.

The idea being when we go to deprecate these flags from cosign verify* ... commands, you will generate the trusted root you need by calling cosign trusted-root create ... with the deprecated flags you were passing to cosign verify* ....

I don't think the flags for verify* are an ideal analog for what needs to be included in the trusted root. The --rekor-url flag used for the verify command is used in an online API call to verify the transparency log inclusion, but the trusted keys for the transparency log are fetched separately using the TUF configuration. We couldn't deprecate --rekor-url, or if we do it would be in favor of offline verification.

Fair enough! I replaced --rekor-url with --ignore-tlog, with a similar implementation to --ignore-sct.

cmurphy · 2024-09-11T23:26:02Z

cmd/cosign/cli/options/trustedroot.go

+	cmd.Flags().StringVar(&o.TSACertChainPath, "timestamp-certificate-chain", "",
+		"path to PEM-encoded certificate chain file for the RFC3161 timestamp authority. Must contain the root CA certificate. "+
+			"Optionally may contain intermediate CA certificates")
+}


It's not uber-critical, but with the options here there's no way to provide a uri for either the CA or the TSA. Other tools like trtool allow the user to manually provide that information.

Yeah, I'm leaning towards keeping this tool more minimal and mirroring the flags of existing cosign verify* ... commands, but people are of course welcome to edit the output to add additional context (including setting URIs or more sane valid timestamps!), or use more complete tooling like trtool.

cmd/cosign/cli/trustedroot/trustedroot.go

Signed-off-by: Zach Steindler <steiza@github.com>

With `--ignore-sct` to support if you are using keys instead of Fulcio. Signed-off-by: Zach Steindler <steiza@github.com>

Similar to `--ignore-sct` Signed-off-by: Zach Steindler <steiza@github.com>

cmurphy · 2024-09-20T21:29:54Z

cmd/cosign/cli/options/trustedroot.go

+		"when set, do not include key for verifying certificate transparency "+
+			"log. Set this if you signed with a key instead of using Fulcio.")
+
+	cmd.Flags().BoolVar(&o.IgnoreTlog, "ignore-tlog", false,


I'm still hesitant about the design of mirroring the flags of the verify* commands. This is a brand new command, it might as well have flag names that are intuitive in relation to its purpose. trusted_root.json is used for both signing and verification, so restricting the flags to just what is relevant to verification feels like an oversight. Using an explicit flag like --certificate-chain along with a negative flag like --ignore-tlog seems inconsistent.

It would make more sense to me if this command picked one direction and followed it for all key material: either every key is explicitly specified by a flag that points to an on-disk key (e.g. cosign trusted-root --certificate-chain=fulcio-chain.pem --rekor-key=rekor.pub --ctfe-key=ctfe.pub)
or all keys including Fulcio certs are retrieved via the TUF client (and ignored if TUF can't find them).

It would make more sense to me if this command picked one direction and followed it for all key material: either every key is explicitly specified by a flag that points to an on-disk key (e.g. cosign trusted-root --certificate-chain=fulcio-chain.pem --rekor-key=rekor.pub --ctfe-key=ctfe.pub)

This sounds good to me! I waffled a bit on files on disk vs. TUFv1 support, but I think supporting files on disk will be more accessible (it's easier for a TUFv1 user to download files and supply them via CLI than it is for someone using files on disk to set up a TUFv1 destination!)

Instead of clients querying remote servers Signed-off-by: Zach Steindler <steiza@github.com>

cmurphy

lgtm!

haydentherapper · 2024-10-01T05:53:54Z

cmd/cosign/cli/trustedroot/trustedroot.go

+			return err
+		}
+
+		ctLogs[id] = &root.TransparencyLog{


Do we need to set validity periods as well? Do we need to take in flags for start time, or default to now?

Since we aren't setting ValidityPeriodStart, it will default to "0001-01-01T00:00:00Z".

Philisophically, I was thinking of this utility more as "enough of a trusted root to get you started" instead of a flexible tool to cover all use-cases. This will be a recurring theme in my responses! And of course once you have the skeleton of a trusted root file, the user can continue editing it and adding additional content.

Agreed it shouldn't cover all cases, but if we're going to instruct users to go manually inspect it afterwards and update a set of values, providing flags to do that for them seems reasonable. Since validity period is a significant benefit of the trusted root, I'd prefer adding a flag for users to set the start time.

I added the ability to specify the validity start time for keys

haydentherapper · 2024-10-01T05:57:13Z

cmd/cosign/cli/options/trustedroot.go

+var _ Interface = (*TrustedRootCreateOptions)(nil)
+
+func (o *TrustedRootCreateOptions) AddFlags(cmd *cobra.Command) {
+	cmd.Flags().StringVar(&o.CAIntermediates, "ca-intermediates", "",


How do cert pools work here? This seems incompatible with the trusted root, as you have to specify a chain.

I see below that you're flattening, assuming 1 intermediate per root and assuming these two lists are ordered. I think that's not always going to be accurate - i would assume a client would throw all their certs into a list without thinking about chain building.

I would recommend we drop these until we support pools in the trust root, and require that clients construct the chains themselves.

Initially we were trying to mirror other cosign flags, but we ended up moving away from that, so how about we specify a single root and a single intermediate file (that can have one or more intermediates)?

Again, this is in the spirit of "enough to get you started" and not a do-it-all utility.

I'd lean towards just removing these flags entirely and only have --certificate-chain as an option. if you specify a root and its intermediates separately, you still have to know their order - might as well just use the chain flag at that point.

Only supporting chains sounds good to me! I updated this pull request to reflect that.

haydentherapper · 2024-10-01T05:57:42Z

cmd/cosign/cli/options/trustedroot.go

+			"when building the certificate chains for the signing certificate. Conflicts with --certificate-chain.")
+	_ = cmd.Flags().SetAnnotation("ca-roots", cobra.BashCompFilenameExt, []string{"cert"})
+
+	cmd.Flags().StringVar(&o.CertChain, "certificate-chain", "",


Should all of these options be plural, so that we can provide multiple chains, multiple logs, and multiple TSAs?

At this point, you can probably anticipate my response : D but I think we should stick to the bare minimum required to get folks off the ground, and let them iterate on top of the basic trusted root we give them.

I feel less strongly on this one, but I think pluralizing is a small enough change that if we did that now, we'd cover future use cases (e.g. trusting both an internal deployment and public deployment)

This did end up being a pretty minor change! You can now use the flags multiple times to specify additional verification materials.

steiza

If the bare-bones approach I describe makes sense, let me know and I can update things to match!

steiza · 2024-10-01T20:48:04Z

cmd/cosign/cli/trustedroot/trustedroot.go

+			return err
+		}
+
+		ctLogs[id] = &root.TransparencyLog{


Since we aren't setting ValidityPeriodStart, it will default to "0001-01-01T00:00:00Z".

Philisophically, I was thinking of this utility more as "enough of a trusted root to get you started" instead of a flexible tool to cover all use-cases. This will be a recurring theme in my responses! And of course once you have the skeleton of a trusted root file, the user can continue editing it and adding additional content.

steiza · 2024-10-01T20:50:03Z

cmd/cosign/cli/options/trustedroot.go

+var _ Interface = (*TrustedRootCreateOptions)(nil)
+
+func (o *TrustedRootCreateOptions) AddFlags(cmd *cobra.Command) {
+	cmd.Flags().StringVar(&o.CAIntermediates, "ca-intermediates", "",


Initially we were trying to mirror other cosign flags, but we ended up moving away from that, so how about we specify a single root and a single intermediate file (that can have one or more intermediates)?

Again, this is in the spirit of "enough to get you started" and not a do-it-all utility.

steiza · 2024-10-01T20:51:13Z

cmd/cosign/cli/options/trustedroot.go

+			"when building the certificate chains for the signing certificate. Conflicts with --certificate-chain.")
+	_ = cmd.Flags().SetAnnotation("ca-roots", cobra.BashCompFilenameExt, []string{"cert"})
+
+	cmd.Flags().StringVar(&o.CertChain, "certificate-chain", "",


At this point, you can probably anticipate my response : D but I think we should stick to the bare minimum required to get folks off the ground, and let them iterate on top of the basic trusted root we give them.

Also add ability to specify validity start time for keys Signed-off-by: Zach Steindler <steiza@github.com>

To go with sigstore/cosign#3876 Signed-off-by: Zach Steindler <steiza@github.com>

dmitris · 2024-10-17T18:56:46Z

As discussed on the Sigstore Slack (#private-sigstore-users channel) - found a case causing a panic on nil pointer (the PR linked below has the details), raised a PR with a trivial fix (nil pointer check on block in the for loop condition): https://github.com/steiza/cosign/pull/2/files#diff-abb0ac148c9642335c919aaffc4700982c9d799bc98d528c4d7d921f2470f2b5R165

for block, contents := pem.Decode(contents); block != nil; block, contents = pem.Decode(contents) {

it would be good to add a unit test covering the case of the nil block panic, just in case. I can do it and send you another "PR-to-PR" - let me know if you'd like me to.

Update tests, also fix documentation for flags that were removed. Co-authored-by: Dmitry S <dsavints@gmail.com> Signed-off-by: Zach Steindler <steiza@github.com>

Signed-off-by: Zach Steindler <steiza@github.com>

steiza · 2024-10-17T20:09:06Z

Thank you @dmitris! I added your fix, along with tests, and added you as a co-author of the commit.

dmitris · 2024-10-17T21:09:32Z

cmd/cosign/cli/options/trustedroot.go

@@ -36,7 +36,7 @@ func (o *TrustedRootCreateOptions) AddFlags(cmd *cobra.Command) {
 		"path to a list of CA certificates in PEM format which will be needed "+
 			"when building the certificate chain for the signing certificate. "+
 			"Must start with the parent intermediate CA certificate of the "+
-			"signing certificate and end with the root certificate. Conflicts with --ca-roots and --ca-intermediates.")


@why do we remove the notice about conflicting flags? When I run cosign verify built from this branch with both --ca-roots file.pem and --certificate-chain file.crt, I'm getting the following error:

Error: if any flags in the group [ca-roots certificate-chain] are set none of the others can be; [ca-roots certificate-chain] were all set main.go:74: error during command execution: if any flags in the group [ca-roots certificate-chain] are set none of the others can be; [ca-roots certificate-chain] were all set

so the "mutual exclusion" of these flags is still there - why then the change of the flags documentation, here and also in doc/cosign_trusted-root_create.md?

Yes, so cosign verify supports both --certificate-chain and --ca-roots, but later we decided that cosign trusted-root create would only support --certificate-chain. There isn't a --ca-roots option for cosign trusted-root create anymore.

would --certificate-chain support PEM files with multiple chains, or only a single one? If you have a PEM bundle with a dozen "top-level" self-signed CA root certificates, would you be able to use cosign trusted-root create to create a trusted root JSON file or not? Would one had to "manually" split that bundle into multiple PEM files, one per certificate (or certificate chain)? Can you use --certificate-chain multiple times?

As mentioned on Slack , if you currently try to pass a PEM bundle with multiple CA root certificates as --certificate-chain, the generated JSON file is incorrect, or at least not what one would expect: it puts all of them in the same chain, with one certificate being the root, and the rest - Intermediates, even though they are all self-signed roots.
The same issue is with parsing a timestamp certificate bundle with multiple certificates.

If these are use cases that we do not intend to support, let's explicitly state it in the documentation.

cmd/cosign/cli/trustedroot/trustedroot.go

Signed-off-by: Zach Steindler <steiza@github.com>

dmitris

👍

haydentherapper

Great work @steiza!

To go with sigstore/cosign#3876 Signed-off-by: Zach Steindler <steiza@github.com>

steiza force-pushed the trusted-root-helper branch from 93f7134 to 2552371 Compare September 11, 2024 13:17

steiza added 2 commits September 11, 2024 09:45

Linter fixes and docgen

ea1c77f

Signed-off-by: Zach Steindler <steiza@github.com>

Fix Windows unit test

a53ec25

Signed-off-by: Zach Steindler <steiza@github.com>

haydentherapper requested review from cmurphy and haydentherapper September 11, 2024 16:32

steiza added a commit to steiza/sigstore-docs that referenced this pull request Sep 11, 2024

Add trusted root docs

363ab2f

To go with sigstore/cosign#3876 Signed-off-by: Zach Steindler <steiza@github.com>

steiza mentioned this pull request Sep 11, 2024

Add docs on using cosign trusted-root create sigstore/docs#327

Merged

cmurphy reviewed Sep 11, 2024

View reviewed changes

steiza added 3 commits September 12, 2024 11:42

Output via stdout instead of stderr

e0041bb

Signed-off-by: Zach Steindler <steiza@github.com>

Add ctlogs to cosign trusted-root create

06284e2

With `--ignore-sct` to support if you are using keys instead of Fulcio. Signed-off-by: Zach Steindler <steiza@github.com>

Replace --rekor-url with --ignore-tlog

b3262d7

Similar to `--ignore-sct` Signed-off-by: Zach Steindler <steiza@github.com>

cmurphy requested a review from kommendorkapten September 12, 2024 20:55

steiza mentioned this pull request Sep 18, 2024

Fix bug in attest-blob when using a timestamp authority with new bundles #3877

Merged

kommendorkapten approved these changes Sep 20, 2024

View reviewed changes

cmurphy reviewed Sep 20, 2024

View reviewed changes

Just use paths to files on disk

cab9148

Instead of clients querying remote servers Signed-off-by: Zach Steindler <steiza@github.com>

cmurphy approved these changes Sep 30, 2024

View reviewed changes

haydentherapper reviewed Oct 1, 2024

View reviewed changes

steiza commented Oct 1, 2024

View reviewed changes

Add the ability to supply multiple verification material

f705836

Also add ability to specify validity start time for keys Signed-off-by: Zach Steindler <steiza@github.com>

steiza added a commit to steiza/sigstore-docs that referenced this pull request Oct 8, 2024

Add trusted root docs

14ad872

To go with sigstore/cosign#3876 Signed-off-by: Zach Steindler <steiza@github.com>

cmurphy approved these changes Oct 11, 2024

View reviewed changes

steiza and others added 2 commits October 17, 2024 16:00

Don't panic if there's unexpected content in PEM file

b8d58d7

Update tests, also fix documentation for flags that were removed. Co-authored-by: Dmitry S <dsavints@gmail.com> Signed-off-by: Zach Steindler <steiza@github.com>

remove trailing newline

1e7a436

Signed-off-by: Zach Steindler <steiza@github.com>

dmitris reviewed Oct 17, 2024

View reviewed changes

dmitris suggested changes Oct 17, 2024

View reviewed changes

cmd/cosign/cli/trustedroot/trustedroot.go Outdated Show resolved Hide resolved

Simplify imports

1bd2b08

Signed-off-by: Zach Steindler <steiza@github.com>

dmitris approved these changes Oct 18, 2024

View reviewed changes

haydentherapper approved these changes Oct 29, 2024

View reviewed changes

haydentherapper merged commit 9b17791 into sigstore:main Oct 29, 2024
23 checks passed

steiza mentioned this pull request Oct 31, 2024

Upgrade to TUF v2 client #3844

Draft

2 tasks

haydentherapper pushed a commit to sigstore/docs that referenced this pull request Nov 7, 2024

Add trusted root docs (#327)

93024b3

To go with sigstore/cosign#3876 Signed-off-by: Zach Steindler <steiza@github.com>


		cmd.MarkFlagsMutuallyExclusive("ca-roots", "certificate-chain")
		cmd.MarkFlagsMutuallyExclusive("ca-intermediates", "certificate-chain")

Add trusted-root create helper command #3876

Add trusted-root create helper command #3876

Conversation

steiza commented Sep 11, 2024 • edited Loading

Summary

Release Note

Documentation

codecov bot commented Sep 11, 2024 • edited Loading

Codecov Report

cmurphy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

steiza Sep 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cmurphy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

steiza left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmitris commented Oct 17, 2024 • edited Loading

steiza commented Oct 17, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmitris Oct 23, 2024 • edited Loading

Choose a reason for hiding this comment

dmitris left a comment

Choose a reason for hiding this comment

haydentherapper left a comment

Choose a reason for hiding this comment

steiza commented Sep 11, 2024 •

edited

Loading

codecov bot commented Sep 11, 2024 •

edited

Loading

steiza Sep 12, 2024 •

edited

Loading

dmitris commented Oct 17, 2024 •

edited

Loading

dmitris Oct 23, 2024 •

edited

Loading