verify-commits should also check for malicious code in merge commits #8089

Assuming that the machine of a maintainer is compromised, it is possible to have the merge commit modified before sign and push.

As we already have the requirement that "everyone without exception contributes patch proposals using pull requests." [1], I'd propose that we add the requirement that a pull request can only be merged when there is no merge conflict to be solved. Thus, verify-commits could not only check the signature of every merge commit but also fail when the merge commit modifies more code than the branch did that is about to be merged.

I spent a bunch of time re-writing verify-commits to validate that no merge commits at all may be unsigned, and decided the result including the restrictions it puts on git workflow, was too messy. But if we're willing to require certain git workflows, then I cant say I particularly mind that approach, in addition to this suggestion.

Right, it would probably require to overhaul our git workflow.

Though, it is not required to verify every merge commit is signed. I think, it even should be avoided because it is known that some contributors prefer not to sign or don't even have a key.

It should be sufficient to verify that the current HEAD of master is signed and also is a 'clean' merge (no conflicts solved, no additional diff). Afterward, verify recursively that each parent of that commit is either a valid (signed, clean merge) commit OR trusted_root OR a (likely unsigned) commit, which is known to be reviewed.

github-merge.py already refuses to merge if there are any conflicts, so in principle this should already be the case for top-level merges.

I do think it should be possible for non-top-level merges (so those part of pull requests). Merges with conflict resolve exist for a good reason, and I don't think the use of git to accomplish development tasks (as well as convenience for contributors) should be unnecessarily restricted for paranoia reasons.

github-merge.py already refuses to merge if there are any conflicts, so in principle this should already be the case for top-level merges.

Though there is no way for other people to check that? It's a local check and assuming a (un)intentionally compromised machine, it renders itself useless.

I do think it should be possible for non-top-level merges (so those part of pull requests). Merges with conflict resolve exist for a good reason, and I don't think the use of git to accomplish development tasks (as well as convenience for contributors) should be unnecessarily restricted for paranoia reasons.

​ I agree. Keep in mind, that there would be no further restrictions on code contributors. Changing the workflow would only place a burden on reviewers. (E.g. having at least one reviewer sign the commit hash of a pull)