cloudflare / cloudflare/quiche

tokio-quiche doesn't propagate handshake errors

Open
#2,274 0 comments 1 reaction 0 assignees View on GitHub
Dominant language
Rust
Stars
11.8k
Forks
1.1k
Avg merge
3d 7h
Merged PRs (30d)
16

Description

When running `InitialQuicConnection.start(driver)` the handshake and resume is completed within. However, in my testing where mTLS fails due to incorrect host names (doesn't match Common Name in TLS Cert), the `.start(driver)` silently fails and what happens is my app assumes the quic handshake succeeded but it actually failed so it initializes the app logic and runs the tasks in them since there is no way to know if the handshake failed or not (to my knowledge).

To fix the issue, I implemented this in my app
```rust
async fn start_connection
(
initial_quic_connection: InitialQuicConnection,
driver: MyDriver
) -> io::Result
where
M: Metrics
{
let (q_conn, handshake) = initial_quic_connection.handshake(driver).await?;

metrics::tokio_task::spawn_with_killswitch(
"quic_handshake_worker",
DefaultMetrics, //can be cloned if used within the crate
async move {
InitialQuicConnection::::resume(handshake)
}
);

Ok(q_conn)
}
```
which is taken from
```rust
pub fn start(self, app: A) -> QuicConnection {
let task_metrics = self.params.metrics.clone();
let (conn, handshake_fut) = Self::handshake_fut(self, app);

let fut = async move {
match handshake_fut.await {
Ok(running) => Self::resume(running),
Err(e) => {
log::error!("QUIC handshake failed in IQC::start"; "error" => e)
},
}
};

crate::metrics::tokio_task::spawn_with_killswitch(
"quic_handshake_worker",
task_metrics,
fut,
);

conn
}
```

Why don't we create a new start function to support returning the result?
```rust
pub async fn start_with_result(self, app: A) -> io::Result {
let task_metrics = self.params.metrics.clone();
let result = self.handshake(app).await;

match result {
Ok((q_conn, handshake)) => {
crate::metrics::tokio_task::spawn_with_killswitch(
"quic_handshake_worker",
task_metrics,
async move {
Self::resume(handshake)
}
);

Ok(q_conn)
},
Err(err) => {
log::error!("QUIC handshake failed in IQC::start_with_result"; "error" => &err);
Err(err) // Pass it upward
}
}
}
```

This would propagate errors upward which can be handled by the user instead being silently logged while also maintaining the same logging behavior as `start()`.

Contributor guide

Open the contributing guide

Research direction

Trace InitialQuicConnection::start, handshake, and resume, focusing on how the handshake future is spawned through spawn_with_killswitch. Compare the existing start flow with the proposed result-returning flow; done means handshake errors reach the caller while successful connections retain the current resume and logging behavior.

Written by the indexing model from the issue text.

Assessment

Tech stack
rust
Domain
networking
Issue type
Bug
Difficulty
3/5
Estimated time
1-2 days
Activity status
Stale
Clarity
Mostly clear
Newbie friendliness
42/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.