re-baseline the four pending remediation tasks against the ADR-007/008 tree
The tasks were decomposed (d7db6b1) before 49d4432/ac440f3 landed;
both commits touched exactly the areas the tasks reference. Fixes
grounded in verified sources (vendored rustls-acme 0.12.1, rustls-pemfile
2.2.0, rustls-native-certs 0.8.4) and a fresh cargo llvm-cov run:
- acme-event-loop-test: the termination assertion was impossible —
rustls-acme's Stream for AcmeState never yields None
(state.rs:407-412, poll_next_infinite + 2^16s backoff); the review's
U-1 exit-condition premise is withdrawn and corrected in place.
Replaced with timeout-bounded event collection, a reachable-arm
inventory (Order warn, AccountCacheStore, Load/Parse error arms,
DeployedCachedCert/CertCacheStore via deterministic DirCache file
pre-seeding), and an explicit mark for the full-fake-CA arms.
server.rs:135 flagged as unreachable dead code (delete or accept).
- coverage-cheap-closes: re-baselined per-line ground truth — original
groups 1 and 5 are already closed by the handshake suites; group 2's
TLS 1.3 half is covered, leaving the TLS 1.2 else-arm (client.rs:316);
new group added for VerifyPresentedCertVerifier::verify_tls12_signature
(server.rs:349-366, opened by ADR-008; required for the >=98% bar);
AcceptAnyCertVerifier refs moved to server.rs:468-475 with the stale
OQ-TLS-09 coordination caveat retired.
- docs-pin-c1-c4-n3-n4: N-4 re-scoped (the mechanism analysis already
lives in ADR-007 + the resolver doc block; what remains is a short
server-verifier note covering both verifiers); C-4 updated for
ADR-007's negotiation-earlier failure point; added the
FingerprintPinVerifier pop cross-ref update (post-ADR-008 the default
verifier does verify possession).
- config-validation-and-trivia: added the feature-gate mechanics note
for the C-3 test (a non-gated test passes vacuously under default
features); refreshed drifted line refs with a re-grep advisory.
- review 001: Status block records OQ-TLS-09/-10 resolutions; U-1
carries the termination correction; U-2 carries the supersession
note. ADR-008 gains the suite-number-to-test-name mapping.
Verification: taskgraph validate 14 tasks; cargo doc --no-deps
warning-free; all edits docs-only (no code paths touched).
This commit is contained in:
@@ -12,17 +12,36 @@ tags: [tests, acme, review-001, u1]
|
||||
|
||||
## Description
|
||||
|
||||
The ACME event-loop body (src/server.rs:94-133) is unreachable by
|
||||
The ACME event-loop body (src/server.rs:94-136) 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.
|
||||
`debug`/`warn`/`error` mapping. 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.
|
||||
|
||||
**Correction to this task (2026-09-12, verified against the vendored
|
||||
rustls-acme 0.12.1 sources) — the review's §U-1 termination premise is
|
||||
wrong, and so was this task's original work item 3.** `rustls-acme
|
||||
0.12.1`'s `Stream for AcmeState` never terminates:
|
||||
`poll_next` is `Poll::Ready(Some(ready!(self.poll_next_infinite(cx))))`
|
||||
(state.rs:407-412) — it can never yield `None`. Order errors enter an
|
||||
exponential backoff (`Timer::after(1 << backoff_cnt)`, capped at 2^16
|
||||
seconds — state.rs:371-375) and retry forever. Two consequences:
|
||||
|
||||
1. **There is no "loop ends when the stream ends"** — the
|
||||
`while let Some(event) = state.next().await` loop in `new_acme`
|
||||
never exits on its own. Do NOT assert that the `JoinHandle`
|
||||
resolves; such a test hangs until timeout. The original item 3 is
|
||||
removed.
|
||||
2. **`src/server.rs:135` ("ACME: state machine ended") is unreachable
|
||||
dead code under this dependency.** It cannot be covered by any test
|
||||
through the real stream. Either delete the line in this task (and
|
||||
note the removal) or explicitly accept it as uncovered — do not
|
||||
leave an implementer to burn hours rediscovering that it is
|
||||
unreachable. (The review's §U-1 framing "if upstream changes the
|
||||
stream's termination semantics, nothing notices" described a
|
||||
termination semantics that does not exist.)
|
||||
|
||||
The acme feature's only runtime surface is this loop; it deserves one
|
||||
real integration test.
|
||||
@@ -33,7 +52,11 @@ real integration test.
|
||||
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.
|
||||
stream. (Mechanically verified: rustls-acme's HTTP layer,
|
||||
`async_web_client` 0.6.3, is plain `async_net::TcpStream` for
|
||||
`http://` URLs and self-drives via async-io 2's reactor thread —
|
||||
it works under a tokio runtime, and a plain-HTTP localhost stub is
|
||||
reachable.)
|
||||
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
|
||||
@@ -43,16 +66,48 @@ real integration test.
|
||||
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
|
||||
3. **Bound event collection by time, never by stream end**: collect
|
||||
events under `tokio::time::timeout` (or a bounded channel + a
|
||||
drop-with-timeout teardown) and assert on what arrived. The
|
||||
spawned task never resolves its `JoinHandle` — see the correction
|
||||
above — so any "await the handle" shape hangs. Note: with
|
||||
`#[tokio::test]` (current-thread runtime) the spawned loop runs on
|
||||
the test thread, so a thread-local default tracing subscriber is
|
||||
visible to it; keep the test single-threaded or the subscriber
|
||||
captures nothing.
|
||||
4. Which arms are reachable, per rustls-acme 0.12.1's state machine
|
||||
(state.rs `poll_next_infinite` / `order`):
|
||||
- `EventError::Order` (warn) — fires against the erroring stub.
|
||||
- `EventOk::AccountCacheStore` — the first event, generated before
|
||||
any network I/O (the account key is generated and stored
|
||||
eagerly, state.rs:377-395); fires even against a dead address.
|
||||
- `EventError::AccountCacheLoad` / `CertCacheLoad` /
|
||||
`CachedCertParse` (error) and `EventOk::DeployedCachedCert` /
|
||||
`CertCacheStore` (ok) — reached by pre-seeding the `DirCache`
|
||||
directory. The cache file names are deterministic
|
||||
(`caches/dir.rs:38-53`): `cached_account_{base64url(SHA256(
|
||||
contact-els…directory_url))}` and `cached_cert_{base64url(
|
||||
SHA256(domain-els…directory_url))}`. A corrupt pre-seeded file
|
||||
gives the Load/Parse error arms; a valid PEM (an ECDSA-P256
|
||||
chain, PKCS#8 key first — `parse_cert` wants
|
||||
key-then-cert-chain, state.rs:192-214) gives the deployed/stored
|
||||
ok arms. The PEM can be generated with rcgen in-test (rcgen is
|
||||
already a dependency on the acme feature path via the server's
|
||||
self-signed helper — it is a [dependencies] member).
|
||||
- `EventOk::DeployedNewCert` / `EventError::NewCertParse` —
|
||||
reachable only through a *successful* order, i.e. a full fake
|
||||
ACME CA (newNonce/newAccount/newOrder/authorizations/finalize/
|
||||
certificate endpoints). Out of scope at this effort level; mark
|
||||
explicitly not-covered rather than silent.
|
||||
5. 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`
|
||||
- [ ] server.rs 94-136 covered under `cargo llvm-cov --all-features`
|
||||
(line 135 either deleted or explicitly accepted as uncovered,
|
||||
per work item 2's second consequence)
|
||||
- [ ] The test performs no real network I/O (binds localhost only)
|
||||
- [ ] `cargo test --features acme`, `--all-features` green; default
|
||||
build unaffected
|
||||
@@ -60,17 +115,27 @@ real integration test.
|
||||
|
||||
## Acceptance Criteria
|
||||
|
||||
- [ ] Every `EventOk`/`EventError` arm is executed by a test
|
||||
- [ ] The "state machine ended" termination path is asserted
|
||||
- [ ] Every reachable arm per work item 4 is executed by a test
|
||||
(`Order` warn, `AccountCacheStore`, the Load/Parse error arms,
|
||||
`DeployedCachedCert`/`CertCacheStore`); `DeployedNewCert`/
|
||||
`NewCertParse` are explicitly marked not-covered (full fake CA
|
||||
needed — out of scope)
|
||||
- [ ] No test awaits the event loop's `JoinHandle` (it never resolves)
|
||||
- [ ] 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)
|
||||
- docs/reviews/001-implementation-review.md §U-1, Part C (§U-1's
|
||||
termination premise is corrected — see this task's Description)
|
||||
- src/server.rs:64-145 (`new_acme` + the spawned loop)
|
||||
- tests/acme_lifecycle.rs (the existing construction-level tests)
|
||||
- ADR-006 (acme feature layout), ADR-002 (`TlsError` scope boundary)
|
||||
- rustls-acme 0.12.1 sources (vendored):
|
||||
`state.rs:407-412` (the never-ending `Stream` impl),
|
||||
`state.rs:371-375` (order-error backoff + retry),
|
||||
`state.rs:192-214` (`parse_cert` — key first, then ≥2 PEMs),
|
||||
`caches/dir.rs:38-53` (the deterministic cache file names)
|
||||
|
||||
## Notes
|
||||
|
||||
|
||||
@@ -16,7 +16,7 @@ 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`
|
||||
(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
|
||||
@@ -32,7 +32,13 @@ session):
|
||||
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.
|
||||
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
|
||||
@@ -80,10 +86,13 @@ session):
|
||||
## 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
|
||||
- src/server.rs:91 (the ALPN append), src/server.rs:64-145
|
||||
(`new_acme` — where the domains validation goes),
|
||||
src/identity.rs:64-66 (`AcmeDirectory::Custom`), Cargo.toml:11
|
||||
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
|
||||
|
||||
|
||||
@@ -1,6 +1,6 @@
|
||||
---
|
||||
id: coverage-cheap-closes
|
||||
name: Cheap coverage closes — resolver resolve() calls, non-Ed25519 pin arm, PEM parse-error arm, fallback seam (U-2)
|
||||
name: Cheap coverage closes — non-Ed25519 pin arms, PEM parse-error arm, fallback seam, escape-hatch methods (U-2, re-baselined)
|
||||
status: pending
|
||||
depends_on: []
|
||||
scope: narrow
|
||||
@@ -12,56 +12,84 @@ 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:
|
||||
The coverage inventory (review 001 Part C) — **re-baselined
|
||||
2026-09-12 by a fresh `cargo llvm-cov --all-features` run** after
|
||||
ADR-007/ADR-008 landed (commits `49d4432`/`ac440f3`): the new
|
||||
handshake and impersonation suites already closed two of the original
|
||||
groups, and ADR-008's new verifier opened one new gap. Current
|
||||
per-line ground truth (all line numbers re-verified against the
|
||||
current tree):
|
||||
|
||||
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
|
||||
1. **client.rs:316** — `FingerprintPinVerifier::
|
||||
verify_tls12_signature`'s non-Ed25519 else-arm
|
||||
(`rustls::crypto::verify_tls12_signature`) is never executed. (The
|
||||
original group-2 text pointed at the TLS 1.3 twin at 297/316-322 —
|
||||
that TLS 1.3 arm is now covered by the handshake suite's pin
|
||||
suites; only the TLS 1.2 else-arm remains.) Pinned test: an
|
||||
ECDSA-P256 rcgen cert + `dss_with_scheme` (P256/SHA256) against
|
||||
`verify_tls12_signature`, assert ok, then a forged sig → err.
|
||||
Mirrors the existing Ed25519 TLS 1.3 routing pin
|
||||
(`fingerprint_pin_verifier_routes_ed25519_spki_tls13_signature_
|
||||
through_raw_key_path`, client.rs:472).
|
||||
2. **server.rs:349-366** — `VerifyPresentedCertVerifier::
|
||||
verify_tls12_signature`'s body (ADR-008's default verifier; did
|
||||
not exist at decomposition time) is entirely uncovered: no TLS 1.2
|
||||
handshake exists in the suite. Same pop-routing shape as the pin
|
||||
verifier (Ed25519-SPKI → `verify_tls13_signature_with_raw_key`,
|
||||
else `verify_tls12_signature`). A direct unit-call test (Ed25519
|
||||
SPKI + valid sig → ok; wrong-key or mismatched-message dss → err)
|
||||
closes it. **This group must be included or the ≥98% acceptance
|
||||
bar is unreachable while the ACME loop stays open** (owned by
|
||||
`acme-event-loop-test`).
|
||||
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
|
||||
exercises it. Verified against rustls-pemfile 2.2.0: malformed
|
||||
base64 in a keyed section is a real `Error::Base64Decoding`-class
|
||||
`Err` from `private_key` — the prescribed input works.
|
||||
4. **client.rs:145-148** — 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
|
||||
test. (`rustls_native_certs::CertificateResult` has public `certs`
|
||||
/ `errors` fields, so the seam shape is a plain refactor — see
|
||||
Work item 2.) The `tracing::warn!` for native-cert errors at
|
||||
client.rs:139 sits in the same uncovered pocket.
|
||||
5. **server.rs:468-475** — `AcceptAnyCertVerifier` (now the
|
||||
*escape-hatch* verifier post-ADR-008, moved to server.rs:429) is
|
||||
only partially covered by the impersonation suite's escape-hatch
|
||||
tests: `verify_tls12_signature`'s body and
|
||||
`supported_verify_schemes()` (468-475) are never called. Two-line
|
||||
test each (call, assert `Ok` / assert the nine-scheme list),
|
||||
making the no-pop posture explicit in the suite. (The original
|
||||
group-6 range 287-303 is now `VerifyPresentedCertVerifier`'s code
|
||||
— the file shifted under the ADR-008 commit.) Note: the original
|
||||
"coordinate with OQ-TLS-09's resolution" caveat is stale —
|
||||
OQ-TLS-09 is resolved (ADR-008), the escape-hatch posture is
|
||||
permanent and pinned by `tests/impersonation_posture.rs`.
|
||||
6. **fingerprint.rs:67** — the second disjunct
|
||||
(`len() != 33 || [0] != 0x00`) matrix case (34-byte bit-string +
|
||||
unused-bits ≠ 0 vs len ≠ 33). Cosmetic.
|
||||
|
||||
**Already closed by the ADR-007/ADR-008 suites (do not redo):** the
|
||||
client resolvers' `resolve` calls (original group 1 —
|
||||
`RawKeyClientCertResolver::resolve` and `NoClientCertResolver::resolve`
|
||||
are exercised by the handshake suites) and `RawKeyCertResolver::resolve`
|
||||
(original group 5 — exercised end-to-end by
|
||||
`tests/handshake_behavior.rs`, e.g.
|
||||
`raw_key_client_presents_spki_and_server_extracts_fingerprint`).
|
||||
`FingerprintPinVerifier::verify_tls13_signature`'s non-Ed25519 arm
|
||||
(original group 2's TLS 1.3 half) is also covered.
|
||||
|
||||
## 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
|
||||
2. Item 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.
|
||||
@@ -69,24 +97,28 @@ tests closes them all. Per group:
|
||||
|
||||
## 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)
|
||||
- [ ] `cargo llvm-cov --all-features` shows the six groups covered
|
||||
(client.rs 316/145-148, pem.rs 30, server.rs 349-366/468-475,
|
||||
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
|
||||
- [ ] Line coverage ≥ 98% (from 95.81% currently; 95.32% at
|
||||
decomposition) with every load-bearing uncovered group closed
|
||||
- [ ] No public-API growth
|
||||
|
||||
## References
|
||||
|
||||
- docs/reviews/001-implementation-review.md §U-2, Part C (the inventory)
|
||||
- docs/reviews/001-implementation-review.md §U-2, Part C (the
|
||||
inventory rows for the closed groups are stale — this task's
|
||||
Description is the current baseline)
|
||||
- src/client.rs, src/pem.rs, src/server.rs, src/fingerprint.rs
|
||||
- tasks/handshake-tests.md (the overlap note for group 5)
|
||||
- tests/handshake_behavior.rs (the suites that closed original groups
|
||||
1 and 5), tests/impersonation_posture.rs (the escape-hatch pins —
|
||||
group 5's `verify_tls13_signature` coverage comes from here)
|
||||
|
||||
## Notes
|
||||
|
||||
|
||||
@@ -25,7 +25,7 @@ question):
|
||||
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
|
||||
pin comparison is exact-string (src/client.rs:293): 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
|
||||
@@ -33,7 +33,19 @@ question):
|
||||
`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.
|
||||
pins fail closed at handshake. **Update for ADR-007 (commit
|
||||
`49d4432`): the pin format is no longer only a verification
|
||||
selector — it also selects the cert-type offer (`ed25519:` → offer
|
||||
`[RawPublicKey]`, `SHA256:` → default X.509 offer), and a
|
||||
cross-format mismatch now fails earlier, at cert-type negotiation,
|
||||
before the pin compare is even reached (pinned by
|
||||
`tests/handshake_behavior.rs`:
|
||||
`ed25519_pin_against_x509_server_fails_closed_at_negotiation`).
|
||||
The review's original C-4 mechanism ("rejected at pin comparison")
|
||||
is superseded — same fail-closed verdict, earlier failure point.
|
||||
Write the doc line from that chain: mismatched *format* fails at
|
||||
negotiation (a config error, ADR-007); same-format-but-wrong pins
|
||||
(case, garbage) construct and fail at the pin compare.**
|
||||
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
|
||||
@@ -41,53 +53,59 @@ question):
|
||||
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`. **Read the corrected mechanism
|
||||
first** — the review's §N-4 original parenthetical ("alknet's own
|
||||
client resolver offers both types, which is why production works")
|
||||
is inaccurate; the review's §Status block and §N-4 now carry the
|
||||
correction (found during the S-1 remediation, commit `e86b8ba`).
|
||||
The accurate chain, from the rustls sources (0.23.41 AND 0.23.44):
|
||||
- rustls sends `client_certificate_types = [RawPublicKey]` (only)
|
||||
iff the *resolver's* `only_raw_public_keys()` is true
|
||||
(client/hs.rs:331-334 in 0.23.41, :355-358 in 0.23.44) — no
|
||||
"offers both types" behavior exists in either version.
|
||||
- Against `AcceptAnyCertVerifier`
|
||||
(`requires_raw_public_keys() == false`), a client offering only
|
||||
`[RawPublicKey]` hits the `(false, true, false)` arm of
|
||||
`process_cert_type_extension` (server/hs.rs:274-288 in 0.23.44)
|
||||
→ `IncorrectCertificateTypeExtension` → handshake failure.
|
||||
- A client with an X.509-typed resolver (or no resolver) offers no
|
||||
cert-type extension / `[X509]` → the `(false, _, true)` or
|
||||
`(false, false, false)` arm → default X.509 → works, and the
|
||||
server passes presented bytes through unparsed to a custom
|
||||
`verify_client_cert`.
|
||||
- So `requires_raw_public_keys()` must stay `false` (do not "fix"
|
||||
it to `true` — that would break X.509 clients), and the doc note
|
||||
must state that a raw-key *client* resolver makes rustls offer
|
||||
`[RawPublicKey]` only, which this verifier rejects
|
||||
(fail-closed, not a downgrade). Note the S-1 doc on the same
|
||||
type (landed, commit `e86b8ba`) already carries the no-pop
|
||||
section; add the negotiation note as a sibling section, and note
|
||||
that a raw-key *server* presentation (`RawKeyCertResolver`,
|
||||
`only_raw_public_keys() == true` on the server cert side) is a
|
||||
different knob (the `server_certificate_types` extension) that
|
||||
is unaffected. 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.
|
||||
4. **N-4** — **re-scoped 2026-09-12: the mechanism analysis this item
|
||||
originally asked for is already documented.** ADR-007
|
||||
(`docs/architecture/decisions/007-cert-type-negotiation.md`,
|
||||
commit `49d4432`) records the whole negotiation chain, and
|
||||
`RawKeyClientCertResolver`'s doc block (src/client.rs:152-171)
|
||||
carries the client-side mechanism note (`only_raw_public_keys() ==
|
||||
false` → X.509 offer; the `(false, true, false)` arm reasoning).
|
||||
Do not re-derive or duplicate that analysis. What remains is a
|
||||
short server-verifier note (a sibling section in the
|
||||
`AcceptAnyCertVerifier` and `VerifyPresentedCertVerifier` docs, or
|
||||
one shared paragraph referenced from both):
|
||||
- `requires_raw_public_keys()` stays `false` on **both** server
|
||||
verifiers — do not "fix" it to `true` (that would reject X.509
|
||||
clients; the request-but-don't-require shape accepts both cert
|
||||
types).
|
||||
- Post-ADR-007, the crate's own `RawKeyClientCertResolver` presents
|
||||
the SPKI under the **X.509 offer** unconditionally
|
||||
(`client.rs:187`, `raw_public_keys: false`), so a raw-key client
|
||||
against this crate's servers never sends a raw-only offer. The
|
||||
`IncorrectCertificateTypeExtension` rejection of a raw-only
|
||||
client offer (`(false, true, false)` arm) can now only arise
|
||||
from a *foreign* rustls resolver that sets
|
||||
`only_raw_public_keys() == true` — state it as an interop
|
||||
boundary of the request-not-require shape, fail-closed, not a
|
||||
downgrade.
|
||||
- A raw-key *server* presentation (`RawKeyCertResolver`,
|
||||
`only_raw_public_keys() == true` on the server-cert side) is the
|
||||
other knob (`server_certificate_types`) and is unaffected.
|
||||
Without the note, the next reader may "fix" the verifier to `true`
|
||||
and break X.509 clients, or mis-diagnose a foreign 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).
|
||||
`FingerprintPinVerifier` — format-exactness + fail-closed, with
|
||||
the ADR-007 negotiation-earlier failure point per the Description).
|
||||
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
|
||||
4. N-4: the short server-verifier negotiation note per the re-scoped
|
||||
item 4 in the Description (a sibling section on
|
||||
`AcceptAnyCertVerifier` + `VerifyPresentedCertVerifier`, or one
|
||||
shared paragraph referenced from both). 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. While in these docs: `FingerprintPinVerifier`'s pop cross-ref
|
||||
(src/client.rs:253-255) currently says only "the **server-side**
|
||||
`AcceptAnyCertVerifier` does not [verify possession]" — post-ADR-008
|
||||
the *default* server verifier `VerifyPresentedCertVerifier` does;
|
||||
mention both (the default verifies, the escape hatch does not).
|
||||
6. Cross-check the review's finding numbering so each doc change
|
||||
cites its finding.
|
||||
|
||||
## Verification
|
||||
@@ -102,24 +120,34 @@ question):
|
||||
|
||||
- [ ] 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
|
||||
- [ ] The raw-only-client interop boundary is documented before a
|
||||
consumer hits it (short note per the re-scope; the full
|
||||
mechanism lives in ADR-007)
|
||||
|
||||
## References
|
||||
|
||||
- docs/reviews/001-implementation-review.md §C-1, §C-4, §N-3, §N-4
|
||||
(§N-4 — read the correction block; the original parenthetical is
|
||||
inaccurate), and the §Status block (the correction summary)
|
||||
- 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` `process_cert_type_extension`
|
||||
(server/hs.rs:263-288 — the `(requires_raw_keys, offers_raw,
|
||||
offers_x509)` negotiation table N-4 documents) and
|
||||
`client/hs.rs:350-358` (the `[RawPublicKey]`-only offer rule) —
|
||||
both pinned in rustls 0.23.41 too (client/hs.rs:326-334)
|
||||
- tests/impersonation_posture.rs (the S-1 probe — its fixed resolver
|
||||
pins `only_raw_public_keys() == false` for the X.509-typed offer;
|
||||
do not confuse that with a raw-key client resolver)
|
||||
(§N-4 — the correction blocks are historical context now; ADR-007
|
||||
is the authoritative record of the negotiation mechanism), and the
|
||||
§Status block
|
||||
- src/server.rs:223-257 (the self-signed helper + `SelfSignedCert`),
|
||||
src/server.rs:18-24 (`TlsServerConfig` — where the N-3 note goes),
|
||||
src/client.rs:253-255 (the pop cross-ref), src/credentials.rs:36-38
|
||||
(`RemoteIdentity::fingerprint`), src/identity.rs (SelfSigned doc)
|
||||
- ADR-007 (`docs/architecture/decisions/007-cert-type-negotiation.md`)
|
||||
— the authoritative negotiation record; ADR-008 (the verifier
|
||||
default change that makes `VerifyPresentedCertVerifier` part of the
|
||||
N-4 note)
|
||||
- rustls 0.23.44 `server/hs.rs::process_cert_type_extension` (the
|
||||
negotiation table) and `client/hs.rs` (the `[RawPublicKey]`-only
|
||||
offer rule) — consult only if extending the analysis beyond what
|
||||
ADR-007 records
|
||||
- tests/impersonation_posture.rs (its fixed resolver pins
|
||||
`only_raw_public_keys() == false` — post-ADR-007 that is also the
|
||||
crate resolver's unconditional shape),
|
||||
tests/handshake_behavior.rs
|
||||
(`ed25519_pin_against_x509_server_fails_closed_at_negotiation` —
|
||||
the negotiation fail-closed pin)
|
||||
|
||||
## Notes
|
||||
|
||||
|
||||
Reference in New Issue
Block a user