Require signed commits for all Kubernetes orgs and repositories

[dims]

CVE-2024-3094 highlights the need to make sure we know the folks who are making changes to our codebase better. One of the aspects (among many others!) is to ensure we require commits that get merged to be proven. CLA helps somewhat in this regard, but we need to do better.

Now that GitHub supports SSH key based git signing in addition to GPG keys:
https://docs.github.com/en/authentication/managing-commit-signature-verification/about-commit-signature-verification

If we are able to require SSH or GPG based keys to sign commits, then folks browsing GitHub UI or inspect our git tree can be confident of the source of the commits (somewhat!). So can we please add one more layer to the multi-layer security for our github orgs by requiring this?

thanks,
Dims

[dims]

/sig steering
/sig contributor-experience

[k8s-ci-robot]

@dims: The label(s) sig/steering cannot be applied, because the repository doesn't have them.

In response to this:

/sig steering
/sig contributor-experience

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[dims]

/committee steering

[thockin]

I agree with this. I started using SSH signing a while back - it's super easy and does not, IMO, represent any hardship to contributors.

I suppose we need:

• a database of known contributors' keys
• a bot that:
  • verifies that l commits from ostensibly known contributors have known signatures
  • flags commits from known contributors with unknown signatures (big scary)
  • flags any commits without signatures or with unknown signatures for closer review
• good docs on how to pull and verify the signature DB and configure git to use it

We still need to deal with commits from the world at large, signed or unsigned. I guess we just raise the bar on reviews?

There must be prior art here, but it's far from my expertise.

[jeefy]

If we are able to require SSH or GPG based keys to sign commits

AFAIK this is configurable at the branch level (OTOH I don't know if you can set a policy higher-up)

There must be prior art here, but it's far from my expertise.

I'll look into it this week and see if this is something we can (somewhat easily) suggest-or-adopt broadly

I guess we just raise the bar on reviews?

My meager two cents, this is probably the biggest preventative measure. Also means more cognitive load on maintainers :\

[aojea]

big +1

[BenTheElder]

I suppose we need:

a database of known contributors' keys
a bot that:
verifies that l commits from ostensibly known contributors have known signatures
flags commits from known contributors with unknown signatures (big scary)
flags any commits without signatures or with unknown signatures for closer review
good docs on how to pull and verify the signature DB and configure git to use it

This sounds incredibly unwieldy, and as you noted we need to allow contributions from those that are not existing "known" contributors. Are we going to require everyone who participates to submit a key to a special kubernetes key databsae?

Also worth noting: collaborative PR development / rebasing others's commits becomes ~impossible AFAIK.

One of the aspects (among many others!) is to ensure we require commits that get merged to be proven.

What does this "prove" vs PR review + merge controls?

If we're merging unreviewed code we have a big problem. We have controls in place to prevent this.

All commits should be traced to PRs currently, which then have author accounts. You can click through to the PR from the merge commit metadata.

If we are able to require SSH or GPG based keys to sign commits, then folks browsing GitHub UI or inspect our git tree can be confident of the source of the commits (somewhat!).

They still can't be, because anyone can push commits to a fork and PR it and have it show up "in the repo" thanks to github.
On the other hand if they're inspecting the log from one of our upstream branches, we already can say that these commits came from approved PRs.

-1

[aojea]

I thought this was advocating for this https://docs.github.com/en/authentication/managing-commit-signature-verification/signing-commits , github is the database https://docs.github.com/en/authentication/connecting-to-github-with-ssh/adding-a-new-ssh-key-to-your-github-account

[BenTheElder]

github is the database https://docs.github.com/en/authentication/connecting-to-github-with-ssh/adding-a-new-ssh-key-to-your-github-account

In which case, I can just add a key to your account when I compromise the account, doesn't really gain us much if anything.

[aojea]

In which case, I can just add a key to your account when I compromise the account, doesn't really gain us much if anything.

don't we require 2FA for github accounts?
I don't think the goal is to find the ultimate and definitive solution, but at least reduce the chances of getting compromised with the minimum disruption
If leveraging existing github capabilities can give us a net improvement then it sounds reasonable to accept it, we are talking from random to at least a github authenticated account with 2FA that seems a net improvement to me

[thockin]

I'm about as far from a security expert as can be, but I don't really buy that signing is of no value. Defense in depth, right?

This sounds incredibly unwieldy, and as you noted we need to allow contributions from those that are not existing "known" contributors. Are we going to require everyone who participates to submit a key to a special kubernetes key databsae?

This is something to discuss. A good first step might be that anyone who has /approve access uses signed commits and that we cross-verify. Sort of akin to the idea of key-signing parties.

E.g. I make a commit adding my pubkey(s), and you can verify with me directly (IRL, Zoom, whatever). Henceforth, the project can trust that any commit signed by one of those keys was me (or that my privkey was compromised). Perfect? Is it "better enough" to be worth the effort? This is where I am ill equipped to judge.

Also worth noting: collaborative PR development / rebasing others's commits becomes ~impossible AFAIK.

If I absorb your commit and then rebase, the result would signed by me but still be attributed to you, right? Is that actively bad or just weird?

One of the aspects (among many others!) is to ensure we require commits that get merged to be proven.

What does this "prove" vs PR review + merge controls?

It proves that some commit which is merged into the codebase with my name is actually from me (or was rebased by someone who we trust). Heaven forbid someone gets push access to master, we could at least identify this. Would a non-PR push also trigger other alarms? I am not sure, truthfully.

All commits should be traced to PRs currently, which then have author accounts. You can click through to the PR from the merge commit metadata.

Do we actively verify this? I see that "Kubernetes Prow Robot" signs merge commits with key "B5690EEEBB952194" but I don't know why I should trust that key specifically and not any other? I can trivially fake a commit that looks like a merge-commit from Prow, with a different key. I also note that "Kubernetes Release Robot" does NOT sign commits.

If we are able to require SSH or GPG based keys to sign commits, then folks browsing GitHub UI or inspect our git tree can be confident of the source of the commits (somewhat!).

They still can't be, because anyone can push commits to a fork and PR it and have it show up "in the repo" thanks to github.

I'm not following - such a commit would not be signed by me.

[BenTheElder]

If I absorb your commit and then rebase, the result would signed by me but still be attributed to you, right? Is that actively bad or just weird?

Fair.

It proves that some commit which is merged into the codebase with my name is actually from me (or was rebased by someone who we trust).

Unless we track our own key database, it does not, it proves that they had access to your account to add the key.
This is why we DO require 2FA on accounts (this is enforced org-wide with github policy).

Heaven forbid someone gets push access to master, we could at least identify this. Would a non-PR push also trigger other alarms? I am not sure, truthfully.

Yes, we have alarms for this. We monitor github push events and alert to slack.

We also have branch protection rules enforced for all repos.

https://docs.github.com/en/repositories/configuring-branches-and-merges-in-your-repository/managing-protected-branches/about-protected-branches

Do we actively verify this? I see that "Kubernetes Prow Robot" signs merge commits with key "B5690EEEBB952194" but I don't know why I should trust that key specifically and not any other? I can trivially fake a commit that looks like a merge-commit from Prow, with a different key. I also note that "Kubernetes Release Robot" does NOT sign commits.

They key isn't relevant, we have webhooks tracking the account that does pushes and we alert on non-robot merges.

Robot account compromise is a different topic that wouldn't be meaningfully improved by adding a key to the robot alongside the existing github auth, they'd have the same access domain.

---

This is an attempt at a technical solution to a social problem:

The social problems are:

• Maintainer burnout
• (New) Maintainer trust
• Inadequate review of PRs

Adding signing does not resolve any of these.

EDIT: IMHO We should however be looking at https://github.com/kubernetes-sigs/maintainers and pruning inactive accounts.

[sftim]

Are there any repos where we're prepared to allow unsigned commits? I'm thinking k/website where just getting writers (and localizers) to use Git is a big ask.

If we were, I'd like to teach Prow when to insist on signed commits, and when to allow skipping it. We can also object further on things like: this contributor's PR includes commits attributed to someone else, and we expect that kind of on-behalf-of committing to always be signed.

For k/k? Let's sign everything.

[thockin]

Unless we track our own key database, it does not, it proves that they had access to your account to add the key.
This is why we DO require 2FA on accounts (this is enforced org-wide with github policy).

I mean, yes? I did mean that we track our own database. There's a git commit signed by a trusted committer which adds signatures to our database. Someone who compromises my account and gets past the 2FA and adds a key STILL can't add something to the key database repo because they don't have your privkey or mine or so forth.

Yes, we have alarms for this. We monitor github push events and alert to slack.

That's good to know :)

This is an attempt at a technical solution to a social problem:

We have many problems. Getting backdoored by trusted contributors is a big one, and yes, key signing does not solve that. I'd still value a way to get the pubkeys of k8s contributors.

[BenTheElder]

I mean, yes? I did mean that we track our own database. There's a git commit signed by a trusted committer which adds signatures to our database. Someone who compromises my account and gets past the 2FA and adds a key STILL can't add something to the key database repo because they don't have your privkey or mine or so forth.

This is a Looot of extra work and friction when we have 2FA ... and does nothing for approving commits from a non-member account. "Tims' account is compromised despite enforced 2FA but we track keys" just means they'll use your account to merge a PR from another account ... and we need to keep accepting PRs from other contributors.

I'd still value a way to get the pubkeys of k8s contributors.

https://github.com/<username>.keys

(Though again, this is mutable gated by the 2FA)

[thockin]

$ curl -s https://github.com/k8s-ci-robot.keys | wc -l
0

Anyway, the concern this issue addresses is primarily "convince me the real Ben made this commit". I find it useful, and everything else around it (except for a repo listing our well-trusted contributors) is optional.

[BenTheElder]

Anyway, the concern this issue addresses is primarily "convince me the real Ben made this commit".

Yes but it doesn't prove that I made it, it proves that something with my account creds made it. That distinction is important, we're fooling ourselves if we think these keys will be more tightly controlled than 2FA.

[...] and everything else around it (except for a repo listing our well-trusted contributors) is optional.

What does this accomplish and how is it different from kubernetes/org or OWNERS?

$ curl -s https://github.com/k8s-ci-robot.keys | wc -l

The merge robot doesn't actually make commits. We merge via the github API, which creates the merge commits.

GitHub will also "sign" from the web editor, which is different from the pub keys you upload and use locally (and again, compromised by your account access / 2FA bypass)

[thockin]

The merge robot doesn't actually make commits. We merge via the github API, which creates the merge commits.

Oh, right. Of course. Sorry, that was dumb of me :)

What does this accomplish and how is it different from kubernetes/org or OWNERS?

It emphasizes for me which commits are "less well known". I'm going to stop debating now, because I know my point is not particularly strong, and reasonable people can disagree on the value of it.

[samuelkarp]

https://github.com/<username>.keys

This is just SSH keys. For GPG keys, you'll find them at https://github.com/<username>.gpg.

[danwinship]

This is an attempt at a technical solution to a social problem:

I mean, the underlying problem is "there are bad people in the world, and they will lie to you", and we will never find a social solution to that, so we should absolutely be thinking about how we can make things better with technical solutions.

(But I agree that this technical solution sounds like maybe it doesn't actually improve things over what we already had.)

[mrbobbytables]

Yeah... IMO the experience around signing for a good chunk of the contributor base is too much for too little return (at least...right now). =/

I think we'd be better off being more proactive around pruning inactive owners who could come in and lgtm / approve with no real review. When we pruned the 700ish people from the org with zero GitHub activity for a year+ there was still a fair amount of owners in there >_>

[brandonkal]

When you sign a commit, all you are saying is that it was authored by a device or person that possesses that key. It does not say anything else about the commit. Also by requiring a signature on commits, users will automate it so that all commits made on their machine are signed, decreasing the value of that signature.

A signature does not say "this code is safe" or even that "I authored this code". All it says is "I authored this commit" which is a very important distinction.

1. The signature does not prove the signer approves of the contents of the commit
2. The signature does not prove the contents of the commit were not altered between staging and committing
3. It cannot prove whether an account was compromised or not
4. There are no guarantees that the account owner has securely stored their private key
5. Given the above, a commit signature cannot prove whether a commit was actually made by the person you think made the commit

Code should be just as thoroughly reviewed regardless of that "verified badge" on GitHub. Any trusted user's machine can be compromised today to push (and automatically sign) bad code in a commit. Whether a user was trusted yesterday does not reduce the requirement that the code changes to be inspected just as thoroughly as for an unknown contributor. A verified badge gives the reviewer the false impression that a commit can be reviewed less thoroughly. Requiring commit signing doesn't reduce the maintainers' burden while increasing the contributors' burden for next to zero benefit in security.

[k8s-triage-robot]

The Kubernetes project currently lacks enough contributors to adequately respond to all issues.

This bot triages un-triaged issues according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

• Mark this issue as fresh with /remove-lifecycle stale
• Close this issue with /close
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

[pacoxu]

/remove-lifecycle stale