Add Tor support for outbound connections via SOCKS #778
Conversation
|
I've assigned @tnull as a reviewer! |
| .next() | ||
| .ok_or_else(|| { | ||
| log_error!(self.logger, "Failed to resolve network address {}", addr); | ||
| let res = if let SocketAddress::OnionV2(old_onion_addr) = addr { |
There was a problem hiding this comment.
nit: did you consider using a match statement here?
| self.propagate_result_to_subscribers(&node_id, Err(Error::InvalidSocketAddress)); | ||
| Error::InvalidSocketAddress | ||
| })?; | ||
| let connection_future = lightning_net_tokio::tor_connect_outbound( |
There was a problem hiding this comment.
Do we want to allow people to proxy the other types over tor as well ? Have to be careful with adding more settings, but I'm thinking of an analog to the always-use-proxy=true CLN setting.
Could be done in a follow-up if preferred.
There was a problem hiding this comment.
Good point, that would be a useful setting...
I'm open to add that on to this PR, would need another cfg setting I suppose.
| let connection_manager = Arc::new(ConnectionManager::new( | ||
| Arc::clone(&peer_manager), | ||
| config.tor_proxy_address.clone(), | ||
| ephemeral_bytes, |
There was a problem hiding this comment.
This will be used to derive passwords sent in plaintext to the SOCKS5 proxy, so out of caution I would have passed a seed here that is different from the seed passed to the PeerManager to derive BOLT 8 per-connection ephemeral keys.
The KeysManager entropy source is also a chacha20 so seems to me we can cheaply do another keys_manager.get_secure_random_bytes();
| node_id, | ||
| addr.clone(), | ||
| proxy_addr, | ||
| self.tor_proxy_rng.clone(), |
There was a problem hiding this comment.
nit: Would have used Arc::clone(&self.tor_proxy_rng) here, same style as the peer manager above, just to make it very clear we are only creating a new pointer to the same state, and not cloning the state itself.
3 participants
Addresses #178 . Builds on lightningdevkit/rust-lightning#4305 .
As mentioned there, I see two drawbacks with this patch as is:
https://github.com/lightningdevkit/rust-lightning/blob/f9ad3450b7d8b722b440f0a5e3d9be8bd7a696ae/lightning-net-tokio/src/lib.rs#L332
Which affects
list_peers()output:{ "pubkey": { "pubkey": "03864ef025fde8fb587d989186ce6a4a186895ee44a926bfc370e2c366597a3f8f" }, "socket_addr": { "address": "3.33.236.230:9735" }, "is_connected": true }, { "pubkey": { "pubkey": "03fe47fdfea0f25fad0013498e8d6cec348ae3d673841ec25ee94f87c21af16ed8" }, "socket_addr": { "address": "127.0.0.1:9050" }, "is_connected": true }But more significantly, AFAICT affects our setup messages with our peer:
https://github.com/lightningdevkit/rust-lightning/blob/f9ad3450b7d8b722b440f0a5e3d9be8bd7a696ae/lightning/src/ln/peer_handler.rs#L1905
The
filter_addresses()call here means we won't include the proxy address, but we also won't include that peer's onion address, which we may want to do?I'm testing this actively now and haven't hit any issues yet, though I haven't had much traffic with that peer either.