Reorganize how wheels are stored, stop special-casing bootstrap #382

legoktm · 2022-09-26T20:46:03Z

Description

The main goal of this refactor is to make room for the new
securedrop-app-code bootstrap and wheels. The main scripts now take a
--project parameter, which is the path in this repository, and
--pkg-dir, which is the path to the Git checkout of it (previously a
PKG_DIR environment variable). In nearly all cases backwards-compat code
has been added so it should do the right thing based on old
documentation.

Each commit message has a detailed rationale and explanation of the changes made.

Considered but deferred

I briefly considered merging all the assorted ./scripts/ into one thing that just DTRT when you invoke it instead of having like 5 things you need to call in a specific order with mostly the same parameters but not exactly, but figured that would be a step too far for what this reorganization needs and deferred it for...later.

Checklist

bullseye builds and jobs are passing
This PR does not introduce any new bookworm test failures
If there are bookworm test failures, an issue has been filed for them

Per <git-lfs/git-lfs#325>, `git lfs pull` does exactly what we want. Tested by running the following in a bullseye container: > $ git clone https://github.com/freedomofpress/securedrop-debian-packaging > $ file bootstrap/build-0.3.0-py2.py3-none-any.whl > bootstrap/build-0.3.0-py2.py3-none-any.whl: ASCII text > # apt install git-lfs -y > $ git lfs install > Updated git hooks. > Git LFS initialized. > $ git lfs pull > $ file bootstrap/build-0.3.0-py2.py3-none-any.whl > bootstrap/build-0.3.0-py2.py3-none-any.whl: Zip archive data, at least > v2.0 to extract

gonzalo-bulnes

I think there is a typo / missing change in the README, and I left a question regarding the set options in a script. Otherwise it looks great 🟢

(I put red circles on the comments that need attention.)

gonzalo-bulnes · 2022-09-27T21:33:21Z

README.md

-of the SecureDrop release key to build the wheel and sign the updated sha256sums file
-with the release key. If you're not sure who to ask, ping @redshiftzero for a pointer.
+The next step is to build the wheels. To do this step, you will need a maintainer
+to build the wheels and sign the updated sha256sums file with the release key.


🔴 Isn't it with ~~"the release key"~~ "their personal key"? (Or "their registered key"?)

Done, switched to "individual key" (to avoid "personal" being conflated to "non-work")

scripts/build-sync-wheels

scripts/update-requirements

gonzalo-bulnes · 2022-09-27T21:55:15Z

scripts/verify-sha256sum-signature

-set -u
-set -o pipefail
-set -o nounset
+set -euo pipefail


🔴 scripts/sync-sha256sums uses set -euxo pipefail. Is there a reason for this one to be different?

The x is for verbose output that prints all the commands as they're executed. I felt it would be a bit too verbose for this script (which I think is probably already too verbose since it prints all the gpg output...) but if you disagree happy to add it in.

Makes sense, if it's intentional I trust your judgment 👍

gonzalo-bulnes · 2022-09-27T21:59:41Z

scripts/verify-sha256sum-signature

@@ -36,7 +26,7 @@ function verify_sha256sum_signature() {
    # Ensure temporary keyring is cleaned up afterward.
    trap 'rm -f "${temp_keyring}"' EXIT

-    # Import SecureDrop release key for verification.
+    # Import public keys for verification.


Since we're now using personal keys and no longer the SecureDrop release key: I think it could be helpful to add a hint or message in case the signature verification fails, to remind someone new to the process that they must "register" their personal key (by adding it to the pubkeys directory) if they want their signature to be valid.

To keep it simple, I added a line like Checking that SHA256SUMs signed by a key in pubkeys/... at the beginning of the script, what do you think? Given that it also prints all the keys, I hope that should be sufficient?

$ ./scripts/verify-sha256sum-signature ./workstation-bootstrap Checking that SHA256SUMs signed by a key in pubkeys/... gpg: key 9232DAD3105B8D58: public key "Allie Crevier <[email protected]>" imported gpg: key F08893B959CAB065: public key "Conor Schaefer <[email protected]>" imported gpg: key 07AD35D378D10BA0: public key "Cory Francis Myers <[email protected]>" imported gpg: key 99D3D09825B8D1DB: public key "Gonzalo Bulnes Guilpain <[email protected]>" imported gpg: key ACFE4353173015EA: public key "Kevin O'Gorman <[email protected]>" imported gpg: key 52FC8E7BEDB7FCA2: public key "Kunal Mehta <[email protected]>" imported gpg: key D8219C8C43F6C5E1: public key "Kushal Das <[email protected]>" imported gpg: key FB1BED6507701B45: public key "Michael Z <[email protected]>" imported gpg: key 188EDD3B7B22E6A3: public key "SecureDrop Release Signing Key <[email protected]>" imported gpg: key FC72AA838C444B47: public key "Rowen S <[email protected]>" imported gpg: Total number processed: 10 gpg: imported: 10 gpg: Signature made Wed Aug 31 03:27:41 2022 EDT gpg: using RSA key D8CB59F05DBB9E0538C4819DF105F8101B05269B gpg: Good signature from "Kunal Mehta <[email protected]>" [unknown] gpg: aka "Kunal Mehta <[email protected]>" [unknown] gpg: aka "Kunal Mehta <[email protected]>" [unknown] gpg: aka "Kunal Mehta <[email protected]>" [unknown] gpg: aka "Kunal Mehta <[email protected]>" [unknown] gpg: aka "Kunal Mehta <[email protected]>" [unknown] gpg: aka "Kunal Mehta <[email protected]>" [unknown] gpg: WARNING: This key is not certified with a trusted signature! gpg: There is no indication that the signature belongs to the owner. Primary key fingerprint: FA1E 9F9A 41E7 F435 02CA 5D63 52FC 8E7B EDB7 FCA2 Subkey fingerprint: D8CB 59F0 5DBB 9E05 38C4 819D F105 F810 1B05 269B Checking that SHA256SUMs from mirror match signed file... OK

This is a good idea I think. If we observe anyone being confused in the future, we can always reconsider moving more explicit guidance closer to the error message. 👍

securedrop-client/sha256sums.txt

securedrop-log/sha256sums.txt

securedrop-proxy/sha256sums.txt

* Recommend people use `make install-deps` to configure the venv, it'll always do the right thing. * sha256sums are signed by individual keys, not the release key. * No need to run sdist after uploading wheels.

Each project, whether it's securedrop-client or the workstation-bootstrap, will now store wheels and associated metadata in the exact same way. Moving files around will happen in follow-up commits. securedrop-client/ debian/ sha256sums.txt sha256sums.txt.asc wheels/ The workstation-bootstrap is similar, except it also contains its requirements files. workstation-bootstrap/ build-requirements.txt requirements.in requirements.txt sha256sums.txt sha256sums.txt.asc wheels/ The main goal of this refactor is to make room for the new securedrop-app-code bootstrap and wheels. The main scripts now take a `--project` parameter, which is the path in this repository, and `--pkg-dir`, which is the path to the Git checkout of it (previously a PKG_DIR environment variable). In nearly all cases backwards-compat code has been added so it should do the right thing based on old documentation. Makefile: * Updates for how scripts are now invoked. * Drop misleading "clean" target, people can use git-clean(1) directly. scripts/build-debianpackage: * Validate $PKG_NAME before we use it. * Set $WHEELS_DIR to that package's wheel directory. * Only verify sha256sums.txt if it exists (securedrop-export and metapackages have no Python dependencies). scripts/build-sync-wheels: * Switch to --pkg-dir/--project args, with backwards-compat. * Allow specifying where requirements.txt lives. * Drop "cache" terminology since this is persistent storage. * Remove dead commented-out code. scripts/install-deps: * Debian 11 is our baseline now, fix typo. * Look for and install wheels from workstation-bootstrap now. scripts/sync-sha256sums: * Require a directory instead of looking for a BOOTSTRAP variable. scripts/update-requirements: * Switch to --pkg-dir/--project args, with backwards-compat. * Allow specifying where requirements.txt lives. * Use pathlib internally scripts/verify-sha256sum-signature: * Require a directory instead of looking for a BOOTSTRAP variable. Refs <freedomofpress/securedrop#5901>.

All wheels and tarballs are identical (they're reproducible!), just moved around. Also verified that `make requirements` matches what is already committed in each of the three component repositories.

Notably remove test_wheel_builds_are_reproducible, which is always skipped.

gonzalo-bulnes

I'll merge when I come in next week. Anyone please feel free to merge before that : )

legoktm force-pushed the reorg branch from 7a91c2a to 83dad42 Compare September 26, 2022 20:47

legoktm force-pushed the reorg branch 2 times, most recently from b57f343 to cab9ddf Compare September 27, 2022 05:58

legoktm requested review from sssoleileraaa and gonzalo-bulnes September 27, 2022 06:04

gonzalo-bulnes suggested changes Sep 27, 2022

View reviewed changes

gonzalo-bulnes mentioned this pull request Sep 27, 2022

Upgrade mako==1.2.2 freedomofpress/securedrop-client#1566

Closed

legoktm added 6 commits September 30, 2022 11:45

Update some README steps

2a633b7

* Recommend people use `make install-deps` to configure the venv, it'll always do the right thing. * sha256sums are signed by individual keys, not the release key. * No need to run sdist after uploading wheels.

Move bootstrap to workstation-bootstrap and use new directory layout

57b8ed0

Use new directory layout for wheels

3ee076e

All wheels and tarballs are identical (they're reproducible!), just moved around. Also verified that `make requirements` matches what is already committed in each of the three component repositories.

Fix tests for script changes

a3808c1

Notably remove test_wheel_builds_are_reproducible, which is always skipped.

Make it clearer that we check against keys in pubkeys/

bf55e6e

legoktm force-pushed the reorg branch from cab9ddf to bf55e6e Compare September 30, 2022 15:46

legoktm requested a review from gonzalo-bulnes September 30, 2022 15:48

legoktm mentioned this pull request Sep 30, 2022

Using reproducible wheels into our securedrop-app-code Debian package on Focal freedomofpress/securedrop#5901

Open

5 tasks

gonzalo-bulnes approved these changes Sep 30, 2022

View reviewed changes

gonzalo-bulnes merged commit 4f4ce9c into main Oct 4, 2022

gonzalo-bulnes deleted the reorg branch October 4, 2022 00:49

This was referenced Oct 4, 2022

Fix command to install packaging dependencies freedomofpress/securedrop-client#1572

Closed

Fix Bookworm CI pipeline freedomofpress/securedrop-client#1565

Merged

eloquence mentioned this pull request Oct 4, 2023

Add support for building packages that use Poetry #467

Merged

10 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reorganize how wheels are stored, stop special-casing bootstrap #382

Reorganize how wheels are stored, stop special-casing bootstrap #382

legoktm commented Sep 26, 2022 •

edited

Loading

gonzalo-bulnes left a comment

gonzalo-bulnes Sep 27, 2022

legoktm Sep 30, 2022

gonzalo-bulnes Sep 27, 2022

legoktm Sep 30, 2022

gonzalo-bulnes Sep 30, 2022

gonzalo-bulnes Sep 27, 2022

legoktm Sep 30, 2022

gonzalo-bulnes Sep 30, 2022

gonzalo-bulnes left a comment

Reorganize how wheels are stored, stop special-casing bootstrap #382

Reorganize how wheels are stored, stop special-casing bootstrap #382

Conversation

legoktm commented Sep 26, 2022 • edited Loading

Description

Considered but deferred

Checklist

gonzalo-bulnes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gonzalo-bulnes left a comment

Choose a reason for hiding this comment

legoktm commented Sep 26, 2022 •

edited

Loading