Skip to content
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

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

Closed
NicolasDorier opened this issue Oct 13, 2022 · 43 comments
Closed

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

NicolasDorier opened this issue Oct 13, 2022 · 43 comments
Assignees
Milestone

Comments

@NicolasDorier
Copy link
Collaborator

NicolasDorier commented Oct 13, 2022

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
Copy link
Collaborator Author

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

@whitslack
Copy link
Collaborator

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
Copy link
Collaborator Author

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

@dennisreimann
Copy link
Contributor

Any ideas, @rustyrussell?

@cdecker
Copy link
Member

cdecker commented Nov 3, 2022

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.

@flowolf flowolf mentioned this issue Nov 3, 2022
@rustyrussell
Copy link
Contributor

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!

@rustyrussell rustyrussell self-assigned this Nov 3, 2022
@rustyrussell rustyrussell added this to the v22.11 milestone Nov 3, 2022
@NicolasDorier
Copy link
Collaborator Author

NicolasDorier commented Nov 3, 2022

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
Copy link
Collaborator Author

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
Copy link
Collaborator

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. ( e0d6f3c connectd: DNS Bolt7 #911 no longer EXPERIMENTAL )

@NicolasDorier
Copy link
Collaborator Author

NicolasDorier commented Nov 29, 2022

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

@NicolasDorier
Copy link
Collaborator Author

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

@NicolasDorier
Copy link
Collaborator Author

NicolasDorier commented Nov 29, 2022

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
Copy link
Collaborator

m-schmoock commented Nov 29, 2022

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
Copy link
Contributor

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
Copy link
Collaborator

@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
Copy link
Collaborator

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
Copy link
Collaborator

Try to kick off some discussion about this in #5657

@whitslack
Copy link
Collaborator

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
Copy link
Collaborator

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
Copy link
Collaborator

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.

@cdecker cdecker modified the milestones: v22.11, v23.02 Dec 1, 2022
@NicolasDorier
Copy link
Collaborator Author

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.

@NicolasDorier
Copy link
Collaborator Author

NicolasDorier commented Dec 5, 2022

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
Copy link
Collaborator

m-schmoock commented Dec 5, 2022

@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
Copy link
Collaborator Author

ok I will try that

m-schmoock added a commit to m-schmoock/lightning that referenced this issue Dec 5, 2022
My little nothing path :)
This extends the testcase for is_dnsaddr for a case that came up
in issue ElementsProject#5657 ElementsProject#5657

Also fixes a typo in a comment.

Changelog-None
@NicolasDorier
Copy link
Collaborator Author

@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
Copy link
Contributor

  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
Copy link
Collaborator

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
Copy link
Collaborator

m-schmoock commented Dec 5, 2022

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
Copy link
Collaborator

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

host_name.example.com

@whitslack
Copy link
Collaborator

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
Copy link
Collaborator

@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
Copy link
Collaborator

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
Copy link
Collaborator

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
Copy link
Contributor

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
Copy link
Collaborator Author

@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
Copy link
Collaborator Author

NicolasDorier commented Dec 6, 2022

@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 added a commit to m-schmoock/lightning that referenced this issue Dec 6, 2022
The hostname part of a DNS FQDN can allow for additional characters
other than specified in `man 7 hostname`.

This extends is_dnsaddr and the test issue ElementsProject#5657.
Also fixes a typo in a comment.

Changelog-None
m-schmoock added a commit to m-schmoock/lightning that referenced this issue Dec 6, 2022
The hostname part of a DNS FQDN can allow for additional characters
other than specified in `man 7 hostname`.

This extends is_dnsaddr and the test issue ElementsProject#5657.
Also fixes a typo in a comment.

Changelog-Fixed: wireaddr: ElementsProject#5657 allow '_' underscore in hostname part of DNS FQDN
cdecker pushed a commit that referenced this issue Dec 6, 2022
The hostname part of a DNS FQDN can allow for additional characters
other than specified in `man 7 hostname`.

This extends is_dnsaddr and the test issue #5657.
Also fixes a typo in a comment.

Changelog-Fixed: wireaddr: #5657 allow '_' underscore in hostname part of DNS FQDN
@m-schmoock
Copy link
Collaborator

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
Copy link
Collaborator

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
Copy link
Collaborator

@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: #5792

@whitslack
Copy link
Collaborator

@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. 🤷‍♂️

@m-schmoock
Copy link
Collaborator

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.

ddustin pushed a commit to ddustin/lightning that referenced this issue Apr 11, 2023
The hostname part of a DNS FQDN can allow for additional characters
other than specified in `man 7 hostname`.

This extends is_dnsaddr and the test issue ElementsProject#5657.
Also fixes a typo in a comment.

Changelog-Fixed: wireaddr: ElementsProject#5657 allow '_' underscore in hostname part of DNS FQDN
ddustin pushed a commit to ddustin/lightning that referenced this issue May 12, 2023
The hostname part of a DNS FQDN can allow for additional characters
other than specified in `man 7 hostname`.

This extends is_dnsaddr and the test issue ElementsProject#5657.
Also fixes a typo in a comment.

Changelog-Fixed: wireaddr: ElementsProject#5657 allow '_' underscore in hostname part of DNS FQDN
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants