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 2 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
21 changes: 11 additions & 10 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 @@ -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
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
10 changes: 8 additions & 2 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 @@ -426,7 +426,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,7 +496,7 @@ 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,
),
Expand Down Expand Up @@ -602,6 +607,7 @@ fn get_cert_chain_expiration(
Ok(link_expiration)
}

#[derive(Debug)]
struct TlsAuthId {
auth_value: Option<String>,
}
Expand Down
Loading