From f9f9c8574aa6945cb9d70953a9a0a900ab2a87fb Mon Sep 17 00:00:00 2001 From: Kayshen-X Date: Sat, 1 Aug 2026 09:48:23 +0800 Subject: [PATCH] fix(collab): harden p2p collaboration against resource exhaustion Addresses a security review of the collaboration subsystem. No auth bypass, key leak, or document-plaintext exposure was found; every finding below is availability or trust-boundary hardening. Landed as one commit because the pieces are not separable: the inbound-direction ceiling spans op-collab, op-collab-transport, and the desktop host atomically, the guarded accept spans transport, smoke, and the desktop host, and the boundary-gate rules only hold against the final state. Splitting would produce commits that fail to build or fail the gate. Relay server (public, internet-facing): - Charge pre-pairing capacity per source address. The auth-concurrency semaphore was taken before the WebSocket upgrade and the peer address was discarded, so one host could pin every permit by connecting and going silent. - Give renewals their own budget. Reauthentication competed for the same semaphore, so an unauthenticated flood progressively closed live tunnels with a policy error. - Release the pair registration when the ready status fails to send; the counterpart only reclaims it if it reads its pairing notice. - Require the X25519 key file to be owned by the running user; mode bits alone do not establish trust. - Summarise capacity rejections instead of logging one line each. Locator service: - Rate-limit publishes per client instead of process-wide. One unauthenticated caller could consume the whole budget and 429 every tenant's invite issuance. Collaboration protocol: - Size the inbound envelope ceiling from the authenticated remote role rather than sharing the 64 MiB snapshot ceiling in both directions, so an admitted guest cannot force a 64 MiB JSON parse per frame. The ceiling is applied before the discriminator and before the generic value decode; a peer-declared snapshot kind cannot raise it. - Reject display names carrying Unicode format characters, which render identically to an existing participant's name. - Reject avatar URLs pointing at non-globally-routable addresses. Transport: - Reclaim a pending-handshake seat from a peer that has not produced a valid first handshake message, and raise the global ceiling. Sixteen seats held for the full handshake window let four addresses deny every join. - Put inbound reassembly under an aggregate budget; only the outbound aggregate was bounded. - Stop heartbeats from refreshing the idle deadline in receive_transfer. - Filter IPv4 link-local discovery advertisements, matching IPv6. Relay client and trust roots: - Bound server-initiated reauthentication per connection by count and minimum interval, sized from the protocol's own cadence. - Close the policy-file TOCTOU window by identity-checking the opened file, and reject group/world-writable or foreign-owned policy files. - Stop discarding bootstrap cache-write failures, which silently disabled the anti-rollback generation floor. --- .github/workflows/collab-security.yml | 5 +- crates/op-collab-policy-file/src/lib.rs | 154 ++++++- crates/op-collab-policy-file/src/tests.rs | 146 +++++- .../src/bridge/guest.rs | 6 + .../src/bridge/owner.rs | 6 + .../src/bridge/protocol.rs | 10 + crates/op-collab-relay-client/src/error.rs | 6 + crates/op-collab-relay-client/src/lib.rs | 1 + .../src/reauth_budget.rs | 179 ++++++++ crates/op-collab-relay-client/src/session.rs | 23 +- .../src/publish.rs | 8 + .../src/config.rs | 54 +++ .../op-collab-relay-locator-server/src/hsm.rs | 6 +- .../src/http.rs | 72 ++- .../src/http/rate_limit.rs | 136 ++++++ .../src/production.rs | 7 + .../src/tests.rs | 147 +++++- crates/op-collab-relay-server/src/config.rs | 23 + .../op-collab-relay-server/src/connection.rs | 48 +- .../src/connection_reauth.rs | 4 +- crates/op-collab-relay-server/src/lib.rs | 1 + .../op-collab-relay-server/src/peer_quota.rs | 123 +++++ crates/op-collab-relay-server/src/server.rs | 91 +++- crates/op-collab-relay-server/src/tests.rs | 81 ++++ .../src/x25519_boundary.rs | 14 +- crates/op-collab-smoke/src/owner.rs | 4 +- crates/op-collab-transport/README.md | 13 +- crates/op-collab-transport/src/chunk.rs | 385 +++------------- crates/op-collab-transport/src/chunk_tests.rs | 432 ++++++++++++++++++ crates/op-collab-transport/src/config.rs | 82 +++- .../src/connection_limit.rs | 178 +++++--- .../src/connection_limit_tests.rs | 189 ++++++++ crates/op-collab-transport/src/discovery.rs | 4 + .../src/discovery_tests.rs | 44 ++ crates/op-collab-transport/src/driver.rs | 16 +- crates/op-collab-transport/src/error.rs | 7 + crates/op-collab-transport/src/frame.rs | 48 +- crates/op-collab-transport/src/lib.rs | 13 +- crates/op-collab-transport/src/noise.rs | 15 + .../src/reassembly_budget.rs | 152 ++++++ crates/op-collab-transport/src/runtime.rs | 127 ++--- .../op-collab-transport/src/runtime_tests.rs | 204 +++++++++ crates/op-collab-transport/src/tcp.rs | 187 +++++++- crates/op-collab/src/codec.rs | 41 +- crates/op-collab/src/error.rs | 4 + crates/op-collab/src/frame_direction.rs | 126 +++++ crates/op-collab/src/guest_sync.rs | 16 +- crates/op-collab/src/lib.rs | 6 + crates/op-collab/src/profile_validation.rs | 217 +++++++++ crates/op-collab/src/protocol.rs | 86 +--- crates/op-collab/src/session_state.rs | 13 +- crates/op-collab/src/ticket_json.rs | 9 +- crates/op-collab/tests/outbound_limits.rs | 91 +++- crates/op-collab/tests/profile.rs | 106 ++++- .../src/collab_runtime/network/owner.rs | 15 +- .../src/collab_runtime/relay_bootstrap.rs | 117 ++--- .../collab_runtime/relay_bootstrap_cache.rs | 188 ++++++++ .../collab_runtime/relay_bootstrap_tests.rs | 174 +++++++ .../src/collab_runtime/types.rs | 10 +- tools/check-collab-deployment-boundaries.sh | 2 + .../check-collab-security-boundaries-cases.sh | 24 +- tools/check-collab-security-boundaries.sh | 36 +- .../check-collab-security-boundaries.test.sh | 139 +++--- 63 files changed, 3999 insertions(+), 872 deletions(-) create mode 100644 crates/op-collab-relay-client/src/reauth_budget.rs create mode 100644 crates/op-collab-relay-locator-server/src/http/rate_limit.rs create mode 100644 crates/op-collab-relay-server/src/peer_quota.rs create mode 100644 crates/op-collab-transport/src/chunk_tests.rs create mode 100644 crates/op-collab-transport/src/connection_limit_tests.rs create mode 100644 crates/op-collab-transport/src/reassembly_budget.rs create mode 100644 crates/op-collab-transport/src/runtime_tests.rs create mode 100644 crates/op-collab/src/frame_direction.rs create mode 100644 crates/op-collab/src/profile_validation.rs create mode 100644 crates/op-host-desktop/src/collab_runtime/relay_bootstrap_cache.rs mode change 100644 => 100755 tools/check-collab-security-boundaries.test.sh diff --git a/.github/workflows/collab-security.yml b/.github/workflows/collab-security.yml index c0f392551..80bb6f5cf 100644 --- a/.github/workflows/collab-security.yml +++ b/.github/workflows/collab-security.yml @@ -87,7 +87,7 @@ jobs: collaboration-security: name: Static boundaries and targeted tests runs-on: ubuntu-latest - timeout-minutes: 20 + timeout-minutes: 45 steps: - uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2 with: @@ -105,6 +105,8 @@ jobs: run: | bash -n tools/check-collab-security-boundaries.sh bash -n tools/check-collab-security-boundaries.test.sh + bash -n tools/check-collab-security-boundaries-cases.sh + bash -n tools/check-collab-deployment-boundaries.sh bash -n tools/check-op-auth-prebuilt.sh bash -n tools/check-op-auth-prebuilt.test.sh bash -n tools/package-op-auth-prebuilt.sh @@ -132,6 +134,7 @@ jobs: - name: Test protocol state machines, properties, and resource limits run: | cargo test --locked -p op-collab + cargo test --locked -p op-collab-transport cargo test --locked -p op-collab-transport config::tests cargo test --locked -p op-collab-transport frame::tests cargo test --locked -p op-collab-relay-protocol --all-features diff --git a/crates/op-collab-policy-file/src/lib.rs b/crates/op-collab-policy-file/src/lib.rs index 415f31adc..3ef48c9ae 100644 --- a/crates/op-collab-policy-file/src/lib.rs +++ b/crates/op-collab-policy-file/src/lib.rs @@ -2,7 +2,7 @@ use std::{ fmt, - fs::{File, OpenOptions}, + fs::{File, Metadata, OpenOptions}, io::{Read, Take}, num::NonZeroU64, path::{Path, PathBuf}, @@ -15,12 +15,67 @@ use op_auth_bridge::{ const FILE_ETAG_CONTEXT: &str = "openpencil/op-collab-policy-file/pinned-policy-file-etag/v1"; +/// Typed rejection reasons for a policy file used as a trust root. +/// +/// Each variant names one distinct reason the file was refused so callers and +/// operators can tell an unreadable path apart from an unsafe one. +#[derive(Clone, Copy, Debug, Eq, PartialEq)] +pub enum PolicyFileTrustError { + /// The path could not be inspected, opened, or read. + Unavailable, + /// The final path component is a symbolic link. + Symlink, + /// The opened object is not a regular file. + NotRegularFile, + /// The object that was opened is not the object that was inspected before + /// the open — the path was swapped between the two syscalls. + OpenedObjectChanged, + /// The file is writable by its group or by other users, so it is not a + /// trustworthy source of verification keys. + GroupOrWorldWritable, + /// The file is owned by neither root nor the user running this process. + ForeignOwner, + /// The file is larger than the caller's maximum. + TooLarge, +} + +impl fmt::Display for PolicyFileTrustError { + fn fmt(&self, formatter: &mut fmt::Formatter<'_>) -> fmt::Result { + formatter.write_str(match self { + Self::Unavailable => "policy file is unavailable", + Self::Symlink => "policy file path is a symbolic link", + Self::NotRegularFile => "policy file is not a regular file", + Self::OpenedObjectChanged => "policy file changed between inspection and open", + Self::GroupOrWorldWritable => "policy file is group- or world-writable", + Self::ForeignOwner => "policy file is owned by neither root nor the running user", + Self::TooLarge => "policy file is larger than the requested maximum", + }) + } +} + +impl std::error::Error for PolicyFileTrustError {} + +impl From for CollabJwksFetchError { + fn from(error: PolicyFileTrustError) -> Self { + match error { + PolicyFileTrustError::Unavailable => Self::Unavailable, + PolicyFileTrustError::TooLarge => Self::ResponseTooLarge, + PolicyFileTrustError::Symlink + | PolicyFileTrustError::NotRegularFile + | PolicyFileTrustError::OpenedObjectChanged + | PolicyFileTrustError::GroupOrWorldWritable + | PolicyFileTrustError::ForeignOwner => Self::RejectedResponse, + } + } +} + /// Reads one operator-pinned signed policy file without following a final /// symlink. /// /// The configured endpoint is compared byte-for-byte with each verifier /// request. File metadata and reads are bounded by the verifier's requested -/// maximum. Unix opens additionally use `O_NOFOLLOW | O_CLOEXEC`. +/// maximum. See [`read_bounded_trust_root_file`] for the exact trust-root +/// checks, including the reduced guarantee on non-Unix platforms. pub struct PinnedPolicyFileFetcher { endpoint: String, path: PathBuf, @@ -80,37 +135,112 @@ impl fmt::Debug for PinnedPolicyFileFetcher { } } +/// [`read_bounded_trust_root_file`] with the rejection reason collapsed into +/// the verifier's fetch error, for callers that speak `CollabJwksFetchError`. pub fn read_bounded_regular_file( path: &Path, maximum: usize, ) -> Result, CollabJwksFetchError> { + read_bounded_trust_root_file(path, maximum).map_err(CollabJwksFetchError::from) +} + +/// Reads a bounded trust-root file, rejecting every unsafe source shape. +/// +/// On Unix the open uses `O_NOFOLLOW | O_CLOEXEC`, and the opened descriptor's +/// `st_dev` / `st_ino` are compared with the pre-open `symlink_metadata` so a +/// path swapped between the two syscalls is refused instead of trusted. The +/// file must additionally be owned by root or by the running user and must not +/// be writable by its group or by other users, because anyone who can rewrite +/// it can replace the verification keys. This is public policy material, so a +/// root-owned `0440` or `0444` file is safe when the process can read it. +/// +/// **Non-Unix platforms provide a strictly weaker guarantee.** There is no +/// `O_NOFOLLOW` equivalent applied here, no stable device/inode identity to +/// re-check after the open, and no ownership or permission model that maps +/// onto the Unix checks. The symlink test therefore remains a plain +/// check-then-open, and a same-privilege attacker who can replace the path +/// between the two syscalls is not detected. Deployments that need the full +/// guarantee must run the policy-file source on Unix. +pub fn read_bounded_trust_root_file( + path: &Path, + maximum: usize, +) -> Result, PolicyFileTrustError> { let link_metadata = - std::fs::symlink_metadata(path).map_err(|_| CollabJwksFetchError::Unavailable)?; + std::fs::symlink_metadata(path).map_err(|_| PolicyFileTrustError::Unavailable)?; if link_metadata.file_type().is_symlink() { - return Err(CollabJwksFetchError::RejectedResponse); + return Err(PolicyFileTrustError::Symlink); } let file = open_no_follow(path)?; let metadata = file .metadata() - .map_err(|_| CollabJwksFetchError::Unavailable)?; + .map_err(|_| PolicyFileTrustError::Unavailable)?; if !metadata.file_type().is_file() { - return Err(CollabJwksFetchError::RejectedResponse); + return Err(PolicyFileTrustError::NotRegularFile); } + verify_trust_root_source(&link_metadata, &metadata)?; if metadata.len() > maximum as u64 { - return Err(CollabJwksFetchError::ResponseTooLarge); + return Err(PolicyFileTrustError::TooLarge); } let mut reader = file.take((maximum as u64).saturating_add(1)); let mut body = Vec::with_capacity(metadata.len() as usize); read_all(&mut reader, &mut body)?; if body.len() > maximum { - return Err(CollabJwksFetchError::ResponseTooLarge); + return Err(PolicyFileTrustError::TooLarge); } Ok(body) } -fn open_no_follow(path: &Path) -> Result { +/// Closes the check-then-open window and enforces trust-root ownership and +/// permissions against the descriptor that was actually opened. +#[cfg(unix)] +fn verify_trust_root_source( + link_metadata: &Metadata, + opened_metadata: &Metadata, +) -> Result<(), PolicyFileTrustError> { + use std::os::unix::fs::MetadataExt as _; + + // Identity is compared on the opened descriptor, not on a second path + // lookup, so a rename/symlink swap between the two syscalls is detected + // rather than merely made unlikely. + if link_metadata.dev() != opened_metadata.dev() || link_metadata.ino() != opened_metadata.ino() + { + return Err(PolicyFileTrustError::OpenedObjectChanged); + } + // SAFETY: `geteuid` takes no arguments, mutates no state, and cannot fail. + verify_unix_owner_and_mode(opened_metadata.uid(), opened_metadata.mode(), unsafe { + libc::geteuid() + }) +} + +#[cfg(unix)] +fn verify_unix_owner_and_mode( + file_uid: libc::uid_t, + file_mode: u32, + effective_uid: libc::uid_t, +) -> Result<(), PolicyFileTrustError> { + if file_mode & 0o022 != 0 { + return Err(PolicyFileTrustError::GroupOrWorldWritable); + } + if file_uid != effective_uid && file_uid != 0 { + return Err(PolicyFileTrustError::ForeignOwner); + } + Ok(()) +} + +/// Non-Unix builds have no device/inode identity, ownership, or writability +/// model to check here; see [`read_bounded_trust_root_file`] for the reduced +/// guarantee this leaves in place. +#[cfg(not(unix))] +fn verify_trust_root_source( + _link_metadata: &Metadata, + _opened_metadata: &Metadata, +) -> Result<(), PolicyFileTrustError> { + Ok(()) +} + +fn open_no_follow(path: &Path) -> Result { let mut options = OpenOptions::new(); options.read(true); #[cfg(unix)] @@ -120,14 +250,14 @@ fn open_no_follow(path: &Path) -> Result { } options .open(path) - .map_err(|_| CollabJwksFetchError::Unavailable) + .map_err(|_| PolicyFileTrustError::Unavailable) } -fn read_all(reader: &mut Take, body: &mut Vec) -> Result<(), CollabJwksFetchError> { +fn read_all(reader: &mut Take, body: &mut Vec) -> Result<(), PolicyFileTrustError> { reader .read_to_end(body) .map(|_| ()) - .map_err(|_| CollabJwksFetchError::Unavailable) + .map_err(|_| PolicyFileTrustError::Unavailable) } fn file_etag(body: &[u8]) -> String { diff --git a/crates/op-collab-policy-file/src/tests.rs b/crates/op-collab-policy-file/src/tests.rs index 66df3c806..838399cfd 100644 --- a/crates/op-collab-policy-file/src/tests.rs +++ b/crates/op-collab-policy-file/src/tests.rs @@ -8,11 +8,24 @@ use op_auth_bridge::{ use super::*; +/// Writes a policy fixture with owner-only permissions, so the suite does not +/// depend on the ambient umask now that group/world-writable files are +/// rejected. +fn write_policy(path: &Path, body: &[u8]) { + std::fs::write(path, body).expect("policy"); + #[cfg(unix)] + { + use std::os::unix::fs::PermissionsExt as _; + std::fs::set_permissions(path, std::fs::Permissions::from_mode(0o600)) + .expect("permissions"); + } +} + #[test] fn bounded_regular_file_reads_exact_bytes_and_redacts_path() { let directory = tempfile::tempdir().expect("temp directory"); let path = directory.path().join("policy.json"); - std::fs::write(&path, b"{\"version\":1}").expect("policy"); + write_policy(&path, b"{\"version\":1}"); assert_eq!( read_bounded_regular_file(&path, 64).expect("read"), b"{\"version\":1}" @@ -36,7 +49,7 @@ fn final_symlink_is_rejected() { let directory = tempfile::tempdir().expect("temp directory"); let target = directory.path().join("target.json"); let link = directory.path().join("policy.json"); - std::fs::write(&target, b"{}").expect("target"); + write_policy(&target, b"{}"); symlink(&target, &link).expect("symlink"); assert!(matches!( read_bounded_regular_file(&link, 64), @@ -44,11 +57,138 @@ fn final_symlink_is_rejected() { )); } +#[cfg(unix)] +#[test] +fn group_or_world_writable_policy_file_is_rejected() { + use std::os::unix::fs::PermissionsExt as _; + + let directory = tempfile::tempdir().expect("temp directory"); + for mode in [0o666, 0o664] { + let path = directory.path().join(format!("policy-{mode:o}.json")); + std::fs::write(&path, b"{}").expect("policy"); + std::fs::set_permissions(&path, std::fs::Permissions::from_mode(mode)) + .expect("permissions"); + assert_eq!( + read_bounded_trust_root_file(&path, 64), + Err(PolicyFileTrustError::GroupOrWorldWritable) + ); + assert!(matches!( + read_bounded_regular_file(&path, 64), + Err(CollabJwksFetchError::RejectedResponse) + )); + std::fs::set_permissions(&path, std::fs::Permissions::from_mode(0o600)) + .expect("permissions"); + assert_eq!( + read_bounded_trust_root_file(&path, 64).expect("read"), + b"{}" + ); + } +} + +#[cfg(unix)] +#[test] +fn root_owned_read_only_public_policy_modes_are_accepted() { + // Ownership/mode checking is split from Metadata so these root-owned + // cases remain deterministic under the ordinary non-root CI runner. + let non_root_euid = 1_000; + for mode in [0o440, 0o444] { + assert_eq!(verify_unix_owner_and_mode(0, mode, non_root_euid), Ok(())); + } +} + +#[cfg(unix)] +#[test] +fn root_owned_group_or_world_writable_policy_modes_are_rejected() { + let non_root_euid = 1_000; + for mode in [0o460, 0o442] { + assert_eq!( + verify_unix_owner_and_mode(0, mode, non_root_euid), + Err(PolicyFileTrustError::GroupOrWorldWritable) + ); + } +} + +#[cfg(unix)] +#[test] +fn foreign_non_root_owner_is_still_rejected() { + assert_eq!( + verify_unix_owner_and_mode(1_001, 0o444, 1_000), + Err(PolicyFileTrustError::ForeignOwner) + ); +} + +/// A policy file owned by another non-root user must be refused. Creating one requires +/// privileges the test process does not have, so the test borrows a +/// well-known system file and skips itself whenever that file is missing, is +/// a symlink, is already writable by group/other, or is owned by the caller +/// (which is the case when the suite runs as root). +#[cfg(unix)] +#[test] +fn policy_file_owned_by_another_user_is_rejected() { + use std::os::unix::fs::MetadataExt as _; + + let path = std::path::Path::new("/etc/hosts"); + let Ok(metadata) = std::fs::symlink_metadata(path) else { + return; + }; + // SAFETY: `geteuid` takes no arguments, mutates no state, and cannot fail. + let effective_uid = unsafe { libc::geteuid() }; + if metadata.file_type().is_symlink() + || !metadata.file_type().is_file() + || metadata.mode() & 0o022 != 0 + || metadata.uid() == effective_uid + || metadata.uid() == 0 + { + return; + } + assert_eq!( + read_bounded_trust_root_file(path, 1_024 * 1_024), + Err(PolicyFileTrustError::ForeignOwner) + ); +} + +#[test] +fn trust_error_display_and_fetch_mapping_are_distinct_per_reason() { + for (error, expected) in [ + ( + PolicyFileTrustError::Unavailable, + CollabJwksFetchError::Unavailable, + ), + ( + PolicyFileTrustError::Symlink, + CollabJwksFetchError::RejectedResponse, + ), + ( + PolicyFileTrustError::NotRegularFile, + CollabJwksFetchError::RejectedResponse, + ), + ( + PolicyFileTrustError::OpenedObjectChanged, + CollabJwksFetchError::RejectedResponse, + ), + ( + PolicyFileTrustError::GroupOrWorldWritable, + CollabJwksFetchError::RejectedResponse, + ), + ( + PolicyFileTrustError::ForeignOwner, + CollabJwksFetchError::RejectedResponse, + ), + ( + PolicyFileTrustError::TooLarge, + CollabJwksFetchError::ResponseTooLarge, + ), + ] { + assert_eq!(CollabJwksFetchError::from(error), expected); + assert!(!error.to_string().is_empty()); + } +} + #[test] fn verifier_endpoint_is_pinned() { let directory = tempfile::tempdir().expect("temp directory"); let path = directory.path().join("policy.json"); - std::fs::write(&path, b"{\"keys\":[]}").expect("policy"); + write_policy(&path, b"{\"keys\":[]}"); let config = test_config().expect("config"); let fetcher = PinnedPolicyFileFetcher::new(&config, path, NonZeroU64::MIN); let verifier = CollabTicketVerifier::new(config, fetcher, CollabJwksCacheLimits::default()); diff --git a/crates/op-collab-relay-client/src/bridge/guest.rs b/crates/op-collab-relay-client/src/bridge/guest.rs index 19d8847ef..e8a0cb642 100644 --- a/crates/op-collab-relay-client/src/bridge/guest.rs +++ b/crates/op-collab-relay-client/src/bridge/guest.rs @@ -15,6 +15,7 @@ use crate::error::{ TunnelError, }; use crate::limits::RelayLimits; +use crate::reauth_budget::ReauthBudget; use crate::session::{cancelled, pump, ClientReauthContext}; /// A single-use loopback listener that connects an existing guest TCP driver @@ -175,11 +176,15 @@ async fn run_guest( return; } + // One budget for the guest's single tunnel, spanning the handshake and the + // pump so the bound covers the whole connection rather than one phase. + let mut reauth_budget = ReauthBudget::new(limits); let socket = match establish_relay( &endpoint, &route, op_collab_relay_protocol::RelayRole::Guest, &auth, + &mut reauth_budget, &mut cancel, started_at, limits, @@ -214,6 +219,7 @@ async fn run_guest( role: op_collab_relay_protocol::RelayRole::Guest, route: route.route(), }, + &mut reauth_budget, &mut cancel, started_at, limits, diff --git a/crates/op-collab-relay-client/src/bridge/owner.rs b/crates/op-collab-relay-client/src/bridge/owner.rs index ac13dcbf9..c210f0b74 100644 --- a/crates/op-collab-relay-client/src/bridge/owner.rs +++ b/crates/op-collab-relay-client/src/bridge/owner.rs @@ -16,6 +16,7 @@ use crate::error::{ TunnelError, }; use crate::limits::{RelayLimits, DEFAULT_OWNER_LANE_COUNT, MAX_OWNER_LANE_COUNT}; +use crate::reauth_budget::ReauthBudget; use crate::session::{cancelled, pump, sleep_or_cancel, ClientReauthContext}; /// A bounded pool of owner relay lanes. @@ -380,11 +381,15 @@ async fn run_lane( return Err(TunnelError::Cancelled); } let started_at = Instant::now(); + // One budget per lane connection: a lane that reconnects starts over, + // and a lane that is spammed cannot borrow another lane's headroom. + let mut reauth_budget = ReauthBudget::new(limits); let socket = establish_relay_with_ready_hook( &endpoint, &route, op_collab_relay_protocol::RelayRole::Owner, &auth, + &mut reauth_budget, &mut cancel, started_at, limits, @@ -421,6 +426,7 @@ async fn run_lane( role: op_collab_relay_protocol::RelayRole::Owner, route: route.route(), }, + &mut reauth_budget, &mut cancel, started_at, limits, diff --git a/crates/op-collab-relay-client/src/bridge/protocol.rs b/crates/op-collab-relay-client/src/bridge/protocol.rs index 077bfa330..3b799d223 100644 --- a/crates/op-collab-relay-client/src/bridge/protocol.rs +++ b/crates/op-collab-relay-client/src/bridge/protocol.rs @@ -11,6 +11,7 @@ use crate::auth::AuthMode; use crate::endpoint::RelayEndpoint; use crate::error::{RelayFailureKind, TunnelError}; use crate::limits::RelayLimits; +use crate::reauth_budget::ReauthBudget; use crate::session::{ connect_socket, next_binary_with_reauth, send_binary, ClientReauthContext, RelaySocket, RelayUpgrade, @@ -43,11 +44,13 @@ impl fmt::Debug for RelayHandshake { } } +#[allow(clippy::too_many_arguments)] pub(crate) async fn establish_relay( endpoint: &RelayEndpoint, handshake: &RelayHandshake, role: RelayRole, auth: &AuthMode, + reauth_budget: &mut ReauthBudget, cancel: &mut watch::Receiver, started_at: Instant, limits: RelayLimits, @@ -57,6 +60,7 @@ pub(crate) async fn establish_relay( handshake, role, auth, + reauth_budget, cancel, started_at, limits, @@ -71,6 +75,7 @@ pub(crate) async fn establish_relay_with_ready_hook( handshake: &RelayHandshake, role: RelayRole, auth: &AuthMode, + reauth_budget: &mut ReauthBudget, cancel: &mut watch::Receiver, started_at: Instant, limits: RelayLimits, @@ -122,6 +127,7 @@ where role, route: &handshake.route, }, + reauth_budget, cancel, hello_timeout, hello_kind, @@ -151,6 +157,7 @@ where role, route: &handshake.route, }, + reauth_budget, cancel, pair_timeout, pair_kind, @@ -164,9 +171,11 @@ where } } +#[allow(clippy::too_many_arguments)] async fn next_status( socket: &mut RelaySocket, reauth: ClientReauthContext<'_>, + reauth_budget: &mut ReauthBudget, cancel: &mut watch::Receiver, timeout: Duration, timeout_kind: RelayFailureKind, @@ -175,6 +184,7 @@ async fn next_status( let bytes = next_binary_with_reauth( socket, reauth, + reauth_budget, cancel, timeout, timeout_kind, diff --git a/crates/op-collab-relay-client/src/error.rs b/crates/op-collab-relay-client/src/error.rs index f984d9f82..4abbd81a2 100644 --- a/crates/op-collab-relay-client/src/error.rs +++ b/crates/op-collab-relay-client/src/error.rs @@ -50,6 +50,12 @@ pub enum RelayFailureKind { Rejected, Protocol, TextFrame, + /// The relay sent a reauthentication challenge sooner than the protocol's + /// slowest possible legitimate rotation cadence allows. + ReauthTooFrequent, + /// The relay exhausted this connection's server-initiated + /// reauthentication budget. + ReauthBudgetExhausted, BinaryFrameTooLarge, IdleTimeout, LifetimeExceeded, diff --git a/crates/op-collab-relay-client/src/lib.rs b/crates/op-collab-relay-client/src/lib.rs index cf7827e04..25e3f3ab3 100644 --- a/crates/op-collab-relay-client/src/lib.rs +++ b/crates/op-collab-relay-client/src/lib.rs @@ -8,6 +8,7 @@ mod bridge; mod endpoint; mod error; mod limits; +mod reauth_budget; mod session; pub use auth::{ diff --git a/crates/op-collab-relay-client/src/reauth_budget.rs b/crates/op-collab-relay-client/src/reauth_budget.rs new file mode 100644 index 000000000..94a553e1a --- /dev/null +++ b/crates/op-collab-relay-client/src/reauth_budget.rs @@ -0,0 +1,179 @@ +use std::time::Duration; + +use tokio::time::Instant; + +use crate::error::{RelayFailureKind, TunnelError}; +use crate::limits::RelayLimits; + +/// Halving divisor applied to the idle window to obtain the minimum spacing +/// between two server-initiated reauthentications. +const MINIMUM_INTERVAL_DIVISOR: u32 = 2; + +/// Headroom multiplier applied to the honest per-lifetime challenge count. +const ATTEMPT_HEADROOM_FACTOR: u64 = 2; + +/// Per-connection ceiling on server-initiated reauthentication. +/// +/// Every text frame the relay sends is a reauthentication challenge, and +/// answering one mints a fresh bearer through the caller's credential +/// provider — on the desktop that is the local authentication bridge and the +/// hub ticket-mint path. Without a bound, a compromised relay can drive one +/// mint per frame for the whole 24 h tunnel lifetime and use the client as a +/// free amplifier against its own authentication backend. This is not a +/// credential-exposure problem (the relay already holds a valid bearer), so +/// the bound is sized purely to keep the amplification factor small while +/// never interfering with the protocol's real reauthentication cadence. +/// +/// # Sizing +/// +/// Server-initiated reauthentication is driven by admission-ticket rotation, +/// not by traffic: the relay challenges once per accepted ticket, as that +/// ticket's deadline approaches. The client never sees ticket lifetimes, so +/// both halves of the bound are derived from the two connection limits it does +/// own, [`RelayLimits::idle`] and [`RelayLimits::lifetime`]. +/// +/// * **Minimum spacing = `idle / 2`** (60 s with the default 2 min idle +/// window). `idle` is the longest a live tunnel may stay silent, so a ticket +/// whose entire lifetime were shorter than one idle window could not survive +/// a single quiet period — no legitimate rotation cadence can be faster than +/// `idle`. Halving it leaves 2x headroom for clock jitter and for a relay +/// that challenges slightly early. +/// * **Maximum count = `2 * (lifetime / idle)`** (2 * (86_400 / 120) = 1_440 +/// with the defaults). An honest relay challenges at most once per rotation, +/// and rotation is at least `idle` apart, so a full-length tunnel needs at +/// most `lifetime / idle` = 720 challenges — exactly half the cap. +/// +/// A legitimate relay therefore uses at most half of each bound over a +/// full-length tunnel, while a malicious relay is held to one bearer mint per +/// minute and 1_440 mints per connection. The count cap is not redundant with +/// the spacing rule: it is time-independent, so it still holds if the +/// monotonic clock stalls or if a caller configures a longer lifetime. +pub(crate) struct ReauthBudget { + remaining: u32, + minimum_interval: Duration, + last_admitted: Option, +} + +impl ReauthBudget { + pub(crate) fn new(limits: RelayLimits) -> Self { + Self { + remaining: maximum_attempts(limits), + minimum_interval: minimum_interval(limits), + last_admitted: None, + } + } + + /// Charges one server-initiated reauthentication against the budget. + /// + /// Exceeding either half of the bound fails the connection closed with a + /// typed failure kind; the frame is never silently ignored, because a + /// relay that pushes past the bound is misbehaving and the tunnel must not + /// continue as if nothing happened. + pub(crate) fn admit(&mut self, now: Instant) -> Result<(), TunnelError> { + if self + .last_admitted + .is_some_and(|previous| now.saturating_duration_since(previous) < self.minimum_interval) + { + return Err(TunnelError::Failure(RelayFailureKind::ReauthTooFrequent)); + } + self.remaining = self.remaining.checked_sub(1).ok_or(TunnelError::Failure( + RelayFailureKind::ReauthBudgetExhausted, + ))?; + self.last_admitted = Some(now); + Ok(()) + } +} + +impl std::fmt::Debug for ReauthBudget { + fn fmt(&self, formatter: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + formatter + .debug_struct("ReauthBudget") + .field("remaining", &self.remaining) + .field("minimum_interval", &self.minimum_interval) + .finish() + } +} + +fn minimum_interval(limits: RelayLimits) -> Duration { + limits.idle / MINIMUM_INTERVAL_DIVISOR +} + +fn maximum_attempts(limits: RelayLimits) -> u32 { + // `max(1)` keeps a degenerate zero idle window from dividing by zero and + // guarantees at least one server-initiated reauthentication is admitted. + let idle_seconds = limits.idle.as_secs().max(1); + let honest_attempts = limits.lifetime.as_secs() / idle_seconds; + let bounded = honest_attempts + .saturating_mul(ATTEMPT_HEADROOM_FACTOR) + .max(1); + u32::try_from(bounded).unwrap_or(u32::MAX) +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn default_bounds_match_the_documented_arithmetic() { + let limits = RelayLimits::default(); + assert_eq!(minimum_interval(limits), Duration::from_secs(60)); + assert_eq!(maximum_attempts(limits), 1_440); + } + + #[test] + fn normal_reauth_cadence_over_a_full_tunnel_lifetime_stays_within_budget() { + let limits = RelayLimits::default(); + let mut budget = ReauthBudget::new(limits); + let started_at = Instant::now(); + // The honest cadence is one challenge per ticket rotation, and rotation + // cannot be faster than one idle window. Walk the whole 24 h lifetime + // at exactly that fastest honest cadence. + let honest_attempts = limits.lifetime.as_secs() / limits.idle.as_secs(); + assert_eq!(honest_attempts, 720); + for attempt in 1..=honest_attempts { + let now = started_at + limits.idle * u32::try_from(attempt).expect("attempt fits"); + assert_eq!( + budget.admit(now), + Ok(()), + "attempt {attempt} must be admitted" + ); + } + } + + #[test] + fn reauth_budget_rejects_a_repeat_inside_the_minimum_interval() { + let limits = RelayLimits::default(); + let mut budget = ReauthBudget::new(limits); + let started_at = Instant::now(); + assert_eq!(budget.admit(started_at), Ok(())); + let too_soon = started_at + minimum_interval(limits) - Duration::from_millis(1); + assert_eq!( + budget.admit(too_soon), + Err(TunnelError::Failure(RelayFailureKind::ReauthTooFrequent)) + ); + assert_eq!(budget.admit(started_at + minimum_interval(limits)), Ok(())); + } + + #[test] + fn reauth_budget_fails_closed_after_the_per_connection_cap() { + let limits = RelayLimits { + idle: Duration::from_secs(10), + lifetime: Duration::from_secs(20), + ..RelayLimits::default() + }; + // 2 * (20 / 10) = 4 admitted challenges, spaced at least 10 / 2 = 5 s. + assert_eq!(maximum_attempts(limits), 4); + let mut budget = ReauthBudget::new(limits); + let started_at = Instant::now(); + for attempt in 0..4 { + let now = started_at + minimum_interval(limits) * attempt; + assert_eq!(budget.admit(now), Ok(())); + } + assert_eq!( + budget.admit(started_at + minimum_interval(limits) * 4), + Err(TunnelError::Failure( + RelayFailureKind::ReauthBudgetExhausted + )) + ); + } +} diff --git a/crates/op-collab-relay-client/src/session.rs b/crates/op-collab-relay-client/src/session.rs index 5027d8cbf..6940a8d12 100644 --- a/crates/op-collab-relay-client/src/session.rs +++ b/crates/op-collab-relay-client/src/session.rs @@ -20,6 +20,7 @@ use crate::auth::{AuthMode, RelayAuthAttempt, RelayCredential}; use crate::endpoint::RelayEndpoint; use crate::error::{RelayFailureKind, TunnelError}; use crate::limits::RelayLimits; +use crate::reauth_budget::ReauthBudget; pub(crate) type RelaySocket = WebSocketStream>; @@ -146,10 +147,12 @@ pub(crate) async fn send_binary( } } +#[allow(clippy::too_many_arguments)] pub(crate) async fn pump( mut socket: RelaySocket, mut local: TcpStream, reauth: ClientReauthContext<'_>, + reauth_budget: &mut ReauthBudget, cancel: &mut watch::Receiver, started_at: Instant, limits: RelayLimits, @@ -226,6 +229,7 @@ pub(crate) async fn pump( &mut socket, &text, reauth, + reauth_budget, cancel, idle_deadline, lifetime_deadline, @@ -242,9 +246,11 @@ pub(crate) async fn pump( } } +#[allow(clippy::too_many_arguments)] pub(crate) async fn next_binary_with_reauth( socket: &mut RelaySocket, reauth: ClientReauthContext<'_>, + reauth_budget: &mut ReauthBudget, cancel: &mut watch::Receiver, timeout: Duration, timeout_kind: RelayFailureKind, @@ -272,7 +278,16 @@ pub(crate) async fn next_binary_with_reauth( return Ok(bytes); } Some(Ok(Message::Text(text))) => { - answer_reauth_challenge(socket, &text, reauth, cancel, deadline, deadline).await?; + answer_reauth_challenge( + socket, + &text, + reauth, + reauth_budget, + cancel, + deadline, + deadline, + ) + .await?; } Some(Ok(Message::Ping(bytes))) => { send_before_deadlines(socket, Message::Pong(bytes), cancel, deadline, deadline) @@ -290,10 +305,12 @@ pub(crate) async fn next_binary_with_reauth( } } +#[allow(clippy::too_many_arguments)] async fn answer_reauth_challenge( socket: &mut RelaySocket, text: &str, reauth: ClientReauthContext<'_>, + reauth_budget: &mut ReauthBudget, cancel: &mut watch::Receiver, idle_deadline: Instant, lifetime_deadline: Instant, @@ -304,6 +321,10 @@ async fn answer_reauth_challenge( let challenge = RelayReauthChallengeV1::decode_text(text) .map_err(|_| TunnelError::Failure(RelayFailureKind::TextFrame))? .into_challenge(); + // The budget is charged only on the path that actually mints a bearer, so + // a relay cannot spend it with frames that never reach the credential + // provider, and the reduced-assurance modes keep their existing rejection. + reauth_budget.admit(Instant::now())?; let attempt = authenticator .begin_attempt() .map_err(|_| TunnelError::Failure(RelayFailureKind::Authentication))?; diff --git a/crates/op-collab-relay-control-plane/src/publish.rs b/crates/op-collab-relay-control-plane/src/publish.rs index 3cdc39a59..c4daceb05 100644 --- a/crates/op-collab-relay-control-plane/src/publish.rs +++ b/crates/op-collab-relay-control-plane/src/publish.rs @@ -24,6 +24,14 @@ const DISCOVERY_LEN_OFFSET: usize = 58; const DISCOVERY_OFFSET: usize = 59; const LIFETIME_OFFSET: usize = 187; +// The discovery-id field is exactly as wide as the longest id the protocol +// admits, which is what makes the `DISCOVERY_OFFSET + discovery_len` slices +// below infallible for any length that passes the bound check. Growing the +// protocol constant without widening the field would turn a single byte of a +// request into an out-of-range slice, so fail the build instead. +const _: () = assert!(LIFETIME_OFFSET - DISCOVERY_OFFSET == MAX_EXPECTED_DISCOVERY_ID_BYTES); +const _: () = assert!(LIFETIME_OFFSET + 4 == OWNER_PUBLISH_REQUEST_BYTES); + #[derive(Clone, Copy, Debug, PartialEq, Eq)] pub struct RelayPublishLifetime(NonZeroU32); diff --git a/crates/op-collab-relay-locator-server/src/config.rs b/crates/op-collab-relay-locator-server/src/config.rs index e4148c6b3..a31506d3f 100644 --- a/crates/op-collab-relay-locator-server/src/config.rs +++ b/crates/op-collab-relay-locator-server/src/config.rs @@ -1,4 +1,5 @@ use std::{ + ffi::OsStr, fmt, net::SocketAddr, num::{NonZeroU32, NonZeroUsize}, @@ -7,13 +8,22 @@ use std::{ pub const DEFAULT_LOCATOR_LISTEN: &str = "127.0.0.1:8092"; pub const MAX_CONFIGURED_TIMEOUT: Duration = Duration::from_secs(30); +/// Environment override for the per-client publish rate ceiling. +pub const LOCATOR_CLIENT_RATE_PER_SECOND_ENV: &str = + "OPENPENCIL_COLLAB_LOCATOR_CLIENT_RATE_PER_SECOND"; +/// Largest per-client publish rate the environment override accepts. +pub const MAX_CLIENT_RATE_PER_SECOND: u32 = 10_000; #[derive(Clone)] pub struct LocatorHttpLimits { pub max_connections: NonZeroUsize, pub max_in_flight: NonZeroUsize, pub max_auth_in_flight: NonZeroUsize, + /// Global publish ceiling shared by every client, in requests per second. pub max_requests_per_second: NonZeroU32, + /// Per-client publish ceiling, keyed by the connecting peer address, in + /// requests per second. Bounded by `max_requests_per_second`. + pub max_client_requests_per_second: NonZeroU32, pub header_timeout: Duration, pub body_timeout: Duration, pub auth_timeout: Duration, @@ -27,6 +37,7 @@ impl Default for LocatorHttpLimits { max_in_flight: NonZeroUsize::new(64).expect("non-zero"), max_auth_in_flight: NonZeroUsize::new(16).expect("non-zero"), max_requests_per_second: NonZeroU32::new(100).expect("non-zero"), + max_client_requests_per_second: NonZeroU32::new(10).expect("non-zero"), header_timeout: Duration::from_secs(5), body_timeout: Duration::from_secs(5), auth_timeout: Duration::from_secs(5), @@ -50,8 +61,43 @@ impl LocatorHttpLimits { if self.max_auth_in_flight > self.max_in_flight { return Err(LocatorServerConfigError::InvalidConcurrency); } + if self.max_client_requests_per_second > self.max_requests_per_second { + return Err(LocatorServerConfigError::InvalidRateLimit); + } Ok(()) } + + /// Applies the environment override for the per-client publish rate. + /// + /// Absent means "keep the configured value"; present must parse as a + /// positive number no larger than [`MAX_CLIENT_RATE_PER_SECOND`], matching + /// the optional bounded-number handling of the other locator settings. + pub fn apply_env_overrides(&mut self) -> Result<(), LocatorServerConfigError> { + let configured = std::env::var_os(LOCATOR_CLIENT_RATE_PER_SECOND_ENV); + if let Some(value) = client_rate_per_second(configured.as_deref())? { + self.max_client_requests_per_second = value; + } + Ok(()) + } +} + +/// Parses the per-client rate override without touching the process +/// environment, so the bound and positivity rules stay unit-testable. +pub(crate) fn client_rate_per_second( + configured: Option<&OsStr>, +) -> Result, LocatorServerConfigError> { + let Some(configured) = configured else { + return Ok(None); + }; + configured + .to_str() + .and_then(|value| value.parse::().ok()) + .filter(|value| *value <= MAX_CLIENT_RATE_PER_SECOND) + .and_then(NonZeroU32::new) + .map(Some) + .ok_or(LocatorServerConfigError::InvalidEnvNumber { + name: LOCATOR_CLIENT_RATE_PER_SECOND_ENV, + }) } impl fmt::Debug for LocatorHttpLimits { @@ -62,6 +108,10 @@ impl fmt::Debug for LocatorHttpLimits { .field("max_in_flight", &self.max_in_flight) .field("max_auth_in_flight", &self.max_auth_in_flight) .field("max_requests_per_second", &self.max_requests_per_second) + .field( + "max_client_requests_per_second", + &self.max_client_requests_per_second, + ) .field("header_timeout", &self.header_timeout) .field("body_timeout", &self.body_timeout) .field("auth_timeout", &self.auth_timeout) @@ -92,4 +142,8 @@ pub enum LocatorServerConfigError { InvalidTimeout { field: &'static str }, #[error("locator authentication concurrency exceeds total request concurrency")] InvalidConcurrency, + #[error("locator per-client rate limit exceeds the global rate limit")] + InvalidRateLimit, + #[error("locator server setting {name} is not a valid bounded number")] + InvalidEnvNumber { name: &'static str }, } diff --git a/crates/op-collab-relay-locator-server/src/hsm.rs b/crates/op-collab-relay-locator-server/src/hsm.rs index 9498c803e..775224586 100644 --- a/crates/op-collab-relay-locator-server/src/hsm.rs +++ b/crates/op-collab-relay-locator-server/src/hsm.rs @@ -10,7 +10,7 @@ use std::{ use op_collab_relay_control_plane::{RelayLocatorSigner, RelayLocatorSignerError}; use op_collab_relay_protocol::LOCATOR_CANONICAL_SIGNING_BYTES; #[cfg(unix)] -use op_collab_relay_protocol::{LocatorKeyId, LocatorSignature}; +use op_collab_relay_protocol::{LocatorKeyId, LocatorSignature, MAX_LOCATOR_KEY_ID_BYTES}; pub const HSM_SIGNING_PROTOCOL_VERSION: u8 = 1; pub const HSM_SIGN_REQUEST_BYTES: usize = 4 + 1 + 1 + 1 + 64 + LOCATOR_CANONICAL_SIGNING_BYTES; @@ -216,6 +216,10 @@ fn encode_request( output[5] = SIGN_OPERATION; output[6] = key_id.as_str().len() as u8; output[7..7 + key_id.as_str().len()].copy_from_slice(key_id.as_str().as_bytes()); + // The key-id field is a fixed 64-byte slot starting at offset 7. The write + // above and the canonical offset below are only in bounds while the longest + // protocol key id fits that slot exactly. + const _: () = assert!(MAX_LOCATOR_KEY_ID_BYTES == 64); let canonical_offset = 7 + 64; output[canonical_offset..].copy_from_slice(canonical); output diff --git a/crates/op-collab-relay-locator-server/src/http.rs b/crates/op-collab-relay-locator-server/src/http.rs index 264c7f228..a2b72ed19 100644 --- a/crates/op-collab-relay-locator-server/src/http.rs +++ b/crates/op-collab-relay-locator-server/src/http.rs @@ -1,13 +1,13 @@ use std::{ future::Future, - num::NonZeroU32, - sync::{Arc, Mutex}, + net::IpAddr, + sync::Arc, time::{Duration, Instant}, }; use axum::{ body::Bytes, - extract::{OriginalUri, State}, + extract::{Extension, OriginalUri, State}, http::{ header::{ ACCEPT, AUTHORIZATION, CONTENT_ENCODING, CONTENT_LENGTH, CONTENT_TYPE, RETRY_AFTER, @@ -42,6 +42,10 @@ use zeroize::Zeroizing; use crate::LocatorServerConfig; +pub(crate) mod rate_limit; + +use rate_limit::RateLimiter; + pub const MAX_HTTP_HEADERS: usize = 32; pub const MAX_HTTP_HEADER_BYTES: usize = 64 * 1024; @@ -68,11 +72,19 @@ where } } +/// Address of the peer that opened the connection a request arrived on. +/// +/// Attached per connection by the accept loop, because the manual +/// `hyper`/`TowerToHyperService` wiring gives the handler no other view of the +/// socket. Requests are rate limited against this address. +#[derive(Clone, Copy, Debug)] +struct ClientAddress(IpAddr); + struct AppState { publisher: Arc, in_flight: Arc, auth_in_flight: Arc, - rate_limiter: Arc, + rate_limiter: Arc, auth_timeout: Duration, } @@ -88,43 +100,6 @@ impl Clone for AppState { } } -struct FixedWindowRateLimiter { - maximum: NonZeroU32, - state: Mutex, -} - -struct RateWindow { - started: Instant, - accepted: u32, -} - -impl FixedWindowRateLimiter { - fn new(maximum: NonZeroU32) -> Self { - Self { - maximum, - state: Mutex::new(RateWindow { - started: Instant::now(), - accepted: 0, - }), - } - } - - fn allow(&self, now: Instant) -> bool { - let Ok(mut state) = self.state.lock() else { - return false; - }; - if now.saturating_duration_since(state.started) >= Duration::from_secs(1) { - state.started = now; - state.accepted = 0; - } - if state.accepted >= self.maximum.get() { - return false; - } - state.accepted += 1; - true - } -} - struct SensitiveBearer(Zeroizing>); impl SensitiveBearer { @@ -172,8 +147,10 @@ where publisher, in_flight: Arc::new(Semaphore::new(config.limits.max_in_flight.get())), auth_in_flight: Arc::new(Semaphore::new(config.limits.max_auth_in_flight.get())), - rate_limiter: Arc::new(FixedWindowRateLimiter::new( + rate_limiter: Arc::new(RateLimiter::new( config.limits.max_requests_per_second, + config.limits.max_client_requests_per_second, + config.limits.max_connections, )), auth_timeout: config.limits.auth_timeout, }; @@ -193,13 +170,17 @@ where biased; () = &mut shutdown => break, accepted = listener.accept() => { - let (stream, _) = accepted.map_err(LocatorServerError::Accept)?; + let (stream, peer) = accepted.map_err(LocatorServerError::Accept)?; let Ok(connection_permit) = Arc::clone(&connection_capacity).try_acquire_owned() else { continue; }; - let service = TowerToHyperService::new(router.clone()); + // The router is shared, so the connecting peer is attached per + // connection: it is the only place the socket address is known. + let service = TowerToHyperService::new( + router.clone().layer(Extension(ClientAddress(peer.ip()))), + ); let connection_shutdown = shutdown_receiver.clone(); let limits = config.limits.clone(); connections.spawn(async move { @@ -280,6 +261,7 @@ async fn health() -> StatusCode { async fn publish_locator( State(state): State, + Extension(client): Extension, OriginalUri(uri): OriginalUri, headers: HeaderMap, body: Bytes, @@ -300,7 +282,7 @@ async fn publish_locator( ); } }; - if !state.rate_limiter.allow(Instant::now()) { + if !state.rate_limiter.allow(Instant::now(), client.0) { return response_with_header( StatusCode::TOO_MANY_REQUESTS, RETRY_AFTER, diff --git a/crates/op-collab-relay-locator-server/src/http/rate_limit.rs b/crates/op-collab-relay-locator-server/src/http/rate_limit.rs new file mode 100644 index 000000000..50a92425f --- /dev/null +++ b/crates/op-collab-relay-locator-server/src/http/rate_limit.rs @@ -0,0 +1,136 @@ +//! Fixed-window rate limiting for the locator publish endpoint. +//! +//! Two ceilings are enforced over the same one-second window: a per-client +//! budget keyed by the connecting peer's IP address, so one unauthenticated +//! source cannot spend everyone else's publish capacity, and a global budget +//! that still bounds total capacity when the load is spread across many +//! sources. + +use std::{ + collections::{hash_map::Entry, HashMap}, + net::IpAddr, + num::{NonZeroU32, NonZeroUsize}, + sync::Mutex, + time::{Duration, Instant}, +}; + +/// Width of the fixed rate-limiting window. +const WINDOW: Duration = Duration::from_secs(1); +/// Tracked client windows allowed per configured connection slot. +const TRACKED_CLIENTS_PER_CONNECTION: usize = 4; +/// Floor for the tracked-client cap, so tiny connection budgets still admit a +/// realistic number of distinct peers. +const MIN_TRACKED_CLIENTS: usize = 64; +/// Ceiling for the tracked-client cap, so a large connection budget cannot turn +/// into an unbounded map. +const MAX_TRACKED_CLIENTS: usize = 65_536; + +pub(crate) struct RateLimiter { + global_maximum: NonZeroU32, + client_maximum: NonZeroU32, + max_tracked_clients: usize, + state: Mutex, +} + +struct RateLimiterState { + global: Window, + clients: HashMap, +} + +#[derive(Clone, Copy)] +struct Window { + started: Instant, + accepted: u32, +} + +impl RateLimiter { + pub(crate) fn new( + global_maximum: NonZeroU32, + client_maximum: NonZeroU32, + max_connections: NonZeroUsize, + ) -> Self { + Self { + global_maximum, + client_maximum, + max_tracked_clients: max_tracked_clients(max_connections), + state: Mutex::new(RateLimiterState { + global: Window::started_at(Instant::now()), + clients: HashMap::new(), + }), + } + } + + /// Accounts one request from `client` and reports whether it is admitted. + /// + /// Budget is consumed only when both ceilings admit the request, so a + /// rejected request never eats capacity from the other gate. + pub(crate) fn allow(&self, now: Instant, client: IpAddr) -> bool { + let Ok(mut guard) = self.state.lock() else { + return false; + }; + let state = &mut *guard; + // A window older than the fixed window carries no budget, so dropping + // it keeps the map proportional to the clients active right now. + state.clients.retain(|_, window| !window.expired(now)); + if state.global.expired(now) { + state.global = Window::started_at(now); + } + if state.global.accepted >= self.global_maximum.get() { + return false; + } + // The key space is remote input: every distinct source address would + // otherwise add an entry that nothing removes, so the map is hard-capped + // in addition to the pruning above. The cap derives from + // `max_connections` because only that many peers can be connected at + // once; still being at the cap after pruning already means a flood, so + // the request is rejected instead of growing the map. + let saturated = state.clients.len() >= self.max_tracked_clients; + let window = match state.clients.entry(client) { + Entry::Occupied(entry) => entry.into_mut(), + Entry::Vacant(entry) => { + if saturated { + return false; + } + entry.insert(Window::started_at(now)) + } + }; + // Pruning above guarantees every surviving window is inside the current + // window, so no per-client reset is needed here. + if window.accepted >= self.client_maximum.get() { + return false; + } + window.accepted += 1; + state.global.accepted += 1; + true + } + + #[cfg(test)] + pub(crate) fn tracked_clients(&self) -> usize { + self.state.lock().expect("rate limiter state").clients.len() + } + + #[cfg(test)] + pub(crate) fn max_tracked_clients(&self) -> usize { + self.max_tracked_clients + } +} + +impl Window { + fn started_at(now: Instant) -> Self { + Self { + started: now, + accepted: 0, + } + } + + fn expired(&self, now: Instant) -> bool { + now.saturating_duration_since(self.started) >= WINDOW + } +} + +fn max_tracked_clients(max_connections: NonZeroUsize) -> usize { + max_connections + .get() + .saturating_mul(TRACKED_CLIENTS_PER_CONNECTION) + .clamp(MIN_TRACKED_CLIENTS, MAX_TRACKED_CLIENTS) +} diff --git a/crates/op-collab-relay-locator-server/src/production.rs b/crates/op-collab-relay-locator-server/src/production.rs index 7e4dfda22..0a4862fa9 100644 --- a/crates/op-collab-relay-locator-server/src/production.rs +++ b/crates/op-collab-relay-locator-server/src/production.rs @@ -102,6 +102,13 @@ impl ProductionLocatorConfig { limits.max_requests_per_second = std::num::NonZeroU32::new(value as u32).expect("bounded non-zero"); } + // Lowering the global ceiling must not turn the default per-client rate + // into a startup error. An explicit per-client override is applied after + // this clamp, so a value that exceeds the ceiling still fails loudly. + limits.max_client_requests_per_second = limits + .max_client_requests_per_second + .min(limits.max_requests_per_second); + limits.apply_env_overrides()?; let server = LocatorServerConfig::new(listen, limits)?; Ok(Self { server, diff --git a/crates/op-collab-relay-locator-server/src/tests.rs b/crates/op-collab-relay-locator-server/src/tests.rs index df8383d6a..9b02cc441 100644 --- a/crates/op-collab-relay-locator-server/src/tests.rs +++ b/crates/op-collab-relay-locator-server/src/tests.rs @@ -1,8 +1,11 @@ #![cfg(test)] use std::{ + ffi::OsStr, + net::{IpAddr, Ipv4Addr}, + num::{NonZeroU32, NonZeroUsize}, sync::Arc, - time::{Duration, SystemTime, UNIX_EPOCH}, + time::{Duration, Instant, SystemTime, UNIX_EPOCH}, }; #[cfg(unix)] @@ -24,7 +27,12 @@ use tokio::{ sync::oneshot, }; -use crate::{serve_listener_until, LocatorHttpLimits, LocatorPublisher, LocatorServerConfig}; +use crate::{ + config::{client_rate_per_second, LOCATOR_CLIENT_RATE_PER_SECOND_ENV}, + http::rate_limit::RateLimiter, + serve_listener_until, LocatorHttpLimits, LocatorPublisher, LocatorServerConfig, + LocatorServerConfigError, +}; #[cfg(unix)] use crate::{ ExpectedUnixPeer, UnixHsmRelayLocatorSigner, HSM_SIGN_REQUEST_BYTES, HSM_SIGN_RESPONSE_BYTES, @@ -193,7 +201,8 @@ async fn real_http_route_rejects_method_query_headers_bearer_and_body() { #[tokio::test] async fn rate_auth_concurrency_and_auth_timeout_fail_closed() { let rate_limits = LocatorHttpLimits { - max_requests_per_second: std::num::NonZeroU32::MIN, + max_requests_per_second: NonZeroU32::MIN, + max_client_requests_per_second: NonZeroU32::MIN, ..LocatorHttpLimits::default() }; let server = TestServer::start(Arc::new(SigningPublisher::immediate()), rate_limits).await; @@ -237,6 +246,138 @@ async fn rate_auth_concurrency_and_auth_timeout_fail_closed() { server.stop().await; } +#[test] +fn per_client_rate_windows_are_independent_across_client_addresses() { + let limiter = RateLimiter::new( + NonZeroU32::new(100).expect("global rate"), + NonZeroU32::new(2).expect("client rate"), + NonZeroUsize::new(256).expect("connections"), + ); + let now = Instant::now(); + assert!(limiter.allow(now, client(1))); + assert!(limiter.allow(now, client(1))); + assert!(!limiter.allow(now, client(1))); + for _ in 0..2 { + assert!( + limiter.allow(now, client(2)), + "a second address must carry its own budget" + ); + } + assert!(!limiter.allow(now, client(2))); + assert!( + limiter.allow(now + Duration::from_millis(1_001), client(1)), + "a per-client window must recover once it elapses" + ); +} + +#[test] +fn one_client_exhausting_its_budget_does_not_throttle_another_client() { + let limiter = RateLimiter::new( + NonZeroU32::new(1_000).expect("global rate"), + NonZeroU32::new(4).expect("client rate"), + NonZeroUsize::new(256).expect("connections"), + ); + let now = Instant::now(); + for _ in 0..512 { + let _ = limiter.allow(now, client(9)); + } + assert!(!limiter.allow(now, client(9))); + for _ in 0..4 { + assert!( + limiter.allow(now, client(10)), + "one flooding address must not spend a legitimate owner's budget" + ); + } +} + +#[test] +fn tracked_client_windows_stay_bounded_under_many_distinct_addresses() { + let limiter = RateLimiter::new( + NonZeroU32::new(1_000_000).expect("global rate"), + NonZeroU32::new(4).expect("client rate"), + NonZeroUsize::new(1).expect("connections"), + ); + let now = Instant::now(); + let capacity = limiter.max_tracked_clients(); + let mut admitted = 0_usize; + for index in 0..4_096_u32 { + if limiter.allow(now, IpAddr::V4(Ipv4Addr::from(index))) { + admitted += 1; + } + } + assert_eq!(admitted, capacity); + assert_eq!(limiter.tracked_clients(), capacity); + assert!( + limiter.allow(now + Duration::from_millis(1_001), client(200)), + "expired windows must be pruned so the map recovers" + ); + assert_eq!(limiter.tracked_clients(), 1); +} + +#[test] +fn global_rate_ceiling_still_bounds_total_capacity_across_clients() { + let limiter = RateLimiter::new( + NonZeroU32::new(3).expect("global rate"), + NonZeroU32::new(2).expect("client rate"), + NonZeroUsize::new(256).expect("connections"), + ); + let now = Instant::now(); + for index in 1..=3 { + assert!(limiter.allow(now, client(index))); + } + assert!( + !limiter.allow(now, client(4)), + "distinct sources under their own budget must still hit the global ceiling" + ); + assert!(!limiter.allow(now, client(1))); + assert!(limiter.allow(now + Duration::from_millis(1_001), client(4))); +} + +#[test] +fn client_rate_environment_override_is_positive_and_bounded() { + assert_eq!(client_rate_per_second(None), Ok(None)); + assert_eq!( + client_rate_per_second(Some(OsStr::new("25"))), + Ok(NonZeroU32::new(25)) + ); + assert_eq!( + client_rate_per_second(Some(OsStr::new("10000"))), + Ok(NonZeroU32::new(10_000)) + ); + for rejected in ["0", "-1", "", " 25", "10001", "abc"] { + assert_eq!( + client_rate_per_second(Some(OsStr::new(rejected))), + Err(LocatorServerConfigError::InvalidEnvNumber { + name: LOCATOR_CLIENT_RATE_PER_SECOND_ENV, + }), + "{rejected}" + ); + } +} + +#[test] +fn limits_reject_a_per_client_rate_above_the_global_rate() { + let limits = LocatorHttpLimits { + max_requests_per_second: NonZeroU32::new(4).expect("global rate"), + max_client_requests_per_second: NonZeroU32::new(5).expect("client rate"), + ..LocatorHttpLimits::default() + }; + assert_eq!( + limits.validate(), + Err(LocatorServerConfigError::InvalidRateLimit) + ); + let bounded = LocatorHttpLimits { + max_client_requests_per_second: NonZeroU32::new(4).expect("client rate"), + ..limits + }; + assert_eq!(bounded.validate(), Ok(())); + assert_eq!(LocatorHttpLimits::default().validate(), Ok(())); +} + +fn client(last: u8) -> IpAddr { + IpAddr::V4(Ipv4Addr::new(203, 0, 113, last)) +} + #[cfg(unix)] #[test] fn unix_hsm_protocol_authenticates_peer_and_returns_signature() { diff --git a/crates/op-collab-relay-server/src/config.rs b/crates/op-collab-relay-server/src/config.rs index 42e002d7f..bfabfe470 100644 --- a/crates/op-collab-relay-server/src/config.rs +++ b/crates/op-collab-relay-server/src/config.rs @@ -7,7 +7,9 @@ use std::{ const LISTEN_ENV: &str = "OPENPENCIL_COLLAB_RELAY_LISTEN"; const MAX_PENDING_ENV: &str = "OPENPENCIL_COLLAB_RELAY_MAX_PENDING"; +const MAX_PENDING_PER_SOURCE_ENV: &str = "OPENPENCIL_COLLAB_RELAY_MAX_PENDING_PER_SOURCE"; const MAX_AUTH_IN_FLIGHT_ENV: &str = "OPENPENCIL_COLLAB_RELAY_MAX_AUTH_IN_FLIGHT"; +const MAX_REAUTH_IN_FLIGHT_ENV: &str = "OPENPENCIL_COLLAB_RELAY_MAX_REAUTH_IN_FLIGHT"; const MAX_ACTIVE_ENV: &str = "OPENPENCIL_COLLAB_RELAY_MAX_ACTIVE"; const MAX_WAITING_PER_ROUTE_ENV: &str = "OPENPENCIL_COLLAB_RELAY_MAX_WAITING_PER_ROUTE"; const RELAY_QUEUE_ENV: &str = "OPENPENCIL_COLLAB_RELAY_QUEUE_CAPACITY"; @@ -23,7 +25,15 @@ pub struct RelayConfig { pub tunnel_lifetime: Duration, pub max_message_bytes: usize, pub max_pending: usize, + /// How many un-paired connections one source address may hold at once. + pub max_pending_per_source: usize, pub max_auth_in_flight: usize, + /// Renewal budget for already-authenticated tunnels. + /// + /// Kept separate from `max_auth_in_flight` so a flood of unauthenticated + /// connections cannot starve the reauthentication of live sessions, which + /// closes them with a policy error when it cannot complete in time. + pub max_reauth_in_flight: usize, pub max_active_pairs: usize, pub max_waiting_per_route: usize, pub relay_queue_capacity: usize, @@ -42,7 +52,9 @@ impl Default for RelayConfig { tunnel_lifetime: Duration::from_secs(12 * 60 * 60), max_message_bytes: 64 * 1024, max_pending: 1_024, + max_pending_per_source: 16, max_auth_in_flight: 128, + max_reauth_in_flight: 128, max_active_pairs: 10_000, max_waiting_per_route: 4, relay_queue_capacity: 32, @@ -65,8 +77,12 @@ impl RelayConfig { .map_err(|source| ConfigError::Listen { value, source })?; } config.max_pending = parse_positive_usize(MAX_PENDING_ENV, config.max_pending)?; + config.max_pending_per_source = + parse_positive_usize(MAX_PENDING_PER_SOURCE_ENV, config.max_pending_per_source)?; config.max_auth_in_flight = parse_positive_usize(MAX_AUTH_IN_FLIGHT_ENV, config.max_auth_in_flight)?; + config.max_reauth_in_flight = + parse_positive_usize(MAX_REAUTH_IN_FLIGHT_ENV, config.max_reauth_in_flight)?; config.max_active_pairs = parse_positive_usize(MAX_ACTIVE_ENV, config.max_active_pairs)?; config.max_waiting_per_route = parse_positive_usize(MAX_WAITING_PER_ROUTE_ENV, config.max_waiting_per_route)?; @@ -86,7 +102,9 @@ impl RelayConfig { for (name, value) in [ ("max_message_bytes", self.max_message_bytes), ("max_pending", self.max_pending), + ("max_pending_per_source", self.max_pending_per_source), ("max_auth_in_flight", self.max_auth_in_flight), + ("max_reauth_in_flight", self.max_reauth_in_flight), ("max_active_pairs", self.max_active_pairs), ("max_waiting_per_route", self.max_waiting_per_route), ("relay_queue_capacity", self.relay_queue_capacity), @@ -115,6 +133,9 @@ impl RelayConfig { if self.max_queued_bytes_per_route > self.max_queued_bytes { return Err(ConfigError::RouteBudgetExceedsGlobal); } + if self.max_pending_per_source > self.max_pending { + return Err(ConfigError::SourceBudgetExceedsGlobal); + } for (name, value) in [ ("handshake_timeout", self.handshake_timeout), ("waiting_timeout", self.waiting_timeout), @@ -181,4 +202,6 @@ pub enum ConfigError { TooLarge { name: &'static str, maximum: usize }, #[error("max_queued_bytes_per_route must not exceed max_queued_bytes")] RouteBudgetExceedsGlobal, + #[error("max_pending_per_source must not exceed max_pending")] + SourceBudgetExceedsGlobal, } diff --git a/crates/op-collab-relay-server/src/connection.rs b/crates/op-collab-relay-server/src/connection.rs index c0380d784..b9262a295 100644 --- a/crates/op-collab-relay-server/src/connection.rs +++ b/crates/op-collab-relay-server/src/connection.rs @@ -37,6 +37,7 @@ use crate::{ perform_reauthentication, ReauthOutcome, ReauthRelayTraffic, RelayAuthState, RelaySessionIdentity, }, + peer_quota::PeerQuotaPermit, registry::{PairNotice, QueuedPayload, Registration, Registry, WaitingRegistration}, }; @@ -51,20 +52,41 @@ pub(crate) struct ConnectionServices { pub(crate) registry: Registry, pub(crate) authenticator: Arc, pub(crate) auth_in_flight: Arc, + pub(crate) reauth_in_flight: Arc, pub(crate) queued_bytes: Arc, } +/// The capacity a connection occupies until it pairs. +/// +/// Both permits cover the same span — the pre-pairing phase — so they are +/// released together. Once paired, the connection's cost is bounded by +/// `max_active_pairs` and the queue budgets instead. +pub(crate) struct AdmissionPermit { + _pending: OwnedSemaphorePermit, + _source: PeerQuotaPermit, +} + +impl AdmissionPermit { + pub(crate) fn new(pending: OwnedSemaphorePermit, source: PeerQuotaPermit) -> Self { + Self { + _pending: pending, + _source: source, + } + } +} + pub(crate) async fn serve_connection( stream: TcpStream, services: ConnectionServices, mut shutdown: watch::Receiver, - pending_permit: OwnedSemaphorePermit, + admission_permit: AdmissionPermit, ) { let ConnectionServices { config, registry, authenticator, auth_in_flight, + reauth_in_flight, queued_bytes, } = services; let started_at = Instant::now(); @@ -181,15 +203,19 @@ pub(crate) async fn serve_connection( } }; if !send_status(&mut sink, RelayServerStatus::Ready).await { - if let Registration::Waiting(waiting) = ®istration { - registry.unregister_waiter(waiting).await; + match ®istration { + Registration::Waiting(waiting) => registry.unregister_waiter(waiting).await, + // The counterpart only reclaims the slot if it reads its pairing + // notice, and its wait loop can time out in the same wake without + // ever reading it. Release the pair here so the entry cannot leak. + Registration::Paired(pair) => registry.close_pair(pair.pair_id).await, } return; } let mut pair = match registration { Registration::Paired(pair) => { - drop(pending_permit); + drop(admission_permit); pair } Registration::Waiting(mut waiting) => { @@ -202,13 +228,13 @@ pub(crate) async fn serve_connection( configured_deadline, strict_reauth, Arc::clone(&authenticator), - Arc::clone(&auth_in_flight), + Arc::clone(&reauth_in_flight), &identity, &mut shutdown, ) .await; registry.unregister_waiter(&waiting).await; - drop(pending_permit); + drop(admission_permit); let Some(pair) = pair else { return; }; @@ -241,7 +267,7 @@ pub(crate) async fn serve_connection( configured_deadline, strict_reauth, authenticator, - auth_in_flight, + reauth_in_flight, identity, &mut shutdown, ) @@ -338,7 +364,7 @@ async fn wait_for_pair( configured_deadline: Instant, strict_reauth: bool, authenticator: Arc, - auth_in_flight: Arc, + reauth_in_flight: Arc, identity: &RelaySessionIdentity, shutdown: &mut watch::Receiver, ) -> Option { @@ -377,7 +403,7 @@ async fn wait_for_pair( sink, source, Arc::clone(&authenticator), - Arc::clone(&auth_in_flight), + Arc::clone(&reauth_in_flight), identity, *auth_state, configured_deadline, @@ -476,7 +502,7 @@ async fn relay_paired( configured_deadline: Instant, strict_reauth: bool, authenticator: Arc, - auth_in_flight: Arc, + reauth_in_flight: Arc, identity: RelaySessionIdentity, shutdown: &mut watch::Receiver, ) { @@ -513,7 +539,7 @@ async fn relay_paired( &mut sink, &mut source, Arc::clone(&authenticator), - Arc::clone(&auth_in_flight), + Arc::clone(&reauth_in_flight), &identity, auth_state, configured_deadline, diff --git a/crates/op-collab-relay-server/src/connection_reauth.rs b/crates/op-collab-relay-server/src/connection_reauth.rs index 271d6751b..59ade33a5 100644 --- a/crates/op-collab-relay-server/src/connection_reauth.rs +++ b/crates/op-collab-relay-server/src/connection_reauth.rs @@ -144,7 +144,7 @@ pub(crate) async fn perform_reauthentication( sink: &mut WebSocketSink, source: &mut WebSocketSource, authenticator: Arc, - auth_in_flight: Arc, + reauth_in_flight: Arc, identity: &RelaySessionIdentity, current: RelayAuthState, configured_deadline: Instant, @@ -166,7 +166,7 @@ pub(crate) async fn perform_reauthentication( sink, source, response_deadline, - auth_in_flight.acquire_owned(), + reauth_in_flight.acquire_owned(), traffic.as_deref_mut(), ) .await? diff --git a/crates/op-collab-relay-server/src/lib.rs b/crates/op-collab-relay-server/src/lib.rs index 09837efe1..ed4e2f3e1 100644 --- a/crates/op-collab-relay-server/src/lib.rs +++ b/crates/op-collab-relay-server/src/lib.rs @@ -3,6 +3,7 @@ mod config; mod connection; mod connection_reauth; mod error; +mod peer_quota; mod pinned_verifiers; mod production; mod registry; diff --git a/crates/op-collab-relay-server/src/peer_quota.rs b/crates/op-collab-relay-server/src/peer_quota.rs new file mode 100644 index 000000000..39ea8b880 --- /dev/null +++ b/crates/op-collab-relay-server/src/peer_quota.rs @@ -0,0 +1,123 @@ +//! Per-source-address admission accounting for the pre-pairing phase. +//! +//! A relay connection performs its most expensive work — the WebSocket +//! upgrade, the hello decode, and ticket verification — before the client has +//! proven anything. The global `max_pending` and `max_auth_in_flight` ceilings +//! bound that work in aggregate but not per source, so a single host can hold +//! every slot by opening connections and then going silent until the handshake +//! deadline expires. Charging those slots to the connecting address caps how +//! much of the shared budget one source can occupy. + +use std::{ + collections::HashMap, + net::IpAddr, + sync::{Arc, Mutex}, +}; + +pub(crate) struct PeerQuota { + limit: usize, + in_flight: Mutex>, +} + +impl PeerQuota { + pub(crate) fn new(limit: usize) -> Arc { + Arc::new(Self { + limit: limit.max(1), + in_flight: Mutex::new(HashMap::new()), + }) + } + + /// Reserve one pre-pairing slot for `address`, or return `None` when that + /// source already holds `limit` of them. + /// + /// The map only ever holds addresses with a live reservation and entries + /// are removed as their last permit drops, so its size is bounded by the + /// global pending ceiling rather than by how many addresses connect. + pub(crate) fn try_acquire(self: &Arc, address: IpAddr) -> Option { + let mut in_flight = self.in_flight.lock().ok()?; + let slot = in_flight.entry(address).or_insert(0); + if *slot >= self.limit { + return None; + } + *slot += 1; + drop(in_flight); + Some(PeerQuotaPermit { + quota: Arc::clone(self), + address, + }) + } + + #[cfg(test)] + pub(crate) fn tracked_addresses(&self) -> usize { + self.in_flight.lock().map_or(0, |in_flight| in_flight.len()) + } +} + +pub(crate) struct PeerQuotaPermit { + quota: Arc, + address: IpAddr, +} + +impl Drop for PeerQuotaPermit { + fn drop(&mut self) { + let Ok(mut in_flight) = self.quota.in_flight.lock() else { + return; + }; + let Some(slot) = in_flight.get_mut(&self.address) else { + return; + }; + *slot = slot.saturating_sub(1); + if *slot == 0 { + in_flight.remove(&self.address); + } + } +} + +#[cfg(test)] +mod tests { + use std::net::{Ipv4Addr, Ipv6Addr}; + + use super::*; + + fn v4(last: u8) -> IpAddr { + IpAddr::V4(Ipv4Addr::new(203, 0, 113, last)) + } + + #[test] + fn quota_caps_slots_per_source_address() { + let quota = PeerQuota::new(2); + let first = quota.try_acquire(v4(1)).expect("first slot"); + let second = quota.try_acquire(v4(1)).expect("second slot"); + assert!(quota.try_acquire(v4(1)).is_none()); + drop(first); + drop(second); + } + + #[test] + fn exhausting_one_source_leaves_others_admissible() { + let quota = PeerQuota::new(1); + let _held = quota.try_acquire(v4(1)).expect("first source"); + assert!(quota.try_acquire(v4(1)).is_none()); + assert!(quota.try_acquire(v4(2)).is_some()); + assert!(quota.try_acquire(IpAddr::V6(Ipv6Addr::LOCALHOST)).is_some()); + } + + #[test] + fn released_permits_free_the_slot_and_drop_the_entry() { + let quota = PeerQuota::new(1); + let permit = quota.try_acquire(v4(1)).expect("slot"); + assert_eq!(quota.tracked_addresses(), 1); + drop(permit); + assert_eq!(quota.tracked_addresses(), 0); + assert!(quota.try_acquire(v4(1)).is_some()); + } + + #[test] + fn a_rejected_acquire_does_not_retain_an_entry() { + let quota = PeerQuota::new(1); + let permit = quota.try_acquire(v4(1)).expect("slot"); + assert!(quota.try_acquire(v4(1)).is_none()); + drop(permit); + assert_eq!(quota.tracked_addresses(), 0); + } +} diff --git a/crates/op-collab-relay-server/src/server.rs b/crates/op-collab-relay-server/src/server.rs index fe27d57cd..0ce76c7ab 100644 --- a/crates/op-collab-relay-server/src/server.rs +++ b/crates/op-collab-relay-server/src/server.rs @@ -1,4 +1,8 @@ -use std::{future::Future, sync::Arc, time::SystemTime}; +use std::{ + future::Future, + sync::Arc, + time::{Duration, Instant, SystemTime}, +}; use tokio::{ net::TcpListener, @@ -9,8 +13,9 @@ use tokio::{ use crate::{ auth::{RelayAuthenticator, UnauthenticatedDevAuthenticator}, config::RelayConfig, - connection::{serve_connection, ConnectionServices}, + connection::{serve_connection, AdmissionPermit, ConnectionServices}, error::RelayServerError, + peer_quota::PeerQuota, registry::Registry, }; @@ -88,10 +93,13 @@ where config.max_queued_bytes_per_route, ); let pending = Arc::new(Semaphore::new(config.max_pending)); + let peer_quota = PeerQuota::new(config.max_pending_per_source); let auth_in_flight = Arc::new(Semaphore::new(config.max_auth_in_flight)); + let reauth_in_flight = Arc::new(Semaphore::new(config.max_reauth_in_flight)); let queued_bytes = Arc::new(Semaphore::new(config.max_queued_bytes)); let (shutdown_tx, shutdown_rx) = watch::channel(false); let mut connections = JoinSet::new(); + let mut rejections = RejectionLog::default(); tokio::pin!(shutdown); loop { @@ -99,15 +107,20 @@ where biased; () = &mut shutdown => break, accepted = listener.accept() => { - let (stream, _peer_addr) = accepted.map_err(RelayServerError::Accept)?; + let (stream, peer_addr) = accepted.map_err(RelayServerError::Accept)?; let Ok(pending_permit) = Arc::clone(&pending).try_acquire_owned() else { - tracing::warn!("relay pending connection capacity reached"); + rejections.record_pending_exhausted(); + continue; + }; + let Some(source_permit) = peer_quota.try_acquire(peer_addr.ip()) else { + rejections.record_source_exhausted(); continue; }; let config = Arc::clone(&config); let registry = registry.clone(); let authenticator = Arc::clone(&authenticator); let auth_in_flight = Arc::clone(&auth_in_flight); + let reauth_in_flight = Arc::clone(&reauth_in_flight); let queued_bytes = Arc::clone(&queued_bytes); let shutdown = shutdown_rx.clone(); connections.spawn(async move { @@ -118,10 +131,11 @@ where registry, authenticator, auth_in_flight, + reauth_in_flight, queued_bytes, }, shutdown, - pending_permit, + AdmissionPermit::new(pending_permit, source_permit), ) .await; }); @@ -134,6 +148,7 @@ where } } + rejections.flush_pending_summary(); tracing::info!("relay shutdown requested"); let _ = shutdown_tx.send(true); while let Some(result) = connections.join_next().await { @@ -143,3 +158,69 @@ where } Ok(()) } + +/// Interval between capacity-rejection summaries. +const REJECTION_SUMMARY_INTERVAL: Duration = Duration::from_secs(60); + +/// Rate-limits capacity-rejection logging. +/// +/// A refused connection is precisely the event a flood generates at line rate, +/// so emitting one line per rejection turns the flood into unbounded log-write +/// amplification on the shared relay. Rejections are counted instead and +/// summarised at most once per interval; the first rejection after a quiet +/// period still logs immediately so operators are not left without a signal. +struct RejectionLog { + summary_started: Instant, + pending_exhausted: u64, + source_exhausted: u64, +} + +impl Default for RejectionLog { + fn default() -> Self { + Self { + summary_started: Instant::now() + .checked_sub(REJECTION_SUMMARY_INTERVAL) + .unwrap_or_else(Instant::now), + pending_exhausted: 0, + source_exhausted: 0, + } + } +} + +impl RejectionLog { + fn record_pending_exhausted(&mut self) { + self.pending_exhausted = self.pending_exhausted.saturating_add(1); + self.summarize_if_due(); + } + + fn record_source_exhausted(&mut self) { + self.source_exhausted = self.source_exhausted.saturating_add(1); + self.summarize_if_due(); + } + + fn summarize_if_due(&mut self) { + let now = Instant::now(); + if now.saturating_duration_since(self.summary_started) < REJECTION_SUMMARY_INTERVAL { + return; + } + self.summary_started = now; + self.emit(); + } + + fn flush_pending_summary(&mut self) { + if self.pending_exhausted == 0 && self.source_exhausted == 0 { + return; + } + self.emit(); + } + + fn emit(&mut self) { + tracing::warn!( + pending_exhausted = self.pending_exhausted, + source_exhausted = self.source_exhausted, + "relay refused connections at capacity" + ); + self.pending_exhausted = 0; + self.source_exhausted = 0; + } +} diff --git a/crates/op-collab-relay-server/src/tests.rs b/crates/op-collab-relay-server/src/tests.rs index 21c7d5408..ea32e3054 100644 --- a/crates/op-collab-relay-server/src/tests.rs +++ b/crates/op-collab-relay-server/src/tests.rs @@ -596,6 +596,87 @@ fn authentication_concurrency_must_be_non_zero() { )); } +#[test] +fn reauthentication_concurrency_must_be_non_zero() { + let mut config = test_config(); + config.max_reauth_in_flight = 0; + assert!(matches!( + config.validate(), + Err(crate::ConfigError::Zero("max_reauth_in_flight")) + )); +} + +#[test] +fn source_budget_must_not_exceed_the_global_pending_ceiling() { + let mut config = test_config(); + config.max_pending = 4; + config.max_pending_per_source = 5; + assert!(matches!( + config.validate(), + Err(crate::ConfigError::SourceBudgetExceedsGlobal) + )); +} + +#[tokio::test] +async fn one_source_cannot_hold_more_than_its_share_of_pending_slots() { + let mut config = test_config(); + config.waiting_timeout = Duration::from_secs(5); + config.max_pending_per_source = 1; + let server = TestServer::start(config).await; + + // An un-paired connection holds its admission slot for the whole wait, so + // the second connection from the same source must be refused outright. + let waiting = connect_ready(&server, &make_valid_hello(RelayRole::Owner, 41)).await; + assert!(connect(server.address, "/v1/tunnel").await.is_err()); + + // Releasing the first connection returns the slot to that source. + drop(waiting); + let mut admitted = None; + for _ in 0..50 { + if let Ok(socket) = connect(server.address, "/v1/tunnel").await { + admitted = Some(socket); + break; + } + time::sleep(Duration::from_millis(20)).await; + } + assert!( + admitted.is_some(), + "a released source slot admits the next connection" + ); + drop(admitted); + server.stop().await; +} + +#[tokio::test] +async fn pairing_releases_the_source_budget() { + let mut config = test_config(); + config.waiting_timeout = Duration::from_secs(5); + config.max_pending_per_source = 2; + let server = TestServer::start(config).await; + + let owner_hello = make_valid_hello(RelayRole::Owner, 42); + let guest_hello = make_valid_hello(RelayRole::Guest, 42); + let mut owner = connect_ready(&server, &owner_hello).await; + let mut guest = connect_ready(&server, &guest_hello).await; + assert_eq!(next_status(&mut owner).await, RelayServerStatus::Paired); + assert_eq!(next_status(&mut guest).await, RelayServerStatus::Paired); + + // Both peers are paired, so neither still charges the pre-pairing budget + // and two fresh connections from the same source fit again. + let mut admitted = 0; + for _ in 0..50 { + if connect(server.address, "/v1/tunnel").await.is_ok() { + admitted += 1; + if admitted == 2 { + break; + } + } + time::sleep(Duration::from_millis(20)).await; + } + assert_eq!(admitted, 2, "paired peers release their source slots"); + server.stop().await; +} + #[test] fn bearer_token_requires_data_before_optional_padding() { for invalid in [ diff --git a/crates/op-collab-relay-server/src/x25519_boundary.rs b/crates/op-collab-relay-server/src/x25519_boundary.rs index 5da627b79..8c380087b 100644 --- a/crates/op-collab-relay-server/src/x25519_boundary.rs +++ b/crates/op-collab-relay-server/src/x25519_boundary.rs @@ -209,10 +209,20 @@ fn read_private_key_file(path: &Path, maximum: usize) -> Result, PinnedX } #[cfg(unix)] { - use std::os::unix::fs::PermissionsExt as _; + use std::os::unix::fs::{MetadataExt as _, PermissionsExt as _}; if metadata.permissions().mode() & 0o077 != 0 { return Err(PinnedX25519KeyError::UnsafePermissions); } + // Mode bits alone do not establish trust: a 0600 key owned by another + // local account is readable by that account, not just by this process. + // Requiring the running user to own it closes that gap. Root is allowed + // because it can read the file regardless of ownership. + let owner = metadata.uid(); + // SAFETY: `getuid` is always successful and takes no arguments. + let current = unsafe { libc::getuid() }; + if owner != current && current != 0 { + return Err(PinnedX25519KeyError::UnsafeOwner); + } } if metadata.len() > maximum as u64 { return Err(PinnedX25519KeyError::FileTooLarge { maximum }); @@ -264,6 +274,8 @@ pub enum PinnedX25519KeyError { UnsafeFile, #[error("relay X25519 key file permissions must deny group and other access")] UnsafePermissions, + #[error("relay X25519 key file must be owned by the user running the relay")] + UnsafeOwner, #[error("relay X25519 key file exceeds {maximum} bytes")] FileTooLarge { maximum: usize }, #[error("relay X25519 key file is malformed")] diff --git a/crates/op-collab-smoke/src/owner.rs b/crates/op-collab-smoke/src/owner.rs index 8bdce82d6..8c30db1bb 100644 --- a/crates/op-collab-smoke/src/owner.rs +++ b/crates/op-collab-smoke/src/owner.rs @@ -10,7 +10,7 @@ use op_collab::{ PeerNamespace, Role, Submit, }; use op_collab_transport::{ - accept_secure_tcp, m1_wire_limits, AdmissionHello, ConnectionLimiter, DeviceStaticKey, + accept_secure_tcp_guarded, m1_wire_limits, AdmissionHello, ConnectionLimiter, DeviceStaticKey, EncodedFrameTransfer, JoinIntent, ServerPrelude, TransportConfig, }; use std::net::{SocketAddr, TcpListener, TcpStream}; @@ -78,7 +78,7 @@ fn serve( fixtures::session_id(), fixtures::EPOCH, )?; - let mut connection = accept_secure_tcp(stream, &owner_key, &prelude, config)?; + let mut connection = accept_secure_tcp_guarded(stream, &owner_key, &prelude, config, &pending)?; let local_hello = AdmissionHello::new(auth.ticket().to_vec(), JoinIntent::New)?; let (_, guest_identity) = connection.exchange_admission_responder( &local_hello, diff --git a/crates/op-collab-transport/README.md b/crates/op-collab-transport/README.md index 94571c576..fb023ba9f 100644 --- a/crates/op-collab-transport/README.md +++ b/crates/op-collab-transport/README.md @@ -77,13 +77,20 @@ loop { ``` For accepted sockets, acquire `ConnectionLimiter::try_begin_handshake` before -calling `accept_secure_tcp`, and convert the pending guard to an active guard -only after ticket admission succeeds. +spawning a worker, call `accept_secure_tcp_guarded`, and convert the pending +guard to an active guard only after ticket admission succeeds. Every live +pending worker continuously owns a global and per-address seat; the guarded +accept applies `handshake_first_message` as the socket deadline until the +first valid Noise message, so a silent worker exits before its seat is freed. The inbound direction policy is part of allocation admission. In particular, an owner rejects guest-originated `Snapshot` transfers from their authenticated header before reserving the snapshot reassembly budget; a guest permits -owner-originated snapshots for initial sync and log-gap recovery. +owner-originated snapshots for initial sync and log-gap recovery. The same +trusted direction selects the pre-parse JSON envelope ceiling, so a guest's +attacker-declared message kind cannot select the larger owner Snapshot budget. +An aggregate reassembly reservation remains charged after the final chunk and +is released only after the completed bytes have been decoded or dropped. `SecureConnection` also exposes a blocking helper API for focused tools and tests. Its `send_transfer` atomically preflights the entire transfer against diff --git a/crates/op-collab-transport/src/chunk.rs b/crates/op-collab-transport/src/chunk.rs index 3c6f8e37f..0cbc1e7ec 100644 --- a/crates/op-collab-transport/src/chunk.rs +++ b/crates/op-collab-transport/src/chunk.rs @@ -4,8 +4,9 @@ use std::time::Instant; use zeroize::Zeroizing; use crate::{ - ChunkError, TimeoutConfig, MAX_CONTROL_TRANSFER_BYTES, MAX_NOISE_PLAINTEXT_BYTES, - MAX_SNAPSHOT_TRANSFER_BYTES, MAX_TXN_TRANSFER_BYTES, + ChunkError, SharedReassemblyBudget, SharedReassemblyReservation, TimeoutConfig, + MAX_CONTROL_TRANSFER_BYTES, MAX_NOISE_PLAINTEXT_BYTES, MAX_SNAPSHOT_TRANSFER_BYTES, + MAX_TXN_TRANSFER_BYTES, }; pub const CHUNK_HEADER_BYTES: usize = 24; @@ -111,13 +112,25 @@ impl ChunkHeader { } } -#[derive(PartialEq, Eq)] pub struct CompletedTransfer { pub(crate) class: TransferClass, pub(crate) transfer_id: u64, pub(crate) bytes: Zeroizing>, + /// Keeps the aggregate reservation charged while the completed bytes are + /// decoded or retained by the caller. + pub(crate) _reservation: Option, } +impl PartialEq for CompletedTransfer { + fn eq(&self, other: &Self) -> bool { + self.class == other.class + && self.transfer_id == other.transfer_id + && self.bytes == other.bytes + } +} + +impl Eq for CompletedTransfer {} + impl CompletedTransfer { pub const fn class(&self) -> TransferClass { self.class @@ -254,6 +267,9 @@ struct InFlightTransfer { next_index: u32, started_at: Instant, bytes: Zeroizing>, + /// Aggregate reservation for `header.total_len`. Completion transfers it + /// alongside the bytes; abort, timeout, or reassembler drop releases it. + _reservation: Option, } impl fmt::Debug for InFlightTransfer { @@ -271,14 +287,28 @@ impl fmt::Debug for InFlightTransfer { #[derive(Debug)] pub struct Reassembler { timeouts: TimeoutConfig, + budget: Option, in_flight: Option, last_started_id: Option, } impl Reassembler { + /// Builds an unbudgeted reassembler. Production connections use + /// [`Self::with_budget`] so their declared allocations are visible to the + /// aggregate inbound bound. pub const fn new(timeouts: TimeoutConfig) -> Self { Self { timeouts, + budget: None, + in_flight: None, + last_started_id: None, + } + } + + pub fn with_budget(timeouts: TimeoutConfig, budget: SharedReassemblyBudget) -> Self { + Self { + timeouts, + budget: Some(budget), in_flight: None, last_started_id: None, } @@ -363,12 +393,26 @@ impl Reassembler { expected: 0, }); } + // The declared total is charged against the aggregate inbound bound + // before it is allocated, so a peer cannot reserve memory the + // process-wide budget cannot see. + let declared = header.total_len as usize; + let reservation = match &self.budget { + Some(budget) => Some(budget.reserve(declared).map_err(|_| { + ChunkError::InboundBudgetExhausted { + class: header.class, + requested: declared, + } + })?), + None => None, + }; self.last_started_id = Some(header.transfer_id); self.in_flight = Some(InFlightTransfer { header, next_index: 0, started_at: now, - bytes: Zeroizing::new(Vec::with_capacity(header.total_len as usize)), + bytes: Zeroizing::new(Vec::with_capacity(declared)), + _reservation: reservation, }); } @@ -403,6 +447,7 @@ impl Reassembler { class: completed.header.class, transfer_id: completed.header.transfer_id, bytes: completed.bytes, + _reservation: completed._reservation, })) } } @@ -452,333 +497,5 @@ fn expected_payload_len(total_len: usize, chunk_index: u32) -> Result: Clone); - - fn mutate_header(mut chunk: Vec, offset: usize, value: u8) -> Vec { - chunk[offset] = value; - chunk - } - - #[test] - fn header_encoding_is_exact_and_big_endian() { - let header = ChunkHeader { - class: TransferClass::Txn, - transfer_id: 0x0102_0304_0506_0708, - chunk_index: 0x1112_1314, - chunk_count: 0x0000_0002, - total_len: 0x0000_f001, - }; - let encoded = header.encode(); - assert_eq!( - encoded, - [ - 1, 3, 0, 0, 1, 2, 3, 4, 5, 6, 7, 8, 0x11, 0x12, 0x13, 0x14, 0, 0, 0, 2, 0, 0, 0xf0, - 1, - ] - ); - assert_eq!(ChunkHeader::decode(&encoded), Ok(header)); - } - - #[test] - fn iterator_and_reassembler_cover_chunk_boundaries() { - let timeouts = TimeoutConfig::default(); - let now = Instant::now(); - for (class, len) in [ - (TransferClass::Control, 1), - (TransferClass::Control, MAX_CHUNK_PAYLOAD), - (TransferClass::Control, MAX_CHUNK_PAYLOAD + 1), - (TransferClass::Txn, MAX_TXN_TRANSFER_BYTES), - ] { - let source = vec![0xa5; len]; - let chunks = TransferChunkIter::new(class, 7, &source) - .unwrap() - .collect::>(); - assert_eq!(chunks.len(), len.div_ceil(MAX_CHUNK_PAYLOAD)); - assert!(chunks - .iter() - .all(|chunk| chunk.len() <= MAX_NOISE_PLAINTEXT_BYTES)); - - let mut reassembler = Reassembler::new(timeouts); - let mut completed = None; - for chunk in chunks { - completed = reassembler.push(now, &chunk).unwrap(); - } - assert_eq!(completed.unwrap().bytes.as_slice(), source); - } - } - - #[test] - fn ticket_chunk_debug_only_reports_class_and_lengths() { - let source = vec![211_u8; MAX_CHUNK_PAYLOAD + 1]; - let mut chunks = TransferChunkIter::new(TransferClass::Ticket, 7, &source).unwrap(); - let iter_debug = format!("{chunks:?}"); - assert!(iter_debug.contains("class: Ticket")); - assert!(iter_debug.contains(&format!("encoded_len: {}", source.len()))); - assert!(!iter_debug.contains("211, 211")); - - let now = Instant::now(); - let mut reassembler = Reassembler::new(TimeoutConfig::default()); - assert_eq!( - reassembler.push(now, &chunks.next().unwrap()).unwrap(), - None - ); - let in_flight_debug = format!("{reassembler:?}"); - assert!(in_flight_debug.contains("class: Ticket")); - assert!(!in_flight_debug.contains("211, 211")); - - let completed = reassembler - .push(now, &chunks.next().unwrap()) - .unwrap() - .unwrap(); - let completed_debug = format!("{completed:?}"); - assert!(completed_debug.contains("class: Ticket")); - assert!(completed_debug.contains(&format!("encoded_len: {}", source.len()))); - assert!(!completed_debug.contains("211, 211")); - } - - #[test] - fn class_caps_are_enforced_before_splitting_or_allocating() { - for class in [ - TransferClass::Control, - TransferClass::Ticket, - TransferClass::Txn, - TransferClass::Snapshot, - ] { - let oversized = vec![0; class.max_transfer_bytes() + 1]; - let exact = - TransferChunkIter::new(class, 1, &oversized[..class.max_transfer_bytes()]).unwrap(); - assert_eq!( - exact.chunk_count() as usize, - class.max_transfer_bytes().div_ceil(MAX_CHUNK_PAYLOAD) - ); - assert_eq!( - TransferChunkIter::new(class, 1, &oversized).unwrap_err(), - ChunkError::TransferTooLarge { - class, - actual: oversized.len(), - maximum: class.max_transfer_bytes(), - } - ); - } - } - - #[test] - fn malformed_headers_fail_closed() { - let valid = TransferChunkIter::new(TransferClass::Control, 1, b"x") - .unwrap() - .next() - .unwrap(); - let mut reassembler = Reassembler::new(TimeoutConfig::default()); - let now = Instant::now(); - - assert_eq!( - reassembler.push(now, &[0; 23]), - Err(ChunkError::HeaderLength(23)) - ); - assert_eq!( - reassembler.push(now, &mutate_header(valid.to_vec(), 0, 2)), - Err(ChunkError::UnsupportedVersion(2)) - ); - assert_eq!( - reassembler.push(now, &mutate_header(valid.to_vec(), 2, 1)), - Err(ChunkError::ReservedBits) - ); - assert_eq!( - reassembler.push(now, &mutate_header(valid.to_vec(), 1, 99)), - Err(ChunkError::UnknownClass(99)) - ); - } - - #[test] - fn order_mismatch_clears_in_flight_state() { - let source = vec![9; MAX_CHUNK_PAYLOAD + 1]; - let chunks = TransferChunkIter::new(TransferClass::Control, 4, &source) - .unwrap() - .collect::>(); - let now = Instant::now(); - let mut reassembler = Reassembler::new(TimeoutConfig::default()); - - assert_eq!(reassembler.push(now, &chunks[0]).unwrap(), None); - assert_eq!( - reassembler.push(now, &chunks[0]), - Err(ChunkError::UnexpectedChunkIndex { - actual: 0, - expected: 1, - }) - ); - assert_eq!( - reassembler.push(now, &chunks[1]), - Err(ChunkError::ReplayedTransferId { - actual: 4, - previous: 4, - }) - ); - let next = TransferChunkIter::new(TransferClass::Control, 5, &source) - .unwrap() - .collect::>(); - assert_eq!(reassembler.push(now, &next[0]).unwrap(), None); - assert!(reassembler.push(now, &next[1]).unwrap().is_some()); - } - - #[test] - fn completed_transfer_ids_must_increase() { - let now = Instant::now(); - let mut reassembler = Reassembler::new(TimeoutConfig::default()); - let transfer = |id| { - TransferChunkIter::new(TransferClass::Control, id, b"x") - .unwrap() - .next() - .unwrap() - }; - - assert!(reassembler.push(now, &transfer(10)).unwrap().is_some()); - assert_eq!( - reassembler.push(now, &transfer(10)), - Err(ChunkError::ReplayedTransferId { - actual: 10, - previous: 10, - }) - ); - assert_eq!( - reassembler.push(now, &transfer(9)), - Err(ChunkError::ReplayedTransferId { - actual: 9, - previous: 10, - }) - ); - assert!(reassembler.push(now, &transfer(11)).unwrap().is_some()); - } - - #[test] - fn transfer_timeout_uses_class_specific_deadline_and_clears_state() { - let timeouts = TimeoutConfig { - ordinary_transfer: Duration::from_secs(2), - snapshot_transfer: Duration::from_secs(8), - ..TimeoutConfig::default() - }; - let now = Instant::now(); - let mut reassembler = Reassembler::new(timeouts); - let source = vec![1; MAX_CHUNK_PAYLOAD + 1]; - let control = TransferChunkIter::new(TransferClass::Control, 1, &source) - .unwrap() - .collect::>(); - - assert_eq!(reassembler.push(now, &control[0]).unwrap(), None); - assert_eq!( - reassembler.next_deadline(), - Some(now + Duration::from_secs(2)) - ); - assert_eq!( - reassembler.push(now + Duration::from_secs(2), &control[1]), - Err(ChunkError::TimedOut(Duration::from_secs(2))) - ); - let retry = TransferChunkIter::new(TransferClass::Control, 2, &source) - .unwrap() - .collect::>(); - assert_eq!(reassembler.push(now, &retry[0]).unwrap(), None); - assert!(reassembler.push(now, &retry[1]).unwrap().is_some()); - - let snapshot = TransferChunkIter::new(TransferClass::Snapshot, 3, &source) - .unwrap() - .collect::>(); - assert_eq!(reassembler.push(now, &snapshot[0]).unwrap(), None); - assert_eq!( - reassembler - .push(now + Duration::from_secs(2), &snapshot[1]) - .unwrap() - .unwrap() - .transfer_id, - 3 - ); - } - - #[test] - fn transfer_timeout_fires_without_another_chunk() { - let timeouts = TimeoutConfig { - ordinary_transfer: Duration::from_secs(2), - snapshot_transfer: Duration::from_secs(8), - ..TimeoutConfig::default() - }; - let start = Instant::now(); - let source = vec![1; MAX_CHUNK_PAYLOAD + 1]; - let first = TransferChunkIter::new(TransferClass::Snapshot, 1, &source) - .unwrap() - .next() - .unwrap(); - let mut reassembler = Reassembler::new(timeouts); - - assert_eq!(reassembler.push(start, &first).unwrap(), None); - assert_eq!( - reassembler.check_timeout(start + Duration::from_secs(7)), - Ok(()) - ); - assert_eq!( - reassembler.check_timeout(start + Duration::from_secs(8)), - Err(ChunkError::TimedOut(Duration::from_secs(8))) - ); - assert_eq!(reassembler.next_deadline(), None); - } - - #[test] - fn exact_payload_lengths_are_required() { - let source = vec![3; MAX_CHUNK_PAYLOAD + 1]; - let mut chunks = TransferChunkIter::new(TransferClass::Control, 3, &source) - .unwrap() - .collect::>(); - chunks[0].0.pop(); - let mut reassembler = Reassembler::new(TimeoutConfig::default()); - - assert_eq!( - reassembler.push(Instant::now(), &chunks[0]), - Err(ChunkError::InvalidPayloadLength { - actual: MAX_CHUNK_PAYLOAD - 1, - expected: MAX_CHUNK_PAYLOAD, - }) - ); - } - - #[test] - fn forged_count_or_total_is_rejected_and_clears_state() { - let source = vec![4; MAX_CHUNK_PAYLOAD + 1]; - let chunks = TransferChunkIter::new(TransferClass::Control, 12, &source) - .unwrap() - .collect::>(); - let now = Instant::now(); - let mut reassembler = Reassembler::new(TimeoutConfig::default()); - - let mut forged_count = chunks[0].to_vec(); - forged_count[16..20].copy_from_slice(&3_u32.to_be_bytes()); - assert_eq!( - reassembler.push(now, &forged_count), - Err(ChunkError::InvalidChunkCount { - actual: 3, - expected: 2, - }) - ); - - assert_eq!(reassembler.push(now, &chunks[0]).unwrap(), None); - let mut forged_total = chunks[1].to_vec(); - forged_total[20..24].copy_from_slice(&(source.len() as u32 - 1).to_be_bytes()); - assert_eq!( - reassembler.push(now, &forged_total), - Err(ChunkError::InvalidChunkCount { - actual: 2, - expected: 1, - }) - ); - - let retry = TransferChunkIter::new(TransferClass::Control, 13, &source) - .unwrap() - .collect::>(); - assert_eq!(reassembler.push(now, &retry[0]).unwrap(), None); - assert!(reassembler.push(now, &retry[1]).unwrap().is_some()); - } -} +#[path = "chunk_tests.rs"] +mod tests; diff --git a/crates/op-collab-transport/src/chunk_tests.rs b/crates/op-collab-transport/src/chunk_tests.rs new file mode 100644 index 000000000..c430dd4aa --- /dev/null +++ b/crates/op-collab-transport/src/chunk_tests.rs @@ -0,0 +1,432 @@ +use std::time::Duration; + +use super::*; +use static_assertions::assert_not_impl_any; + +assert_not_impl_any!(CompletedTransfer: Clone); +assert_not_impl_any!(TransferChunk: Clone); +assert_not_impl_any!(TransferChunkIter<'static>: Clone); + +fn mutate_header(mut chunk: Vec, offset: usize, value: u8) -> Vec { + chunk[offset] = value; + chunk +} + +#[test] +fn header_encoding_is_exact_and_big_endian() { + let header = ChunkHeader { + class: TransferClass::Txn, + transfer_id: 0x0102_0304_0506_0708, + chunk_index: 0x1112_1314, + chunk_count: 0x0000_0002, + total_len: 0x0000_f001, + }; + let encoded = header.encode(); + assert_eq!( + encoded, + [1, 3, 0, 0, 1, 2, 3, 4, 5, 6, 7, 8, 0x11, 0x12, 0x13, 0x14, 0, 0, 0, 2, 0, 0, 0xf0, 1,] + ); + assert_eq!(ChunkHeader::decode(&encoded), Ok(header)); +} + +#[test] +fn iterator_and_reassembler_cover_chunk_boundaries() { + let timeouts = TimeoutConfig::default(); + let now = Instant::now(); + for (class, len) in [ + (TransferClass::Control, 1), + (TransferClass::Control, MAX_CHUNK_PAYLOAD), + (TransferClass::Control, MAX_CHUNK_PAYLOAD + 1), + (TransferClass::Txn, MAX_TXN_TRANSFER_BYTES), + ] { + let source = vec![0xa5; len]; + let chunks = TransferChunkIter::new(class, 7, &source) + .unwrap() + .collect::>(); + assert_eq!(chunks.len(), len.div_ceil(MAX_CHUNK_PAYLOAD)); + assert!(chunks + .iter() + .all(|chunk| chunk.len() <= MAX_NOISE_PLAINTEXT_BYTES)); + + let mut reassembler = Reassembler::new(timeouts); + let mut completed = None; + for chunk in chunks { + completed = reassembler.push(now, &chunk).unwrap(); + } + assert_eq!(completed.unwrap().bytes.as_slice(), source); + } +} + +#[test] +fn ticket_chunk_debug_only_reports_class_and_lengths() { + let source = vec![211_u8; MAX_CHUNK_PAYLOAD + 1]; + let mut chunks = TransferChunkIter::new(TransferClass::Ticket, 7, &source).unwrap(); + let iter_debug = format!("{chunks:?}"); + assert!(iter_debug.contains("class: Ticket")); + assert!(iter_debug.contains(&format!("encoded_len: {}", source.len()))); + assert!(!iter_debug.contains("211, 211")); + + let now = Instant::now(); + let mut reassembler = Reassembler::new(TimeoutConfig::default()); + assert_eq!( + reassembler.push(now, &chunks.next().unwrap()).unwrap(), + None + ); + let in_flight_debug = format!("{reassembler:?}"); + assert!(in_flight_debug.contains("class: Ticket")); + assert!(!in_flight_debug.contains("211, 211")); + + let completed = reassembler + .push(now, &chunks.next().unwrap()) + .unwrap() + .unwrap(); + let completed_debug = format!("{completed:?}"); + assert!(completed_debug.contains("class: Ticket")); + assert!(completed_debug.contains(&format!("encoded_len: {}", source.len()))); + assert!(!completed_debug.contains("211, 211")); +} + +#[test] +fn class_caps_are_enforced_before_splitting_or_allocating() { + for class in [ + TransferClass::Control, + TransferClass::Ticket, + TransferClass::Txn, + TransferClass::Snapshot, + ] { + let oversized = vec![0; class.max_transfer_bytes() + 1]; + let exact = + TransferChunkIter::new(class, 1, &oversized[..class.max_transfer_bytes()]).unwrap(); + assert_eq!( + exact.chunk_count() as usize, + class.max_transfer_bytes().div_ceil(MAX_CHUNK_PAYLOAD) + ); + assert_eq!( + TransferChunkIter::new(class, 1, &oversized).unwrap_err(), + ChunkError::TransferTooLarge { + class, + actual: oversized.len(), + maximum: class.max_transfer_bytes(), + } + ); + } +} + +#[test] +fn malformed_headers_fail_closed() { + let valid = TransferChunkIter::new(TransferClass::Control, 1, b"x") + .unwrap() + .next() + .unwrap(); + let mut reassembler = Reassembler::new(TimeoutConfig::default()); + let now = Instant::now(); + + assert_eq!( + reassembler.push(now, &[0; 23]), + Err(ChunkError::HeaderLength(23)) + ); + assert_eq!( + reassembler.push(now, &mutate_header(valid.to_vec(), 0, 2)), + Err(ChunkError::UnsupportedVersion(2)) + ); + assert_eq!( + reassembler.push(now, &mutate_header(valid.to_vec(), 2, 1)), + Err(ChunkError::ReservedBits) + ); + assert_eq!( + reassembler.push(now, &mutate_header(valid.to_vec(), 1, 99)), + Err(ChunkError::UnknownClass(99)) + ); +} + +#[test] +fn order_mismatch_clears_in_flight_state() { + let source = vec![9; MAX_CHUNK_PAYLOAD + 1]; + let chunks = TransferChunkIter::new(TransferClass::Control, 4, &source) + .unwrap() + .collect::>(); + let now = Instant::now(); + let mut reassembler = Reassembler::new(TimeoutConfig::default()); + + assert_eq!(reassembler.push(now, &chunks[0]).unwrap(), None); + assert_eq!( + reassembler.push(now, &chunks[0]), + Err(ChunkError::UnexpectedChunkIndex { + actual: 0, + expected: 1, + }) + ); + assert_eq!( + reassembler.push(now, &chunks[1]), + Err(ChunkError::ReplayedTransferId { + actual: 4, + previous: 4, + }) + ); + let next = TransferChunkIter::new(TransferClass::Control, 5, &source) + .unwrap() + .collect::>(); + assert_eq!(reassembler.push(now, &next[0]).unwrap(), None); + assert!(reassembler.push(now, &next[1]).unwrap().is_some()); +} + +#[test] +fn completed_transfer_ids_must_increase() { + let now = Instant::now(); + let mut reassembler = Reassembler::new(TimeoutConfig::default()); + let transfer = |id| { + TransferChunkIter::new(TransferClass::Control, id, b"x") + .unwrap() + .next() + .unwrap() + }; + + assert!(reassembler.push(now, &transfer(10)).unwrap().is_some()); + assert_eq!( + reassembler.push(now, &transfer(10)), + Err(ChunkError::ReplayedTransferId { + actual: 10, + previous: 10, + }) + ); + assert_eq!( + reassembler.push(now, &transfer(9)), + Err(ChunkError::ReplayedTransferId { + actual: 9, + previous: 10, + }) + ); + assert!(reassembler.push(now, &transfer(11)).unwrap().is_some()); +} + +#[test] +fn transfer_timeout_uses_class_specific_deadline_and_clears_state() { + let timeouts = TimeoutConfig { + ordinary_transfer: Duration::from_secs(2), + snapshot_transfer: Duration::from_secs(8), + ..TimeoutConfig::default() + }; + let now = Instant::now(); + let mut reassembler = Reassembler::new(timeouts); + let source = vec![1; MAX_CHUNK_PAYLOAD + 1]; + let control = TransferChunkIter::new(TransferClass::Control, 1, &source) + .unwrap() + .collect::>(); + + assert_eq!(reassembler.push(now, &control[0]).unwrap(), None); + assert_eq!( + reassembler.next_deadline(), + Some(now + Duration::from_secs(2)) + ); + assert_eq!( + reassembler.push(now + Duration::from_secs(2), &control[1]), + Err(ChunkError::TimedOut(Duration::from_secs(2))) + ); + let retry = TransferChunkIter::new(TransferClass::Control, 2, &source) + .unwrap() + .collect::>(); + assert_eq!(reassembler.push(now, &retry[0]).unwrap(), None); + assert!(reassembler.push(now, &retry[1]).unwrap().is_some()); + + let snapshot = TransferChunkIter::new(TransferClass::Snapshot, 3, &source) + .unwrap() + .collect::>(); + assert_eq!(reassembler.push(now, &snapshot[0]).unwrap(), None); + assert_eq!( + reassembler + .push(now + Duration::from_secs(2), &snapshot[1]) + .unwrap() + .unwrap() + .transfer_id, + 3 + ); +} + +#[test] +fn transfer_timeout_fires_without_another_chunk() { + let timeouts = TimeoutConfig { + ordinary_transfer: Duration::from_secs(2), + snapshot_transfer: Duration::from_secs(8), + ..TimeoutConfig::default() + }; + let start = Instant::now(); + let source = vec![1; MAX_CHUNK_PAYLOAD + 1]; + let first = TransferChunkIter::new(TransferClass::Snapshot, 1, &source) + .unwrap() + .next() + .unwrap(); + let mut reassembler = Reassembler::new(timeouts); + + assert_eq!(reassembler.push(start, &first).unwrap(), None); + assert_eq!( + reassembler.check_timeout(start + Duration::from_secs(7)), + Ok(()) + ); + assert_eq!( + reassembler.check_timeout(start + Duration::from_secs(8)), + Err(ChunkError::TimedOut(Duration::from_secs(8))) + ); + assert_eq!(reassembler.next_deadline(), None); +} + +#[test] +fn exact_payload_lengths_are_required() { + let source = vec![3; MAX_CHUNK_PAYLOAD + 1]; + let mut chunks = TransferChunkIter::new(TransferClass::Control, 3, &source) + .unwrap() + .collect::>(); + chunks[0].0.pop(); + let mut reassembler = Reassembler::new(TimeoutConfig::default()); + + assert_eq!( + reassembler.push(Instant::now(), &chunks[0]), + Err(ChunkError::InvalidPayloadLength { + actual: MAX_CHUNK_PAYLOAD - 1, + expected: MAX_CHUNK_PAYLOAD, + }) + ); +} + +#[test] +fn forged_count_or_total_is_rejected_and_clears_state() { + let source = vec![4; MAX_CHUNK_PAYLOAD + 1]; + let chunks = TransferChunkIter::new(TransferClass::Control, 12, &source) + .unwrap() + .collect::>(); + let now = Instant::now(); + let mut reassembler = Reassembler::new(TimeoutConfig::default()); + + let mut forged_count = chunks[0].to_vec(); + forged_count[16..20].copy_from_slice(&3_u32.to_be_bytes()); + assert_eq!( + reassembler.push(now, &forged_count), + Err(ChunkError::InvalidChunkCount { + actual: 3, + expected: 2, + }) + ); + + assert_eq!(reassembler.push(now, &chunks[0]).unwrap(), None); + let mut forged_total = chunks[1].to_vec(); + forged_total[20..24].copy_from_slice(&(source.len() as u32 - 1).to_be_bytes()); + assert_eq!( + reassembler.push(now, &forged_total), + Err(ChunkError::InvalidChunkCount { + actual: 2, + expected: 1, + }) + ); + + let retry = TransferChunkIter::new(TransferClass::Control, 13, &source) + .unwrap() + .collect::>(); + assert_eq!(reassembler.push(now, &retry[0]).unwrap(), None); + assert!(reassembler.push(now, &retry[1]).unwrap().is_some()); +} + +#[test] +fn completed_transfer_holds_the_declared_reservation_until_drop() { + let source = vec![7_u8; MAX_CONTROL_TRANSFER_BYTES]; + let chunks = TransferChunkIter::new(TransferClass::Control, 1, &source) + .unwrap() + .collect::>(); + let budget = SharedReassemblyBudget::new(source.len()).unwrap(); + let mut reassembler = Reassembler::with_budget(TimeoutConfig::default(), budget.clone()); + let now = Instant::now(); + + assert_eq!(reassembler.push(now, &chunks[0]).unwrap(), None); + assert_eq!( + budget.used().unwrap(), + source.len(), + "the declared total is charged on chunk 0, before the buffer grows" + ); + let completed = reassembler.push(now, &chunks[1]).unwrap().unwrap(); + assert_eq!(completed.bytes(), source.as_slice()); + assert_eq!(budget.used().unwrap(), source.len()); + drop(completed); + assert_eq!(budget.used().unwrap(), 0); +} + +#[test] +fn inbound_reassembly_budget_rejects_over_aggregate_reservations() { + let source = vec![9_u8; MAX_CHUNK_PAYLOAD + 1]; + let chunks = TransferChunkIter::new(TransferClass::Control, 1, &source) + .unwrap() + .collect::>(); + let budget = SharedReassemblyBudget::new(source.len()).unwrap(); + let mut first = Reassembler::with_budget(TimeoutConfig::default(), budget.clone()); + let mut second = Reassembler::with_budget(TimeoutConfig::default(), budget.clone()); + let now = Instant::now(); + + assert_eq!(first.push(now, &chunks[0]).unwrap(), None); + assert_eq!( + second.push(now, &chunks[0]), + Err(ChunkError::InboundBudgetExhausted { + class: TransferClass::Control, + requested: source.len(), + }) + ); + assert_eq!(budget.used().unwrap(), source.len()); + + // The rejected transfer never started, so the same id may be retried once + // the aggregate has room again. + drop(first); + assert_eq!(budget.used().unwrap(), 0); + assert_eq!(second.push(now, &chunks[0]).unwrap(), None); + assert!(second.push(now, &chunks[1]).unwrap().is_some()); +} + +#[test] +fn inbound_reassembly_budget_releases_on_abort_timeout_and_drop() { + let timeouts = TimeoutConfig { + ordinary_transfer: Duration::from_secs(2), + ..TimeoutConfig::default() + }; + let source = vec![5_u8; MAX_CHUNK_PAYLOAD + 1]; + let chunks = TransferChunkIter::new(TransferClass::Control, 1, &source) + .unwrap() + .collect::>(); + let budget = SharedReassemblyBudget::new(source.len() * 4).unwrap(); + let now = Instant::now(); + + let mut aborted = Reassembler::with_budget(timeouts, budget.clone()); + assert_eq!(aborted.push(now, &chunks[0]).unwrap(), None); + assert_eq!(budget.used().unwrap(), source.len()); + assert!(aborted.push(now, &chunks[0]).is_err()); + assert_eq!(budget.used().unwrap(), 0, "an aborted transfer releases"); + + let mut expired = Reassembler::with_budget(timeouts, budget.clone()); + let later = TransferChunkIter::new(TransferClass::Control, 2, &source) + .unwrap() + .next() + .unwrap(); + assert_eq!(expired.push(now, &later).unwrap(), None); + assert_eq!(budget.used().unwrap(), source.len()); + assert_eq!( + expired.check_timeout(now + Duration::from_secs(2)), + Err(ChunkError::TimedOut(Duration::from_secs(2))) + ); + assert_eq!(budget.used().unwrap(), 0, "a timed-out transfer releases"); + + let mut dropped = Reassembler::with_budget(timeouts, budget.clone()); + let orphan = TransferChunkIter::new(TransferClass::Control, 3, &source) + .unwrap() + .next() + .unwrap(); + assert_eq!(dropped.push(now, &orphan).unwrap(), None); + assert_eq!(budget.used().unwrap(), source.len()); + drop(dropped); + assert_eq!( + budget.used().unwrap(), + 0, + "dropping the reassembler releases" + ); + + let mut unbudgeted = Reassembler::new(timeouts); + let untracked = TransferChunkIter::new(TransferClass::Control, 4, &source) + .unwrap() + .next() + .unwrap(); + assert_eq!(unbudgeted.push(now, &untracked).unwrap(), None); + assert_eq!(budget.used().unwrap(), 0); +} diff --git a/crates/op-collab-transport/src/config.rs b/crates/op-collab-transport/src/config.rs index 6eba1edd3..9516aa70c 100644 --- a/crates/op-collab-transport/src/config.rs +++ b/crates/op-collab-transport/src/config.rs @@ -23,11 +23,40 @@ pub const M1_MAX_PRESENCE_BYTES: u32 = 44 * 1024; /// Leaves room for the Submit/Commit envelope around the encoded transaction. pub const M1_MAX_TXN_BODY_BYTES: u32 = (MAX_TXN_TRANSFER_BYTES - 32 * 1024) as u32; -pub const DEFAULT_MAX_PENDING_HANDSHAKES: usize = 16; +/// Global ceiling on concurrent pending handshakes. +/// +/// A pending seat is taken before any cryptographic work and is only converted +/// into an active connection after ticket admission, so the ceiling has to keep +/// honest joins working while unauthenticated peers are connecting. The cost of +/// one pending seat is bounded by the accepted socket plus the largest buffers a +/// pre-admission connection can own: one Noise record +/// (`MAX_NOISE_CIPHERTEXT_BYTES`, 60 KiB + 16 B) and one reassembling Ticket +/// transfer (`MAX_TICKET_TRANSFER_BYTES`, 64 KiB), i.e. under 128 KiB. At this +/// ceiling the worst case is 128 seats x 128 KiB = 16 MiB held for at most +/// `timeouts.handshake` (5 s), and — because a seat is also capped per source +/// address — filling it needs 128 / `DEFAULT_MAX_PENDING_HANDSHAKES_PER_IP` = 32 +/// distinct addresses instead of the 4 the previous 16-seat ceiling needed. +/// +/// The ceiling alone is not the defence: guarded owner accepts enforce +/// `timeouts.handshake_first_message` as the actual socket read deadline. A +/// silent connection must exit and drop its continuously charged guard before +/// its global seat is released. See [`crate::ConnectionLimiter`]. +pub const DEFAULT_MAX_PENDING_HANDSHAKES: usize = 128; pub const DEFAULT_MAX_PENDING_HANDSHAKES_PER_IP: usize = 4; pub const DEFAULT_MAX_ACTIVE_CONNECTIONS: usize = 64; pub const DEFAULT_OUTBOUND_QUEUE_ITEMS: usize = 8; pub const DEFAULT_GLOBAL_QUEUED_BYTES: usize = 256 * 1024 * 1024; +/// Aggregate ceiling for inbound reassembly buffers across every connection. +/// +/// Inbound transfers allocate the peer-declared total on chunk 0, so the +/// per-class caps alone bound only one connection at a time: 64 authenticated +/// peers x `MAX_TXN_TRANSFER_BYTES` (4 MiB) is 256 MiB of buffer no +/// per-connection limit can see. This aggregate admits one full owner snapshot +/// (`MAX_SNAPSHOT_TRANSFER_BYTES`, 64 MiB) plus 16 concurrent maximum-size +/// transactions (16 x 4 MiB) and refuses the rest with a typed error, holding +/// transport-owned inbound heap to half the outbound aggregate +/// (`DEFAULT_GLOBAL_QUEUED_BYTES`). +pub const DEFAULT_GLOBAL_INBOUND_REASSEMBLY_BYTES: usize = 128 * 1024 * 1024; pub const MAX_CONFIGURED_CONNECTIONS: usize = 256; pub const MAX_CONFIGURED_QUEUE_ITEMS: usize = 1_024; pub const MAX_CONFIGURED_QUEUE_BYTES: usize = 512 * 1024 * 1024; @@ -49,6 +78,11 @@ pub fn m1_wire_limits() -> WireLimits { pub struct TimeoutConfig { pub connect: Duration, pub handshake: Duration, + /// Actual socket-read deadline in which a freshly accepted connection must + /// produce its first valid handshake message. The connection is closed and + /// its continuously held pending seat is released on expiry; after valid + /// progress, the remaining handshake uses the full `handshake` deadline. + pub handshake_first_message: Duration, pub admission: Duration, pub ordinary_transfer: Duration, pub snapshot_transfer: Duration, @@ -62,6 +96,11 @@ impl Default for TimeoutConfig { Self { connect: Duration::from_secs(5), handshake: Duration::from_secs(5), + // One round trip after TCP establishment is enough for an honest + // initiator, including relay-mediated paths, while keeping a silent + // peer's hold on a global pending seat to a fifth of the handshake + // window. + handshake_first_message: Duration::from_secs(1), admission: Duration::from_secs(10), ordinary_transfer: Duration::from_secs(10), snapshot_transfer: Duration::from_secs(60), @@ -234,6 +273,7 @@ impl TransportConfig { if [ timeouts.connect, timeouts.handshake, + timeouts.handshake_first_message, timeouts.admission, timeouts.ordinary_transfer, timeouts.snapshot_transfer, @@ -243,6 +283,7 @@ impl TransportConfig { ] .into_iter() .any(|timeout| timeout.is_zero() || timeout > MAX_CONFIGURED_TIMEOUT) + || timeouts.handshake_first_message > timeouts.handshake || timeouts.snapshot_transfer < timeouts.ordinary_transfer || timeouts.read_write > timeouts.idle || timeouts.admission > timeouts.idle @@ -272,6 +313,30 @@ mod tests { assert!(config.rate.byte_burst >= maximum_snapshot_wire_bytes as u64); } + #[test] + fn pending_ceiling_leaves_headroom_for_honest_joins_under_a_silent_flood() { + let config = TransportConfig::default().validate().unwrap(); + let connections = config.connections; + // The documented flood: every address opens its per-address maximum. + // The ceiling must still leave seats for honest guests, and a silent + // peer only holds its seat for the first-message window. + let flood_addresses = 4; + let flood_seats = flood_addresses * connections.max_pending_handshakes_per_ip; + assert!(flood_seats < connections.max_pending_handshakes); + assert!( + connections.max_pending_handshakes / connections.max_pending_handshakes_per_ip >= 32, + "filling the pending pool must require many distinct source addresses" + ); + assert!(config.timeouts.handshake_first_message < config.timeouts.handshake); + assert!(connections.max_pending_handshakes <= MAX_CONFIGURED_CONNECTIONS); + } + + #[test] + fn inbound_reassembly_aggregate_admits_a_full_snapshot() { + assert!(DEFAULT_GLOBAL_INBOUND_REASSEMBLY_BYTES >= MAX_SNAPSHOT_TRANSFER_BYTES); + assert!(DEFAULT_GLOBAL_INBOUND_REASSEMBLY_BYTES <= DEFAULT_GLOBAL_QUEUED_BYTES); + } + #[test] fn invalid_resource_limits_fail_closed() { let mut config = TransportConfig::default(); @@ -321,5 +386,20 @@ mod tests { config.validate(), Err(ConfigError::InvalidValue { field: "timeouts" }) ); + + let mut config = TransportConfig::default(); + config.timeouts.handshake_first_message = + config.timeouts.handshake + Duration::from_secs(1); + assert_eq!( + config.validate(), + Err(ConfigError::InvalidValue { field: "timeouts" }) + ); + + let mut config = TransportConfig::default(); + config.timeouts.handshake_first_message = Duration::ZERO; + assert_eq!( + config.validate(), + Err(ConfigError::InvalidValue { field: "timeouts" }) + ); } } diff --git a/crates/op-collab-transport/src/connection_limit.rs b/crates/op-collab-transport/src/connection_limit.rs index 3106651c3..2b904aee6 100644 --- a/crates/op-collab-transport/src/connection_limit.rs +++ b/crates/op-collab-transport/src/connection_limit.rs @@ -1,8 +1,9 @@ use std::collections::HashMap; use std::net::IpAddr; use std::sync::{Arc, Mutex}; +use std::time::Instant; -use crate::{ConfigError, ConnectionLimits, TransportConfig}; +use crate::{ConfigError, ConnectionLimits, TimeoutConfig, TransportConfig}; #[derive(Debug, thiserror::Error, Clone, Copy, PartialEq, Eq)] pub enum ConnectionLimitError { @@ -12,19 +13,31 @@ pub enum ConnectionLimitError { PendingHandshakesPerIpFull, #[error("the active-connection limit is full")] ActiveConnectionsFull, + #[error("the pending-handshake guard no longer owns a global seat")] + PendingHandshakeSeatLost, #[error("the connection limiter is unavailable")] Unavailable, } #[derive(Debug, Clone, Copy, PartialEq, Eq)] pub struct ConnectionCounts { + /// Global pending seats in use. Every live pending guard owns one. pub pending_handshakes: usize, + /// Live pending guards. Kept separate for observability even though the + /// fail-closed accounting requires it to equal `pending_handshakes`. + pub pending_guards: usize, pub active_connections: usize, } +#[derive(Debug)] +struct PendingSlot { + peer_ip: IpAddr, +} + #[derive(Debug)] struct LimiterState { - pending: usize, + slots: HashMap, + next_slot: u64, pending_by_ip: HashMap, active: usize, } @@ -40,6 +53,12 @@ struct LimiterInner { /// A host acquires a pending guard before starting any Noise work, then turns /// it into an active guard only after ticket admission. Dropping either guard /// releases its count, including on early-return error paths. +/// +/// Every live guard continuously owns both its global and per-address seats. +/// The owner-side guarded TCP accept enforces `handshake_first_message` as a +/// real socket read deadline; a silent connection exits and drops its guard +/// before either seat is released. Accounting is never reclaimed while the +/// socket, worker, or cryptographic handshake remains alive. #[derive(Debug, Clone)] pub struct ConnectionLimiter { inner: Arc, @@ -47,8 +66,18 @@ pub struct ConnectionLimiter { impl ConnectionLimiter { pub fn new(limits: ConnectionLimits) -> Result { + Self::with_timeouts(limits, TimeoutConfig::default()) + } + + /// Builds a limiter after validating the connection and handshake limits + /// that its guarded accept path will enforce. + pub fn with_timeouts( + limits: ConnectionLimits, + timeouts: TimeoutConfig, + ) -> Result { TransportConfig { connections: limits, + timeouts, ..TransportConfig::default() } .validate()?; @@ -56,7 +85,8 @@ impl ConnectionLimiter { inner: Arc::new(LimiterInner { limits, state: Mutex::new(LimiterState { - pending: 0, + slots: HashMap::new(), + next_slot: 1, pending_by_ip: HashMap::new(), active: 0, }), @@ -67,37 +97,58 @@ impl ConnectionLimiter { pub fn try_begin_handshake( &self, peer_ip: IpAddr, + ) -> Result { + self.try_begin_handshake_at(peer_ip, Instant::now()) + } + + /// Monotonic-clock variant of [`Self::try_begin_handshake`]. + pub fn try_begin_handshake_at( + &self, + peer_ip: IpAddr, + _now: Instant, ) -> Result { let mut state = self .inner .state .lock() .map_err(|_| ConnectionLimitError::Unavailable)?; - if state.pending >= self.inner.limits.max_pending_handshakes { + if state.slots.len() >= self.inner.limits.max_pending_handshakes { return Err(ConnectionLimitError::PendingHandshakesFull); } let per_ip = state.pending_by_ip.get(&peer_ip).copied().unwrap_or(0); if per_ip >= self.inner.limits.max_pending_handshakes_per_ip { return Err(ConnectionLimitError::PendingHandshakesPerIpFull); } - state.pending += 1; + let slot = state.next_slot; + state.next_slot = slot + .checked_add(1) + .ok_or(ConnectionLimitError::Unavailable)?; + state.slots.insert(slot, PendingSlot { peer_ip }); state.pending_by_ip.insert(peer_ip, per_ip + 1); drop(state); Ok(PendingHandshakeGuard { inner: Arc::clone(&self.inner), + slot, peer_ip, held: true, }) } pub fn counts(&self) -> Result { + self.counts_at(Instant::now()) + } + + /// Monotonic-clock variant of [`Self::counts`]. Time never changes a live + /// guard's accounting; only dropping or activating it releases the seat. + pub fn counts_at(&self, _now: Instant) -> Result { let state = self .inner .state .lock() .map_err(|_| ConnectionLimitError::Unavailable)?; Ok(ConnectionCounts { - pending_handshakes: state.pending, + pending_handshakes: state.slots.len(), + pending_guards: state.slots.len(), active_connections: state.active, }) } @@ -106,6 +157,7 @@ impl ConnectionLimiter { #[derive(Debug)] pub struct PendingHandshakeGuard { inner: Arc, + slot: u64, peer_ip: IpAddr, held: bool, } @@ -115,16 +167,53 @@ impl PendingHandshakeGuard { self.peer_ip } + /// Records that the peer produced its first valid handshake message. + /// + /// The guard already owns its seat continuously. This check lets the TCP + /// accept path fail immediately if that invariant is ever broken. + pub fn note_handshake_progress(&self) -> Result<(), ConnectionLimitError> { + self.note_handshake_progress_at(Instant::now()) + } + + /// Monotonic-clock variant of [`Self::note_handshake_progress`]. + pub fn note_handshake_progress_at(&self, _now: Instant) -> Result<(), ConnectionLimitError> { + let state = self + .inner + .state + .lock() + .map_err(|_| ConnectionLimitError::Unavailable)?; + if !self.held || !state.slots.contains_key(&self.slot) { + return Err(ConnectionLimitError::PendingHandshakeSeatLost); + } + Ok(()) + } + + /// Reports whether this connection currently occupies a global seat. + pub fn holds_global_seat(&self) -> bool { + self.holds_global_seat_at(Instant::now()) + } + + /// Monotonic-clock variant of [`Self::holds_global_seat`]. + pub fn holds_global_seat_at(&self, _now: Instant) -> bool { + let Ok(state) = self.inner.state.lock() else { + return false; + }; + self.held && state.slots.contains_key(&self.slot) + } + pub fn activate(mut self) -> Result { let mut state = self .inner .state .lock() .map_err(|_| ConnectionLimitError::Unavailable)?; + if !self.held || !state.slots.contains_key(&self.slot) { + return Err(ConnectionLimitError::PendingHandshakeSeatLost); + } if state.active >= self.inner.limits.max_active_connections { return Err(ConnectionLimitError::ActiveConnectionsFull); } - release_pending(&mut state, self.peer_ip); + release_pending(&mut state, self.slot); state.active += 1; self.held = false; drop(state); @@ -146,7 +235,7 @@ impl Drop for PendingHandshakeGuard { .state .lock() .unwrap_or_else(|poisoned| poisoned.into_inner()); - release_pending(&mut state, self.peer_ip); + release_pending(&mut state, self.slot); self.held = false; } } @@ -179,75 +268,18 @@ impl Drop for ActiveConnectionGuard { } } -fn release_pending(state: &mut LimiterState, peer_ip: IpAddr) { - state.pending = state.pending.saturating_sub(1); - if let Some(per_ip) = state.pending_by_ip.get_mut(&peer_ip) { +fn release_pending(state: &mut LimiterState, slot: u64) { + let Some(slot) = state.slots.remove(&slot) else { + return; + }; + if let Some(per_ip) = state.pending_by_ip.get_mut(&slot.peer_ip) { *per_ip = per_ip.saturating_sub(1); if *per_ip == 0 { - state.pending_by_ip.remove(&peer_ip); + state.pending_by_ip.remove(&slot.peer_ip); } } } #[cfg(test)] -mod tests { - use super::*; - use std::net::{Ipv4Addr, Ipv6Addr}; - - fn limits() -> ConnectionLimits { - ConnectionLimits { - max_pending_handshakes: 2, - max_pending_handshakes_per_ip: 1, - max_active_connections: 1, - ..ConnectionLimits::default() - } - } - - #[test] - fn pending_limits_are_exact_and_drop_releases_them() { - let limiter = ConnectionLimiter::new(limits()).unwrap(); - let first_ip = IpAddr::V4(Ipv4Addr::LOCALHOST); - let second_ip = IpAddr::V6(Ipv6Addr::LOCALHOST); - let first = limiter.try_begin_handshake(first_ip).unwrap(); - assert!(matches!( - limiter.try_begin_handshake(first_ip), - Err(ConnectionLimitError::PendingHandshakesPerIpFull) - )); - let second = limiter.try_begin_handshake(second_ip).unwrap(); - assert!(matches!( - limiter.try_begin_handshake(IpAddr::V4(Ipv4Addr::new(192, 0, 2, 1))), - Err(ConnectionLimitError::PendingHandshakesFull) - )); - drop(first); - assert_eq!(limiter.counts().unwrap().pending_handshakes, 1); - drop(second); - assert_eq!(limiter.counts().unwrap().pending_handshakes, 0); - } - - #[test] - fn activation_is_bounded_and_raii_releases_both_phases() { - let limiter = ConnectionLimiter::new(limits()).unwrap(); - let first = limiter - .try_begin_handshake(IpAddr::V4(Ipv4Addr::LOCALHOST)) - .unwrap(); - let active = first.activate().unwrap(); - assert_eq!( - limiter.counts().unwrap(), - ConnectionCounts { - pending_handshakes: 0, - active_connections: 1, - } - ); - - let pending = limiter - .try_begin_handshake(IpAddr::V6(Ipv6Addr::LOCALHOST)) - .unwrap(); - assert!(matches!( - pending.activate(), - Err(ConnectionLimitError::ActiveConnectionsFull) - )); - assert_eq!(limiter.counts().unwrap().pending_handshakes, 0); - drop(active); - assert_eq!(limiter.counts().unwrap().active_connections, 0); - } -} +#[path = "connection_limit_tests.rs"] +mod tests; diff --git a/crates/op-collab-transport/src/connection_limit_tests.rs b/crates/op-collab-transport/src/connection_limit_tests.rs new file mode 100644 index 000000000..40a8d088c --- /dev/null +++ b/crates/op-collab-transport/src/connection_limit_tests.rs @@ -0,0 +1,189 @@ +use super::*; +use std::net::{Ipv4Addr, Ipv6Addr}; +use std::time::Duration; + +const FIRST_MESSAGE: Duration = Duration::from_millis(200); + +fn limits() -> ConnectionLimits { + ConnectionLimits { + max_pending_handshakes: 2, + max_pending_handshakes_per_ip: 1, + max_active_connections: 1, + ..ConnectionLimits::default() + } +} + +fn timeouts() -> TimeoutConfig { + TimeoutConfig { + handshake_first_message: FIRST_MESSAGE, + ..TimeoutConfig::default() + } +} + +fn limiter(limits: ConnectionLimits) -> ConnectionLimiter { + ConnectionLimiter::with_timeouts(limits, timeouts()).unwrap() +} + +fn address(last: u8) -> IpAddr { + IpAddr::V4(Ipv4Addr::new(198, 51, 100, last)) +} + +#[test] +fn pending_limits_are_exact_and_drop_releases_them() { + let limiter = limiter(limits()); + let now = Instant::now(); + let first_ip = IpAddr::V4(Ipv4Addr::LOCALHOST); + let second_ip = IpAddr::V6(Ipv6Addr::LOCALHOST); + let first = limiter.try_begin_handshake_at(first_ip, now).unwrap(); + assert!(matches!( + limiter.try_begin_handshake_at(first_ip, now), + Err(ConnectionLimitError::PendingHandshakesPerIpFull) + )); + let second = limiter.try_begin_handshake_at(second_ip, now).unwrap(); + assert!(matches!( + limiter.try_begin_handshake_at(address(1), now), + Err(ConnectionLimitError::PendingHandshakesFull) + )); + drop(first); + assert_eq!(limiter.counts_at(now).unwrap().pending_handshakes, 1); + drop(second); + assert_eq!(limiter.counts_at(now).unwrap().pending_handshakes, 0); +} + +#[test] +fn activation_is_bounded_and_raii_releases_both_phases() { + let limiter = limiter(limits()); + let now = Instant::now(); + let first = limiter + .try_begin_handshake_at(IpAddr::V4(Ipv4Addr::LOCALHOST), now) + .unwrap(); + let active = first.activate().unwrap(); + assert_eq!( + limiter.counts_at(now).unwrap(), + ConnectionCounts { + pending_handshakes: 0, + pending_guards: 0, + active_connections: 1, + } + ); + + let pending = limiter + .try_begin_handshake_at(IpAddr::V6(Ipv6Addr::LOCALHOST), now) + .unwrap(); + assert!(matches!( + pending.activate(), + Err(ConnectionLimitError::ActiveConnectionsFull) + )); + assert_eq!(limiter.counts_at(now).unwrap().pending_handshakes, 0); + drop(active); + assert_eq!(limiter.counts_at(now).unwrap().active_connections, 0); +} + +#[test] +fn live_silent_guards_stay_charged_until_the_socket_worker_drops_them() { + let limiter = limiter(limits()); + let start = Instant::now(); + let silent_first = limiter.try_begin_handshake_at(address(1), start).unwrap(); + let silent_second = limiter.try_begin_handshake_at(address(2), start).unwrap(); + assert!(matches!( + limiter.try_begin_handshake_at(address(3), start), + Err(ConnectionLimitError::PendingHandshakesFull) + )); + + let after_deadline = start + FIRST_MESSAGE * 10; + assert!(silent_first.holds_global_seat_at(after_deadline)); + assert!(silent_second.holds_global_seat_at(after_deadline)); + let counts = limiter.counts_at(after_deadline).unwrap(); + assert_eq!(counts.pending_handshakes, 2); + assert_eq!(counts.pending_guards, 2); + assert!(matches!( + limiter.try_begin_handshake_at(address(3), after_deadline), + Err(ConnectionLimitError::PendingHandshakesFull) + )); + + drop(silent_first); + let replacement = limiter + .try_begin_handshake_at(address(3), after_deadline) + .unwrap(); + drop(silent_second); + drop(replacement); + assert_eq!( + limiter.counts_at(after_deadline).unwrap(), + ConnectionCounts { + pending_handshakes: 0, + pending_guards: 0, + active_connections: 0, + } + ); +} + +#[test] +fn valid_handshake_progress_preserves_the_continuously_held_seat() { + let limiter = limiter(limits()); + let start = Instant::now(); + let proven = limiter.try_begin_handshake_at(address(1), start).unwrap(); + let silent = limiter.try_begin_handshake_at(address(2), start).unwrap(); + proven.note_handshake_progress_at(start).unwrap(); + + let long_after = start + FIRST_MESSAGE * 10; + assert!(proven.holds_global_seat_at(long_after)); + assert!(silent.holds_global_seat_at(long_after)); + assert_eq!( + limiter.counts_at(long_after).unwrap().pending_handshakes, + 2, + "every live socket worker must keep charging the global pool" + ); + assert!(matches!( + limiter.try_begin_handshake_at(address(3), long_after), + Err(ConnectionLimitError::PendingHandshakesFull) + )); + drop(silent); +} + +#[test] +fn progress_and_activation_fail_if_a_guard_ever_loses_its_seat() { + let limiter = limiter(limits()); + let start = Instant::now(); + let pending = limiter.try_begin_handshake_at(address(1), start).unwrap(); + { + let mut state = limiter.inner.state.lock().unwrap(); + release_pending(&mut state, pending.slot); + } + + assert_eq!( + pending.note_handshake_progress_at(start), + Err(ConnectionLimitError::PendingHandshakeSeatLost) + ); + assert!(matches!( + pending.activate(), + Err(ConnectionLimitError::PendingHandshakeSeatLost) + )); +} + +#[test] +fn default_ceiling_keeps_honest_joins_working_under_a_silent_flood() { + let limits = ConnectionLimits::default(); + let limiter = ConnectionLimiter::new(limits).unwrap(); + let now = Instant::now(); + let flood: Vec<_> = (0..4_u8) + .flat_map(|source| (0..limits.max_pending_handshakes_per_ip).map(move |_| address(source))) + .map(|peer| limiter.try_begin_handshake_at(peer, now).unwrap()) + .collect(); + assert_eq!(flood.len(), 4 * limits.max_pending_handshakes_per_ip); + + // Four addresses at their per-address maximum still leave space under the + // reviewed global ceiling for honest guests. + let guests: Vec<_> = (100..164_u8) + .map(|guest| { + limiter + .try_begin_handshake_at(address(guest), now) + .expect("honest guests still get a pending seat during the flood") + }) + .collect(); + assert_eq!( + limiter.counts_at(now).unwrap().pending_handshakes, + flood.len() + guests.len() + ); + drop(guests); + drop(flood); +} diff --git a/crates/op-collab-transport/src/discovery.rs b/crates/op-collab-transport/src/discovery.rs index 649f9a4c4..ad3c90dfa 100644 --- a/crates/op-collab-transport/src/discovery.rs +++ b/crates/op-collab-transport/src/discovery.rs @@ -678,9 +678,13 @@ fn disabled_listener_family(endpoint: SocketAddr, dual_stack: bool) -> Option bool { !address.is_unspecified() && !address.is_multicast() + && !matches!(address, IpAddr::V4(address) if address.is_link_local()) && !matches!(address, IpAddr::V6(address) if address.is_unicast_link_local()) } diff --git a/crates/op-collab-transport/src/discovery_tests.rs b/crates/op-collab-transport/src/discovery_tests.rs index 44525f3ef..e10e10a18 100644 --- a/crates/op-collab-transport/src/discovery_tests.rs +++ b/crates/op-collab-transport/src/discovery_tests.rs @@ -306,3 +306,47 @@ fn uncertain_unregister_failure_stops_publisher() { .is_err()); assert!(publisher.is_stopped()); } + +#[test] +fn ipv4_link_local_advertisements_are_not_dialled() { + let link_local = IpAddr::V4(Ipv4Addr::new(169, 254, 13, 37)); + let routable = IpAddr::V4(Ipv4Addr::new(192, 0, 2, 10)); + assert!(!is_usable_address(&link_local)); + assert!(!is_usable_address(&IpAddr::V6("fe80::1".parse().unwrap()))); + assert!(is_usable_address(&routable)); + + // A mixed advertisement keeps only the routable address. + let mixed = service( + "openpencil-mixed", + &valid_properties(), + &[link_local, routable], + 45123, + ); + let parsed = parse_service_info(&mixed, Instant::now()).unwrap(); + assert_eq!( + parsed.addresses(), + [SocketAddr::new(routable, 45123)].as_slice() + ); + + // A link-local-only advertisement leaves nothing to dial. + let only = service( + "openpencil-link-local", + &valid_properties(), + &[link_local], + 45123, + ); + assert!(matches!( + parse_service_info(&only, Instant::now()), + Err(DiscoveryError::InvalidMetadata) + )); + + // Publishing one is refused on the same rule as its IPv6 counterpart. + assert!(matches!( + validate_publish_addresses(&[link_local]), + Err(DiscoveryError::InvalidAddress) + )); + assert!(matches!( + build_service_info(ID, 45123, &[link_local], "instance", "host.local."), + Err(DiscoveryError::InvalidAddress) + )); +} diff --git a/crates/op-collab-transport/src/driver.rs b/crates/op-collab-transport/src/driver.rs index 79496c90f..4778aee96 100644 --- a/crates/op-collab-transport/src/driver.rs +++ b/crates/op-collab-transport/src/driver.rs @@ -2,7 +2,7 @@ use std::io::Write; use std::net::TcpStream; use std::time::{Duration, Instant}; -use op_collab::FrameEnvelope; +use op_collab::{FrameEnvelope, InboundFrameDirection}; use zeroize::Zeroizing; use crate::driver_io::{ @@ -51,6 +51,13 @@ impl InboundTransferPolicy { const fn allows(self, class: TransferClass) -> bool { !matches!((self, class), (Self::PeerToOwner, TransferClass::Snapshot)) } + + const fn frame_direction(self) -> InboundFrameDirection { + match self { + Self::PeerToOwner => InboundFrameDirection::GuestToOwner, + Self::OwnerToGuest => InboundFrameDirection::OwnerToGuest, + } + } } struct OutboundTransfer { @@ -568,7 +575,10 @@ impl ConnectionDriver { let Some(ciphertext) = self.inbound_ciphertext.as_ref() else { return Ok((progressed, None, None)); }; - let expected_plaintext = ciphertext.len().saturating_sub(16) as u64; + let expected_plaintext = ciphertext + .len() + .saturating_sub(crate::config::NOISE_AEAD_TAG_BYTES) + as u64; let ready = rate_ready_at( &mut self.connection.inbound_records, &mut self.connection.inbound_bytes, @@ -611,6 +621,7 @@ impl ConnectionDriver { class: completed.class, transfer_id: completed.transfer_id, bytes: completed.bytes, + _reservation: completed._reservation, }; let event = self.decode_event(transfer)?; Ok((true, Some(event), None)) @@ -643,6 +654,7 @@ impl ConnectionDriver { transfer.class, &transfer.bytes, self.connection.config.wire_limits, + self.inbound_policy.frame_direction(), )?; Ok(DriverEvent::Frame { frame, encoded_len }) } diff --git a/crates/op-collab-transport/src/error.rs b/crates/op-collab-transport/src/error.rs index dca73b29a..86d9b8b48 100644 --- a/crates/op-collab-transport/src/error.rs +++ b/crates/op-collab-transport/src/error.rs @@ -52,6 +52,11 @@ pub enum ChunkError { InvalidPayloadLength { actual: usize, expected: usize }, #[error("transfer timed out after {0:?}")] TimedOut(Duration), + #[error("inbound reassembly budget rejected a {class:?} transfer of {requested} bytes")] + InboundBudgetExhausted { + class: TransferClass, + requested: usize, + }, #[error("transfer length arithmetic overflow")] LengthOverflow, } @@ -88,6 +93,8 @@ pub enum NoiseTransportError { HandshakeFrameLength(usize), #[error("Noise handshake did not expose a 32-byte remote static key")] MissingRemoteStatic, + #[error("Noise handshake lost its pending-connection seat")] + PendingSeatUnavailable, } #[derive(Debug, thiserror::Error)] diff --git a/crates/op-collab-transport/src/frame.rs b/crates/op-collab-transport/src/frame.rs index f59707d53..e4f5bbaef 100644 --- a/crates/op-collab-transport/src/frame.rs +++ b/crates/op-collab-transport/src/frame.rs @@ -4,7 +4,7 @@ use std::sync::Arc; use op_collab::{ decode_renew_ticket_frame_from_json_slice, encode_commit_frame_to_json_vec, encode_renew_ticket_frame_to_zeroizing_json, CollabMessage, Commit, Epoch, FrameEnvelope, - OpaqueTicket, SensitiveFrameJson, SessionId, WireLimits, + InboundFrameDirection, OpaqueTicket, SensitiveFrameJson, SessionId, WireLimits, }; use crate::queue::QueueItem; @@ -114,9 +114,13 @@ impl EncodedFrameTransfer { } /// Decodes this validated transfer under the same wire limits. - pub fn decode(&self, limits: WireLimits) -> Result { + pub fn decode( + &self, + limits: WireLimits, + inbound_direction: InboundFrameDirection, + ) -> Result { self.validate_for(limits)?; - decode_frame_transfer(self.class, self.bytes(), limits) + decode_frame_transfer(self.class, self.bytes(), limits, inbound_direction) } pub(crate) fn validate_for( @@ -196,12 +200,17 @@ pub fn decode_frame_transfer( declared_class: TransferClass, encoded: &[u8], limits: WireLimits, + inbound_direction: InboundFrameDirection, ) -> Result { enforce_class_limit(declared_class, encoded.len())?; let frame = if declared_class == TransferClass::Ticket { decode_renew_ticket_frame_from_json_slice(encoded, limits)? } else { - FrameEnvelope::from_json_slice_with_limits(encoded, limits)? + FrameEnvelope::from_json_slice_with_limits_for_direction( + encoded, + limits, + inbound_direction, + )? }; let actual = frame_transfer_class(&frame); if actual != declared_class { @@ -251,10 +260,15 @@ mod tests { let (class, bytes) = encode_frame_transfer(&control, m1_wire_limits()).unwrap(); assert_eq!(class, TransferClass::Control); assert_eq!( - decode_frame_transfer(class, &bytes, m1_wire_limits()) - .unwrap() - .body() - .kind(), + decode_frame_transfer( + class, + &bytes, + m1_wire_limits(), + InboundFrameDirection::GuestToOwner, + ) + .unwrap() + .body() + .kind(), "bye" ); @@ -276,7 +290,12 @@ mod tests { })); let bytes = control.to_json_vec_with_limits(m1_wire_limits()).unwrap(); assert!(matches!( - decode_frame_transfer(TransferClass::Txn, &bytes, m1_wire_limits()), + decode_frame_transfer( + TransferClass::Txn, + &bytes, + m1_wire_limits(), + InboundFrameDirection::GuestToOwner, + ), Err(FrameTransportError::ClassMismatch { .. }) )); } @@ -298,7 +317,12 @@ mod tests { TransferClass::Snapshot, ] { assert!(matches!( - decode_frame_transfer(declared_class, malformed_ticket_payload, m1_wire_limits()), + decode_frame_transfer( + declared_class, + malformed_ticket_payload, + m1_wire_limits(), + InboundFrameDirection::GuestToOwner, + ), Err(FrameTransportError::Protocol( op_collab::ProtocolError::SensitiveCredentialRequiresDedicatedCodec )) @@ -354,7 +378,9 @@ mod tests { assert!(debug.contains(&format!("encoded_len: {}", encoded.encoded_len()))); assert!(!debug.contains(SECRET)); assert!(!debug.contains(&format!("{:?}", SECRET.as_bytes()))); - let decoded = encoded.decode(m1_wire_limits()).unwrap(); + let decoded = encoded + .decode(m1_wire_limits(), InboundFrameDirection::GuestToOwner) + .unwrap(); let CollabMessage::RenewTicket(decoded) = decoded.into_body() else { panic!("dedicated ticket codec must retain the message kind"); }; diff --git a/crates/op-collab-transport/src/lib.rs b/crates/op-collab-transport/src/lib.rs index d34b269e0..5e0c9c682 100644 --- a/crates/op-collab-transport/src/lib.rs +++ b/crates/op-collab-transport/src/lib.rs @@ -24,6 +24,7 @@ mod noise; mod os_key_store; mod prelude; mod queue; +mod reassembly_budget; mod record; mod runtime; mod tcp; @@ -40,9 +41,10 @@ pub use chunk::{ }; pub use config::{ m1_wire_limits, ConnectionLimits, RateLimitConfig, TimeoutConfig, TransportConfig, - MAX_CONTROL_TRANSFER_BYTES, MAX_NOISE_CIPHERTEXT_BYTES, MAX_NOISE_PLAINTEXT_BYTES, - MAX_SNAPSHOT_TRANSFER_BYTES, MAX_TICKET_BYTES, MAX_TICKET_TRANSFER_BYTES, - MAX_TXN_TRANSFER_BYTES, + DEFAULT_GLOBAL_INBOUND_REASSEMBLY_BYTES, DEFAULT_MAX_PENDING_HANDSHAKES, + DEFAULT_MAX_PENDING_HANDSHAKES_PER_IP, MAX_CONTROL_TRANSFER_BYTES, MAX_NOISE_CIPHERTEXT_BYTES, + MAX_NOISE_PLAINTEXT_BYTES, MAX_SNAPSHOT_TRANSFER_BYTES, MAX_TICKET_BYTES, + MAX_TICKET_TRANSFER_BYTES, MAX_TXN_TRANSFER_BYTES, }; pub use connection_limit::{ ActiveConnectionGuard, ConnectionCounts, ConnectionLimitError, ConnectionLimiter, @@ -70,12 +72,13 @@ pub use prelude::{ TRANSPORT_PROTOCOL_VERSION, }; pub use queue::{SharedQueueBudget, SharedQueueReservation, TokenBucket}; +pub use reassembly_budget::{SharedReassemblyBudget, SharedReassemblyReservation}; pub use record::{ decrypt_record, encrypt_record, read_ciphertext_record, write_ciphertext_record, TRANSPORT_HEARTBEAT_PLAINTEXT, }; pub use runtime::{SecureConnection, TransportTransfer}; pub use tcp::{ - accept_secure_tcp, connect_manual, connect_secure_tcp, connect_secure_tcp_until, - connect_secure_tcp_until_cancellable, prepare_tcp_stream, + accept_secure_tcp, accept_secure_tcp_guarded, connect_manual, connect_secure_tcp, + connect_secure_tcp_until, connect_secure_tcp_until_cancellable, prepare_tcp_stream, }; diff --git a/crates/op-collab-transport/src/noise.rs b/crates/op-collab-transport/src/noise.rs index 79373d309..c509c5e4a 100644 --- a/crates/op-collab-transport/src/noise.rs +++ b/crates/op-collab-transport/src/noise.rs @@ -48,9 +48,24 @@ pub fn run_xx_responder( stream: &mut (impl Read + Write), local_static: &DeviceStaticKey, prelude: &EncodedServerPrelude, +) -> Result { + run_xx_responder_observed(stream, local_static, prelude, &mut || Ok(())) +} + +/// Responder variant that reports the first structurally valid Noise message. +/// +/// `on_first_message` runs only after the initiator's opening message has been +/// accepted by Noise, so an accept path can distinguish a peer that proved +/// liveness from one that merely opened a socket. +pub(crate) fn run_xx_responder_observed( + stream: &mut (impl Read + Write), + local_static: &DeviceStaticKey, + prelude: &EncodedServerPrelude, + on_first_message: &mut dyn FnMut() -> Result<(), NoiseTransportError>, ) -> Result { let mut handshake = build_handshake(local_static, prelude, false)?; read_handshake_message(stream, &mut handshake)?; + on_first_message()?; write_handshake_message(stream, &mut handshake)?; read_handshake_message(stream, &mut handshake)?; finish_handshake(handshake) diff --git a/crates/op-collab-transport/src/reassembly_budget.rs b/crates/op-collab-transport/src/reassembly_budget.rs new file mode 100644 index 000000000..55e8fbd9f --- /dev/null +++ b/crates/op-collab-transport/src/reassembly_budget.rs @@ -0,0 +1,152 @@ +use std::sync::{Arc, Mutex, OnceLock}; + +use crate::{QueueError, DEFAULT_GLOBAL_INBOUND_REASSEMBLY_BYTES}; + +#[derive(Debug)] +struct ReassemblyBudgetState { + maximum: usize, + used: usize, +} + +/// An aggregate byte budget for inbound reassembly buffers. +/// +/// A transfer allocates its peer-declared total on chunk 0, so per-connection +/// class caps bound only one connection at a time. This is the inbound twin of +/// [`crate::SharedQueueBudget`]: the declared total is reserved before the +/// buffer is allocated and the reservation is released by RAII only after the +/// completed bytes finish decode/drop, or when a transfer aborts or times out. +#[derive(Debug, Clone)] +pub struct SharedReassemblyBudget { + state: Arc>, +} + +/// An RAII reservation against a [`SharedReassemblyBudget`]. +#[derive(Debug)] +#[must_use = "dropping the reservation immediately releases its byte budget"] +pub struct SharedReassemblyReservation { + budget: SharedReassemblyBudget, + amount: usize, +} + +impl SharedReassemblyReservation { + pub const fn amount(&self) -> usize { + self.amount + } +} + +impl Drop for SharedReassemblyReservation { + fn drop(&mut self) { + self.budget.release(self.amount); + } +} + +impl SharedReassemblyBudget { + pub fn new(maximum: usize) -> Result { + if maximum == 0 { + return Err(QueueError::ByteBudget); + } + Ok(Self { + state: Arc::new(Mutex::new(ReassemblyBudgetState { maximum, used: 0 })), + }) + } + + /// The process-wide budget every connection is charged against unless its + /// host installs a narrower one. + /// + /// Inbound reassembly is started deep inside the record loop, far from any + /// host-owned session object, so the aggregate has to be reachable from the + /// default constructors. A host that wants a tighter per-session bound + /// passes its own budget to + /// [`crate::SecureConnection::with_inbound_budget`]. + pub fn process_default() -> Self { + static DEFAULT: OnceLock = OnceLock::new(); + DEFAULT + .get_or_init(|| { + Self::new(DEFAULT_GLOBAL_INBOUND_REASSEMBLY_BYTES) + .expect("the default inbound reassembly budget is a non-zero constant") + }) + .clone() + } + + pub fn used(&self) -> Result { + self.state + .lock() + .map(|state| state.used) + .map_err(|_| QueueError::Unavailable) + } + + pub fn maximum(&self) -> Result { + self.state + .lock() + .map(|state| state.maximum) + .map_err(|_| QueueError::Unavailable) + } + + pub fn reserve(&self, amount: usize) -> Result { + let mut state = self.state.lock().map_err(|_| QueueError::Unavailable)?; + let next = state + .used + .checked_add(amount) + .ok_or(QueueError::ByteBudget)?; + if next > state.maximum { + return Err(QueueError::ByteBudget); + } + state.used = next; + drop(state); + Ok(SharedReassemblyReservation { + budget: self.clone(), + amount, + }) + } + + fn release(&self, amount: usize) { + let mut state = self + .state + .lock() + .unwrap_or_else(|poisoned| poisoned.into_inner()); + state.used = state.used.saturating_sub(amount); + } +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn reservations_are_exact_and_release_on_drop() { + let budget = SharedReassemblyBudget::new(10).unwrap(); + assert_eq!(budget.maximum().unwrap(), 10); + let reservation = budget.reserve(7).unwrap(); + assert_eq!(reservation.amount(), 7); + assert_eq!(budget.used().unwrap(), 7); + assert!(matches!(budget.reserve(4), Err(QueueError::ByteBudget))); + drop(reservation); + assert_eq!(budget.used().unwrap(), 0); + assert_eq!(budget.reserve(10).unwrap().amount(), 10); + } + + #[test] + fn zero_and_overflowing_budgets_fail_closed() { + assert!(matches!( + SharedReassemblyBudget::new(0), + Err(QueueError::ByteBudget) + )); + let budget = SharedReassemblyBudget::new(usize::MAX).unwrap(); + let held = budget.reserve(usize::MAX - 1).unwrap(); + assert!(matches!(budget.reserve(2), Err(QueueError::ByteBudget))); + drop(held); + } + + #[test] + fn the_process_default_is_one_shared_aggregate() { + let first = SharedReassemblyBudget::process_default(); + let second = SharedReassemblyBudget::process_default(); + assert_eq!( + first.maximum().unwrap(), + DEFAULT_GLOBAL_INBOUND_REASSEMBLY_BYTES + ); + let reservation = first.reserve(4_096).unwrap(); + assert!(second.used().unwrap() >= 4_096); + drop(reservation); + } +} diff --git a/crates/op-collab-transport/src/runtime.rs b/crates/op-collab-transport/src/runtime.rs index 45f6d847a..1b443f930 100644 --- a/crates/op-collab-transport/src/runtime.rs +++ b/crates/op-collab-transport/src/runtime.rs @@ -2,22 +2,23 @@ use std::fmt; use std::io::{Read, Write}; use std::time::{Duration, Instant}; -use op_collab::{FrameEnvelope, Role}; +use op_collab::{FrameEnvelope, InboundFrameDirection, Role}; use zeroize::Zeroizing; use crate::record::read_ciphertext_record_until; use crate::{ decode_frame_transfer, encrypt_record, verify_initial_ticket, write_ciphertext_record, AdmissionError, AdmissionHello, AdmissionIdentity, AdmissionPhase, AdmissionState, ChunkHeader, - EncodedFrameTransfer, NoiseSession, Reassembler, RecordError, RuntimeError, TicketVerifier, - TokenBucket, TransferChunkIter, TransferClass, TransportConfig, CHUNK_HEADER_BYTES, - TRANSPORT_HEARTBEAT_PLAINTEXT, + EncodedFrameTransfer, NoiseSession, Reassembler, RecordError, RuntimeError, + SharedReassemblyBudget, TicketVerifier, TokenBucket, TransferChunkIter, TransferClass, + TransportConfig, CHUNK_HEADER_BYTES, TRANSPORT_HEARTBEAT_PLAINTEXT, }; pub struct TransportTransfer { pub(crate) class: TransferClass, pub(crate) transfer_id: u64, pub(crate) bytes: Zeroizing>, + pub(crate) _reservation: Option, } impl TransportTransfer { @@ -73,6 +74,24 @@ impl SecureConnection { noise: NoiseSession, config: TransportConfig, now: Instant, + ) -> Result { + Self::with_inbound_budget( + stream, + noise, + config, + now, + SharedReassemblyBudget::process_default(), + ) + } + + /// Builds a connection whose inbound reassembly buffers are charged against + /// `inbound_budget` instead of the process-wide aggregate. + pub fn with_inbound_budget( + stream: S, + noise: NoiseSession, + config: TransportConfig, + now: Instant, + inbound_budget: SharedReassemblyBudget, ) -> Result { let config = config.validate()?; let outbound_bytes = @@ -98,7 +117,7 @@ impl SecureConnection { stream, noise, config, - reassembler: Reassembler::new(config.timeouts), + reassembler: Reassembler::with_budget(config.timeouts, inbound_budget), next_send_id: Some(1), outbound_bytes, outbound_records, @@ -242,8 +261,19 @@ impl SecureConnection { fn receive_transfer_inner(&mut self) -> Result { self.check_idle(Instant::now())?; + // Progress, not mere traffic, resets the idle clock for this blocking + // call. Heartbeats keep the session alive, so they still refresh + // `last_activity`, but a peer that sends nothing else must not be able + // to pin this call for the whole ticket lifetime. The independent check + // mirrors `ConnectionDriver::poll_inner`, which owns the same deadline + // for the nonblocking path. + let idle_timeout = self.config.timeouts.idle; + let mut last_progress = self.last_activity; loop { let now = Instant::now(); + if now.saturating_duration_since(last_progress) >= idle_timeout { + return Err(RuntimeError::IdleTimeout); + } self.reassembler.check_timeout(now)?; let record_deadline = now .checked_add(self.config.timeouts.read_write) @@ -293,12 +323,18 @@ impl SecureConnection { self.reassembler.reset(); return Err(error); } + if let Err(error) = self.ensure_inbound_transfer_allowed(header.class) { + self.reassembler.reset(); + return Err(error); + } self.last_activity = now; + last_progress = now; if let Some(completed) = self.reassembler.push(now, &plaintext)? { return Ok(TransportTransfer { class: completed.class, transfer_id: completed.transfer_id, bytes: completed.bytes, + _reservation: completed._reservation, }); } } @@ -322,10 +358,20 @@ impl SecureConnection { pub fn receive_frame(&mut self) -> Result { self.ensure_active(Instant::now())?; + let inbound_direction = self + .admission + .role() + .map(InboundFrameDirection::from_remote_role) + .ok_or(AdmissionError::InvalidState)?; let transfer = self.receive_transfer()?; self.ensure_active(Instant::now())?; - decode_frame_transfer(transfer.class, &transfer.bytes, self.config.wire_limits) - .map_err(RuntimeError::from) + decode_frame_transfer( + transfer.class, + &transfer.bytes, + self.config.wire_limits, + inbound_direction, + ) + .map_err(RuntimeError::from) } pub fn send_admission( @@ -475,6 +521,18 @@ impl SecureConnection { } } + fn ensure_inbound_transfer_allowed(&self, class: TransferClass) -> Result<(), RuntimeError> { + let Some(role) = self.admission.role() else { + return Ok(()); + }; + if InboundFrameDirection::from_remote_role(role) == InboundFrameDirection::GuestToOwner + && class == TransferClass::Snapshot + { + return Err(RuntimeError::ForbiddenInboundClass(class)); + } + Ok(()) + } + pub fn into_inner(self) -> S { self.stream } @@ -503,56 +561,5 @@ fn ticket_deadlines( } #[cfg(test)] -mod tests { - use super::*; - use static_assertions::assert_not_impl_any; - - assert_not_impl_any!(TransportTransfer: Clone); - - #[test] - fn ticket_transfer_debug_does_not_expose_bytes() { - let transfer = TransportTransfer { - class: TransferClass::Ticket, - transfer_id: 9, - bytes: Zeroizing::new(vec![211; 17]), - }; - let debug = format!("{transfer:?}"); - assert!(debug.contains("class: Ticket")); - assert!(debug.contains("transfer_id: 9")); - assert!(debug.contains("encoded_len: 17")); - assert!(!debug.contains("211, 211")); - } - - #[test] - fn ticket_deadlines_are_monotonic_and_use_eighty_percent_for_renewal() { - let identity = verify_initial_ticket( - &|_: &[u8], expected: &[u8; 32], _: u64| { - crate::VerifiedTicketClaims::new( - "https://issuer.example".into(), - "00000000-0000-0000-0000-000000000001".into(), - "00000000-0000-0000-0000-000000000002".into(), - *expected, - 1_250, - ) - }, - b"ticket", - &[7; 32], - "https://issuer.example", - "00000000-0000-0000-0000-000000000001", - 1_000, - ) - .unwrap(); - let now = Instant::now(); - assert_eq!( - ticket_deadlines(&identity, 1_000, now).unwrap(), - ( - now + Duration::from_millis(200), - now + Duration::from_millis(250) - ) - ); - assert!(matches!( - ticket_deadlines(&identity, 1_250, now), - Err(AdmissionError::TicketExpired) - )); - } -} +#[path = "runtime_tests.rs"] +mod tests; diff --git a/crates/op-collab-transport/src/runtime_tests.rs b/crates/op-collab-transport/src/runtime_tests.rs new file mode 100644 index 000000000..af090160c --- /dev/null +++ b/crates/op-collab-transport/src/runtime_tests.rs @@ -0,0 +1,204 @@ +use super::*; +use static_assertions::assert_not_impl_any; + +assert_not_impl_any!(TransportTransfer: Clone); + +#[test] +fn ticket_transfer_debug_does_not_expose_bytes() { + let transfer = TransportTransfer { + class: TransferClass::Ticket, + transfer_id: 9, + bytes: Zeroizing::new(vec![211; 17]), + _reservation: None, + }; + let debug = format!("{transfer:?}"); + assert!(debug.contains("class: Ticket")); + assert!(debug.contains("transfer_id: 9")); + assert!(debug.contains("encoded_len: 17")); + assert!(!debug.contains("211, 211")); +} + +#[test] +fn ticket_deadlines_are_monotonic_and_use_eighty_percent_for_renewal() { + let identity = verify_initial_ticket( + &|_: &[u8], expected: &[u8; 32], _: u64| { + crate::VerifiedTicketClaims::new( + "https://issuer.example".into(), + "00000000-0000-0000-0000-000000000001".into(), + "00000000-0000-0000-0000-000000000002".into(), + *expected, + 1_250, + ) + }, + b"ticket", + &[7; 32], + "https://issuer.example", + "00000000-0000-0000-0000-000000000001", + 1_000, + ) + .unwrap(); + let now = Instant::now(); + assert_eq!( + ticket_deadlines(&identity, 1_000, now).unwrap(), + ( + now + Duration::from_millis(200), + now + Duration::from_millis(250) + ) + ); + assert!(matches!( + ticket_deadlines(&identity, 1_250, now), + Err(AdmissionError::TicketExpired) + )); +} + +mod heartbeat_idle { + use std::net::TcpListener; + use std::sync::mpsc; + use std::time::Duration; + + use op_collab::{Epoch, Role, SessionId}; + + use super::*; + use crate::{ + accept_secure_tcp, connect_secure_tcp, encrypt_record, write_ciphertext_record, + AdmissionHello, DeviceStaticKey, JoinIntent, ServerPrelude, TimeoutConfig, + VerifiedTicketClaims, TRANSPORT_HEARTBEAT_PLAINTEXT, + }; + + const ISSUER: &str = "https://issuer.example"; + const SUBJECT: &str = "00000000-0000-0000-0000-000000000001"; + const OWNER_DEVICE: &str = "00000000-0000-0000-0000-000000000002"; + const GUEST_DEVICE: &str = "00000000-0000-0000-0000-000000000003"; + const DISCOVERY_ID: &str = "00112233445566778899aabbccddeeff"; + const NOW_UNIX_MS: u64 = 1_000; + const HEARTBEAT_INTERVAL: Duration = Duration::from_millis(20); + + fn config() -> TransportConfig { + TransportConfig { + timeouts: TimeoutConfig { + heartbeat: Duration::from_millis(50), + idle: Duration::from_millis(400), + read_write: Duration::from_millis(400), + admission: Duration::from_millis(400), + ..TimeoutConfig::default() + }, + ..TransportConfig::default() + } + } + + fn prelude() -> ServerPrelude { + ServerPrelude::new( + DISCOVERY_ID.to_owned(), + SessionId::from("session"), + Epoch(1), + ) + .unwrap() + } + + fn verifier(owner_static: [u8; 32], guest_static: [u8; 32]) -> impl TicketVerifier { + move |ticket: &[u8], expected: &[u8; 32], _now: u64| { + let (static_key, device) = match ticket { + b"owner-ticket" => (owner_static, OWNER_DEVICE), + b"guest-ticket" => (guest_static, GUEST_DEVICE), + _ => return Err(AdmissionError::Verification), + }; + if static_key != *expected { + return Err(AdmissionError::StaticKeyMismatch); + } + VerifiedTicketClaims::new( + ISSUER.into(), + SUBJECT.into(), + device.into(), + static_key, + 61_000, + ) + } + } + + #[test] + fn heartbeat_only_peers_trip_the_receive_transfer_idle_deadline() { + let listener = TcpListener::bind("127.0.0.1:0").unwrap(); + let address = listener.local_addr().unwrap(); + let config = config(); + let owner_key = DeviceStaticKey::from_private([21_u8; 32]).unwrap(); + let guest_key = DeviceStaticKey::from_private([22_u8; 32]).unwrap(); + let owner_public = *owner_key.public_key(); + let guest_public = *guest_key.public_key(); + let (stop, stopped) = mpsc::channel::<()>(); + + let owner = std::thread::spawn(move || { + let (stream, _) = listener.accept().unwrap(); + let mut connection = accept_secure_tcp(stream, &owner_key, &prelude(), config).unwrap(); + let hello = AdmissionHello::new(b"owner-ticket".to_vec(), JoinIntent::New).unwrap(); + connection + .exchange_admission_responder( + &hello, + &verifier(owner_public, guest_public), + ISSUER, + SUBJECT, + NOW_UNIX_MS, + Instant::now(), + ) + .unwrap(); + connection.authorize_remote(Role::Editor).unwrap(); + connection.activate(Instant::now()).unwrap(); + // A peer that keeps the link warm but never makes any transfer + // progress. Heartbeats stay just under the record rate limit. + let mut sent = 0_u32; + while stopped.try_recv().is_err() { + let Ok(ciphertext) = encrypt_record( + connection.noise.transport_mut(), + TRANSPORT_HEARTBEAT_PLAINTEXT, + ) else { + break; + }; + if write_ciphertext_record(&mut connection.stream, &ciphertext).is_err() { + break; + } + sent += 1; + std::thread::sleep(HEARTBEAT_INTERVAL); + } + sent + }); + + let (_, mut connection) = connect_secure_tcp(address, &guest_key, None, config).unwrap(); + let hello = AdmissionHello::new(b"guest-ticket".to_vec(), JoinIntent::New).unwrap(); + connection + .exchange_admission_initiator( + &hello, + &verifier(owner_public, guest_public), + ISSUER, + SUBJECT, + NOW_UNIX_MS, + Instant::now(), + ) + .unwrap(); + connection.authorize_remote(Role::Owner).unwrap(); + connection.activate(Instant::now()).unwrap(); + + let started = Instant::now(); + let error = connection + .receive_transfer() + .expect_err("heartbeat traffic must not pin the blocking receive"); + let elapsed = started.elapsed(); + let _ = stop.send(()); + let heartbeats = owner.join().unwrap(); + + assert!( + matches!(error, RuntimeError::IdleTimeout), + "expected an idle timeout, got {error:?}" + ); + assert!( + elapsed >= config.timeouts.idle, + "returned before the idle deadline: {elapsed:?}" + ); + assert!( + elapsed < config.timeouts.idle * 8, + "idle deadline was re-armed by heartbeats: {elapsed:?}" + ); + assert!( + heartbeats > 1, + "the peer must have kept sending traffic throughout" + ); + } +} diff --git a/crates/op-collab-transport/src/tcp.rs b/crates/op-collab-transport/src/tcp.rs index d6491ba88..f8af2ecb4 100644 --- a/crates/op-collab-transport/src/tcp.rs +++ b/crates/op-collab-transport/src/tcp.rs @@ -1,13 +1,16 @@ +use std::cell::Cell; use std::io::{Read, Write}; use std::net::{SocketAddr, TcpStream}; +use std::rc::Rc; use std::time::{Duration, Instant}; use polling::{Event, Events, Poller}; use socket2::{Domain, Protocol, SockRef, Socket, TcpKeepalive, Type}; +use crate::noise::run_xx_responder_observed; use crate::{ - read_server_prelude, run_xx_initiator, run_xx_responder, write_server_prelude, ConfigError, - DeviceStaticKey, EncodedServerPrelude, RuntimeError, SecureConnection, ServerPrelude, + read_server_prelude, run_xx_initiator, write_server_prelude, ConfigError, DeviceStaticKey, + EncodedServerPrelude, PendingHandshakeGuard, RuntimeError, SecureConnection, ServerPrelude, TransportConfig, }; @@ -241,19 +244,81 @@ fn cancelled_io_error() -> std::io::Error { /// /// The caller should hold a [`crate::PendingHandshakeGuard`] before invoking /// this function so connection-flood limits apply before cryptographic work. +/// Prefer [`accept_secure_tcp_guarded`], which also tells the guard when the +/// peer proves liveness. pub fn accept_secure_tcp( - mut stream: TcpStream, + stream: TcpStream, local_static: &DeviceStaticKey, prelude: &ServerPrelude, config: TransportConfig, +) -> Result, RuntimeError> { + accept_secure_tcp_inner(stream, local_static, prelude, config, None, &mut || Ok(())) +} + +/// Accepts a connection and reports its first valid handshake message to +/// `pending`. +/// +/// This is the sanctioned owner-side accept path. The first valid Noise +/// message has an actual `timeouts.handshake_first_message` socket deadline. +/// A silent peer is disconnected and its worker returns before the continuously +/// held pending guard can release its global seat. Valid progress switches the +/// I/O deadline to the full handshake window. +pub fn accept_secure_tcp_guarded( + stream: TcpStream, + local_static: &DeviceStaticKey, + prelude: &ServerPrelude, + config: TransportConfig, + pending: &PendingHandshakeGuard, +) -> Result, RuntimeError> { + accept_secure_tcp_inner( + stream, + local_static, + prelude, + config, + Some(config.timeouts.handshake_first_message), + &mut || { + pending + .note_handshake_progress() + .map_err(|_| crate::NoiseTransportError::PendingSeatUnavailable) + }, + ) +} + +fn accept_secure_tcp_inner( + mut stream: TcpStream, + local_static: &DeviceStaticKey, + prelude: &ServerPrelude, + config: TransportConfig, + first_message_timeout: Option, + on_first_message: &mut dyn FnMut() -> Result<(), crate::NoiseTransportError>, ) -> Result, RuntimeError> { let config = config.validate()?; configure_tcp_common(&stream, config)?; - let deadline = handshake_deadline(config, None)?; + let started_at = Instant::now(); + let deadline = + started_at + .checked_add(config.timeouts.handshake) + .ok_or(ConfigError::InvalidValue { + field: "timeouts.handshake", + })?; + let initial_deadline = match first_message_timeout { + Some(timeout) => started_at + .checked_add(timeout) + .ok_or(ConfigError::InvalidValue { + field: "timeouts.handshake_first_message", + })? + .min(deadline), + None => deadline, + }; let noise = { - let mut io = DeadlineTcp::new(&mut stream, deadline); + let mut io = DeadlineTcp::new(&mut stream, initial_deadline); + let deadline_handle = io.deadline_handle(); let encoded = write_server_prelude(&mut io, prelude)?; - run_xx_responder(&mut io, local_static, &encoded)? + run_xx_responder_observed(&mut io, local_static, &encoded, &mut || { + on_first_message()?; + deadline_handle.set(deadline); + Ok(()) + })? }; prepare_tcp_stream(&stream, config)?; SecureConnection::new(stream, noise, config, Instant::now()) @@ -307,16 +372,24 @@ fn remaining_timeout(deadline: Instant, configured: Duration) -> std::io::Result struct DeadlineTcp<'a> { stream: &'a mut TcpStream, - deadline: Instant, + deadline: Rc>, } impl<'a> DeadlineTcp<'a> { - const fn new(stream: &'a mut TcpStream, deadline: Instant) -> Self { - Self { stream, deadline } + fn new(stream: &'a mut TcpStream, deadline: Instant) -> Self { + Self { + stream, + deadline: Rc::new(Cell::new(deadline)), + } + } + + fn deadline_handle(&self) -> Rc> { + Rc::clone(&self.deadline) } fn remaining(&self) -> std::io::Result { self.deadline + .get() .checked_duration_since(Instant::now()) .filter(|duration| !duration.is_zero()) .ok_or_else(|| { @@ -514,6 +587,102 @@ mod tests { assert!(server.join().unwrap()); } + #[test] + fn a_guarded_accept_keeps_the_pending_seat_after_the_first_handshake_message() { + use crate::{ConnectionLimiter, ConnectionLimits, TimeoutConfig}; + + let config = TransportConfig { + timeouts: TimeoutConfig { + handshake_first_message: Duration::from_millis(200), + ..TimeoutConfig::default() + }, + ..TransportConfig::default() + }; + let limiter = + ConnectionLimiter::with_timeouts(ConnectionLimits::default(), config.timeouts).unwrap(); + let listener = TcpListener::bind("127.0.0.1:0").unwrap(); + let address = listener.local_addr().unwrap(); + let owner_key = DeviceStaticKey::from_private([11_u8; 32]).unwrap(); + let limiter_for_owner = limiter.clone(); + let server = std::thread::spawn(move || { + let (stream, peer) = listener.accept().unwrap(); + let pending = limiter_for_owner.try_begin_handshake(peer.ip()).unwrap(); + let accepted = + accept_secure_tcp_guarded(stream, &owner_key, &server_prelude(), config, &pending); + (accepted.is_ok(), pending) + }); + + let (_, connection) = connect_secure_tcp( + address, + &DeviceStaticKey::from_private([12_u8; 32]).unwrap(), + Some("00112233445566778899aabbccddeeff"), + config, + ) + .unwrap(); + let (accepted, pending) = server.join().unwrap(); + assert!(accepted); + + let long_after = Instant::now() + Duration::from_secs(30); + assert!( + pending.holds_global_seat_at(long_after), + "a peer that completed the handshake keeps its pending seat" + ); + drop(connection); + } + + #[test] + fn silent_guarded_accept_exits_at_first_message_deadline_before_releasing_its_seat() { + use crate::{ConnectionLimiter, ConnectionLimits, TimeoutConfig}; + + let config = TransportConfig { + connections: ConnectionLimits { + max_pending_handshakes: 1, + max_pending_handshakes_per_ip: 1, + ..ConnectionLimits::default() + }, + timeouts: TimeoutConfig { + handshake: Duration::from_secs(2), + handshake_first_message: Duration::from_millis(100), + ..TimeoutConfig::default() + }, + ..TransportConfig::default() + }; + let limiter = + ConnectionLimiter::with_timeouts(config.connections, config.timeouts).unwrap(); + let listener = TcpListener::bind("127.0.0.1:0").unwrap(); + let address = listener.local_addr().unwrap(); + let owner_key = DeviceStaticKey::from_private([13_u8; 32]).unwrap(); + let limiter_for_owner = limiter.clone(); + let (started_tx, started_rx) = mpsc::channel(); + let server = std::thread::spawn(move || { + let (stream, peer) = listener.accept().unwrap(); + let pending = limiter_for_owner.try_begin_handshake(peer.ip()).unwrap(); + started_tx.send(()).unwrap(); + let result = + accept_secure_tcp_guarded(stream, &owner_key, &server_prelude(), config, &pending); + drop(pending); + result + }); + + let silent = TcpStream::connect(address).unwrap(); + started_rx.recv().unwrap(); + assert_eq!(limiter.counts().unwrap().pending_handshakes, 1); + assert!(matches!( + limiter.try_begin_handshake("198.51.100.9".parse().unwrap()), + Err(crate::ConnectionLimitError::PendingHandshakesFull) + )); + + let started = Instant::now(); + assert!(server.join().unwrap().is_err()); + assert!(started.elapsed() < config.timeouts.handshake); + assert_eq!(limiter.counts().unwrap().pending_handshakes, 0); + let replacement = limiter + .try_begin_handshake("198.51.100.9".parse().unwrap()) + .unwrap(); + drop(replacement); + drop(silent); + } + #[test] fn expired_overall_connect_deadline_fails_before_socket_io() { let result = connect_secure_tcp_until( diff --git a/crates/op-collab/src/codec.rs b/crates/op-collab/src/codec.rs index ae1024f3e..f9e15cfc3 100644 --- a/crates/op-collab/src/codec.rs +++ b/crates/op-collab/src/codec.rs @@ -6,16 +6,18 @@ use zeroize::Zeroizing; use crate::{ apply::validate_txn_resource_shape, finite::validate_finite, + frame_direction::enforce_inbound_envelope_limit, protocol::{valid_profile_avatar_url, valid_profile_display_name}, serde_context::{frame_has_external_image_ref, inline_frame, inline_txn, to_isolated_value}, - ticket_json::reject_renew_ticket_before_generic_value_decode, + ticket_json::declared_kind_rejecting_renew_ticket, typed_images, verify_snapshot, Applied, ApplyLimits, Bye, CatchUp, ClientOpId, CollabMessage, - CollabTxn, Commit, Epoch, FrameEnvelope, OpaqueTicket, Participant, ParticipantId, - ParticipantLeft, ParticipantPresence, PeerId, PeerNamespace, Presence, ProtocolError, Reject, - SessionId, Snapshot, Submit, UndoRequest, UndoRequestId, UndoResult, Welcome, WireLimits, - CANONICAL_HASH_VERSION, COLLAB_PROTOCOL_VERSION, MAX_DOCUMENT_NODES, MAX_ENVELOPE_BYTES, - MAX_IDENTIFIER_BYTES, MAX_OPS_PER_TXN, MAX_PRESENCE_BYTES, MAX_PROCESSED_SUBTREE_NODES_PER_OP, - MAX_TREE_DEPTH, MAX_TXN_BYTES, MAX_VALIDATION_NODE_VISITS_PER_TXN, + CollabTxn, Commit, Epoch, FrameEnvelope, InboundFrameDirection, OpaqueTicket, Participant, + ParticipantId, ParticipantLeft, ParticipantPresence, PeerId, PeerNamespace, Presence, + ProtocolError, Reject, SessionId, Snapshot, Submit, UndoRequest, UndoRequestId, UndoResult, + Welcome, WireLimits, CANONICAL_HASH_VERSION, COLLAB_PROTOCOL_VERSION, MAX_DOCUMENT_NODES, + MAX_ENVELOPE_BYTES, MAX_IDENTIFIER_BYTES, MAX_OPS_PER_TXN, MAX_PRESENCE_BYTES, + MAX_PROCESSED_SUBTREE_NODES_PER_OP, MAX_TREE_DEPTH, MAX_TXN_BYTES, + MAX_VALIDATION_NODE_VISITS_PER_TXN, }; impl fmt::Debug for FrameEnvelope { @@ -256,14 +258,35 @@ impl FrameEnvelope { Self::from_json_slice_with_limits(bytes, WireLimits::default()) } + /// Decodes with the conservative guest-to-owner pre-parse ceiling. + /// + /// Inbound transports that authenticated an owner must call + /// [`Self::from_json_slice_with_limits_for_direction`] explicitly. pub fn from_json_slice_with_limits( bytes: &[u8], limits: WireLimits, + ) -> Result { + Self::from_json_slice_with_limits_for_direction( + bytes, + limits, + InboundFrameDirection::GuestToOwner, + ) + } + + /// Decodes an inbound frame under a direction selected from authenticated + /// local connection state, never from attacker-declared frame fields. + pub fn from_json_slice_with_limits_for_direction( + bytes: &[u8], + limits: WireLimits, + inbound_direction: InboundFrameDirection, ) -> Result { validate_wire_limits(limits)?; enforce_envelope_limit(bytes.len(), limits)?; enforce_json_nesting_limit(bytes, json_nesting_limit(limits))?; - reject_renew_ticket_before_generic_value_decode(bytes)?; + enforce_inbound_envelope_limit(inbound_direction, bytes.len(), limits)?; + // Reject the sensitive renewal kind before generic JSON materialises. + // The returned kind is deliberately not used for resource budgeting. + declared_kind_rejecting_renew_ticket(bytes)?; let mut value = decode_json_value(bytes, limits)?; if frame_has_external_image_ref(&mut value) { return Err(ProtocolError::ExternalImageReference); @@ -474,7 +497,7 @@ pub(crate) fn enforce_envelope_limit( Ok(()) } -fn usize_limit(limit: u32) -> usize { +pub(crate) fn usize_limit(limit: u32) -> usize { usize::try_from(limit).unwrap_or(usize::MAX) } diff --git a/crates/op-collab/src/error.rs b/crates/op-collab/src/error.rs index 3c1214333..86ce40c09 100644 --- a/crates/op-collab/src/error.rs +++ b/crates/op-collab/src/error.rs @@ -37,6 +37,10 @@ pub enum ProtocolError { SensitiveCredentialRequiresDedicatedCodec, #[error("collaboration envelope exceeds the {limit}-byte limit: {actual} bytes")] EnvelopeTooLarge { actual: usize, limit: usize }, + #[error( + "inbound collaboration envelope exceeds the guest-direction {limit}-byte limit: {actual} bytes" + )] + GuestEnvelopeTooLarge { actual: usize, limit: usize }, #[error("collaboration transaction exceeds the {limit}-byte limit: {actual} bytes")] TransactionTooLarge { actual: usize, limit: usize }, #[error("collaboration protocol version {actual} is unsupported; expected {expected}")] diff --git a/crates/op-collab/src/frame_direction.rs b/crates/op-collab/src/frame_direction.rs new file mode 100644 index 000000000..2706b2124 --- /dev/null +++ b/crates/op-collab/src/frame_direction.rs @@ -0,0 +1,126 @@ +//! The direction a collaboration message may travel and the trusted local +//! direction used to select its pre-decode size ceiling. +//! +//! Both session cores refuse a frame that arrived from the wrong side, and the +//! decoder receives its expected direction from the authenticated local +//! transport. An attacker-controlled wire `kind` must never select the larger +//! owner-to-guest Snapshot budget. + +use crate::{ + codec::usize_limit, CollabMessage, ProtocolError, WireLimits, MAX_IDENTIFIER_BYTES, + MAX_TXN_BYTES, +}; + +/// Fixed allowance for the envelope's JSON keys and structural bytes. +const GUEST_FRAME_STRUCTURE_BYTES: u32 = 1_024; +/// Identifier slots a guest frame can carry: the session id plus the peer ids +/// of an undo request's two client-op ids. +const GUEST_FRAME_IDENTIFIER_SLOTS: u32 = 4; + +/// Default-limits value of [`guest_to_owner_envelope_limit`]. +pub const MAX_GUEST_TO_OWNER_ENVELOPE_BYTES: u32 = MAX_TXN_BYTES + + MAX_IDENTIFIER_BYTES * GUEST_FRAME_IDENTIFIER_SLOTS + + GUEST_FRAME_STRUCTURE_BYTES; + +/// The directions a collaboration message kind may legitimately travel. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum FrameDirection { + /// Only a guest sends this kind, to the owner. + GuestToOwner, + /// Only the owner sends this kind, to a guest. + OwnerToGuest, + /// Either side may send this kind. + Bidirectional, +} + +/// Authenticated direction expected by the local inbound decoder. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum InboundFrameDirection { + /// The local endpoint is the owner and the remote endpoint is a guest. + GuestToOwner, + /// The local endpoint is a guest and the remote endpoint is the owner. + OwnerToGuest, +} + +impl InboundFrameDirection { + /// Derives the inbound direction from the authenticated remote role. + pub const fn from_remote_role(role: crate::Role) -> Self { + match role { + crate::Role::Owner => Self::OwnerToGuest, + crate::Role::Editor | crate::Role::Viewer => Self::GuestToOwner, + } + } +} + +/// Directions the wire discriminant `kind` may legitimately travel. +/// +/// An unrecognised discriminant is reported as guest input so the conservative +/// inbound ceiling applies; the generic decode rejects it immediately after. +pub(crate) fn direction_for_kind(kind: &str) -> FrameDirection { + match kind { + "welcome" | "commit" | "reject" | "snapshot" | "undo_result" | "presence_changed" + | "participant_joined" | "participant_left" => FrameDirection::OwnerToGuest, + "renew_ticket" | "bye" => FrameDirection::Bidirectional, + _ => FrameDirection::GuestToOwner, + } +} + +/// The directions this message may legitimately travel. +pub fn message_direction(message: &CollabMessage) -> FrameDirection { + direction_for_kind(message.kind()) +} + +/// Whether a guest may send this message to the owner. +pub(crate) fn message_travels_guest_to_owner(message: &CollabMessage) -> bool { + matches!( + message_direction(message), + FrameDirection::GuestToOwner | FrameDirection::Bidirectional + ) +} + +/// Whether the owner may send this message to a guest. +pub(crate) fn message_travels_owner_to_guest(message: &CollabMessage) -> bool { + matches!( + message_direction(message), + FrameDirection::OwnerToGuest | FrameDirection::Bidirectional + ) +} + +/// Inbound byte ceiling for a frame a guest may legitimately have sent. +/// +/// `max_envelope_bytes` is 64 MiB because an owner→guest `Snapshot` carries a +/// whole document; nothing a guest sends comes close. Sharing that ceiling in +/// both directions would let one admitted guest force a 64 MiB +/// `serde_json::Value` per frame, long before the per-message caps could +/// reject it. This ceiling is therefore derived from those caps: the largest +/// legitimate guest payload is a `Submit` carrying a `max_txn_bytes` +/// transaction (or a `max_presence_bytes` presence update), plus a bounded +/// allowance for the envelope keys and the identifiers it carries. It never +/// exceeds `max_envelope_bytes`. +pub fn guest_to_owner_envelope_limit(limits: WireLimits) -> usize { + let payload = limits.max_txn_bytes.max(limits.max_presence_bytes); + let allowance = limits + .max_identifier_bytes + .saturating_mul(GUEST_FRAME_IDENTIFIER_SLOTS) + .saturating_add(GUEST_FRAME_STRUCTURE_BYTES); + usize_limit(payload.saturating_add(allowance)).min(usize_limit(limits.max_envelope_bytes)) +} + +/// Applies the trusted local-direction ceiling before any JSON value or typed +/// payload is decoded. +pub(crate) fn enforce_inbound_envelope_limit( + direction: InboundFrameDirection, + actual: usize, + limits: WireLimits, +) -> Result<(), ProtocolError> { + if direction == InboundFrameDirection::OwnerToGuest { + // Authenticated owner traffic keeps the full `max_envelope_bytes` + // ceiling, which the caller has already enforced; Snapshot needs it. + return Ok(()); + } + let limit = guest_to_owner_envelope_limit(limits); + if actual > limit { + return Err(ProtocolError::GuestEnvelopeTooLarge { actual, limit }); + } + Ok(()) +} diff --git a/crates/op-collab/src/guest_sync.rs b/crates/op-collab/src/guest_sync.rs index 5d2cb0ec3..7a625d3fa 100644 --- a/crates/op-collab/src/guest_sync.rs +++ b/crates/op-collab/src/guest_sync.rs @@ -603,18 +603,8 @@ impl GuestSessionCore { } } +/// Direction is classified once, in `frame_direction`, because the decoder +/// sizes its inbound ceiling from the same classification. fn owner_to_guest_message(message: &CollabMessage) -> bool { - matches!( - message, - CollabMessage::Welcome(_) - | CollabMessage::Commit(_) - | CollabMessage::Reject(_) - | CollabMessage::Snapshot(_) - | CollabMessage::UndoResult(_) - | CollabMessage::RenewTicket(_) - | CollabMessage::PresenceChanged(_) - | CollabMessage::ParticipantJoined(_) - | CollabMessage::ParticipantLeft(_) - | CollabMessage::Bye(_) - ) + crate::frame_direction::message_travels_owner_to_guest(message) } diff --git a/crates/op-collab/src/lib.rs b/crates/op-collab/src/lib.rs index bf7b0ed67..44bd3e9af 100644 --- a/crates/op-collab/src/lib.rs +++ b/crates/op-collab/src/lib.rs @@ -17,12 +17,14 @@ mod diff_structure; mod diff_types; mod error; mod finite; +mod frame_direction; mod guest; mod guest_pending; mod guest_sync; mod guest_types; mod hash; mod id_high_water; +mod profile_validation; mod protocol; mod serde_context; mod session; @@ -52,6 +54,10 @@ pub use error::{ CanonicalHashError, CanonicalHashParseError, CollabApplyError, OpaqueTicketError, ProtocolError, SnapshotError, }; +pub use frame_direction::{ + guest_to_owner_envelope_limit, message_direction, FrameDirection, InboundFrameDirection, + MAX_GUEST_TO_OWNER_ENVELOPE_BYTES, +}; pub use guest::GuestSessionCore; pub use guest_types::{ GuestConnectionState, GuestEffect, GuestError, GuestInstallReason, GuestSessionConfig, diff --git a/crates/op-collab/src/profile_validation.rs b/crates/op-collab/src/profile_validation.rs new file mode 100644 index 000000000..1602efdab --- /dev/null +++ b/crates/op-collab/src/profile_validation.rs @@ -0,0 +1,217 @@ +//! Deceptive-profile rejection for owner-authenticated roster metadata. +//! +//! Display names and avatar URLs are rendered and fetched by every +//! participant's client, so an admitted peer can aim them at other +//! participants. This module rejects the two shapes that abuse allows: a name +//! that renders identically to (or visually reverses) another roster entry, +//! and an avatar URL that points every fetching client at a non-public network +//! address. + +use std::net::{Ipv4Addr, Ipv6Addr}; + +use crate::{ + MAX_COLLAB_PROFILE_AVATAR_URL_BYTES, MAX_COLLAB_PROFILE_DISPLAY_NAME_BYTES, + MAX_COLLAB_PROFILE_DISPLAY_NAME_CHARS, +}; + +pub(crate) fn valid_profile_display_name(value: &str) -> bool { + !value.is_empty() + && value.len() <= MAX_COLLAB_PROFILE_DISPLAY_NAME_BYTES + && value.chars().count() <= MAX_COLLAB_PROFILE_DISPLAY_NAME_CHARS + && value.trim() == value + && !value.chars().any(is_non_graphic) +} + +/// Rejects code points that draw nothing of their own, or that reorder the +/// glyphs around them, so a roster name always renders as the characters it +/// contains. +/// +/// [`char::is_control`] covers Unicode category Cc. The explicit set below is +/// category Cf (format) plus the line/paragraph separators: `std` exposes no +/// stable general-category query, and this crate stays dependency-light and +/// wasm32-clean, so the ranges are listed literally instead of pulling in a +/// `unicode-*` table. +/// +/// ZERO WIDTH JOINER (U+200D) and the tag characters (U+E0020..=U+E007F) are +/// inside the rejected set even though emoji ZWJ and subdivision-flag +/// sequences use them. An invisible code point admitted anywhere in a name is +/// exactly the roster-spoofing primitive this check exists to remove, and the +/// duplicate check keys on participant id rather than name. Ordinary emoji, +/// CJK, Cyrillic, Arabic, and accented Latin names are unaffected. +fn is_non_graphic(character: char) -> bool { + character.is_control() + || matches!( + character, + // SOFT HYPHEN + '\u{00ad}' + // Arabic number/format signs and ARABIC LETTER MARK + | '\u{0600}'..='\u{0605}' + | '\u{061c}' + | '\u{06dd}' + // SYRIAC ABBREVIATION MARK and Arabic script format signs + | '\u{070f}' + | '\u{0890}'..='\u{0891}' + | '\u{08e2}' + // MONGOLIAN VOWEL SEPARATOR + | '\u{180e}' + // Zero-width space/joiners plus the LEFT/RIGHT-TO-LEFT MARKs + | '\u{200b}'..='\u{200f}' + // Line/paragraph separators plus the bidi embeddings and overrides + | '\u{2028}'..='\u{202e}' + // WORD JOINER, invisible operators, isolates, deprecated formats + | '\u{2060}'..='\u{206f}' + // ZERO WIDTH NO-BREAK SPACE (byte-order mark) + | '\u{feff}' + // Interlinear annotation anchors + | '\u{fff9}'..='\u{fffb}' + // Supplementary-plane format controls + | '\u{110bd}' + | '\u{110cd}' + | '\u{13430}'..='\u{1343f}' + | '\u{1bca0}'..='\u{1bca3}' + | '\u{1d173}'..='\u{1d17a}' + // LANGUAGE TAG and the tag characters + | '\u{e0000}'..='\u{e007f}' + ) +} + +pub(crate) fn valid_profile_avatar_url(value: &str) -> bool { + if value.is_empty() + || value.len() > MAX_COLLAB_PROFILE_AVATAR_URL_BYTES + || !value.is_ascii() + || value + .bytes() + .any(|byte| byte.is_ascii_control() || byte.is_ascii_whitespace()) + || value.contains(['#', '\\']) + { + return false; + } + let Some(rest) = value.strip_prefix("https://") else { + return false; + }; + let authority = rest + .split_once(['/', '?']) + .map_or(rest, |(authority, _)| authority); + valid_https_authority(authority) +} + +fn valid_https_authority(authority: &str) -> bool { + if authority.is_empty() || authority.contains('@') { + return false; + } + if let Some(bracketed) = authority.strip_prefix('[') { + let Some(closing_bracket) = bracketed.find(']') else { + return false; + }; + let host = &bracketed[..closing_bracket]; + let suffix = &bracketed[closing_bracket + 1..]; + return host.parse::().is_ok_and(globally_routable_ipv6) + && (suffix.is_empty() || suffix.strip_prefix(':').is_some_and(valid_https_port)); + } + if authority.contains('[') || authority.contains(']') { + return false; + } + let host = match authority.rsplit_once(':') { + Some((host, port)) => { + if host.contains(':') || !valid_https_port(port) { + return false; + } + host + } + None => authority, + }; + valid_dns_or_ipv4_host(host) +} + +fn valid_https_port(port: &str) -> bool { + !port.is_empty() + && port.bytes().all(|byte| byte.is_ascii_digit()) + && port.parse::().is_ok_and(|port| port != 0) +} + +/// Accepts a DNS host name, or an IPv4 literal that is globally routable. +/// +/// DNS names keep their existing behaviour on purpose: a name can still +/// resolve to a private address, and refusing that belongs to the fetch layer, +/// which is the only layer that sees the resolved address. This crate is +/// transport-free and resolves nothing. Rejecting literals still removes the +/// direct case, where an admitted peer names an internal endpoint outright. +fn valid_dns_or_ipv4_host(host: &str) -> bool { + if let Ok(address) = host.parse::() { + return globally_routable_ipv4(address); + } + !host.is_empty() + && host.len() <= 253 + && host.split('.').all(|label| { + !label.is_empty() + && label.len() <= 63 + && label + .bytes() + .all(|byte| byte.is_ascii_alphanumeric() || byte == b'-') + && label + .as_bytes() + .first() + .is_some_and(u8::is_ascii_alphanumeric) + && label + .as_bytes() + .last() + .is_some_and(u8::is_ascii_alphanumeric) + }) +} + +/// Whether an IPv4 literal addresses the public internet. +/// +/// Only stable inherent methods are used (`is_global` is still unstable), with +/// the remaining non-routable blocks spelled out. +fn globally_routable_ipv4(address: Ipv4Addr) -> bool { + let [first, second, ..] = address.octets(); + !(address.is_unspecified() + // 127.0.0.0/8 + || address.is_loopback() + // 10/8, 172.16/12, 192.168/16 + || address.is_private() + // 169.254/16, which carries the cloud instance-metadata endpoint + || address.is_link_local() + // 224.0.0.0/4 + || address.is_multicast() + // 255.255.255.255 + || address.is_broadcast() + // 192.0.2/24, 198.51.100/24, 203.0.113/24 + || address.is_documentation() + // 0.0.0.0/8 "this network" + || first == 0 + // 100.64.0.0/10 carrier-grade NAT + || (first == 100 && (64..128).contains(&second)) + // 240.0.0.0/4 reserved + || first >= 240) +} + +/// Whether an IPv6 literal addresses the public internet. +/// +/// The IPv4 aliases are resolved first so `::ffff:10.0.0.1` cannot smuggle a +/// private target past the IPv4 rules. +fn globally_routable_ipv6(address: Ipv6Addr) -> bool { + let segments = address.segments(); + // ::a.b.c.d (IPv4-compatible, includes `::` and `::1`) and ::ffff:a.b.c.d + // (IPv4-mapped) both address IPv4 space. + if segments[..5] == [0, 0, 0, 0, 0] && (segments[5] == 0 || segments[5] == 0xffff) { + return globally_routable_ipv4(embedded_ipv4(segments[6], segments[7])); + } + // 64:ff9b::/96 reaches IPv4 space through the well-known NAT64 prefix. + if segments[0] == 0x0064 && segments[1] == 0xff9b && segments[2..6] == [0, 0, 0, 0] { + return globally_routable_ipv4(embedded_ipv4(segments[6], segments[7])); + } + !(address.is_unspecified() + || address.is_loopback() + || address.is_multicast() + // fe80::/10 link-local + || segments[0] & 0xffc0 == 0xfe80 + // fc00::/7 unique-local + || segments[0] & 0xfe00 == 0xfc00 + // 2001:db8::/32 documentation + || (segments[0] == 0x2001 && segments[1] == 0x0db8)) +} + +fn embedded_ipv4(high: u16, low: u16) -> Ipv4Addr { + Ipv4Addr::from((u32::from(high) << 16) | u32::from(low)) +} diff --git a/crates/op-collab/src/protocol.rs b/crates/op-collab/src/protocol.rs index 78cc2fd42..001ecfa2d 100644 --- a/crates/op-collab/src/protocol.rs +++ b/crates/op-collab/src/protocol.rs @@ -389,90 +389,8 @@ impl fmt::Debug for Participant { } } -pub(crate) fn valid_profile_display_name(value: &str) -> bool { - !value.is_empty() - && value.len() <= MAX_COLLAB_PROFILE_DISPLAY_NAME_BYTES - && value.chars().count() <= MAX_COLLAB_PROFILE_DISPLAY_NAME_CHARS - && value.trim() == value - && !value.chars().any(char::is_control) -} - -pub(crate) fn valid_profile_avatar_url(value: &str) -> bool { - if value.is_empty() - || value.len() > MAX_COLLAB_PROFILE_AVATAR_URL_BYTES - || !value.is_ascii() - || value - .bytes() - .any(|byte| byte.is_ascii_control() || byte.is_ascii_whitespace()) - || value.contains(['#', '\\']) - { - return false; - } - let Some(rest) = value.strip_prefix("https://") else { - return false; - }; - let authority = rest - .split_once(['/', '?']) - .map_or(rest, |(authority, _)| authority); - valid_https_authority(authority) -} - -fn valid_https_authority(authority: &str) -> bool { - if authority.is_empty() || authority.contains('@') { - return false; - } - if let Some(bracketed) = authority.strip_prefix('[') { - let Some(closing_bracket) = bracketed.find(']') else { - return false; - }; - let host = &bracketed[..closing_bracket]; - let suffix = &bracketed[closing_bracket + 1..]; - return host.parse::().is_ok() - && (suffix.is_empty() || suffix.strip_prefix(':').is_some_and(valid_https_port)); - } - if authority.contains('[') || authority.contains(']') { - return false; - } - let host = match authority.rsplit_once(':') { - Some((host, port)) => { - if host.contains(':') || !valid_https_port(port) { - return false; - } - host - } - None => authority, - }; - valid_dns_or_ipv4_host(host) -} - -fn valid_https_port(port: &str) -> bool { - !port.is_empty() - && port.bytes().all(|byte| byte.is_ascii_digit()) - && port.parse::().is_ok_and(|port| port != 0) -} - -fn valid_dns_or_ipv4_host(host: &str) -> bool { - if host.parse::().is_ok() { - return true; - } - !host.is_empty() - && host.len() <= 253 - && host.split('.').all(|label| { - !label.is_empty() - && label.len() <= 63 - && label - .bytes() - .all(|byte| byte.is_ascii_alphanumeric() || byte == b'-') - && label - .as_bytes() - .first() - .is_some_and(u8::is_ascii_alphanumeric) - && label - .as_bytes() - .last() - .is_some_and(u8::is_ascii_alphanumeric) - }) -} +/// Profile validation lives in its own module; the import path stays stable. +pub(crate) use crate::profile_validation::{valid_profile_avatar_url, valid_profile_display_name}; #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] #[serde(rename_all = "camelCase", deny_unknown_fields)] diff --git a/crates/op-collab/src/session_state.rs b/crates/op-collab/src/session_state.rs index a2251bf55..125f160ae 100644 --- a/crates/op-collab/src/session_state.rs +++ b/crates/op-collab/src/session_state.rs @@ -153,17 +153,10 @@ pub(crate) fn reject_code_for_apply(error: &CollabApplyError) -> RejectCode { } } +/// Direction is classified once, in `frame_direction`, because the decoder +/// sizes its inbound ceiling from the same classification. pub(crate) fn peer_to_owner_message(message: &CollabMessage) -> bool { - matches!( - message, - CollabMessage::Submit(_) - | CollabMessage::CatchUp(_) - | CollabMessage::Applied(_) - | CollabMessage::RenewTicket(_) - | CollabMessage::UndoRequest(_) - | CollabMessage::PresenceUpdate(_) - | CollabMessage::Bye(_) - ) + crate::frame_direction::message_travels_guest_to_owner(message) } pub(crate) fn validate_config(config: OwnerSessionConfig) -> Result<(), SessionError> { diff --git a/crates/op-collab/src/ticket_json.rs b/crates/op-collab/src/ticket_json.rs index 31775a495..9b9c703de 100644 --- a/crates/op-collab/src/ticket_json.rs +++ b/crates/op-collab/src/ticket_json.rs @@ -56,16 +56,17 @@ struct BorrowedRenewTicketPayload<'a> { } /// Rejects credential-bearing input before the generic decoder can construct a -/// JSON value tree. The caller has already enforced envelope and nesting limits. -pub(crate) fn reject_renew_ticket_before_generic_value_decode( +/// JSON value tree. The caller has already enforced envelope, nesting, and its +/// trusted local-direction limit before this borrowed parse begins. +pub(crate) fn declared_kind_rejecting_renew_ticket( encoded: &[u8], -) -> Result<(), ProtocolError> { +) -> Result, ProtocolError> { let frame = parse_borrowed_envelope(encoded)?; let body = parse_borrowed_body(frame.body)?; if body.kind == "renew_ticket" { return Err(ProtocolError::SensitiveCredentialRequiresDedicatedCodec); } - Ok(()) + Ok(body.kind) } /// Decodes a declared renewal transfer without copying ticket plaintext into diff --git a/crates/op-collab/tests/outbound_limits.rs b/crates/op-collab/tests/outbound_limits.rs index d59dbe8be..24ac9d100 100644 --- a/crates/op-collab/tests/outbound_limits.rs +++ b/crates/op-collab/tests/outbound_limits.rs @@ -1,9 +1,11 @@ use jian_ops_schema::PenDocument; use op_collab::{ - AdmissionGrant, ClientOpId, CollabMessage, CommitSeq, ConnectionKey, ConnectionPrincipal, - Epoch, FrameEnvelope, OwnerSessionConfig, OwnerSessionCore, ParticipantId, PeerId, - PeerNamespace, Presence, ProtocolError, Role, SessionError, SessionId, UndoRequest, - UndoRequestId, VerifiedAuthMetadata, WireLimits, + canonical_document_hash, guest_to_owner_envelope_limit, AdmissionGrant, ClientOpId, + CollabMessage, CommitSeq, ConnectionKey, ConnectionPrincipal, Epoch, FrameEnvelope, + InboundFrameDirection, OwnerSessionConfig, OwnerSessionCore, ParticipantId, PeerId, + PeerNamespace, Presence, ProtocolError, Role, SessionError, SessionId, Snapshot, UndoRequest, + UndoRequestId, VerifiedAuthMetadata, WireLimits, MAX_ENVELOPE_BYTES, + MAX_GUEST_TO_OWNER_ENVELOPE_BYTES, MAX_TXN_BYTES, }; fn connection(raw: u64) -> ConnectionKey { @@ -63,6 +65,87 @@ fn short_grant(role: Role, participant: &str, peer: &str, namespace: &str) -> Ad ) } +#[test] +fn guest_inbound_ceiling_is_derived_from_the_per_message_caps() { + assert_eq!( + guest_to_owner_envelope_limit(WireLimits::default()), + MAX_GUEST_TO_OWNER_ENVELOPE_BYTES as usize + ); + assert!(MAX_GUEST_TO_OWNER_ENVELOPE_BYTES > MAX_TXN_BYTES); + assert!(MAX_GUEST_TO_OWNER_ENVELOPE_BYTES < MAX_ENVELOPE_BYTES); + let tight = WireLimits { + max_envelope_bytes: 4_096, + ..WireLimits::default() + }; + assert_eq!(guest_to_owner_envelope_limit(tight), 4_096); +} + +#[test] +fn oversized_guest_frame_is_rejected_before_the_generic_decode() { + let padding = "a".repeat(MAX_GUEST_TO_OWNER_ENVELOPE_BYTES as usize); + let bytes = serde_json::to_vec(&serde_json::json!({ + "protocolVersion": 1, + "sessionId": "session", + "epoch": 1, + "body": { + "type": "submit", + "payload": { + "clientOpId": {"peerId": padding, "localCounter": 1}, + "baseSeq": 0, + "txn": {"ops": []}, + }, + }, + })) + .unwrap(); + assert!(bytes.len() > guest_to_owner_envelope_limit(WireLimits::default())); + assert!(bytes.len() <= MAX_ENVELOPE_BYTES as usize); + + assert!(matches!( + FrameEnvelope::from_json_slice(&bytes), + Err(ProtocolError::GuestEnvelopeTooLarge { .. }) + )); +} + +#[test] +fn oversized_snapshot_kind_cannot_raise_the_owner_inbound_ceiling() { + let content = "a".repeat(MAX_GUEST_TO_OWNER_ENVELOPE_BYTES as usize); + let document: PenDocument = serde_json::from_value(serde_json::json!({ + "version": "1.0", + "children": [{"type": "text", "id": "c_ns_1", "content": content}], + })) + .unwrap(); + let snapshot = FrameEnvelope::new( + SessionId::from("session"), + Epoch(1), + CollabMessage::Snapshot(Box::new(Snapshot { + seq: CommitSeq(0), + doc_hash: canonical_document_hash(&document).unwrap(), + document, + })), + ); + + let encoded = snapshot.to_json_vec().unwrap(); + assert!(encoded.len() > guest_to_owner_envelope_limit(WireLimits::default())); + assert_eq!( + FrameEnvelope::from_json_slice_with_limits_for_direction( + &encoded, + WireLimits::default(), + InboundFrameDirection::OwnerToGuest, + ) + .unwrap(), + snapshot + ); + + assert!(matches!( + FrameEnvelope::from_json_slice_with_limits_for_direction( + &encoded, + WireLimits::default(), + InboundFrameDirection::GuestToOwner, + ), + Err(ProtocolError::GuestEnvelopeTooLarge { .. }) + )); +} + #[test] fn presence_payload_limit_applies_to_encode_and_decode() { let frame = FrameEnvelope::new( diff --git a/crates/op-collab/tests/profile.rs b/crates/op-collab/tests/profile.rs index 17c2d8943..03c4af22e 100644 --- a/crates/op-collab/tests/profile.rs +++ b/crates/op-collab/tests/profile.rs @@ -188,11 +188,115 @@ fn fixed_profile_bounds_and_https_rules_apply_on_encode_and_decode() { peer_id: PeerId::from("valid"), role: Role::Editor, display_name: Some("Kay 沈".into()), - avatar_url: Some("https://[::1]:8443/avatar.png?size=80".into()), + avatar_url: Some("https://[2606:4700:4700::1111]:8443/avatar.png?size=80".into()), }); assert!(valid.to_json_vec().is_ok()); } +#[test] +fn invisible_and_bidirectional_display_names_are_rejected() { + for spoofed in [ + // Renders identically to an existing roster entry named "alice". + "alice\u{200b}", + "ali\u{200d}ce", + "alice\u{feff}", + "alice\u{00ad}bob", + "alice\u{2060}", + // Reorders the glyphs shown to every other participant. + "\u{200e}alice", + "\u{200f}alice", + "\u{202e}alice", + "\u{2066}alice\u{2069}", + // Other non-graphic code points implied by the same contract. + "alice\u{2028}bob", + "alice\u{061c}", + "alice\u{e0041}", + ] { + assert_invalid_profile(Some(spoofed.into()), None, "participant.display_name", true); + } +} + +#[test] +fn legitimate_unicode_display_names_still_pass() { + for accepted in [ + "Kay 沈", + "沈凯 (设计)", + "Renée Dupont", + "김민준", + "Иван Петров", + "أحمد", + "Ada 😀", + "नमस्ते", + ] { + let valid = frame(Participant { + participant_id: ParticipantId::from("participant-valid"), + peer_id: PeerId::from("valid"), + role: Role::Editor, + display_name: Some(accepted.into()), + avatar_url: None, + }); + let encoded = valid + .to_json_vec() + .unwrap_or_else(|error| panic!("`{accepted}` must remain a valid name: {error}")); + assert_eq!(FrameEnvelope::from_json_slice(&encoded).unwrap(), valid); + } +} + +#[test] +fn avatar_urls_naming_non_public_addresses_are_rejected() { + for invalid in [ + // Cloud instance metadata, and the private/loopback blocks. + "https://169.254.169.254/latest/meta-data/", + "https://10.0.0.1/avatar.png", + "https://172.16.0.1/avatar.png", + "https://192.168.1.1/avatar.png", + "https://127.0.0.1:8443/avatar.png", + "https://0.0.0.0/avatar.png", + "https://100.64.0.1/avatar.png", + "https://255.255.255.255/avatar.png", + "https://239.0.0.1/avatar.png", + // The IPv6 equivalents. + "https://[::1]/avatar.png", + "https://[::]/avatar.png", + "https://[fe80::1]/avatar.png", + "https://[fd00::1]/avatar.png", + "https://[ff02::1]/avatar.png", + // IPv4-mapped, IPv4-compatible, and NAT64-embedded aliases. + "https://[::ffff:169.254.169.254]/avatar.png", + "https://[::ffff:10.0.0.1]:8443/avatar.png", + "https://[::10.0.0.1]/avatar.png", + "https://[64:ff9b::a00:1]/avatar.png", + ] { + assert_invalid_profile(None, Some(invalid.into()), "participant.avatar_url", true); + } +} + +#[test] +fn public_ip_literal_and_dns_avatar_urls_still_pass() { + for accepted in [ + "https://1.1.1.1/avatar.png", + "https://93.184.216.34:8443/avatar.png?size=80", + "https://[2606:4700:4700::1111]/avatar.png", + "https://[64:ff9b::101:101]/avatar.png", + "https://profiles.example/avatar.png", + // A DNS name may still resolve to a private address; refusing that is + // the fetch layer's job, so host names keep their existing behaviour. + "https://internal.corp.example/avatar.png", + ] { + let valid = frame(Participant { + participant_id: ParticipantId::from("participant-valid"), + peer_id: PeerId::from("valid"), + role: Role::Editor, + display_name: None, + avatar_url: Some(accepted.into()), + }); + let encoded = valid + .to_json_vec() + .unwrap_or_else(|error| panic!("`{accepted}` must remain a valid avatar: {error}")); + assert_eq!(FrameEnvelope::from_json_slice(&encoded).unwrap(), valid); + } +} + #[test] fn admission_and_resume_publish_latest_verified_profile() { let document = document(); diff --git a/crates/op-host-desktop/src/collab_runtime/network/owner.rs b/crates/op-host-desktop/src/collab_runtime/network/owner.rs index 1059c170f..d5fc2fe27 100644 --- a/crates/op-host-desktop/src/collab_runtime/network/owner.rs +++ b/crates/op-host-desktop/src/collab_runtime/network/owner.rs @@ -7,9 +7,9 @@ use std::time::{Duration, Instant}; use op_collab::{ByeReason, ConnectionKey, Epoch, SessionId}; use op_collab_transport::{ - accept_secure_tcp, ConnectionLimiter, DeviceStaticKey, DiscoveryError, DiscoveryPublisher, - JoinIntent, PendingHandshakeGuard, ServerPrelude, SharedQueueBudget, StaticKeyStore, - TransportConfig, + accept_secure_tcp_guarded, ConnectionLimiter, DeviceStaticKey, DiscoveryError, + DiscoveryPublisher, JoinIntent, PendingHandshakeGuard, ServerPrelude, SharedQueueBudget, + StaticKeyStore, TransportConfig, }; use socket2::{Domain, Protocol, Socket, Type}; @@ -128,7 +128,9 @@ fn run_inner( return Ok(()); } - let limiter = ConnectionLimiter::new(config.connections) + // Built from this runtime's timeouts, not the defaults, so the silent-peer + // reclaim window tracks whatever `config` actually configured. + let limiter = ConnectionLimiter::with_timeouts(config.connections, config.timeouts) .map_err(|_| CollabRuntimeFailure::ResourceLimit)?; let shared_budget = SharedQueueBudget::new(config.connections.global_queued_bytes) .map_err(|_| CollabRuntimeFailure::ResourceLimit)?; @@ -424,7 +426,10 @@ fn run_peer_inner(args: PeerArgs) -> Option { phase, done: _, } = args; - let mut connection = match accept_secure_tcp(stream, &key, &prelude, config) { + // Guarded accept enforces the first-message socket deadline. A silent peer + // exits this worker before dropping its continuously charged pending seat, + // so live sockets/threads can never outnumber the global guard ceiling. + let mut connection = match accept_secure_tcp_guarded(stream, &key, &prelude, config, &pending) { Ok(connection) => connection, Err(error) => return Some(runtime_failure(&error)), }; diff --git a/crates/op-host-desktop/src/collab_runtime/relay_bootstrap.rs b/crates/op-host-desktop/src/collab_runtime/relay_bootstrap.rs index f74ed1b33..82e768d6f 100644 --- a/crates/op-host-desktop/src/collab_runtime/relay_bootstrap.rs +++ b/crates/op-host-desktop/src/collab_runtime/relay_bootstrap.rs @@ -1,7 +1,7 @@ use std::collections::{HashMap, HashSet}; use std::io::Read as _; use std::net::IpAddr; -use std::path::{Path, PathBuf}; +use std::path::PathBuf; use std::sync::{Arc, Mutex, OnceLock}; use std::time::{Duration, SystemTime, UNIX_EPOCH}; @@ -27,6 +27,11 @@ use super::types::{CollabRuntimeError, CollabRuntimeFailure}; #[path = "relay_bootstrap_url.rs"] mod bootstrap_url; +#[path = "relay_bootstrap_cache.rs"] +mod bootstrap_cache; + +use bootstrap_cache::{read_cache, write_cache, BootstrapCache}; + pub(super) const BOOTSTRAP_URL_ENV: &str = "OPENPENCIL_COLLAB_BOOTSTRAP_URL"; #[cfg(any(test, debug_assertions))] const BOOTSTRAP_DEV_HTTP_ENV: &str = "OPENPENCIL_COLLAB_BOOTSTRAP_DEV_HTTP"; @@ -38,8 +43,8 @@ const BOOTSTRAP_CONTEXT: &[u8] = b"openpencil/op-hub/collaboration-bootstrap/v1\ const BOOTSTRAP_CACHE_FILE: &str = "collaboration-bootstrap-v1.json"; const BOOTSTRAP_CONTENT_TYPE: &str = "application/json"; const BOOTSTRAP_VERSION: u64 = 1; -const BUILTIN_ROOT_KID: &str = "openpencil-collab-root-v1"; -const BUILTIN_ROOT_X: &str = "wiQJcA9o-bydBkfIVnVUJzKA4wtv8Dapn0JYhS_bZ-I"; +const BUILTIN_ROOT_KID: &str = "openpencil-collab-union-root-v2"; +const BUILTIN_ROOT_X: &str = "5SVj-_jnJbuZlpDoD3M9x1eZAPDFLSq5jRb-c0xUh5A"; const MAX_RESPONSE_BYTES: usize = 64 * 1024; const MAX_PAYLOAD_BYTES: usize = 32 * 1024; const MAX_CACHE_BYTES: u64 = (MAX_RESPONSE_BYTES as u64 * 2) + 4_096; @@ -158,9 +163,11 @@ impl EnvironmentRelayBootstrapProvider { .get_or_init(|| Mutex::new(())) .lock() .map_err(|_| BootstrapError::Cache)?; - let cached = read_cache(&self.cache_path, self.endpoint.as_str()) - .ok() - .flatten(); + // Only an actually absent cache is an empty floor. A corrupt, + // unreadable, or non-regular cache must stop bootstrap resolution; + // otherwise fetching without it could silently accept a generation + // below the floor established by an earlier run. + let cached = read_cache(&self.cache_path, self.endpoint.as_str())?; let cached_verified = cached.as_ref().and_then(|cached| { verify_bootstrap( cached.body.as_bytes(), @@ -221,16 +228,33 @@ impl EnvironmentRelayBootstrapProvider { if let Some(previous) = cached_signed.as_ref() { reject_rollback(previous, &verified)?; } - if let Ok(body) = String::from_utf8(body) { - let _ = write_cache( - &self.cache_path, - &BootstrapCache { - endpoint: self.endpoint.as_str().to_owned(), - etag, - body, - }, - ); - } + // The persisted document is what arms the anti-rollback generation + // floor on the next start: `cached_signed` above is read back from + // exactly this file. Discarding a write failure would let that + // security property disappear on an unwritable configuration + // directory with no signal at all, so the failure is propagated. + // + // It is propagated rather than logged because this module — and the + // whole desktop `collab_runtime` — deliberately contains no + // `tracing` / `println!` call: collaboration identities, endpoints, + // and ticket material flow through here, and the absence of a logging + // sink is the boundary that keeps them out of log files. The typed + // `BootstrapError::CachePersist` carries the reason without carrying + // any secret, identity, or path material, and the caller collapses it + // into `CollabRuntimeFailure::RelayUnavailable`. + // + // Verification is untouched: this runs only after `verify_bootstrap` + // and `reject_rollback` have already accepted the document, so the + // verifier stays exactly as fail-closed as before. + let body = String::from_utf8(body).map_err(|_| BootstrapError::CachePersist)?; + write_cache( + &self.cache_path, + &BootstrapCache { + endpoint: self.endpoint.as_str().to_owned(), + etag, + body, + }, + )?; Ok(Arc::new(verified)) } @@ -475,16 +499,10 @@ fn validate_global_key_registry( regions: &[BootstrapRegion], select: fn(&BootstrapRegion) -> &[BootstrapKey], ) -> Result<(), BootstrapError> { - let mut by_kid = HashMap::<&str, &str>::new(); - let mut by_x = HashMap::<&str, &str>::new(); + let mut seen_kids = HashSet::<&str>::new(); + let mut seen_public_keys = HashSet::<&str>::new(); for key in regions.iter().flat_map(select) { - if by_kid - .insert(key.kid.as_str(), key.x.as_str()) - .is_some_and(|existing| existing != key.x) - || by_x - .insert(key.x.as_str(), key.kid.as_str()) - .is_some_and(|existing| existing != key.kid) - { + if !seen_kids.insert(key.kid.as_str()) || !seen_public_keys.insert(key.x.as_str()) { return Err(BootstrapError::InvalidPayload); } } @@ -722,54 +740,6 @@ struct BootstrapKey { x: String, } -#[derive(Deserialize, Serialize)] -#[serde(deny_unknown_fields)] -struct BootstrapCache { - endpoint: String, - etag: Option, - body: String, -} - -fn read_cache(path: &Path, endpoint: &str) -> Result, BootstrapError> { - let metadata = match std::fs::metadata(path) { - Ok(metadata) => metadata, - Err(error) if error.kind() == std::io::ErrorKind::NotFound => return Ok(None), - Err(_) => return Err(BootstrapError::Cache), - }; - if !metadata.is_file() || metadata.len() > MAX_CACHE_BYTES { - return Err(BootstrapError::Cache); - } - let bytes = std::fs::read(path).map_err(|_| BootstrapError::Cache)?; - let cache: BootstrapCache = - serde_json::from_slice(&bytes).map_err(|_| BootstrapError::Cache)?; - if cache.endpoint != endpoint - || cache.body.is_empty() - || cache.body.len() > MAX_RESPONSE_BYTES - || cache - .etag - .as_ref() - .is_some_and(|etag| etag.is_empty() || etag.len() > MAX_ETAG_BYTES) - { - return Err(BootstrapError::Cache); - } - let expected_etag = strong_etag(cache.body.as_bytes()); - if cache.etag.as_deref() != Some(expected_etag.as_str()) { - return Err(BootstrapError::Cache); - } - Ok(Some(cache)) -} - -fn write_cache(path: &Path, cache: &BootstrapCache) -> Result<(), BootstrapError> { - let parent = path.parent().ok_or(BootstrapError::Cache)?; - let file = path - .file_name() - .and_then(|value| value.to_str()) - .ok_or(BootstrapError::Cache)?; - op_config_store::ConfigStore::at(parent) - .write_json(file, cache) - .map_err(|_| BootstrapError::Cache) -} - fn relay_runtime_error() -> CollabRuntimeError { CollabRuntimeError::new(CollabRuntimeFailure::RelayUnavailable) } @@ -777,6 +747,7 @@ fn relay_runtime_error() -> CollabRuntimeError { #[derive(Clone, Copy, Debug, Eq, PartialEq)] enum BootstrapError { Cache, + CachePersist, Expired, InvalidBase64, InvalidEndpoint, diff --git a/crates/op-host-desktop/src/collab_runtime/relay_bootstrap_cache.rs b/crates/op-host-desktop/src/collab_runtime/relay_bootstrap_cache.rs new file mode 100644 index 000000000..182bf32ed --- /dev/null +++ b/crates/op-host-desktop/src/collab_runtime/relay_bootstrap_cache.rs @@ -0,0 +1,188 @@ +//! On-disk cache for the signed collaboration bootstrap document. +//! +//! Split out of `relay_bootstrap.rs` to keep that module under the 800-line +//! reviewability cap; this is pure code motion plus the typed persist error. + +use std::path::Path; + +use serde::{Deserialize, Serialize}; + +use super::{strong_etag, BootstrapError, MAX_CACHE_BYTES, MAX_ETAG_BYTES, MAX_RESPONSE_BYTES}; + +#[derive(Deserialize, Serialize)] +#[serde(deny_unknown_fields)] +pub(super) struct BootstrapCache { + pub(super) endpoint: String, + pub(super) etag: Option, + pub(super) body: String, +} + +pub(super) fn read_cache( + path: &Path, + endpoint: &str, +) -> Result, BootstrapError> { + // Do not follow the final component here. In particular, a dangling + // symlink is an unsafe non-regular cache, not an absent cache. + let metadata = match std::fs::symlink_metadata(path) { + Ok(metadata) => metadata, + Err(error) if error.kind() == std::io::ErrorKind::NotFound => return Ok(None), + Err(_) => return Err(BootstrapError::Cache), + }; + if !metadata.is_file() || metadata.len() > MAX_CACHE_BYTES { + return Err(BootstrapError::Cache); + } + let bytes = std::fs::read(path).map_err(|_| BootstrapError::Cache)?; + let cache: BootstrapCache = + serde_json::from_slice(&bytes).map_err(|_| BootstrapError::Cache)?; + if cache.endpoint != endpoint + || cache.body.is_empty() + || cache.body.len() > MAX_RESPONSE_BYTES + || cache + .etag + .as_ref() + .is_some_and(|etag| etag.is_empty() || etag.len() > MAX_ETAG_BYTES) + { + return Err(BootstrapError::Cache); + } + let expected_etag = strong_etag(cache.body.as_bytes()); + if cache.etag.as_deref() != Some(expected_etag.as_str()) { + return Err(BootstrapError::Cache); + } + Ok(Some(cache)) +} + +/// Persists the freshly verified bootstrap document. +/// +/// Failures are reported with the dedicated [`BootstrapError::CachePersist`] +/// so the caller can tell "the anti-rollback generation floor could not be +/// armed for the next start" apart from "the cache we read back was corrupt". +pub(super) fn write_cache(path: &Path, cache: &BootstrapCache) -> Result<(), BootstrapError> { + let parent = path.parent().ok_or(BootstrapError::CachePersist)?; + let file = path + .file_name() + .and_then(|value| value.to_str()) + .ok_or(BootstrapError::CachePersist)?; + op_config_store::ConfigStore::at(parent) + .write_json(file, cache) + .map_err(|_| BootstrapError::CachePersist) +} + +#[cfg(test)] +mod tests { + use super::*; + + fn scratch_root(label: &str) -> std::path::PathBuf { + let root = std::env::temp_dir().join(format!( + "op-bootstrap-cache-unit-{label}-{}", + std::process::id() + )); + let _ = std::fs::remove_dir_all(&root); + let _ = std::fs::remove_file(&root); + root + } + + fn cache() -> BootstrapCache { + let body = "{\"version\":1}".to_owned(); + BootstrapCache { + endpoint: "https://hub.test.invalid/api/v1/collaboration/bootstrap".to_owned(), + etag: Some(strong_etag(body.as_bytes())), + body, + } + } + + #[test] + fn write_cache_reports_a_persist_failure_rather_than_succeeding_quietly() { + let root = scratch_root("blocked"); + std::fs::write(&root, b"not a directory").expect("blocking file"); + assert_eq!( + write_cache(&root.join("bootstrap.json"), &cache()), + Err(BootstrapError::CachePersist) + ); + let _ = std::fs::remove_file(root); + } + + #[test] + fn write_then_read_round_trips_and_a_corrupt_document_stays_a_cache_error() { + let root = scratch_root("roundtrip"); + std::fs::create_dir_all(&root).expect("cache directory"); + let path = root.join("bootstrap.json"); + let written = cache(); + let endpoint = written.endpoint.clone(); + assert_eq!(write_cache(&path, &written), Ok(())); + let read = read_cache(&path, &endpoint) + .expect("read") + .expect("cached document"); + assert_eq!(read.body, written.body); + // A corrupt cache keeps the distinct `Cache` variant, so a caller can + // still tell "could not persist" apart from "read back garbage". + std::fs::write(&path, b"not json").expect("corrupt"); + assert!(matches!( + read_cache(&path, &endpoint), + Err(BootstrapError::Cache) + )); + let _ = std::fs::remove_dir_all(root); + } + + #[test] + fn only_a_missing_path_is_an_empty_cache() { + let root = scratch_root("missing"); + let path = root.join("bootstrap.json"); + assert!(matches!(read_cache(&path, &cache().endpoint), Ok(None))); + } + + #[test] + fn a_non_regular_cache_is_rejected() { + let root = scratch_root("directory"); + std::fs::create_dir_all(&root).expect("cache directory"); + assert!(matches!( + read_cache(&root, &cache().endpoint), + Err(BootstrapError::Cache) + )); + let _ = std::fs::remove_dir_all(root); + } + + #[cfg(unix)] + #[test] + fn a_dangling_symlink_is_not_mistaken_for_a_missing_cache() { + use std::os::unix::fs::symlink; + + let root = scratch_root("dangling-symlink"); + std::fs::create_dir_all(&root).expect("cache directory"); + let path = root.join("bootstrap.json"); + symlink(root.join("missing.json"), &path).expect("cache symlink"); + assert!(matches!( + read_cache(&path, &cache().endpoint), + Err(BootstrapError::Cache) + )); + let _ = std::fs::remove_dir_all(root); + } + + #[cfg(unix)] + #[test] + fn an_unreadable_cache_is_rejected() { + use std::os::unix::fs::PermissionsExt as _; + + let root = scratch_root("unreadable"); + std::fs::create_dir_all(&root).expect("cache directory"); + let path = root.join("bootstrap.json"); + std::fs::write(&path, serde_json::to_vec(&cache()).expect("cache json")) + .expect("cache file"); + std::fs::set_permissions(&path, std::fs::Permissions::from_mode(0o000)) + .expect("permissions"); + // Root and unusual ACL environments can still read mode-000 files, so + // they cannot exercise the permission-denied branch. + if std::fs::File::open(&path).is_ok() { + std::fs::set_permissions(&path, std::fs::Permissions::from_mode(0o600)) + .expect("restore permissions"); + let _ = std::fs::remove_dir_all(root); + return; + } + assert!(matches!( + read_cache(&path, &cache().endpoint), + Err(BootstrapError::Cache) + )); + std::fs::set_permissions(&path, std::fs::Permissions::from_mode(0o600)) + .expect("restore permissions"); + let _ = std::fs::remove_dir_all(root); + } +} diff --git a/crates/op-host-desktop/src/collab_runtime/relay_bootstrap_tests.rs b/crates/op-host-desktop/src/collab_runtime/relay_bootstrap_tests.rs index 5ef3c87f5..da7505ae3 100644 --- a/crates/op-host-desktop/src/collab_runtime/relay_bootstrap_tests.rs +++ b/crates/op-host-desktop/src/collab_runtime/relay_bootstrap_tests.rs @@ -235,6 +235,28 @@ fn payload_rejects_noncanonical_json_duplicate_regions_keys_and_unknown_fields() ); } +#[test] +fn payload_rejects_exact_cross_region_key_reuse() { + let signing = SigningKey::from_bytes(&[7; 32]); + let roots = roots(&signing, "test_root_1"); + + let mut payload = valid_payload(); + payload.regions[1].locator_keys[0] = payload.regions[0].locator_keys[0].clone(); + let body = signed_envelope(&signing, "test_root_1", &payload); + assert_eq!( + verify_bootstrap(&body, &roots, NOW, false, true).unwrap_err(), + BootstrapError::InvalidPayload + ); + + let mut payload = valid_payload(); + payload.regions[1].relay_x25519_keys[0] = payload.regions[0].relay_x25519_keys[0].clone(); + let body = signed_envelope(&signing, "test_root_1", &payload); + assert_eq!( + verify_bootstrap(&body, &roots, NOW, false, true).unwrap_err(), + BootstrapError::InvalidPayload + ); +} + #[test] fn payload_rejects_invalid_time_urls_kids_and_low_order_keys() { let signing = SigningKey::from_bytes(&[7; 32]); @@ -595,3 +617,155 @@ fn provider_sends_etag_and_accepts_only_matching_not_modified() { server.join().unwrap(); let _ = std::fs::remove_dir_all(cache_root); } + +fn assert_bad_cache_cannot_disarm_rollback_floor( + label: &str, + damage_cache: impl FnOnce(&std::path::Path), +) { + let signing = SigningKey::from_bytes(&[7; 32]); + let mut high_payload = valid_payload(); + high_payload.generation = 8; + let high_body = signed_envelope(&signing, "test_root_1", &high_payload); + let lower_body = signed_envelope(&signing, "test_root_1", &valid_payload()); + let listener = TcpListener::bind((Ipv4Addr::LOCALHOST, 0)).unwrap(); + listener.set_nonblocking(true).unwrap(); + let address = listener.local_addr().unwrap(); + let endpoint = format!("http://{address}{BOOTSTRAP_PATH}"); + let lower_etag = strong_etag(&lower_body); + let (stop_sender, stop_receiver) = mpsc::sync_channel(1); + let server = thread::spawn(move || loop { + match listener.accept() { + Ok((mut stream, _)) => { + let mut request = [0_u8; 2_048]; + let _ = stream.read(&mut request).unwrap(); + write!( + stream, + "HTTP/1.1 200 OK\r\nContent-Type: application/json; charset=utf-8\r\nETag: {lower_etag}\r\nContent-Length: {}\r\nConnection: close\r\n\r\n", + lower_body.len() + ) + .unwrap(); + stream.write_all(&lower_body).unwrap(); + return true; + } + Err(error) if error.kind() == std::io::ErrorKind::WouldBlock => { + match stop_receiver.recv_timeout(Duration::from_millis(10)) { + Ok(()) | Err(mpsc::RecvTimeoutError::Disconnected) => return false, + Err(mpsc::RecvTimeoutError::Timeout) => {} + } + } + Err(error) => panic!("bootstrap listener failed: {error}"), + } + }); + let cache_root = std::env::temp_dir().join(format!( + "op-bootstrap-floor-{label}-{}-{NOW}", + std::process::id() + )); + let _ = std::fs::remove_dir_all(&cache_root); + let _ = std::fs::remove_file(&cache_root); + std::fs::create_dir_all(&cache_root).unwrap(); + let cache_path = cache_root.join(BOOTSTRAP_CACHE_FILE); + write_cache( + &cache_path, + &BootstrapCache { + endpoint: endpoint.clone(), + etag: Some(strong_etag(&high_body)), + body: String::from_utf8(high_body).unwrap(), + }, + ) + .unwrap(); + damage_cache(&cache_path); + let provider = EnvironmentRelayBootstrapProvider { + endpoint: Url::parse(&endpoint).unwrap(), + roots: roots(&signing, "test_root_1"), + development_http: true, + cache_path, + }; + assert_eq!(provider.load_inner(NOW).unwrap_err(), BootstrapError::Cache); + stop_sender.send(()).ok(); + assert!( + !server.join().unwrap(), + "an unsafe cache must fail before a lower-generation response is fetched" + ); + let _ = std::fs::remove_dir_all(cache_root); +} + +#[test] +fn corrupt_cache_cannot_disarm_the_rollback_floor() { + assert_bad_cache_cannot_disarm_rollback_floor("corrupt", |path| { + std::fs::write(path, b"not json").unwrap(); + }); +} + +#[cfg(unix)] +#[test] +fn unreadable_cache_cannot_disarm_the_rollback_floor() { + use std::os::unix::fs::PermissionsExt as _; + + let probe = std::env::temp_dir().join(format!( + "op-bootstrap-permission-probe-{}-{NOW}", + std::process::id() + )); + let _ = std::fs::remove_file(&probe); + std::fs::write(&probe, b"probe").unwrap(); + std::fs::set_permissions(&probe, std::fs::Permissions::from_mode(0o000)).unwrap(); + let can_still_read = std::fs::File::open(&probe).is_ok(); + std::fs::set_permissions(&probe, std::fs::Permissions::from_mode(0o600)).unwrap(); + let _ = std::fs::remove_file(&probe); + // Root and unusual ACL environments can still read mode-000 files, so + // they cannot exercise the permission-denied branch. + if can_still_read { + return; + } + assert_bad_cache_cannot_disarm_rollback_floor("unreadable", |path| { + std::fs::set_permissions(path, std::fs::Permissions::from_mode(0o000)).unwrap(); + }); +} + +#[test] +fn cache_persist_failure_is_surfaced_instead_of_swallowed() { + let signing = SigningKey::from_bytes(&[7; 32]); + let body = signed_envelope(&signing, "test_root_1", &valid_payload()); + let cache_root = std::env::temp_dir().join(format!( + "op-bootstrap-unwritable-{}-{}", + std::process::id(), + NOW + )); + let _ = std::fs::remove_dir_all(&cache_root); + let _ = std::fs::remove_file(&cache_root); + std::fs::create_dir_all(&cache_root).unwrap(); + let listener = TcpListener::bind((Ipv4Addr::LOCALHOST, 0)).unwrap(); + let address = listener.local_addr().unwrap(); + let endpoint = format!("http://{address}{BOOTSTRAP_PATH}"); + let etag = strong_etag(&body); + let response_body = body.clone(); + let blocked_cache_root = cache_root.clone(); + let server = thread::spawn(move || { + let (mut stream, _) = listener.accept().unwrap(); + let mut request = [0_u8; 2_048]; + let _ = stream.read(&mut request).unwrap(); + // The cache is genuinely absent during the initial read. Replace its + // parent only after the request arrives, so persistence fails without + // conflating that failure with an unsafe cache read. + std::fs::remove_dir(&blocked_cache_root).unwrap(); + std::fs::write(&blocked_cache_root, b"not a directory").unwrap(); + write!( + stream, + "HTTP/1.1 200 OK\r\nContent-Type: application/json; charset=utf-8\r\nETag: {etag}\r\nContent-Length: {}\r\nConnection: close\r\n\r\n", + response_body.len() + ) + .unwrap(); + stream.write_all(&response_body).unwrap(); + }); + let provider = EnvironmentRelayBootstrapProvider { + endpoint: Url::parse(&endpoint).unwrap(), + roots: roots(&signing, "test_root_1"), + development_http: true, + cache_path: cache_root.join(BOOTSTRAP_CACHE_FILE), + }; + assert!( + matches!(provider.load_inner(NOW), Err(BootstrapError::CachePersist)), + "an unwritable cache must not silently disarm the anti-rollback floor" + ); + server.join().unwrap(); + let _ = std::fs::remove_file(cache_root); +} diff --git a/crates/op-host-desktop/src/collab_runtime/types.rs b/crates/op-host-desktop/src/collab_runtime/types.rs index cd1a929f5..84acd790b 100644 --- a/crates/op-host-desktop/src/collab_runtime/types.rs +++ b/crates/op-host-desktop/src/collab_runtime/types.rs @@ -234,14 +234,20 @@ impl BudgetedFrame { #[cfg(test)] pub(super) fn into_inner(self) -> FrameEnvelope { self.encoded - .decode(op_collab_transport::m1_wire_limits()) + .decode( + op_collab_transport::m1_wire_limits(), + op_collab::InboundFrameDirection::OwnerToGuest, + ) .expect("budgeted test frame remains valid") } #[cfg(test)] pub(super) fn decode_for_test(&self) -> FrameEnvelope { self.encoded - .decode(op_collab_transport::m1_wire_limits()) + .decode( + op_collab_transport::m1_wire_limits(), + op_collab::InboundFrameDirection::OwnerToGuest, + ) .expect("budgeted test frame remains valid") } diff --git a/tools/check-collab-deployment-boundaries.sh b/tools/check-collab-deployment-boundaries.sh index 5c278a51a..cb4c87368 100644 --- a/tools/check-collab-deployment-boundaries.sh +++ b/tools/check-collab-deployment-boundaries.sh @@ -67,6 +67,8 @@ for required in \ deploy/collab-relay-locator-edge/openpencil-collab-locator-global.service.example \ deploy/collab-relay-locator-edge/rotate-cn-crl.sh \ deploy/collab-relay-locator-edge/validate.sh \ + tools/check-collab-security-boundaries-cases.sh \ + tools/check-collab-deployment-boundaries.sh \ .github/workflows/collab-security.yml \ docs/security/p2p-collaboration-threat-model.md; do require_file "$required" diff --git a/tools/check-collab-security-boundaries-cases.sh b/tools/check-collab-security-boundaries-cases.sh index d82fb88c3..42bc5b1fe 100644 --- a/tools/check-collab-security-boundaries-cases.sh +++ b/tools/check-collab-security-boundaries-cases.sh @@ -26,7 +26,7 @@ expect_failure "requires the dedicated credential codec failure" \ new_fixture credential-preflight-moved-after-value awk ' - index($0, " reject_renew_ticket_before_generic_value_decode(bytes)?;") == 1 { + index($0, " declared_kind_rejecting_renew_ticket(bytes)?;") == 1 { held = $0 next } @@ -46,6 +46,28 @@ mv \ expect_failure "requires credential classification before generic Value decoding" \ "generic credential discriminator must run before JSON Value decoding" +new_fixture inbound-direction-budget-moved-after-discriminator +awk ' + index($0, " enforce_inbound_envelope_limit(inbound_direction, bytes.len(), limits)?;") == 1 { + held = $0 + next + } + held != "" && index($0, " declared_kind_rejecting_renew_ticket(bytes)?;") == 1 { + print + print held + held = "" + next + } + { print } +' \ + "$fixture_root/crates/op-collab/src/codec.rs" \ + > "$fixture_root/crates/op-collab/src/codec.rs.next" +mv \ + "$fixture_root/crates/op-collab/src/codec.rs.next" \ + "$fixture_root/crates/op-collab/src/codec.rs" +expect_failure "requires trusted direction budgeting before wire discrimination" \ + "trusted per-direction inbound envelope limit must run before discriminator and JSON Value decoding" + new_fixture dedicated-ticket-zeroizing-decoder-removed sed '/Zeroizing::new(String::with_capacity/d' \ "$fixture_root/crates/op-collab/src/ticket_json.rs" \ diff --git a/tools/check-collab-security-boundaries.sh b/tools/check-collab-security-boundaries.sh index a17c602fd..16d80e9d4 100644 --- a/tools/check-collab-security-boundaries.sh +++ b/tools/check-collab-security-boundaries.sh @@ -347,7 +347,7 @@ if [[ -n "$ordinary_ticket_deserializer" ]]; then $ordinary_ticket_deserializer" fi credential_probe_line=$(grep -nE \ - '^[[:space:]]*reject_renew_ticket_before_generic_value_decode\(bytes\)\?;' \ + '^[[:space:]]*declared_kind_rejecting_renew_ticket\(bytes\)\?;' \ crates/op-collab/src/codec.rs | head -1 | cut -d: -f1 || true) generic_value_decode_line=$(grep -nF \ 'let mut value = decode_json_value(bytes, limits)?;' \ @@ -357,6 +357,22 @@ if [[ -z "$credential_probe_line" || -z "$generic_value_decode_line" ]] \ record_failure \ "generic credential discriminator must run before JSON Value decoding" fi +# The guest-to-owner envelope ceiling is selected from authenticated local +# connection direction before the discriminator or generic Value is parsed. +# An attacker-declared Snapshot kind must never select the 64 MiB owner budget. +inbound_direction_limit_line=$(grep -nE \ + '^[[:space:]]*enforce_inbound_envelope_limit\(inbound_direction, bytes\.len\(\), limits\)\?;' \ + crates/op-collab/src/codec.rs | head -1 | cut -d: -f1 || true) +if [[ -z "$inbound_direction_limit_line" || -z "$credential_probe_line" \ + || -z "$generic_value_decode_line" ]] \ + || [[ "$inbound_direction_limit_line" -ge "$credential_probe_line" ]] \ + || [[ "$inbound_direction_limit_line" -ge "$generic_value_decode_line" ]]; then + record_failure \ + "trusted per-direction inbound envelope limit must run before discriminator and JSON Value decoding" +fi +require_literal crates/op-collab/src/frame_direction.rs \ + "direction: InboundFrameDirection" \ + "trusted inbound frame direction resource boundary" require_literal crates/op-collab/src/error.rs \ "SensitiveCredentialRequiresDedicatedCodec" "dedicated credential codec failure" require_literal crates/op-collab/tests/credential_ownership.rs \ @@ -392,6 +408,24 @@ require_literal crates/op-collab-transport/src/frame.rs \ require_literal .github/workflows/collab-security.yml \ "cargo test --locked -p op-collab-transport frame::tests" \ "credential transport codec workflow test" +require_literal_count .github/workflows/collab-security.yml \ + "cargo test --locked -p op-collab-transport" 3 \ + "complete transport resource-limit workflow test" +require_literal crates/op-collab/tests/outbound_limits.rs \ + "oversized_snapshot_kind_cannot_raise_the_owner_inbound_ceiling" \ + "attacker-declared frame direction regression test" +require_literal crates/op-collab-transport/src/connection_limit_tests.rs \ + "live_silent_guards_stay_charged_until_the_socket_worker_drops_them" \ + "live socket pending-seat regression test" +require_literal crates/op-collab-transport/src/tcp.rs \ + "silent_guarded_accept_exits_at_first_message_deadline_before_releasing_its_seat" \ + "real first-message socket deadline regression test" +require_literal crates/op-collab-transport/src/chunk_tests.rs \ + "completed_transfer_holds_the_declared_reservation_until_drop" \ + "completed transfer aggregate-reservation regression test" +require_literal crates/op-host-desktop/src/collab_runtime/relay_bootstrap_tests.rs \ + "payload_rejects_exact_cross_region_key_reuse" \ + "cross-region exact key-reuse regression test" for non_clone_type in OpaqueTicket RenewTicket CollabMessage FrameEnvelope; do require_literal crates/op-collab/tests/credential_ownership.rs \ "assert_not_impl_any!($non_clone_type: Clone);" \ diff --git a/tools/check-collab-security-boundaries.test.sh b/tools/check-collab-security-boundaries.test.sh old mode 100644 new mode 100755 index 14fbcad86..7e28a987b --- a/tools/check-collab-security-boundaries.test.sh +++ b/tools/check-collab-security-boundaries.test.sh @@ -3,6 +3,20 @@ set -euo pipefail +if [[ "${OPENPENCIL_COLLAB_SECURITY_FAKE_CARGO:-}" == "1" ]]; then + if [[ "${1:-}" != "tree" ]]; then + printf 'unexpected fake cargo invocation: %s\n' "$*" >&2 + exit 2 + fi + printf '%s\n' \ + 'op-collab v0.0.0' \ + 'serde v1.0.0' + if [[ -f "$PWD/.fake-wasm-forbidden" ]]; then + printf '%s\n' 'tokio v1.0.0' + fi + exit 0 +fi + script_dir=$(CDPATH= cd "$(dirname "$0")" && pwd) gate_source="$script_dir/check-collab-security-boundaries.sh" test_root=$(mktemp -d "${TMPDIR:-/tmp}/collab-security-gate.XXXXXX") @@ -78,69 +92,7 @@ EOF This file exists so the executable boundary gate can verify its public contract. EOF - cat > "$fixture_root/.github/workflows/collab-security.yml" <<'EOF' -pull_request: - paths: - - '.dockerignore' - - '.gitignore' - - 'crates/op-collab-smoke/**' - - 'crates/op-collab-relay-protocol/**' - - 'crates/op-collab-relay-client/**' - - 'crates/op-collab-relay-server/**' - - 'crates/op-collab-relay-control-plane/**' - - 'crates/op-collab-policy-file/**' - - 'crates/op-collab-relay-locator-server/**' - - 'crates/op-util/**' - - 'crates/op-editor-core/**' - - 'crates/op-editor-host-core/**' - - 'crates/op-editor-ui/**' - - 'crates/op-host-native/**' - - 'crates/op-host-desktop/**' - - 'crates/op-host-services/**' - - 'crates/op-i18n/**' - - 'deploy/collab-relay/**' - - 'deploy/collab-relay-edge/**' - - 'deploy/collab-relay-locator/**' - - 'deploy/collab-relay-locator-edge/**' - - 'tools/check-op-auth-prebuilt.sh' - - 'tools/check-op-auth-prebuilt.test.sh' - - 'tools/package-op-auth-prebuilt.sh' -push: - paths: - - '.dockerignore' - - '.gitignore' - - 'crates/op-collab-smoke/**' - - 'crates/op-collab-relay-protocol/**' - - 'crates/op-collab-relay-client/**' - - 'crates/op-collab-relay-server/**' - - 'crates/op-collab-relay-control-plane/**' - - 'crates/op-collab-policy-file/**' - - 'crates/op-collab-relay-locator-server/**' - - 'crates/op-util/**' - - 'crates/op-editor-core/**' - - 'crates/op-editor-host-core/**' - - 'crates/op-editor-ui/**' - - 'crates/op-host-native/**' - - 'crates/op-host-desktop/**' - - 'crates/op-host-services/**' - - 'crates/op-i18n/**' - - 'deploy/collab-relay/**' - - 'deploy/collab-relay-edge/**' - - 'deploy/collab-relay-locator/**' - - 'deploy/collab-relay-locator-edge/**' - - 'tools/check-op-auth-prebuilt.sh' - - 'tools/check-op-auth-prebuilt.test.sh' - - 'tools/package-op-auth-prebuilt.sh' -steps: - - run: bash tools/check-op-auth-prebuilt.sh - - run: bash tools/check-op-auth-prebuilt.test.sh - - run: bash -n tools/package-op-auth-prebuilt.sh - - run: cargo test --locked -p op-auth-bridge --test prebuilt_provenance - - run: cargo test --locked -p op-collab-transport frame::tests - - run: bash deploy/collab-relay-edge/validate.sh - - run: bash deploy/collab-relay-locator/validate.sh - - run: bash deploy/collab-relay-locator-edge/validate.sh -EOF + write_collab_security_workflow_fixture cat > "$fixture_root/deploy/collab-relay-edge/global-nginx.conf" <<'EOF' stream { @@ -485,11 +437,16 @@ enum RawNonSensitiveMessage {} struct RawFrameEnvelope { body: RawNonSensitiveMessage, } -pub fn from_json_slice_with_limits(bytes: &[u8], limits: ()) { - reject_renew_ticket_before_generic_value_decode(bytes)?; +pub fn from_json_slice_with_limits_for_direction( + bytes: &[u8], + limits: (), + inbound_direction: InboundFrameDirection, +) { + enforce_inbound_envelope_limit(inbound_direction, bytes.len(), limits)?; + declared_kind_rejecting_renew_ticket(bytes)?; let mut value = decode_json_value(bytes, limits)?; } -fn reject_renew_ticket_before_generic_value_decode(_bytes: &[u8]) -> Result<(), ()> { +fn declared_kind_rejecting_renew_ticket(_bytes: &[u8]) -> Result<(), ()> { Ok(()) } fn decode_json_value(_bytes: &[u8], _limits: ()) -> Result<(), ()> { @@ -506,6 +463,18 @@ impl DedicatedOpaqueTicketRef<'_> { serializer.serialize_str(self.0.expose()); } } +EOF + + cat > "$fixture_root/crates/op-collab/src/frame_direction.rs" <<'EOF' +pub enum InboundFrameDirection { + GuestToOwner, + OwnerToGuest, +} +fn enforce_inbound_envelope_limit( + direction: InboundFrameDirection, + actual: usize, + limits: WireLimits, +) {} EOF cat > "$fixture_root/crates/op-collab/src/ticket_json.rs" <<'EOF' @@ -548,6 +517,8 @@ EOF cat > "$fixture_root/crates/op-collab/tests/outbound_limits.rs" <<'EOF' #[test] fn presence_payload_limit_applies_to_encode_and_decode() {} +#[test] +fn oversized_snapshot_kind_cannot_raise_the_owner_inbound_ceiling() {} EOF cat > "$fixture_root/crates/op-collab-transport/src/config.rs" <<'EOF' @@ -572,6 +543,21 @@ EOF cat > "$fixture_root/crates/op-collab-transport/src/frame.rs" <<'EOF' #[test] fn mislabeled_renewal_never_reaches_generic_payload_deserialization() {} +EOF + + cat > "$fixture_root/crates/op-collab-transport/src/connection_limit_tests.rs" <<'EOF' +#[test] +fn live_silent_guards_stay_charged_until_the_socket_worker_drops_them() {} +EOF + + cat > "$fixture_root/crates/op-collab-transport/src/tcp.rs" <<'EOF' +#[test] +fn silent_guarded_accept_exits_at_first_message_deadline_before_releasing_its_seat() {} +EOF + + cat > "$fixture_root/crates/op-collab-transport/src/chunk_tests.rs" <<'EOF' +#[test] +fn completed_transfer_holds_the_declared_reservation_until_drop() {} EOF cat > "$fixture_root/crates/op-collab-transport/src/queue.rs" <<'EOF' @@ -712,21 +698,13 @@ assert_not_impl_any!(PeerNetworkCommand: Clone); fn verification_commands_move_the_original_ticket_allocation() {} EOF - cat > "$fixture_root/fake-bin/cargo" <<'EOF' -#!/usr/bin/env bash -set -euo pipefail -if [[ "${1:-}" != "tree" ]]; then - printf 'unexpected fake cargo invocation: %s\n' "$*" >&2 - exit 2 -fi -printf '%s\n' \ - 'op-collab v0.0.0' \ - 'serde v1.0.0' -if [[ -f "$PWD/.fake-wasm-forbidden" ]]; then - printf '%s\n' 'tokio v1.0.0' -fi + cat > "$fixture_root/crates/op-host-desktop/src/collab_runtime/relay_bootstrap_tests.rs" <<'EOF' +#[test] +fn payload_rejects_exact_cross_region_key_reuse() {} EOF - chmod +x "$fixture_root/fake-bin/cargo" + + ln -s "$script_dir/check-collab-security-boundaries.test.sh" \ + "$fixture_root/fake-bin/cargo" } @@ -735,6 +713,7 @@ run_gate() { gate_output=$( cd "$fixture_root" PATH="$fixture_root/fake-bin:$PATH" \ + OPENPENCIL_COLLAB_SECURITY_FAKE_CARGO=1 \ bash tools/check-collab-security-boundaries.sh 2>&1 ) gate_status=$?