gossipd: use remote addr #5052

m-schmoock · 2022-02-23T18:22:51Z

What it does:

This is the second part of the "IP discovery by remote peer feature" that actually uses the remote_addr field to create node_annoucement updates when clightning detects that our peers report the same new remote_addr.

Currently implements the most simple algorithm I came up with that just checks:

two distinct peers
we have a channel with
report the same remote_addr
if so, we send an updated node_announcement

There is a failsafe that prevents this code from running when we have always_use_proxy enabled. Normally, this code would not be executed for TOR connections as a TOR node would not set the remote_addr to something useful other than a TOR string that we would ignore, but a nefarious TOR node may respond (thorugh the proxy) with a init TLV having an IP remote_addr set that we would otherwise use.

NextSteps/Todos:

direct probing a random node on the network to confirm a remote_addr qickly.
documentation about this: Addressed by doc: update faq and docs for IP discovery #5088
IPv4: make a self connection for confirmation. If it works we should announce as we know for sure this is the right address. Also we don't need to wait for another node to confirm that. Note: This does not apply to IPv6, as a self connection will be handeled locally (because the IPv6 address is bound locally and not at the router) and that does not tell us anything if the router has the ports forwarded or not. Also this will likely not work for a lot of routers

Questions:

Do we need a config switch for using remote_addr to send updated node_announcement?
This is decision has tradeoffs. In general it is not harmful to announce one additional address that is potentially unreachable (if the port at the router is not open). However it would affect everyone. Having a config switch increases the complexity for people using this.
Note: rusty noted we may introduce a config switch to turn that off but default to enabled.
Note: if we have a unset config switch but no announced addresses at all, we may likely also ignore the config
Note: If we have a disabled config switch, but we can make a succesful IPv4 self-connection (see above) we should ignore that config switch and send the update any way, as it will improve the situation. (not sure if

m-schmoock · 2022-03-05T12:25:29Z

just solved conflicts and rebased on master b5a1715

rustyrussell · 2022-03-07T18:32:51Z

I don't think an IPv4 self-connection is reliable either. At least with older Linux NAT setups it won't work; I'm surprised it does for you!

rustyrussell

Nice series! Some minor feedback only (and needs trivial rebase).

lightningd/peer_control.c

gossipd/gossip_generation.c

gossipd/gossipd.c

tests/test_connection.py

"announcable" is a common misspelling of "announceable", see: https://en.wiktionary.org/wiki/announcable

Changelog-None

When compiled without DEVELOPER this will now filter out `remote_addr` that come from localhost. The testcase checks for DEVELOPER to test for correct function of `remote_addr`. Also, I renamed "test_connect" to "test_connect_basic" so it can be started without all the other tests in that file that start with "test_connect..."

Adds wireaddr_eq_without_port so it can be used later. Moves wireaddr_cmp_type from connectd.c to this file, so it can be reused later.

This is the cheapest algo I came up with that simply checks that the same `remote_addr` has been report by two different peers. Can be improved in many ways: - Check by connecting to a radonm peers in the network - Check for more than two confirmations or a certain fraction - ... Changelog-Added: Send updated node_annoucement when two peers report the same remote_addr.

Increases test coverage by adding a testcase for connectd/netaddress.c Changelog-None

rustyrussell · 2022-03-11T06:12:39Z

Ack dda19f5

Thanks!

heyambob · 2022-03-19T14:37:32Z

Do we need a config switch for using remote_addr to send updated node_announcement? This is decision has tradeoffs. In general it is not harmful to announce one additional address that is potentially unreachable (if the port at the router is not open). However it would affect everyone. Having a config switch increases the complexity for people using this. Note: rusty noted we may introduce a config switch to turn that off but default to enabled.

I think it might not be a desire behavior for a lot of people due to privacy leak. I use tor + always-use-proxy=false to have some sort of privacy and better performance when peering with clearnet nodes (they should be the only ones who knows my clearnet addr from what I understand). However, when c-lightning announces my address, the entire network knows it, and sites like amboss pick it up make a public record that last forever. Even though my clearnet address is unreachable as I don't NAT, but people now easily knows my home.

It's a smart feature, but if it turns on by default, I can see node operators be in shock after update.

#5109

m-schmoock · 2022-03-21T10:22:15Z

@heyambob @rustyrussell
You might have a point here. We can discuss this in todays dev meeting on Jitsi.

Currently (without this PR), if you use a TOR but without always_use_proxy your IP address is not really a secret, because your node will still make normal connections if possible (and even prefer those over TOR). An attacker 'Charlie' can trick you into disclosing your IP address by:

Charlie opens a channel to you via a TOR connection
Charlie also announces its IP address
Charlie removes TOR from his config and restarts its node
Your node will connect to Charlie via IP

Also your node can and will do other requests that reveal your IP without always_use_proxy such as DNS seed, plugins discover prices, and so on... Though, a big difference is that only Charlie knows your IP and not everyone else.

My assumption is that 99% of the people that use TOR without always_use_proxy just do so to get past their NAT router/firewall.

heyambob · 2022-03-21T13:55:00Z

Right. My concern is for people who want to keep low profile, not for those who want to stay anonymous.

My assumption is that 99% of the people that use TOR without always_use_proxy just do so to get past their NAT router/firewall.

Initially, people play around, they might not care so much. Once they get comfortable and commit a meaningful liquidity to be a useful node, they could be regret. At least there has to be
config to turn it off.

m-schmoock · 2022-04-03T14:06:01Z

@heyambob addressed by #5136

m-schmoock force-pushed the gossip/use_remote_addr branch 7 times, most recently from f38fe5f to d56a23e Compare March 1, 2022 13:37

m-schmoock marked this pull request as ready for review March 1, 2022 14:30

m-schmoock requested a review from cdecker as a code owner March 1, 2022 14:30

m-schmoock force-pushed the gossip/use_remote_addr branch 2 times, most recently from 501f6d7 to 9edb967 Compare March 5, 2022 12:25

m-schmoock requested a review from rustyrussell March 5, 2022 21:33

rustyrussell approved these changes Mar 7, 2022

View reviewed changes

m-schmoock added 8 commits March 8, 2022 11:28

chore: fix typo announcable -> announceable

3c4a522

"announcable" is a common misspelling of "announceable", see: https://en.wiktionary.org/wiki/announcable

pyln-proto: fix port typo in example script

b23a97f

Changelog-None

wireaddr: adds wireaddr_eq_without_port and wireaddr_cmp_type

18565d6

Adds wireaddr_eq_without_port so it can be used later. Moves wireaddr_cmp_type from connectd.c to this file, so it can be reused later.

lightningd: store recently reported remote_addr

5bcfeae

pytest: IP address discovery updates node_announcement

1fd4c99

test: connectd netaddress

4dff537

Increases test coverage by adding a testcase for connectd/netaddress.c Changelog-None

m-schmoock force-pushed the gossip/use_remote_addr branch from 9edb967 to 4dff537 Compare March 8, 2022 12:00

m-schmoock added 2 commits March 9, 2022 14:28

chore: remove EXPERIMENTAL for rfc ElementsProject#917 remote_addr

a7d7b2f

lightningd: do not use remote_addr for always_use_proxy

dda19f5

m-schmoock force-pushed the gossip/use_remote_addr branch from f4ed33f to dda19f5 Compare March 9, 2022 13:28

rustyrussell merged commit db95893 into ElementsProject:master Mar 11, 2022

m-schmoock deleted the gossip/use_remote_addr branch March 11, 2022 06:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

gossipd: use remote addr #5052

gossipd: use remote addr #5052

m-schmoock commented Feb 23, 2022 •

edited

Loading

m-schmoock commented Mar 5, 2022

rustyrussell commented Mar 7, 2022

rustyrussell left a comment

rustyrussell commented Mar 11, 2022

heyambob commented Mar 19, 2022

m-schmoock commented Mar 21, 2022 •

edited

Loading

heyambob commented Mar 21, 2022

m-schmoock commented Apr 3, 2022

gossipd: use remote addr #5052

gossipd: use remote addr #5052

Conversation

m-schmoock commented Feb 23, 2022 • edited Loading

What it does:

NextSteps/Todos:

Questions:

m-schmoock commented Mar 5, 2022

rustyrussell commented Mar 7, 2022

rustyrussell left a comment

Choose a reason for hiding this comment

rustyrussell commented Mar 11, 2022

heyambob commented Mar 19, 2022

m-schmoock commented Mar 21, 2022 • edited Loading

heyambob commented Mar 21, 2022

m-schmoock commented Apr 3, 2022

m-schmoock commented Feb 23, 2022 •

edited

Loading

m-schmoock commented Mar 21, 2022 •

edited

Loading