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(s2n-quic-dc): derive crypto before opening TCP stream #2451

Merged
merged 1 commit into from
Jan 22, 2025
Merged

fix(s2n-quic-dc): derive crypto before opening TCP stream #2451

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
30 changes: 23 additions & 7 deletions dc/s2n-quic-dc/src/stream/client/tokio.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ use crate::{
endpoint,
environment::tokio::{self as env, Environment},
socket::Protocol,
TransportFeatures,
},
};
use std::{io, net::SocketAddr};
Expand All @@ -29,12 +30,15 @@ where
// ensure we have a secret for the peer
let peer = handshake.await?;

let (crypto, parameters) = peer.pair(&TransportFeatures::UDP);

let stream = endpoint::open_stream(
env,
peer,
peer.map(),
crypto,
parameters,
env::UdpUnbound(acceptor_addr.into()),
subscriber,
None,
)?;

// build the stream inside the application context
Expand All @@ -60,7 +64,14 @@ where
Sub: event::Subscriber,
{
// Race TCP handshake with the TLS handshake
let (socket, peer) = tokio::try_join!(TcpStream::connect(acceptor_addr), handshake,)?;
let handshake = async {
let peer = handshake.await?;
let (crypto, parameters) = peer.pair(&TransportFeatures::TCP);
Ok((peer, crypto, parameters))
};
// poll the crypto first so the server can read the first packet on accept in the happy path
Copy link
Collaborator

Choose a reason for hiding this comment

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

One thought I just had: this does mean that if the acceptor address is variable (e.g., because there are more than one server sharing the same endpoint) and it doesn't respond, we'd introduce a gap in our key ID sequence where previously that wouldn't happen.

I think that gap is acceptable though. If we wanted to avoid it we could probably try to have a tiny cache of derived keys, but that seems expensive in memory and not worth the hassle (at least until we actually have problems here).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it's still an issue either way, right? Cause we're not blocking the crypto derivation on the connect completing. We just want to issue the socket + connect syscalls after we derive keys. So before it would be:

socket
connect // EWOULDBLOCK
derive crypto
epoll(connect)
sendmsg

now it's

derive crypto
socket
connect // EWOULDBLOCK
epoll(connect)
sendmsg

Copy link
Collaborator

Choose a reason for hiding this comment

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

The crypto was in open_stream, right? So I think this is a marked change (including failed opens and slow opens). If it was as you say then our original attempt of just flipping order would have sufficed in practice (since we always hit the would block with tokio today).

Well, more accurately, tokio doesn't check that the stream is open immediately - that is deferred to epoll. Connect in non-blocking mode doesn't actually get called more than once.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The crypto was in open_stream, right?

Ah you're correct. Yeah that's a good callout.

let ((peer, crypto, parameters), socket) =
tokio::try_join!(handshake, TcpStream::connect(acceptor_addr))?;

// Make sure TCP_NODELAY is set
let _ = socket.set_nodelay(true);
Expand All @@ -77,14 +88,15 @@ where

let stream = endpoint::open_stream(
env,
peer,
peer.map(),
crypto,
parameters,
env::TcpRegistered {
socket,
peer_addr,
local_port,
},
subscriber,
None,
)?;

// build the stream inside the application context
Expand Down Expand Up @@ -114,16 +126,20 @@ where
{
let local_port = socket.local_addr()?.port();
let peer_addr = socket.peer_addr()?.into();

let (crypto, parameters) = peer.pair(&TransportFeatures::TCP);

let stream = endpoint::open_stream(
env,
peer,
peer.map(),
crypto,
parameters,
env::TcpRegistered {
socket,
peer_addr,
local_port,
},
subscriber,
None,
)?;

// build the stream inside the application context
Expand Down
15 changes: 5 additions & 10 deletions dc/s2n-quic-dc/src/stream/endpoint.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
use crate::{
event::{self, api::Subscriber as _, IntoEvent as _},
msg, packet,
path::secret::{self, map, Map},
path::secret::{self, Map},
random::Random,
stream::{
application,
Expand Down Expand Up @@ -35,21 +35,16 @@ pub struct AcceptError<Peer> {
#[inline]
pub fn open_stream<Env, P>(
env: &Env,
entry: map::Peer,
map: &Map,
crypto: secret::map::Bidirectional,
parameters: dc::ApplicationParams,
peer: P,
subscriber: Env::Subscriber,
parameter_override: Option<&dyn Fn(dc::ApplicationParams) -> dc::ApplicationParams>,
) -> Result<application::Builder<Env::Subscriber>>
where
Env: Environment,
P: Peer<Env>,
{
let (crypto, mut parameters) = entry.pair(&peer.features());

if let Some(o) = parameter_override {
parameters = o(parameters);
}

let key_id = crypto.credentials.key_id;
let stream_id = packet::stream::Id {
key_id,
Expand All @@ -74,7 +69,7 @@ where
stream_id,
None,
crypto,
entry.map(),
map,
parameters,
None,
None,
Expand Down
Loading