diff --git a/Cargo.toml b/Cargo.toml index 8cb4390..249db06 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -8,7 +8,7 @@ description = "Shared TLS setup types: server and client rustls configs, cert re repository = "https://git.alk.dev/alkdev/alktls" keywords = ["tls", "rustls", "acme", "noq", "network"] categories = ["network-programming", "cryptography", "asynchronous"] -exclude = [".opencode/", "AGENTS.md", "docs/reviews/", "docs/research/", "docs/plans/", "docs/sdd_process.md"] +exclude = [".opencode/", "AGENTS.md", "tasks/", "docs/reviews/", "docs/research/", "docs/plans/", "docs/architecture/", "docs/sdd_process.md"] [lib] name = "alktls" diff --git a/src/identity.rs b/src/identity.rs index ef7f5cf..9fd47ab 100644 --- a/src/identity.rs +++ b/src/identity.rs @@ -61,7 +61,11 @@ pub enum AcmeDirectory { /// The Let's Encrypt staging directory (rate limits are relaxed; /// certificates it issues are not trusted by browsers). Staging, - /// A custom ACME directory URL. + /// A custom ACME directory URL. The URL is passed to rustls-acme + /// verbatim and **must be an `https://` ACME directory URL** — an + /// `http://` URL would run ACME (token-bearing) over plaintext. + /// No runtime validation is applied (a non-https test directory + /// must stay usable); this is a caller contract. Custom(String), } diff --git a/src/server.rs b/src/server.rs index 98a78b8..b5f4348 100644 --- a/src/server.rs +++ b/src/server.rs @@ -30,6 +30,11 @@ pub struct TlsServerConfig { impl TlsServerConfig { /// Build a server config from a [`TlsIdentity`] and ALPN list. /// ACME identities spawn a background cert-renewal task. + /// + /// ALPN asymmetry: ACME identities always serve `acme-tls/1`, so it + /// is appended to the caller's list here (idempotently — a caller + /// who already includes it gets a single entry); non-ACME identities + /// use the ALPN list verbatim. pub async fn new(tls_identity: &TlsIdentity, alpns: &[Vec]) -> Result { match tls_identity { TlsIdentity::Acme { @@ -72,6 +77,12 @@ impl TlsServerConfig { use rustls_acme::caches::DirCache; use rustls_acme::{AcmeConfig, EventError, EventOk}; + if domains.is_empty() { + return Err(TlsError::AcmeConfig( + "TlsIdentity::Acme requires a non-empty domain list".to_string(), + )); + } + let acme_config = AcmeConfig::new(domains.to_vec()) .cache(DirCache::new(cache_dir.to_path_buf())) .directory(directory.url()) @@ -88,7 +99,9 @@ impl TlsServerConfig { config.max_early_data_size = u32::MAX; let mut alpn = alpns.to_vec(); - alpn.push(b"acme-tls/1".to_vec()); + if !alpn.contains(&b"acme-tls/1".to_vec()) { + alpn.push(b"acme-tls/1".to_vec()); + } config.alpn_protocols = alpn; let domains_owned: Vec = domains.to_vec(); @@ -755,6 +768,49 @@ mod tests { ); } + #[cfg(feature = "acme")] + #[tokio::test] + async fn new_acme_caller_supplied_acme_tls_alpn_is_not_duplicated() { + let dir = tempfile::tempdir().unwrap(); + let identity = TlsIdentity::Acme { + domains: vec!["localhost".to_string()], + cache_dir: dir.path().join("cache"), + directory: crate::identity::AcmeDirectory::Custom( + "http://127.0.0.1:9/directory".to_string(), + ), + contact: vec!["mailto:dev@example.com".to_string()], + }; + let alpn = vec![b"alktls/test".to_vec(), b"acme-tls/1".to_vec()]; + let setup = TlsServerConfig::new(&identity, &alpn) + .await + .expect("ACME config builds without awaiting the order"); + assert_eq!( + setup.rustls_config().alpn_protocols, + vec![b"alktls/test".to_vec(), b"acme-tls/1".to_vec()], + "a caller-supplied acme-tls/1 must yield exactly one entry" + ); + } + + #[cfg(feature = "acme")] + #[tokio::test] + async fn new_acme_empty_domains_returns_config_error() { + let dir = tempfile::tempdir().unwrap(); + let identity = TlsIdentity::Acme { + domains: vec![], + cache_dir: dir.path().join("cache"), + directory: crate::identity::AcmeDirectory::Staging, + contact: vec!["mailto:dev@example.com".to_string()], + }; + let err = match TlsServerConfig::new(&identity, &[b"alktls/test".to_vec()]).await { + Ok(_) => panic!("empty domain list must not construct an ACME config"), + Err(e) => e, + }; + assert!( + matches!(err, TlsError::AcmeConfig(_)), + "empty domains must surface as TlsError::AcmeConfig, got {err:?}" + ); + } + #[cfg(not(feature = "acme"))] #[tokio::test] async fn new_acme_identity_without_feature_returns_config_error() { diff --git a/tasks/config-validation-and-trivia.md b/tasks/config-validation-and-trivia.md index 89b1386..952e220 100644 --- a/tasks/config-validation-and-trivia.md +++ b/tasks/config-validation-and-trivia.md @@ -1,7 +1,7 @@ --- id: config-validation-and-trivia name: Config robustness + trivia batch — ALPN dedup, empty-domains validation, packaging excludes, https doc line (C-2, C-3, N-6, N-7) -status: pending +status: completed depends_on: [] scope: narrow risk: low @@ -96,8 +96,47 @@ session): ## Notes -> Agent fills this during implementation. +- 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 -> Agent fills this on completion. \ No newline at end of file +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. \ No newline at end of file