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

Turn off SSL when custom Electrum server address is a hidden service #1278

Merged
merged 3 commits into from Jan 29, 2020

Conversation

@CandleHater
Copy link
Contributor

CandleHater commented Jan 15, 2020

It would be great if I could connect the Eclair mobile wallet to my Tor only electrs server using Orbot. Sadly I can't because it requires a valid SSL certificate. Please make an exception if the custom Electrum server address is an onion domain.

As suggested from others in my initial PR ACINQ/eclair-mobile#229, I created this PR to cover up eclair-core and not only eclair-mobile.

It would be great if I could connect the Eclair mobile wallet to my Tor only electrs server using Orbot. Sadly I can't because it requires a valid SSL certificate. Please make an exception if the custom Electrum server address is an onion domain.

As suggested from others in my initial PR ACINQ/eclair-mobile#229, I created this PR to cover up eclair-core and not only eclair-mobile.
Copy link
Member

t-bast left a comment

Thanks! Added a code suggestion that fixes the build (Scala doesn't like mutable values).

As suggested by t-blast in #1278 (comment) I rewrote the implementation to fix the building process.
@CandleHater

This comment has been minimized.

Copy link
Contributor Author

CandleHater commented Jan 15, 2020

Thank you, I missed that. Made a new commit to address this issue.

@CandleHater CandleHater requested a review from t-bast Jan 15, 2020
Copy link
Member

t-bast left a comment

LGTM, thanks!

@CandleHater CandleHater removed the request for review from dpad85 Jan 15, 2020
Copy link
Member

pm47 left a comment

I guess we just need the result of the E2E test.

As suggested in #1278 (comment) I've rewritten the SSL related part to be more clean, compact and easier to read.
@CandleHater CandleHater dismissed stale reviews from pm47 and t-bast via 233bcda Jan 21, 2020
@t-bast
t-bast approved these changes Jan 21, 2020
@pm47
pm47 approved these changes Jan 21, 2020
@sstone
sstone approved these changes Jan 28, 2020
Copy link
Member

sstone left a comment

I agree that using SSL with TOR is not needed so this PR, which makes electrum client use plain tcp when connecting to a custom Electrum server behind TOR, makes sense.

However, it also means that users will NOT be able to use custom servers that only expose their SSL port on TOR (that is what most .onion public Electrum servers do) so it will have to be documented.

I've hacked our electrum client, did some testing with Orbot and "it works on my Phone" so we're good to go (but still need to add proper socks5 proxy support).

@sstone sstone merged commit 16456bb into ACINQ:master Jan 29, 2020
2 checks passed
2 checks passed
ci/semaphoreci/pr: Build & Test The build passed on Semaphore 2.0.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
case "off" => SSL.OFF
case "loose" => SSL.LOOSE
case _ => SSL.STRICT // strict mode is the default when we specify a custom electrum server, we don't want to be MITMed
case _ if address.getHostName.endsWith(".onion") => SSL.OFF // Tor already adds end-to-end encryption, adding TLS on top doesn't add anything

This comment has been minimized.

Copy link
@Kixunil

Kixunil Feb 13, 2020

Shouldn't this be conditioned on !"strict"?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.