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

Utilize NEW_TOKEN frames #1912

Draft
wants to merge 21 commits into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from 20 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
77 changes: 66 additions & 11 deletions quinn-proto/src/config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ use std::{
fmt,
net::{SocketAddrV4, SocketAddrV6},
num::TryFromIntError,
sync::Arc,
sync::{Arc, Mutex},
time::Duration,
};

Expand All @@ -20,6 +20,8 @@ use crate::{
cid_generator::{ConnectionIdGenerator, HashedConnectionIdGenerator},
congestion,
crypto::{self, HandshakeTokenKey, HmacKey},
new_token_store::{InMemNewTokenStore, NewTokenStore},
token_reuse_preventer::{BloomTokenReusePreventer, TokenReusePreventer},
shared::ConnectionId,
RandomConnectionIdGenerator, VarInt, VarIntBoundsExceeded, DEFAULT_SUPPORTED_VERSIONS,
INITIAL_MTU, MAX_CID_SIZE, MAX_UDP_PAYLOAD,
Expand Down Expand Up @@ -779,17 +781,26 @@ pub struct ServerConfig {
/// Transport configuration to use for incoming connections
pub transport: Arc<TransportConfig>,

/// TLS configuration used for incoming connections.
/// TLS configuration used for incoming connections
///
/// Must be set to use TLS 1.3 only.
pub crypto: Arc<dyn crypto::ServerConfig>,

/// Used to generate one-time AEAD keys to protect handshake tokens
pub(crate) token_key: Arc<dyn HandshakeTokenKey>,

/// Microseconds after a stateless retry token was issued for which it's considered valid.
/// Duration after a stateless retry token was issued for which it's considered valid
pub(crate) retry_token_lifetime: Duration,

/// Duration after a NEW_TOKEN frame token was issued for which it's considered valid
pub(crate) new_token_lifetime: Duration,

/// Responsible for limiting clients' ability to reuse tokens from NEW_TOKEN frames
pub(crate) token_reuse_preventer: Option<Arc<Mutex<Box<dyn TokenReusePreventer>>>>,

/// Number of NEW_TOKEN frames sent to a client when its path is validated.
pub(crate) new_tokens_sent_upon_validation: u32,

/// Whether to allow clients to migrate to new addresses
///
/// Improves behavior for clients that move between different internet connections or suffer NAT
Expand All @@ -806,16 +817,22 @@ pub struct ServerConfig {

impl ServerConfig {
/// Create a default config with a particular handshake token key
///
/// Setting `token_reuse_preventer` to `None` makes the server ignore all NEW_HANDSHAKE tokens.
Copy link
Collaborator

Choose a reason for hiding this comment

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

What's a NEW_HANDSHAKE token?

pub fn new(
crypto: Arc<dyn crypto::ServerConfig>,
token_key: Arc<dyn HandshakeTokenKey>,
token_reuse_preventer: Option<Box<dyn TokenReusePreventer>>,
) -> Self {
Self {
transport: Arc::new(TransportConfig::default()),
crypto,

token_key,
retry_token_lifetime: Duration::from_secs(15),
new_token_lifetime: Duration::from_secs(2 * 7 * 24 * 60 * 60),
token_reuse_preventer: token_reuse_preventer.map(Mutex::new).map(Arc::new),
new_tokens_sent_upon_validation: 2,

migration: true,

Expand All @@ -834,18 +851,32 @@ impl ServerConfig {
self
}

/// Private key used to authenticate data included in handshake tokens.
/// Private key used to authenticate data included in handshake tokens
pub fn token_key(&mut self, value: Arc<dyn HandshakeTokenKey>) -> &mut Self {
self.token_key = value;
self
}

/// Duration after a stateless retry token was issued for which it's considered valid.
/// Duration after a stateless retry token was issued for which it's considered valid
pub fn retry_token_lifetime(&mut self, value: Duration) -> &mut Self {
self.retry_token_lifetime = value;
self
}

/// Duration after a NEW_TOKEN frame token was issued for which it's considered valid
pub fn new_token_lifetime(&mut self, value: Duration) -> &mut Self {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would it be clearer to refer to these as "address validation tokens"? "new token" doesn't mean anything to someone who hasn't studied the RFC.

self.new_token_lifetime = value;
self
}

/// Number of tokens issue to clients in NEW_TOKEN frames when their path is validated
///
/// Defaults to 2.
pub fn new_tokens_sent_upon_validation(&mut self, value: u32) -> &mut Self {
self.new_tokens_sent_upon_validation = value;
self
}

/// Whether to allow clients to migrate to new addresses
///
/// Improves behavior for clients that move between different internet connections or suffer NAT
Expand Down Expand Up @@ -937,24 +968,31 @@ impl ServerConfig {
impl ServerConfig {
/// Create a server config with the given [`crypto::ServerConfig`]
///
/// Uses a randomized handshake token key.
/// Uses a randomized handshake token key and a default `BloomTokenReusePreventer`.
pub fn with_crypto(crypto: Arc<dyn crypto::ServerConfig>) -> Self {
let rng = &mut rand::thread_rng();
let mut master_key = [0u8; 64];
rng.fill_bytes(&mut master_key);
let master_key = ring::hkdf::Salt::new(ring::hkdf::HKDF_SHA256, &[]).extract(&master_key);

Self::new(crypto, Arc::new(master_key))
Self::new(
crypto,
Arc::new(master_key),
Some(Box::new(BloomTokenReusePreventer::default())),
)
}
}

impl fmt::Debug for ServerConfig {
fn fmt(&self, fmt: &mut fmt::Formatter<'_>) -> fmt::Result {
fmt.debug_struct("ServerConfig<T>")
.field("transport", &self.transport)
.field("crypto", &"ServerConfig { elided }")
.field("token_key", &"[ elided ]")
.field("retry_token_lifetime", &self.retry_token_lifetime)
.field("new_token_lifetime", &self.new_token_lifetime)
.field(
"new_tokens_sent_upon_validation",
&self.new_tokens_sent_upon_validation,
)
.field("migration", &self.migration)
.field("preferred_address_v4", &self.preferred_address_v4)
.field("preferred_address_v6", &self.preferred_address_v6)
Expand All @@ -964,7 +1002,7 @@ impl fmt::Debug for ServerConfig {
"incoming_buffer_size_total",
&self.incoming_buffer_size_total,
)
.finish()
.finish_non_exhaustive()
}
}

Expand All @@ -980,6 +1018,9 @@ pub struct ClientConfig {
/// Cryptographic configuration to use
pub(crate) crypto: Arc<dyn crypto::ClientConfig>,

/// New token store to use
pub(crate) new_token_store: Option<Arc<dyn NewTokenStore>>,

/// Provider that populates the destination connection ID of Initial Packets
pub(crate) initial_dst_cid_provider: Arc<dyn Fn() -> ConnectionId + Send + Sync>,

Expand All @@ -993,6 +1034,7 @@ impl ClientConfig {
Self {
transport: Default::default(),
crypto,
new_token_store: Some(Arc::new(InMemNewTokenStore::<2>::default())),
initial_dst_cid_provider: Arc::new(|| {
RandomConnectionIdGenerator::new(MAX_CID_SIZE).generate_cid()
}),
Expand Down Expand Up @@ -1022,6 +1064,18 @@ impl ClientConfig {
self
}

/// Set a custom [`NewTokenStore`]
///
/// Defaults to an in-memory store limited to 256 servers and 2 tokens per server. Setting to
/// `None` disables the use of tokens from NEW_TOKEN frames as a client.
pub fn new_token_store(
&mut self,
new_token_store: Option<Arc<dyn NewTokenStore>>,
) -> &mut Self {
self.new_token_store = new_token_store;
self
}

/// Set the QUIC version to use
pub fn version(&mut self, version: u32) -> &mut Self {
self.version = version;
Expand Down Expand Up @@ -1053,7 +1107,8 @@ impl fmt::Debug for ClientConfig {
fn fmt(&self, fmt: &mut fmt::Formatter<'_>) -> fmt::Result {
fmt.debug_struct("ClientConfig<T>")
.field("transport", &self.transport)
.field("crypto", &"ClientConfig { elided }")
// crypto isn't debug
// new token store isn't debug
.field("version", &self.version)
.finish_non_exhaustive()
}
Expand Down
82 changes: 70 additions & 12 deletions quinn-proto/src/connection/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ use std::{
fmt, io, mem,
net::{IpAddr, SocketAddr},
sync::Arc,
time::{Duration, Instant},
time::{Duration, Instant, SystemTime},
};

use bytes::{Bytes, BytesMut};
Expand All @@ -21,7 +21,8 @@ use crate::{
config::{ServerConfig, TransportConfig},
crypto::{self, KeyPair, Keys, PacketKey},
frame,
frame::{Close, Datagram, FrameStruct},
frame::{Close, Datagram, FrameStruct, NewToken},
new_token_store::NewTokenStore,
packet::{
FixedLengthConnectionIdParser, Header, InitialHeader, InitialPacket, LongType, Packet,
PacketNumber, PartialDecode, SpaceId,
Expand All @@ -31,7 +32,7 @@ use crate::{
ConnectionEvent, ConnectionEventInner, ConnectionId, DatagramConnectionEvent, EcnCodepoint,
EndpointEvent, EndpointEventInner,
},
token::ResetToken,
token::{NewTokenToken, ResetToken},
transport_parameters::TransportParameters,
Dir, EndpointConfig, Frame, Side, StreamId, Transmit, TransportError, TransportErrorCode,
VarInt, MAX_STREAM_COUNT, MIN_INITIAL_SIZE, TIMER_GRANULARITY,
Expand Down Expand Up @@ -194,7 +195,7 @@ pub struct Connection {
error: Option<ConnectionError>,
/// Sent in every outgoing Initial packet. Always empty for servers and after Initial keys are
/// discarded.
retry_token: Bytes,
token: Bytes,
/// Identifies Data-space packet numbers to skip. Not used in earlier spaces.
packet_number_filter: PacketNumberFilter,

Expand Down Expand Up @@ -227,6 +228,9 @@ pub struct Connection {
/// no outgoing application data.
app_limited: bool,

new_token_store: Option<Arc<dyn NewTokenStore>>,
server_name: Option<String>,

streams: StreamsState,
/// Surplus remote CIDs for future use on new paths
rem_cids: CidQueue,
Expand Down Expand Up @@ -258,6 +262,8 @@ impl Connection {
allow_mtud: bool,
rng_seed: [u8; 32],
path_validated: bool,
new_token_store: Option<Arc<dyn NewTokenStore>>,
server_name: Option<String>,
) -> Self {
let side = if server_config.is_some() {
Side::Server
Expand All @@ -274,6 +280,16 @@ impl Connection {
client_hello: None,
});
let mut rng = StdRng::from_seed(rng_seed);
let mut token = Bytes::new();
if let Some(new_token_store) = new_token_store.as_ref() {
if let Some(new_token) = new_token_store.take(server_name.as_ref().unwrap()) {
token = new_token;
}
}
let new_tokens_to_send = server_config
.as_ref()
.map(|sc| sc.new_tokens_sent_upon_validation)
.unwrap_or(0);
Comment on lines +289 to +292
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we assert that we don't have both a server config and a token store? Or maybe pass them in an enum?

let mut this = Self {
endpoint_config,
server_config,
Expand All @@ -286,7 +302,15 @@ impl Connection {
now,
if pref_addr_cid.is_some() { 2 } else { 1 },
),
path: PathData::new(remote, allow_mtud, None, now, path_validated, &config),
path: PathData::new(
remote,
allow_mtud,
None,
now,
path_validated,
&config,
new_tokens_to_send,
),
allow_mtud,
local_ip,
prev_path: None,
Expand Down Expand Up @@ -321,7 +345,7 @@ impl Connection {
timers: TimerTable::default(),
authentication_failures: 0,
error: None,
retry_token: Bytes::new(),
token,
#[cfg(test)]
packet_number_filter: match config.deterministic_packet_numbers {
false => PacketNumberFilter::new(&mut rng),
Expand All @@ -343,6 +367,9 @@ impl Connection {
receiving_ecn: false,
total_authed_packets: 0,

new_token_store,
server_name,

streams: StreamsState::new(
side,
config.max_concurrent_uni_streams,
Expand Down Expand Up @@ -2079,7 +2106,7 @@ impl Connection {
trace!("discarding {:?} keys", space_id);
if space_id == SpaceId::Initial {
// No longer needed
self.retry_token = Bytes::new();
self.token = Bytes::new();
}
let space = &mut self.spaces[space_id];
space.crypto = None;
Expand Down Expand Up @@ -2395,7 +2422,7 @@ impl Connection {
self.streams.retransmit_all_for_0rtt();

let token_len = packet.payload.len() - 16;
self.retry_token = packet.payload.freeze().split_to(token_len);
self.token = packet.payload.freeze().split_to(token_len);
self.state = State::Handshake(state::Handshake {
expected_token: Bytes::new(),
rem_cid_set: false,
Expand Down Expand Up @@ -2829,15 +2856,17 @@ impl Connection {
self.update_rem_cid();
}
}
Frame::NewToken { token } => {
Frame::NewToken(new_token) => {
if self.side.is_server() {
return Err(TransportError::PROTOCOL_VIOLATION("client sent NEW_TOKEN"));
}
if token.is_empty() {
if new_token.token.is_empty() {
return Err(TransportError::FRAME_ENCODING_ERROR("empty token"));
}
trace!("got new token");
// TODO: Cache, or perhaps forward to user?
if let Some(new_token_store) = self.new_token_store.as_ref() {
new_token_store.store(self.server_name.as_ref().unwrap(), new_token.token);
}
}
Frame::Datagram(datagram) => {
if self
Expand Down Expand Up @@ -2947,6 +2976,10 @@ impl Connection {
now,
false,
&self.config,
self.server_config
.as_ref()
.map(|sc| sc.new_tokens_sent_upon_validation)
.unwrap_or(0),
)
};
new_path.challenge = Some(self.rng.gen());
Expand Down Expand Up @@ -3222,8 +3255,33 @@ impl Connection {
self.datagrams.send_blocked = false;
}

// STREAM
if space_id == SpaceId::Data {
// NEW_TOKEN
while self.path.new_tokens_to_send > 0 {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Relying on PathData like this will cause lost NEW_TOKEN frames to not be retransmitted. Consider also adding a field to Retransmits and coordinating it with path changes.

Copy link
Collaborator

Choose a reason for hiding this comment

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

These tokens look like they have predictable sizes. Should we skip this branch up front if there's not enough space?

let new_token = self.path.pending_new_token.take().unwrap_or_else(|| {
let token = NewTokenToken {
rand: self.rng.gen(),
issued: SystemTime::now(),
}
.encode(
&*self.server_config.as_ref().unwrap().token_key,
&self.path.remote.ip(),
);
NewToken {
token: token.into(),
}
});

if buf.len() + new_token.size() >= max_size {
self.path.pending_new_token = Some(new_token);
Copy link
Collaborator

Choose a reason for hiding this comment

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

These look like stateless tokens. Why bother saving them if we can't fit one?

break;
}

new_token.encode(buf);
self.path.new_tokens_to_send -= 1;
}

// STREAM
sent.stream_frames = self.streams.write_stream_frames(buf, max_size);
self.stats.frame_tx.stream += sent.stream_frames.len() as u64;
}
Expand Down
2 changes: 1 addition & 1 deletion quinn-proto/src/connection/packet_builder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -122,7 +122,7 @@ impl PacketBuilder {
SpaceId::Initial => Header::Initial(InitialHeader {
src_cid: conn.handshake_cid,
dst_cid,
token: conn.retry_token.clone(),
token: conn.token.clone(),
number,
version,
}),
Expand Down
Loading
Loading