Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Verifies signatures in CI #4

Merged
merged 1 commit into from
Jan 31, 2020
Merged

Verifies signatures in CI #4

merged 1 commit into from
Jan 31, 2020

Conversation

emkll
Copy link
Contributor

@emkll emkll commented Jan 23, 2020

very simple scripts that iterates through files in the workstation repo and verifies their signature. Specifies --test in this repo as this is the dev repo signed with the test key.

Note that the repo as initially cloned does not have git lfs hooks enabled due to circle re-writing the git urls per the gitconfig. We reclone via HTTPS inside the checked out repo to avoid having to deal with rewriting remote urls in the repo git config in the circle container.

Test plan

@emkll emkll force-pushed the verify-sigs branch 3 times, most recently from af92b36 to d974449 Compare January 23, 2020 22:33
@emkll emkll requested a review from conorsch January 23, 2020 22:43
@emkll emkll marked this pull request as ready for review January 23, 2020 22:55
@conorsch
Copy link
Contributor

Looking good. Haven't done an in-depth functional review, since I'd prefer to shake out RPM nightly implications such as freedomofpress/securedrop-workstation#406 (comment) before deciding what the sig verification story needs to look like.

Every clone is 3GB

On a full copy of this repo, I'm seeing 5207MB. However, if I clone with --depth=1, I see 1627MB. So perhaps we should consider shallow clones in CI, since we only care about verifying sigs on new artifacts.

@emkll
Copy link
Contributor Author

emkll commented Jan 28, 2020

Thanks @conorsch , updated with shallow clone and rebased on latest master. I've also opened freedomofpress/securedrop-builder#135 to reduce lfs usage for nightlies as well.

Copy link
Contributor

@conorsch conorsch left a comment

Choose a reason for hiding this comment

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

Looks good. Left some comments throughout targeting some cleanups, mostly to make the verification more resilient in the face of user error, as well as solidify our approach to using git-lfs in CircleCI.

Most of these approaches are optional; however, wanted you to review the comments before clicking merge, in case the changes apply to other repos, such as packaging.

docker:
- image: circleci/python:3.7-buster
steps:
- checkout
Copy link
Contributor

Choose a reason for hiding this comment

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

From CircleCI docs:

If you require doing git over HTTPS you should not use this step as it configures git to checkout over ssh.

Since there's a git clone --depth=1 in the subsequent tasks, I believe we want to remove the "checkout" step entirely—we could probably remove the "workaround for git-lfs" task, as well.

.circleci/config.yml Outdated Show resolved Hide resolved
scripts/check.py Outdated Show resolved Hide resolved
scripts/check.py Outdated Show resolved Hide resolved
scripts/check.py Outdated Show resolved Hide resolved
scripts/check.py Outdated Show resolved Hide resolved
Small scripts that iterates through files in the workstation repo and verifies their signature. Specifies `--test` in this repo as this is the dev repo signed with the test key.
@emkll
Copy link
Contributor Author

emkll commented Jan 31, 2020

@conorsch thanks for the review, all great points. All comments have been addressed, commit squashed, ready for re-review

@conorsch conorsch self-requested a review January 31, 2020 20:20
Copy link
Contributor

@conorsch conorsch left a comment

Choose a reason for hiding this comment

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

Fantastic, much obliged!

Local review of script functionality checks out, confirmed test/prod pubkeys are what we expect.

@conorsch conorsch merged commit 5571039 into master Jan 31, 2020
@emkll emkll deleted the verify-sigs branch July 21, 2020 15:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants