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

Add nginx container to serve ruleset files #12

Merged
merged 5 commits into from
Oct 1, 2020
Merged

Add nginx container to serve ruleset files #12

merged 5 commits into from
Oct 1, 2020

Conversation

maeve-fpf
Copy link
Contributor

This adds a very basic container image definition to serve the ruleset files with Nginx. They're served on /https-everywhere/; / will return a 403.

I chose a port arbitrarily; this is intended to be deployed behind a routing layer of some sort (e.g. k8s ingress) that only forwards requests matching a path to this backend.

Status

Ready for review

Review Checklist

  • No changes to onboarded.txt
  • No changes to default.rulesets.TIMESTAMP.gz -- inspecting the contents of the JSON file produces the expected rules
  • The ruleset has been verified by modifying the HTTPS Everywhere configuration in a Tor Browser instance pointing to Path Prefix: https://raw.githubusercontent.com/freedomofpress/securedrop-https-everywhere-ruleset/$BRANCH_NAME -- I added the HTTPS Everywhere extension to Tor Browser, updated the Path Prefix for the existing entry, visited a site in the ruleset, nothing seems obviously broken.
  • Running ./update_index.sh produces no changes, as expected

@echoesactiii
Copy link

/ will return a 403.

Where does this 403 come from? The nginx config in this PR doesn't seem as though it will do that.

@maeve-fpf
Copy link
Contributor Author

@ketudb there's no index.html in /opt/nginx/root directly, and we only enable index, not autoindex.

@echoesactiii
Copy link

@ketudb there's no index.html in /opt/nginx/root directly, and we only enable index, not autoindex.

Indeed that's true. I misread and thought the https-everywhere/ folder was meant to return a 403. My bad!

echoesactiii
echoesactiii previously approved these changes Sep 30, 2020
Copy link

@echoesactiii echoesactiii 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!

emkll
emkll previously approved these changes Sep 30, 2020
Copy link
Contributor

@emkll emkll left a comment

Choose a reason for hiding this comment

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

Thanks @maeve-fpf and @ketudb for the review, changes here look good to me.

If understand correctly (please correct me if I'm wrong), with these changes, once migrated to the new platform, and when changes are merge into main, a container will be automatically built and deployed. (details, like the timestamp docker arg will be handled by CI/CD platform). To perform a rollback, we just need to revert the commit in main, and a new container will be built (with the rolled-back rules) and deployed.

Perhaps adding quick instructions on how to locally test upstream nginx changes (using docker commands, since the upstream hashed is pinned in the Dockerfile) could be useful, but also does not need to block merge.

@maeve-fpf maeve-fpf dismissed stale reviews from emkll and echoesactiii via 7472e9a September 30, 2020 18:26
@maeve-fpf
Copy link
Contributor Author

That's correct! (It is not, however, set up yet.) I've added a make serve rule and some notes.

Copy link
Contributor

@emkll emkll left a comment

Choose a reason for hiding this comment

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

Thanks for adding docs and Makefile target, @maeve-fpf ! Ran into two minor issues with the Docker commands in my local environment (see inline)

Makefile Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
docker/Dockerfile Outdated Show resolved Hide resolved
Copy link
Contributor

@emkll emkll left a comment

Choose a reason for hiding this comment

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

thanks @maeve-fpf !

@emkll emkll merged commit cb9ec59 into main Oct 1, 2020
@emkll emkll deleted the nginx-container branch October 1, 2020 15:04
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.

3 participants