reverse-proxy/tasks/fix/acme-contact-and-challenge.md at d7f811ffb59df497d5e1103cd6afaa2eb03c9516

Files

glm-5.1 f9d7b8112b Decompose implementation review fixes into 14 atomic tasks with post-fix review

Break down findings from review #002 into dependency-ordered fix tasks:

Critical/High:
- fix/acme-contact-and-challenge (C1+C2): Add acme_contact field, wire to
  ACME, remove unused challenge_config, add validation rule 19
- fix/remove-health-and-hardcode-https (W5+W14+ADR-022): Remove /health
  from main listener, hardcode X-Forwarded-Proto to https
- fix/config-reload-static-drift (C4): Use ArcSwap<StaticConfig> so reload
  diffs against last config, not startup config
- fix/access-logging (W13): Wire up log_request! macro for every proxied
  request with client_ip, host, method, path, status, upstream, duration_ms

Medium:
- fix/graceful-shutdown (W1+W7): Join HTTPS tasks with timeout instead of
  abort, add shutdown signal to admin socket and eviction task
- fix/connect-timeout (W4): Wire upstream_connect_timeout_secs to enforce
  separate connect timeout

Low/Independent:
- fix/token-bucket-nanosecond (W6): Use as_nanos() instead of as_millis()
- fix/normalize-host-ipv6 (S3): Handle IPv6 bracket notation in normalize_host
- fix/http-port-validation (S1): Validate http_port in range 0 or 1-65535
- fix/integration-test-toml (S10): Fix double-nested listeners.listeners.sites
- fix/logging-test-global-subscriber (W9): Use try_init() to avoid test conflicts
- fix/fragile-error-detection (W3): Add typed error matching or documented string match
- fix/add-code-comments (C3,W8,W10,W11,S9): Document correct-but-non-obvious behaviors
- fix/request-timeout-scope (S8): Document full-request timeout scope
- fix/clean-dead-code (S4+S2): Remove dead_code annotations, add #[non_exhaustive]

Review gate:
- review/post-fix-review: Verify all fixes against architecture spec

2026-06-12 04:08:45 +00:00

3.6 KiB

Raw Blame History

id, name, status, depends_on, scope, risk, impact, level, review_findings

name

status

depends_on

scope

risk

impact

level

review_findings

fix/acme-contact-and-challenge

Fix ACME contact email wiring and remove unused challenge config

pending

moderate

high

component

implementation

Description

Fix two related ACME bugs that prevent Let's Encrypt certificate provisioning from working:

C2: The acme_contact field is defined in the architecture (TlsConfig.acme_contact in config.md) but missing from the TlsConfig struct in static_config.rs. The ACME setup in acceptor.rs always passes contact: vec![], which will cause production Let's Encrypt registration to fail with a 400-level error.
C1: When TlsMode::Acme is matched in main.rs, the challenge_config and resolver fields are destructured with _ (discarded). The separate challenge_config is unnecessary — the rustls-acme crate's ResolvesServerCertAcme resolver handles TLS-ALPN-01 challenges automatically on the main listener when acme-tls/1 is included in the ALPN list (which build_acme_server_config already does at line 28). The separate challenge_config and build_acme_challenge_config function should be removed.

Changes Required

src/config/static_config.rs:

Add acme_contact: String field to TlsConfig struct (with #[serde(default)])
Update TOML deserialization tests to include acme_contact

src/config/validation.rs:

Add validation rule 19: In ACME mode, tls.acme_contact must be a non-empty string starting with mailto:
Add ValidationError::AcmeContactInvalid variant

src/tls/acceptor.rs:

Pass tls_config.acme_contact to AcmeTlsConfig.contact instead of vec![]
Remove build_acme_challenge_config function (dead code after C1 fix)
Remove challenge_config field from TlsMode::Acme variant
Update setup_tls to only return default_config and resolver (no challenge_config)

src/main.rs:

Update TlsMode::Acme destructuring to remove challenge_config: _
Remove any references to the discarded fields

src/tls/acme.rs:

Ensure AcmeTlsConfig.contact is used properly (it already has contact: Vec<String> field and self.contact.iter().map(|c| c.as_str()) in setup() — the struct is fine, it just needs to receive the actual contact from config)

Tests:

Update static_config.rs tests for new acme_contact field
Update validation.rs tests for new ACME contact validation
Update acceptor.rs tests for removed challenge_config
Verify ACME setup still creates resolver correctly

Acceptance Criteria

acme_contact field exists on TlsConfig and deserializes from TOML
Validation rejects ACME mode without acme_contact (or with non-mailto: value)
AcmeTlsConfig.contact receives the configured acme_contact value (not empty vec)
build_acme_challenge_config function is removed
TlsMode::Acme variant no longer has challenge_config field
main.rs no longer discards ACME resolver or challenge config
ACME TLS-ALPN-01 challenge handling still works via main listener's ALPN protocol list
All existing tests pass
cargo clippy passes with no warnings

References

docs/architecture/config.md — acme_contact field definition, validation rule 19
docs/architecture/tls.md — ACME mode, contact email
docs/reviews/002-implementation-review.md — C1, C2 findings
src/tls/acceptor.rs — current ACME setup code
src/tls/acme.rs — AcmeTlsConfig struct
src/config/static_config.rs — TlsConfig struct
src/config/validation.rs — validation rules

Notes

To be filled by implementation agent

Summary

To be filled on completion

3.6 KiB Raw Blame History