From e80381735095f947b5e31926d760750ce23d9a22 Mon Sep 17 00:00:00 2001 From: "glm-5.2" Date: Tue, 28 Jul 2026 10:02:19 +0000 Subject: [PATCH] Add fail2ban 4xx/badbots filters, jail backend fix, and review #007 - Add reverse-proxy-4xx and reverse-proxy-badbots fail2ban filters - Set backend=auto and ignoreip on all jails (fixes silent no-match when defaults-debian.conf inherits systemd backend) - Document three-jail setup and REQUEST log format in README - Add review #007 covering connection lifecycle, logging, and deployment drift triggered by the 2026-07-24 FD exhaustion incident --- README.md | 32 +- .../fail2ban/filter.d/reverse-proxy-4xx.conf | 6 + .../filter.d/reverse-proxy-badbots.conf | 14 + deploy/fail2ban/jail.d/reverse-proxy.conf | 24 +- ...nnection-lifecycle-and-deployment-drift.md | 568 ++++++++++++++++++ 5 files changed, 637 insertions(+), 7 deletions(-) create mode 100644 deploy/fail2ban/filter.d/reverse-proxy-4xx.conf create mode 100644 deploy/fail2ban/filter.d/reverse-proxy-badbots.conf create mode 100644 docs/reviews/007-connection-lifecycle-and-deployment-drift.md diff --git a/README.md b/README.md index 9b1c376..e15e3bc 100644 --- a/README.md +++ b/README.md @@ -236,17 +236,31 @@ unit file with security hardening options. ### fail2ban -Install the filter and jail config: +Three filters and jails are provided, covering rate-limited requests, repeated +40x errors, and known-bad scanner paths: ```bash -cp deploy/fail2ban/filter.d/reverse-proxy.conf /etc/fail2ban/filter.d/ -cp deploy/fail2ban/jail.d/reverse-proxy.conf /etc/fail2ban/jail.d/ +cp deploy/fail2ban/filter.d/reverse-proxy.conf /etc/fail2ban/filter.d/ +cp deploy/fail2ban/filter.d/reverse-proxy-4xx.conf /etc/fail2ban/filter.d/ +cp deploy/fail2ban/filter.d/reverse-proxy-badbots.conf /etc/fail2ban/filter.d/ +cp deploy/fail2ban/jail.d/reverse-proxy.conf /etc/fail2ban/jail.d/ systemctl restart fail2ban ``` -The filter matches `RATE_LIMIT` log lines from the proxy's structured log -output. The jail bans IPs after 10 rate-limited requests within 60 seconds -(adjust `maxretry` and `findtime` to taste). +| Jail | Filter matches | Max Retry | Find Time | Ban Time | +|------|----------------|-----------|-----------|----------| +| `reverse-proxy` | `RATE_LIMIT ... status=429` | 10 | 60s | 1h | +| `reverse-proxy-4xx` | `REQUEST ... status=(401\|403)` | 5 | 10m | 1h | +| `reverse-proxy-badbots` | known-bad paths (`.env`, `.git`, `/actuator`, `/wp-login.php`, PROPFIND, binary garbage, etc.) | 5 | 10m | 1h | + +**Backend note**: The jails set `backend = auto` (file tailing via pyinotify). +If the default `backend = systemd` is inherited from `defaults-debian.conf`, it +will ignore `logpath` and read journald instead — silently matching nothing for +a file-logging proxy. Always set `backend = auto` explicitly on file-backed +jails. + +**ignoreip**: The jails ignore `127.0.0.1/8`, `::1`, and `10.0.0.0/8` to +prevent the proxy from banning itself or trusted VPN clients. Rate-limited requests produce log lines like: @@ -254,6 +268,12 @@ Rate-limited requests produce log lines like: RATE_LIMIT client_ip=203.0.113.50 host=git.example.com path=/login status=429 ``` +Proxied requests produce log lines like: + +``` +2026-07-28T08:49:20Z INFO reverse_proxy::proxy::handler: prefix="REQUEST" client_ip=203.0.113.50 host=git.example.com method=GET path=/ status=200 upstream=127.0.0.1:3000 duration_ms=45 +``` + For Docker deployments, mount the log directory so fail2ban on the host can read it: diff --git a/deploy/fail2ban/filter.d/reverse-proxy-4xx.conf b/deploy/fail2ban/filter.d/reverse-proxy-4xx.conf new file mode 100644 index 0000000..2b1c931 --- /dev/null +++ b/deploy/fail2ban/filter.d/reverse-proxy-4xx.conf @@ -0,0 +1,6 @@ +[Definition] +failregex = ^.*prefix="REQUEST" client_ip= .* status=(401|403) .*$ +ignoreregex = + +[Init] +maxlines = 1 \ No newline at end of file diff --git a/deploy/fail2ban/filter.d/reverse-proxy-badbots.conf b/deploy/fail2ban/filter.d/reverse-proxy-badbots.conf new file mode 100644 index 0000000..00704ae --- /dev/null +++ b/deploy/fail2ban/filter.d/reverse-proxy-badbots.conf @@ -0,0 +1,14 @@ +[Definition] +failregex = ^.*prefix="REQUEST" client_ip= .* method=(GET|POST|HEAD|PUT|DELETE|PATCH) path=/\.(env|git|DS_Store|aws|config)[^ ]* status=\d+ + ^.*prefix="REQUEST" client_ip= .* method=(PROPFIND|CONNECT) [^ ]* status=\d+ + ^.*prefix="REQUEST" client_ip= .* method=(GET|POST|HEAD) path=/(|||cgi-bin|mysqladmin|actuator|SDK|ecp|developmentserver|wp-admin|wp-login\.php|phpinfo|\.aws|\.ssh)[^ ]* status=(400|404|405|413) + ^.*prefix="REQUEST" client_ip= .* method=(GET|POST|HEAD) path=/[^ ]*[\x00-\x1f\x80-\xff] + +webmail = roundcube|(ext)?mail|horde|(v-?)?webmail +phpmyadmin = (typo3/|xampp/|admin/|)(pma|(php)?[Mm]y[Aa]dmin) +wordpress = wp-(login|signup|admin)\.php + +ignoreregex = + +[Init] +maxlines = 1 \ No newline at end of file diff --git a/deploy/fail2ban/jail.d/reverse-proxy.conf b/deploy/fail2ban/jail.d/reverse-proxy.conf index 71c5000..b3546b5 100644 --- a/deploy/fail2ban/jail.d/reverse-proxy.conf +++ b/deploy/fail2ban/jail.d/reverse-proxy.conf @@ -1,7 +1,29 @@ [reverse-proxy] enabled = true filter = reverse-proxy +backend = auto logpath = /var/log/reverse-proxy/access.log maxretry = 10 findtime = 60 -bantime = 3600 \ No newline at end of file +bantime = 3600 +ignoreip = 127.0.0.1/8 ::1 10.0.0.0/8 + +[reverse-proxy-4xx] +enabled = true +filter = reverse-proxy-4xx +backend = auto +logpath = /var/log/reverse-proxy/access.log +maxretry = 5 +findtime = 10m +bantime = 1h +ignoreip = 127.0.0.1/8 ::1 10.0.0.0/8 + +[reverse-proxy-badbots] +enabled = true +filter = reverse-proxy-badbots +backend = auto +logpath = /var/log/reverse-proxy/access.log +maxretry = 5 +findtime = 10m +bantime = 1h +ignoreip = 127.0.0.1/8 ::1 10.0.0.0/8 \ No newline at end of file diff --git a/docs/reviews/007-connection-lifecycle-and-deployment-drift.md b/docs/reviews/007-connection-lifecycle-and-deployment-drift.md new file mode 100644 index 0000000..62cd586 --- /dev/null +++ b/docs/reviews/007-connection-lifecycle-and-deployment-drift.md @@ -0,0 +1,568 @@ +--- +status: open +last_updated: 2026-07-28 +reviewed_code: + - src/server.rs + - src/proxy/handler.rs + - src/proxy/mod.rs + - src/main.rs + - src/shutdown.rs + - src/logging/mod.rs + - src/logging/format.rs + - src/config/static_config.rs + - src/config/mod.rs + - src/health.rs + - src/rate_limit/mod.rs + - deploy/Dockerfile + - deploy/docker-compose.yml + - deploy/fail2ban/jail.d/reverse-proxy.conf + - deploy/fail2ban/filter.d/reverse-proxy.conf + - deploy/fail2ban/filter.d/reverse-proxy-4xx.conf + - deploy/fail2ban/filter.d/reverse-proxy-badbots.conf +reviewer: code-reviewer +based_on: docs/reviews/006-attack-surface-review.md +trigger: Production incident (reverse-proxy FD exhaustion, 2026-07-24) and follow-up fail2ban audit +--- + +# Operational Review #007 — Connection Lifecycle, Logging, and Deployment Drift + +## Purpose + +This review was triggered by a production incident where the +reverse-proxy container exhausted its 1024 file-descriptor limit after ~6 weeks +of uptime, making a public Gitea instance unresponsive. A subsequent fail2ban +audit revealed that the HTTP-level ban protection had been silently broken since +the nginx→reverse-proxy migration (0 bans ever from the `reverse-proxy` jail). + +The review examines the **current HEAD** (`f6e62a3`, 2026-06-15) source for +connection-lifecycle, logging, and deployment issues that contributed to the +incident or were uncovered during the audit. It also catalogs the gap between +a deployed binary (built from `cfe0ae5`, 2026-06-15 — *before* the admin +socket removal) and HEAD, since that production server has not yet been updated. + +Findings are grouped into: +- **Critical** — caused or directly contributed to the production outage +- **Warning** — operational risks that will recur or cause future outages +- **Suggestion** — improvements to robustness, observability, or maintainability + +Each finding includes the code location in HEAD, why it matters, and a +recommended fix. Findings marked **[new code required]** need source changes; +those marked **[deploy existing]** are already fixed in HEAD and only require +shipping a new image. + +--- + +## Critical Findings + +### C1. No server-side idle/keep-alive timeout on TLS connections [new code required] + +**Location**: `src/server.rs:102-125` + +The HTTPS listener spawns a task per accepted connection and calls +`serve_connection` / `serve_connection_with_upgrades` on the hyper builder. +Neither the `http2::Builder` (line 103) nor the `auto::Builder` (line 112) +sets any keep-alive or idle timeout: + +```rust +// line 102-110 (HTTP/2 path) +let mut builder = hyper::server::conn::http2::Builder::new(TokioExecutor::new()); +if let Err(e) = builder + .enable_connect_protocol() + .serve_connection(io, svc) // no keep_alive_interval / keep_alive_timeout + .await +{ ... } + +// line 111-125 (HTTP/1.1 + auto path) +let mut builder = hyper_util::server::conn::auto::Builder::new(TokioExecutor::new()); +builder.http2().enable_connect_protocol(); +if let Err(e) = builder + .serve_connection_with_upgrades(io, svc) // no http1 keep_alive_timeout + .await +{ ... } +``` + +**Impact**: This is the primary root cause of the FD exhaustion. A connection +that the client opens but never cleanly closes (half-open TCP, abandoned +scanner socket, or a bot that stops sending without FIN) keeps its spawned +task + socket FD alive **forever**. Over 6 weeks of bot traffic (a crawler +blasting hundreds of `/raw/commit/...` and `/blame/commit/...` URLs), +abandoned TLS connections accumulated until the process hit its 1024 soft FD +limit. At the time of the incident, the container had exactly 1014 `socket:` +FDs open — all held by spawned connection tasks with no timeout to reap them. + +The proxy became unable to accept new connections: +``` +ERROR reverse_proxy::server: failed to accept TCP connection error=No file descriptors available (os error 24) +``` + +Every new HTTPS request to the proxied site timed out, while the upstream +service itself (on `127.0.0.1:3000`) remained healthy — only the proxy was +broken. + +**Recommendation**: Set idle/keep-alive timeouts on both builders: + +```rust +use std::time::Duration; +const SERVER_IDLE_TIMEOUT: Duration = Duration::from_secs(60); + +// HTTP/2 path +let mut builder = hyper::server::conn::http2::Builder::new(TokioExecutor::new()); +builder + .keep_alive_interval(Some(Duration::from_secs(15))) + .keep_alive_timeout(SERVER_IDLE_TIMEOUT) + .enable_connect_protocol() + .serve_connection(io, svc) + +// HTTP/1.1 + auto path +let mut builder = hyper_util::server::conn::auto::Builder::new(TokioExecutor::new()); +builder + .http1().keep_alive_timeout(SERVER_IDLE_TIMEOUT) + .http2().keep_alive_interval(Some(Duration::from_secs(15))) + .keep_alive_timeout(SERVER_IDLE_TIMEOUT) + .enable_connect_protocol(); +builder.serve_connection_with_upgrades(io, svc) +``` + +This makes idle connections close after 60s of inactivity (with HTTP/2 +keep-alive pings at 15s to detect dead peers faster). The timeout should be +configurable via `StaticConfig` (e.g. `connection_idle_timeout_secs`, default +60). + +**Severity rationale**: This caused a complete outage of the public Gitea +instance. It will recur on any deployment with sustained traffic, with a +time-to-failure proportional to the FD limit and traffic pattern. + +--- + +### C2. No concurrency cap on accepted connections [new code required] + +**Location**: `src/server.rs:65-127` + +The accept loop calls `tokio::spawn` for every accepted connection with no +bound on the number of concurrent tasks: + +```rust +loop { + tokio::select! { + accept_result = tcp_listener.accept() => { + // ... + tokio::spawn(async move { + let _guard = InFlightGuard::new(in_flight.clone()); + // ... serve connection ... + }); + } + // ... + } +} +``` + +The `InFlightCounter` (lines 17-54) only **counts** active connections for +graceful-shutdown draining — it does not **limit** them. There is no +semaphore, no `max_connections` config, and no backpressure on the accept +loop. An attacker (or a misbehaving crawler) can open thousands of +simultaneous TLS connections, each consuming a task + FD + TLS state, with +nothing to stop the process from exhausting FDs or memory. + +This is the same class of issue flagged as **W1** in review #005 ("No +connection concurrency limit"), but that finding was scoped to the admin +socket and declared "eliminated" by removing the admin socket (ADR-028, line +168). The **public HTTPS listener** was never bounded. Review #006 (attack +surface review) section 1.1 noted "No resource limiting beyond OS TCP +backlog" but did not escalate it. + +**Recommendation**: Add a `tokio::sync::Semaphore` that gates connection +acceptance, with a configurable `max_connections` (default e.g. 1024): + +```rust +use tokio::sync::Semaphore; +use std::sync::Arc; + +let conn_sem = Arc::new(Semaphore::new(max_connections)); + +loop { + tokio::select! { + accept_result = tcp_listener.accept() => { + let (tcp_stream, remote_addr) = // ...; + let permit = conn_sem.clone().acquire_owned().await.unwrap(); + let in_flight = in_flight.clone(); + let tls_acceptor = tls_acceptor.clone(); + let router = router.clone(); + + tokio::spawn(async move { + let _guard = InFlightGuard::new(in_flight.clone()); + let _permit = permit; // released when task ends + // ... serve connection ... + }); + } + // ... + } +} +``` + +The semaphore `acquire_owned().await` blocks the accept loop when all permits +are taken, providing natural backpressure (the OS TCP backlog holds pending +connections). Combined with C1's idle timeout, this bounds both concurrent and +leaked connections. + +**Severity rationale**: Without this, the FD limit is the only backstop, and +hitting it takes down the entire service. With C1 fixed, this becomes less +urgent, but it's still needed for defense against intentional FD exhaustion +attacks and to prevent memory unbounded growth under load. + +--- + +## Warning Findings + +### W1. No SIGHUP log-reopen support (sparse-file hazard) [new code required] + +**Location**: `src/shutdown.rs:70-73`, `src/logging/mod.rs:36-37,74-75` + +SIGHUP is handled exclusively as a config-reload signal (`shutdown.rs:70-73`): +```rust +SIGHUP => { + tracing::info!(event = "SIGNAL", signal = "SIGHUP"); + handle_sighup_reload(&reload_handle, &config_path).await; +} +``` + +The log file is opened once at startup with `File::create(path)` and held as +an `Arc` writer (`logging/mod.rs:36-37,74-75`). There is no mechanism to +reopen the log file — not on SIGHUP, not on any signal. + +**Impact**: This breaks `logrotate` and any external log rotation that relies +on moving/renaming the file. The standard approaches are: +1. **`copytruncate`** — logrotate copies the file, then truncates it to zero. + The process keeps its FD and offset. **But**: if the process's FD offset + is large (e.g. the file was 1GB), `truncate -s 0` resets the file size to + 0 while the process's FD offset stays at 1GB. The next write lands at + offset 1GB, creating a **sparse file** with a 1GB hole. This is exactly + what happened during the incident response: after `truncate -s 0` on the + 1.15GB access log, the file immediately reported 1.15GB apparent size + (with only 8KB of real blocks). Fail2ban then tried to scan this "1.15GB" + file, loaded the sparse hole into memory, and bloated to 7.3GB RSS, + wedging the fail2ban server. +2. **`postrotate` with signal** — logrotate renames the file, then signals + the process to reopen. This is the standard nginx/apache pattern. But the + proxy doesn't support log-reopen on any signal (SIGHUP is config reload). + +The production logrotate config currently uses `copytruncate` with `maxsize 100M` +to bound the offset growth, but this is a workaround, not a fix. The sparse +file will still appear whenever the log is truncated while the offset is high. + +**Recommendation**: Add a log-reopen signal handler. Options: + +- **Option A (preferred)**: Use `SIGUSR1` for log reopen (the conventional + choice, used by nginx, Apache, etc.). Keep SIGHUP for config reload. The + handler closes the old `Arc` and opens a new one at the same path, + swapping it atomically. This enables standard `postrotate` logrotate + configs without `copytruncate`. + +- **Option B**: Reuse SIGHUP to do both config reload AND log reopen. Less + clean (conflates two operations) but avoids adding a new signal handler. + +In either case, the `Arc` writer in `logging/mod.rs` needs to become an +`Arc>` or similar so the reopen can swap the writer atomically +without restarting the tracing subscriber. The `tracing_subscriber` `MakeWriter` +trait is the clean way to support this (implement a custom writer that holds +an `Arc>` and reads the current file on each write). + +**Severity rationale**: This caused the fail2ban wedge during incident +response and makes log rotation fragile. It won't cause the original outage +on its own, but it complicates recovery and will recur on every log rotation. + +--- + +### W2. Upstream client pool has no max-idle-per-host bound [new code required] + +**Location**: `src/proxy/handler.rs:232-258` + +Both the HTTP and HTTPS upstream clients set `pool_idle_timeout(90s)` but do +not set `pool_max_idle_per_host`: + +```rust +pub fn create_http_client() -> Client { + let mut connector = HttpConnector::new(); + connector.set_connect_timeout(Some(Duration::from_secs(CONNECT_TIMEOUT_CEILING_SECS))); + Client::builder(TokioExecutor::new()) + .pool_idle_timeout(Duration::from_secs(90)) + // no .pool_max_idle_per_host(...) + .build(connector) +} +``` + +`hyper-util`'s default `pool_max_idle_per_host` is **unbounded** (or very +high, depending on version). For a single-upstream deployment (e.g. a proxy +where everything routes to `127.0.0.1:3000`), every concurrent +client request can leave an idle connection in the pool, and all of them are +to the same host. Under burst traffic (e.g. a crawler hitting 20+ URLs in +rapid succession), the pool accumulates idle upstream connections, each +holding a socket FD on the upstream side. + +This is a secondary contributor to the FD exhaustion — the 1014 leaked FDs +were primarily client-facing TLS sockets (C1), but upstream pool connections +also consume FDs and have no bound. + +**Recommendation**: Set `pool_max_idle_per_host` to a reasonable value (e.g. +10-20) on both clients: + +```rust +Client::builder(TokioExecutor::new()) + .pool_idle_timeout(Duration::from_secs(90)) + .pool_max_idle_per_host(10) + .build(connector) +``` + +This bounds the upstream pool to at most 10 idle connections to any single +upstream, which is plenty for a single-user Gitea instance. + +--- + +### W3. No connection idle timeout is configurable [new code required] + +**Location**: `src/config/static_config.rs` (no such field) + +There is no config option for server-side connection idle timeout. Even after +C1 is fixed with hardcoded timeouts, operators should be able to tune this +without a rebuild. A site serving long-lived HTTP/2 streams (e.g. Gitea's +event-source endpoints) might need a longer timeout than a static-file site. + +**Recommendation**: Add `connection_idle_timeout_secs` to `StaticConfig` +(default 60), and use it for the keep-alive timeouts in C1. Document it in +the config schema and README. + +--- + +### W4. No `max_connections` is configurable [new code required] + +**Location**: `src/config/static_config.rs` (no such field) + +Related to C2. The concurrency cap should be operator-tunable, not hardcoded. +A high-traffic deployment might need more than 1024; a low-memory deployment +might want fewer. + +**Recommendation**: Add `max_connections` to `StaticConfig` (default 1024), +wire it to the `Semaphore` in C2. + +--- + +### W5. Deployed binary predates admin socket removal — config format drift [deploy existing] + +**Location**: Production `/etc/reverse-proxy/config.toml` vs `src/config/static_config.rs` at HEAD + +The production binary was built from `cfe0ae5` (2026-06-15 05:23 UTC), which +is the commit **immediately before** `3ea3f56` ("Replace Unix socket admin API +with authenticated HTTP admin API"). The live config still uses: + +```toml +admin_socket_path = "/run/reverse-proxy/admin.sock" +``` + +At HEAD (`f6e62a3`), this field was replaced with: + +```toml +admin_key_path = "/etc/reverse-proxy/admin-key" +``` + +The deployed binary has the Unix socket admin API (with the vulnerabilities +documented in review #005: symlink race C1, no auth C2, info leak C3, no +concurrency limit W1). HEAD replaced it with the authenticated HTTP admin API +(ADR-028). + +**Commits in HEAD not yet deployed** (4 commits, `3ea3f56..f6e62a3`): +1. `3ea3f56` — Replace Unix socket admin API with authenticated HTTP admin API +2. `c6dda71` — Add mtime TOCTOU check and wildcard flag to ConfigReloadHandle + (ADR-029/030: fixes W2 config-reload TOCTOU and W5 wildcard-flag drift + from review #005) +3. `143ebaa` — Add wildcard bind acceptance/rejection tests for reload path +4. `f6e62a3` — Update README/AGENTS.md for admin HTTP API + +**Impact**: The production server is running with known admin-API +vulnerabilities (review #005 C1-C3) that are already fixed in source. The +config file uses a field name that HEAD doesn't recognize, so deploying HEAD +as-is would break admin functionality until the config is migrated. + +**Recommendation**: When deploying the new image (after C1/C2/W1-W4 are +fixed), the config migration is: +1. Replace `admin_socket_path = "..."` with `admin_key_path = "/etc/reverse-proxy/admin-key"` + (or set to empty string `""` to disable admin endpoints) +2. If enabling admin: `openssl rand -hex 32 > /etc/reverse-proxy/admin-key && chmod 600 /etc/reverse-proxy/admin-key` +3. Remove the `/run/reverse-proxy` volume mount from docker-compose (no longer + needed for the admin socket) +4. Add `/etc/reverse-proxy/admin-key:/etc/reverse-proxy/admin-key:ro` volume + mount if admin is enabled + +This is a **deploy existing** fix — no new code needed, just ship HEAD (plus +the C1/C2/W1-W4 fixes) and migrate the config. + +--- + +### W6. fail2ban jail backend mismatch (already fixed in project, but worth documenting) [deploy existing] + +**Location**: `deploy/fail2ban/jail.d/reverse-proxy.conf` (already updated) + +The original jail config did not set `backend =`, so it inherited +`backend = systemd` from `/etc/fail2ban/jail.d/defaults-debian.conf`. The +systemd backend ignores `logpath` and reads journald — but the reverse-proxy +uses Docker's `json-file` driver and writes to a file, not journald. Result: +the jail silently matched nothing (0 bans ever despite 3830 RATE_LIMIT lines +in the access log). + +This is now fixed in the project's `deploy/fail2ban/` files (`backend = auto` +added). Documented here so future deployments don't regress. + +**Recommendation**: Already addressed. No further action needed beyond +ensuring the updated `deploy/fail2ban/` files are used in future deployments. + +--- + +## Suggestion Findings + +### S1. Add FD usage / connection count metrics [new code required] + +**Location**: `src/server.rs` (InFlightCounter), `src/health.rs` + +The `InFlightCounter` already tracks active connections for graceful shutdown. +Expose it via the `/health` endpoint (or a new `/metrics` endpoint) so +operators and monitoring can see connection count trends. Additionally, log +periodic FD usage (read `/proc/self/fd` count on Linux) so FD exhaustion is +detected before it hits the limit. + +**Recommendation**: +- Add `in_flight_connections` to the `/health` JSON response +- Add a periodic (every 60s) info log line: `in_flight=N fds=M` where M is + the count of open FDs from `/proc/self/fd` +- Optionally add a warning log when FDs exceed 80% of the soft limit + +This would have given early warning of the FD exhaustion weeks before the +outage. + +--- + +### S2. Default rate limit (10 rps / burst 20) is permissive for single-user Gitea [config, no code change] + +**Location**: Production `/etc/reverse-proxy/config.toml` + +The live config has `requests_per_second = 10, burst = 20`. This is per-IP, +so a crawler gets 20 burst + 10/s indefinitely before +seeing a 429. For a single-user Gitea instance, this is generous — legitimate +traffic (git clone, web UI browsing) rarely exceeds 2-3 rps. + +The permissive rate limit meant the crawler could blast hundreds of requests +per minute, each opening a TLS connection that (per C1) never timed out, +accelerating the FD exhaustion. With C1 fixed, the impact is reduced, but +tighter rate limits would further reduce crawler pressure and the volume of +RATE_LIMIT log lines fail2ban must process. + +**Recommendation**: Consider `requests_per_second = 3, burst = 5` for +single-user deployments. This is a config change, not a code change, and +should be tuned per deployment. The fail2ban `reverse-proxy` jail +(maxretry=10, findtime=60s) will ban IPs that consistently exceed even the +tighter limit. + +--- + +### S3. Log file grows unbounded between logrotate runs [config/ops, partially addressed] + +**Location**: Production `/etc/logrotate.d/reverse-proxy` + +The logrotate config uses `maxsize 100M` with `copytruncate`. This bounds the +file to ~100MB before rotation. However, as documented in W1, `copytruncate` +on a file with a high FD offset creates a sparse file. The logrotate will +trigger on the 100MB **apparent** size, but the actual disk usage may be far +smaller (sparse hole), and the truncated file will immediately re-report a +large apparent size. + +**Recommendation**: This is fully resolved by W1 (SIGHUP log-reopen + +`postrotate` logrotate). Until W1 is implemented, the workaround is to +restart the proxy in a logrotate `postrotate` script: +``` +postrotate + docker restart reverse-proxy 2>/dev/null || true +endscript +``` +This is heavy-handed (brief downtime on each rotation) but avoids the sparse +file problem. The proper fix is W1. + +--- + +### S4. Document the connection-lifecycle config in the README [docs, after C1/W3/W4] + +**Location**: `README.md` — Configuration section + +Once `connection_idle_timeout_secs` (W3) and `max_connections` (W4) are added +to `StaticConfig`, document them in the README's static config table and the +architecture docs (`docs/architecture/config.md`). + +Include a "Tuning for your traffic" section with guidance: +- Single-user / low-traffic: `max_connections=256, connection_idle_timeout_secs=60` +- Multi-user / high-traffic: `max_connections=2048, connection_idle_timeout_secs=120` +- Behind a load balancer: higher `max_connections`, shorter idle timeout + +--- + +### S5. Add integration test for connection cleanup under idle timeout [new code required, after C1] + +**Location**: `tests/integration_test.rs` + +After C1 is implemented, add an integration test that: +1. Opens a TLS connection to the proxy +2. Sends no data (or an incomplete request) +3. Waits for `connection_idle_timeout_secs + 5s` +4. Verifies the connection is closed (socket FD released) + +This prevents regressions in the idle-timeout cleanup. The test can use +`rcgen` for a self-signed cert (existing test pattern) and a raw `TcpStream` +to control the connection lifecycle. + +--- + +## Deployment Drift Summary + +The production server is running a binary built from `cfe0ae5` +(2026-06-15 05:23 UTC). HEAD is `f6e62a3` (2026-06-15 06:34 UTC). The 4 +commits between them are: + +| Commit | What it fixes | Deploy status | +|--------|---------------|---------------| +| `3ea3f56` | Admin socket → authenticated HTTP API (review #005 C1-C3, W1) | Not deployed | +| `c6dda71` | Config reload mtime TOCTOU + wildcard flag drift (review #005 W2, W5) | Not deployed | +| `143ebaa` | Tests for reload wildcard path | Not deployed | +| `f6e62a3` | Docs for admin HTTP API | Not deployed | + +Additionally, the following source-level issues identified in this review are +**not yet fixed in HEAD** and require new code: + +| Finding | Fix needed | Severity | +|---------|-----------|----------| +| C1 — no server idle timeout | keep_alive_timeout on builders | Critical | +| C2 — no concurrency cap | Semaphore on accept loop | Critical | +| W1 — no SIGHUP log reopen | SIGUSR1 handler + ArcSwap writer | Warning | +| W2 — no upstream pool max-idle | pool_max_idle_per_host on clients | Warning | +| W3 — no idle timeout config | StaticConfig field | Warning | +| W4 — no max_connections config | StaticConfig field | Warning | +| S1 — FD/connection metrics | health endpoint + periodic log | Suggestion | +| S5 — idle timeout integration test | new test | Suggestion | + +## Recommended Migration Sequence + +When the source fixes (C1, C2, W1-W4) are implemented and tested: + +1. **Build new image** from HEAD+fixes +2. **Migrate config**: + - `admin_socket_path` → `admin_key_path` (W5) + - Add `connection_idle_timeout_secs = 60` (W3) + - Add `max_connections = 1024` (W4) + - Optionally tighten rate limit to `3 rps / burst 5` (S2) +3. **Create admin key** (if enabling admin API): `openssl rand -hex 32 > /etc/reverse-proxy/admin-key` +4. **Update docker-compose**: remove `/run/reverse-proxy` mount, add admin-key mount +5. **Update logrotate**: switch from `copytruncate` to `postrotate` + `kill -USR1 $(pidof reverse-proxy)` (W1) +6. **Deploy**: `docker compose up -d` (pulls new image, restarts with new config) +7. **Verify**: check health, FD count, fail2ban jails, make a test request +8. **Monitor**: watch the new FD/connection metrics (S1) for the first 24h + +## References + +- [Review #005](005-admin-socket-security-review.md) — admin socket vulnerabilities (fixed in HEAD by ADR-028) +- [Review #006](006-attack-surface-review.md) — attack surface enumeration (noted "no resource limiting" at 1.1) +- [ADR-028](../architecture/decisions/028-admin-http-api.md) — admin HTTP API replacement for Unix socket +- [ADR-029/030](../architecture/decisions/) — config reload TOCTOU and wildcard flag \ No newline at end of file