-
Notifications
You must be signed in to change notification settings - Fork 65
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
Improves tor flag logic in watchtower-plugin #160
Conversation
e4a9e7b
to
3be8ccb
Compare
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.
Some early review/suggestions.
watchtower-plugin/src/net/http.rs
Outdated
let client = if url.host_str().unwrap().ends_with(".onion") { | ||
if let Some(proxy) = proxy { | ||
let proxy = reqwest::Proxy::http(format!("socks5h://{}", proxy)) | ||
let proxy = reqwest::Proxy::http(proxy.get_socks_addr()) | ||
.map_err(|e| RequestError::ConnectionError(format!("{}", e)))?; | ||
reqwest::Client::builder() | ||
.proxy(proxy) |
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.
So this checks if the host is a .onion
url and uses the proxy for it.
But we don't make use of the always_use_proxy
option flag that CLN provides. We should prioritize that first, i.e.: if always_use_proxy
flag is set and the url isn't a .onion
we should still use the proxy, as not using it in such a case will expose the node's IP address.
Something along the lines of:
if let Some(proxy) = proxy {
if proxy.always_use || url.is_onion() {
// construct a proxied client.
} else {
// construct a non-proxied client.
}
} else {
if url.is_onion() {
return Err("url is onion but no proxy")
}
// construct a non-proxid client.
}
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.
This should be covered now, alongside some minor improvements.
I've added an endpoint param to some of the http functions. A followup would be nice defining the tower endpoints as an enum and replacing all string endpoint with the proper variant (I'm already working on this).
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
The `watchtower-plugin` was specifying a custom tor flag to signal whether Tor may be used by the client. This was due to `cln-plugin (v0.1.1-)` not allowing plugins to access the CoreLN configuration options and, therefore, our plugin was unable to fetch the `proxy` / `always-use-proxy` options. This fetches the aforementioned options and revamps the logic to comply with the `always-use-proxy` requirements, that is, if the flag is set all communications must be performed using Tor. Also, it replaces some of the currently used `String`s for more meaningful types to store network data (such as `AddressType`, `NetAddress`, or `ProxyInfo`). This drops our custom `watchtower-proxy` config option
Rebased to squash all changes |
Changes
The
watchtower-plugin
was specifying a custom tor flag to signal whether Tor may be used by the client. This was due tocln-plugin (v0.1.1-)
not allowing plugins to access the CoreLN configuration options and, therefore, our plugin was unable to fetch theproxy
/always-use-proxy
options.This fetches the aforementioned options and revamps the logic to comply with the
always-use-proxy
requirements, that is, if the flag is set all communications must be performed using Tor. Also, it replaces some of the currently usedString
s for more meaningful types to store network data (such asAddressType
,NetAddress
, orProxyInfo
).This drops our custom
watchtower-proxy
config option