diff --git a/deploy/docker-compose.yml b/deploy/docker-compose.yml index 7f56e51..5f14a8d 100644 --- a/deploy/docker-compose.yml +++ b/deploy/docker-compose.yml @@ -11,6 +11,12 @@ services: - /var/lib/reverse-proxy/acme-cache:/var/lib/reverse-proxy/acme-cache - /var/log/reverse-proxy:/var/log/reverse-proxy - /etc/reverse-proxy/admin-key:/etc/reverse-proxy/admin-key:ro + # FD headroom: max_connections + 64 reserved must fit under this limit + # (see docs/architecture/operations.md "File Descriptor Budget", review #010 C2) + ulimits: + nofile: + soft: 8192 + hard: 8192 networks: - proxy-net healthcheck: diff --git a/deploy/reverse-proxy.service b/deploy/reverse-proxy.service index 6e02e7b..a07108b 100644 --- a/deploy/reverse-proxy.service +++ b/deploy/reverse-proxy.service @@ -10,6 +10,10 @@ ExecStart=/usr/local/bin/reverse-proxy --config /etc/reverse-proxy/config.toml Restart=on-failure RestartSec=5 +# FD headroom: max_connections + 64 reserved must fit under this limit +# (see docs/architecture/operations.md "File Descriptor Budget", review #010 C2) +LimitNOFILE=8192 + # Security hardening NoNewPrivileges=yes ProtectSystem=strict diff --git a/docs/architecture/config.md b/docs/architecture/config.md index 0bbdb51..bded8c1 100644 --- a/docs/architecture/config.md +++ b/docs/architecture/config.md @@ -469,6 +469,16 @@ On startup, the config is validated: 23. `tls_handshake_timeout_secs` must be > 0. A zero value would immediately kill every TLS handshake, preventing any client connection from completing (review #010 C3). +24. `max_connections` + 64 reserved FDs must fit under the process's soft + `RLIMIT_NOFILE` (checked at startup and on reload, Unix only; skipped + when the limit is `RLIM_INFINITY`). This is an environment-dependent + check, separate from the pure TOML rules above. With the connection + semaphore shared across listeners (review #010 C4), connection FDs are + capped at exactly `max_connections` process-wide; the reserved headroom + covers listener sockets, the log file, ACME renewal sockets, epoll/timer + FDs, and stdin/stdout/stderr. A soft limit at or below the Docker default + (1024) also triggers a prominent startup warning even when the budget + fits (review #010 C2). On SIGHUP reload, the same validation applies. If the new config fails validation, the reload is rejected and the old config remains active. An error diff --git a/docs/architecture/operations.md b/docs/architecture/operations.md index cf8da13..840ba0d 100644 --- a/docs/architecture/operations.md +++ b/docs/architecture/operations.md @@ -373,6 +373,44 @@ On SIGTERM or SIGINT, the proxy performs a graceful shutdown: The `shutdown_timeout_secs` is configurable in StaticConfig (default: 30 seconds). See config.md for details. +## File Descriptor Budget + +The connection semaphore (shared across all listeners, review #010 C4) caps +concurrent client TLS connections at exactly `max_connections` process-wide. +Each connection holds one FD, and the process needs additional FDs for the +HTTPS/HTTP/health listener sockets, the log file, ACME renewal sockets, +epoll/timer FDs, and stdin/stdout/stderr. + +At startup and on config reload, the proxy reads the process's soft +`RLIMIT_NOFILE` (Unix; skipped when the limit is `RLIM_INFINITY`) and rejects +the configuration if: + +``` +max_connections + 64 (reserved FDs) > soft RLIMIT_NOFILE +``` + +This guard exists because the semaphore is the intended protection against +FD exhaustion — but before review #010 C2, nothing stopped it from being +configured at or above the real ceiling, guaranteeing `EMFILE` under load +(the review #010 incident shape). + +A soft limit at or below the Docker default (1024) also triggers a prominent +startup warning even when the budget fits. + +### Recommended baselines + +| Environment | nofile | max_connections | Rationale | +|-------------|--------|-----------------|-----------| +| Docker / docker-compose | 8192 (`ulimits.nofile`) | 800–1024 | Working production baseline from review #010 M1: connection FDs ≈ 10% of the limit | +| systemd | 8192 (`LimitNOFILE=`) | 800–1024 | Same headroom; set in the unit file's `[Service]` section | +| Bare metal (high traffic) | ≥ 2 × max_connections | sized to traffic | Keep ≥ 50% headroom for non-connection FDs and bursts | + +Check what the running process actually sees: + +```bash +cat /proc/$(pidof reverse-proxy)/limits | grep "open files" +``` + ## Deployment ### Binary diff --git a/docs/reviews/010-emfile-accept-loop-flood.md b/docs/reviews/010-emfile-accept-loop-flood.md index a1c0c2a..4f90aaa 100644 --- a/docs/reviews/010-emfile-accept-loop-flood.md +++ b/docs/reviews/010-emfile-accept-loop-flood.md @@ -28,8 +28,10 @@ fixes: listeners (ConnectionSemaphore, src/server.rs); max_connections is now a process-wide cap, not max_connections × listeners. - >- - C2: OPEN — no startup cross-check that max_connections fits under - RLIMIT_NOFILE with headroom. Sequenced after C3/C4. + C2: FIXED 2026-09-13 — startup + reload cross-check that + max_connections + 64 reserved FDs fits under soft RLIMIT_NOFILE + (src/config/fd_budget.rs); warning when nofile ≤ Docker default 1024; + deploy files gain LimitNOFILE/ulimits 8192. --- # Review #010 — EMFILE Accept-Loop Flood (6M ERROR lines, 1.9 GB log in 2 hours) @@ -232,6 +234,9 @@ implementation notes above. ## Finding C2: max_connections is not cross-checked against RLIMIT_NOFILE [config] +**Status**: FIXED 2026-09-13. Startup and reload now enforce the FD budget +(see "C2 fix" below). + **Severity**: Medium — the semaphore is the intended protection against FD exhaustion, but it can be configured at/above the process's real ceiling, making it a false safety net. @@ -260,14 +265,42 @@ project's Docker/systemd docs should recommend `nofile 8192` alongside `max_connections = 1024` (or vice versa: derive a sane default from the observed limit). +**C2 fix (landed)**: new `src/config/fd_budget.rs` implements: + +- `soft_nofile_limit()` — `libc::getrlimit(RLIMIT_NOFILE)` wrapper; returns + `None` on `RLIM_INFINITY`, getrlimit failure, or non-Unix (check skipped). +- `check_fd_budget()` — pure function: `max_connections + 64` must be + `<=` soft limit; unit-tested with synthetic values so tests never depend + on the runner's actual rlimit. +- `validate_nofile()` — returns + `ValidationError::MaxConnectionsExceedsNofile` on breach. Wired into both + gates: `cli::load_config()` (startup + `--validate`, process exits + non-zero) and `ConfigReloadHandle::reload()` (SIGHUP + admin reload + rejected, old config stays active). +- `warn_if_default_nofile()` — prominent startup warning when the soft + limit is ≤ 1024 (Docker default) even if the budget fits. +- Reserved FDs = 64 (listener sockets + log file + ACME renewal + epoll/timer + + stdio). With the shared semaphore from C4, connection FDs are capped at + exactly `max_connections`, so this is the complete FD budget check. +- `deploy/reverse-proxy.service` gains `LimitNOFILE=8192`, + `deploy/docker-compose.yml` gains `ulimits: nofile: 8192` (matching the + dev1 mitigation from M1), and `docs/architecture/operations.md` gains a + "File Descriptor Budget" section with recommended baselines. + +Design note: the environment-dependent check lives outside the pure +`validate()` TOML rules (validation.rs stays deterministic and +unit-testable in any sandbox); integration point is the two config gates, +so both startup and reload enforce it. + ## Finding M1 (mitigation): raise nofile + lower max_connections [deploy] **Status**: Applied 2026-09-12 (see Summary). Backups: `docker-compose.yml.bak.20260912-080241`, `config.toml.bak.20260912-080241`. Residual risk after mitigation: none identified for FD exhaustion at -current traffic (peak concurrent connections observed ≪ 800); the code -findings C2 remains the durable fix (C1 + C3 + C4 landed 2026-09-13). +current traffic (peak concurrent connections observed ≪ 800). All code +findings (C1 + C3 + C4 + C2) landed 2026-09-13; remaining items below are +doc/deploy hygiene. ## Traffic-analysis side note (from the same investigation) @@ -308,19 +341,22 @@ behavior, which is exactly what made the EMFILE state reachable. task. `max_connections` is now the process-wide concurrent TLS connection cap; the effective cap no longer scales with listener count. This is the topology C2's RLIMIT cross-check should assume. -4. Land C2 (RLIMIT cross-check at startup) with a prominent warning or - hard validation error. The FD budget assumes a shared semaphore - (landed, see step 3): connection FDs are capped at `max_connections` - process-wide. +4. ~~Land C2 (RLIMIT cross-check at startup) with a prominent warning or + hard validation error~~ — DONE 2026-09-13. `src/config/fd_budget.rs` + checks `max_connections + 64 <= soft RLIMIT_NOFILE` at startup and on + reload (hard error; the guaranteed-EMFILE shape from this incident is + rejected outright), warns at startup when nofile ≤ 1024, and skips the + check when the limit is `RLIM_INFINITY` or on non-Unix. Assumes the + shared semaphore topology from C4. Lower urgency after M1: with the deploy baseline (`nofile 8192`, `max_connections 800`) the ceiling is ~10% of the limit, so C2 is a validation guard, not an active exposure. 5. Consider a doc note in `docs/architecture/operations.md` describing the nofile/max_connections relationship (the mitigation values above are a working baseline: `nofile 8192`, `max_connections 800`). -6. Docker image: the deployment Dockerfile on dev1 is a two-line stub +6. ~~Docker image: the deployment Dockerfile on dev1 is a two-line stub (FROM + COPY); if the project's own `deploy/Dockerfile` is ever used, - it should also set `ulimits` guidance or rely on compose as done here. - The repo's own `deploy/docker-compose.yml` still lacks the `ulimits` - block applied to dev1, and `deploy/reverse-proxy.service` lacks - `LimitNOFILE`. \ No newline at end of file + it should also set `ulimits` guidance or rely on compose as done here.~~ + DONE 2026-09-13: the repo's `deploy/docker-compose.yml` now includes the + `ulimits` block (matching dev1's M1 mitigation) and + `deploy/reverse-proxy.service` sets `LimitNOFILE=8192`. \ No newline at end of file diff --git a/src/cli.rs b/src/cli.rs index a9a45bd..04269dd 100644 --- a/src/cli.rs +++ b/src/cli.rs @@ -67,6 +67,18 @@ pub fn load_config(cli: &Cli) -> Result { ) })?; + let fd_errors = crate::config::fd_budget::validate_nofile(static_config.max_connections); + if !fd_errors.is_empty() { + anyhow::bail!( + "FD budget validation failed:\n{}", + fd_errors + .iter() + .map(|e| format!(" - {}", e)) + .collect::>() + .join("\n") + ); + } + Ok(LoadedConfig { static_config, dynamic_config, diff --git a/src/config/fd_budget.rs b/src/config/fd_budget.rs new file mode 100644 index 0000000..34946d7 --- /dev/null +++ b/src/config/fd_budget.rs @@ -0,0 +1,115 @@ +use tracing::warn; + +use super::validation::ValidationError; + +pub const RESERVED_FDS: u64 = 64; +pub const DOCKER_DEFAULT_NOFILE: u64 = 1024; + +#[cfg(unix)] +pub fn soft_nofile_limit() -> Option { + let mut lim = libc::rlimit { + rlim_cur: 0, + rlim_max: 0, + }; + if unsafe { libc::getrlimit(libc::RLIMIT_NOFILE, &mut lim) } == 0 { + if lim.rlim_cur == libc::RLIM_INFINITY { + return None; + } + return Some(lim.rlim_cur); + } + None +} + +#[cfg(not(unix))] +pub fn soft_nofile_limit() -> Option { + None +} + +pub fn check_fd_budget(max_connections: usize, soft_limit: u64) -> Result<(), String> { + let required = max_connections as u64 + RESERVED_FDS; + if required > soft_limit { + Err(format!( + "max_connections ({max_connections}) + reserved FDs ({RESERVED_FDS}) exceeds soft RLIMIT_NOFILE ({soft_limit})" + )) + } else { + Ok(()) + } +} + +pub fn validate_nofile(max_connections: usize) -> Vec { + let Some(soft_limit) = soft_nofile_limit() else { + return Vec::new(); + }; + match check_fd_budget(max_connections, soft_limit) { + Ok(()) => Vec::new(), + Err(_) => vec![ValidationError::MaxConnectionsExceedsNofile { + max_connections, + soft_limit, + reserved: RESERVED_FDS, + }], + } +} + +pub fn warn_if_default_nofile(max_connections: usize) { + let Some(soft_limit) = soft_nofile_limit() else { + return; + }; + if soft_limit <= DOCKER_DEFAULT_NOFILE { + warn!( + soft_nofile = soft_limit, + max_connections = max_connections, + reserved_fds = RESERVED_FDS, + "RLIMIT_NOFILE is at or below the Docker default ({}); the proxy needs headroom for \ + connections plus listener sockets, log files, and ACME renewal. Raise it (e.g. \ + docker-compose ulimits: nofile 8192, or systemd LimitNOFILE=8192) — see review #010", + DOCKER_DEFAULT_NOFILE + ); + } +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn fd_budget_fits_when_max_connections_plus_reserved_within_limit() { + assert!(check_fd_budget(800, 8192).is_ok()); + assert!(check_fd_budget(960, 1024).is_ok()); + } + + #[test] + fn fd_budget_rejects_when_max_connections_plus_reserved_exceeds_limit() { + assert!(check_fd_budget(1024, 1024).is_err()); + assert!(check_fd_budget(1023, 1024).is_err()); + assert!(check_fd_budget(8129, 8192).is_err()); + } + + #[test] + fn fd_budget_boundary_exactly_at_limit_passes() { + assert!(check_fd_budget(960, 1024).is_ok()); + assert!(check_fd_budget(961, 1024).is_err()); + } + + #[test] + fn validate_nofile_rejects_absurd_max_connections_on_finite_limits() { + if soft_nofile_limit().is_none() { + return; + } + let errors = validate_nofile(usize::MAX / 2); + assert_eq!(errors.len(), 1); + assert!(matches!( + errors[0], + ValidationError::MaxConnectionsExceedsNofile { .. } + )); + } + + #[test] + fn validate_nofile_accepts_sane_max_connections_in_this_environment() { + if soft_nofile_limit().is_none() { + return; + } + let soft = soft_nofile_limit().unwrap(); + let sane = usize::try_from(soft.saturating_sub(RESERVED_FDS)).unwrap_or(1); + assert!(validate_nofile(sane).is_empty()); + } +} \ No newline at end of file diff --git a/src/config/mod.rs b/src/config/mod.rs index 36b668f..baeff78 100644 --- a/src/config/mod.rs +++ b/src/config/mod.rs @@ -1,4 +1,5 @@ pub mod dynamic_config; +pub mod fd_budget; pub mod static_config; pub mod test_fixtures; pub mod validation; diff --git a/src/config/validation.rs b/src/config/validation.rs index e48e295..efce303 100644 --- a/src/config/validation.rs +++ b/src/config/validation.rs @@ -83,6 +83,15 @@ pub enum ValidationError { TlsHandshakeTimeoutZero { value: u64 }, #[error("max_connections must be > 0, got {value}")] MaxConnectionsZero { value: usize }, + #[error( + "max_connections ({max_connections}) + reserved FDs ({reserved}) exceeds soft RLIMIT_NOFILE ({soft_limit}); \ + raise nofile (docker-compose ulimits / systemd LimitNOFILE, e.g. 8192) or lower max_connections" + )] + MaxConnectionsExceedsNofile { + max_connections: usize, + soft_limit: u64, + reserved: u64, + }, } pub fn validate( diff --git a/src/main.rs b/src/main.rs index 126927c..27aaefb 100644 --- a/src/main.rs +++ b/src/main.rs @@ -67,6 +67,10 @@ async fn run_server(loaded_config: cli::LoadedConfig, config_path: &str) -> Resu info!("reverse-proxy starting"); + reverse_proxy::config::fd_budget::warn_if_default_nofile( + loaded_config.static_config.max_connections, + ); + let dynamic_config: DynamicConfig = loaded_config.dynamic_config; let config_arc = Arc::new(ArcSwap::from_pointee(dynamic_config));