-
Notifications
You must be signed in to change notification settings - Fork 3
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
Creates scripts for regenerating rulesets #23
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @conorsch, this is very helpful, tested these changes locally, some comments inline. We should also update this repos readme prior to merge
scripts/generate-and-sign
Outdated
sd_rules_dir="securedrop-rules" | ||
rm -rf "$sd_rules_dir" | ||
mkdir "$sd_rules_dir" | ||
docker run -it -v "$(pwd):/opt" --workdir /opt python:3.6 python3 utils/merge-rulesets.py |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a specific reason to use Docker here? In the past, this script worked well with system provided python3
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, happy to remove. For the record, that line was taken verbatim from https://github.com/EFForg/https-everywhere/blob/master/docs/en_US/ruleset-update-channels.md#signing, but I agree we can just use python3 on the host.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
docker run -it -v "$(pwd):/opt" --workdir /opt python:3.6 utils/sign-rulesets/async-request.sh public_release.pem "$sd_rules_dir" | ||
|
||
echo "Finished. Review files in ${https_everywhere_repo}/${sd_rules_dir}/" | ||
cp -v "${https_everywhere_repo}/${sd_rules_dir}/"* . |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there's one last step (that is always forgotten): update_index.sh which will update index.html
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aye, I've forgotten that one too. Opened #21 so CI nags us about it, but let's just add it to the script to avoid in the future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
|
||
|
||
# We need the upstream repo by EFF for a few select scripts. | ||
https_everywhere_repo="https-everywhere" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this won't update/pull latest changes to the ruleset merge logic and the signing script, but i think it's fine given our test plan usually consists of the reviewer adding a new channel in the browser.
docker run -it -v "$(pwd):/opt" --workdir /opt python:3.6 utils/sign-rulesets/async-request.sh public_release.pem "$sd_rules_dir" | ||
|
||
echo "Finished. Review files in ${https_everywhere_repo}/${sd_rules_dir}/" | ||
cp -v "${https_everywhere_repo}/${sd_rules_dir}/"* . |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: git adding the files could potentially be useful here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should have been more explicit: I agree with you that adding would be helpful, but adding via glob right now may introduce problems given #20
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added some helpful messaging to the script with instructions on next steps. Not automatically 'git add'ing due to #20, but still an improvement, thanks for recommending.
Discussed at standup today. We'll likely have another onion name request finalized soon, but let's not block on review here. I'll add the changes @emkll requested, then mark as ready. |
50b258e
to
65a2f0c
Compare
@emkll ready for re-review. |
Used the script in [0] to regenerate all the rules, including the sig. Looks good. [0] freedomofpress#23
Trying to bottle up the humdrum tasks into a single action, as far as possible. Some more guiding language about the specific steps of the airgap procedure would be welcome, but likely best handled in separate documentation.
Based on feedback during review. * Don't use docker, just use system python * Rebuild index.html * Instruct user to commit changes after local review That's it for now. We can automatically 'git add' files once we have the ruleset generation sorted out wrt #20.
65a2f0c
to
72ee7df
Compare
Used during #25, and pleased with the results. Rebased on latest main. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @conorsch , looks good to me. I've appended a commit to update the README based on the changes introduced here.
Status
Ready for review
Refs #22
Trying to bottle up the humdrum tasks into a single action, as
far as possible. Some more guiding language about the specific steps of
the airgap procedure would be welcome, but likely best handled in
separate documentation.
Review Checklist
onboarded.txt
are accuratedefault.rulesets.TIMESTAMP.gz
has been updated, extracting that file and inspecting the contents of the JSON file produces the expected rulesPath Prefix
:https://raw.githubusercontent.com/freedomofpress/securedrop-https-everywhere-ruleset/$BRANCH_NAME
index.html
has been updated using./update_index.sh