From d7db6b17a685b79a13e6c17271d9f09ea1776a9e Mon Sep 17 00:00:00 2001 From: "glm-5.3-flash" Date: Thu, 10 Sep 2026 21:16:13 +0000 Subject: [PATCH] review-001 decomposition: 6 remediation tasks from verified findings Verify every review-001 finding before decomposition: - S-1 re-confirmed by fresh executable probe (X.509 + raw-key impersonation both complete the handshake with the victim's fingerprint extracted server-side) - U-2/U-3 uncovered-line inventory re-derived from cargo llvm-cov (--all-features); matches Part C exactly - rcgen 1975/4096 defaults, rustls cert-type negotiation arm, packaging list, doc texts: all verified against sources Tasks (all verified, none speculative): - fix-accept-any-cert-verifier-posture (S-1 + N-1 + OQ-TLS-09 + probe) - handshake-tests (U-3 suites 1-3; suite 4 lives in the S-1 task) - coverage-cheap-closes (U-2's seven groups) - acme-event-loop-test (U-1; depends on coverage-cheap-closes for the tracing-capture pattern) - config-validation-and-trivia (C-2 ALPN dedup, C-3 empty-domains, N-6 excludes, N-7 https doc line) - docs-pin-c1-c4-n3-n4 (C-1, C-4, N-3 decision note, N-4 negotiation note) Graph: acme-event-loop-test is generation 2; the rest run in generation 1. taskgraph validate: 14 tasks, no cycles. Verification: cargo test (default + --all-features) green before and after; probe file deleted after its run. --- tasks/acme-event-loop-test.md | 81 ++++++++++++ tasks/config-validation-and-trivia.md | 94 ++++++++++++++ tasks/coverage-cheap-closes.md | 97 +++++++++++++++ tasks/docs-pin-c1-c4-n3-n4.md | 101 +++++++++++++++ tasks/fix-accept-any-cert-verifier-posture.md | 116 ++++++++++++++++++ tasks/handshake-tests.md | 91 ++++++++++++++ 6 files changed, 580 insertions(+) create mode 100644 tasks/acme-event-loop-test.md create mode 100644 tasks/config-validation-and-trivia.md create mode 100644 tasks/coverage-cheap-closes.md create mode 100644 tasks/docs-pin-c1-c4-n3-n4.md create mode 100644 tasks/fix-accept-any-cert-verifier-posture.md create mode 100644 tasks/handshake-tests.md diff --git a/tasks/acme-event-loop-test.md b/tasks/acme-event-loop-test.md new file mode 100644 index 0000000..80daf5c --- /dev/null +++ b/tasks/acme-event-loop-test.md @@ -0,0 +1,81 @@ +--- +id: acme-event-loop-test +name: ACME event-loop coverage — fake-directory integration test (U-1) +status: pending +depends_on: [coverage-cheap-closes] +scope: moderate +risk: medium +impact: component +level: implementation +tags: [tests, acme, review-001, u1] +--- + +## Description + +The ACME event-loop body (src/server.rs:94-133) is unreachable by +tests: 23 uncovered lines — every `EventOk`/`EventError` arm, the +`debug`/`warn`/`error` mapping, and the "state machine ended" log are +dead code as far as the suite can prove. The `acme_lifecycle` tests +construct the config and assert spawn + ALPN + resolver wiring; the +spawned task runs against a blackhole URL and its events are never +observed. A refactor that drops or mislevels an arm (e.g. +`EventError::Order` warn → error) lands green; the loop's exit +condition (`state.next() == None` → "ACME: state machine ended") is +untested — if upstream changes the stream's termination semantics, +nothing notices. + +The acme feature's only runtime surface is this loop; it deserves one +real integration test. + +## Work + +1. Drive a `DirCache`-backed `AcmeState` against a local fake + directory: a stub HTTP server (tokio, `std::net::TcpListener` on an + ephemeral port — no new deps) that serves a directory JSON with no + usable endpoints, forcing the error path through the *real* event + stream. +2. Assert the log events fire. Two shapes, pick one (or both): + - `tracing` test subscriber capturing the `warn!`/`error!` events + (tracing-subscriber with a test layer — add as dev-dependency + only), or + - extract the event-mapping match into a helper fn taking the + event, returning (level, message-class), and test the helper + directly (no subscriber needed; cheaper, but the loop body + itself stays uncovered — prefer the subscriber shape if the dev + dep is acceptable). +3. Assert termination: with the fake directory erroring out, the + spawned task's `JoinHandle` resolves (the loop ends when the + stream ends) — pins the exit condition. +4. Keep `TlsError` out of it: per ADR-002/ADR-006, ACME runtime errors + are stream events, not error variants — the test asserts events, + never a `TlsError`. + +## Verification + +- [ ] server.rs 94-133 covered under `cargo llvm-cov --all-features` +- [ ] The test performs no real network I/O (binds localhost only) +- [ ] `cargo test --features acme`, `--all-features` green; default + build unaffected +- [ ] clippy/fmt/doc green + +## Acceptance Criteria + +- [ ] Every `EventOk`/`EventError` arm is executed by a test +- [ ] The "state machine ended" termination path is asserted +- [ ] The ACME feature's runtime surface is no longer + refactor-fragile + +## References + +- docs/reviews/001-implementation-review.md §U-1, Part C +- src/server.rs:61-139 (`new_acme` + the spawned loop) +- tests/acme_lifecycle.rs (the existing construction-level tests) +- ADR-006 (acme feature layout), ADR-002 (`TlsError` scope boundary) + +## Notes + +> Agent fills this during implementation. + +## Summary + +> Agent fills this on completion. \ No newline at end of file diff --git a/tasks/config-validation-and-trivia.md b/tasks/config-validation-and-trivia.md new file mode 100644 index 0000000..cbae287 --- /dev/null +++ b/tasks/config-validation-and-trivia.md @@ -0,0 +1,94 @@ +--- +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 +depends_on: [] +scope: narrow +risk: low +impact: component +level: implementation +tags: [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:87-89); 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. +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:87-89 (ALPN append), src/server.rs:62-76 + (`new_acme` — where the domains validation goes), + src/identity.rs:64-66 (`AcmeDirectory::Custom`), Cargo.toml:11 + (exclude list) + +## Notes + +> Agent fills this during implementation. + +## Summary + +> Agent fills this on completion. \ No newline at end of file diff --git a/tasks/coverage-cheap-closes.md b/tasks/coverage-cheap-closes.md new file mode 100644 index 0000000..635bf33 --- /dev/null +++ b/tasks/coverage-cheap-closes.md @@ -0,0 +1,97 @@ +--- +id: coverage-cheap-closes +name: Cheap coverage closes — resolver resolve() calls, non-Ed25519 pin arm, PEM parse-error arm, fallback seam (U-2) +status: pending +depends_on: [] +scope: narrow +risk: low +impact: component +level: implementation +tags: [tests, coverage, review-001, u2] +--- + +## Description + +The coverage inventory (review 001 Part C, re-verified in the +decomposition session — llvm-cov matches every listed range) shows +several small, cheap-to-close gaps. One pass over unit + integration +tests closes them all. Per group: + +1. **client.rs 183-189, 212-218** — `RawKeyClientCertResolver::resolve` + and `NoClientCertResolver::resolve` are never *called* by a test. + One-line tests: `resolve` returns `Some(key)` for the raw-key + resolver and `None` for `NoClientCertResolver`. +2. **client.rs 297, 316-322** — `FingerprintPinVerifier`'s + non-Ed25519 TLS 1.3 signature arm + (`rustls::crypto::verify_tls13_signature`) is only covered by the + deleted review probes. Pinned test: an ECDSA-P256 rcgen cert + + `dss_with_scheme` (P256/SHA256), assert ok, then a forged sig → + err. Mirrors the existing Ed25519 routing pin + (`fingerprint_pin_verifier_routes_ed25519_spki_tls13_signature_through_raw_key_path`). +3. **pem.rs 30** — the `Err(e) => Err(io::Error::other(e))` arm of + `load_private_key` (a *parse* failure, distinct from "no key + found") has no test: garbage-but-keyed file + (`b"-----BEGIN PRIVATE KEY-----\n!!!\n-----END PRIVATE KEY-----\n"`) + exercises it. +4. **client.rs 144-147** — the webpki-roots fallback *push* loop is + covered only nondeterministically (passes vacuously when the + platform store is non-empty). Deterministic remediation: a + `#[cfg(test)]`-visible helper taking the "native certs" as a + parameter (or an injectable `load_native_certs` seam) so the + empty-platform case is testable without root. This is a + load-bearing invariant whose fallback branch has no deterministic + test. +5. **server.rs 341-346** — `RawKeyCertResolver::resolve` is never + called (only `only_raw_public_keys()` is). A one-line assert + (`resolve(hello).is_some()`) covers it — a `ClientHello` can be + synthesized via `rustls::server::test_client_hello`-style helpers; + the end-to-end raw-key handshake in `handshake-tests` also covers + it, but the one-liner keeps this task independent of that one. +6. **server.rs 287-303** — `AcceptAnyCertVerifier`'s two + signature-assertion methods are never called by a test. Two-line + test each (call, assert `Ok`), making the no-pop posture explicit + in the suite. Note: this pins the *current* behavior; if + OQ-TLS-09's resolution changes the verifier, these tests change + with it (coordinate with `fix-accept-any-cert-verifier-posture`). +7. **fingerprint.rs 67** — the second disjunct + (`len() != 33 || [0] != 0x00`) matrix case (34-byte bit-string + + unused-bits ≠ 0 vs len ≠ 33). Cosmetic. + +## Work + +1. Write the unit tests (in-module `#[cfg(test)]` where the items are + private-visible, tests/ where public API suffices). +2. Group (4) is the only one touching non-test code: extract a + test-visible seam for the fallback loop. Keep the seam + `#[cfg(test)]`-visible or behind a plain `pub(crate)` fn — do not + grow the public API. +3. Run llvm-cov and confirm the ranges close. + +## Verification + +- [ ] `cargo llvm-cov --all-features` shows the seven groups covered + (client.rs 183-189/212-218/297/316-322, pem.rs 30, server.rs + 287-303/341-346, fingerprint.rs 67) +- [ ] The fallback test deterministically exercises the push loop + (platform-store-independent) +- [ ] `cargo test`, `cargo test --all-features`, clippy, fmt green + +## Acceptance Criteria + +- [ ] Line coverage ≥ 98% (from 95.32%) with every load-bearing + uncovered group closed +- [ ] No public-API growth + +## References + +- docs/reviews/001-implementation-review.md §U-2, Part C (the inventory) +- src/client.rs, src/pem.rs, src/server.rs, src/fingerprint.rs +- tasks/handshake-tests.md (the overlap note for group 5) + +## Notes + +> Agent fills this during implementation. + +## Summary + +> Agent fills this on completion. \ No newline at end of file diff --git a/tasks/docs-pin-c1-c4-n3-n4.md b/tasks/docs-pin-c1-c4-n3-n4.md new file mode 100644 index 0000000..9e56048 --- /dev/null +++ b/tasks/docs-pin-c1-c4-n3-n4.md @@ -0,0 +1,101 @@ +--- +id: docs-pin-c1-c4-n3-n4 +name: Docs + pin batch — self-signed validity, fingerprint pin format-exactness, Clone-under-no-acme, cert-type negotiation note (C-1, C-4, N-3, N-4) +status: pending +depends_on: [] +scope: narrow +risk: low +impact: component +level: implementation +tags: [docs, review-001, c1, c4, n3, n4] +--- + +## Description + +Four doc/doc+pin findings that close as rustdoc (plus one decided +question): + +1. **C-1** — `generate_self_signed_cert()` produces a cert valid + 1975→4096 (rcgen's `CertificateParams::default()` — verified + against rcgen 0.13.2 source, certificate.rs:94-96) and has no SANs. + "Dev cert" silently meaning "never expires" is a documented-behavior + gap. One doc line on `generate_self_signed_cert` (and + `SelfSignedCert`): the generated cert never expires in practice, + carries no SANs, and pins against CA verification by design; if a + tighter validity is ever wanted, `not_before`/`not_after` are + additive params. +2. **C-4** — `RemoteIdentity::fingerprint` accepts any string and the + pin comparison is exact-string (src/client.rs:272): uppercase-hex + pins, `sha256:` lowercase prefix, and cross-prefix pins + (`SHA256:`-of-an-ed25519-remote) construct fine and reject at + handshake — fail-closed, never a downgrade, but a config-author + trap. Doc line on `RemoteIdentity::fingerprint` (and/or + `FingerprintPinVerifier`): pins must be produced by + `fingerprint_from_cert_der` — case- and format-exact + (`ed25519:` / `SHA256:`); malformed + pins fail closed at handshake. +3. **N-3** — `TlsServerConfig` is not `Clone` even under + `default = []` where the struct has no `JoinHandle` field. The + all-features posture is what the API freeze pins (conditional + `Clone` is its own trap: an API difference between feature + configurations). Decision: keep non-Clone for v1; record the + reasoning in the type's doc (one sentence) and close the question + — revisit only with a concrete consumer demanding it. +4. **N-4** — the cert-type negotiation interaction needs an explicit + doc note on `AcceptAnyCertVerifier`: `requires_raw_public_keys()` + stays `false` (both cert types accepted), and — the subtle part, + verified against rustls 0.23.44 `validate_client_cert_type_extension` + in the decomposition session — **a client offering ONLY + `RawPublicKey` fails the handshake** with + `IncorrectCertificateTypeExtension`; it must offer both + `[X509, RawPublicKey]` (or the verifier would have to set + `requires_raw_public_keys() == true`). The extracted alknet client + resolver offers both types, which is why production works. Without + the note, the next reader may "fix" the verifier to `true` and + break X.509 clients, or mis-diagnose the raw-only-client failure as + a bug. + +## Work + +1. C-1 doc lines (`generate_self_signed_cert` + `SelfSignedCert`). +2. C-4 doc lines (`RemoteIdentity::fingerprint`, + `FingerprintPinVerifier` — format-exactness + fail-closed). +3. N-3: the one-sentence decision note on `TlsServerConfig`'s doc; + close N-3 in the review's finding list (no code change). +4. N-4: the negotiation note on `AcceptAnyCertVerifier`'s doc. If + desired, add a pinned unit test asserting + `requires_raw_public_keys() == false` stays default (it's a trait + default — the test just documents the choice). +5. Cross-check the review's finding numbering so each doc change + cites its finding. + +## Verification + +- [ ] All four doc notes exist and render (`cargo doc --no-deps` + warning-free) +- [ ] The N-3 decision is recorded (doc + this task's Summary) +- [ ] No code-behavior change beyond an optional unit pin +- [ ] `cargo test`, clippy, fmt, doc green + +## Acceptance Criteria + +- [ ] A config author reading the rustdoc cannot mis-case a pin, pin + the wrong prefix, or expect a never-expiring dev cert to expire +- [ ] The raw-only-client negotiation trap is documented before a + consumer hits it + +## References + +- docs/reviews/001-implementation-review.md §C-1, §C-4, §N-3, §N-4 +- src/server.rs:226-256, src/client.rs:225-236, src/credentials.rs:35-39, + src/identity.rs (SelfSigned doc) +- rustls 0.23.44 `server/hs.rs` `validate_client_cert_type_extension` + (the negotiation table N-4 documents) + +## Notes + +> Agent fills this during implementation. + +## Summary + +> Agent fills this on completion. \ No newline at end of file diff --git a/tasks/fix-accept-any-cert-verifier-posture.md b/tasks/fix-accept-any-cert-verifier-posture.md new file mode 100644 index 0000000..21ffd7c --- /dev/null +++ b/tasks/fix-accept-any-cert-verifier-posture.md @@ -0,0 +1,116 @@ +--- +id: fix-accept-any-cert-verifier-posture +name: S-1 remediation — AcceptAnyCertVerifier no-pop posture (doc + ADR/OQ + permanent probe) +status: pending +depends_on: [] +scope: narrow +risk: medium +impact: project +level: implementation +tags: [security, docs, review-001, s1] +--- + +## Description + +The `AcceptAnyCertVerifier` (src/server.rs:257-318) never checks the +client's CertificateVerify signature — no proof-of-possession — so any +party that sees a peer's public cert/SPKI can complete a handshake *as +that peer*, and the server hands the auth layer the victim's fingerprint. +Confirmed by executable probe during review 001 and **re-verified by a +fresh probe in the 001-decomposition session** (both X.509 and RFC 7250 +raw-key variants complete the handshake with the victim's cert + the +attacker's signing key; `peer_certificates()` yields the victim's cert +and the server-extracted fingerprint equals the victim's). + +The verifier behavior itself is the alknet-inherited design (alknet +ADR-034) and is behavior-preserving — this is a **documentation/posture** +finding, not a code-change mandate. What's missing: + +1. The load-bearing doc on `AcceptAnyCertVerifier` (src/server.rs:242-256) + states "does not verify the presented cert against a CA" but nowhere + states the signature is unverified. To a consumer that reads as the + *only* gap, when the actual consequence is: **presented identity is + spoofable by anyone with the public bytes**. +2. No ADR/OQ records which layer owns proof-of-possession on the server + path. +3. The spoofable posture has no permanent test pinning it — a refactor + that *adds* pop (or upstream changes the verifier contract) would + land green, silently changing the documented behavior. + +## Work + +1. **Doc on `AcceptAnyCertVerifier`**: state explicitly that the + client's CertificateVerify signature is not verified (no + proof-of-possession), that consequently the fingerprint the server + extracts is attacker-suppliable from observed peer fingerprints, and + point at the two safe patterns the auth layer can own: + - challenge-response over the established channel bound to the + cert's public key, or + - a server-side verifier that *does* verify the CertificateVerify + (the same routing the client-side `FingerprintPinVerifier` + implements — the crate has the code, just not on this type). +2. **N-1 companion line on `FingerprintPinVerifier`** + (src/client.rs:232-236): fix the "stolen-but-stale fingerprint" + phrasing (the cert is presented fresh each time — the real threat is + a stolen *private key*) and add the cross-reference: this verifier + checks proof-of-possession; the server-side `AcceptAnyCertVerifier` + does not. +3. **Record OQ-TLS-09** in `docs/architecture/open-questions.md`: + which layer owns proof-of-possession on the server path? Options: + (a) keep request-but-don't-require + auth-layer + challenge-response; (b) additive `VerifyPresentedCertVerifier` + sibling (request-and-verify — same nine-scheme list, permissive + `verify_client_cert`, signature methods delegate to + `rustls::crypto::verify_tls{12,13}_signature(_with_raw_key)`); + (c) make the auth layer's fingerprint resolution possession-checked. + Until decided, the S-1 doc note is the honest description. If + option (b) is chosen, it must land before the first consumer. +4. **Permanent behavior-pin test** (tests/, tcp-feature-gated): the + impersonation probe as a documented test — server config via + `build_rustls_server_config`, a client presenting the victim's cert + bytes with an attacker signer completes the handshake and + `peer_certificates()` yields the victim's fingerprint. Assert the + spoofable posture in both directions (X.509 + raw-key SPKI). If a + future change adds pop, this test fails and forces the doc/ADR + update. Probe shape: rustls `ServerConnection`/`ClientConnection` + pair over a duplex, `complete_io` on each — the + 001-decomposition session's probe (deleted after its run) is the + reference; a client-side test-local accept-anything verifier keeps + the probe independent of the pin path. + +## Verification + +- [ ] The doc note exists on `AcceptAnyCertVerifier` and states the + no-pop consequence in one reading +- [ ] The N-1 companion line exists on `FingerprintPinVerifier` +- [ ] OQ-TLS-09 recorded with status `open` and the three options +- [ ] The permanent probe test passes and asserts the spoofable + fingerprint end-to-end (both cert types) +- [ ] `cargo test`, `cargo clippy --all-targets -- -D warnings`, + `cargo fmt --check`, `cargo doc --no-deps` green +- [ ] `cargo test --all-features` green (the probe is tcp-gated) + +## Acceptance Criteria + +- [ ] A consumer reading only the rustdoc understands that presented + identity is spoofable without caller-enforced possession +- [ ] The spoofable posture is pinned by an executed test, not just + prose +- [ ] The ownership question is tracked (OQ-TLS-09) — not silently + inherited + +## References + +- docs/reviews/001-implementation-review.md §S-1 (the finding) and §N-1 +- src/server.rs:242-318 (`AcceptAnyCertVerifier`), src/client.rs:225-323 + (`FingerprintPinVerifier` — the pop-complete routing to mirror) +- alknet ADR-034 (the inherited request-but-don't-require shape) +- docs/architecture/open-questions.md (where OQ-TLS-09 lands) + +## Notes + +> Agent fills this during implementation. + +## Summary + +> Agent fills this on completion. \ No newline at end of file diff --git a/tasks/handshake-tests.md b/tasks/handshake-tests.md new file mode 100644 index 0000000..c57a68a --- /dev/null +++ b/tasks/handshake-tests.md @@ -0,0 +1,91 @@ +--- +id: handshake-tests +name: Handshake-level test suite — pin, fail-closed, raw-key path executed (U-3) +status: pending +depends_on: [] +scope: moderate +risk: medium +impact: component +level: implementation +tags: [tests, tcp, review-001, u3] +--- + +## Description + +Every existing test asserts config *construction*; the crate's actual +TLS behavior (server accepts, pin verifies, fail-closed manifests) is +never executed — no handshake-level test exists. These are the +highest-value missing tests in the crate: they turn "fail closed" and +"pin" from documentation into executed behavior. The ADR-006 boundary +(handshakes belong to transport crates) is not violated by a +duplex-pair handshake: no external transport dep, just tokio + +tokio-rustls under the existing `tcp` feature (dev/test only — the +feature gate already exists for `for_tcp_tls`). + +The four handshake-level gaps (review 001 §U-3): + +1. **The pin path end-to-end**: client config with + `FingerprintPinVerifier` handshaking a server whose presentation + matches the pin → ok; mismatched pin → fails. +2. **The fail-closed path**: `remote_identity: None` + raw-key server + → handshake fails (the structural claim, executed). +3. **The RFC 7250 raw-key server path end-to-end**: `RawKeyCertResolver` + + client → handshake completes with the raw-key cert type negotiated + (currently only `only_raw_public_keys()` is asserted). +4. **S-1's impersonation probe made permanent** — covered by + `fix-accept-any-cert-verifier-posture` (kept separate: different + remediation owner, doc+ADR+probe). + +## Work + +1. Add `tests/handshake_behavior.rs` (`#![cfg(feature = "tcp")]`): + tokio duplex pair + `for_tcp_tls()` acceptor / `into_rustls_config()` + connector — or raw `ServerConnection`/`ClientConnection` with + `complete_io` (the review-probe shape; fewer moving parts). +2. Suite (1): server = `TlsIdentity::RawKey(sk)`, client pin = + `fingerprint_from_cert_der(spki)` → handshake ok + app-data + round-trip; wrong pin → error. +3. Suite (2): server = RawKey, credentials without `remote_identity` → + handshake fails (`UnknownCertificateType`/alert — assert failure, + not the specific error text unless rustls pins it). +4. Suite (3): raw-key server + raw-key client (RFC 7250 both sides): + handshake ok; server `peer_certificates()` is an SPKI; client + presented X509+RawPublicKey cert types per N-4's correction (the + client resolver already offers the right shape — see N-4). +5. Keep every test within the crate's public API + rustls types; no new + dependencies. + +## Verification + +- [ ] All three executed handshake tests pass under + `cargo test --features tcp` (and `--all-features`) +- [ ] The fail-closed test actually fails the handshake (assert error, + not success) +- [ ] `cargo test` (default) still green — the file is feature-gated + and contributes nothing without `tcp` +- [ ] `cargo clippy --all-targets --all-features -- -D warnings`, + `cargo fmt --check` green + +## Acceptance Criteria + +- [ ] Pin / fail-closed / raw-key are executed behaviors in the suite, + not just structural assertions +- [ ] No new dependencies; the default build stays lean + +## References + +- docs/reviews/001-implementation-review.md §U-3, §N-4 (the cert-type + negotiation correction — read before writing suite (3)) +- src/client.rs (`FingerprintPinVerifier`, `select_server_verifier`), + src/server.rs (`RawKeyCertResolver`) +- ADR-006 (the handshake scope boundary — and why the duplex-pair shape + stays inside it) +- tasks/integration-suite.md (the existing suite this extends) + +## Notes + +> Agent fills this during implementation. + +## Summary + +> Agent fills this on completion. \ No newline at end of file