New Check: signed commits

[laurentsimon]

In addition to signed release and tags, we may check for signed commits.

We need a good story around key management/storage. Long-term, it would be nice to suggest more recent algorithms such as ECDSA/EdDSA instead of RSA/DSA and more recent crypto libraries, such as Tink.

[laurentsimon]

Try to integrate with https://sigstore.dev/

[dlorenc]

Right now signed git commits are problematic. The only real method is gpg, and there's no real key-management story so in most cases signed commits add little value IMO.

There's an effort here to improve the native git-commit signing: https://github.com/TrustFrame/git-cryptography-protocol

And sigstore has some plans to improve git signing by "working around" git. It's all a fair ways out though...

[laurentsimon]

related sigstore/cosign#865

[pnacht]

Would the idea be to see if branch protection requires signed commits? Or just that maintainers sign their commits?

I'd be very much in favor of the latter (just maintainers), but significantly against the former (branch protection).

Just checking whether maintainers sign their commits

Checking whether maintainers sign their commits makes a lot of sense, since it gives us stronger guarantees that the code is actually coming from the maintainer. The question just becomes how to identify maintainers, since GitHub doesn't make that information public (API requires an admin token).

Checking that very active (and therefore perhaps trusted) external contributors sign their commits also makes sense, since their contributions may not be as closely vetted if maintainers trust them. But identifying such cases is even harder than identifying maintainers.

This also has the added problem that it'd require traversing the git commit history, which means Scorecard would have to clone the repo (or make a bunch of API calls), not just download a tarball.

Checking whether branch protection requires signed commits

I just think the cost-benefit of this makes little sense.

On the benefit side... What value do you get from receiving a signed commit in an external PR from new contributor @uber_haxxer? All you know is that the PR really came from @uber_haxxer, but... you still don't trust them, so... what's the point?

Meanwhile, on the cost side... ~0% of contributors cryptographically sign their commits*. Therefore, a project enabling "require signed commits" is roughly equivalent to "we don't accept external contributions", unless the maintainers are willing to spend time educating ~100% of new contributors on how to set up signed commits.

I'd absolutely love for maintainers to make that effort to increase adoption of signed commits, but I also don't think Scorecard should punish projects for thinking that's not their job.

---

* Ok, busted, I don't actually have data to back this up. It's just a general feeling I have from personal experience. But here's some anec-data instead: looking at the first page of merged CPython PRs (the first big, complex project I could think of, which I'm wildly assuming attracts more "serious" developers), I found 11 non-bot PRs by unique contributors, listed below along with whether the commits are signed (✅ / 🚫)

• 🚫 gh-113980: Fix resource warnings in test_asyncgen python/cpython#113984
• 🚫 gh-113903: Fix an IDLE configdialog test python/cpython#113973
• ✅ Two small corrections to docs for codeobject.co_lines() python/cpython#113970
• 🚫 [3.11] gh-81489: Use Unicode APIs for mmap tagname on Windows (GH-14133) python/cpython#113962
• 🚫 Update KDE recipe to match the standard use of the h parameter python/cpython#113958
• 🚫 gh-108364: In sqlite3, disable foreign keys before dumping SQL schema python/cpython#113957
• 🚫 GH-113860: Get rid of _PyUOpExecutorObject python/cpython#113954
• 🚫 gh-113937 Fix failures in type cache tests due to re-running python/cpython#113953
• ❓ gh-107901: duplicate blocks with no lineno that have an eval break and multiple predecessors python/cpython#113950 (some signed, some not)
• 🚫 gh-113858: Cut down ccache size python/cpython#113945
• 🚫 gh-91960: Remove Cirrus CI configuration python/cpython#113938

[laurentsimon]

if maintainers are required to sign their commit but branch protection is not enabled, does it mean it allows detection but not prevention? I mean, the release process would have to walk the commit to determine if it was signed.. and by which people.

Main problem is which people really. If scorecard only checks for signed commits, it says nothing: a compromised GitHub user may change the signing key... or if they use Sigstore with GitHub identity provider, account compromise leads to immediate key compromise. With a non-GitHub identity provider, you need to be sure there's 2FA enabled, otherwise it's just an account takeover.

In general, I'm not really convinced that signed commit helps unless identity management is solved and its resists account compromises. At that point, it seems like 2FA is what we should be recommending. Everything else is internal GitHub infra threat?

Caveats: PAT, fine-grained PATs: those escape the 2FA, so commit signing helps to detect them. Is there another way to identify PATs used for commits?

[eli-schwartz]

I think this issue displays a total disconnect between the OSSF vs projects that actually have rules such as "all commits must be signed".

As noted, requiring signed commits for people without a commit bit is nonsensically meaningless. But that is not even remotely the same as not accepting external contributions.

There are six ways to merge a PR, in a matrix that covers:

Merge strategy:

• squash commits
• rebase commits
• merge via a merge commit

How it's done:

• clicking the merge button on GitHub
• by a maintainer locally performing the action and pushing it via the command line

The distinction here is really just about how it's done. It doesn't matter what you try, who you are, which merge strategy you use, or anything else. You CANNOT sign commits by using the GitHub web UI. ¹

Merging via the command line can be done with any strategy but only "merge via merge commit" will show up on the website as a merged PR -- the other two show up as a "closed PR".

Here are some projects which just do the "rebase and sign all external contributions via the command line" approach:

• https://github.com/curl/curl/commits/master
• https://github.com/gentoo/gentoo/commits/master/
• all Gentoo owned repositories as defined by policy document: https://www.gentoo.org/glep/glep-0066.html#id7

Discussion related to asking GitHub for explicit support for closing the gap here, and showing PRs which were manually merged, as merged:

• Explicit "mark as merged" option in pull request UI isaacs/github#2
• https://github.com/orgs/community/discussions/6414
• https://github.com/bagder/github-feedback/blob/main/merges.md

Tooling for automatically applying PRs locally:

• https://github.com/projg2/pram
• https://github.com/leahneukirchen/git-merge-pr
• https://ttboj.wordpress.com/2015/03/16/fancy-git-aliases-and-git-cherryfetch/

Footnotes

1. You can have GitHub sign your commits but then the author of the merge action is GitHub and people who actually look at cryptographic signatures basically hate this. We don't use or trust this since it proves nothing other than that it was pushed by someone that had push access at that precise time. ↩

[lasomethingsomething]

@spencerschrock @afmarcum and others: From our Jan 25 meeting, we said we'd close this one as a "won't do."

[spencerschrock]

As noted, requiring signed commits for people without a commit bit is nonsensically meaningless. But that is not even remotely the same as not accepting external contributions.

I don't think anyone was trying to make this distinction, rather the comment was pointing out that GitHub doesn't provide an API to identify project maintainers.

You CANNOT sign commits by using the GitHub web UI
You can have GitHub sign your commits but then the author of the merge action is GitHub
Merging via the command line can be done with any strategy but only "merge via merge commit" will show up on the website as a merged PR -- the other two show up as a "closed PR".

Thanks for the detailed response. I was familiar with the GitHub signing aspect of the web UI, but not with the interaction around how some projects manually merge results in closed PRs.

[eli-schwartz]

GitHub doesn't provide an API to identify project maintainers.

It sort of does, you can use https://api.github.com/orgs/ORG/members but that only helps if the individuals in question publicly identify as organization members. Doesn't help for collaborators, either.

It would be possible to go to the old standby of "expect projects to list additional information in a text file checked into the repository", I guess...