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

Onion location header #695

Merged
merged 3 commits into from
Jun 24, 2020
Merged

Onion location header #695

merged 3 commits into from
Jun 24, 2020

Conversation

chigby
Copy link
Contributor

@chigby chigby commented Jun 23, 2020

This pull request adds a piece of middleware to set the Onion-Location header based on its value in the site's FooterSettings. The https protocol is added to the URL if it's not present, though it's not a foolproof URL transformation. I mostly looked at what is currently there on the production site, and asked "What's the simplest possible way to transform this into the required format for the onion location header?"

If the actual protocol is http, let me know and I can change it.

Fixes #693

@chigby chigby requested a review from harrislapiroff as a code owner June 23, 2020 20:46
@chigby chigby force-pushed the onion-location-header branch from 8074596 to fe286e5 Compare June 23, 2020 20:54
@eloquence
Copy link
Member

We do support HTTPS for securedrop.org (it was only recently re-enabled), so that should work. Onion services provide their own encryption layer, though for v2 services, HTTPS provides useful defense in depth.

@conorsch
Copy link
Contributor

The changes here force HTTPS on any Onion URL, which isn't what we want—the v3 URLs won't have HTTPS, for instance, at least not as presently planned. While we could add HTTPS to the v3 URL at some later point, I don't think the middleware should force it, it should honor the spec and permit either http or https:

The Onion-Location value must be a valid URL with http: or https: protocol and a .onion hostname.

So in terms of validating what's been put into the field by a user of the Wagtail admin, sanity checking that either http or https was prepended, and that a suffix of .onion exists, seems like a good idea. We also have the ability to validate that the URL is actually a v2 or v3 onion url via regex, but that seems more complicated than it's worth.

@chigby chigby force-pushed the onion-location-header branch from 8309829 to 0f3ac65 Compare June 24, 2020 16:28
@chigby
Copy link
Contributor Author

chigby commented Jun 24, 2020

Thanks for the insights, everyone. I've reworked the middleware to use the address as-entered, and validation to ensure it's correctly formatted for use in the header. I think this means it will need to be updated from its current value to have the correct "http" or "https" protocol, as right now it doesn't look like it has any. Though this may have to wait until this PR is merged since, as far as I can tell, there's no way currently in the admin UI to actually modify this value.

chigby added 3 commits June 24, 2020 12:31
This adds a new piece of middleware for the single purpose of adding
the Onion-Location header from the site's footer settings, if it
exists.  There's some small logic to ensure that the header value has
a https protocol if what's stored in the settings does not have an
accompanying protocol.
Validate onion addresses as though they are URLs (which they are)
requiring a .onion domain.

Also provide a way to edit them!
@chigby chigby force-pushed the onion-location-header branch from 0f3ac65 to e9a1ada Compare June 24, 2020 16:31
Copy link
Contributor

@harrislapiroff harrislapiroff left a comment

Choose a reason for hiding this comment

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

Confirmed this works with ngrok! As @chigby flagged, this will require updating the settings in the admin (under "Footer Settings") to include a protocol in the onion url when deployed.

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.

Set Onion-Location header
4 participants