Following up on #23020 from jamesob with achow101’s additional features on top.
Both mentioned that they will be away for the next few weeks, so this is intended to keep review going.
All credit to the jamesob and achow101. See #23020 for the original description and here for the added features.
I squashed the last commit from https://github.com/achow101/bitcoin/tree/pr23020-direct-bins-gpg-parse into the first commit here.
Fetching and local verification seem to work as intended for me.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | josibake |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
No conflicts as of last run.
This commit adds the functionality necessary to transition from
doing binary verification on the basis of a single signature to
requiring a minimum threshold of trusted signatures.
A signature can appear as "good" from GPG output, but it may not come
from an identity the user trusts. We call these "good, untrusted"
signatures.
We report bad signatures but do not necessarily fail in their presence,
since a bad signature might coexist with enough good, trusted signatures
to fulfill our criteria.
If "--import-keys" is enabled, we will prompt the user to
optionally try to retrieve unknown keys. Marking them as trusted locally
is a WIP, but keys which are retrieved successfully and appear on the
builder-keys list will immediately count as being useful towards
fulfilling the threshold.
Logging is improved and an option to output JSON that summarizes the
whole sum signature and binary verification processes has been added.
Co-authored-by: Russ Yanofsky <russ@yanofsky.org>
Co-authored-by: willcl-ark <will8clark@gmail.com>
Prepares for the option to provide local binaries, sha256sums, and
signatures directly.
In addition to verifying the published releases with the `pub` command,
the verifybinaries script is updated to take a `bin` command where the
user specifies the local files, sums, and sigs to verify.
GPG has an option to provide machine parseable output. Use that instead
of trying to parse the human readable output.
19
20-The script returns 0 if everything passes the checks. It returns 1 if either the signature check or the hash check doesn't pass. If an error occurs the return value is 2.
21+You can obtain these keys by
22+ - through a browser using a key server (e.g. keyserver.ubuntu.com),
23+ - manually using the `gpg --keyserver <url> --recv-keys <key>` command, or
24+ - you can run the packaged `verifybinaries.py ... --import-keys` script to
2e187ffafc9cb0490eedd079ce1cfffafecd9dba: trailing whitespace
0 - you can run the packaged `verifybinaries.py ... --import-keys` script to
0@@ -0,0 +1,59 @@
1+#!/usr/bin/env python3
verify-commits, so I think the directory should be verify-binaries.
19
20-The script returns 0 if everything passes the checks. It returns 1 if either the signature check or the hash check doesn't pass. If an error occurs the return value is 2.
21+You can obtain these keys by
22+ - through a browser using a key server (e.g. keyserver.ubuntu.com),
23+ - manually using the `gpg --keyserver <url> --recv-keys <key>` command, or
24+ - you can run the packaged `verifybinaries.py ... --import-keys` script to
verify.py to verifybinaries.py defined?
verify.py ?
verify.py.
44+
45+#### Examples
46+
47+Validate releases with default settings:
48+```sh
49+./contrib/verifybinaries/verify.py 22.0
pub or bin args, no?
46+
47+Validate releases with default settings:
48+```sh
49+./contrib/verifybinaries/verify.py 22.0
50+./contrib/verifybinaries/verify.py 22.0-rc2
51+./contrib/verifybinaries/verify.py bitcoin-core-23.0
bitcoin-core prefix to the example?
Fetching and local verification seem to work as intended for me.
Can you post your commands? Nothing is currently working as advertised in the README for me ^^.
@TheCharlatan I see it working using e.g. the pub subcommand. You need to choose pub or bin to verify either the publicly-hosted binaries, or bin for a local binary. But you are correct the docs are now outdated…
0₿ ./contrib/verifybinaries/verify.py pub bitcoin-core-23.0-x86_64
1[INFO] got file https://bitcoincore.org/bin/bitcoin-core-23.0/SHA256SUMS.asc as SHA256SUMS.asc
2[WARNING] https://bitcoin.org failed to provide file (https://bitcoin.org/bin/bitcoin-core-23.0/SHA256SUMS.asc). Continuing based solely upon https://bitcoincore.org.
3[INFO] got file https://bitcoincore.org/bin/bitcoin-core-23.0/SHA256SUMS as SHA256SUMS
4[WARNING] https://bitcoin.org failed to provide file (https://bitcoin.org/bin/bitcoin-core-23.0/SHA256SUMS). Continuing based solely upon https://bitcoincore.org.
5[INFO] got 17 good signatures
6[INFO] GOOD SIGNATURE (untrusted): SigData('17565732E08E5E41', 'Andrew Chow <andrew@achow101.com>', trusted=False, status='')
7[INFO] GOOD SIGNATURE (untrusted): SigData('D7CC770B81FD22A8', 'Ben Carman <benthecarman@live.com>', trusted=False, status='expired')
8[INFO] GOOD SIGNATURE (untrusted): SigData('E13FC145CD3F4304', 'Antoine Poinsot <darosior@protonmail.com>', trusted=False, status='')
9[INFO] GOOD SIGNATURE (untrusted): SigData('ED357015286A333D', 'Duncan Dean <duncangleeddean@gmail.com>', trusted=False, status='')
10[INFO] GOOD SIGNATURE (untrusted): SigData('3152347D07DA627C', 'Stephan Oeste (it) <it@oeste.de>', trusted=False, status='')
11[INFO] GOOD SIGNATURE (untrusted): SigData('2EEB9F5CC09526C1', 'Michael Ford (bitcoin-otc) <fanquake@gmail.com>', trusted=False, status='')
12[INFO] GOOD SIGNATURE (untrusted): SigData('8E4256593F177720', 'Oliver Gugger <gugger@gmail.com>', trusted=False, status='')
13[INFO] GOOD SIGNATURE (untrusted): SigData('410108112E7EA81F', 'Hennadii Stepanov (GitHub key) <32963518+hebasto@users.noreply.github.com>', trusted=False, status='')
14[INFO] GOOD SIGNATURE (untrusted): SigData('D11BD4F33F1DB499', 'jackielove4u <jackielove4u@hotmail.com>', trusted=False, status='')
15[INFO] GOOD SIGNATURE (untrusted): SigData('DD78544CF91B1514', 'Jonas Ott (jonas-ott) <git@jonas-ott.de>', trusted=False, status='')
16[INFO] GOOD SIGNATURE (untrusted): SigData('F62711DBDCA8AE56', 'Dimitri <kvaciral@protonmail.com>', trusted=False, status='')
17[INFO] GOOD SIGNATURE (untrusted): SigData('1E4AED62986CD25D', 'Wladimir J. van der Laan <laanwj@protonmail.com>', trusted=False, status='')
18[INFO] GOOD SIGNATURE (untrusted): SigData('BD02942421F4889F', 'Luke Dashjr <luke@dashjr.org>', trusted=False, status='revoked')
19[INFO] GOOD SIGNATURE (untrusted): SigData('0A41BDC3F4FAFF1C', 'Aaron Clauson (sipsorcery) <aaron@sipsorcery.com>', trusted=False, status='')
20[INFO] GOOD SIGNATURE (untrusted): SigData('7E296D555E7F63A7', 'Sjors Provoost <sjors@purpledunes.com>', trusted=False, status='')
21[INFO] GOOD SIGNATURE (untrusted): SigData('A7BEB2621678D37D', 'vertion <vertion@protonmail.com>', trusted=False, status='expired')
22[INFO] GOOD SIGNATURE (untrusted): SigData('099BAD163C70FBFA', 'Will Clark <will8clark@gmail.com>', trusted=False, status='')
23[WARNING] key D7CC770B81FD22A8 for Ben Carman <benthecarman@live.com> is expired
24[WARNING] key A7BEB2621678D37D for vertion <vertion@protonmail.com> is expired
25[INFO] removing *-unsigned binaries (bitcoin-23.0-arm64-apple-darwin-unsigned.dmg, bitcoin-23.0-arm64-apple-darwin-unsigned.tar.gz, bitcoin-23.0-x86_64-apple-darwin-unsigned.dmg, bitcoin-23.0-x86_64-apple-darwin-unsigned.tar.gz, bitcoin-23.0-win64-setup-unsigned.exe, bitcoin-23.0-win64-unsigned.tar.gz) from verification since https://bitcoincore.org does not host *-unsigned binaries
26[INFO] removing *-debug binaries (bitcoin-23.0-aarch64-linux-gnu-debug.tar.gz, bitcoin-23.0-arm-linux-gnueabihf-debug.tar.gz, bitcoin-23.0-powerpc64-linux-gnu-debug.tar.gz, bitcoin-23.0-powerpc64le-linux-gnu-debug.tar.gz, bitcoin-23.0-riscv64-linux-gnu-debug.tar.gz, bitcoin-23.0-x86_64-linux-gnu-debug.tar.gz, bitcoin-23.0-win64-debug.zip) from verification since https://bitcoincore.org does not host *-debug binaries
27[INFO] removing *-codesignatures binaries (bitcoin-23.0-codesignatures-e36a046909ad.tar.gz) from verification since https://bitcoincore.org does not host *-codesignatures binaries
28[INFO] downloading bitcoin-23.0-aarch64-linux-gnu.tar.gz
29 [INFO] downloading bitcoin-23.0-arm-linux-gnueabihf.tar.gz
30[INFO] downloading bitcoin-23.0-arm64-apple-darwin.dmg
31[INFO] downloading bitcoin-23.0-arm64-apple-darwin.tar.gz
32[INFO] downloading bitcoin-23.0.tar.gz
33[INFO] downloading bitcoin-23.0-powerpc64-linux-gnu.tar.gz
34[INFO] downloading bitcoin-23.0-powerpc64le-linux-gnu.tar.gz
35[INFO] downloading bitcoin-23.0-riscv64-linux-gnu.tar.gz
36[INFO] downloading bitcoin-23.0-x86_64-apple-darwin.dmg
37[INFO] downloading bitcoin-23.0-x86_64-apple-darwin.tar.gz
38[INFO] downloading bitcoin-23.0-x86_64-linux-gnu.tar.gz
39[INFO] downloading bitcoin-23.0-win64-setup.exe
40[INFO] downloading bitcoin-23.0-win64.zip
41[INFO] did not clean up /tmp/bitcoin_verify_binaries.bitcoin-core-23.0-x86_64
42VERIFIED: bitcoin-23.0-aarch64-linux-gnu.tar.gz
43VERIFIED: bitcoin-23.0-arm-linux-gnueabihf.tar.gz
44VERIFIED: bitcoin-23.0-arm64-apple-darwin.dmg
45VERIFIED: bitcoin-23.0-arm64-apple-darwin.tar.gz
46VERIFIED: bitcoin-23.0.tar.gz
47VERIFIED: bitcoin-23.0-powerpc64-linux-gnu.tar.gz
48VERIFIED: bitcoin-23.0-powerpc64le-linux-gnu.tar.gz
49VERIFIED: bitcoin-23.0-riscv64-linux-gnu.tar.gz
50VERIFIED: bitcoin-23.0-x86_64-apple-darwin.dmg
51VERIFIED: bitcoin-23.0-x86_64-apple-darwin.tar.gz
52VERIFIED: bitcoin-23.0-x86_64-linux-gnu.tar.gz
53VERIFIED: bitcoin-23.0-win64-setup.exe
54VERIFIED: bitcoin-23.0-win64.zip
@TheCharlatan I see it working using e.g. the pub subcommand. You need to choose pub or bin to verify either the publicly-hosted binaries, or bin for a local binary. But you are correct the docs are now outdated…
Mmh, shouldn’t the sample command you posted only have verified x86_64 binaries?
@TheCharlatan I see it working using e.g. the pub subcommand. You need to choose pub or bin to verify either the publicly-hosted binaries, or bin for a local binary. But you are correct the docs are now outdated…
Mmh, shouldn’t the sample command you posted only have verified x86_64 binaries?
Mmm, yeah, this is awkward. @achow101’s original changes forced the user to specify which binaries should be checked. I requested using the contents of the SHA256SUMS file instead.
The problem with teaching the bin command the version syntax from pub is that that syntax is pretty remote-url centric. So I’m not sure it makes sense to allow the user to use the same type of filter for bin.
Should we just go back to requiring the user to specify the bins? Downloading to a clean dir and using a wildcard should still work for tooling I think. @TheBlueMatt ?
82
83 If you do not want to keep the downloaded binaries, specify anything as the second parameter.
84
85 ```sh
86-./verify.py bitcoin-core-0.13.0 delete
87+./contrib/verifybinaries/verify.py bitcoin-core-22.0 delete
From #23020 (review)
Should we just add an explicit
--deleteargument to the script, rather than “append a random anything”, as was proposed in #26985.
pub --cleanup. A quick test shows it working as intended. Will update the doc.
47+Validate releases with default settings:
48+```sh
49+./contrib/verifybinaries/verify.py 22.0
50+./contrib/verifybinaries/verify.py 22.0-rc2
51+./contrib/verifybinaries/verify.py bitcoin-core-23.0
52+./contrib/verifybinaries/verify.py bitcoin-core-23.0-rc1
nit: it would be nice to have a bin example as well
0./contrib/verifybinaries/verify.py pub bitcoin-core-23.0-rc1
1./contrib/verifybinaries/verify.py bin ~/Downloads/SHA256SUMS ~/Downloads/bitcoin-24.0.1-x86_64-linux-gnu.tar.gz
547+ if gpg_retval == 1:
548+ log.critical(f"Bad signature (code: {gpg_retval}).")
549+ if gpg_retval == 2:
550+ log.critical(
551+ "gpg error. Do you have the Bitcoin Core binary release "
552+ "signing key installed?")
nit: This seems like it can be deleted, since 2 is an allowed return code and missing “the Bitcoin Core binary release signing key” doesn’t apply
Downloading to a clean dir and using a wildcard should still work for tooling I think. @TheBlueMatt ?
Not sure exactly what you mean here - part of the goal is to have tooling to verify all the binaries sitting in a folder? If you can “download” all of them in one go from file:/// I guess that works too?
I believe I’ve addressed the comments here. I’ve chosen to add fresh comments on top rather than squashing to avoid mixing up my changes on top of @jamesob’s/@achow101’s.
The os/arch filters like verify.py 22.0-osx no longer work for me and I’m not quite sure why. Otherwise remote/local verification seem to work ok.
302+ success, output = download_with_wget(url, filename)
303+ if not success:
304+ log.error(
305+ f"couldn't fetch file ({url}). "
306+ "Have you specified the version number in the following format?\n"
307+ f"[{VERSIONPREFIX}]{VERSION_FORMAT} "
VERSIONPREFIX from this output.
747+ help=(
748+ 'The minimum number of good signatures to require successful termination.'),
749+ )
750+ parser.add_argument(
751+ '--keyserver', action='store', nargs='?',
752+ default=os.environ.get('BINVERIFY_KEYSERVER', 'hkp://keyserver.ubuntu.com'),
61+
62+Don't trust builder-keys by default, and rely only on local GPG state and manually
63+specified keys, while requiring a threshold of at least 10 trusted signatures:
64+```sh
65+./contrib/verifybinaries/verify.py pub 22.0-x86 \
66+ --no-trust-builder-keys \
--no-trust-builder-keys is no-longer a thing? Arguments here need re-ordering as well.
63+specified keys, while requiring a threshold of at least 10 trusted signatures:
64+```sh
65+./contrib/verifybinaries/verify.py pub 22.0-x86 \
66+ --no-trust-builder-keys \
67+ --trusted-keys 74E2DEF5D77260B98BC19438099BAD163C70FBFA,9D3CC86A72F8494342EA5FD10A41BDC3F4FAFF1C \
68+ --min-trusted-sigs 10
0 --min-good-sigs 10
54+```
55+
56+Get JSON output and don't prompt for user input (no auto key import):
57+
58+```sh
59+./contrib/verifybinaries/verify.py pub 22.0-x86 --json
0./contrib/verifybinaries/verify.py --json pub 22.0-x86
otherwise i get this:
0$ ./contrib/verifybinaries/verify.py pub 22.0-x86 --json
1usage: verify.py [-h] [-v] [-q] [--import-keys] [--min-good-sigs [MIN_GOOD_SIGS]] [--keyserver [KEYSERVER]] [--trusted-keys [TRUSTED_KEYS]] [--json] {pub,bin} ...
2verify.py: error: unrecognized arguments: --json
./verify.py --json pub 22.0-x86 downloads everything
65+./contrib/verifybinaries/verify.py \
66+ --trusted-keys 74E2DEF5D77260B98BC19438099BAD163C70FBFA,9D3CC86A72F8494342EA5FD10A41BDC3F4FAFF1C \
67+ --min-good-sigs 10 pub 22.0-x86
68 ```
69
70 If you only want to download the binaries of certain platform, add the corresponding suffix, e.g.:
nit:
0If you only want to download the binaries for a certain platform, add the corresponding suffix, e.g.:
10+ """Tests ordered roughly from faster to slower."""
11+ expect_code(run_verify("", "pub", '0.32'), 4, "Nonexistent version should fail")
12+ expect_code(run_verify("", "pub", '0.32.awefa.12f9h'), 11, "Malformed version should fail")
13+ expect_code(run_verify('--min-good-sigs 20', "pub", "22.0"), 9, "--min-good-sigs 20 should fail")
14+
15+ print("- testing multisig verification (22.0)", flush=True)
nit: would avoid using the term multisig because of its meaning in other bitcoin contexts
0 print("- testing verification (22.0)", flush=True)
19
20-The script returns 0 if everything passes the checks. It returns 1 if either the signature check or the hash check doesn't pass. If an error occurs the return value is 2.
21+You can obtain these keys by
22+ - through a browser using a key server (e.g. keyserver.ubuntu.com),
23+ - manually using the `gpg --keyserver <url> --recv-keys <key>` command, or
24+ - you can run the packaged `verifybinaries.py ... --import-keys` script to
0 - you can run the packaged `verify.py ... --import-keys` script to
1- ```
48 ```sh
49-./verify.py bitcoin-core-0.11.2
50-./verify.py bitcoin-core-0.12.0
51-./verify.py bitcoin-core-0.13.0-rc3
52+./contrib/verifybinaries/verify.py pub 22.0
53+./contrib/verifybinaries/verify.py pub 22.0-rc2
README.md?
62+Rely only on local GPG state and manually specified keys, while requiring a
63+threshold of at least 10 trusted signatures:
64+```sh
65+./contrib/verifybinaries/verify.py \
66+ --trusted-keys 74E2DEF5D77260B98BC19438099BAD163C70FBFA,9D3CC86A72F8494342EA5FD10A41BDC3F4FAFF1C \
67+ --min-good-sigs 10 pub 22.0-x86
-x86)
Concept ACK
Downloading for a specific platform doesn’t work, so I’d suggest we either fix it before merging or remove it from the docs. I also left a few other nits but they aren’t important.
580+ if sigs_status == ReturnCode.INTEGRITY_FAILURE:
581+ cleanup()
582+ return sigs_status
583+
584+ # Extract hashes and filenames
585+ hashes_to_verify = parse_sums_file(SUMS_FILENAME, os_filter)
0 hashes_to_verify = parse_sums_file(SUMS_FILENAME, [os_filter])
parse_sums_file expects a list of strings. This should fix the download selection filter.
Nit: After discovering #27358 (review) I ran mypy verify.py and:
0verify.py:129: error: Module has no attribute "HTTPError"
1verify.py:588: error: Argument 1 to "verify_shasums_signature" has incompatible type "Path"; expected "str"
2Found 2 errors in 1 file (checked 1 source file)
- Use correct name for verify.py
- Add usage examples for verifybinaries bin
- Document proper use of new cleanup option
- Fixup broken example
It may be useful for local validation.
Co-authored-by: Reproducibility Matters <seb.kung@gmail.com>
602
603 # download binaries
604 for _, binary_filename in hashes_to_verify:
605- print(f"Downloading {binary_filename}")
606- download_with_wget(HOST1 + remote_dir + binary_filename)
607+ log.info(f"downloading {binary_filename}")
Thanks for the review @TheCharlatan and @josibake. I’ve addressed most of the feedback with the exception of a few nits as I don’t want this to drag out forever.
I squashed some of the readme changes to keep the number of commits down. The diff from the previous changes can be seen with: git diff 96344222f3..754fb6bb81 .
Followup suggestions:
allow user to specify the domain(s), e.g. --domain=bitcoincore.org so you don’t get warnings about bitcoin.org, which only has up to version 22.0 at the moment. Conversely, it will make a future domain evacuation safer.
“osx” in the documentation example should be “macos” (but it doesn’t work either way as @theuni found)
“The os/arch filters like verify.py 22.0-osx no longer work for me and I’m not quite sure why.” - would be nice to restore this, or just file a Github issue and remove it from the docs
@Sjors That filter problem should’ve been fixed by https://github.com/bitcoin/bitcoin/pull/27358/commits/4b23b488d2c5662215d78e4963ef5a2b86b4e25b, my comment was from before then. Is what’s now in master still broken for you?
Edit: Also, agreed on your first suggestion.
Labels
Scripts and tools
Milestone
25.0