alktls/tasks/config-validation-and-trivia.md

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

id	name	status	depends_on	scope	risk	impact	level	tags
config-validation-and-trivia	Config robustness + trivia batch — ALPN dedup, empty-domains validation, packaging excludes, https doc line (C-2, C-3, N-6, N-7)	completed		narrow	low	component	implementation	robustness packaging docs review-001 c2 c3 n6 n7

Description

Four small findings in one pass (all verified in the decomposition session):

1. C-2 — acme-tls/1 is appended unconditionally (src/server.rs:91); a caller who already includes it in alpns gets it twice (probed: alpn_protocols == [acme-tls/1, acme-tls/1]). Harmless to rustls-acme's challenge dispatch today, but the duplication leaks into the wire config and the ACME/non-ACME asymmetry is undocumented. Fix: a one-line if !alpn.contains(&b"acme-tls/1".to_vec()) guard — or pin the current shape in a doc note + test. The guard is the better default (idempotent construction), but either way a test pins the chosen shape.
2. C-3 — TlsIdentity::Acme with an empty domains list constructs successfully (probed) and spawns the order loop that then fails per-order at runtime as logged-only events. A one-line validation (if domains.is_empty() { return Err(TlsError::AcmeConfig("empty domain list".into())) }) at construction is additive and cheap. Note: empty contact stays legal (RFC 8555 §7.3 allows zero-contact accounts) — do NOT validate that. Mechanics note (2026-09-12 re-check): the new test must be #[cfg(feature = "acme")]-gated like the existing ACME tests — under default features the Acme arm already errors with AcmeConfig for the missing-feature reason (pinned by new_acme_identity_without_feature_returns_config_error, server.rs), so a non-gated empty-domains test would pass vacuously and prove nothing.
3. N-6 — tasks/*.md and docs/architecture/** ship in the published package (cargo package --list re-verified). One-line exclude addition: "tasks/, "docs/architecture/ per the alktunnels house pattern (internal SDD notes + architecture docs don't belong in the public package).
4. N-7 — AcmeDirectory::Custom(String) accepts any string and the URL goes to rustls-acme verbatim, so an http:// custom directory silently runs ACME over plaintext (token-bearing). One doc line on the variant ("must be an https:// ACME directory URL") closes it. Do not add runtime URL validation — the doc line is the fix (a caller pointing at a non-https test directory should not be blocked).

Work

1. C-2: the dedup guard (preferred) + a test pinning idempotence (caller-supplied acme-tls/1 → single entry).
2. C-3: the empty-domains validation + a test (TlsServerConfig::new(&Acme{domains: vec![]}, ..) → TlsError::AcmeConfig), feature-gated like the existing ACME tests.
3. N-6: exclude = [... , "tasks/", "docs/architecture/"]; re-run cargo package --list --allow-dirty + cargo publish --dry-run --allow-dirty.
4. N-7: the doc line on AcmeDirectory::Custom.

Verification

• Dedup: caller-supplied acme-tls/1 yields exactly one entry (test pinned)
• Empty-domains ACME construction errors with TlsError::AcmeConfig (test pinned)
• cargo package --list --allow-dirty contains neither tasks/ nor docs/architecture/; cargo publish --dry-run --allow-dirty passes
• The Custom variant's doc states the https requirement
• cargo test, --all-features, clippy, fmt, doc green

Acceptance Criteria

• All four findings resolved (or explicitly doc-pinned with a test, per finding)
• No behavior change beyond the three intended ones

References

• docs/reviews/001-implementation-review.md §C-2, §C-3, §N-6, §N-7
• src/server.rs:91 (the ALPN append), src/server.rs:64-145 (new_acme — where the domains validation goes), src/identity.rs:65 (AcmeDirectory::Custom), Cargo.toml:11 (exclude list)
• (line refs re-checked 2026-09-12 against the post-ADR-007/008 tree; the file has shifted since the decomposition — re-grep before editing rather than trusting these absolutely)

Notes

• C-2: implemented the dedup guard (preferred option) in new_acme — if !alpn.contains(&b"acme-tls/1".to_vec()) before the push. Also documented the ACME/non-ACME ALPN asymmetry on TlsServerConfig::new (ACME always serves acme-tls/1, appended idempotently; non-ACME uses the caller's list verbatim). Pinned by new_acme_caller_supplied_acme_tls_alpn_is_not_duplicated (src/server.rs, #[cfg(feature = "acme")]).
• C-3: added the empty-domains validation at the top of new_acme (before any AcmeConfig construction or task spawn) returning TlsError::AcmeConfig("TlsIdentity::Acme requires a non-empty domain list"). contact left unvalidated per the finding (RFC 8555 §7.3 zero-contact accounts are legal). Test new_acme_empty_domains_returns_config_error is #[cfg(feature = "acme")]-gated per the task's mechanics note.
• N-6: exclude now also lists "tasks/" and "docs/architecture/". Verified cargo package --list --allow-dirty contains neither path and cargo publish --dry-run --allow-dirty passes.
• N-7: doc line added on AcmeDirectory::Custom (src/identity.rs): the URL goes to rustls-acme verbatim and must be https:// (an http:// URL would run ACME token-bearing over plaintext); explicitly notes no runtime validation is applied so non-https test directories stay usable. No runtime check added, per the finding.

Summary

All four findings resolved. Changes:

1. src/server.rs — new_acme dedups the acme-tls/1 ALPN append (idempotent construction) and rejects an empty domains list with TlsError::AcmeConfig before spawning the order loop; the ACME/non-ACME ALPN asymmetry is documented on TlsServerConfig::new.
2. src/identity.rs — AcmeDirectory::Custom documents the https-only caller contract (no runtime validation, per finding).
3. Cargo.toml — exclude gains "tasks/" and "docs/architecture/".

New tests (both #[cfg(feature = "acme")]): caller-supplied acme-tls/1 yields exactly one entry; empty domains constructs to TlsError::AcmeConfig.

Verification: cargo test (default) and cargo test --all-features (77 lib tests, both new tests pass), cargo clippy --all-targets -- -D warnings, cargo fmt --check, cargo doc --no-deps, cargo package --list --allow-dirty (no tasks/ / docs/architecture/ entries), cargo publish --dry-run --allow-dirty — all green.