Makefile updates and tools for requirements.txt and wheels #23

kushaldas · 2018-11-12T14:09:39Z

This PR has a few new scripts and related Makefile update. @conorsch I am guessing you will update the Makefile bit to make a bit more professional :)

These scripts and Makefile targets will help our package maintainers to create requirements.txt and requirements-dev.txt files for the actual release. Those files will contain the sha256sums for the binary wheels from our PyPI (the s3 bucket).

How to test?

First, let us the create/update the files, requires PKG_DIR environment variable.

PKG_DIR=/home/user/code/securedrop-client make requirements

This command will update the requirements/versions from the upstream PyPI. If there is any new update or if any of the new binary wheels are missing, this will fail. It will also print the next command a developer can use to generate the missing wheels.

$ PKG_DIR=/home/user/code/securedrop-client ./scripts/update-requirements 
The following dependent wheel(s) are missing:
pytest==3.10.1

Please build the wheel by using the following command:

	PKG_DIR=/home/user/code/securedrop-client make build-wheels

Then sync the newly built wheels and sources to the s3 bucket.
Also update the index HTML files accordingly and sync to s3.
After these steps, please rerun the command again.

If such missing wheels are there (you can delete any related wheel and test this), the above mentioned command will create such new wheels.

PKG_DIR=/home/user/code/securedrop-client make build-wheels

After this, remember to sync the localwheels directory to the s3, and also update the HTML index files and also the sha256sums.txt files as required. Also remember to deploy any new wheel+index to the s3 bucket.

The sha256sums.txt should always get new entries, but, there should not be any update of the any existing wheels or hashes. If there is any such change, it means one did a rebuild of the wheel which is already there in our s3/PyPI index. So, from the security aspect, we will have to check that in the CI for each commit.

kushaldas · 2018-11-13T17:48:19Z

Please remember that the pytest-3.10.1 is an example, I have already built and uploaded that to the s3.

conorsch · 2018-11-14T00:54:41Z

Rebasing on latest master (5411e3f) to review.

redshiftzero · 2018-11-14T01:30:08Z

Makefile

+.PHONY: build-wheels
+build-wheels: syncwheels ## Builds the wheels and syncs to the localwheels directory
+	./scripts/build-sync-wheels -r ${PKG_DIR}/requirements.txt
+	./scripts/build-sync-wheels -r ${PKG_DIR}/requirements-dev.txt


we don't need this right?

we don't need this right?

The build-wheels action is to help us build all the dependencies into wheels. We do need this.

sorry for lack of clarity, i meant we don't need the dev requirements files specifically, since we're not using them in the build

sorry for lack of clarity, i meant we don't need the dev requirements files specifically, since we're not using them in the build

Yup, and I removed that file. I forgot to update the comment here :(

redshiftzero · 2018-11-14T01:30:33Z

Makefile

+
+.PHONY: build-wheels
+build-wheels: syncwheels ## Builds the wheels and syncs to the localwheels directory
+	./scripts/build-sync-wheels -r ${PKG_DIR}/requirements.txt


so we don't need the requirements*.txt files in any of the proxy or client repos if this is merged, correct?

or do we use this script to generate those files and then commit them in the upstream repository? we should document this in the README if so

requirements*.txt files will be stored in the git and also be part of the source release. The pip will use these files to do the final install for the debian package. I will update the guide and readme for this.

redshiftzero · 2018-11-14T01:30:45Z

scripts/create-requirements

+
+# Now create both of the files
+pipenv lock -r > requirements.txt
+pipenv lock -d -r > requirements-dev.txt


remove this line?

~~We are keeping both of these files.~~ Now as I understood, you want to keep/track those development dependencies in our PyPI/setup at all, I can remove these then.

redshiftzero · 2018-11-14T01:33:38Z

scripts/sync-sha256sums

+set -o pipefail
+
+cd ./localwheels/
+sha256sum * > ../sha256sums.txt


let's document the fact that this file should be signed to the README

Yup, I will do that.

redshiftzero · 2018-11-14T01:33:50Z

scripts/update-requirements

+
+    # First remove index line and any PyQt or sip dependency
+    cleanup(req_path)
+    cleanup(reqdev_path)


redshiftzero · 2018-11-14T01:33:55Z

scripts/update-requirements

+
+    # Now let us update the files along with the sha256sums from localwheels
+    add_sha256sums(req_path)
+    add_sha256sums(reqdev_path)


remove (this applies to any dev requirements file)

Will be done.

scripts/update-requirements

conorsch · 2018-11-14T01:37:35Z

After this, remember to sync the localwheels directory to the s3, and also update the HTML index files and also the sha256sums.txt files as required. Also remember to deploy any new wheel+index to the s3 bucket.

Those steps should absolutely be automated—the "syncwheels" terminology presently in the repo should perhaps be called "fetch-wheels", since it doesn't actually synchronize them. Detailed instructions for syncing the wheels can be found over in freedomofpress/securedrop-debian-packaging-guide#6 ; those AWS S3 commands should absolutely be pulled into this repo's README, and ideally tooling, in order to bridge gaps in the dev workflow.

scripts/update-requirements

conorsch · 2018-11-14T01:49:04Z

scripts/create-requirements

+
+# Now create both of the files
+pipenv lock -r > requirements.txt
+pipenv lock -d -r > requirements-dev.txt


Having the requirements.txt files generated (and later committed) in a separate repo strikes me as a bad pattern. Let's instead pull those generated reqs files into this repo, and commit them here.

Those reqs files also must have hashes included (from the Pipfile.lock is fine).

Having the requirements.txt files generated (and later committed) in a separate repo strikes me as a bad pattern. Let's instead pull those generated reqs files into this repo, and commit them here.

The requirements.txt is part of the actual Python project and all Python projects should include one. This repository is only providing tooling+files related to debian packaging.

redshiftzero · 2018-11-14T01:57:32Z

So the python wheel builder steps are:

make requirements -> Generates requirements files for each project
make sync-sha256sums -> Generates SHA256 hashes for the tarballs

Important to add as discussed this morning: This should then bail if the SHA256 hashes (of the tarballs only) are not matching what is in Pipfile.lock in the project to be packaged

make build-wheels -> Builds wheels

Important to add:

Whenever SHA256sums are edited, the packager must be prompted to sign the document (in the script and in the README)
Whenever SHA256sums are used, the signature must be verified. If it does not verify, if it is not signed with the proper key, or if it does not exist, the Makefile target should bail.

These steps, if you agree (if not please correct them when you add to the README) we should add to the README.

redshiftzero · 2018-11-14T02:02:51Z

The other item we need to add is logic to scripts/build-debianpackage to bail if the signature on the document that contains the SHA256sums of the built wheels does not verify

These scripts will help a project maintainer to generate right requirements.txt and requirements-dev.txt files.

There are few new commands make requirements: to create requirements files for a project make build-wheels: to create and sync locally new wheels

We first download abnd verify the source tarballs based on the upstream PyPI hashes (from Pipfile.lock). Then when verify the sha256sum.txt file (gpg verify) and then using that file, we update the hashes in the requirements.txt file. Finally before the debian package build, we again gpg verify the sha256sums.txt file, and check the requirements.txt file and each of the hash values inside of it in the sha256sums.txt file.

Makefile

redshiftzero · 2018-11-14T21:54:41Z

Makefile

+	./scripts/update-requirements
+
+.PHONY: build-wheels
+build-wheels: syncwheels ## Builds the wheels and syncs to the localwheels directory


at this step we need to dump the current shasums from what we just downloaded from S3 to a new sha256sums-downloaded.txt file, and then verify the sig on the existing sha256sums.txt, and diff the two files. Or we can end up signing the hash of a compromised wheel from S3

The operation is strictly one-way, downloading the remote wheels from S3 and storing them in the `localwheels/` dir inside the packaging repository. For clarity, renaming the action to distinguish it from the *other* one-way action, pushing new wheels up to S3 (not yet codified). Also implements an optimization on the fetching operation, via the `--continue-at` flag to curl. Essentially the files will not be redownloaded if they exist locally. It's OK to skip a full download, since we'll be verifying integrity of the contents of the S3 buckets via a SHA256SUMS file, with detached sig, inside this packaging repo.

This option is used by default if there are hashes with the dependencies in the requirements.txt file. We specify it anyway to guard against use of a requirements.txt file without hashes.

The sha256sums file is used to verify the integrity of assets pulled in from S3; in order to trust the contents of the sha256sums file itself, we'll verify it via a detached, ASCII-armored GPG signature, stored in this packaging repository. We're creating a temporary keyring to perform the verification, otherwise gpg will report success if *any* key in the local keyring was used to create the detached sig; that's not what we want. Instead, only a small list of pubkeys should be considered valid, so we'll store those pubkeys here. Sprinkles the new script logic in a few locations, where we want to check the integrity; pending review, we may revise the locations of these calls out to the detached sig verification.

Manually checking for the local file and skipping if found. As comment indicates, `--continue-at -` doesn't work with S3.

Since we have a detached sig file on the SHA256SUMS list, we can check the assets fetched from S3 to ensure they match expectations.

Must come after the `--no-binary` option, since `:all:` is a `format_control` arg to `--no-binary`. Simple reorder in the command concatenation fixes.

redshiftzero

Tested locally and approving based on these latest changes

kushaldas requested review from conorsch, emkll and redshiftzero November 12, 2018 14:09

conorsch mentioned this pull request Nov 13, 2018

Create tooling for building packages #6

Closed

redshiftzero reviewed Nov 14, 2018

View reviewed changes

scripts/update-requirements Show resolved Hide resolved

redshiftzero reviewed Nov 14, 2018

View reviewed changes

scripts/update-requirements Outdated Show resolved Hide resolved

redshiftzero reviewed Nov 14, 2018

View reviewed changes

scripts/update-requirements Show resolved Hide resolved

redshiftzero reviewed Nov 14, 2018

View reviewed changes

scripts/update-requirements Show resolved Hide resolved

conorsch reviewed Nov 14, 2018

View reviewed changes

conorsch force-pushed the requirements branch from 967102f to 0f7edbd Compare November 14, 2018 02:03

kushaldas added 5 commits November 14, 2018 16:22

Adds scripts to autogenerate requirements files

6840a12

These scripts will help a project maintainer to generate right requirements.txt and requirements-dev.txt files.

Adds script to update sha256sums

8f56f1a

Updates script to use hashes of the wheels

4a544a2

Updates Makefile for requirements and wheel generation

3d20065

There are few new commands make requirements: to create requirements files for a project make build-wheels: to create and sync locally new wheels

Removes any extra ;

423ebfc

kushaldas force-pushed the requirements branch from 0f7edbd to 13d5907 Compare November 14, 2018 13:51

kushaldas force-pushed the requirements branch from 13d5907 to ebbfa90 Compare November 14, 2018 17:33

redshiftzero reviewed Nov 14, 2018

View reviewed changes

Makefile Show resolved Hide resolved

redshiftzero reviewed Nov 14, 2018

View reviewed changes

Makefile Outdated Show resolved Hide resolved

redshiftzero reviewed Nov 14, 2018

View reviewed changes

Makefile Show resolved Hide resolved

redshiftzero reviewed Nov 14, 2018

View reviewed changes

Makefile Show resolved Hide resolved

redshiftzero added 2 commits November 14, 2018 13:45

Remove make sync-sha256sums

897212d

Sign sha256sums file after generation

d6ff776

redshiftzero reviewed Nov 14, 2018

View reviewed changes

redshiftzero and others added 6 commits November 14, 2018 13:59

Point the user to the documentation for updating the S3 PyPI index

5a41d58

Add --require-hashes option to pip download

3bf465d

This option is used by default if there are hashes with the dependencies in the requirements.txt file. We specify it anyway to guard against use of a requirements.txt file without hashes.

Explain use of make build-wheels and make requirements

6739760

Remove step now done by release managers

551a183

Add a diagram of the packaging workflow for clarity

24b01c8

This was referenced Nov 14, 2018

Remove now unnecessary requirements-dev.txt freedomofpress/securedrop-client#172

Merged

Remove now unnecessary requirements-dev.txt freedomofpress/securedrop-proxy#20

Merged

Conor Schaefer and others added 5 commits November 14, 2018 15:39

Updates existence check in fetch-wheels

f1d5279

Manually checking for the local file and skipping if found. As comment indicates, `--continue-at -` doesn't work with S3.

Compares checksums of S3 assets to expected list

e820c44

Since we have a detached sig file on the SHA256SUMS list, we can check the assets fetched from S3 to ensure they match expectations.

Fixes ordering on --require-hashes for pip3

c2ddf5a

Must come after the `--no-binary` option, since `:all:` is a `format_control` arg to `--no-binary`. Simple reorder in the command concatenation fixes.

Move notification about S3 push to Makefile

2e5220c

redshiftzero force-pushed the requirements branch from ae79e1c to 2e5220c Compare November 15, 2018 00:40

redshiftzero approved these changes Nov 15, 2018

View reviewed changes

redshiftzero merged commit f7963ce into master Nov 15, 2018

redshiftzero deleted the requirements branch November 15, 2018 01:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Makefile updates and tools for requirements.txt and wheels #23

Makefile updates and tools for requirements.txt and wheels #23

kushaldas commented Nov 12, 2018

kushaldas commented Nov 13, 2018

conorsch commented Nov 14, 2018

redshiftzero Nov 14, 2018

kushaldas Nov 14, 2018

redshiftzero Nov 14, 2018

kushaldas Nov 14, 2018

redshiftzero Nov 14, 2018

redshiftzero Nov 14, 2018

kushaldas Nov 14, 2018

redshiftzero Nov 14, 2018

kushaldas Nov 14, 2018 •

edited

Loading

redshiftzero Nov 14, 2018

kushaldas Nov 14, 2018

redshiftzero Nov 14, 2018

kushaldas Nov 14, 2018

redshiftzero Nov 14, 2018 •

edited

Loading

kushaldas Nov 14, 2018

conorsch commented Nov 14, 2018

conorsch Nov 14, 2018

kushaldas Nov 14, 2018

redshiftzero commented Nov 14, 2018 •

edited

Loading

redshiftzero commented Nov 14, 2018

redshiftzero Nov 14, 2018

redshiftzero left a comment

Makefile updates and tools for requirements.txt and wheels #23

Makefile updates and tools for requirements.txt and wheels #23

Conversation

kushaldas commented Nov 12, 2018

How to test?

kushaldas commented Nov 13, 2018

conorsch commented Nov 14, 2018

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

kushaldas Nov 14, 2018 • edited Loading

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

redshiftzero Nov 14, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

conorsch commented Nov 14, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

redshiftzero commented Nov 14, 2018 • edited Loading

redshiftzero commented Nov 14, 2018

Choose a reason for hiding this comment

redshiftzero left a comment

Choose a reason for hiding this comment

kushaldas Nov 14, 2018 •

edited

Loading

redshiftzero Nov 14, 2018 •

edited

Loading

redshiftzero commented Nov 14, 2018 •

edited

Loading