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

Strengthen CI checks to sanity check rulesets #21

Open
2 of 3 tasks
conorsch opened this issue Dec 9, 2020 · 6 comments · May be fixed by #202
Open
2 of 3 tasks

Strengthen CI checks to sanity check rulesets #21

conorsch opened this issue Dec 9, 2020 · 6 comments · May be fixed by #202
Assignees

Comments

@conorsch
Copy link
Contributor

conorsch commented Dec 9, 2020

The review process for PRs into this repo is currently entirely manual: https://github.com/freedomofpress/securedrop-https-everywhere-ruleset/blob/276c1da5f83bef16661d4367772a7b50f611701a/.github/PULL_REQUEST_TEMPLATE.md

Some simple checks we can perform in CI are:

  • Does the number XML files in rulesets/ and the number of rules declared in the gzipped json match? (I recently made a mistake generating rulesets, and the gzip file had a json file with zero rules. Would have been nice to have CI complain about this.)
  • Does the timestamp in latest-rulesets-timestamp match the latest available in the gzipped files?
  • Is the signature valid? We perform similar checks for package releases, and having the CI red -> green transition really helps with collaboration by multiple folks on the same PR, as with Add 3 new instances to ruleset #19

More suggestions welcome.

@conorsch
Copy link
Contributor Author

Also: do the number of rules gzipped and number of signatures match 1:1? They didn't in #19 (review), for example.

@conorsch
Copy link
Contributor Author

conorsch commented Feb 4, 2021

Also: Onion URLs should not have trailing slashes, e.g. https://github.com/freedomofpress/securedrop-https-everywhere-ruleset/pull/31/files#r570428975

@redshiftzero
Copy link
Contributor

for validating signatures in CI, referring to this logic over in HTTPS everywhere is a good starting point: https://github.com/EFForg/https-everywhere/blob/d30051e17921148a64d1f5a5c1e2ed8118916f05/chromium/background-scripts/update.js#L146-L156

@eloquence
Copy link
Member

Here's an example verification invocation that seems to work for me:

$ openssl dgst -signature rulesets-signature.1612546470.sha256  -sigopt rsa_padding_mode:pss -sigopt rsa_pss_saltlen:32  -verify public_release.pem < default.rulesets.1612546470.gz 
Verified OK

This is using the same options that are specified in https://github.com/EFForg/https-everywhere/blob/master/utils/sign-rulesets/async-airgap.sh#L19.

If that looks right to y'all, I can take a stab at adding this to CI and the README.

@conorsch
Copy link
Contributor Author

conorsch commented Feb 8, 2021

Looks great! I'm able to run locally:

$ openssl dgst -signature rulesets-signature.$(cat latest-rulesets-timestamp).sha256  -sigopt rsa_padding_mode:pss -sigopt rsa_pss_saltlen:32  -verify public_release.pem default.rulesets.$(cat latest-rulesets-timestamp).gz
Verified OK

You can optionally omit <, and just provide the filename. Detecting non-zero exit code is good enough to fail there!

@eloquence eloquence mentioned this issue Feb 9, 2021
3 tasks
@eloquence eloquence changed the title Add CI checks to sanity check rulesets Strengthen CI checks to sanity check rulesets Feb 9, 2021
@eloquence
Copy link
Member

(Retitled and checklistified to clarify remaining scope :)

@legoktm legoktm self-assigned this Sep 19, 2024
legoktm added a commit that referenced this issue Sep 19, 2024
As requested in #21, this verifies:

* generated rulesets files match the XML files. (In some sense it's just
  a reimplementation of the `upstream/merge-rulesets.py` script, but would
  stop a zero-length ruleset from being deployed)
* there are no extra nor missing signatures

Fixes #21.
@legoktm legoktm linked a pull request Sep 19, 2024 that will close this issue
2 tasks
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 a pull request may close this issue.

4 participants