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

feat: QUIC Address discovery extension #2043

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open
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
128 changes: 128 additions & 0 deletions quinn-proto/src/address_discovery.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,128 @@
//! Address discovery types from
//! <https://datatracker.ietf.org/doc/draft-seemann-quic-address-discovery/>

use crate::VarInt;

pub(crate) const TRANSPORT_PARAMETER_CODE: u64 = 0x9f81a176;
Copy link
Member

Choose a reason for hiding this comment

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

Nit: constants last in the file, please.


/// The role of each participant.
///
/// When enabled, this is reported as a transport parameter.
#[derive(PartialEq, Eq, Clone, Copy, Debug, Default)]
pub(crate) enum Role {
Copy link
Collaborator

Choose a reason for hiding this comment

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

It seems like we go to a lot of trouble to convert this to and from a pair of bools. Would it it be simpler to represent it that way in the first place?

Copy link
Member

Choose a reason for hiding this comment

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

I also don't love the Role name for something arguably pretty niche, should we make the type name AddressDiscoveryRole instead? Or maybe with the bool fields it's no longer really a Role, either?

/// Is able to report observer addresses to other peers, but it's not interested in receiving
/// reports about its own address.
SendOnly,
/// Is interested on reports about its own observed address, but will not report back to other
/// peers.
ReceiveOnly,
/// Will both report and receive reports of observed addresses.
Both,
/// Address discovery is disabled.
#[default]
Disabled,
}

impl TryFrom<VarInt> for Role {
type Error = crate::transport_parameters::Error;

fn try_from(value: VarInt) -> Result<Self, Self::Error> {
match value.0 {
0 => Ok(Self::SendOnly),
1 => Ok(Self::ReceiveOnly),
2 => Ok(Self::Both),
_ => Err(crate::transport_parameters::Error::IllegalValue),
}
}
}

impl Role {
/// Whether address discovery is disabled.
pub(crate) fn is_disabled(&self) -> bool {
matches!(self, Self::Disabled)
}

/// Whether this peer's role allows for address reporting to other peers.
fn is_reporter(&self) -> bool {
matches!(self, Self::SendOnly | Self::Both)
}

/// Whether this peer's role accepts observed address reports.
fn receives_reports(&self) -> bool {
matches!(self, Self::ReceiveOnly | Self::Both)
}

/// Whether this peer should report observed addresses to the other peer.
pub(crate) fn should_report(&self, other: &Self) -> bool {
self.is_reporter() && other.receives_reports()
}

/// Sets whether this peer should provide observed addresses to other peers.
pub(crate) fn send_reports_to_peers(&mut self, provide: bool) {
if provide {
self.enable_sending_reports_to_peers()
} else {
self.disable_sending_reports_to_peers()
}
}

/// Enables sending reports of observed addresses to other peers.
fn enable_sending_reports_to_peers(&mut self) {
match self {
Self::SendOnly => {} // already enabled
Self::ReceiveOnly => *self = Self::Both,
Self::Both => {} // already enabled
Self::Disabled => *self = Self::SendOnly,
}
}

/// Disables sending reports of observed addresses to other peers.
fn disable_sending_reports_to_peers(&mut self) {
match self {
Self::SendOnly => *self = Self::Disabled,
Self::ReceiveOnly => {} // already disabled
Self::Both => *self = Self::ReceiveOnly,
Self::Disabled => {} // already disabled
}
}

/// Sets whether this peer should accept received reports of observed addresses from other
/// peers.
pub(crate) fn receive_reports_from_peers(&mut self, receive: bool) {
if receive {
self.enable_receiving_reports_from_peers()
} else {
self.disable_receiving_reports_from_peers()
}
}

/// Enables receiving reports of observed addresses from other peers.
fn enable_receiving_reports_from_peers(&mut self) {
match self {
Self::SendOnly => *self = Self::Both,
Self::ReceiveOnly => {} // already enabled
Self::Both => {} // already enabled
Self::Disabled => *self = Self::ReceiveOnly,
}
}

/// Disables receiving reports of observed addresses from other peers.
fn disable_receiving_reports_from_peers(&mut self) {
match self {
Self::SendOnly => {} // already disabled
Self::ReceiveOnly => *self = Self::Disabled,
Self::Both => *self = Self::SendOnly,
Self::Disabled => {} // already disabled
}
}

/// Gives the [`VarInt`] representing this [`Role`] as a transport parameter.
pub(crate) fn as_transport_parameter(&self) -> Option<VarInt> {
match self {
Self::SendOnly => Some(VarInt(0)),
Self::ReceiveOnly => Some(VarInt(1)),
Self::Both => Some(VarInt(2)),
Self::Disabled => None,
}
}
}
28 changes: 28 additions & 0 deletions quinn-proto/src/config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ use thiserror::Error;
#[cfg(any(feature = "rustls-aws-lc-rs", feature = "rustls-ring"))]
use crate::crypto::rustls::{configured_provider, QuicServerConfig};
use crate::{
address_discovery,
cid_generator::{ConnectionIdGenerator, HashedConnectionIdGenerator},
congestion,
crypto::{self, HandshakeTokenKey, HmacKey},
Expand Down Expand Up @@ -63,6 +64,8 @@ pub struct TransportConfig {
pub(crate) congestion_controller_factory: Arc<dyn congestion::ControllerFactory + Send + Sync>,

pub(crate) enable_segmentation_offload: bool,

pub(crate) address_discovery_role: crate::address_discovery::Role,
}

impl TransportConfig {
Expand Down Expand Up @@ -334,6 +337,27 @@ impl TransportConfig {
self.enable_segmentation_offload = enabled;
self
}

/// Whether to send observed address reports to peers.
///
/// This will aid peers in inferring their reachable address, which in most NATd networks
/// will not be easily available to them.
pub fn send_observed_address_reports(&mut self, enabled: bool) -> &mut Self {
self.address_discovery_role.send_reports_to_peers(enabled);
self
}

/// Whether to receive observed address reports from other peers.
///
/// Peers with the address discovery extension enabled that are willing to provide observed
/// address reports will do so if this transport parameter is set. In general, observed address
/// reports cannot be trusted. This, however, can aid the current endpoint in inferring its
/// reachable address, which in most NATd networks will not be easily available.
pub fn receive_observed_address_reports(&mut self, enabled: bool) -> &mut Self {
self.address_discovery_role
.receive_reports_from_peers(enabled);
self
}
}

impl Default for TransportConfig {
Expand Down Expand Up @@ -374,6 +398,8 @@ impl Default for TransportConfig {
congestion_controller_factory: Arc::new(congestion::CubicConfig::default()),

enable_segmentation_offload: true,

address_discovery_role: address_discovery::Role::default(),
}
}
}
Expand Down Expand Up @@ -405,6 +431,7 @@ impl fmt::Debug for TransportConfig {
deterministic_packet_numbers: _,
congestion_controller_factory: _,
enable_segmentation_offload,
address_discovery_role,
} = self;
fmt.debug_struct("TransportConfig")
.field("max_concurrent_bidi_streams", max_concurrent_bidi_streams)
Expand Down Expand Up @@ -432,6 +459,7 @@ impl fmt::Debug for TransportConfig {
.field("datagram_send_buffer_size", datagram_send_buffer_size)
.field("congestion_controller_factory", &"[ opaque ]")
.field("enable_segmentation_offload", enable_segmentation_offload)
.field("address_discovery_role", address_discovery_role)
.finish()
}
}
Expand Down
Loading
Loading