Remove laanwj from trusted-keys

[laanwj]

allow-revsig-commits list generated using:

git log --format="%H %ce" --merges 577bd51a4b8de066466a445192c1c653872657e2..master | grep laanwj | cut -c -40 >> allow-revsig-commits

[DrahtBot]

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	Sjors, achow101, fanquake

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #27058 (contrib: Improve verify-commits.py to work with maintainers leaving by achow101)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

[ajtowns]

allow-revsig-commits list generated using:

git log --format="%H %ce" --merges 577bd51a4b8de066466a445192c1c653872657e2..master | grep laanwj | cut -c -40 >> allow-revsig-commits

Seems to match with:

git log --format="%H %GK" --merges $(cat contrib/verify-commits/trusted-git-root)..master | grep -E " 1E4AED62986CD25D" | cut -c -40

if you prefer querying by key id, rather than committer email. Bit slower though (20s vs 0.2s). The key id matches the signing subkey:

$ gpg --list-key --with-colons 71A3B16735405025D447E8F274810B012346C9A6 |  cut -d: -f1,5,12 | grep 'sub:.*:s'
sub:1E4AED62986CD25D:s

[achow101]

I don't think adding your merged commits to allow-revsig-commits actually allows them. Once verify-commits gets to those commits, since they are signed with a key no longer in trusted-keys, it will fail. Currently the only solution is to update trusted-git-root, although I'm working on a longer term solution that lets us set a "valid until" for the trusted keys.

[Sjors]

tACK aafa5e9 😢

---

The behavior of verify-commits is quite confusing, because it is checking out different commits along the way. allow-revsig-commits is loaded in memory at the start so it stays the same throughout. But trusted-keys is loaded via the gpg.sh, and so it does change.

That makes sense because the list of trusted keys changes through time. But it means the newly added hash only have an effect if you call verify-commits with one of those hashes. In that case we check that commit against the current trusted-keys. If on the other hand you start traversing from HEAD~1, it uses the old trusted-keys and won't error.

You can see this difference in behavior by amending this commit to drop one of the hashes. The verify-commits script will fail if you pass it that hash, and succeed if you don't.

It would probably help if it cloned the repo in tmp rather than operate on itself.

Still I think we can merge with these hashes added and improve the script later.

I've checked the commit list in aafa5e9 against the list generated by @ajtowns's incantation. Also checked that this commit was signed by @laanwj.

Note to other testers: the verify-commits.py script can be a bit cryptic when a PGP key has expired. Use git verify-commit on the commit it trips over to see which key is expired and then refresh it (or just refresh all of them).

[achow101]

The behavior of verify-commits is quite confusing, because it is checking out different commits along the way. allow-revsig-commits is loaded in memory at the start so it stays the same throughout. But trusted-keys is loaded via the gpg.sh, and so it does change.

It should only be checking out commits in order to do the clean merge check. When it does that, it will always reset itself to the commit given, and thus gpg.sh will use the trusted-keys of that commit. If you disable the clean merge check with --clean-merge 0, then it uses the trusted keys of the current tree and does not modify it at all.

[Sjors]

reset itself to the commit given

Unfortunately that commit can't be (an amended) aafa5e9 because that would simply fail. So when testing this PR it reset to the commit before it (that I used as the argument), with the trusted-keys at that time. But trying with --clean-merge 0 makes sense. And then indeed it seems to ignore the hashes in allow-revsig-commits.

So I guess the verification script only "works" if you enable the option(s) that cause it change commits, but either way allow-revsig-commits seems to be ignored.

[maflcko]

I think you can just bump the root, like it was done before, see d4b3dc5

[maflcko]

In any case, I think this can be merged as is, the intent should be clear here, and any bug fixes or behaviour changes of the script are better handled in a separate pull.

[maflcko]

left a comment, if you retouch

[maflcko]

Looks like this can be dropped after commit 2b0cd76

Probably the whole file can be dropped, but this can be done either here or in a follow-up.

[fanquake]

Lets sort this out in a followup, like #27058.

[achow101]

ACK aafa5e9

[fanquake]

ACK aafa5e9