allow adding `public_peers` with DNS name #1164

eugene-babichenko · 2019-11-20T15:34:45Z

Resolves #785

NicolasDP

Looking good. Could you make this a new module in the setting directory? That will improve readability.

NicolasDP · 2019-11-21T16:28:31Z

We cannot use that changes yet as we need to remove the need to use the trusted peers node id first.

mzabaluev · 2019-11-22T12:15:51Z

jormungandr/src/settings/start/trusted_peer.rs

+impl std::str::FromStr for TrustedPeer {
+    type Err = String;
+    fn from_str(s: &str) -> Result<Self, Self::Err> {
+        let mut split = s.split('@');


I'm not sure we need to make an official microformat for this.

mzabaluev · 2019-11-22T12:27:36Z

jormungandr/src/settings/start/mod.rs

+            .filter_map(|tp| {
+                tp.address.to_addresses().ok().map(|addrs| {
+                    addrs.into_iter().map(move |addr| TrustedPeer {
+                        id: tp.id.clone(),


Umm, do we have provisions elsewhere for the duplicated peer entries?
These are going to overwrite each other in PeerMap. To do it properly, there should be a "happy eyeballs" style async connection routine for a single peer, racing up to two connection trains – one going through a shuffled list of IPv4 addresses, the other through IPv6 – but only to the point where one connects on TCP, then the other needs to be discarded and the protocol handshake with subscriptions should proceed on the winning connection.

Provisions may need to be made in network-grpc to enable two-stage connections.

OK, maybe happy eyeballs is a little overkill as there is not much interactive difference in how many milliseconds it takes to establish a connection. So shuffle the list of all addresses for a single peer and go through them with TCP connection attempts until one succeeds, that would do it.

Even better perhaps, we could let hyper deal with DNS addresses because we give it an URL to connect to.

How much headache would it be to implement DNSSEC signature validation within Jormungandr and WARN if provided FQDN is not signed? Seems (I checked just three) Rust libraries do not support DNSSEC validation, does hyper rely on OS DNS resolver or a Rust library outside of the OS?

hyper supports custom resolvers, and previous commits on this branch used trust-dns-resolver which supports DNSSec. @eugene-babichenko @NicolasDP what do you think of re-integrating it back, either in this form or hooking into hyper?

Umm, do we have provisions elsewhere for the duplicated peer entries?

No, and actually this is my main concern about this pull request which I discussed with @NicolasDP. The correct approach is testing IPs until we find a reachable one and then just go for that address. But we must do it right after we got the whole list of addresses, because we also need to feed the node data to poldercast. We can rework poldercast crate a bit though to make it able to accept DNS instead of bare IPs.

Even better perhaps, we could let hyper deal with DNS addresses because we give it an URL to connect to.

If the whole network stuff including gossiping is done via hyper (that's what I can see from the code), I think it's totally fine.

what do you think of re-integrating it back, either in this form or hooking into hyper?

The whole point of the last commit was to remove an additional dependency, but I feel like we will return if want to support DNSSEC. I'm more into hooking that stuff into hyper, just because it sounds way more elegant.

To sum everything up, my approach would be to allow using DNS just everywhere in the code. Let's say, make it a first-class citizen along with the IPs. But this may require some modification of external dependencies, specifically poldercast.

mzabaluev · 2019-11-22T12:36:40Z

jormungandr/src/settings/start/trusted_peer.rs

+                    _ => None,
+                })
+                .collect(),
+            Some(AddrComponent::DNS6(fqdn)) => format!("{}:{}", fqdn, port)


It's... weird that multiaddr does not support just dns/ which would resolve both A and AAAA records. Which seems to be what's going to happen under the hood anyway, except that you discard the addresses of the wrong type.

Converting from multiaddr to an FQDN URL for hyper, as per #1164 (comment), would erase the IP address preference, so I'm not sure what would be more proper here.

Theoretically, HttpConnector::set_local_address in hyper can be used to restrict connections to IPv4 or IPv6, but I'm not sure it works with hyper's internal resolver.

Either approach requires us to make the whole system treat FQDNs the same way as IPs and probably bring everything to network-grpc (see #1164 (comment)).

mzabaluev · 2019-11-22T12:38:34Z

jormungandr/src/settings/start/trusted_peer.rs

+}
+
+impl TrustedAddress {
+    pub fn to_addresses(&self) -> Result<Vec<Address>, AddressError> {


Please add a prominent (doc) comment notice that a blocking network resolver roundtrip happens here, so that we remember to avoid calling this in async code.

mzabaluev · 2019-11-22T13:12:29Z

We cannot use that changes yet as we need to remove the need to use the trusted peers node id first.

We still need some way to identify a node with multiple IP addresses, including the corner cases when we receive its information from gossip with a different, maybe partially overlapping, set of addresses. Any of the old IP addresses may be taken over by other, logically different nodes, so I'm not sure intersecting on any IP address would give you a moral right to put it under the same node entry and reuse the connection.

mark-stopka · 2019-11-22T15:30:56Z

We cannot use that changes yet as we need to remove the need to use the trusted peers node id first.

We still need some way to identify a node with multiple IP addresses, including the corner cases when we receive its information from gossip with a different, maybe partially overlapping, set of addresses. Any of the old IP addresses may be taken over by other, logically different nodes, so I'm not sure intersecting on any IP address would give you a moral right to put it under the same node entry and reuse the connection.

That's what was good on the initial private key, public key design, now when node_id is just a random string, anybody can replicate someone else's node_id.

mzabaluev

@mark-stopka We are still debating the correct approach with node IDs and their authentication and whether they are worth having at all.

eugene-babichenko · 2020-05-08T10:13:00Z

closing this in favor of #2193

allow adding public_peers with DNS name

caac50d

eugene-babichenko force-pushed the trusted-peers-dns branch from 4d51710 to caac50d Compare November 20, 2019 15:53

avoid empty vectors in TrustedAddress::to_addresses

f914eef

eugene-babichenko force-pushed the trusted-peers-dns branch from a05c616 to f914eef Compare November 21, 2019 08:09

NicolasDP requested a review from a team November 21, 2019 15:59

NicolasDP added enhancement New feature or request A-jormungandr Area: Issues affecting jörmungandr labels Nov 21, 2019

NicolasDP suggested changes Nov 21, 2019

View reviewed changes

NicolasDP self-assigned this Nov 21, 2019

NicolasDP added the DO NOT MERGE not to be merged until something else is done label Nov 21, 2019

NicolasDP mentioned this pull request Nov 21, 2019

Remove need to have trusted peer NodeID #1174

Open

eugene-babichenko added 2 commits November 21, 2019 20:26

refactor to remove trust-dns from dependencies

2685020

move TrustedPeer to a separate module

bb4218a

mzabaluev reviewed Nov 22, 2019

View reviewed changes

mzabaluev suggested changes Nov 22, 2019

View reviewed changes

mzabaluev reviewed Nov 22, 2019

View reviewed changes

eugene-babichenko mentioned this pull request May 7, 2020

DNS for trusted peers #2193

Merged

eugene-babichenko closed this May 8, 2020

NicolasDP deleted the trusted-peers-dns branch May 16, 2020 07:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

allow adding `public_peers` with DNS name #1164

allow adding `public_peers` with DNS name #1164

eugene-babichenko commented Nov 20, 2019

NicolasDP left a comment

NicolasDP commented Nov 21, 2019

mzabaluev Nov 22, 2019

mzabaluev Nov 22, 2019

mzabaluev Nov 22, 2019

mzabaluev Nov 22, 2019

mark-stopka Nov 22, 2019

mzabaluev Nov 22, 2019

eugene-babichenko Nov 22, 2019

mzabaluev Nov 22, 2019

mzabaluev Nov 22, 2019

mzabaluev Nov 22, 2019 •

edited

Loading

eugene-babichenko Nov 22, 2019

mzabaluev Nov 22, 2019

mzabaluev commented Nov 22, 2019

mark-stopka commented Nov 22, 2019

mzabaluev left a comment

eugene-babichenko commented May 8, 2020

allow adding public_peers with DNS name #1164

allow adding public_peers with DNS name #1164

Conversation

eugene-babichenko commented Nov 20, 2019

NicolasDP left a comment

Choose a reason for hiding this comment

NicolasDP commented Nov 21, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mzabaluev Nov 22, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mzabaluev commented Nov 22, 2019

mark-stopka commented Nov 22, 2019

mzabaluev left a comment

Choose a reason for hiding this comment

eugene-babichenko commented May 8, 2020

allow adding `public_peers` with DNS name #1164

allow adding `public_peers` with DNS name #1164

mzabaluev Nov 22, 2019 •

edited

Loading