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

better onion services #15666

Closed
wants to merge 2 commits into from
Closed

better onion services #15666

wants to merge 2 commits into from

Conversation

dr-bonez
Copy link
Contributor

@dr-bonez dr-bonez commented Feb 7, 2021

  • disable https for onion sites
  • select between http and https based on domain in webfinger for federation

@dr-bonez
Copy link
Contributor Author

dr-bonez commented Feb 9, 2021

Ok, this has now been tested, and it fixes federation with onion sites.

I have tested interactions from onion -> onion, onion -> clearnet, clearnet -> onion, and clearnet -> clearnet.

If you would like me to squash commits, just let me know.

@dr-bonez dr-bonez marked this pull request as ready for review February 9, 2021 02:39
Copy link
Member

@Gargron Gargron left a comment

Choose a reason for hiding this comment

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

Besides comments left, I believe that disabling e-mail notifications and the CLI changes need to be separate PRs. While the e-mail notifications can be argued about, I do not see the value of the CLI change. Why provide password via env variable when it could be provided with an option? Why special behaviour for single-user mode that makes create behave like modify?

config/initializers/1_hosts.rb Outdated Show resolved Hide resolved
config/initializers/devise.rb Outdated Show resolved Hide resolved
config/navigation.rb Outdated Show resolved Hide resolved
app/controllers/accounts_controller.rb Outdated Show resolved Hide resolved
app/controllers/accounts_controller.rb Outdated Show resolved Hide resolved
@jtracey
Copy link
Contributor

jtracey commented Feb 10, 2021

It looks like this is taking a different approach from #15560 for the first bullet. Specifically, this PR seems to explicitly mark .onion domains as not secure, while #15560 just preserves however the server is configured. IMHO, it's better not to make exceptions for .onion domains, since onion services with TLS certificates do exist (e.g., https://www.facebookcorewwwi.onion/, https://www.nytimes3xbfgragh.onion, etc.), and while they're currently rare (and largely unnecessary), there's no real reason to think that will always be the case.

@dr-bonez
Copy link
Contributor Author

Happy to split up the PRs.

@jtracey: The most important part of this PR is federation with .onion services. For that, we must make a decision whether to use http or https based only on the domain.

@dr-bonez dr-bonez marked this pull request as ready for review February 10, 2021 21:37
@dr-bonez
Copy link
Contributor Author

Alright I have addressed the comments, split out the unrelated changes so I can make separate PRs, and squashed the commits so there can be a cleaner git history. I will open the the other PRs after this one gets merged since they aren't as important to me to upstream, and they are based on this branch.

@Gargron yes, several of the changes appeared to be unnecessary. I have removed them.

@jtracey
Copy link
Contributor

jtracey commented Feb 10, 2021

For that, we must make a decision whether to use http or https based only on the domain.

I see, because webfinger URIs don't include the protocol. Ostensibly it should still be negotiated, so that HSTS can function properly, but that's sufficiently complicated that it belongs in another PR.

I still think that direct web requests should just use whatever the server is configured as, so admins can enable TLS certificates, especially for the use case of verifying that the onion address corresponds to an openweb address, which is currently the norm for all the .onion mastodon instances I know of. The HTTPS connection generally gives a cert error for the .onion right now (since LE doesn't support onion addresses yet), but that should still be a decision left to how the admin configured the site, not hard coded as plain HTTP only.

@dr-bonez
Copy link
Contributor Author

I'd be fine with putting HTTPS usage behind another env var. But, nothing about this PR prevents using HTTPS with an onion domain, it just doesn't force it. IMO the intent of forcing ssl is to make sure connections are secure, and if a connection is over .onion, it is secure, whether its on HTTPS or not. Therefore the decision on whether to force ssl should be based on the domain being .onion.
WRT webfinger, this solution is good enough forever IMO. A .onion service operator that uses HTTPS can simply expose `.well-known' over HTTP instead of redirecting without losing anything in security or UX.

@dr-bonez
Copy link
Contributor Author

closing in favor of #15560

@dr-bonez dr-bonez closed this Feb 11, 2021
@alevchuk
Copy link

Thank you! This is awsome - going to try it out. Before this we had to make these horrible patches to get HTTP to work https://github.com/alevchuk/pi-mastodon/blob/main/README.md#12-remove-https-from-mastodon

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.

None yet

4 participants