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

Verify: require cert to match committer? #104

Closed
wlynch opened this issue Aug 5, 2022 · 7 comments · Fixed by #246
Closed

Verify: require cert to match committer? #104

wlynch opened this issue Aug 5, 2022 · 7 comments · Fixed by #246
Labels
enhancement New feature or request

Comments

@wlynch
Copy link
Member

wlynch commented Aug 5, 2022

This could be a disruptive change to users, so looking for feedback before we commit to this - should we require by default that the cert contained in the git commit match the email of the git committer?

Right now we just check for a valid signature, and users would need to add additional checks to verify the identity matches.

This would be in line with Sigstore clients should require a provided identity by @haydentherapper and @znewman01, since we don't have an easy way to pass additional params via git verify-commit or git log. This also means we don't have a good way to specify the OIDC provider or Sigstore instance, so we may need additional out of band config (i.e. envvar / #99) for this to be complete.

#65 may be a hard requirement here, since otherwise we won't be able to generate privacy preserving certs. We could also provide an option to disable this behavior (though prefer not to do this if possible).

Thoughts? cc @imjasonh @nsmith5

@wlynch wlynch added the question Further information is requested label Aug 5, 2022
@znewman01
Copy link

Yeah, I think the default of "check that someone signed but not who" is pretty dangerous in general and I strongly support being able configure a "verification policy" (with some sensible default; I think the proposed policy is a good one). Options include:

  • must be signed by the Git commit email (need to think about how to handle merge commits)
  • "unsafe": the behavior today
  • allowlist of identities: only trust commits signed by specific Fulcio identities

@dlorenc
Copy link
Member

dlorenc commented Aug 5, 2022

I like this in general. Another check we could add is that the timestamp in the commit is during the validity window of the cert, to prevent signing "backdated" commits.

@wlynch
Copy link
Member Author

wlynch commented Sep 2, 2022

Some more thinking about this - using committer emails to verify against for human users should be relatively easy, but we still need to figure out:

  1. How to find what IdP to trust for the email

  2. What email to use for commits made by automation

    For GitHub Actions: https://github.com/orgs/community/discussions/26560 suggests we can use 41898282+github-actions[bot]@users.noreply.github.com to link commits to the GitHub Actions user, but this doesn't help us ID the workflow claims to verify, and also requires users to be setting this when making commits, which probably isn't reasonable to assume is being done broadly.

I'm thinking we'll probably need to make a new gitsign verify subcommand to match cosign, though we might be able to populate the committer email by default, which users could then override for verifying automated commits.

@wlynch wlynch added enhancement New feature or request and removed question Further information is requested labels Sep 2, 2022
@znewman01
Copy link

  1. How to find what IdP to trust for the email

Oof, this is really tricky and important to get right. There's no canonical IdP for a given email (see also sigstore/cosign#1947).

<warning: I'm going to repeat myself a bunch below 😄 >

That said: I think we can push this problem to wherever we're solving the problem of "whose commits do I trust, in general?" If you e.g. configure a policy that says "for repo X, I trust *@example.com and support@microsoft.com we would just pick the IdPs there. Until we've got a story there, we're dangerously close to checking that there is some signature, but not whose signature, which is an antipattern.

Not sure if this is technically feasible with the hooks we have into Git, but I think putting the the issuer in the commit would be no worse security-wise than where we are today.

@wlynch
Copy link
Member Author

wlynch commented Sep 20, 2022

Did something thinking + experimentation around this - here's a rough proposal:

For gitsign verify, we can provide customized flags that match the experience of cosign verify (e.g. --certificate-email, etc.). For git verify-commit this is a bit more complex, since we can't pass in additional flags and we're limited by the commit data (which is basically user emails).

User Emails

This is the simplest case, since we should generally have a valid email associated to the commit that should match the cert.

For git verify-commit, we can introduce 2 new gitsign options:

  1. gitsign.verifyPolicy that can take the following values (bikeshedding welcome):

    Value Description
    emailOnly (default) Only validate email matches cert. If provided, gitsign will output a warning that the signature cannot be fully verified unless the identity provider is also checked, but will still pass. Users will be recommended to use gitsign verify instead.
    strict Require strict identity provider checking. This requires that the cert identity must be issued by a provider in gitsign.identityProviders and will prompt users to use gitsign verify on failure.
  2. gitsign.identityProviders - a comma separated list of allowed identity provider mappings, specified by domain:idp i.e. gmail.com:https://accounts.google.com

This allows us to keep compatibility with existing git workflows while still giving users control to restrict down usage. It's probably unreasonable to require users to pre-configure the list of identity providers per repo, since this may vary per repo or even per user.

Robot emails

Automated workflows (Kubernetes, GitHub Actions, etc.) present more of a challenge, because we don't have a clear email to use for each service account user.

For these cases, we propose allowing tools to set specialized Git committer user names that gitsign will know how to interpret. These should match the identity formats we are aiming to support for other tools (e.g. cosign). Gitsign will need to parse the identifiers to extract them and compare against the claims included in the cert. We can reuse the gitsign.identityProviders config to map allowed identity providers.

e.g. for GitHub Actions, the way to attribute commits to actions is to use the special 41898282+github-actions[bot]@users.noreply.github.com email where 41898282 is the GitHub User ID of GitHub Actions: https://api.github.com/user/41898282

With this, we could have gitsign clients populate a special username + email - i.e. github://sigstore/gitsign:refs/heads/main:workflow.yml <41898282+github-actions[bot]@users.noreply.github.com>, where github://sigstore/gitsign:refs/heads/main:workflow.yml represents the Service Account identifier (actual spec TBD - follow sigstore/fulcio#716 for the real spec)

NOTE: technically user names in emails should be max 64 characters, but since we're using this primarily for signature validation and service account identities will likely have noreply emails associated with them anyway, we are ignoring this restriction w.r.t. git and gitsign verification. For email based git workflows, the user name can be truncated if necessary - we're considering the git repo itself to be the source of truth for validation.

Alternatives

We've observed that GitHub is actually quite a bit permissive in what emails it lets you associate (asdf is just arbitrary text):

Email GitHub Shows Actions User
41898282+github-actions[bot]@users.noreply.github.com
41898282+github-actions[asdf]@users.noreply.github.com
41898282+asdf@users.noreply.github.com
41898282+asdf[bot]@users.noreply.github.com
41898282+github-actions[bot]+asdf@users.noreply.github.com
41898282+asdf+github-actions[bot]@users.noreply.github.com

We could use this to include identity information in the email itself, but since we don't control the domain this feels much riskier to rely on for stability without GitHub's blessing.

@znewman01
Copy link

emailOnly (default)

Only validate email matches cert.

In this case, I would strongly recommend hard-coding a default identityProviders configuration with a small list of trusted providers like *:https://accounts.google.com,....

I support teaching users (including gitsign itself, as well as gitsign users) that "Fulcio issues a cert" means only "Fulcio saw a very specific OIDC token," not "the recipient of the cert controls this email/identity." To that end, I am in favor of adding a "blank check" OIDC provider that will give you free certs to any username/email you'd like. My opinion here is very extreme, but the broader point isn't (see sigstore/fulcio#639).

If provided, gitsign will output a warning that the signature cannot be fully verified unless the identity provider is also checked, but will still pass. Users will be recommended to use gitsign verify instead

Users very quickly learn to ignore security warnings 1, 2, 3, 4.

(I'm okay with a warning for a deprecation period.)

gitsign.identityProviders - a comma separated list of allowed identity provider mappings, specified by domain:idp i.e. gmail.com:https://accounts.google.com

My immediate reservation is that "trusted IDP" makes more sense on a per-identity basis than a per-repo or even per-domain basis. If I have a @fastmail.com, I may have a Google account associated with that, or I may have a GitHub account associated with that. This immediately rules out things like "multi-vantage-point" signing: requiring that I have a signature from "zack@fastmail.com, according to Google IdP" and "zack@fastmail.com, according to GitHub IdP".

I think it does cover the 80% case, though.


w/r/t default policy: the whole situation feels a little no-win. That might indicate that the long-run state would be: (1) for ideal security, you configure some policy (in the repo/forge, or, if necessary, in your global/user/repo gitconfig) about trusted identities, and (2) otherwise, you're not using the power of gitsign, and the verification it gives you is a little weak.


These should match the identity formats we are aiming to support for other tools (e.g. cosign)

That issue doesn't seem to be the exact relevant one: it's just saying that subjects in X.509 certs have "types," and any given instance of Fulcio (1) should:

  1. Only ever issue certs with appropriately-typed subjects for a specific provider (you'd express this when you add a provider via configuration: Google does emails only).
  2. Enforce that "email"-typed subjects actually look like emails, URIs look like URIs, etc.

So for parity, you'd just need to know know type of the subject. This is embedded in the X.509 cert:

$ step certificate inspect /tmp/cert.pem | grep -A1 "X509v3 Subject Alternative Name"
            X509v3 Subject Alternative Name: critical
                email:zjn@chainguard.dev

But it really shouldn't matter—Fulcio will only ever issue certs with appropriate types.

I think what you're looking for is a canonical string representation of an (identity provider, username/email) tuple in the Sigstore world, which is a great idea and something I've wanted before (I don't know if it's tracked anywhere). You may want the ability to query arbitrary claims in the cert as well. If that becomes urgent, I can help spec something out.

With this, we could have gitsign clients populate a special username + email[...]

Yes, once we have such a way to represent identities as strings, I think this is a great way to encode them.

We could use this to include identity information in the email itself, but since we don't control the domain this feels much riskier to rely on for stability without GitHub's blessing.

I agree that username is possibly more natural. But the "is this stable in GitHub" question is eminently resolvable, since we do have interested GitHub folks in the Sigstore community.

@operatorequals
Copy link

Inspired by this Issue, I created a Github Action that tries to catch all possible scenarios of signed (and unsigned) commits
that violate security.

In the README's scenarios section there are some cases that this Github Action would fail to verify a signed commit, given some security assumption.

The Github Action
https://github.com/operatorequals/gitsign-action

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.

4 participants