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

DNS for trusted peers #2193

Merged
merged 3 commits into from
May 13, 2020
Merged

DNS for trusted peers #2193

merged 3 commits into from
May 13, 2020

Conversation

eugene-babichenko
Copy link
Contributor

@eugene-babichenko eugene-babichenko commented May 7, 2020

Resolves #785
Replaces #1164

@eugene-babichenko eugene-babichenko added enhancement New feature or request subsys-network network related A-jormungandr Area: Issues affecting jörmungandr labels May 7, 2020
@eugene-babichenko eugene-babichenko requested a review from a team May 7, 2020 11:50
@eugene-babichenko eugene-babichenko self-assigned this May 7, 2020
Copy link
Contributor

@mzabaluev mzabaluev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the part of the PR fixing the to_socketaddr deprecation, but the part having to do with DNS lookups is problematic. Split? :)


let maybe_socket_addr = match ip_or_fqdn {
AddrComponent::DNS4(fqdn) => (fqdn.as_ref(), port)
.to_socket_addrs()?
Copy link
Contributor

@mzabaluev mzabaluev May 7, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is blocking for the duration of the DNS lookup, right?

We might want to have an async function here and use an async resolver.

let maybe_socket_addr = match ip_or_fqdn {
AddrComponent::DNS4(fqdn) => (fqdn.as_ref(), port)
.to_socket_addrs()?
.find(|addr| matches!(addr, SocketAddr::V4(_))),
Copy link
Contributor

@mzabaluev mzabaluev May 7, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another problem is that it will only take the first IP address, so this will be counterproductive for load-balancing setups. Not sure what is the recommended behavior for HTTP clients, though.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIRC DNS may be resolved in a "round-robin" fashion. This really depends on the server implementation. For example, Cloudflare states that

the server hands out a different address each time, operating on a rotation

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, simplistic clients have been known to only try the first IP address provided by the server.

However, we use a fully capable HTTP stack, it's only an internal information loss problem if we can't pass it an FQDN as such from the configuration.

Comment on lines 43 to 45
AddrComponent::DNS6(fqdn) => (fqdn.as_ref(), port)
.to_socket_addrs()?
.find(|addr| matches!(addr, SocketAddr::V6(_))),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a bit silly part of the multiaddr, too. We are supposed to only use IPv4 or IPv6 results, but we can't try both, which is a very common use case?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Exactly that :(

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a bit silly part of the multiaddr, too. We are supposed to only use IPv4 or IPv6 results, but we can't try both, which is a very common use case?

DNS uses different records for IPv6, so you can query either or both. 'A' records for IPv4 and "AAAA" records for IPv6.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can be done I think, but will require introducing a separate library for DNS resolution.

Copy link
Contributor

@mzabaluev mzabaluev May 9, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hyper has an async resolver abstraction and a default resolver pooling getaddrinfo calls in threads. I think this can be used to plug in a customized resolver that can limit the DNS queries to A, AAAA, or do both. The main point is, converting multiaddr to a single IP address here is a lossy transformation. Ultimately, we want proper HTTP behavior, Happy Eyeballs and all.

Copy link

@JamesRobertKelley JamesRobertKelley May 9, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I may have missed it, but are you considering how to pass the port number through DNS?

What I am writing below will enable the config file to contain ONLY the root domain name to be trusted. For example, iohk.com, without any host names or port numbers.

There are two scenarios depending on whether you want the genesis hash to be involved in the lookup. Including a genesis hash will allow you to use the same config but switch testnets or the mainnet by specifying a different genesis hash.

The genesis hash changes the approach because you cannot specify it as part of the DNS name. The maximum length for any part of a DNS name is 63 characters, but the genesis hash is 64 characters.

With a Genesis Hash

You will want to use TXT records similarly to how antispam SPF records are used. TXT records will allow 255 characters, allowing you to specify the hash, multiple host's DNS names, and the port all in one record.

Example TXT record:
cardano=8e4d2a343f3dcf9330ad9035b3e8d168e6728904262f2c434a4f8f934ec7b676 node.example.com:3000 node2.example.com:3000

Multiple TXT records may be created, one for each genesis hash you want to maintain.

Since the TXT record can include any domain name, the usual rules geo-based or load balancing would still apply when looking up the IP for the domain name. Notice that it can include ANY domain name, so simply inputting iohk.com in the config file would allow hosts with any other domain name to be returned.

Just keep in mind that the limitation is the length of the TXT record, which is 255 characters.

No Genesis Hash

If considerations regarding the genesis hash are not needed or desired, then SRV records are ideal because they specify port numbers and priority, similarly to MX records:

# _service._proto.name.      TTL   class SRV priority weight port target.
_cardano._tcp.example.com.   300   IN    SRV 10       60     3000 node.example.com.
_cardano._tcp.example.com.   300   IN    SRV 20       20     3000 node2.example.com.

The lookup will return a number of hosts, including the priority, to which the node should connect. This is a clean method of configuring which hosts to connect to because you can specify the port, protocol (TCP or UDP), and TTL, all in these records.

Each SRV record returns a DNS name which can benefit from geo-based or load balancing when looking up the IP address. You would also be able to specify many different hosts all with the same priority, and the cardano-node could continue to look up and connect to hosts as necessary.

Possible Combo of Genesis Hash and SRV Records

Just to throw this out there. A hash of the genesis hash could be used to shorten it for DNS use and then it could be used in the SRV records. Below is an example using two fictional hashes of a testnet and mainnet:

# _service._proto.name.                 TTL   class SRV priority weight port target.
_cardano-cf9330ad90._tcp.example.com.   300   IN    SRV 10       60     3000 mainnet1.example.com.
_cardano-cf9330ad90._tcp.example.com.   300   IN    SRV 20       20     3000 mainnet2.example.com.
_cardano-e672890426._tcp.example.com.   300   IN    SRV 10       60     3000 testnet1.example.com.
_cardano-e672890426._tcp.example.com.   300   IN    SRV 20       20     3000 testnet2.example.com.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @JamesRobertKelley. I like the approach with the SRV and I can see how that could be useful on the long run. Would you mind converting this to a JIP? It will be easier to track down the discussion around this in a separate issue.

jormungandr/src/settings/start/mod.rs Show resolved Hide resolved
@mzabaluev
Copy link
Contributor

Some past discussion in the other issues:
#785 (comment)
#1164 (comment)

Copy link
Contributor

@mzabaluev mzabaluev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need to at least remove blocking behavior before we go ahead with this.

@eugene-babichenko
Copy link
Contributor Author

@mzabaluev is that necessary right now? It is not used in an asynchronous context.

@mzabaluev
Copy link
Contributor

I dislike the FQDN implementation of ToSocketAddrs, the unfortunate offspring of the synchronous resolve APIs that every internet stack has dragged since the 1970s, and would prefer it not being used anywhere, especially hidden under public API.

This may do for now, but alternatively we should convert an FQDN multiaddr to an FQDN http::Uri and an IP address family preference flag to feed as connection parameters to tonic and let the HTTP stack asynchronously resolve the IP addresses and work through possible round-robin alternatives, instead of doing a poor job of it ourselves.

Also, rust-multiaddr should be made to support dns (accepted to the "spec" from the back door in multiformats/multiaddr#100), and maybe http once the multiaddr people decide how it should actually be used (multiformats/multiaddr#63)

@eugene-babichenko
Copy link
Contributor Author

resolved merge conflicts

@NicolasDP
Copy link
Contributor

@eugene-babichenko can you rebase, the tests should pass now

@NicolasDP
Copy link
Contributor

Also updating the documentation would be handy

@eugene-babichenko
Copy link
Contributor Author

done

Comment on lines +60 to +62
bootstrap the p2p topology (and bootstrap our local blockchain). Note that you can use a DNS
name in the following format: `/dns4/node.example.com/tcp/3000`. Use `dns6` instead of `dns4`
if you want the peer to connect with IPv6.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice, now it is more obvious for the user how to use this feature

@NicolasDP NicolasDP merged commit 1747395 into master May 13, 2020
@NicolasDP NicolasDP deleted the trusted-peers-dns-1 branch May 16, 2020 07:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-jormungandr Area: Issues affecting jörmungandr enhancement New feature or request subsys-network network related
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Trusted peers should accept DNS name
4 participants