Automatic ACME (Let's Encrypt) certificate provisioning & renewal (Part of #1608)#1858
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.
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
Provision and auto-renew the server's TLS certificate over the ACME HTTP-01 challenge, building on the #1603 TLS listener. The issued certificate hot-swaps into the existing ReloadableCertResolver — no second resolver or listener. A :80 challenge/redirect listener answers HTTP-01 and bounces plain HTTP to HTTPS; a self-signed placeholder lets :443 bind before first issuance; a coordinator-leader-elected renewal loop renews before expiry on the serving tier. - config: [server.tls.acme] (domains, contact_email, directory, cache_dir, http_challenge_port, renew_before_days); TlsConfig cert_path/key_path now Option; TlsConfig::validate() enforces exactly-one-of static/ACME and rejects wildcards (DNS-01 is out of scope, #1620). Staging default. - acme module: challenge (:80 router + Http01Tokens), store (AcmeStore trait + 0600 FsAcmeStore), renewal (issuance, needs_renewal, placeholder, AcmeStatus + AcmeHealthIndicator, renewal loop). - app: validate guard, ACME listener bring-up, challenge + renewal task spawns as server_shutdown children, health indicator + reporter wiring. - doctor: `--online` (alias `--preflight`) ACME preflight probes (port 80/443 reachability, DNS-points-here) plus offline stored-cert expiry; pure graders with bounded I/O wrappers. - deps: instant-acme + rcgen, both default-features=false + ring so no aws-lc-rs enters the autumn-web graph. Part of #1608
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
Part of #1608. Address review findings on the ACME provisioning slice: - Namespace stored ACME material per directory. Both the account and the certificate files now live under {cache_dir}/{directory-label}/ (0700), so promoting directory = "production" can no longer reuse a leftover, still-valid but browser-untrusted staging leaf. Extend the doctor cert scan to descend one level so it still finds the newest pair. - Make the store write atomic: write to {path}.tmp (0600) then rename over the target, so a crash mid-write cannot leave a torn cert/key for the loader. - Guarantee HTTP-01 token cleanup on every exit path with an RAII guard, so a failing set_ready/poll_ready no longer leaks published tokens that accumulate across repeated transient failures. - Order immediately at boot when the stored leaf is already inside its renew-before window (or expired), instead of serving a dead cert for up to the renewal check interval. - Document that HTTP-01 ACME is single-host in this slice and warn at startup when it is configured alongside a distributed scheduler backend (multi-replica deployment), where local-store validation is not fleet-safe. Add unit tests for per-directory cert isolation, token cleanup on the error and success paths, and the boot-time order decision.
|
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: c3c54ac455
ℹ️ 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".
| // 8a-bis. Automatic ACME provisioning (issue #1608). When [server.tls.acme] | ||
| // is configured: always inspect the stored certificate offline (expiry), and | ||
| // — only under --online — actively probe port reachability and DNS. | ||
| if let Some(acme) = resolve_acme_doctor_config(toml_table.as_ref()) { |
There was a problem hiding this comment.
When a valid ACME-only config is present ([server.tls.acme] with no cert_path/key_path), run() still adds the existing static tls doctor task before reaching this new ACME branch. resolve_tls_paths() treats the [server.tls] table as configured and returns empty paths, so check_tls_impl emits a failing “must set both cert_path and key_path” result for every ACME deployment, and autumn doctor --strict fails even though TlsConfig::validate() accepts this mode.
Useful? React with 👍 / 👎.
| Err(e) => { | ||
| let msg = format!("ACME renewal leader election failed: {e}"); | ||
| tracing::warn!("{msg}"); | ||
| return; |
There was a problem hiding this comment.
If the scheduler coordinator returns an error here (for example the Postgres advisory-lock pool is unavailable on first boot), renewal is skipped while the server keeps serving the self-signed placeholder, but AcmeStatus is not marked failed and the reporter callback is not invoked. In that scenario the ACME health indicator can remain Up with no cert and operators lose the configured error-reporting signal until they notice the log line.
Useful? React with 👍 / 👎.
| let path = entry.path(); | ||
| if path.is_dir() { | ||
| search_dirs.push(path); |
There was a problem hiding this comment.
Scanning every immediate subdirectory means doctor can report on a certificate the runtime will never load. For example, after promoting directory = "production", a newer leftover cache_dir/staging/*.chain.pem can be selected here even though FsAcmeStore for production reads only cache_dir/production, causing acme_stored_cert to pass or fail based on the staging cert instead of the configured production cert.
Useful? React with 👍 / 👎.
…rt coordinator errors; scan only configured ACME directory — Part of #1608
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fd5f7432de
ℹ️ 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".
| // The `:80` challenge/redirect listener. Fail-fast on a bind error: | ||
| // `:80` needs privilege (CAP_NET_BIND_SERVICE) and ACME validation | ||
| // cannot succeed without it. | ||
| let challenge_addr = format!("0.0.0.0:{http_challenge_port}"); |
There was a problem hiding this comment.
When the configured ACME domain has only AAAA records, or the app is otherwise deployed IPv6-first, the TLS listener can be configured to bind IPv6 via server.host, but this challenge listener is hard-coded to IPv4-only 0.0.0.0. Let's Encrypt will validate HTTP-01 against the domain's IPv6 address and get a refused/timeout connection even though the app is listening on HTTPS; bind the challenge listener to the configured host or to both IPv4 and IPv6 addresses.
Useful? React with 👍 / 👎.
| // table and reports empty paths, so `check_tls_impl` would emit a spurious | ||
| // "must set both cert_path and key_path" Fail for EVERY ACME deployment. | ||
| // ACME mode is graded by the acme checks below instead. | ||
| let acme_config = resolve_acme_doctor_config(toml_table.as_ref()); |
There was a problem hiding this comment.
When [server.tls.acme] is supplied only by the active profile ([profile.prod] or autumn-prod.toml), this raw toml_table lookup does not see it, while resolve_tls_paths() below uses the merged runtime table and sees [server.tls] with no static paths. Fresh evidence beyond the earlier static-TLS comment is this profile-specific mismatch: autumn doctor --strict still runs the static TLS check and fails a valid ACME deployment unless the ACME block is top-level.
Useful? React with 👍 / 👎.
| tasks.push(Box::new(move || check_acme_stored_cert_impl(&stored))); | ||
|
|
||
| if opts.online | ||
| && let Some(domain) = acme.domains.first().cloned() |
There was a problem hiding this comment.
For a multi-SAN ACME config, --online only probes the first domain, but the renewal flow orders authorizations for every value in config.domains. If domains = ["ok.example.com", "bad.example.com"], doctor can pass while issuance later fails on the unprobed name; enqueue the port/DNS checks for each configured domain.
Useful? React with 👍 / 👎.
…config; bind HTTP-01 listener dual-stack; probe all configured ACME domains — Part of #1608
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fa435e4a83
ℹ️ 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".
| let static_cert_key_present = resolve_tls_paths() | ||
| .is_some_and(|(cert, key)| !cert.as_os_str().is_empty() && !key.as_os_str().is_empty()); | ||
| if should_run_static_tls_check(acme_config.is_some(), static_cert_key_present) { |
There was a problem hiding this comment.
When [server.tls.acme] is configured together with static paths, the runtime fails fast in TlsConfig::validate() because the modes are mutually exclusive. Here doctor only uses the static paths to decide whether to run the static cert inspection, so a config with both valid cert_path/key_path and ACME runs both checks and can pass --strict; with just one static path it skips the static check entirely. This lets doctor bless a config that the app will not boot with; add an explicit XOR/half-pair validation before enqueuing checks.
Useful? React with 👍 / 👎.
| /// before issuance is not a failure). | ||
| fn resolve_acme_stored_cert_data(cert_dir: &std::path::Path) -> TlsDoctorData { | ||
| // Find the newest `<id>.chain.pem` + matching `<id>.key.pem` pair. | ||
| let Some((chain, key)) = newest_acme_cert_pair(cert_dir) else { |
There was a problem hiding this comment.
When the configured domains change but old cert files remain in the same directory namespace, this scans whichever <id>.chain.pem is newest instead of the CertId::from_domains(acme.domains) pair that build_acme_tls_listener will load. An expired old cert can make doctor --strict fail even though runtime ignores it and issues the new id, while a healthy old cert can be reported as the stored cert for domains it does not cover. Target the cert id derived from the active ACME domains rather than the newest pair.
Useful? React with 👍 / 👎.
| && let Ok(addr) = sock.local_addr() | ||
| { | ||
| let ip = addr.ip(); | ||
| if !ip.is_loopback() && !out.contains(&ip) { |
There was a problem hiding this comment.
In a NAT or container deployment, the UDP source address discovered here is commonly a private address such as 10.x or 172.16/12, while the ACME domain resolves to a public load balancer or elastic IP. Because any non-loopback address is accepted as a public local IP, resolve_dns_points_here returns ResolvesElsewhere and doctor --online --strict fails even though HTTP-01 can route through NAT; filter private/link-local addresses or downgrade this case to LocalIpsUnknown.
Useful? React with 👍 / 👎.
…nfigured ACME cert-id; treat private source IPs as unknown; fix TOML typo-suggester regression — Part of #1608 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LMeNyt3tNtHRvcQm9QGsZs
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3d9042d7e1
ℹ️ 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".
| std::sync::Arc::new(crate::scheduler::InProcessSchedulerCoordinator::new( | ||
| config.scheduler.resolved_replica_id(), | ||
| )) |
There was a problem hiding this comment.
When scheduler.backend = "postgres" is configured but coordinator_from_config cannot build the distributed coordinator (for example no DB pool/db feature in this process), this falls back to InProcessSchedulerCoordinator. Fresh evidence beyond the earlier leader-election path is that this construction error is swallowed before AcmeRenewalTask::try_acquire can record a failure, so every replica can acquire its own local lease and start ordering the same certificate, which races the per-process HTTP-01 token maps/local stores and can burn CA rate limits; this should fail/record ACME unhealthy instead of silently degrading leadership.
Useful? React with 👍 / 👎.
…a configured distributed coordinator is unavailable — Part of #1608
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8aca68f3a6
ℹ️ 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".
| .map(str::to_owned) | ||
| .collect::<Vec<_>>() | ||
| }) | ||
| .unwrap_or_default(); |
There was a problem hiding this comment.
When [server.tls.acme] omits domains or sets domains = [], this unwrap_or_default() turns it into an empty list and run() subsequently reports acme_stored_cert as Pass with no online probes. The runtime's TlsConfig::validate() rejects the same config at boot, so autumn doctor --strict can bless a deployment that immediately exits; mirror AcmeConfig::validate() before adding the ACME checks.
Useful? React with 👍 / 👎.
| let status = if snap.last_failure.is_some() && in_danger { | ||
| HealthStatus::Down | ||
| } else { | ||
| HealthStatus::Up |
There was a problem hiding this comment.
When a stored ACME certificate is already expired at boot, build_acme_tls_listener records its cert_not_after_unix and serves it while renewal runs. Since last_failure is still None, this branch returns Up even with negative days_until_expiry, and if leadership is skipped or issuance is just pending the health indicator hides an already-invalid TLS certificate; treat expired/not-after <= now as Down independently of a renewal failure.
Useful? React with 👍 / 👎.
…ACME health Down for an already-expired stored certificate — Part of #1608
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8f8079a2ed
ℹ️ 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".
| if due { | ||
| self.try_renew_once(coordinator, reporter).await; | ||
| } |
There was a problem hiding this comment.
When a non-leader is serving the placeholder or an older cert, a leader can later persist a healthy cert to a shared store; on the next tick stored_not_after() returns a future expiry, so this if due branch skips try_renew_once, and the only call to adopt_stored_cert_if_newer() is never reached. In that shared-store/distributed-coordinator scenario, followers can keep serving the placeholder or stale cert until the next renew window instead of hot-swapping the certificate the leader already issued.
Useful? React with 👍 / 👎.
| write_owner_only(&chain_path, &chain).await?; | ||
| write_owner_only(&key_path, &key).await |
There was a problem hiding this comment.
During renewal, these two independent renames can publish the new chain while the old key is still in place if the process crashes or another replica reads between writes. load_cert then sees both files, but the pair cannot be loaded; after a retry fails, stored_not_after() still reads the future-dated chain and maybe_renew stops retrying, leaving the process on the placeholder or old cert until the chain enters the renew window. Publish the pair as a single generation or make an unusable pair count as absent for renewal decisions.
Useful? React with 👍 / 👎.
…eat a torn cert/key pair as absent for renewal decisions — Part of #1608
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 02c6bd7fdb
ℹ️ 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".
| let (has_cert_path, has_key_path) = resolve_tls_paths() | ||
| .map_or((false, false), |(cert, key)| { | ||
| (!cert.as_os_str().is_empty(), !key.as_os_str().is_empty()) | ||
| }); |
There was a problem hiding this comment.
After the TLS-mode fix, this still collapses an explicitly present but empty static path to false. With [server.tls.acme] plus cert_path = "" (or an empty env override), the runtime sees Some("") and rejects the mixed static+ACME configuration in TlsConfig::validate(), but doctor treats it as ACME-only, skips the static check, and can pass --strict; keep key presence separate from path emptiness when computing the XOR inputs.
Useful? React with 👍 / 👎.
| let directory = acme | ||
| .get("directory") | ||
| .cloned() | ||
| .and_then(|v| v.try_into::<autumn_web::config::AcmeDirectory>().ok()) | ||
| .unwrap_or_default(); |
There was a problem hiding this comment.
When server.tls.acme.directory has a typo or malformed custom table, this try_into(...).ok().unwrap_or_default() silently turns it into staging. The runtime deserializes AcmeDirectory and fails to boot on the same value, while autumn doctor --strict can inspect the staging cache and pass; surface the parse error as an acme_config failure instead of defaulting.
Useful? React with 👍 / 👎.
…nvalid acme.directory in doctor — Part of #1608
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e79a4579ad
ℹ️ 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".
| for domain in &self.domains { | ||
| if domain.starts_with("*.") { |
There was a problem hiding this comment.
When [server.tls.acme] domains = [""] (or a whitespace-only string) is configured, domains.is_empty() is false and this loop only rejects wildcard prefixes, so TlsConfig::validate() accepts the config. The server then tries to create/order a certificate for an empty DNS identifier via the placeholder/CSR and ACME order paths, causing startup or first issuance to fail while doctor can still report the ACME config as valid; trim each entry and reject empty domain names here.
Useful? React with 👍 / 👎.
| /// Port to serve the HTTP-01 challenge (and the HTTP→HTTPS redirect) on. | ||
| /// The ACME CA always validates HTTP-01 over port 80, so this defaults to | ||
| /// `80`; override it when a front-end forwards `:80` to another port. | ||
| #[serde(default = "default_acme_http_challenge_port")] |
There was a problem hiding this comment.
If an operator sets http_challenge_port = 0, bind_challenge_listeners(0) binds an ephemeral local port, but HTTP-01 validation still reaches the domain on port 80 and there is no code that discovers or publishes the random port. The process can stay up serving the placeholder while every issuance attempt fails externally; validate this field as non-zero instead of allowing an unusable random-port bind.
Useful? React with 👍 / 👎.
…p_challenge_port — Part of #1608 AcmeConfig::validate() accepted domains = [""] (whitespace-only entries pass the is_empty() length check) and http_challenge_port = 0 (binds an ephemeral OS port the HTTP-01 validator on port 80 can never reach). Both now fail validation with actionable errors, and doctor's check_acme_config_impl mirrors the same rejections so the two stay in parity.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ac74cfcc43
ℹ️ 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".
| if resolved.iter().any(|ip| local_public.contains(ip)) { | ||
| DnsPointsHere::Matches |
There was a problem hiding this comment.
When a domain has multiple A/AAAA records and only one of them is this host, this any marks the DNS check as a pass. In this ACME HTTP-01 implementation the token map is per-process, so a stale second address or an AAAA record pointing elsewhere can still be selected by the CA and get a 404/timeout while autumn doctor --online --strict stays green; treat unmatched resolved addresses as a failure (or at least report them) instead of passing on the first match.
Useful? React with 👍 / 👎.
| let http_challenge_port = acme | ||
| .get("http_challenge_port") | ||
| .and_then(toml::Value::as_integer) | ||
| .map_or(80, |v| u16::try_from(v).unwrap_or(80)); |
There was a problem hiding this comment.
If http_challenge_port is present but not a valid u16 (for example 70000, -1, or a quoted string), the runtime's typed AcmeConfig deserialization rejects the config before boot, but this resolver silently falls back to 80. With otherwise-valid domains/email, acme_config can pass and the online probes check the wrong port, so doctor --strict can bless a deployment that the app will not start.
Useful? React with 👍 / 👎.
Brings the ACME HTTP-01 slice up to date on trunk-dev, which now contains the merged #1603 TLS-listener hardening (concurrent-handshake TlsListener, validate_chain_certs, not-before / chain expiry grading, reload+handshake tuning). Resolved via a single merge (rebase produced repeated conflicts replaying the old TLS commits onto the evolved ones). Conflicts resolved to keep BOTH the #1603 hardening and the #1608 ACME work: - config.rs: TlsConfig keeps the tls tuning fields AND Option cert/key paths + acme: Option<AcmeConfig>; validate() keeps both the #1603 and #1608 XOR rules. - tls.rs: trunk's hardened TlsListener / validate_chain_certs / inspect_leaf, plus the acme-gated certified_key_from_pem and leaf_not_after_from_pem helpers (leaf_not_after adapted onto trunk's leaf_validity_unix). - app.rs: server_shutdown created before the bind block; build_tls_listener and build_acme_tls_listener thread a shutdown child token into TlsListener::new; ACME :80 challenge listener + renewal spawn + leadership_degraded preserved. - doctor.rs: trunk's recognized-key / dotenv / manifest-dir TLS detection and chain/expiry grading AND the #1608 ACME checks (XOR presence, directory / port / domain validation); ACME stored-cert resolution and grading updated to the 3-field Healthy enum (not_yet_valid / expired / days_until_expiry). - Cargo: acme feature still implies tls; instant-acme / rcgen / socket2 kept. Also folds in two deferred #1858 Codex P2s (both in doctor.rs): - ACME DNS check: a partial match (the domain resolves to this host AND to at least one other address) is no longer a clean Pass. HTTP-01's per-process token map means the CA validating an unmatched address 404s, so the grader now reports a Warn naming the unmatched address instead of passing on the first match. Graded Warn (not hard Fail) because local_ips() is best-effort. - ACME http_challenge_port: a value that is present but not a valid u16 (out of range like 70000/-1, or a non-integer such as a quoted string) now surfaces as an acme_config FAIL, mirroring the invalid-directory handling, instead of silently defaulting to 80 for a config the runtime rejects at boot. Tests added for both nits.
Local verification (the gate)Per the project's "local verification is the gate" policy, this is a clean-rebuild verification on head
This branch is now rebased onto Per the "local verification is the gate" policy, this PR is merge-ready on the strength of the green local gates above. Now that it targets Generated by Claude Code |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 604948d11a
ℹ️ 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".
| let cert_env = std::env::var("AUTUMN_SERVER__TLS__CERT_PATH").is_ok(); | ||
| let key_env = std::env::var("AUTUMN_SERVER__TLS__KEY_PATH").is_ok(); |
There was a problem hiding this comment.
When static TLS paths are supplied through .env/.env.<profile>, the runtime and resolve_tls_paths() see them via the profile-aware dotenv overlay, but this XOR check looks only at the process environment. In a config with [server.tls.acme] in TOML and AUTUMN_SERVER__TLS__CERT_PATH/KEY_PATH in dotenv, tls_mode is not failed and doctor --strict can pass if the static cert inspection passes, while the server rejects the same mixed static+ACME configuration at boot; read these keys through the same dotenv-aware env used above.
Useful? React with 👍 / 👎.
| a.iter() | ||
| .filter_map(toml::Value::as_str) | ||
| .map(str::to_owned) |
There was a problem hiding this comment.
Using filter_map drops non-string entries instead of reporting the deserialization error the runtime would hit for Vec<String>. With domains = ["app.example.com", 123], doctor keeps the valid string and can pass all ACME checks, but the server fails to parse [server.tls.acme] before boot; surface a config failure whenever any domain entry is not a string.
Useful? React with 👍 / 👎.
| let due = self.stored_not_after().await.is_none_or(|not_after| { | ||
| needs_renewal(not_after, self.config.renew_before_days, now_unix()) |
There was a problem hiding this comment.
If an operator sets renew_before_days greater than or equal to the CA's issued certificate lifetime (for example 100 with Let's Encrypt's ~90-day certs), this check remains true immediately after every successful renewal, so the hourly loop orders a fresh certificate every tick until rate limits are hit. Validate or cap the configured window, or add a post-success cooldown, so a typo cannot burn ACME issuance quota.
Useful? React with 👍 / 👎.
…e; fail on non-string ACME domains and dotenv-supplied mixed TLS mode in doctor — Part of #1608
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d5062912dd
ℹ️ 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".
| let renew_before_days = acme | ||
| .get("renew_before_days") | ||
| .and_then(toml::Value::as_integer) | ||
| .map_or(30, |v| u32::try_from(v).unwrap_or(u32::MAX)); |
There was a problem hiding this comment.
When renew_before_days is present but not a TOML integer, for example renew_before_days = "30", this as_integer() chain treats it the same as an absent key and defaults to 30. The runtime deserializes the field as u32 and rejects the same config before boot, so autumn doctor --strict can pass a deployment that will not start; preserve a parse error here like http_challenge_port does instead of silently defaulting malformed values.
Useful? React with 👍 / 👎.
| for (index, domain) in self.domains.iter().enumerate() { | ||
| let trimmed = domain.trim(); |
There was a problem hiding this comment.
When a configured domain has leading or trailing whitespace, such as " app.example.com ", validation trims only for the blank/wildcard checks but leaves the accepted self.domains value unchanged. That untrimmed string is later passed to the placeholder/CSR generation and ACME Identifier::Dns ordering paths, so validation succeeds and startup or issuance then fails on an invalid DNS identifier; reject domain != trimmed or normalize before use.
Useful? React with 👍 / 👎.
2 participants
Before / After
Before: serving HTTPS meant an operator-provided certificate on disk
(
[server.tls] cert_path/key_path, #1603) or a reverse proxy that terminates TLS.After: set a domain and a contact email and the app obtains and auto-renews a
Let's Encrypt certificate itself:
:80and 308-redirects allother
http://traffic tohttps://.:443binds immediately behind a self-signed placeholder; the real certhot-swaps in once issued — no restart, no dropped connections.
fleet so only one replica orders per certificate.
production quota. Opt into production with
directory = "production".How
ReloadableCertResolvervia its existingstore()(RwLock<Arc<CertifiedKey>>swap);
build_server_config/TlsListenerare reused unchanged. No parallelmachinery.
instant-acmedrives the HTTP-01 order state machine;rcgengenerates the ACMEaccount key, the certificate keypair + CSR, and the placeholder cert.
autumn/src/acme/module:challenge(the:80router +Http01Tokens),store(an asyncAcmeStoretrait + a0600FsAcmeStore; account keyed perdirectory so staging↔production re-registers cleanly),
renewal(issuance,needs_renewal, placeholder,AcmeStatus/AcmeHealthIndicator, the loop).:80challenge listener and the renewal loop spawn as siblings of themain server, each a
child_token()ofserver_shutdown.TlsConfig.cert_path/key_pathare nowOption;TlsConfig::validate()(called from the same pre-bind guard that rejects TLS+unix_socket) enforces exactly
one of static-cert XOR ACME, requires paths together, requires non-empty
domains+email in ACME mode, and rejects
*.wildcards with a message pointing atIssue wildcard certificates via DNS-01 for tenant subdomains #1620 (DNS-01, out of scope).
AcmeHealthIndicator(groupHealthOnly, so a renewal blipdoes not fail
/ready) gradesDownonly when a failure coincides with realexpiry danger. Each renewal failure is dispatched through the registered
ErrorReporterchain (Sentry/etc.) and always logged.autumn doctor --online(alias--preflight) adds ACME preflightprobes — port 80/443 reachability and DNS-points-here for the configured domain —
plus an always-on offline stored-cert expiry check. Network probes run only
under
--onlineso defaultdoctorstays offline and non-flaky. Every grader is apure function of injected inputs (mirroring
check_tls_impl).Two new crates, both behind the off-by-default
acmefeature, both pinneddefault-features = false+ring:The
default-features = falseis load-bearing: the crates' default features pullaws-lc-rs, which would violate the workspace single-crypto-backend rule (rustls 0.23i.e. aws-lc-rs is entirely absent from autumn-web's
tls,acmegraph (it exists inthe workspace only via
autumn-storage-s3's AWS SDK, as allowed). The isolated graphresolves to
ring v0.17,rustls 0.23,instant-acme 0.8.5,rcgen 0.14.8,tokio-rustls,hyper-rustls.Limitations / Follow-ups
*.domains are rejected with amessage pointing at Issue wildcard certificates via DNS-01 for tenant subdomains #1620.
AcmeStoretrait exists so Let tenants connect custom domains with per-domain ACME certificates #1635 can add a DB/per-tenant impl; onlyFsAcmeStoreships here.
Http01Tokensmap is per-host, soHTTP-01 assumes challenge traffic reaches the ordering replica (single-host
assumption). Renewal is leader-elected; non-leaders adopt a newer cert from a
shared store, but a network-partitioned token map is not addressed here.
"reverse-proxy opt-out" note in the deployment guide. Not written here.
Tests
All unit tests land green (no real network in the default suite):
challenge_router(200+key-auth / 404 / 308-preserving-path+query),Http01Tokensround-trip,
FsAcmeStoreaccount+cert round-trip with0600perms,needs_renewalthreshold matrix,
TlsConfig::validate()matrix (static-only / acme-only / both /neither / cert-without-key / empty domains / empty email / wildcard→#1620), placeholder
self-signed load+swap via
resolver.store(),AcmeHealthIndicatorgrading, and thedoctor
check_acme_*_implpure graders. A real-ACME (pebble/LE-staging) integrationtest is intentionally not included in the default suite (tracked as a follow-up).
Generated by Claude Code