diff --git a/Cargo.lock b/Cargo.lock index e939caf769a..4d30ae54a3d 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -6355,6 +6355,7 @@ dependencies = [ "nym-contracts-common", "nym-crypto", "nym-explorer-client", + "nym-http-api-client", "nym-network-defaults", "nym-node-metrics", "nym-node-requests", diff --git a/common/client-libs/validator-client/src/coconut/mod.rs b/common/client-libs/validator-client/src/coconut/mod.rs index 95270a25ed9..67974c0170a 100644 --- a/common/client-libs/validator-client/src/coconut/mod.rs +++ b/common/client-libs/validator-client/src/coconut/mod.rs @@ -83,6 +83,12 @@ impl TryFrom for EcashApiClient { let url_address = Url::parse(&share.announce_address)?; + // The NymApiClient constructed here uses the default (hickory DoT/DoH) resolver because + // this EcashApiClient is used by both client and non-client applications. + // + // In non-client applications this resolver can cause warning logs about H2 connection + // failure. This indicates that the long lived https connection was closed by the remote + // peer and the resolver will have to reconnect. It should not impact actual functionality Ok(EcashApiClient { api_client: NymApiClient::new(url_address), verification_key: VerificationKeyAuth::try_from_bs58(&share.share)?, diff --git a/common/http-api-client/src/dns.rs b/common/http-api-client/src/dns.rs index 7e0519de98e..4d4aae82d63 100644 --- a/common/http-api-client/src/dns.rs +++ b/common/http-api-client/src/dns.rs @@ -1,3 +1,6 @@ +// Copyright 2023 - Nym Technologies SA +// SPDX-License-Identifier: Apache-2.0 + //! DNS resolver configuration for internal lookups. //! //! The resolver itself is the set combination of the google, cloudflare, and quad9 endpoints @@ -9,6 +12,19 @@ //! //! Requires the `dns-over-https-rustls`, `webpki-roots` feature for the //! `hickory-resolver` crate +//! +//! +//! Note: The hickory DoH resolver can cause warning logs about H2 connection failure. This +//! indicates that the long lived https connection was closed by the remote peer and the resolver +//! will have to reconnect. It should not impact actual functionality. +//! +//! code ref: https://github.com/hickory-dns/hickory-dns/blob/06a8b1ce9bd9322d8e6accf857d30257e1274427/crates/proto/src/h2/h2_client_stream.rs#L534 +//! +//! example log: +//! +//! ```txt +//! WARN /home/ubuntu/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/hickory-proto-0.24.3/src/h2/h2_client_stream.rs:493: h2 connection failed: unexpected end of file +//! ``` #![deny(missing_docs)] use crate::ClientBuilder; @@ -33,6 +49,13 @@ impl ClientBuilder { /// Override the DNS resolver implementation used by the underlying http client. pub fn dns_resolver(mut self, resolver: Arc) -> Self { self.reqwest_client_builder = self.reqwest_client_builder.dns_resolver(resolver); + self.use_secure_dns = false; + self + } + + /// Override the DNS resolver implementation used by the underlying http client. + pub fn no_hickory_dns(mut self) -> Self { + self.use_secure_dns = false; self } } diff --git a/common/http-api-client/src/lib.rs b/common/http-api-client/src/lib.rs index 4b2938daa4d..5a360f7be1c 100644 --- a/common/http-api-client/src/lib.rs +++ b/common/http-api-client/src/lib.rs @@ -228,6 +228,8 @@ pub struct ClientBuilder { timeout: Option, custom_user_agent: bool, reqwest_client_builder: reqwest::ClientBuilder, + #[allow(dead_code)] // not dead code, just unused in wasm + use_secure_dns: bool, } impl ClientBuilder { @@ -239,37 +241,46 @@ impl ClientBuilder { U: IntoUrl, E: Display, { - // a naive check: if the provided URL does not start with http(s), add that scheme let str_url = url.as_str(); + // a naive check: if the provided URL does not start with http(s), add that scheme if !str_url.starts_with("http") { let alt = format!("http://{str_url}"); warn!("the provided url ('{str_url}') does not contain scheme information. Changing it to '{alt}' ..."); // TODO: or should we maybe default to https? Self::new(alt) } else { - #[cfg(target_arch = "wasm32")] - let reqwest_client_builder = reqwest::ClientBuilder::new(); - - #[cfg(not(target_arch = "wasm32"))] - let reqwest_client_builder = { - let r = reqwest::ClientBuilder::new() - .dns_resolver(Arc::new(HickoryDnsResolver::default())); - - // Note this is extra as the `gzip` feature for `reqwest` crate should be enabled which - // `"Enable[s] auto gzip decompression by checking the Content-Encoding response header."` - // - // I am going to leave it here anyways so that gzip decompression is attempted even if - // that feature is removed. - r.gzip(true) - }; - - Ok(ClientBuilder { - url: url.into_url()?, - timeout: None, - custom_user_agent: false, - reqwest_client_builder, - }) + Ok(Self::new_with_url(url.into_url()?)) + } + } + + /// Constructs a new http `ClientBuilder` from a valid url. + pub fn new_with_url(url: Url) -> Self { + if !url.scheme().starts_with("http") { + warn!("the provided url ('{url}') does not use HTTP / HTTPS scheme"); + } + + #[cfg(target_arch = "wasm32")] + let reqwest_client_builder = reqwest::ClientBuilder::new(); + + #[cfg(not(target_arch = "wasm32"))] + let reqwest_client_builder = { + let r = reqwest::ClientBuilder::new(); + + // Note this is extra as the `gzip` feature for `reqwest` crate should be enabled which + // `"Enable[s] auto gzip decompression by checking the Content-Encoding response header."` + // + // I am going to leave it here anyways so that gzip decompression is attempted even if + // that feature is removed. + r.gzip(true) + }; + + ClientBuilder { + url, + timeout: None, + custom_user_agent: false, + reqwest_client_builder, + use_secure_dns: true, } } @@ -325,10 +336,18 @@ impl ClientBuilder { let mut builder = self .reqwest_client_builder .timeout(self.timeout.unwrap_or(DEFAULT_TIMEOUT)); + + // if no custom user agent was set, use a default if !self.custom_user_agent { builder = builder.user_agent(format!("nym-http-api-client/{}", env!("CARGO_PKG_VERSION"))) } + + // unless explicitly disabled use the DoT/DoH enabled resolver + if self.use_secure_dns { + builder = builder.dns_resolver(Arc::new(HickoryDnsResolver::default())); + } + builder.build()? }; @@ -355,6 +374,9 @@ pub struct Client { impl Client { /// Create a new http `Client` // no timeout until https://github.com/seanmonstar/reqwest/issues/1135 is fixed + // + // In order to prevent interference in API requests at the DNS phase we default to a resolver + // that uses DoT and DoH. pub fn new(base_url: Url, timeout: Option) -> Self { Self::new_url::<_, String>(base_url, timeout).expect( "we provided valid url and we were unwrapping previous construction errors anyway", diff --git a/nym-api/src/node_describe_cache/mod.rs b/nym-api/src/node_describe_cache/mod.rs index 8fa5adf452f..cdb9ab67b85 100644 --- a/nym-api/src/node_describe_cache/mod.rs +++ b/nym-api/src/node_describe_cache/mod.rs @@ -191,6 +191,7 @@ async fn try_get_client( // if provided host was malformed, no point in continuing let client = match nym_node_requests::api::Client::builder(address).and_then(|b| { b.with_timeout(Duration::from_secs(5)) + .no_hickory_dns() .with_user_agent("nym-api-describe-cache") .build() }) { diff --git a/nym-node-status-api/nym-node-status-api/Cargo.toml b/nym-node-status-api/nym-node-status-api/Cargo.toml index 0642149101a..f78218e246a 100644 --- a/nym-node-status-api/nym-node-status-api/Cargo.toml +++ b/nym-node-status-api/nym-node-status-api/Cargo.toml @@ -29,6 +29,7 @@ nym-bin-common = { path = "../../common/bin-common", features = ["models"] } nym-node-status-client = { path = "../nym-node-status-client" } nym-crypto = { path = "../../common/crypto", features = ["asymmetric", "serde"] } nym-explorer-client = { path = "../../explorer-api/explorer-client" } +nym-http-api-client = { path = "../../common/http-api-client" } nym-network-defaults = { path = "../../common/network-defaults" } nym-serde-helpers = { path = "../../common/serde-helpers"} nym-statistics-common = { path = "../../common/statistics" } diff --git a/nym-node-status-api/nym-node-status-api/src/monitor/mod.rs b/nym-node-status-api/nym-node-status-api/src/monitor/mod.rs index 05b7dc843d1..81711d76400 100644 --- a/nym-node-status-api/nym-node-status-api/src/monitor/mod.rs +++ b/nym-node-status-api/nym-node-status-api/src/monitor/mod.rs @@ -97,8 +97,12 @@ impl Monitor { .clone() .expect("rust sdk mainnet default missing api_url"); - let api_client = - NymApiClient::new_with_timeout(default_api_url, self.nym_api_client_timeout); + let nym_api = nym_http_api_client::ClientBuilder::new_with_url(default_api_url) + .no_hickory_dns() + .with_timeout(self.nym_api_client_timeout) + .build::<&str>()?; + + let api_client = NymApiClient { nym_api }; let described_nodes = api_client .get_all_described_nodes() diff --git a/nym-node-status-api/nym-node-status-api/src/node_scraper/mod.rs b/nym-node-status-api/nym-node-status-api/src/node_scraper/mod.rs index 04dd8970984..f294b47b0f0 100644 --- a/nym-node-status-api/nym-node-status-api/src/node_scraper/mod.rs +++ b/nym-node-status-api/nym-node-status-api/src/node_scraper/mod.rs @@ -57,7 +57,12 @@ async fn run( .clone() .expect("rust sdk mainnet default missing api_url"); - let api_client = NymApiClient::new_with_timeout(default_api_url, nym_api_client_timeout); + let nym_api = nym_http_api_client::ClientBuilder::new_with_url(default_api_url) + .no_hickory_dns() + .with_timeout(nym_api_client_timeout) + .build::<&str>()?; + + let api_client = NymApiClient { nym_api }; //SW TBC what nodes exactly need to be scraped, the skimmed node endpoint seems to return more nodes let bonded_nodes = api_client.get_all_bonded_nym_nodes().await?; @@ -170,6 +175,7 @@ impl MetricsScrapingData { let client = match nym_node_requests::api::Client::builder(address).and_then(|b| { b.with_timeout(Duration::from_secs(5)) .with_user_agent("node-status-api-metrics-scraper") + .no_hickory_dns() .build() }) { Ok(client) => client, diff --git a/nym-node/Cargo.toml b/nym-node/Cargo.toml index 0cf816ba5bd..79685d1ae9f 100644 --- a/nym-node/Cargo.toml +++ b/nym-node/Cargo.toml @@ -71,7 +71,7 @@ nym-verloc = { path = "../common/verloc" } nym-metrics = { path = "../common/nym-metrics" } nym-gateway-stats-storage = { path = "../common/gateway-stats-storage" } nym-topology = { path = "../common/topology" } - +nym-http-api-client = { path = "../common/http-api-client" } # http server # useful for `#[axum_macros::debug_handler]` diff --git a/nym-node/src/error.rs b/nym-node/src/error.rs index 5eb5545e21e..d7cec1edbd9 100644 --- a/nym-node/src/error.rs +++ b/nym-node/src/error.rs @@ -3,6 +3,7 @@ use crate::node::http::error::NymNodeHttpError; use crate::wireguard::error::WireguardError; +use nym_http_api_client::HttpClientError; use nym_ip_packet_router::error::ClientCoreError; use nym_validator_client::ValidatorClientError; use std::io; @@ -209,3 +210,9 @@ pub enum ServiceProvidersError { #[error(transparent)] ExternalClientCore(#[from] ClientCoreError), } + +impl From for NymNodeError { + fn from(value: HttpClientError) -> Self { + Self::HttpFailure(NymNodeHttpError::ClientError { source: value }) + } +} diff --git a/nym-node/src/node/http/error.rs b/nym-node/src/node/http/error.rs index 251705f5206..f63ed26f49c 100644 --- a/nym-node/src/node/http/error.rs +++ b/nym-node/src/node/http/error.rs @@ -1,6 +1,7 @@ // Copyright 2024 - Nym Technologies SA // SPDX-License-Identifier: GPL-3.0-only +use nym_http_api_client::HttpClientError; use std::io; use std::net::SocketAddr; use thiserror::Error; @@ -24,4 +25,10 @@ pub enum NymNodeHttpError { #[from] source: nym_crypto::asymmetric::encryption::KeyRecoveryError, }, + + #[error("error building or using HTTP client: {source}")] + ClientError { + #[from] + source: HttpClientError, + }, } diff --git a/nym-node/src/node/mod.rs b/nym-node/src/node/mod.rs index 3a8d7bf8673..f68a2d5553c 100644 --- a/nym-node/src/node/mod.rs +++ b/nym-node/src/node/mod.rs @@ -814,9 +814,23 @@ impl NymNode { return; } - for nym_api in &self.config.mixnet.nym_api_urls { - info!("trying {nym_api}..."); - let client = NymApiClient::new_with_user_agent(nym_api.clone(), self.user_agent()); + for nym_api_url in &self.config.mixnet.nym_api_urls { + info!("trying {nym_api_url}..."); + + let nym_api = + match nym_http_api_client::ClientBuilder::new_with_url(nym_api_url.clone()) + .no_hickory_dns() + .with_user_agent(self.user_agent()) + .build::<&str>() + { + Ok(b) => b, + Err(e) => { + warn!("failed to build http client for \"{nym_api_url}\": {e}",); + continue; + } + }; + + let client = NymApiClient { nym_api }; // make new request every time in case previous one takes longer and invalidates the signature let request = NodeRefreshBody::new(self.ed25519_identity_keys.private_key()); diff --git a/nym-node/src/node/shared_network.rs b/nym-node/src/node/shared_network.rs index c127aa15866..aa2e378541f 100644 --- a/nym-node/src/node/shared_network.rs +++ b/nym-node/src/node/shared_network.rs @@ -9,6 +9,7 @@ use nym_node_metrics::prometheus_wrapper::{PrometheusMetric, PROMETHEUS_METRICS} use nym_task::ShutdownToken; use nym_topology::node::RoutingNode; use nym_topology::{EpochRewardedSet, NymTopology, Role, TopologyProvider}; +use nym_validator_client::nym_api::NymApiClientExt; use nym_validator_client::nym_nodes::{NodesByAddressesResponse, SkimmedNode}; use nym_validator_client::{NymApiClient, ValidatorClientError}; use std::collections::HashSet; @@ -167,6 +168,7 @@ impl NodesQuerier { ) -> Result { let res = self .client + .nym_api .nodes_by_addresses(ips) .await .inspect_err(|err| error!("failed to obtain node information: {err}")); @@ -174,7 +176,7 @@ impl NodesQuerier { if res.is_err() { self.use_next_nym_api() } - res + Ok(res?) } } @@ -263,9 +265,14 @@ impl NetworkRefresher { pending_check_interval: Duration, shutdown_token: ShutdownToken, ) -> Result { + let nym_api = nym_http_api_client::Client::builder(nym_api_urls[0].clone())? + .no_hickory_dns() + .with_user_agent(user_agent) + .build()?; + let mut this = NetworkRefresher { querier: NodesQuerier { - client: NymApiClient::new_with_user_agent(nym_api_urls[0].clone(), user_agent), + client: NymApiClient { nym_api }, nym_api_urls, currently_used_api: 0, },