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.
This commit is contained in:
@@ -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.
|
||||
@@ -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.
|
||||
@@ -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.
|
||||
@@ -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:<lowercase hex>` / `SHA256:<lowercase hex>`); 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.
|
||||
@@ -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.
|
||||
@@ -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.
|
||||
Reference in New Issue
Block a user