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.

Sign up for GitHub

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

Fix TLS endpoint port propagation in gossip #1708

Merged
merged 3 commits into from
Jan 13, 2025
+58 −17
Merged

Fix TLS endpoint port propagation in gossip #1708

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
5bb64b4
Fix TLS endpoint port propagation in gossip
Mallets Jan 13, 2025
53d78c7
Support QUIC port propagation
Mallets Jan 13, 2025
f9ab0b4
Log common name upon TLS/QUIC connection
Mallets Jan 13, 2025
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
37 changes: 24 additions & 13 deletions io/zenoh-links/zenoh-link-quic/src/unicast.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@
//

use std::{
fmt,
fmt::{self, Debug},
net::{IpAddr, Ipv4Addr, Ipv6Addr, SocketAddr},
sync::Arc,
time::Duration,
Expand Down Expand Up @@ -42,7 +42,7 @@ use zenoh_protocol::{
use zenoh_result::{bail, zerror, ZResult};

use crate::{
utils::{get_quic_addr, TlsClientConfig, TlsServerConfig},
utils::{get_quic_addr, get_quic_host, TlsClientConfig, TlsServerConfig},
ALPN_QUIC_HTTP, QUIC_ACCEPT_THROTTLE_TIME, QUIC_DEFAULT_MTU, QUIC_LOCATOR_PREFIX,
};

Expand Down Expand Up @@ -251,11 +251,7 @@ impl LinkManagerUnicastQuic {
impl LinkManagerUnicastTrait for LinkManagerUnicastQuic {
async fn new_link(&self, endpoint: EndPoint) -> ZResult<LinkUnicast> {
let epaddr = endpoint.address();
let host = epaddr
.as_str()
.split(':')
.next()
.ok_or("Endpoints must be of the form quic/<address>:<port>")?;
let host = get_quic_host(&epaddr)?;
let epconf = endpoint.config();

let dst_addr = get_quic_addr(&epaddr).await?;
Expand Down Expand Up @@ -358,6 +354,7 @@ impl LinkManagerUnicastTrait for LinkManagerUnicastQuic {
};

let addr = get_quic_addr(&epaddr).await?;
let host = get_quic_host(&epaddr)?;

// Server config
let mut server_crypto = TlsServerConfig::new(&epconf)
Expand Down Expand Up @@ -418,12 +415,18 @@ impl LinkManagerUnicastTrait for LinkManagerUnicastQuic {
let local_addr = quic_endpoint
.local_addr()
.map_err(|e| zerror!("Can not create a new QUIC listener on {}: {}", addr, e))?;
let local_port = local_addr.port();

// Update the endpoint locator address
let endpoint = EndPoint::new(
let locator = Locator::new(
endpoint.protocol(),
local_addr.to_string(),
format!("{host}:{local_port}"),
endpoint.metadata(),
)?;
let endpoint = EndPoint::new(
locator.protocol(),
locator.address(),
locator.metadata(),
endpoint.config(),
)?;

Expand All @@ -446,8 +449,6 @@ impl LinkManagerUnicastTrait for LinkManagerUnicastQuic {
};

// Initialize the QuicAcceptor
let locator = endpoint.to_locator();

self.listeners
.add_listener(endpoint, local_addr, task, token)
.await?;
Expand Down Expand Up @@ -539,7 +540,7 @@ async fn accept_task(
}
}

tracing::debug!("Accepted QUIC connection on {:?}: {:?}", src_addr, dst_addr);
tracing::debug!("Accepted QUIC connection on {:?}: {:?}. {:?}.", src_addr, dst_addr, auth_id);
// Create the new link object
let link = Arc::<LinkUnicastQuic>::new_cyclic(|weak_link| {
let mut expiration_manager = None;
Expand Down Expand Up @@ -627,11 +628,21 @@ fn get_cert_chain_expiration(conn: &quinn::Connection) -> ZResult<Option<OffsetD
Ok(link_expiration)
}

#[derive(Debug, Clone)]
#[derive(Clone)]
struct QuicAuthId {
auth_value: Option<String>,
}

impl Debug for QuicAuthId {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
write!(
f,
"Common Name: {}",
self.auth_value.as_deref().unwrap_or("None")
)
}
}

impl From<QuicAuthId> for LinkAuthId {
fn from(value: QuicAuthId) -> Self {
LinkAuthId::builder()
Expand Down
8 changes: 8 additions & 0 deletions io/zenoh-links/zenoh-link-quic/src/utils.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -506,6 +506,14 @@ pub async fn get_quic_addr(address: &Address<'_>) -> ZResult<SocketAddr> {
}
}

pub fn get_quic_host<'a>(address: &'a Address<'a>) -> ZResult<&'a str> {
address
.as_str()
.split(':')
.next()
.ok_or_else(|| zerror!("Invalid QUIC address").into())
}

pub fn base64_decode(data: &str) -> ZResult<Vec<u8>> {
use base64::{engine::general_purpose, Engine};
Ok(general_purpose::STANDARD
Expand Down
30 changes: 26 additions & 4 deletions io/zenoh-links/zenoh-link-tls/src/unicast.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,14 @@
// Contributors:
// ZettaScale Zenoh Team, <[email protected]>
//
use std::{cell::UnsafeCell, convert::TryInto, fmt, net::SocketAddr, sync::Arc, time::Duration};
use std::{
cell::UnsafeCell,
convert::TryInto,
fmt::{self, Debug},
net::SocketAddr,
sync::Arc,
time::Duration,
};

use async_trait::async_trait;
use time::OffsetDateTime;
Expand Down Expand Up @@ -426,7 +433,12 @@ impl LinkManagerUnicastTrait for LinkManagerUnicastTls {
format!("{host}:{local_port}"),
endpoint.metadata(),
)?;

let endpoint = EndPoint::new(
locator.protocol(),
locator.address(),
Copy link
Contributor

@oteffahi oteffahi Jan 13, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

In QUIC link this is local_addr.to_string(). Not sure if it can be different from locator.address() in this case.

Copy link
Member Author

Choose a reason for hiding this comment

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

local_addr.to_string() allocates and then the creation of the EndPoint creation allocates a second time. locator.address() returns &str so it allocates one time less.

Copy link
Contributor

@oteffahi oteffahi Jan 13, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I was more concerned about if the contents of the two variables diverge. Indeed there is a case where they diverge, it seems that locator.address() contains the unresolved hostname, while local_addr contains the resolved one.

For example (on MacOS) when listening on localhost:0:

  • locator.address(): "localhost:49999"
  • local_addr.to_string(): "[::1]:49999"

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like multicast scouting fails to connect when the propagated locator is tls/[::1]:49999, so it's correct to use locator.address(). Will check if QUIC link also has this issue

Copy link
Member Author

Choose a reason for hiding this comment

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

The unresolved address, i.e. locator.address(), should be used in my opinion since the listener has been created on a DNS name. Propagating the already resolved address could lead to some hidden problem about DNS caches in zenoh. Let's every host resolve the IP address from the DNS name based on its own DNS configuration.

Copy link
Contributor

Choose a reason for hiding this comment

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

After some testing, I can confirm there is a similar issue on QUIC.

Copy link
Contributor

@oteffahi oteffahi Jan 13, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

local_addr.to_string() should be replaced by format!("{host}:{}", local_addr.port()) (after computing host variable like so).

zenoh/io/zenoh-links/zenoh-link-quic/src/unicast.rs

Lines 423 to 428 in 911382d

let endpoint = EndPoint::new(
endpoint.protocol(),
local_addr.to_string(),
endpoint.metadata(),
endpoint.config(),
)?;

Copy link
Member Author

Choose a reason for hiding this comment

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

Just added

locator.metadata(),
endpoint.config(),
)?;
self.listeners
.add_listener(endpoint, local_addr, task, token)
.await?;
Expand Down Expand Up @@ -491,14 +503,14 @@ async fn accept_task(
match get_cert_chain_expiration(&tls_conn.peer_certificates())? {
exp @ Some(_) => maybe_expiration_time = exp,
None => tracing::warn!(
"Cannot monitor expiration for TLS link {:?} => {:?} : client does not have certificates",
"Cannot monitor expiration for TLS link {:?} => {:?}: client does not have certificates",
src_addr,
dst_addr,
),
}
}

tracing::debug!("Accepted TLS connection on {:?}: {:?}", src_addr, dst_addr);
tracing::debug!("Accepted TLS connection on {:?}: {:?}. {:?}.", src_addr, dst_addr, auth_identifier);
// Create the new link object
let link = Arc::<LinkUnicastTls>::new_cyclic(|weak_link| {
let mut expiration_manager = None;
Expand Down Expand Up @@ -606,6 +618,16 @@ struct TlsAuthId {
auth_value: Option<String>,
}

impl Debug for TlsAuthId {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
write!(
f,
"Common Name: {}",
self.auth_value.as_deref().unwrap_or("None")
)
}
}

impl From<TlsAuthId> for LinkAuthId {
fn from(value: TlsAuthId) -> Self {
LinkAuthId::builder()
Expand Down
Loading