ElementsProject / lightning

Core Lightning — Lightning Network implementation focusing on spec compliance and performance
Other
2.85k stars 902 forks source link

announce-addr: address 'dns:port' is not announceable #5657

Closed NicolasDorier closed 1 year ago

NicolasDorier commented 2 years ago

Trying to update c-lightning from 0.10.2 to 0.12.1

Get this error during start:

lightningd: /root/.lightning/config line 9: announce-addr: address 'btcpay763334.lndyn.com:9735' is not announceable

My config is the following

bitcoin-datadir=/etc/bitcoin
bitcoin-rpcconnect=bitcoind
experimental-offers

proxy=tor:9050

bind-addr=0.0.0.0:9735
network=bitcoin
announce-addr=btcpay763334.lndyn.com:9735
announce-addr=uewlcjmuuwcgs43lpcc7d64k2pf4xs7yyer7ztsq6cblyjxue3bc35qd.onion:9735
rpc-file=/root/.lightning/lightning-rpc

What should I do instead?

NicolasDorier commented 2 years ago

In our unit tests, clightning doesn't work either /root/.lightning/config line 4: announce-addr: address 'lightningd_dest' is not announceable

whitslack commented 2 years ago

Indeed, they broke it at some point. I used to have DNS names in my announce-addr= config lines, but they stopped working, and I had to start hard-coding IP addresses there, which is brittle.

Supposedly there's been defined a new node announcement type that allows gossiping DNS names in addition to IP addresses. Looking forward to that.

NicolasDorier commented 2 years ago

That's nice feature, but for the time being that would be nice to have a work around as we can't update :(

dennisreimann commented 2 years ago

Any ideas, @rustyrussell?

cdecker commented 2 years ago

That is correct, we had to disable automatic resolution, to allow non-resolved hostnames to be added and announced. If we were to resolve on our end we'd replace what is supposed to be a dynamic address that changes with DNS into a static IP that'll not work when the address changes (ISP renewing lease).

I'm sure @m-schmoock can explain it in detail, but the tldr is that some implementations didn't like us announcing the new address type (DNS symbolic names) and were dropping our node_announcements so we had to temporarily disable it.

rustyrussell commented 2 years ago

This is a regression, I had not appreciated that we would now treat names as literals to announce.

Until DNS-name address records become widely supported, we should always turn it to IP address unless overridden somehow!

NicolasDorier commented 2 years ago

Before DNS name gossip, if I remember, the DNS name were resolved into IP and those were what got gossiped.

I understand the problem with ISP dynamic ip, so I am happy about DNS announcement, it's shame it can't be enabled because of other implementations for now. But rather than disabling it, you should probably fallback to IP announcement for those hosts like before.

NicolasDorier commented 2 years ago

If there is a patch to restore previous behavior, I would be happy to just take it and use it on our distrib, so at least we can up to date with latest clightning versions.

m-schmoock commented 2 years ago

I'm sure @m-schmoock can explain it in detail, but the tldr is that some implementations didn't like us announcing the new address type (DNS symbolic names) and were dropping our node_announcements so we had to temporarily disable it.

This LND issue has been resolved, and on current master the gossip announcement of type DNS is already non-experimental anymore. ( e0d6f3ceb connectd: DNS Bolt7 #911 no longer EXPERIMENTAL )

NicolasDorier commented 1 year ago

Still nothing here? please we have many people who can't update c-lightning because of this. :(

NicolasDorier commented 1 year ago

Forget about it, we will just remove announce-addr for all of our users.

NicolasDorier commented 1 year ago

Actually, we can't remove announce-addr as our unit tests depends on it too much, and BTCPay Server also depends on it to fetch the node information to show to the customers in the checkout page. We are stuck until you fix it.

m-schmoock commented 1 year ago

Okay @NicolasDorier I looked into it briefly.

Version 0.12.1 still has DNS support experimental, thus the is not announceable. The next release will actually announce a DNS address descriptor type 5 (without resolving it locally). If compiled with experimental features, this error message should be gone already.

What older version of clightning did was to try to resolve a DNS name these IPs would have then been announced once statically (without checking for changes ever, this is another topic).

Maybe we can do the following to 'fix' the breakage: If a DNS name is given, announce it as an DNS name AND ALSO, if it resolves to a local interface, the statically old fashioned way.

In our unit tests, clightning doesn't work either /root/.lightning/config line 4: announce-addr: address 'lightningd_dest' is not announceable

In your quote above I wonder what the is 'lightningd_dest' (DNS?) is supposed to be, is this some alias from the test framework? In any case that would currently be treated as a possibly valid DNS name.

dennisreimann commented 1 year ago

In your quote above I wonder what the is 'lightningd_dest' (DNS?) is supposed to be

It's the Docker hostname of the Core Lightning container in our test setup.

m-schmoock commented 1 year ago

@cdecker @rustyrussell I did some mainnet testing to see if the network is finally able to propagate DNS names in node_announcements. Turns out, it still doesn't. Thing is, the experimental flags have been removed on master already, this would mean the feature would go live with the next release.

Anyone using DNS names will effectively make their node disapear, as node_announcements that contain regular IP/Tor entries along with DNS names will be dropped by vast parts of the network.

m-schmoock commented 1 year ago

We could re-experimentalize including DNS names in node_announcemt and also add old fashined IP entries for if DNS hostname has been used for announce-addr. Then we can wait a couple of month to see if the network finally gets compatible with this and re-evaluate this decision

m-schmoock commented 1 year ago

Try to kick off some discussion about this in https://github.com/ElementsProject/lightning/issues/5657

whitslack commented 1 year ago

Wouldn't it make sense to construct and sign two node announcements: one containing the DNS names and the other containing the addresses to which those names resolve? Send out both announcements. Peers that don't support DNS names will drop the announcement containing them and propagate the other one. Nodes that support the names will replace the announcement containing addresses.

m-schmoock commented 1 year ago

Interesting thought. But one is not supposed to send out gossip too quickly. This applies to channel and node announcements. Otherwise on gets throttled (and again unvisible).

Also the recent update that's parseable/readable invalidates the last one. So then only the DNS name would be seen, which is not nice when you maybe also want to announce a TOR address (for connectivity).

whitslack commented 1 year ago

So alternate which one you're sending every day or whatever the period is. And include your Tor address in all of your announcements. The IP-containing announcements that you send should always have serial numbers / expiration times that are less than that of the name-containing announcement you most recently sent, so that nodes that support name-containing announcements will drop your no-name announcements on the spot as stale.

NicolasDorier commented 1 year ago

Note that actually we aren't using announce-addr for announcing on the p2p network. We are using it for showing the host part of the node info to customers of merchant wishing to open a direct channel.

m-schmoock commented 1 year ago

@NicolasDorier

Note that actually we aren't using announce-addr for announcing on the p2p network. We are using it for showing the host part of the node info to customers of merchant wishing to open a direct channel.

Can you elaborate on this? Do you mean you want to use it so getinfo returns something different on the address part?

Also, check out recent version 22.11, it will accept DNS names for announce-addr again (without experimental) and announce this as a proper DNS name.

NicolasDorier commented 1 year ago

Checking 22.11 now.

Can you elaborate on this? Do you mean you want to use it so getinfo returns something different on the address part?

I mean that the reason I use announce-addr isn't anything to do with P2P. I just use it so getinfo returns a public endpoint that customers can use to create a channel with the merchant.

NicolasDorier commented 1 year ago

Doesn't work lightningd: /root/.lightning/config line 4: announce-addr: address 'lightningd_dest' is not announceable

Our address lightningd_dest is an address on the local network. I would expect that getinfo returns us a presentable .address.address (either the IP resolved from the DNS we passed to announce-addr, or the DNS itself).

There are too many problems now to be able to update:

  1. The requirement to have jsonrpc v2, (we will workaround and communicate that we need allow-deprecated-apis, but we can't use v2 support until we are certain v1 isn't used by BTCPay Server users anymore, so I guess it's 1-2 years down the road)
  2. Unable to build on arm32
  3. The inability to get a presentable node info string with a domain name or even IP caused by this issue.
m-schmoock commented 1 year ago

@NicolasDorier

The 'lightningd_dest' is not announceable issue is because lightningd_dest isn't a valid IPv4/v6/Tor address nor DNS hostname. DNS addresses must not contain underscores, only lowercase a-z, 0-9, - (hyphen), . (dots for separation).

If you use a valid hostname, at least this issue is gone. Keep in mind that characters like _ underscores work when used in /etc/hosts aliases, but thats not hostname compliant. See https://man7.org/linux/man-pages/man7/hostname.7.html

NicolasDorier commented 1 year ago

ok I will try that

NicolasDorier commented 1 year ago

@m-schmoock I tried, I confirm you are right. We still have problems to update, but this is unrelated to this issue. I close this one.

rustyrussell commented 1 year ago
  1. The requirement to have jsonrpc v2, (we will workaround and communicate that we need allow-deprecated-apis, but we can't use v2 support until we are certain v1 isn't used by BTCPay Server users anymore, so I guess it's 1-2 years down the road)

Ok, is there an issue for this?

Deprecations are a tool to make lives easier, not a rigid stick to beat developers with: if enforcing this causes problems we should have delayed removal until that was fixed!

whitslack commented 1 year ago

DNS addresses must not contain underscores

That's not true. Registered domain names must not contain underscores. DNS records can contain underscores, and in fact that's a common pattern in several standardized protocols. For example, DMARC records are named _dmarc, and SRV records are in the _tcp and _udp subdomains. DNS host names can certainly contain underscores, and in fact there are two devices on my home network whose hostnames contain underscores, and I didn't name them myself: Tesla_Model_3 and SimpliSafe_Basestation. So CLN is in the wrong on this point.

m-schmoock commented 1 year ago

DNS addresses must not contain underscores

That's not true. Registered domain names must not contain underscores. DNS records can contain underscores, and in fact that's a common pattern in several standardized protocols.

Well okay, but does it make sense to announce hostnames from your local network?

m-schmoock commented 1 year ago

Or do you mean something like this is valid on public dns servers?

host_name.example.com

whitslack commented 1 year ago

FQDNs containing underscores are globally resolvable. You just can't register a first-level domain name containing underscores at a domain registrar. So, it does make sense to announce FQDNs containing underscores.

It makes less sense to announce unqualified names or names qualified by a domain that's not resolvable from the global root name servers, but I would argue it's not CLN's place to make that judgment.

m-schmoock commented 1 year ago

@whitslack Thanks, I'll check that and change the code accordingly. Maybe we should allow underscores in the hostname part of an announced DNS name.

Any other additional characters allowed except underscores?

whitslack commented 1 year ago

Or do you mean something like this is valid on public dns servers?

host_name.example.com

Sure. I just created one to demonstrate. See for yourself:

$ host example_name.home.mattwhitlock.com
example_name.home.mattwhitlock.com has IPv6 address 2001:db8::dead:beef
whitslack commented 1 year ago

Any other additional characters allowed except underscores?

There are no restrictions on what bytes may appear in a domain name label. IETF RFC 1035 says (emphasis mine):

Although labels can contain any 8 bit values in octets that make up a label, it is strongly recommended that labels follow the preferred syntax described elsewhere in this memo, which is compatible with existing host naming conventions.

rustyrussell commented 1 year ago

Yeah, @m-schmoock please relax this requirement for the imminent point release?

We should probably note in the CHANGELOG (and release draft) that we recommend using IP addresses for announce-addr and addr configs in public nodes until DNS records are more widely recognized.

NicolasDorier commented 1 year ago

@rustyrussell I think that's fine about jsonrpc v2. I tried to add it on our library and tested it against old nodes, and they accept it, so we can add it to our lib and support still old nodes.

NicolasDorier commented 1 year ago

@m-schmoock I agree with @whitslack. The DNS name validation should be relaxed. I stumbled into this because in our test environment 2 lightning nodes in docker containers connected together on a local network. One of the hostname was lightningd_dest. Before it was working fine.

m-schmoock commented 1 year ago

Yeah, @m-schmoock please relax this requirement for the imminent point release?

We should probably note in the CHANGELOG (and release draft) that we recommend using IP addresses for announce-addr and addr configs in public nodes until DNS records are more widely recognized.

whitslack commented 1 year ago

On a related note, why do you reject uppercase characters in domain names? DNS labels are case-insensitive. Quoting IETF RFC 1035 again:

For all parts of the DNS that are part of the official protocol, all comparisons between character strings (e.g., labels, domain names, etc.) are done in a case-insensitive manner. At present, this rule is in force throughout the domain system without exception. However, future additions beyond current usage may need to use the full binary octet capabilities in names, so attempts to store domain names in 7-bit ASCII or use of special bytes to terminate labels, etc., should be avoided.

When data enters the domain system, its original case should be preserved whenever possible. In certain circumstances this cannot be done. For example, if two RRs are stored in a database, one at x.y and one at X.Y, they are actually stored at the same place in the database, and hence only one casing would be preserved. The basic rule is that case can be discarded only when data is used to define structure in a database, and two names are identical when compared in a case insensitive manner.

m-schmoock commented 1 year ago

@whitslack No special reason, initially this was because man 7 hostname said

Valid characters for hostnames are ASCII(7) letters from a to z, the digits from 0 to 9, and the hyphen (-).  A hostname may not start with a hyphen.

... And I didn't read to the part where it says

If a case-insensitive match is found between the hostname to be resolved and the first field of a line in the file, the substituted name is looked up with no further processing.

I just hadn't time to address this yet, see: https://github.com/ElementsProject/lightning/pull/5792

whitslack commented 1 year ago

@m-schmoock: You keep referring to hostname(7), but that man page describes the operation of Glibc's name resolver. It's not a normative specification of domain names. For what it's worth, that page refers (in its "See Also" section) to IETF RFC 1123, which states:

The DNS defines domain name syntax very generally -- a string of labels each containing up to 63 8-bit octets, separated by dots, and with a maximum total of 255 octets. Particular applications of the DNS are permitted to further constrain the syntax of the domain names they use, although the DNS deployment has led to some applications allowing more general names.

I guess you're exercising that second sentence very liberally, as you're arbitrarily inventing and imposing artificial constraints on DNS names. I'm done arguing this, though. If someone needs to announce a more general name, they'll just have to hack the CLN source themselves. :man_shrugging:

m-schmoock commented 1 year ago

I was not referring, I was answering your question "why do you reject uppercase" and how it came to be initially.

Yes, I don't think its a good idea to allow users to typo in names that will not be globally resolvable by other nodes. I think for other future/custom DNS naming systems/infrastructure, we can always add another type in the protocol ( https://github.com/lightning/bolts/blob/master/07-routing-gossip.md#the-node_announcement-message ) or relax the implementation here, if it becomes a thing.

Keep in mind that these node_announcement are send to all nodes in the network, and we must assume that everyone can understand them.