BOLT 1: adds remote address to optional init_tlvs (IP discovery)

[m-schmoock]

This is an alternate approach to #911 (BOLT 7: add gossip address descriptor type DNS hostname)
relying less on centralized services (DNS, IP query sites, ...) to solve the issue
of detecting a nodes own public IPv4 behind a NAT.

This adds the option to report an remote IP address back to a connecting
peer using the init message. A node can decide to use that information
to discover a potential update to its public IPv4 address (NAT) and use
that for a node_announcement update message containg the new address.

The proposal includes reporting the IPv4 and IPv6 address,
however in IPv6 there are likely no NAT issues. TOR is skipped for
obvious reasons.

Certain appoaches to check and use this information are thinkable:

• Wait for multiple peers or a certiain fraction to report the
  same new address.
• Check some random node known via gossip to also report the new
  address by making a test connection just for that purpose.
• Verify this information by making a test connection to itself (can work, must not work).

[t-bast]

I like that better than #911, it's very easy for nodes to start filling this field soon.

[t-bast]

Concept ACK, I'll need to play a bit with a prototype implementation

[Roasbeef]

Is this actually practically an alternative to #911?

With the DNS based one, peers are able to easily query for the latest IPs related to a node and update them in their local table. It also makes configuration of a "fleet" of nodes easier, as a dynamic DNS record can be used to point to the latest set of "healthy/active" nodes.

With this, AFAIK, it only actually happens on the initial handshake, so a node would continually need to make new connection in order to track a possibly dynamic IP.

[m-schmoock]

Update: Slightly changed the wording from:

  - SHOULD set `remote_addr` to reflect the public IP address of an incoming
    connection, if the node is the receiver and the connection was done via IP.
    IP addresses that are within private networks (RFC-1918) must not be used.

To:

  - SHOULD set `remote_addr` to reflect the remote IP address (and port) of an
    incoming connection, if the node is the receiver and the connection was done
    via IP. IP addresses within private networks (RFC-1918) MUST not be used.

[m-schmoock]

Also someone mentioned that this can be useful also for TOR connection, maybe someone can elaborate on that...

[TheBlueMatt]

Also someone mentioned that this can be useful also for TOR connection, maybe someone can elaborate on that...

Yea, its possible, but probably mostly not tbh. more of a "user didn't bother configuring things fully but did configure things partially" thing, so maybe not a huge value, but could be useful, I guess - if the user gets an onion address, but doesn't actually tell their lightning node, and then they tell someone else to connect to them, their lightning node may "learn" their onion address". Though this does have some MITM concerns so it may not be the best way to go about it.

[m-schmoock]

Maybe we should change the wording regarding private networks, since this includes more RFCs than just 1918, for example RFC 4862, RFC 5737, ...

I added the checks c-lightning used for other purposes, which is:

static bool IsRoutable(const struct wireaddr *addr)
{
    return IsValid(addr) && !(IsRFC1918(addr) || IsRFC2544(addr) || IsRFC3927(addr) || IsRFC4862(addr) || IsRFC6598(addr) || IsRFC5737(addr) || (IsRFC4193(addr) && !IsTor(addr)) || IsRFC4843(addr) || IsLocal(addr) || IsInternal(addr));
}

Maybe change it the other way around.

Only public addresses MUST be used.

[m-schmoock]

@t-bast What do you think on rewording the usage of private IPs part to i.e. "Only public addresses MUST be used." (see comment above). Or are we fine just mentioning RFC 1918 specifically?

[t-bast]

I'm fine with Only public addresses MUST be used., but I'm definitely not the expert here, I don't know if @TheBlueMatt or @rustyrussell want to be more explicit than that.

[m-schmoock]

@t-bast I updated our branch ElementsProject/lightning#4864
It does what its supposed to do now, still not merged. Currently only logs the remote_addr.
When it gets merged it will still have one round round of EXPERIMENTAL feature before getting in regularly.

I have several ideas on how to use that information to forge updated node_announcements.
Maybe we do this in another PR of our code, as reading and sending this information is the
minimum viable product for this ;)

[t-bast]

ACK 5e08475

I have tested cross-compatibility between eclair (ACINQ/eclair#1973) and c-lightning (ElementsProject/lightning#4864) so this should be ready to go.

[m-schmoock]

@t-bast
I think we should re-add the private address note, but not strict like this: Only public addresses SHOULD be used. It could expose internal information not intended to be send. This leaves the option to use it private or local addresses for testing code etc. If your okay with that I can change that.

[t-bast]

Can you explain how it could expose internal information?
Otherwise I'm fine with letting the receiver of the remote_addr filter out private addresses if they're only interested in public ones.

[m-schmoock]

Yes I'm fine letting the receiver filter out private addresses. And the way I understand home NATed networks this should not even happen. However, Rusty noted ElementsProject/lightning#4864 (comment) in our PR. I think he has a point here. If a node software implementation sees a connection from a private IP (for whatever wired network setup reasons), he 'SHOULD NOT' disclose this as this information is useless at best, but potential tells an attacker about internal network setup.

Not sure if there are routers that do address mapping (not only port mapping like normal NATs do).

[t-bast]

Good point, we don't want to reveal internal network details...it's probably safer then to filter out private addresses on both sides.

[m-schmoock]

The only thing I just added: SHOULD NOT set private addresses as 'remote_addr'.

[t-bast]

ACK efe9d2d

[vincenzopalazzo]

Concept ACK efe9d2d

[m-schmoock]

@t-bast
Another thing: The source port of a TCP connection is dynamically assigned. With this PR we report this dynamic port back to the remote peer. This is almost never the actual listening port of the peer.

Using remote_addr feature to update node_announcement will have to make assumptions i.e. that the public listening port is the same as the local listening port or the protocol default 9375. In some circumstances the node can actually test that. Some nodes maybe will do this by 'best guess'.

My question: Do we still want to include the (source) port to have this field similar to the address descriptor types of bolt 7?

Edit: I would say yes, include (source) port and use address descriptor format for the sake of simplicity

[t-bast]

The source port of a TCP connection is dynamically assigned. With this PR we report this dynamic port back to the remote peer. This is almost never the actual listening port of the peer.

Yes, I don't think this is an issue. The remote peer knows that this is clearly not their listening port, they're only interested in the IP address part (the port should be unchanged compared to their previous announcement, unless the node operator changed it).

It's true that we could in that case drop the port from the tlv, which would completely remove the risk of a mistake...I don't care that much, both options look good to me, so it's your call!

[vincenzopalazzo]

It's true that we could in that case drop the port from the tlv, which would completely remove the risk of a mistake...I don't care that much, both options look good to me, so it's your call!

This option sound safer to avoid confusion for who is reading the spec, so I agree with your point here

[m-schmoock]

I tend to include the (source) port as it currently is for two reasons:

• it's simpler to re-use the data-structure we have from bolt7
• it can be used by the software to detect if there was a NAT in between (it changed the port)

[m-schmoock]

Hey @t-bast

Just a little update on this, as its now out in the wild for cln and acinq.
I noted that ACINQs main node 03864ef025fde8fb587d989186ce6a4a186895ee44a926bfc370e2c366597a3f8f seem to report 13.248.122.1/24 and 13.248.123.1/24 addresses as remote_addr to the peers. I'm quite sure that this is some DPI/cloudflare reverse IP proxy stuff going on on your side. Can you confirm that?

I think I need to change our code to handle remote_addr better. The current algorithm is quite unpolished. It just waits for two or more peers (with a channel) to report the same public IP. Does your code make use of this field yet (besides sending it to the peer)? Any idea/suggestions?

[t-bast]

I noted that ACINQs main node 03864ef025fde8fb587d989186ce6a4a186895ee44a926bfc370e2c366597a3f8f seem to report 13.248.122.1/24 and 13.248.123.1/24 addresses as remote_addr to the peers. I'm quite sure that this is some DPI/cloudflare reverse IP proxy stuff going on on your side. Can you confirm that?

This is because of our clustering mode (see https://github.com/ACINQ/eclair/releases/tag/v0.5.0 for details). Our node splits the work between front-end nodes that handle connection logic / routing stuff and a back-end node that does channel operations. What you see are the front-end nodes, and there is indeed some load-balancing that's done automatically between them. But any of these IP addresses would work!

Does your code make use of this field yet (besides sending it to the peer)? Any idea/suggestions?

Not yet, we just send it for now. We're collecting data about it, and we'll later analyze that data to figure out whether we can use this reliably or not.

[m-schmoock]

Thanks for the clarification.

But any of these IP addresses would work

Afaik in terms of IP discovery for the peers they can't work. They just tell us something about ACINQ internals ;)

[m-schmoock]

@t-bast
Maybe you want to disable sending acinq frontend IPs as remote_addr back to the peers. Can the code detect if its handled by an frontend, if so not fill in that field or maybe let the frontend fill/overwrite that field with the real public IP of the peer?

[t-bast]

Right, I misunderstood your comment, I thought that was already handled by our frontend! It should be, it's a bug on our side then, thanks for the report.