Serve HTTPS directly via config-driven TLS listener (Part of #1603)#1852
Conversation
Add an off-by-default `tls` feature that terminates HTTPS in-process when `[server.tls]` names a certificate + key, with no sidecar reverse proxy. - New `autumn_web::tls` module: fail-fast cert/key loading + validation (missing file, unparseable PEM, key/cert mismatch, expired leaf), a reloadable `ResolvesServerCert`, and offline leaf-expiry inspection. - A custom `axum::serve::Listener` (`TlsListener`) that performs the rustls handshake in `accept` and yields the peer `SocketAddr`, so all existing connect-info, trusted-proxy, and graceful-shutdown wiring is reused unchanged. A failed handshake is skipped, never killing the accept loop. - Background hot-reload task swaps the certificate on file change (e.g. after an ACME/certbot renewal) without a restart; a bad reload keeps the old cert. - Always-compiled `[server.tls]` config (cert_path, key_path, reload_interval_secs) with env overrides; fail-fast when configured on a binary built without the `tls` feature, or combined with a Unix socket. - `autumn doctor` gains a TLS check: fail on missing/invalid/expired cert, warn within 30 days of expiry. Gated behind a default-on cli `tls` feature. Reuses the already-vendored ring-backed rustls (no second TLS backend); adds tokio-rustls and x509-parser behind the feature. Covered by unit, config, doctor, and end-to-end HTTPS + graceful-shutdown integration tests. Part of #1603.
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
Apply review fixes to the direct-HTTPS listener: - Bound each inbound TLS handshake with a configurable timeout so a client that opens TCP but never sends a ClientHello can no longer park the accept loop and deny service to every other connection. Adds `[server.tls] handshake_timeout_secs` (default 10, clamped to >= 1) plus the `AUTUMN_SERVER__TLS__HANDSHAKE_TIMEOUT_SECS` env override, threaded into `TlsListener`; a stalled or silent handshake is now dropped and the loop continues. - Move the per-tick cert/key `stat` and PEM read in the cert-reload task onto `spawn_blocking` so blocking filesystem I/O no longer runs on a tokio worker; a join failure just skips the tick. - Delete the unused `NEAR_EXPIRY_WARN_DAYS` constant. Add an integration test proving a hung (silent) handshake no longer wedges the listener: a well-behaved HTTPS request succeeds while a raw, silent TCP connection is held open. Part of #1603
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 14a43fa310
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| /// Whole days from `now_unix` until `notAfter`. Negative once expired. | ||
| #[must_use] | ||
| pub const fn days_until_expiry(&self, now_unix: i64) -> i64 { | ||
| (self.not_after_unix - now_unix) / 86_400 |
There was a problem hiding this comment.
When autumn doctor inspects a certificate that expired less than 24 hours ago, this division truncates toward zero, so days_until_expiry becomes 0; check_tls_impl then reports only a near-expiry warning instead of the promised failure. That lets a deploy with a freshly expired certificate pass the TLS check as a warning even though the runtime rejects expired certs at startup; base the failure on the raw timestamp or use floor division for negative durations.
Useful? React with 👍 / 👎.
| // TLS is "configured" if the section is present OR any TLS env var is set. | ||
| let configured = tls.is_some() | ||
| || std::env::var_os("AUTUMN_SERVER__TLS__CERT_PATH").is_some() | ||
| || std::env::var_os("AUTUMN_SERVER__TLS__KEY_PATH").is_some(); |
There was a problem hiding this comment.
Runtime env loading materializes server.tls when AUTUMN_SERVER__TLS__RELOAD_INTERVAL_SECS or AUTUMN_SERVER__TLS__HANDSHAKE_TIMEOUT_SECS is set, leaving empty cert/key paths and causing startup (or a feature-disabled build) to fail fast. This doctor detection only treats the cert/key env vars as TLS configuration, so a deployment with only one of those TLS tuning vars set reports tls as Pass/NotConfigured instead of the same actionable failure the server will hit.
Useful? React with 👍 / 👎.
…TLS tuning env vars — Part of #1603
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ef4e040d0e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // ClientHello (or stalls mid-handshake) must not park the accept | ||
| // loop and deny every other client. On timeout or handshake error, | ||
| // drop the connection and keep accepting. | ||
| match tokio::time::timeout(self.handshake_timeout, self.acceptor.accept(stream)).await { |
There was a problem hiding this comment.
When the direct-HTTPS listener is exposed to untrusted clients, awaiting the rustls handshake inside Listener::accept means axum cannot accept the next TCP connection until this client completes or handshake_timeout expires. With the default 10s timeout, a single silent/stalled TCP client can repeatedly occupy the accept loop and make all legitimate clients wait or time out; the handshakes need to be driven concurrently (or queued from background tasks) rather than performed inline before returning from accept.
Useful? React with 👍 / 👎.
| let env_cert = std::env::var("AUTUMN_SERVER__TLS__CERT_PATH") | ||
| .ok() | ||
| .filter(|v| !v.trim().is_empty()); | ||
| let env_key = std::env::var("AUTUMN_SERVER__TLS__KEY_PATH") | ||
| .ok() | ||
| .filter(|v| !v.trim().is_empty()); |
There was a problem hiding this comment.
When [server.tls] is supplied via the same .env / .env.<profile> layer that TomlEnvConfigLoader loads at runtime (for example in dev/test, or prod with dotenv enabled), this resolver only calls std::env::var, so autumn doctor reports TLS as not configured and skips certificate validation even though the app will boot the TLS listener. Resolve these variables through the runtime dotenv overlay, as other doctor checks do, so strict doctor cannot greenlight a dotenv-configured broken certificate.
Useful? React with 👍 / 👎.
…t loop; resolve TLS doctor vars via dotenv overlay FIX 1 (P1): TlsListener no longer runs the rustls handshake inline in accept(). A background acceptor task drains tcp.accept() and spawns each handshake as a bounded task (Arc<Semaphore>, MAX_CONCURRENT_HANDSHAKES = 256), forwarding completed streams over a bounded mpsc channel to accept(). A flood of silent/stalled clients can no longer serialize the accept loop. The acceptor loop is tied to the server_shutdown child token and breaks on cancel; accept() parks (never busy-loops) once the channel closes so axum's graceful-shutdown path drives teardown. Per-handshake timeout retained to release a stalled handshake's permit. Adds a concurrency regression test. FIX 2 (P2): resolve_tls_paths now reads the AUTUMN_SERVER__TLS__* vars (including the prefix scan) through the profile-aware dotenv overlay (os_env_with_dotenv_for_profile), the same layer sibling doctor checks use, so TLS config supplied via .env/.env.<profile> is detected instead of reported not-configured. Adds a dotenv-detection test. Part of #1603
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3f33376d6f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| detail: e.to_string(), | ||
| } | ||
| })?; | ||
| Ok(parsed.validity().not_after.timestamp()) |
There was a problem hiding this comment.
When the configured leaf certificate has a notBefore timestamp in the future, this helper only returns notAfter, so load_certified_key accepts the pair and the app boots even though normal TLS clients will reject every handshake until the validity window starts. Since the runtime already fail-fast rejects expired leaves for the same client-visible breakage, include the not_before timestamp in the inspection and reject certificates where now_unix is before it.
Useful? React with 👍 / 👎.
| .filter(|v| !v.trim().is_empty()); | ||
| let env_key = denv | ||
| .var("AUTUMN_SERVER__TLS__KEY_PATH") | ||
| .ok() | ||
| .filter(|v| !v.trim().is_empty()); |
There was a problem hiding this comment.
If a deployment has valid [server.tls] paths in TOML but sets AUTUMN_SERVER__TLS__CERT_PATH= or ...KEY_PATH= to an empty value, the runtime still treats the env var as present and overwrites the path with an empty PathBuf, causing startup to fail. Doctor drops empty env values here and falls back to the TOML path, so autumn doctor --strict can pass exactly the configuration the server will reject; keep the empty override so it resolves to the same missing-path failure.
Useful? React with 👍 / 👎.
…pty TLS env overrides in doctor to match runtime fail-fast - load_certified_key now fail-fast rejects a leaf whose notBefore is in the future (new TlsError::NotYetValid), symmetric to the expired check; LeafInspection carries not_before_unix + is_not_yet_valid, and doctor's check_tls_impl grades a not-yet-valid cert as Fail. - doctor's resolve_tls_paths reads AUTUMN_SERVER__TLS__CERT_PATH/KEY_PATH with .ok() (no empty-filter), so a present-but-empty override clears the TOML path exactly as the runtime materializes it, surfacing the same missing-path failure instead of a false doctor Pass. Part of #1603
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 20ec48d37a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| path: cert_path.to_path_buf(), | ||
| source, | ||
| })?; | ||
| chain.push(cert); |
There was a problem hiding this comment.
When cert_path contains a valid leaf followed by a malformed intermediate certificate, this loop only PEM-decodes and stores the extra block; the later checks parse and match only chain[0], so startup and autumn doctor can pass while rustls serves a chain that normal clients reject. Parse each certificate in the chain before constructing the CertifiedKey so a broken intermediate fails fast instead of booting a broken HTTPS listener.
Useful? React with 👍 / 👎.
| /// lets doctor treat those deployments as configured and surface the same | ||
| /// actionable failure instead of a false Pass. | ||
| fn any_tls_env_var_set() -> bool { | ||
| std::env::vars_os().any(|(name, _)| name.to_string_lossy().starts_with("AUTUMN_SERVER__TLS__")) |
There was a problem hiding this comment.
When an unrelated or mistyped variable shares this prefix, such as AUTUMN_SERVER__TLS__ENABLED=false, doctor treats TLS as configured and fails on missing cert/key. The runtime only materializes [server.tls] from the four recognized variables in apply_server_env_overrides_with_env, so that same process would serve plain HTTP; this divergence can make autumn doctor --strict block a deployment the server itself accepts.
Useful? React with 👍 / 👎.
…idate the full certificate chain — Part of #1603 - doctor: detect TLS-via-env only on the four recognized AUTUMN_SERVER__TLS__* keys the runtime reads (CERT_PATH, KEY_PATH, RELOAD_INTERVAL_SECS, HANDSHAKE_TIMEOUT_SECS) via the dotenv overlay, dropping the broad prefix scan so an unrecognized/mistyped var like AUTUMN_SERVER__TLS__ENABLED no longer forces a false doctor Fail on a config the server serves as plain HTTP. - tls: parse every non-leaf certificate in the chain before accepting it (new TlsError::ParseChainCert naming the 1-based position), so a malformed intermediate fails fast at startup and in doctor instead of booting a listener that serves a chain clients reject (rustls validates only the leaf).
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c8366ec3c5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| // Skip index 0 (the leaf); `leaf_validity_unix` already parses it. | ||
| for (idx, cert) in chain.iter().enumerate().skip(1) { | ||
| x509_parser::certificate::X509Certificate::from_der(cert.as_ref()).map_err(|e| { |
There was a problem hiding this comment.
When cert_path contains a valid leaf followed by an expired or not-yet-valid intermediate, this loop only parses the DER and then discards the parsed certificate, so startup and autumn doctor accept a chain that normal TLS clients reject during path validation. The leaf validity window is checked separately, but every served chain certificate should also have its validity window checked before returning Ok(()).
Useful? React with 👍 / 👎.
| /// is visible to doctor instead of being reported as not-configured. | ||
| fn resolve_tls_paths() -> Option<(std::path::PathBuf, std::path::PathBuf)> { | ||
| let (canonical, selected, _) = resolve_active_profiles(); | ||
| let merged = get_merged_toml_table_runtime(&canonical, &selected); |
There was a problem hiding this comment.
When an installed app is launched with AUTUMN_MANIFEST_DIR pointing at its config directory, the runtime prefers that directory for autumn.toml, but this newly added TLS resolver still reads the merged TOML from the doctor's current working directory via get_merged_toml_table_runtime. A [server.tls] section present only in the manifest-dir config is therefore treated as NotConfigured, letting strict doctor pass without checking the cert/key the server will actually use.
Useful? React with 👍 / 👎.
…; validate intermediate certificate lifetimes — Part of #1603
Local verification (the gate)Fresh, cold local verification of head commit
All gates PASS on a cold rebuild. The direct-HTTPS TLS listener slice was hardened through multiple adversarial-review and Codex rounds — handshake concurrency/DoS resistance, full cert-chain parse plus lifetime validation, and recognized-key/dotenv/manifest-dir-aware doctor grading including expired and not-yet-valid detection — each landed with accompanying regression tests. The GitHub Actions checks are currently sitting Generated by Claude Code |
2 participants
Part of #1603. First honest slice of serving HTTPS directly from the app process.
Before / After
Before — there is no inbound TLS. To serve HTTPS you must front the app with a reverse proxy (nginx/Caddy/an LB) that terminates TLS. Absent that, the app speaks plain HTTP on
host:port.After — adding a
[server.tls]section with a certificate + key makes the app terminate HTTPS itself, on the samehost:port:[server.tls]→ unchanged. Plain HTTP exactly as today; zero behavior change for existing apps.certbot/ACME renewal) — no restart. A failed reload logs an error and keeps serving the previously loaded certificate; a bad renewal never breaks the listener.autumn doctorflags problems. Offline check: fail on a missing/invalid/expired certificate, warn within 30 days of expiry, pass otherwise.The feature is off by default (
tlscargo feature). A binary built without it that finds[server.tls]configured fails fast with a clear "built without thetlsfeature" message rather than silently serving plain HTTP.How
autumn_web::tls(new module, behindtls): loads + validates the cert chain and key (rustls'CertifiedKey::from_dercompares the key'sSubjectPublicKeyInfoagainst the leaf, catching mismatches), rejects an expired leaf, exposes a reloadableResolvesServerCertbacked by anRwLock<Arc<CertifiedKey>>(a read-lock per handshake; a write-lock to swap), and an offlineinspect_leaffor doctor.axum::servevia a customListener.TlsListenerwraps the existingtokio::net::TcpListenerin atokio_rustls::TlsAcceptor; itsaccept()performs the rustls handshake and returns(TlsStream<TcpStream>, SocketAddr). Because the peer is a realSocketAddr, the HTTPS serve arm is byte-for-byte the TCP arm — the sameConnectInfo<SocketAddr>,TrustedProxiesLayer/ClientAddrresolution, SSE/WebSocket streaming, request timeouts, and graceful-shutdown wiring all apply unchanged. A thirdBoundListener::Tlsvariant and a mirroring serve arm are the only serving-loop additions.rustls— the same backend the outbound Postgres TLS path uses. No second TLS backend is introduced (no aws-lc-rs / native-tls / openssl).[server.tls]is always compiled (so it parses regardless of features and the "no feature" error is possible), withAUTUMN_SERVER__TLS__CERT_PATH/__KEY_PATH/__RELOAD_INTERVAL_SECSenv overrides. Nested[server.tls]keys are accepted bystrict_configautomatically (schema is serde-derived; the schema snapshot is updated).New dependencies (please review)
Both are feature-gated (
optional = true, pulled only by the newtlsfeature) and were already resolved inCargo.lockat these versions (via other crates), so adding them introduces no version churn:tokio-rustls0.26 — async accept-time rustls handshake.default-features = false+ring(no aws-lc-rs).x509-parser0.16 — parse the leaf certificate'snotAfterfor expired-cert fail-fast and the doctor near-expiry warning. (No default features.)The
autumn-clitlsfeature (default-on, so the shippedautumnbinary can diagnose TLS) enablesautumn-web/tls; it pulls in no additional CLI-level crates.Tests
autumn/src/tls.rs): valid load; missing file names the path; unparseable PEM; key/cert mismatch → error; expired leaf → error;inspect_leafreports future and past expiry; resolver swap.autumn/src/config.rs):[server.tls]TOML parse, reload-interval default/override, env-var materialization + partial override, defaultNone.autumn-cli/src/doctor.rs): expired → Fail, ≤30 days → Warn, healthy → Pass, invalid → Fail, feature-disabled → Warn, not-configured → Pass.autumn/tests/integration/tls_serving.rs,#[cfg(feature = "tls")]): a rustls client GETs/healthover TLS and gets 200; peerSocketAddrconnect-info flows through the TLS listener; graceful shutdown joins cleanly under TLS; a bad (plaintext) handshake does not kill the listener.Gates run green:
cargo fmt --all -- --check;cargo clippy --workspace --all-targets --features tls -- -D warningsand the plain no-feature clippy; targeted-ptests for all of the above; default-featurecargo checkfor both crates.Scope / Limitations / Follow-ups
This slice deliberately does not do (tracked for follow-up):
certbot/mkcertwalkthroughs) — a docs session owns these.ConnectInfo<SocketAddr>stream as plain TCP so awsupgrade rides the identical service; a full tokio-tungstenite-over-rustls client is disproportionate here and is left as a clearly-markedTODO(#1603 follow-up).accept, so one slow handshake briefly delays new accepts. Acceptable for this slice; a future change can offload the handshake to a spawned task if it becomes a bottleneck.Generated by Claude Code