Skip to content

Revert 7deba93bdc76616011a9f493cbc203d60084416f and fix expired-key-sigs properly#12822

Merged
laanwj merged 3 commits intobitcoin:masterfrom
TheBlueMatt:master
Mar 30, 2018
Merged

Revert 7deba93bdc76616011a9f493cbc203d60084416f and fix expired-key-sigs properly#12822
laanwj merged 3 commits intobitcoin:masterfrom
TheBlueMatt:master

Conversation

@TheBlueMatt
Copy link
Copy Markdown
Contributor

@TheBlueMatt TheBlueMatt commented Mar 28, 2018

7deba93 was took the wrong approach to updating verify-commits for a key expiration. Namely, adding each commit to allow-revsig-commits should have been done instead, allowing them to still be validated, but with the expired key.

This reverts commit 7deba93.

This is neither a "test" change, nor should the trusted-git-root
have been updated - there is a process for expired PGP keys.
@laanwj
Copy link
Copy Markdown
Member

laanwj commented Mar 29, 2018

ACK 9471576

@TheBlueMatt
Copy link
Copy Markdown
Contributor Author

Added some documentation as requested, should be more clear in the future, I believe.

verify-commits will start failing to verify all commits which were signed by
said key. In order to avoid bumping the root-of-trust `trusted-git-root`
file, individual commits which were signed by such a key can be added to the
`allow-revsig-commits` file. That way, the PGP signatures are still verified
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

q: Is there a command to get a list of commits signed by a certain key? If so makes sense to mention that here.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not currently, I generated the diff here with this patch:

diff --git a/contrib/verify-commits/verify-commits.sh b/contrib/verify-commits/verify-commits.sh
index 532b97a438..eb41d90941 100755
--- a/contrib/verify-commits/verify-commits.sh
+++ b/contrib/verify-commits/verify-commits.sh
@@ -62,17 +62,22 @@ while true; do
        fi
 
        if ! git -c "gpg.program=${DIR}/gpg.sh" verify-commit "$CURRENT_COMMIT" > /dev/null; then
-               if [ "$PREV_COMMIT" != "" ]; then
-                       echo "No parent of $PREV_COMMIT was signed with a trusted key!" > /dev/stderr
-                       echo "Parents are:" > /dev/stderr
-                       PARENTS=$(git show -s --format=format:%P $PREV_COMMIT)
-                       for PARENT in $PARENTS; do
-                               git show -s $PARENT > /dev/stderr
-                       done
+               export BITCOIN_VERIFY_COMMITS_ALLOW_REVSIG=1
+               if git -c "gpg.program=${DIR}/gpg.sh" verify-commit "$CURRENT_COMMIT" > /dev/null; then
+                       echo "$CURRENT_COMMIT" > /dev/stderr
                else
-                       echo "$CURRENT_COMMIT was not signed with a trusted key!" > /dev/stderr
+                       if [ "$PREV_COMMIT" != "" ]; then
+                               echo "No parent of $PREV_COMMIT was signed with a trusted key!" > /dev/stderr
+                               echo "Parents are:" > /dev/stderr
+                               PARENTS=$(git show -s --format=format:%P $PREV_COMMIT)
+                               for PARENT in $PARENTS; do
+                                       git show -s $PARENT > /dev/stderr
+                               done
+                       else
+                               echo "$CURRENT_COMMIT was not signed with a trusted key!" > /dev/stderr
+                       fi
+                       exit 1
                fi
-               exit 1
        fi
 
        # We always verify the top of the tree

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah yes, patching the tool to print them out makes sense.

@laanwj
Copy link
Copy Markdown
Member

laanwj commented Mar 29, 2018

BTW: as I've started timestamping merge commits using opentimestamps, in theory it would be possible to verify the old commits using the expired key without worrying about backdating attacks. But that's probably quite involved, just an idea for the future.

@petertodd
Copy link
Copy Markdown
Contributor

@laanwj Actually it should be as easy as running git log --show-signature with OTS installed. GPG will verify the signature correctly regardless of whether it's expired or revoked, albeit with a large warning. All you have to do is look at the time that OTS reports and decide for yourself if that was prior to when the key became invalid. If it is, the signature can be trusted.

@TheBlueMatt
Copy link
Copy Markdown
Contributor Author

@petertodd care to look into a patch for gpg.sh to check if OTS is installed/at a locatoin specified by an env variable and call out to OTS if it is to aut-check that the timestamp is within a day or whatever of the claimed timestamp?

@petertodd
Copy link
Copy Markdown
Contributor

@TheBlueMatt As in, just check that the timestamp on the git commit is valid? Or check that the signature is also valid? I think most of the code required to check the timestamp on an arbitrary git commit exists. So doing that last part wouldn't be too hard.

@TheBlueMatt
Copy link
Copy Markdown
Contributor Author

@petertodd well the gpg.sh script already checks the signature, so just have to add the timestamp-checking part :p.

@laanwj
Copy link
Copy Markdown
Member

laanwj commented Mar 30, 2018

Going to merge this for now, let's do the timestamp thing separately.

@laanwj laanwj merged commit 9471576 into bitcoin:master Mar 30, 2018
laanwj added a commit that referenced this pull request Mar 30, 2018
9471576 [verify-commits] Add some additional useful documentation. (Matt Corallo)
de7e931 Add Marco-expired-key-signed-commits to allow-revsig-commits (Matt Corallo)
99f6d48 Revert "test: Update trust git root". (Matt Corallo)

Pull request description:

  7deba93 was took the wrong approach to updating verify-commits for a key expiration. Namely, adding each commit to allow-revsig-commits should have been done instead, allowing them to still be validated, but with the expired key.

Tree-SHA512: 9fdc67eda8f6daa95082f6c1a2af81beb730a9ff3f8cf930bb2311fe29b5f05e1f89259aba5f112153ca2e9c62577cf60d31b4c8e9ac1bf3f5506e78f8401378
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Oct 22, 2020
9471576 [verify-commits] Add some additional useful documentation. (Matt Corallo)
de7e931 Add Marco-expired-key-signed-commits to allow-revsig-commits (Matt Corallo)
99f6d48 Revert "test: Update trust git root". (Matt Corallo)

Pull request description:

  7deba93 was took the wrong approach to updating verify-commits for a key expiration. Namely, adding each commit to allow-revsig-commits should have been done instead, allowing them to still be validated, but with the expired key.

Tree-SHA512: 9fdc67eda8f6daa95082f6c1a2af81beb730a9ff3f8cf930bb2311fe29b5f05e1f89259aba5f112153ca2e9c62577cf60d31b4c8e9ac1bf3f5506e78f8401378
Signed-off-by: pasta <pasta@dashboost.org>

# Conflicts:
#	contrib/verify-commits/allow-revsig-commits
harding added a commit to bitcoin-core/bitcoincore.org that referenced this pull request Apr 23, 2021
6ffff92 Verify commits: put btcdrak's commits in revsig-commits (David A. Harding)
b0df500 Update gpg.sh to upstream version (David A. Harding)

Pull request description:

  @btcdrak's old key has expired, causing the verify-commits.sh script to fail and the website not to build normally.  Edit: this PR now updates the allow-revsig list as was indicated in bitcoin/bitcoin#12822

  ~~This updates the trusted commit to point to the latest commit using that key so only subsequent commits are validated.  I believe the procedure is for people to check their own local repository and see if the new trusted commit id is a part of their copy of the master branch---even better if it's buried somewhere under a commit you PGP signed.~~

  ```text
  user@devbco:~/bcco-master$ git show --show-signature 32b7869
  commit 32b7869
  gpg: Signature made Sat 16 Dec 2017 10:35:52 PM EST
  gpg:                using RSA key C26B028A44F111A0
  gpg: Good signature from "BtcDrak <btcdrak@gmail.com>" [unknown]
  gpg: Note: This key has expired!
  Primary key fingerprint: 912F D322 8387 123D C97E  0E57 D556 6241 A029 5FA9
       Subkey fingerprint: 8E75 26B0 B84F 2D55 8C5F  A39E C26B 028A 44F1 11A0
  ```

  ~~I'm just mimicking what was done [here](bitcoin/bitcoin@7deba93) and confirming that it resolves the problem with verify-commits.sh.  If there's a more appropriate solution, please let me know.~~

  @MarcoFalke of the regular reviewers for this repository, you seem to have the best understanding of the github-merge.py/verify-commits.sh workflow, so your review would be especially appreciated.

Top commit has no ACKs.

Tree-SHA512: 2182692f4633ebf2eb6bf7ba8f110c8623754b5561608e3bc04effa410ef3ab354475cd9b20366436b9a32d882b3ac958f7dda58ed97fd124a9f2044aac8f822
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants