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

Discourage (disallow?) naked cosign verify invocations #2056

Closed
znewman01 opened this issue Jul 7, 2022 · 19 comments · Fixed by #2411
Closed

Discourage (disallow?) naked cosign verify invocations #2056

znewman01 opened this issue Jul 7, 2022 · 19 comments · Fixed by #2411
Assignees
Labels
bug Something isn't working

Comments

@znewman01
Copy link
Contributor

Inspired by sigstore/sigstore-python#155

cosign verify (in keyless mode) without any --cert-email provided will succeed when literally anybody has signed an image/blob. This is almost never the right behavior: [email protected] can sign any blob you can.

Instead, we should require some verification policy:

(Keyfull mode is okay, because you're passing the key material or CA in explicitly)

@haydentherapper
Copy link
Contributor

haydentherapper commented Jul 14, 2022

+100000 to this. A couple thoughts:

  • This is a significant breaking change, so we should prepare for issues to be filed. Blog posts/Slack/documentation beforehand is critical.
  • We do have a gap that needs to be resolved first, that a flag like certificate-identity must be added in order to support non-email IDs (SPIFFE and GitHub Actions currently).
  • There is recent support for checking GitHub Actions identities by multiple flags (feat: cert-extensions verify #1626). This could be allowed as an alternative to an identity flag, if all of these flags are set simultaneously, since we should be able to reconstruct the job_workflow_ref which is the SAN of the certificate.
  • I'd prefer no --insecure flag. This is going to be a breaking change regardless, so why continue to support risky behavior?
  • I would also prefer that we don't try to tackle policy as part of this, at least for now. Let's let Cosign handle the "simple" case of one identity, and let a policy engine handle more complex use-cases (Cosign as a library).
  • On this topic, we could consider small UX improvements such as support for multiple identity flags. I don't think we need to tackle this right now though.

Let's get the ball rolling and require certificate-email and certificate-oidc-provider in the next major Cosign release. To unblock this, we'll need to add an identity flag. We can either go with one flag for email-based identities, another flag for URI-based identities, but this may be revealing details of SANs which I'd prefer not to do. The other option is a single identity flag and Cosign handles the look up in the SAN regardless of it is an email or URI (or other?). My preference is a single identity flag, and you have to specify the exact value of the SAN ([email protected], spiffe://domain/id, or https://github.com/{job_workflow_ref}). Thoughts?

I'd also like a blog post or documentation on this topic after we resolve it, as we're closing a significant risk.

We'll also need to sync with the clients for consistent behavior (I know Python is ready to implement this), and sync with policy-controller to make sure it supports non-email based identities.

@znewman01
Copy link
Contributor Author

This is a significant breaking change, so we should prepare for issues to be filed. Blog posts/Slack/documentation beforehand is critical.

[...]

Let's get the ball rolling and require certificate-email and certificate-oidc-provider in the next major Cosign release.

My preference is to immediately stick a deprecation warning in, and leave it for a release or two first. But yeah, I'm on-board overall.

I'd prefer no --insecure flag. This is going to be a breaking change regardless, so why continue to support risky behavior?

I don't feel super strongly about this, but I do think it'd be pretty useful for debugging (not just debugging Cosign, but debugging Sigstore integrations generally).

We do have a gap that needs to be resolved first, that a flag like certificate-identity must be added in order to support non-email IDs (SPIFFE and GitHub Actions currently).

Good call. See also #1964

I would also prefer that we don't try to tackle policy as part of this, at least for now. Let's let Cosign handle the "simple" case of one identity, and let a policy engine handle more complex use-cases (Cosign as a library).

Works for me.

We can either go with one flag for email-based identities, another flag for URI-based identities....The other option is a single identity flag and Cosign handles the look up in the SAN regardless of it is an email or URI (or other?). My preference is a single identity flag, and you have to specify the exact value of the SAN....

Agreed on single identity flag.

I'd also like a blog post or documentation on this topic after we resolve it, as we're closing a significant risk.

I think that's a good idea.


As you point out, it's a moderately disruptive change so I'd like to get community buy-in before implementing it. Perhaps we should have a 1-pager on the proposed changes ready to go next week and share in Slack, then present it at the next community meeting (there is none next week).

@haydentherapper
Copy link
Contributor

My preference is to immediately stick a deprecation warning in, and leave it for a release or two first. But yeah, I'm on-board overall.

Sounds good, let's get the deprecation warning in after the community meeting.

I don't feel super strongly about this, but I do think it'd be pretty useful for debugging (not just debugging Cosign, but debugging Sigstore integrations generally).

I'm hesitant because we've seen testing features like the environment variables for providing roots of trust work their way into documentation. Maybe we start without one, and if we hear complaints from developers, we add one?

As you point out, it's a moderately disruptive change so I'd like to get community buy-in before implementing it. Perhaps we should have a 1-pager on the proposed changes ready to go next week and share in Slack, then present it at the next community meeting (there is none next week).

Sent you the 1-pager, feel free to update. Let's see how many folks are at this week's office hours, it might be enough to start getting some feedback, then we can present again the following week/post on Slack.

@znewman01
Copy link
Contributor Author

SGTM; I'll take a look. I won't make office hours this coming week, but I should be available the week after for the community meeting.

Appreciate you knocking out the 1-pager so quick!

@dlorenc
Copy link
Member

dlorenc commented Jul 16, 2022

What about a command to inspect the bundle? Instead of verify, cosign inspect? I agree this is usage is dangerous and that we should remove it. It's also useful for debugging so I'd love to keep it in some form if we can make it clear that no verification is performed.

@haydentherapper
Copy link
Contributor

That’d also give us the ability to define a machine readable format with information about the bundle and signature, would be very useful for policy engines to consume to make decisions.

@hectorj2f
Copy link
Contributor

+1000
I remember we discussed this topic on a slack thread before. I like the idea of cosign inspect which gives us more freedom to require certain flags and support more complex scenarios i the future (e.g. using policies). We could add warning messages in cosign verify suggesting a switch to inspect for a more secure validation.

@haydentherapper
Copy link
Contributor

haydentherapper commented Jul 16, 2022

I think we should have both - A simple-to-use secure verification with verify, and inspect for those who want to author their own policies. For simple use cases, like one-off checks, policy might not be necessary.

@asraa
Copy link
Contributor

asraa commented Aug 8, 2022

cosign inspect would also allow someone to make policy based on what TUF metadata was used. e.g. can we include the Rekor/Fulcio repository name included? Even if your client accepts a public good and scaffolding instance for signing, you may only want to verify against public good rekor/fulcio.

@haydentherapper
Copy link
Contributor

haydentherapper commented Sep 1, 2022

Design doc: https://docs.google.com/document/d/1o8_bXIygufgiohJGlmBzqF4_BnXCTfgh4ILgJFJxYRs/edit?resourcekey=0-YEar3v67uoT31kj83dCVvA# (sigstore-dev@ for access)

Related issues I'm going to leave open that will be closed once this is completed: #1947, #1964

Related but will be tackled independent from this: #2210

@haydentherapper
Copy link
Contributor

@kpk47 will be taking on this issue. Thanks @kpk47!

@kpk47
Copy link
Contributor

kpk47 commented Sep 1, 2022

ACKing so you can assign me the issue.

@woodruffw
Copy link
Member

Circling back: what's the status here? I see #2278 adds --cert-identity, but doesn't close this issue entirely 🙂

@kpk47
Copy link
Contributor

kpk47 commented Nov 3, 2022

@woodruffw Your timing is impeccable. 😅 I'm cleaning up the tests and then the PR will be ready for review.

@johnfosborneiii
Copy link

Some open source projects only care that the artifact was signed and are not worried about who signed it, maybe add a --any-signature flag for those users that way it's just more explicit and clear about the ramifications?

@znewman01
Copy link
Contributor Author

Some open source projects only care that the artifact was signed and are not worried about who signed it, maybe add a --any-signature flag for those users that way it's just more explicit and clear about the ramifications?

Can you give context? This is basically always the wrong thing to check, and my gut reaction is that we should be correcting that behavior rather than indulging it.

If you truly don't care whether you're getting any security guarantees from a signature, something like a cosign inspect command (#2210) will show you who signed something.

@woodruffw
Copy link
Member

Strong +1 to what @znewman01 said -- absent a trusted identity, an arbitrary sigstore signature is equivalent to an integrity check against an untrusted hash.

If this is something that cosign (and sigstore-python) intentionally support, my recommendation is that we shove it behind a "scary" flag/environment variable, something like:

cosign verify --insecure-trust-any-signature ...

@haydentherapper
Copy link
Contributor

+1, not checking identity is equivalent to being handed a verification key alongside the signature without checking the verification key belongs to an entity you trust. I think it's fine for the API to be flexible, but I would prefer the CLI not be. Please let us know if there's a use-case we're missing.

@di
Copy link
Member

di commented Nov 23, 2022

FWIW, the latest release of sigstore-python now requires --cert-identity and --cert-oidc-issuer at verification time.

haydentherapper added a commit to haydentherapper/website that referenced this issue Dec 13, 2022
Verification of the identity of the signer is a critical part of Sigstore verification. Otherwise, you are only verifying that there is some signature that is valid, instead of checking that the signature was generated by someone you trust.

For more information, sigstore/cosign#2056 contains some discussion around why we want to require these flags, and sigstore/cosign#1947 for why we require both of these flags.

An open question for you is if this specified identity (which came from the certificate) will change between releases. If so, how would you like users to know which identity signed a release?
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants