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

allow adding public_peers with DNS name #1164

Closed
wants to merge 4 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions jormungandr/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ hyper = "0.12"
jormungandr-lib = { path = "../jormungandr-lib" }
lazy_static = "1.3"
linked-hash-map = "0.5"
multiaddr = "0.3"
native-tls = "0.2.2"
network-core = { path = "../chain-deps/network-core" }
network-grpc = { path = "../chain-deps/network-grpc" }
Expand Down
5 changes: 3 additions & 2 deletions jormungandr/src/network/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -137,7 +137,7 @@ impl GlobalState {
.map(|tp| {
let mut builder = poldercast::NodeProfileBuilder::new();
builder.id(tp.id.into());
builder.address(tp.address.into());
builder.address(tp.address);
builder.build()
})
.map(p2p::Gossip::from)
Expand Down Expand Up @@ -432,7 +432,8 @@ fn trusted_peers_shuffled(config: &Configuration) -> Vec<SocketAddr> {
let mut peers = config
.trusted_peers
.iter()
.filter_map(|peer| peer.address.to_socketaddr())
.map(|peer| peer.address.to_socketaddr().into_iter())
.flatten()
.collect::<Vec<_>>();
let mut rng = rand::thread_rng();
peers.shuffle(&mut rng);
Expand Down
2 changes: 1 addition & 1 deletion jormungandr/src/settings/command_arguments.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
use crate::settings::{start::config::TrustedPeer, LOG_FILTER_LEVEL_POSSIBLE_VALUES};
use crate::settings::{start::trusted_peer::*, LOG_FILTER_LEVEL_POSSIBLE_VALUES};
use slog::FilterLevel;
use std::net::SocketAddr;
use std::path::PathBuf;
Expand Down
46 changes: 13 additions & 33 deletions jormungandr/src/settings/start/config.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,10 @@
use crate::{
network::p2p::{topic, Id, PolicyConfig},
settings::logging::{LogFormat, LogOutput},
settings::LOG_FILTER_LEVEL_POSSIBLE_VALUES,
settings::{
logging::{LogFormat, LogOutput},
start::trusted_peer::*,
LOG_FILTER_LEVEL_POSSIBLE_VALUES,
},
};
use jormungandr_lib::{interfaces::Mempool, time::Duration};
use poldercast;
Expand Down Expand Up @@ -110,13 +113,6 @@ pub struct P2pConfig {
pub max_unreachable_nodes_to_connect_per_event: Option<usize>,
}

#[derive(Debug, Clone, Serialize, Deserialize)]
#[serde(deny_unknown_fields)]
pub struct TrustedPeer {
pub address: Address,
pub id: Id,
}

#[derive(Debug, Clone, PartialEq, Eq, Deserialize, Serialize)]
#[serde(deny_unknown_fields)]
pub struct Leadership {
Expand Down Expand Up @@ -182,36 +178,19 @@ impl Default for Leadership {
}
}

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

let address = if let Some(address) = split.next() {
address
.parse::<poldercast::Address>()
.map(Address)
.map_err(|e| e.to_string())?
} else {
return Err("Missing address component".to_owned());
};

let id = if let Some(id) = split.next() {
id.parse::<Id>().map_err(|e| e.to_string())?
} else {
return Err("Missing id component".to_owned());
};

Ok(TrustedPeer { address, id })
}
}

impl Address {
pub fn to_socketaddr(&self) -> Option<SocketAddr> {
self.0.to_socketaddr()
}
}

custom_error! {pub AddressError
DnsLookupError { source: std::io::Error } = "failed to resolve DNS name {source}",
NoPortSpecified = "no TCP port specified",
NoAppropriateDNSFound = "the address was resolved, but it doesn't provide IPv4 or IPv6 addresses",
UnsupportedProtocol = "the provided protocol is unsupported, please use one of ip4/ip6/dns4/dns6",
}

impl std::fmt::Display for Address {
fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result {
self.0.fmt(f)
Expand All @@ -226,6 +205,7 @@ impl Serialize for Address {
serializer.serialize_str(&format!("{}", self.0))
}
}

impl Serialize for Topic {
fn serialize<S>(&self, serializer: S) -> Result<S::Ok, S::Error>
where
Expand Down
13 changes: 12 additions & 1 deletion jormungandr/src/settings/start/mod.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
pub mod config;
pub mod network;
pub mod trusted_peer;

use self::config::{Config, Leadership};
pub use self::config::{Cors, Rest};
Expand Down Expand Up @@ -181,6 +182,8 @@ fn generate_network(
command_arguments: &StartArguments,
config: &Option<Config>,
) -> Result<network::Configuration, Error> {
use crate::settings::start::network::TrustedPeer;

let mut p2p = if let Some(cfg) = config {
cfg.p2p.clone()
} else {
Expand Down Expand Up @@ -233,7 +236,15 @@ fn generate_network(
.clone()
.unwrap_or(vec![])
.into_iter()
.map(Into::into)
.filter_map(|tp| {
tp.address.to_addresses().ok().map(|addrs| {
addrs.into_iter().map(move |addr| TrustedPeer {
id: tp.id.clone(),
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

address: addr.0,
})
})
})
.flatten()
.collect(),
protocol: Protocol::Grpc,
policy: p2p.policy.clone(),
Expand Down
9 changes: 0 additions & 9 deletions jormungandr/src/settings/start/network.rs
Original file line number Diff line number Diff line change
Expand Up @@ -76,15 +76,6 @@ pub struct TrustedPeer {
pub id: Id,
}

impl From<super::config::TrustedPeer> for TrustedPeer {
fn from(tp: super::config::TrustedPeer) -> Self {
TrustedPeer {
address: tp.address.0,
id: tp.id,
}
}
}

impl Peer {
pub fn new(connection: SocketAddr, protocol: Protocol) -> Self {
Peer {
Expand Down
146 changes: 146 additions & 0 deletions jormungandr/src/settings/start/trusted_peer.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,146 @@
use crate::{network::p2p::Id, settings::start::config::Address};
use multiaddr::AddrComponent;
use serde::{de::Visitor, Deserialize, Deserializer, Serialize, Serializer};
use std::{
fmt,
iter::FromIterator,
net::{SocketAddr, ToSocketAddrs},
};

#[derive(Debug, Clone, Serialize, Deserialize)]
#[serde(deny_unknown_fields)]
pub struct TrustedPeer {
pub address: TrustedAddress,
pub id: Id,
}

#[derive(Debug, Clone, PartialEq, Eq)]
pub struct TrustedAddress(pub multiaddr::Multiaddr);

custom_error! {pub AddressError
DnsLookupError { source: std::io::Error } = "failed to resolve DNS name {source}",
NoPortSpecified = "no TCP port specified",
NoAppropriateDNSFound = "the address was resolved, but it doesn't provide IPv4 or IPv6 addresses",
UnsupportedProtocol = "the provided protocol is unsupported, please use one of ip4/ip6/dns4/dns6",
}

impl TrustedAddress {
pub fn to_addresses(&self) -> Result<Vec<Address>, AddressError> {
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

let mut components = self.0.iter();
let protocol = components.next();

if let Some(AddrComponent::IP4(_)) | Some(AddrComponent::IP6(_)) = protocol {
return Ok(vec![Address(
poldercast::Address::new(self.0.clone()).unwrap(),
)]);
}

let port = match components.next() {
Some(AddrComponent::TCP(port)) => port,
_ => return Err(AddressError::NoPortSpecified),
};

let addresses: Vec<AddrComponent> = match protocol {
Some(AddrComponent::DNS4(fqdn)) => format!("{}:{}", fqdn, port)
.to_socket_addrs()
.map_err(|e| AddressError::DnsLookupError { source: e })?
.into_iter()
.filter_map(|r| match r {
SocketAddr::V4(addr) => Some(AddrComponent::IP4(*addr.ip())),
_ => None,
})
.collect(),
Some(AddrComponent::DNS6(fqdn)) => format!("{}:{}", fqdn, port)
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@mzabaluev mzabaluev Nov 22, 2019

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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)).

.to_socket_addrs()
.map_err(|e| AddressError::DnsLookupError { source: e })?
.into_iter()
.filter_map(|r| match r {
SocketAddr::V6(addr) => Some(AddrComponent::IP6(*addr.ip())),
_ => None,
})
.collect(),
_ => return Err(AddressError::UnsupportedProtocol),
};

if addresses.is_empty() {
return Err(AddressError::NoAppropriateDNSFound);
}

let addresses = addresses
.into_iter()
.map(|addr| {
let new_components = vec![addr, AddrComponent::TCP(port)];
let new_multiaddr = multiaddr::Multiaddr::from_iter(new_components.into_iter());
Address(poldercast::Address::new(new_multiaddr).unwrap())
})
.collect();

Ok(addresses)
}
}

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

Choose a reason for hiding this comment

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

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


let address = if let Some(address) = split.next() {
multiaddr::Multiaddr::from_bytes(address.as_bytes().iter().cloned().collect())
.map(TrustedAddress)
.map_err(|e| e.to_string())?
} else {
return Err("Missing address component".to_owned());
};

let id = if let Some(id) = split.next() {
id.parse::<Id>().map_err(|e| e.to_string())?
} else {
return Err("Missing id component".to_owned());
};

Ok(TrustedPeer { address, id })
}
}

impl Serialize for TrustedAddress {
fn serialize<S>(&self, serializer: S) -> Result<S::Ok, S::Error>
where
S: Serializer,
{
serializer.serialize_str(&format!("{}", self.0))
}
}

impl std::fmt::Display for TrustedAddress {
fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result {
self.0.fmt(f)
}
}

impl<'de> Deserialize<'de> for TrustedAddress {
fn deserialize<D>(deserializer: D) -> Result<Self, D::Error>
where
D: Deserializer<'de>,
{
struct TrustedAddressVisitor;
impl<'de> Visitor<'de> for TrustedAddressVisitor {
type Value = TrustedAddress;

fn expecting(&self, fmt: &mut fmt::Formatter) -> fmt::Result {
write!(fmt, "Multiaddr (example: /ip4/192.168.0.1/tcp/443)")
}

fn visit_str<'a, E>(self, v: &'a str) -> std::result::Result<Self::Value, E>
where
E: serde::de::Error,
{
use serde::de::Unexpected;
match v.parse() {
Err(_err) => Err(E::invalid_value(Unexpected::Str(v), &self)),
Ok(addr) => Ok(TrustedAddress(addr)),
}
}
}
deserializer.deserialize_str(TrustedAddressVisitor)
}
}