Remove laanwj from trusted-keys

laanwj commented at 9:19 am on February 7, 2023: member

allow-revsig-commits list generated using:

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

allow-revsig-commits list generated using:

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

Tree-SHA512: e665d1f3f6ae45ad435cb2802d49988f5133d695b145aa2dc65af95c052e562e0afaf585c351a41529985b4229965cf555f7197a44c90ba7daaea7a28975648d

aafa5e945c

DrahtBot commented at 9:19 am on February 7, 2023: contributor

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 commented at 10:53 am on February 7, 2023: contributor

allow-revsig-commits list generated using:

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

Seems to match with:

0git 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:

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

achow101 commented at 5:55 pm on February 7, 2023: member

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 commented at 6:20 pm on February 7, 2023: member

tACK aafa5e945cef7a4f65ddadcf548932dd4e27ada1 😢

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 aafa5e945cef7a4f65ddadcf548932dd4e27ada1 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 commented at 6:26 pm on February 7, 2023: member

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 commented at 6:36 pm on February 7, 2023: member

reset itself to the commit given

Unfortunately that commit can’t be (an amended) aafa5e945cef7a4f65ddadcf548932dd4e27ada1 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 commented at 9:35 am on February 8, 2023: member

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

maflcko commented at 9:36 am on February 8, 2023: member

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.

fanquake referenced this in commit 2b0cd7679f on Feb 15, 2023

in contrib/verify-commits/allow-revsig-commits:820 in aafa5e945c

815+fee16b15fa3425871670239c25d4e61ae961e0c5
816+216f4ca9e7ccb1f0fcb9bab0f9940992a87ae55f
817+2d0bdb2089644f5904629413423cdc897911b081
818+50c502f54abd9eb15c8ddca013f0fdfae3d132a9
819+c840ab0231bc29057172179f005001c9ab299554
820+aab5e48d422d396aec09bd6389de68613b19def5

maflcko commented at 1:08 pm on February 15, 2023:

Looks like this can be dropped after commit 2b0cd7679f826af13e809df889093b4371c3e972

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

fanquake commented at 9:41 am on February 16, 2023:

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

maflcko approved

maflcko commented at 1:10 pm on February 15, 2023: member

left a comment, if you retouch

achow101 commented at 4:41 pm on February 15, 2023: member

ACK aafa5e945cef7a4f65ddadcf548932dd4e27ada1

fanquake approved

fanquake commented at 9:41 am on February 16, 2023: member

ACK aafa5e945cef7a4f65ddadcf548932dd4e27ada1

fanquake merged this on Feb 16, 2023

fanquake closed this on Feb 16, 2023

bitcoin locked this on Feb 16, 2024

Remove laanwj from trusted-keys #27054

Reviews

Conflicts