Skip to content

Commit

Permalink
Allow app to accept/reject/retry before handshake begins
Browse files Browse the repository at this point in the history
Summary of changes to `quinn` API
---

Removes use_retry setting from the server config.

`Endpoint::accept` now returns `Connecting` futures for which the server
has not yet responded to the client or allocated state. The application
can use the `Connecting` in a mostly normal way to accept the
connection, or can call `reject` or `retry` to reject/retry instead.

A server application could call methods such as `remote_address` and
`is_validated` to implement policies ranging from simply retrying non-
validated connections, thus emulating the old use_retry = true behavior,
to implementing various sorts of IP address blocking systems.

Of note is that this makes it possible for the server to fail to fully
process a connection-creating packet even after a corresponding
`Connecting` future has been delivered to the application. **This means
that it is now possible for `into_0rtt` to error for incoming
connections, which was not previously the case.**

The error type of `into_0rtt` is changed into a new `Into0RttError`
type which intentionally avoids implementing `Debug` so as to cause user
code which was blindly unwrap such errors in servers to fail to compile.
From what I can tell, other Rust QUIC implementations avoid this quirk
by not giving the application that much manual control.

Example
---

Additional options are added to the server and client example to
illustrate one reasonable way to use this new API. Running them as such
showcases IP blocking behavior:

    cargo run --example server ./ --listen 127.0.0.1:4433 --stateless-retry --block 127.0.0.1:8065
    cargo run --example client https://127.0.0.1:4433/Cargo.toml --host localhost --bind 127.0.0.1:8065

One thing to note is that that example places the reject condition
before the retry condition. This expends slightly less effort rejecting
connections, but does create a blocked IP address oracle for an attacker
who can do address spoofing.

Misc notes
---

- Quinn-proto has a different `IncomingConnection` type for incoming
  connection attempts for which the server has not yet allocated state.
  In quinn, the `Connecting` future does the work of abstracting over
  this transition.
- `DatagramEvent::NewConnection` now just contains `IncomingConnection`.
- For borrowing reasons, two fields of `State` have been factored out
  into `TransmitState`, those being the fields necessary to process a
  `proto::Transmit`, which now gets processed from more call sites than
  before.
- This commit establishes some new architectural precedents for some
  code in the `quinn::connection` module to hold a handle to the
  endpoint and calling methods on it that require it to lock its state.
- A server endpoint no longer accepts incoming connections
  automatically, the application must actually be taking them and doing
  stuff with them. This should not be a problem for real applications,
  but may break certain artificial tests.
- The `accept_after_close` test was completely removed because the
  functionality it was testing for is simply no longer supported.
- Adds some entries to the .gitignore I was using for LOC coverage
  testing.
  • Loading branch information
gretchenfrage committed Jan 30, 2024
1 parent 1c7c460 commit 4c64e85
Show file tree
Hide file tree
Showing 13 changed files with 874 additions and 320 deletions.
4 changes: 4 additions & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -6,3 +6,7 @@ Cargo.lock
.idea
.DS_Store
.vscode

cargo-test-*
tarpaulin-report.html

14 changes: 0 additions & 14 deletions quinn-proto/src/config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -741,10 +741,6 @@ pub struct ServerConfig {
/// Used to generate one-time AEAD keys to protect handshake tokens
pub(crate) token_key: Arc<dyn HandshakeTokenKey>,

/// Whether to require clients to prove ownership of an address before committing resources.
///
/// Introduces an additional round-trip to the handshake to make denial of service attacks more difficult.
pub(crate) use_retry: bool,
/// Microseconds after a stateless retry token was issued for which it's considered valid.
pub(crate) retry_token_lifetime: Duration,

Expand All @@ -769,7 +765,6 @@ impl ServerConfig {
crypto,

token_key,
use_retry: false,
retry_token_lifetime: Duration::from_secs(15),

concurrent_connections: 100_000,
Expand All @@ -790,14 +785,6 @@ impl ServerConfig {
self
}

/// Whether to require clients to prove ownership of an address before committing resources.
///
/// Introduces an additional round-trip to the handshake to make denial of service attacks more difficult.
pub fn use_retry(&mut self, value: bool) -> &mut Self {
self.use_retry = value;
self
}

/// 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;
Expand Down Expand Up @@ -858,7 +845,6 @@ impl fmt::Debug for ServerConfig {
.field("transport", &self.transport)
.field("crypto", &"ServerConfig { elided }")
.field("token_key", &"[ elided ]")
.field("use_retry", &self.use_retry)
.field("retry_token_lifetime", &self.retry_token_lifetime)
.field("concurrent_connections", &self.concurrent_connections)
.field("migration", &self.migration)
Expand Down
2 changes: 1 addition & 1 deletion quinn-proto/src/connection/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -254,6 +254,7 @@ impl Connection {
version: u32,
allow_mtud: bool,
rng_seed: [u8; 32],
path_validated: bool,
) -> Self {
let side = if server_config.is_some() {
Side::Server
Expand All @@ -270,7 +271,6 @@ impl Connection {
client_hello: None,
});
let mut rng = StdRng::from_seed(rng_seed);
let path_validated = server_config.as_ref().map_or(true, |c| c.use_retry);
let mut this = Self {
endpoint_config,
server_config,
Expand Down
Loading

0 comments on commit 4c64e85

Please sign in to comment.