-
Notifications
You must be signed in to change notification settings - Fork 64
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
Adds Tor support to cln-plugin #96
Conversation
@CMDRZOD feel free to give this a go, it should fix your issue with connecting to the tower via |
Also tagging you @tee8z since you wrote the original piece of code. Can you give it a try? |
I gave this PR branch a test, seems I am getting "connection refused" when I try to connect from the plugin but a Tor call works with tor-sock from command line so the watchtower is accessible from the onion address. Attached is what I saw when testing (let me know if you see something weird in the logs, and I can also gives this another try later to make sure it wasn't something I configured incorrectly). |
@tee8z actually got this same error but fixed it (locally at least) before pushing the last rebase 🤔 Are you testing this with both the client and the tower on the same host? What does your setup look like? |
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.
watchtower-plugin/src/net/http.rs
Outdated
} | ||
}) | ||
let client = if endpoint.contains(".onion:") { | ||
let proxy = reqwest::Proxy::http("socks5h://127.0.0.1:9050") |
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.
Can't we ask cln to give us the SOCKS proxy host and port instead?
Or maybe provide them as options with 127.0.0.1:9050
as the default.
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'm working on it with the second approach, but it becomes a bit more complex.
Thought about asking cln, but that'd mean that if cln is using Tor
the plugin always will, and if it is not, the plugin would never. I think we may want to give the user the option to pick any combination.
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.
Fixed in dcbcee3. Not completely convinced it is the most elegant way though.
@tee8z Are you sure you rebuilt the cln plugin before running |
a6be333
to
dcbcee3
Compare
Fixed the related comments. I'm still curious about the error @tee8z was facing. Would love to learn more about it given I cannot reproduce it. I used to face the exact same issue, which was coming from the proxy not being able to resolve DNS. I fixed it by replacing rust-teos/watchtower-plugin/src/net/http.rs Line 134 in dcbcee3
|
Looks like there are two tests failing, given we used to flag malformed urls as |
Dunno what cases now trigger the |
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.
LGTM.
One thing I did think of though, is to have 127.0.0.1:9050
be the default proxy (we won't have None
s in this case). But actually both techniques are perfectly fine.
The default should be to have no proxy at all, shouldn't it? Otherwise, you'd be trying to redirect your traffic to an inexistent proxy. |
This needs rebasing all commits into a single one. Will wait for @tee8z review before doing so and potentially merging. |
I'm sure this is something with my setup, but I'm still struggling to get the plugin to work with Tor, new error message though!
I'm going to keep trying to get this over the coming days, but don't let me hold up merging this in as it's probably specific to my environment. |
@tee8z I added a standalone option for the plugin Try replacing |
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.
LGTM
Nice! I think I'm going to improve this a bit and use |
Ok, nvm, this will need to be added in a follow-up given the native I'm documenting the improvements that we can land after the next |
Check if the tower address is an onion address and proxies it through Tor if so.
This assumes Tor is running on the backend, which is the same assumption used by CoreLN AFAICT https://lightning.readthedocs.io/TOR.html
Close #92 #93