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

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

Closed
woodruffw opened this issue Jun 6, 2022 · 13 comments · Fixed by #2278
Closed

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

woodruffw opened this issue Jun 6, 2022 · 13 comments · Fixed by #2278
Labels
enhancement New feature or request

Comments

@woodruffw
Copy link
Member

This is related to our work on sigstore-python (sigstore/sigstore-python#108): --cert-email is slightly misleading, since what it really does is verify the X.509v3 Subject Alternative Name extension, whatever it happens to be (email, URL, or any other identity string).

My proposal: we add --cert-identity (or --cert-san, or something similar) and soft-deprecate --cert-email by allowing it to continue functioning but with a warning until a future major release.

@woodruffw
Copy link
Member Author

@jku pointed out an important implementation hiccup: --cert-email in sigstore-python only checks SANs that are explicitly marked as emails, i.e. that have the RFC822Name type.

I haven't fully confirmed that cosign does the same, but I would expect it to based on this usage of the x509 APIs:

cosign/pkg/cosign/verify.go

Lines 207 to 218 in 75b2bd1

if co.CertEmail != "" {
emailVerified := false
for _, em := range cert.EmailAddresses {
if co.CertEmail == em {
emailVerified = true
break
}
}
if !emailVerified {
return errors.New("expected email not found in certificate")
}
}

In other words, --cert-email doesn't work at all for non-email identities 🙂

If the above proposal sounds good, I propose the following behavior to make it compatible with different identity types:

This has the following advantages:

  • Compatibility: --cert-email does not change whatsoever, at least until a future major version
  • Convenience: --cert-identity without an explicit identity type behaves exactly like --cert-email, which we expect to be the most common case
  • Flexibility: --cert-identity handles all possible identity types via unambiguous prefixes, giving users control when they need it

cc @di for thoughts as well.

@vaikas
Copy link
Contributor

vaikas commented Jun 7, 2022

Also chiming in from the policy-controller side, I did add this for policy-controller that should pull all the things that could be SANs (might have made a mistake of course):
https://github.com/sigstore/cosign/blob/main/pkg/cosign/verify.go#L282

And then it you can pass in what you want to validate, I called it Subject, but the struct is here:
https://github.com/sigstore/cosign/blob/main/pkg/cosign/verify.go#L60

You can specify those in co to this function
https://github.com/sigstore/cosign/blob/main/pkg/cosign/verify.go#L224

I'm really just wanting to make sure things are consistent, not saying this is how it should be done :)

@woodruffw
Copy link
Member Author

Also chiming in from the policy-controller side, I did add this for policy-controller that should pull all the things that could be SANs (might have made a mistake of course):

There probably isn't a high degree of risk here, but it's potentially confusable: the namespaces for URIs, domains, IP addresses, etc. aren't formally discrete, so an attacker who manages to get trick Fulcio into minting a certificate with an "email" SAN that's really a domain might also be able to fool verification.

Ideally that would never happen, but IMO we should preserve the typedness of SANs here to make it a little less problematic if it does.

And then it you can pass in what you want to validate, I called it Subject, but the struct is here:

That looks pretty good!

One thing: the X.509 data model is "one issuer, many potential SANs", so I wonder if issuer should be excluded from the Identity struct (since you ultimately accept a list of them, implying that multiple issues would be valid for the certificate)?

In other words, something closer to this (forgive my pseudo-Go, I don't know it that well):

type Identity struct {
	Subject       SubjectVariant // Email(string), Ip(string), ...
	SubjectRegExp SubjectRegExpVariant 
}

// ...

type CheckOpts struct {
  // ...
  Issuer string  
  Identities []Identity
}

But all of that is yakshaving, so please feel free to ignore 🙂

@woodruffw
Copy link
Member Author

(Another tiny nit is that "subject" is a little bit overloaded between X.509 and OIDC, which is why I suggested "identity" as the cross-cutting thing between X.509 SANs and OIDC identities. But ultimately I don't think it matters, as long as we're consistent!)

@haydentherapper
Copy link
Contributor

This has some up before - #1313

There's some questions around how best to represent a subject. I haven't given a lot of thought to the UX side of this, but from a security perspective, I want to make sure it's hard to specify the wrong matching identity.

@haydentherapper
Copy link
Contributor

haydentherapper commented Jun 7, 2022

cert-san reveals unnecessary details of X.509 in my opinion, where the subject is in the certificate is just an implementation detail.

We need to consider that the subject may not be an email either, it could be a SPIFFE ID, it could be a GitHub/GitLab/CI identity. I'm somewhat leaning towards preferring separate flags for different identity types.

There's also the question, to come back to the previous discussion, of what represents an identity. Is an identity the pairing of subject and issuer (i think yes)?

@woodruffw
Copy link
Member Author

Yeah, I agree re: --cert-san -- if for whatever reason the SAN extension ceases to be sufficient in the future, users shouldn't have to be aware of that.

And yeah, I think this is all predicated on a final decision about what an identity really is in Sigstore 🙂 -- I've been treating it as just the OIDC subject/SAN without regard for the OIDC issuer (given that we trust all IdPs currently), but @znewman01 has raised excellent points about IdPs being unable to prevent bad practices by users (like old, insecure credentials on forgotten accounts). If we'd like to punt on that to move this forward, I think --cert-subject probably best encompasses what we're trying to model here.

(My 0.02c on design: I'd personally prefer a single --cert-subject flag with a discriminant/variant syntax, but that's a very weak preference due to how we use click in sigstore-python).

@woodruffw
Copy link
Member Author

As a motivating example: here's something I'd expect a Python end-user to want to do, to verify that some package was signed by at least one trusted identity:

$ sigstore verify                      \
    --certificate foo.crt              \
    --signature foo.sig                \
    --cert-identity [email protected] \
    --cert-identity [email protected] \
    foo.tar.gz

Or, for example, trusting maintainers or one of their publishing workflows:

$ sigstore verify                      \
    --certificate foo.crt              \
    --signature foo.sig                \
    --cert-identity [email protected] \
    --cert-identity [email protected] \
    --cert-identity github:foo/bar     \
    foo.tar.gz

(Where github: is a psuedo-type that expands to something like uri:https://github.com/foo/bar/...)

That latter one is a little further afield, but maybe something aspirational?

@znewman01
Copy link
Contributor

Whew, long (but interesting) thread! I see 3-ish related problems in the above discussion:

  1. Clarify the --cert-* family of flags.
  2. Provide a syntax for resolving strings to identities (or perhaps identity predicates).
  3. Provide policies about identity verification.

I think @woodruffw's proposal w/r/t (1) is a good idea: we should add a --cert-identity flag. However, it's somewhat blocked on (2), which is blocked on "figure out everything about identity in Sigstore." (We could very carefully define a future-proof syntax for --cert-identity but I worry that it might prematurely limit us in solving (2) for real.)

One way to deal with this in the near term is to focus on (3): force the users to specify explicit verification policies (using, e.g., CUE). This has a few advantages:

  • it's really explicit, and there's no risk of typing a string that checks something other than what I intended (as in Cosigned policies interpreted as regex by default #1871 )
  • we can "lint" policies ("did you really mean to omit the OIDC provider?")
  • it allows predicates over arbitrary certificate attributes (as in Verify certificate against job_workflow_ref string #1313)
  • it allows for unambiguous combinations of subpredicates (AND vs. OR---it's not obvious to me that passing --cert-identity multiple times should mean OR)
  • easy to line up behavior between cosign and policy-controller
  • buys us time to figure out the "sugar" in (1) and (2)

The chief disadvantage to my mind is that the UX is substantially worse for simple cases. The other is that docs in this area are a little light; see Cosign Generic Predicate Specification and Sigstore Docs > Cosign > In-Toto Attestations.

@jku
Copy link
Member

jku commented Jun 8, 2022

--cert-identity github:foo/bar \
(Where github: is a psuedo-type that expands to something like uri:https://github.com/foo/bar/...)

Just to be sure we're on the same page: The "github identity" is the workflow that was executed, not the specific GitHub project. I'm pretty sure I could create project of my own, and use the sigstore-python workflow to build my projects sources. To actually verify the project build in a useful way we are going to need access to more or less custom X509 extensions: in Github case it's the (I believe sigstore reserved) 1.3.6.1.4.1.57264.1.*. Whether exposing these semi-custom extensions values as github-specific options to the CLI tools makes sense is unclear... but they absolutely need to be available if we want to verify GitHub builds.

I just wanted to make the context clear: defining "identity" is not enough make builds usefully verifiable, access to the other extensions is 100% required.

As my 2c, I'm not sure adding a service-specific "github:" as a special value in the CLI interface makes sense: you are still going to have to include 95% of the URI in the argument, why not just use a generic "URI:" prefix -- essentially follow the openssl text output here?

@woodruffw
Copy link
Member Author

Just to be sure we're on the same page: The "github identity" is the workflow that was executed, not the specific GitHub project.

Yep, that was meant to be shorthand for this:

https://github.com/foo/bar/.github/workflows/workflow.yml@refs/something

I'm pretty sure I could create project of my own, and use the sigstore-python workflow to build my projects sources.

How would you do this? I'm not saying it's impossible, just that I wasn't aware that you could -- my understanding is/was that you could only run a repository's workflow by triggering one of the workflow's own events, not an event on your own repository. You can always trigger the "same" workflow in a fork of course, but then the org name will vary.

@jku
Copy link
Member

jku commented Jun 8, 2022

my understanding is/was that you could only run a repository's workflow by triggering one of the workflow's own events, not an event on your own repository

I think you're entirely correct and I was confused. the SAN does seem to identify the project for the reasons you state (but does not promise much about the workflow or source content since the ref they use is a tag and not a hash)

@woodruffw
Copy link
Member Author

I think you're entirely correct and I was confused. the SAN does seem to identify the project for the reasons you state (but does not promise much about the workflow or source content since the ref they use is a tag and not a hash)

Yep...there's another extension (1.3.6.1.4.1.57264.1.3) that contains the SHA hash for the symbolic ref, which we'll also probably want a flag for.

Hashes via a CLI are not the best thing in terms of UX, but in terms of integrity guarantees (for a user who absolutely can't trust symbolic refs) I suppose we can't do much better 🙂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants