alkdev/reverse-proxy
2
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity
Files
0d54eba41e8ef94974f735ed5001cb972fcbc2f5
reverse-proxy/docs/architecture/open-questions.md
glm-5.1 0d54eba41e Update architecture specs to reflect live deployment findings and fix two bugs 
Architecture updates based on gaps discovered during live deployment testing:

- ADR-023: HTTP/2 client-facing support via ALPN-based protocol detection.
  The spec previously said HTTP/2 was out of scope, but the deployment
  revealed that modern browsers negotiate HTTP/2 via ALPN. The proxy now
  correctly detects the negotiated ALPN protocol and uses the appropriate
  HTTP server builder (http2::Builder for h2, auto::Builder for http/1.1).
  Upstream connections remain HTTP/1.1. Host resolution now falls back to
  URI host for HTTP/2 :authority pseudo-headers.

- ADR-024: ANSI-disabled logging. All tracing-subscriber layers now use
  with_ansi(false) to prevent ANSI escape codes in log output, which broke
  fail2ban regex matching in Docker deployments. Also documents the fail2ban
  regex anchor fix (^RATE_LIMIT → RATE_LIMIT).

Bug fixes found by architecture review:

- Fix missing ALPN protocols in manual TLS mode. build_manual_server_config
  and build_multi_domain_server_config did not set alpn_protocols, meaning
  manual TLS mode could not support HTTP/2. Added h2 and http/1.1 ALPN
  entries to both functions (acme-tls/1 only in ACME mode).

- Fix missing with_ansi(false) in JSON log format. The init_json function
  with file output did not disable ANSI on stdout or file layers, which would
  break fail2ban in production JSON logging mode.

Other spec updates:

- All document statuses updated from draft to reviewed
- proxy.md: documented Server header removal, upstream HTTPS client,
  two-phase timeout enforcement, HTTP/2 host resolution, connect timeout
- tls.md: documented ALPN configuration differing by mode (ACME vs manual)
- overview.md: added HTTP/2 client-facing support to scope, updated crate
  deps (hyper-rustls, rustls-native-certs, hyper-util), clarified out-of-scope
- config.md: fixed http_port type (u16→u32) to match implementation, added
  ANSI-disabled note for LoggingConfig
- operations.md: documented ANSI-disabled logging, fail2ban regex anchor
- open-questions.md: updated OQ-09 resolution (connect timeout fully
  implemented), OQ-10 (C2 bug is fixed)
2026-06-12 11:28:31 +00:00

171 lines
7.5 KiB
Markdown
Raw Blame History

---
status: reviewed
last_updated: 2026-06-12
---
# Open Questions
## TLS
### ~~OQ-01: Should cipher suites be restricted beyond rustls defaults?~~
- **Origin**: [tls.md](tls.md)
- **Status**: resolved
- **Priority**: medium
- **Resolution**: Restrict cipher suites to match the nginx scope: four
ECDHE-AES-GCM suites for TLS 1.2 plus all TLS 1.3 suites. This provides
behavioral parity during migration. See ADR-012.
- **Cross-references**: ADR-005, ADR-012
### ~~OQ-02: What log format should fail2ban consume?~~
- **Origin**: [operations.md](operations.md), [proxy.md](proxy.md)
- **Status**: resolved
- **Priority**: high
- **Resolution**: Custom structured log format with `key=value` pairs and
`RATE_LIMIT` prefix. A corresponding custom fail2ban filter will be provided.
See ADR-007.
- **Cross-references**: ADR-007
### ~~OQ-07: Should per-site TLS overrides be supported for mixed ACME/manual domains?~~
- **Origin**: [tls.md](tls.md), [config.md](config.md)
- **Status**: resolved
- **Priority**: low
- **Resolution**: Resolved by introducing `[[listeners]]` configuration. Each
listener is an independent TLS endpoint with its own bind address, TLS config,
and site routing. This supports both deployment models: (1) shared-IP
multi-domain (one listener, SAN certificate, SNI routing) and (2) dedicated-IP
single-domain (multiple listeners, each with its own IP/cert/domain). Mixed
ACME/manual configurations are naturally supported since each listener has its
own TLS mode. See ADR-019.
- **Cross-references**: ADR-011, ADR-019
## Logging and Monitoring
### ~~OQ-03: Should the health check endpoint be on a separate port?~~
- **Origin**: [operations.md](operations.md)
- **Status**: resolved
- **Priority**: low
- **Resolution**: Add a configurable local health check port (default: 9900)
bound to `127.0.0.1` only. Health checks work even when TLS is misconfigured.
There is no `/health` route on the main HTTPS listener — health checking is
handled exclusively by the local port and admin socket. See ADR-013 and
ADR-022.
- **Cross-references**: ADR-013, ADR-022
## Configuration
### ~~OQ-04: Should config reload support a Unix domain socket API in addition to SIGHUP?~~
- **Origin**: [config.md](config.md)
- **Status**: resolved
- **Priority**: low
- **Resolution**: Yes. Add a Unix domain socket admin API alongside SIGHUP.
The socket accepts a `reload` command and returns structured success/failure
responses. SIGHUP is retained as a fallback. See ADR-014.
- **Cross-references**: ADR-014
## Deployment
### ~~OQ-05: Should the proxy bind to multiple addresses or just one?~~
- **Origin**: [overview.md](overview.md)
- **Status**: resolved
- **Priority**: low
- **Resolution**: A single `bind_addr` per listener entry is sufficient. ADR-019
introduced `[[listeners]]`, where each listener has its own `bind_addr`. This
supports multiple bind addresses in a single process — one per listener —
without needing an array of addresses on a single listener. See ADR-016 and
ADR-019.
- **Cross-references**: ADR-016, ADR-019
## Proxy
### ~~OQ-06: Should upstream timeouts be configurable per-site?~~
- **Origin**: [proxy.md](proxy.md)
- **Status**: resolved
- **Priority**: low
- **Resolution**: Resolved by ADR-015. Per-site upstream timeout overrides with
sensible defaults (5s connect, 60s request). Optional fields in SiteConfig
that override global defaults when specified.
- **Cross-references**: ADR-015, ADR-017
### ~~OQ-08: Should the `/health` path use a less common endpoint to avoid upstream collision?~~
- **Origin**: Implementation review finding W5, [proxy.md](proxy.md)
- **Status**: resolved
- **Priority**: medium
- **Resolution**: The `/health` route does not belong on the main listener at
all. Health checking is an operational concern served by the dedicated local
port (9900) and the admin socket's `status` command — not by intercepting
traffic on the public-facing proxy. Serving `/health` on the main listener
creates collision with upstream applications, requires special-case routing
logic before host-based matching, and is architecturally wrong: the main
listener's job is to proxy requests, not to serve operational endpoints. The
local health check port (bound to `127.0.0.1:9900`) and the admin socket are
the sole health/status mechanisms. See ADR-022.
- **Cross-references**: ADR-013, ADR-022
### ~~OQ-09: How should `upstream_connect_timeout_secs` be enforced?~~
- **Origin**: Implementation review finding W4, ADR-015, ADR-017
- **Status**: resolved
- **Priority**: medium
- **Resolution**: Implemented using a two-phase `tokio::time::timeout` approach.
The inner timeout uses the per-site `upstream_connect_timeout_secs` (default
5s) for the connect + first-byte phase, and the outer timeout uses
`upstream_request_timeout_secs` (default 60s) for the full request/response
cycle. Additionally, `HttpConnector::set_connect_timeout()` enforces the
TCP-level connect timeout on both HTTP and HTTPS clients. The implementation
is in `handler.rs` and `create_http_client()`/`create_https_client()`.
No new ADR needed; the decision was already made in ADR-015.
- **Cross-references**: ADR-015, ADR-017
### ~~OQ-10: Should ACME contact email be a required config field?~~
- **Origin**: Implementation review finding C2, [tls.md](tls.md), [config.md](config.md)
- **Status**: resolved
- **Priority**: high
- **Resolution**: This is not an open question — the architecture already
specifies `acme_contact` as a required field in ACME mode (config.md
validation rule 19). The field is defined in the `ListenerConfig` table and
shown in TOML examples. Let's Encrypt requires a contact email for production
certificate requests. The implementation bug (C2: `contact: vec![]`) has been
fixed — `acme_contact` is now correctly wired from config to the ACME state
machine. No new ADR needed — the decision is already documented in config.md
and tls.md.
- **Cross-references**: ADR-004
### ~~OQ-11: How should `X-Forwarded-Proto` be derived per-listener?~~
- **Origin**: Implementation review finding W14, [proxy.md](proxy.md)
- **Status**: resolved
- **Priority**: medium
- **Resolution**: The hardcoded `is_https: true` behavior is correct for a
TLS-terminating reverse proxy. The proxy only proxies requests on the HTTPS
listener, which always sets `X-Forwarded-Proto: https`. The HTTP redirect
listener sends a 301 redirect and does NOT proxy requests, so
`X-Forwarded-Proto` is not set there. This behavior is correct and matches
the architecture spec (proxy.md). The implementation should add a comment
documenting this rationale to prevent future "fixes" that would change the
behavior. No ADR or spec change needed — just a code comment.
- **Cross-references**: ADR-021
## Operations
### ~~OQ-12: Should request access logging be mandatory or optional?~~
- **Origin**: Implementation review finding W13, [operations.md](operations.md)
- **Status**: resolved
- **Priority**: high
- **Resolution**: Access logging is mandatory and always-on at `info` level.
The architecture spec (operations.md) already states: "Access logging is
**always-on** — it is the primary observability mechanism for the proxy and
is required for fail2ban integration. There is no configuration option to
disable access logging." The `log_request!` macro exists in the codebase
but is not called — this is an implementation gap (W13), not an
architectural question. No ADR needed; ADR-007 already covers the log format.
- **Cross-references**: ADR-007
Reference in New Issue View Git Blame Copy Permalink