# Review 001 — alktls v1 Implementation (correctness, security, coverage)

## Status

**Implemented-conformant with findings (S-1 [major, security] documented
posture gap with a spoofability consequence; minors + coverage notes
logged, N-2 retracted).** Findings filed from the `review-impl`
post-port review (2026-09-10), an independent pass on top of the
already-green phase gate (`tasks/review-impl.md`, the same day).

**Remediation status (2026-09-11): S-1 (+N-1) remediated** — the
no-pop doc note is on `AcceptAnyCertVerifier`, the N-1 companion fix is
on `FingerprintPinVerifier`, OQ-TLS-09 records the ownership question
(open), and the impersonation probe is permanent
(`tests/impersonation_posture.rs`, tcp-gated, both cert types) —
commit `e86b8ba`, task
`tasks/fix-accept-any-cert-verifier-posture.md` (completed). The
remaining findings are tracked in the six remediation tasks from the
001-decomposition (commit `d7db6b1`).

**Status update (2026-09-12): OQ-TLS-09 is resolved** (ADR-008,
commit `ac440f3` — `VerifyPresentedCertVerifier` is the default
client-cert verifier on every `TlsServerConfig` path;
`AcceptAnyCertVerifier` remains public as the explicit no-pop escape
hatch, no longer installed by any crate path) and **OQ-TLS-10 is
resolved** (ADR-007, commit `49d4432` — the cert-type offer follows
the pin format / identity; `RawKeyClientCertResolver` presents the
SPKI under the X.509 offer). The pending remediation tasks
(`config-validation-and-trivia`, `docs-pin-c1-c4-n3-n4`,
`coverage-cheap-closes`, `acme-event-loop-test`) have been re-baselined
against that tree — see the correction notes in each task file and in
§U-1/§U-2 below.

**Status update (2026-09-12, docs-pin batch): C-1, C-4, N-3, N-4 are
closed** by `tasks/docs-pin-c1-c4-n3-n4.md` (completed) as rustdoc
pins: the self-signed cert's never-expires/no-SAN facts (§C-1), the
pin's case-/format-exactness with the ADR-007 negotiation-earlier
failure point (§C-4), the non-Clone decision recorded on
`TlsServerConfig` (§N-3, closed as decided), and the
request-not-require negotiation boundary on the server verifiers
(§N-4, with the pinned
`server_verifiers_keep_requires_raw_public_keys_default_false` unit
test). The review's original §C-4 mechanism ("rejected at pin
comparison") is superseded by ADR-007 for the cross-format case — the
docs are written from the corrected chain.

**One correction to this review, found during the S-1 remediation:** the
N-4 parenthetical "alknet's own client resolver offers both types" is
**inaccurate** — rustls 0.23.41 and 0.23.44 both send
`client_certificate_types = [RawPublicKey]` 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); there is no "offers both types" behavior in
either version. The raw-key impersonation probe works because the
attacker presents the SPKI under the *default* X.509 offer
(`only_raw_public_keys() == false`), which the server accepts unparsed
(`(false, _, true)` arm of `process_cert_type_extension` — server/hs.rs
0.23.44:274-288). A rustls-driven raw-key client resolver (offering
`[RawPublicKey]` only) would fail `IncorrectCertificateTypeExtension`
against this verifier — see N-4 below for the corrected mechanism, and
write the N-4 doc note from the rustls sources, not the parenthetical.

This review's focus (per the user's direction): the crate stands on
its own as the publication target — correctness, security, and test
coverage — with the alknet sources consulted only as behavior-preservation
prior art (alknet itself is being rewritten next).

**S-1 is a documentation/posture finding, not a code-change mandate** —
the verifier behavior is the alknet-inherited design (alknet ADR-034)
and is behavior-preserving; what's missing is the security analysis
that makes the threat model explicit for the auth layer that consumes
it. See the finding for the exact trust-boundary consequence and the
recommended remediation shape.

Findings use prefix `S` (security-relevant), `C` (correctness),
`U` (units — remediation-work findings), and `N` (notes/trivia). This
is the first review in this crate's series (numbering follows the
house pattern — each review numbers independently).

## Scope

The full v1 implementation at tree `8a15978` (post phase-gate) reviewed
against `docs/architecture/` (overview.md, server.md, client.md,
ADR-001..006, open-questions.md) + the AGENTS.md conventions, and
cross-checked against the extraction sources for behavior preservation:

```
Verified against: alktls 8a15978 (post phase-gate, API frozen)
Reading list: src/{lib,server,client,credentials,identity,fingerprint,
  pem,signing}.rs, tests/{acme_lifecycle,client_seams,invariant_pins,
  server_seams}.rs, docs/architecture/**, tasks/*.md
Prior art: /workspace/@alkdev/alknet/crates/alknet-tls/src/*,
  /workspace/@alkdev/alknet/crates/alknet-core/src/{config,credentials,
  fingerprint}.rs, alknet docs/architecture/crates/tls/README.md
  (+ rustls 0.23.44 / noq-proto 1.3.0 / rustls-acme 0.12.1 sources as
  needed for upstream-contract verification)
Executed: cargo test (68 default / 75 --all-features, clean both),
  clippy -D warnings (all-targets, default + all-features), fmt --check,
  cargo doc --no-deps (warning-free), cargo publish --dry-run
  --allow-dirty (passes), cargo llvm-cov --all-features (95.32% line
  coverage — uncovered-line inventory in Part C)
Probes (temporary test files, deleted after the review run):
  - fingerprint cross-prefix / case-sensitivity / garbage-pin probes
  - DER-parser adversarial-input battery (no panics)
  - empty-PEM chain → TlsError::Rustls probe
  - self-signed cert validity DER probe (1975–4096 validity found)
  - acme-tls/1 duplication probe
  - empty-domains ACME + empty-ALPN construction probes
  - client-config Debug fingerprint-leakage probe
  - server handshake probe: victim-cert + attacker-key impersonation
    (the S-1 evidence — see the finding)
```

## Severity legend

Same scale as the alktunnels review (001 in that series):

- **[critical]** — a decided spec invariant is violated in a way that
  makes a promised capability unreachable end-to-end; or corrupts data;
  or a security boundary a consumer would rely on does not actually hold.
- **[major]** — a core protocol path cannot serve a decided behavior;
  works only via shapes the spec does not describe; or a wire/ABI
  promise is not actually delivered by the code; or an undocumented
  security property materially changes the deployment risk profile.
- **[minor]** — drift, doc/spec inconsistency, or a missing
  convenience with no correctness impact.

---

# Checklist verdicts (the phase-gate five sections, re-verified)

1. **API surface == ADR-004 — PASS.** Every accessor signature matches
   the ADR sketches verbatim (`src/server.rs:30,147,156,162`;
   `src/client.rs:29,52,60`); the re-export block is the documented
   surface; no auth-layer types leaked.
2. **`TlsError` == ADR-002 — PASS.** Six variants, `#[non_exhaustive]`,
   typed `#[from]` chains, `AcmeConfig(String)` holds exactly the two
   config-mismatch cases (`src/lib.rs:51-89`). No `Config(String)`
   catch-all.
3. **Invariants == ADR-001/server.md/client.md — PASS, all seven.**
   `max_early_data_size = u32::MAX` per server path (pinned per-variant
   in unit + integration tests), `enable_early_data = true` client-side
   (pinned), aws-lc-rs provider on every path (suite-count pinned),
   nine-scheme exact-list pin (unit + integration, exact vec equality),
   `acme-tls/1` ACME-only append (exact ALPN-list equality), non-empty
   root store (fallback test), fail-closed structure (`None` installs
   `WebPkiServerVerifier` — asserted via config Debug).
4. **Deltas vs the extraction source — PASS.** The full src/ diff
   against `alknet-tls` + the moved `alknet-core` modules maps entirely
   to ADR-pinned deltas; the behavior-preservation comparison found
   **zero unrecorded behavioral divergences** (details in Part B). The
   one dropped marker impl (`zeroize::ZeroizeOnDrop` on
   `Ed25519SecretKey`) is behaviorally inert — see N-5.
5. **Feature hygiene + docs sync — PASS.** `default = []` lean; tokio
   subset in `[dependencies]`; zero inline `//` comments in non-test
   code; zero `unwrap`/`expect`/`panic!` in non-test code
   (grep-verified, 81 matches all inside `#[cfg(test)]`); doc warnings
   zero; packaging passes. One packaging note: `tasks/*.md` and
   `docs/architecture/**` ship in the package (N-6) — harmless but
   probably not intended.

---

# Part A — Findings

## S-1 [major, security posture] — `AcceptAnyCertVerifier` does not verify the client's CertificateVerify (no proof-of-possession), and no doc states the consequence: any party that sees a peer's public cert/SPKI can complete a handshake *as that peer*, and the server will hand the auth layer the victim's fingerprint

**Verified: YES, by executable probe against a real rustls handshake**
(temporary test file, deleted after the run; probe trace quoted below).

The mechanism, in rustls 0.23.44's own flow: after receiving the client
Certificate message, the TLS 1.3 server state machine calls
`verifier.verify_client_cert(...)` (rustls
`src/server/tls13.rs:1109`), then — for the CertificateVerify — calls
`verifier.verify_tls13_signature(construct_client_verify_message(...),
&certs[0], sig)` (`tls13.rs:1157-1161`). Whether proof-of-possession is
enforced is **entirely the verifier's contract**: rustls only forwards
the signature to `verify_tls13_signature` and trusts its verdict.

`AcceptAnyCertVerifier` returns `Ok(HandshakeSignatureValid::assertion())`
unconditionally from both `verify_tls12_signature` and
`verify_tls13_signature` (`src/server.rs:287-303`), and
`Ok(ClientCertVerified::assertion())` from `verify_client_cert`
(`src/server.rs:278-285`). So the CertificateVerify signature is never
checked against the presented cert's public key. A client that presents
a **victim's certificate bytes** (public — fingerprints are designed to
be shareable) while the `CertifiedKey`'s signer is the **attacker's
own key** completes the handshake, and `ServerConnection::
peer_certificates()` yields the victim's cert.

Probe evidence (X.509 variant):

```
SECURITY-PROBE client: handshake completed, data sent
SECURITY-PROBE server: read 5 bytes of application data
SECURITY-PROBE status: handshake-ok
SECURITY-PROBE server-extracted fingerprint:
    Some("SHA256:e5b35f9ca4a2d607e3ef5d20f84050c7afe56ea9b6233f316365fa846d140370")
SECURITY-PROBE victim fingerprint:
    SHA256:e5b35f9ca4a2d607e3ef5d20f84050c7afe56ea9b6233f316365fa846d140370
CONFIRMED: handshake completed with the victim's cert + attacker's
signing key (no proof-of-possession) — the server-side fingerprint is
spoofable
```

And the RFC 7250 raw-key variant — the same attack with the victim's
**SPKI** (only the public part, which peers legitimately see and
fingerprint):

```
SECURITY-PROBE2 server-extracted fingerprint:
    Some("ed25519:5b0a83cd170e7694269a9b6b633b2589744e1fd660de07f858dde70b871f92ca")
SECURITY-PROBE2 victim fingerprint:
    ed25519:5b0a83cd170e7694269a9b6b633b2589744e1fd660de07f858dde70b871f92ca
CONFIRMED (raw-key variant): handshake completed with the victim's SPKI
+ attacker's signing key — RFC 7250 fingerprint is spoofable the same way
```

(The asymmetry: the **client-side** `FingerprintPinVerifier` *does*
enforce proof-of-possession — it routes Ed25519-SPKI certs through
`verify_tls13_signature_with_raw_key` and non-Ed25519 certs through
`verify_tls12/13_signature` with the provider's algorithms, so a
forged CertificateVerify fails there (`src/client.rs:282-318`, pinned
by `fingerprint_pin_verifier_routes_ed25519_spki_tls13_signature_
through_raw_key_path` including the forged-signature rejection). The
server side has no such check.)

### Why this is not the auth layer's problem alone

The documented story (server.md, the verifier's own doc) is: "The
fingerprint is matched against peer records by the auth layer
(`IdentityProvider::resolve_from_fingerprint`) *outside* this crate —
the TLS crate hands over the fingerprint string." That division of
labor is correct for *authorization* — but the auth layer can only
resolve a fingerprint it is **handed**, and with no
proof-of-possession the handed string is **attacker-chosen from the
set of peer fingerprints the attacker has observed**. The auth layer's
peer table gives it no way to detect this: the fingerprint *is* the
victim's. Every downstream decision (scopes, session resumption,
token issuance) is made against a spoofed identity. The
"auth layer's concern" boundary cannot retroactively verify a
signature the TLS layer already accepted the cert for and discarded.

This is not a *code* bug in the alknet-inheritance sense — the
extracted alknet code has the identical verifier (alknet-tls
`server.rs:244-297`), so behavior-preservation holds and this review
does not demand a deviation. It is a **posture/documentation gap with
real attack consequences**: the "request-but-don't-require" doc
describes the request behavior but nowhere states that the presented
signature is unverified, and the "does not verify the presented cert
against a CA" phrasing reads to a consumer like the *only* gap ("no
CA check"), not "no proof-of-possession, so presented identity is
spoofable by anyone with the public bytes."

### Why [major], not [critical]

The decided spec invariant (request-but-don't-require, alknet ADR-034)
is honored as written — the code does what its spec says. What makes
it [major]: the failure mode is silent impersonation on the
security-critical server-auth path, the exploit needs only public
material, and the docs that should carry the warning don't exist. If
the auth layer's design assumes `peer_certificates()` proves key
possession (a reasonable reading of the docs today), the deployment is
vulnerable *now*.

### The fix (doc-level, plus an upstream-tracked question)

1. **Doc on `AcceptAnyCertVerifier`** (load-bearing): state explicitly
   that *the client's CertificateVerify signature is not verified* —
   presented certs are not checked for proof-of-possession — and that
   consequently the fingerprint the server extracts is
   **attacker-suppliable** unless the caller enforces possession. Point
   at the two safe patterns the auth layer can own:
   - (a) challenge-response over the established channel bound to the
     certificate's public key (a stolen fingerprint can't be replayed
     without the key); or
   - (b) a server-side verifier that *does* verify the CertificateVerify
     signature against the presented cert (the same
     `verify_tls13_signature_with_raw_key` /
     `verify_tls12_signature` routing the client-side pin verifier
     already implements — the crate has the code, just not on this
     type).
2. **Record an ADR/OQ** for the identity-impersonation surface: which
   layer owns proof-of-possession on the server path (alknet ADR-034
   inherited the shape but never recorded this property). Until
   decided, the S-1 doc note is the honest description.
3. **Consider a `require` sibling**: a
   `VerifyPresentedCertVerifier` (request-and-verify) is additive —
   same nine-scheme list, `verify_client_cert` still permissive, but
   the two signature methods delegate to `rustls::crypto::
   verify_tls{12,13}_signature(_with_raw_key)`. If the rewrite's
   endpoint model wants mandatory pop, that's a cheap, additive
   companion type — decide before the first consumer.

**Remediation task:** ~~none exists yet;~~ **landed** —
`tasks/fix-accept-any-cert-verifier-posture.md` (completed, commit
`e86b8ba`): the S-1 doc note on `AcceptAnyCertVerifier`, the N-1
companion fix, OQ-TLS-09 (open, three options a/b/c), and the
impersonation probe permanent as `tests/impersonation_posture.rs`
(tcp-gated, both cert types — handshake completes, app data flows, the
server extracts the victim's fingerprint; a future pop change must
fail/update that test with the doc + OQ together). server.md and
client.md carry the same posture. This review does not change code —
the finding is doc + ADR work.

## C-1 [minor, correctness] — `generate_self_signed_cert()` produces a cert valid 1975→4096 (rcgen's default), i.e. effectively never-expiring, and no doc states it

**Verified: YES, by DER probe** (temporary test file, deleted after the
run). Parsing the generated cert's Validity SEQUENCE:

```
probe6 validity: not_before = UTCTime "750101000000Z" (1975-01-01)
                 not_after  = GeneralizedTime "40960101000000Z" (4096-01-01)
```

rcgen 0.13's `CertificateParams::default()` carries these bounds; the
function passes `params` untouched (`src/server.rs:230`). Consequences:

- The `SelfSigned` dev cert **never expires in practice** — fine for a
  dev/loopback identity, and it can never *fail* the way an ACME or
  ops-managed cert can.
- A consumer that **pins the SHA256 fingerprint** of a self-signed cert
  (the `SHA256:<hex>` remote path) gets a pin that stays valid for two
  millennia — the pin-vs-expiry interplay is silent.
- The cert also has **no SANs** (probed: no `subjectAltName`
  extension), so CA verification of it fails as expected —
  self-consistent with "dev cert".

Not a bug (the behavior is deterministic and matches the extracted
source); filed because "dev cert" silently meaning "cert that never
expires" is a documented-behavior gap: one doc line on
`generate_self_signed_cert` (and/or `SelfSignedCert`) pins it, plus the
validity facts for the record. If a tighter validity is ever wanted,
the params are one `not_before`/`not_after` away — additive.

## C-2 [minor, correctness] — `acme-tls/1` is appended unconditionally; a caller who already includes it in `alpns` gets it twice

**Verified: YES, by probe** (temporary test, deleted):

```
probe5b: TlsServerConfig::new(Acme, alpns=[b"acme-tls/1"]) →
         alpn_protocols == [b"acme-tls/1", b"acme-tls/1"]  (duplicated)
```

`src/server.rs:87-89` appends without checking membership. Harmless
today (rustls dedups nothing but ACME's `is_tls_alpn_challenge` requires
the client to offer *exactly* `[acme-tls/1]` — rustls-acme
`helpers.rs:6-8` — so a duplicated server-side entry doesn't break
challenge detection; a normal client offering a real ALPN + the
duplicate still negotiates the real one). Two nits ride with it: the
duplication leaks into the wire config (cosmetic), and the non-ACME
paths pass the caller's list through untouched while the ACME path
mutates — an asymmetry the doc doesn't call out. Fix is a one-line
`if !alpn.contains(...)` guard, or pin the current shape in a doc note
+ test. Low priority.

## C-3 [minor, robustness] — `TlsIdentity::Acme` with an **empty** `domains` list constructs successfully (spawns the state machine, registers the resolver); no validation fires

**Verified: YES, by probe** (temporary test, deleted):

```
PROBE14: empty-domains ACME config CONSTRUCTS OK (rustls-acme handles it)
```

`TlsServerConfig::new` passes `domains.to_vec()` straight into
`AcmeConfig::new` (`src/server.rs:72`); rustls-acme does not reject an
empty domain list at construction. The spawned order loop will then
fail per-connection/per-order at runtime — a warn-level event stream
(`EventError::Order`) with **no `TlsError`** (the ADR-002 scope
boundary makes this correct). The construction-time gap is that the
assembly layer gets no signal until the (logged-only) order failures
appear. A one-line validation (`if domains.is_empty() { return
Err(TlsError::AcmeConfig("empty domain list")) }`) would be additive
and cheap; alternatively pin the permissive shape in the docs. Same
class: empty `contact` is legal upstream (ACME allows
zero-contact accounts — RFC 8555 §7.3), so *that* one is correct as-is.

## C-4 [minor, robustness] — `RemoteIdentity::fingerprint` accepts any string; malformed pins are rejected only at handshake with an opaque `Error::General`

**Verified: YES, by probe** (temporary test, deleted) — all of
`"ed25519:"`, `"SHA256:"`, `""`, `"not-a-fingerprint"`,
`"ed25519:zzzz"`, `"sha256:cafe"` construct a `TlsClientConfig` fine.
That is by design (config construction can't validate what the remote
will present; handshake-time rejection is the scope boundary, and the
fail-closed rule holds — a garbage pin rejects, never downgrades). But
two sub-observations worth pinning in docs:

1. The pin comparison is **exact-string** (`src/client.rs:272`), so an
   uppercase-hex pin (`ED25519:ABCD…` / `SHA256:ABCD…`) or a
   lowercase `sha256:` prefix **rejects a matching remote** (probe
   pinned). The fingerprint formats are lowercase-hex by construction
   (`src/fingerprint.rs:34,39` — `hex::encode` is lowercase, tests pin
   it), so a config-layer consumer that upcases for display will
   silently break pinning. A doc line on `RemoteIdentity::fingerprint`
   ("must be produced by `fingerprint_from_cert_der`; case- and
   format-exact") closes it.
2. Cross-prefix pins are fail-closed: pinning a raw-key remote by its
   `SHA256:` cert hash can never match, because the verifier computes
   `ed25519:<hex>` for SPKIs (`src/client.rs:268-272` — the formatted
   strings can't be equal). Probed: rejects. Correct, but again a
   config-author trap worth the same doc line.

## U-1 [minor, coverage] — The ACME event-loop body (`src/server.rs:94-133`) is unreachable by tests: 23 uncovered lines, and the only exercise of `new_acme` asserts construction, never event handling

`cargo llvm-cov --all-features`: `server.rs` sits at 88.24% lines; the
block from `while let Some(event) = state.next().await` (line 95)
through the closing log (line 132) has **zero coverage** — every
`EventOk`/`EventError` arm, the `debug`/`warn`/`error` mapping, and the
"state machine ended" log are dead code as far as the test suite can
prove. The `acme_lifecycle` tests construct the config and assert
spawn + ALPN + resolver wiring — the spawned task runs against
`http://127.0.0.1:9/directory` (a port-9 dead address) and its events
are never observed. Consequences:

- A refactor that drops or mislevels an arm (e.g. the
  `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.

**Correction (2026-09-12, verified against the vendored rustls-acme
0.12.1 sources): the termination premise above is wrong.** The
`Stream for AcmeState` impl is `Poll::Ready(Some(ready!(
self.poll_next_infinite(cx))))` (state.rs:407-412) — the stream never
yields `None`; order errors enter an exponential backoff (capped at
2^16 s) and retry forever. There are no termination semantics for
upstream to change, the `while let` loop never exits on its own, and
the "ACME: state machine ended" log (src/server.rs:135) is unreachable
dead code under this dependency. The exit-condition bullet and the
"assert the JoinHandle resolves" remediation shape are withdrawn —
`tasks/acme-event-loop-test.md` carries the corrected work items
(timeout-bounded event collection; the reachable-arm inventory;
server.rs:135 either deleted or accepted as uncovered).

The event-mapping match is deterministic and its arms are trivial, so
the risk is low — but this is exactly the "untested code hides bugs"
class the review is hunting. Cheap remediation: an integration test
that drives a `DirCache`-backed `AcmeState` against a local fake
directory (a stub HTTP server that serves a directory JSON with no
endpoints, forcing the error path through the real event stream and
asserting the log events / termination via the `JoinHandle`) — or at
minimum a unit test extracting the match into a helper and testing it
directly. Fold into the acme-hardening batch (C-3).

## U-2 [minor, coverage] — `FingerprintPinVerifier`'s CA-fallback arms are asserted but the *non-Ed25519* TLS 1.3 signature path and the verifier's `verify_server_cert` failure mode on malformed certs are only covered by the X.509 probes; `pem.rs`'s `rustls_pemfile::private_key` Err arm (`pem.rs:30`) is unreachable by tests

**Status note (2026-09-12): partially superseded.** The ADR-007/ADR-008
handshake and impersonation suites (`tests/handshake_behavior.rs`,
`tests/impersonation_posture.rs`) have since closed several groups
below — the client resolvers' `resolve` calls, `RawKeyCertResolver::
resolve`, and `FingerprintPinVerifier::verify_tls13_signature`'s
non-Ed25519 arm — and ADR-008's `VerifyPresentedCertVerifier` opened a
new uncovered region (its `verify_tls12_signature` body, server.rs
349-366). `tasks/coverage-cheap-closes.md` carries the re-baselined
per-line ground truth (fresh `cargo llvm-cov` run) and supersedes the
inventory rows here.

From the coverage inventory:

- `src/client.rs:316,320-322` — the `verify_tls13_signature`
  **non-Ed25519** arm (`rustls::crypto::verify_tls13_signature`) and
  the surrounding raw-key-`else` are only covered via the X.509 pin
  tests in the probe file; the permanent suite pins the Ed25519 routing
  (`client.rs:439-485`) but has no pinned test for "an RSA/ECDSA cert's
  TLS 1.3 signature verifies through the CA-style path." A pinned test
  (an ECDSA-P256 rcgen cert + `dss_with_scheme`, assert ok, then a
  forged sig → err) closes it.
- `src/client.rs:183-189, 212-218` — `RawKeyClientCertResolver::resolve`
  and `NoClientCertResolver::resolve` are never *called* by a test
  (the matrices assert `has_certs`/`only_raw_public_keys`, not what
  `resolve` yields). One-line tests: `resolve` returns `Some(key)` for
  the raw-key resolver and `None` for `NoClientCertResolver`.
- `src/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: `rustls_pemfile::private_key` returns Err on malformed
  PEM that isn't just keyless. A garbage-but-keyed file
  (e.g. `b"-----BEGIN PRIVATE KEY-----\n!!!\n-----END PRIVATE
  KEY-----\n"`) exercises it.
- `src/server.rs:287-303` — `AcceptAnyCertVerifier`'s two
  signature-assertion methods are never called by a test (the
  assertion-bodies sit uncovered). Two-line test each (call, assert
  `Ok`), making the no-pop posture explicit in the suite (and giving
  S-1's doc note a test to point at).
- `src/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 or
  a direct `resolve` on a dummy hello through the trait object with a
  cast — the cheapest shape is an end-to-end raw-key handshake probe
  (which would also pin the RFC 7250 server path end-to-end, a
  handshake-level test the crate currently lacks entirely — see U-3).
- `src/fingerprint.rs:67` — the
  `bit_string_contents.len() != 33 || bit_string_contents[0] != 0x00`
  *second* disjunct (wrong unused-bits byte) is covered by
  `bad_bit_string_lengths_extract_nothing`'s third case, but line 67's
  first branch (len == 33 with unused-bits ≠ 0 vs len ≠ 33) shows as
  partially uncovered — a 34-byte-bit-string + unused-bits≠0 matrix
  case pins both branches. Cosmetic.
- `src/client.rs:144-147` — the webpki-roots fallback *push* loop is
  covered only on the `roots.is_empty() == true` path of the probe
  environment (the test asserts non-emptiness, which can pass either
  way if the platform store is non-empty — the loop then never
  runs). 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. Cheap and worth it — this is a load-bearing invariant
  whose fallback branch currently has no deterministic test.

## U-3 [minor, coverage/verification] — No handshake-level test exists at all: every test asserts config *construction*; the crate's actual TLS behavior (server accepts, pin verifies, fail-closed manifests) is never executed

This is the deliberate ADR-006 scope call ("handshakes are out of
scope — they belong to transport crates"), and for the transport
crates' own suites that's right. But the crate ships two *verifiers*
whose behavior only manifests at handshake, and one of them
(`FingerprintPinVerifier`) has a fail-closed contract that is asserted
only structurally ("`None` installs WebPkiServerVerifier"). What no
test proves today:

1. **The pin path end-to-end**: a client config with
   `FingerprintPinVerifier` handshaking a server config whose
   presentation matches the pin → handshake ok; mismatched pin →
   handshake fails. (tokio duplex + `TlsAcceptor`/`TlsConnector` under
   the `tcp` feature gets this in ~60 lines — the probe files for this
   review did exactly this shape.)
2. **The fail-closed path**: same setup, `remote_identity: None` +
   raw-key server → handshake fails (the structural claim, executed).
3. **The RFC 7250 raw-key server path end-to-end**: `RawKeyCertResolver`
   + a 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** (as a documented
   behavior-pin test, if the doc-not-code-fix route is taken) so the
   spoofable-identity posture can't silently change either direction
   without a test flagging it.

These are the highest-value missing tests in the crate: they turn the
"fail closed" and "pin" language from documentation into executed
behavior. The ADR-006 boundary (no transport crates in scope) is not
violated by a duplex-pair handshake — no external transport dep, just
tokio + tokio-rustls under the existing `tcp` feature.

## N-1 [minor, docs] — `FingerprintPinVerifier`'s doc says "a stolen-but-stale fingerprint can't be replayed with a forged signature" — true for the *client* verifier, but the sentence reads as if the *fingerprint* protects the presenter; a stolen PRIVATE key is the actual threat, and the doc's phrasing invites mis-reading the verifier as pop-complete

`src/client.rs:232-235`: "Handshake signatures are still verified …
so that a stolen-but-stale fingerprint can't be replayed with a forged
signature: the presenter must prove possession of the corresponding
private key." The mechanism described is real and tested; the phrasing
"stolen-but-stale fingerprint" is a bit garbled (a fingerprint isn't
the thing being stolen-and-replayed — the *cert* is presented fresh
each time), and — critically — the doc does not contrast itself with
the server-side verifier (S-1). One clarifying line ("this verifier
checks proof-of-possession; the *server-side* AcceptAnyCertVerifier
does not — see that type's docs") closes the loop.

**Remediated (commit `e86b8ba`)**: the phrasing is replaced (the cert
is presented fresh each handshake; the signature check defeats use of
a stolen/observed cert by a party without the private key) and the
cross-reference to `AcceptAnyCertVerifier` exists on the type, in
client.md, and server.md.

## N-2 — NON-FINDING (retracted): the client config's `dangerous()` verifier path was suspected of allowing an unverified-CA downgrade — it does not

Filed as a suspect ("`with_custom_certificate_verifier` + `None`
remote = trust-on-first-use?"), then falsified by code trace before
filing: the CA path installs `WebPkiServerVerifier` — real CA
verification — not a permissive verifier; the permissive
`FingerprintPinVerifier` only appears under `Some` (pin), where the
fingerprint IS the anchor (and pop holds there). The selection matrix
has no downgrade path. Recorded so the next reader doesn't re-derive
the suspicion.

## N-3 [minor, docs] — The crate-level doc and server.md say "not `Clone` (it holds the ACME task's `JoinHandle`)", but under `default` features (no `acme`) the struct has **no** handle field at all — so a noq/tcp-only consumer could safely derive `Clone`, yet the API forbids it

`src/server.rs:20-25`: `acme_handle` is `#[cfg(feature = "acme")]`-gated;
with `default = []` the struct is two `Clone`-able fields. The
non-`Clone` decision (ADR-001's "one ACME state machine per domain"
protection) is *conditionally* motivated: it prevents cloning only
when `acme` is on, and forbids an ergonomic pattern (`Arc`-free
sharing) for every other consumer. That's defensible — the API must
hold across feature combinations, and making `Clone` conditional on a
feature is its own trap (a consumer compiling both configurations
would see an API difference). Recorded as an observation, not a
change request; the all-features posture is the one the freeze pins.

**Remediation status (2026-09-12): closed by decision** —
`tasks/docs-pin-c1-c4-n3-n4.md` (completed): `TlsServerConfig` stays
non-`Clone` for v1, the reasoning is recorded in the type's rustdoc
(src/server.rs), and this finding is closed. Revisit only if a
concrete consumer demands it.

## N-4 [minor, docs] — `AcceptAnyCertVerifier::root_hint_subjects() == &[]` means clients see no CA hints; combined with `client_auth_mandatory() == false` this is the correct request-not-require shape, but `requires_raw_public_keys()` (the trait's third relevant knob, rustls `src/verify.rs:143`, default `false`) is never overridden — correct here (the verifier accepts both X.509 and RFC 7250 client certs), worth one doc sentence so the next reader doesn't "fix" it to `true` for the raw-key peers

The raw-key client path works because rustls negotiates the
ClientCertificateType extension from the *resolver's*
`only_raw_public_keys()` (server cert side) and the *verifier's*
`requires_raw_public_keys()` (client-cert side, default `false` =
X.509-or-raw both accepted). `AcceptAnyCertVerifier` correctly keeps
the default (`false`) — pinned by the verifier tests. One sentence in
the type's doc ("accepts both X.509 and RFC 7250 raw public keys;
`requires_raw_public_keys()` stays `false` so both cert types
negotiate") prevents a well-meaning tightening that would break raw-key
clients. Verified against rustls 0.23.44's
`validate_client_cert_type_extension` (`server/hs.rs:242-283`): with
`requires_raw_keys == false` and the client offering only
`RawPublicKey`, negotiation still succeeds (`(false, true, false)` arm
→ X509 type negotiated — wait, no: `(false, true, false)` →
`Err(IncorrectCertificateTypeExtension)`; the client offering
`[RawPublicKey]` while the verifier doesn't *require* raw keys
*errors*). Correction: a client offering ONLY the raw-key
certificate type against this verifier fails the handshake — the
client must offer both `[X509, RawPublicKey]` (or the verifier must
set `requires_raw_public_keys() == true` to accept raw-only clients).
The extracted alknet code has the identical shape (no
`requires_raw_public_keys` override), so behavior is preserved — but
the interaction is subtle enough that the doc note should state it
explicitly: **a raw-key client must include `X509` in its
`client_certificate_types` offer, or the request-but-don't-require
verifier rejects it.** (alknet's own client resolver offers both
types, which is why the extracted code works in production.)

**Correction to the parenthetical above (found during the S-1
remediation, 2026-09-11):** "alknet's own client resolver offers both
types" is **inaccurate**. rustls 0.23.41 and 0.23.44 both send
`client_certificate_types = [RawPublicKey]` (only) whenever 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 version offers both types. The
consequence chain, verified from the rustls sources:

1. A rustls-driven client with a raw-key resolver (e.g.
   `RawKeyClientCertResolver` with an Ed25519 SPKI,
   `only_raw_public_keys() == true`) offers `[RawPublicKey]` only.
2. Against `AcceptAnyCertVerifier`
   (`requires_raw_public_keys() == false`), the `(false, true, false)`
   arm of `process_cert_type_extension` (server/hs.rs:274-288) fires →
   `IncorrectCertificateTypeExtension` → **handshake failure**.
3. So a raw-key *client* cannot handshake against this verifier at
   all (fail-closed, not a downgrade — but a real interop trap the
   doc note must state).
4. The S-1 raw-key impersonation probe sidesteps this: the attacker
   presents the victim's SPKI under the *default* X.509 offer
   (`only_raw_public_keys() == false` → no cert-type extension →
   `(false, false, false)` → default X.509), and the server passes the
   SPKI bytes through unparsed (a custom `verify_client_cert` receives
   opaque bytes) — which is why the probe shape pins
   `only_raw_public_keys() == false` on its fixed resolver.

The doc note for task 6 (`docs-pin-c1-c4-n3-n4`) should be written
from this chain (and the rustls sources), not from the review's
original parenthetical: the accurate statement is that
`requires_raw_public_keys()` must stay `false` for X.509-or-raw
*server* offers, and that a raw-key *client* resolver makes rustls
offer `[RawPublicKey]` only, which this verifier rejects.

## N-5 — NON-FINDING (verified): the dropped `zeroize::ZeroizeOnDrop` marker impl does not change memory-erasure behavior

The port review recorded the omission as accepted
(`tasks/review-impl.md` findings table). Re-verified here, deeper:
ed25519-dalek 2.2.0's default features include `zeroize`, whose
`SigningKey: Drop` impl zeroizes the inner secret
(`ed25519-dalek-2.2.0/src/signing.rs:658-666`); alktls' resolved tree
activates it (`Cargo.lock` + `cargo tree -e features -i zeroize@0.2` —
`ed25519-dalek feature "zeroize"` ← `"default"` ← alktls). The
wrapper's `Debug` is `finish_non_exhaustive()` (leak-free, pinned by
`ed25519_secret_key_debug_does_not_leak_material` + the review probe
through `TlsIdentity`'s derived Debug). Only a downstream *marker-trait
bound* on `Ed25519SecretKey` would observe the difference (nothing
known bounds on it). If the rewrite's vault wants the marker back,
`zeroize` as a direct dep + the one-line impl is additive. Recorded so
the next reader doesn't "restore" it thinking erasure was lost.

## N-6 [minor, packaging] — `tasks/*.md` and `docs/architecture/**` ship in the published package

`cargo package --list` shows `tasks/crate-init.md` …
`tasks/review-impl.md`, the whole `docs/architecture/` tree (including
open-questions and ADRs) inside the crate package. The `exclude` list
(`Cargo.toml:11`) covers `.opencode/`, `AGENTS.md`, `docs/reviews/`,
`docs/research/`, `docs/plans/`, `docs/sdd_process.md` — but not
`tasks/` or `docs/architecture/`. Harmless (adds ~50 KB, crates.io
renders only the README), but the house pattern (alktunnels) ships
neither; the internal SDD task notes (agent-facing) in a public
package is noise at best. One-line `exclude` addition. No functional
impact.

## N-7 [minor, docs] — `AcmeDirectory::Custom(String)` accepts any string; the URL is passed to rustls-acme verbatim, so an `http://` custom directory silently runs ACME over plaintext (token-bearing)

rustls-acme does not enforce https for the directory URL (the ACME
spec requires https for directories, but a malicious/misconfigured
custom URL is a caller-side concern). The `TlsIdentity::Acme` doc says
"a custom ACME directory URL" without warning that plaintext http
exposes account keys + authorizations on the wire. One doc line on
`AcmeDirectory::Custom` ("must be an `https://` ACME directory URL")
closes it. (The pinned Production/Staging strings are correct.) Trivial.

---

# Part B — Non-findings (verified correct, recorded to bound the re-review)

- **Behavior preservation vs the extraction source — full match.**
  Independent diff of every alktls module against
  `alknet/crates/alknet-tls/src/*` + the moved alknet-core modules:
  `max_early_data_size` on all four server paths (alknet server.rs
  83/168/179/191 ↔ alktls server.rs 85/186/196/206), client
  `enable_early_data` (client.rs 36 ↔ 42), the nine-scheme list
  verbatim in identical order, `acme-tls/1` append placement identical,
  root-store fallback identical (same log levels + messages, same
  direct `roots.roots.push` bypass), `FingerprintPinVerifier` exact
  string comparison with lowercase-hex formats in both, the
  tls12/tls13 raw-key signature routing identical (including the
  quirk that the *tls12* method routes Ed25519 SPKI through
  `verify_tls13_signature_with_raw_key` — preserved exactly), pem.rs
  logic identical, DER parser byte-identical. No validation dropped.
- **Client-side proof-of-possession is real and tested**: the pin
  verifier's signature checks route through
  `verify_tls13_signature_with_raw_key` for SPKI certs and
  `verify_tls12/13_signature` for X.509 — with the provider's
  `WebPkiSupportedAlgorithms` — and the tests assert both
  accept-correct and reject-forged (client.rs:439-485). rustls
  verifies the CertificateVerify through this verifier
  (server/tls13.rs:1157-1161 is the server-side analogue), so the pin
  cannot be satisfied by a stolen cert alone.
- **Fail-closed is structural, and the cross-prefix case fails closed
  too** (probe): a `SHA256:`-formatted pin of an Ed25519 remote can
  never match (the verifier computes `ed25519:`-formatted
  fingerprints for SPKIs), and malformed pins (`""`, `"not-a-fingerprint"`,
  wrong-case prefixes) construct but never match anything — rejection
  at handshake, never a silent downgrade. The matrix has no fourth
  path.
- **The DER parser is panic-free under adversarial input** (probe
  battery: truncated headers, oversized lengths including a
  0x88-length 8-byte monster that would overflow u32-based parsers,
  indefinite-length 0x80, wrong tags, zero-length inputs — all
  extract-None / hash-SHA256, no panics). The length decoding bounds
  `num_bytes ≤ 4` and `checked_add` on total length — no overflow
  paths on 64-bit (and the 4-byte cap bounds it on 32-bit too).
- **The empty-PEM chain fails loudly at construction** (probe): an
  empty cert file loads to an empty chain, and `with_single_cert`
  rejects it with rustls' `NoCertificatesPresented` →
  `TlsError::Rustls` — no panic, no silently-empty server config.
- **`TlsIdentity` Debug does not leak key material** (probe):
  `Ed25519SecretKey`'s manual Debug is `finish_non_exhaustive()`, and
  the derived `TlsIdentity` Debug inherits it — the raw bytes do not
  appear. `FingerprintPinVerifier`'s Debug *does* print the pinned
  fingerprint (a public value — correct), and the client config's
  derived Debug therefore contains it (observable but not secret).
- **`Ed25519SecretKey` surface is iroh-exact**: `from_bytes(&[u8;32])`
  / `as_bytes() -> [u8;32]` / `public()` / `sign()`, with the
  zeroization posture verified (N-5).
- **The noq wrap is the real noq contract**: `for_noq` →
  `QuicServerConfig::try_from(inner.clone())` — noq-proto 1.3.0
  requires TLS 1.3 in the config (its `initial_suite_from_provider`
  needs a TLS 1.3 suite; aws-lc-rs default has it) and documents
  `max_early_data_size = u32::MAX` as the 0-RTT-enabling value —
  exactly what the invariant sets, so the QUIC path composes with the
  invariant rather than fighting it. The one noq failure
  (`NoInitialCipherSuite`) is the `NoqWrap` variant's source — shape
  verified in noq-proto 1.3.0 source (`crypto/rustls.rs:587-597`).
- **ACME ALPN + challenge wiring is upstream-correct**: rustls-acme's
  `ResolvesServerCertAcme::resolve` dispatches on
  `is_tls_alpn_challenge` (client offers exactly `[acme-tls/1]`) and
  serves the challenge cert from `auth_keys` by SNI — the crate's
  `acme-tls/1` append is the list that makes that dispatch reachable.
  The challenge-connection close semantics live in rustls-acme's
  acceptor (out of crate scope). The TLS-ALPN-01 cert itself is keyed
  by the domain's `key_authorization` SHA-256 inside rustls-acme —
  nothing for alktls to add.
- **TLS 1.2 remains enabled** (`with_safe_default_protocol_versions`
  → `DEFAULT_VERSIONS` = `ALL_VERSIONS` = TLS13 + TLS12) on all paths,
  matching the extracted code; the pin verifier's TLS 1.2 signature
  path is exercised by test for Ed25519 (client.rs:467-485).
- **Packaging + battery**: `cargo publish --dry-run --allow-dirty`
  passes; 68/75 tests green (default/all-features); clippy clean both
  configs; fmt clean; `cargo doc` warning-free; coverage 95.32% line
  (94.48% functions) — the gaps are inventoried in Part C/U-1..U-3.

---

# Part C — Coverage inventory (cargo llvm-cov, --all-features)

Total: **95.32% lines / 94.48% functions** (2477 regions, 116 missed).
Per file: credentials.rs 100%, signing.rs 100%, identity.rs 98.04%,
fingerprint.rs 98.85%, lib.rs 93.75%, pem.rs 95.24%, client.rs
94.97%, server.rs 88.24%.

The load-bearing uncovered groups (full analysis in U-1..U-3):

| Lines | What | Why it matters |
|-------|------|----------------|
| server.rs 95–133 | the entire ACME event-loop body | the only runtime behavior of the acme feature; untested |
| server.rs 287–303 | AcceptAnyCertVerifier's signature-assertion bodies | the no-pop posture; never executed by a test |
| server.rs 341–346 | RawKeyCertResolver::resolve | the RFC 7250 server path's core method |
| client.rs 183–189, 212–218 | the two client cert resolvers' `resolve` | never called by a test |
| client.rs 297, 316–322 | FingerprintPinVerifier's non-Ed25519 tls12 + raw-key tls13 arms | the pop-routing under-test |
| client.rs 144–147 | the webpki-roots fallback loop | load-bearing invariant; only nondeterministically covered |
| pem.rs 30 | load_private_key's parse-error arm | the third PEM failure mode |
| fingerprint.rs 67, 125, 143 | DER parser edges | already probed no-panic; near-full |

Not covered but acceptable: `identity.rs:201,232` + `lib.rs:113` are
test-only panic arms; `credentials.rs`/`signing.rs` are fully covered.

# Remediation ordering

1. **S-1 doc + ADR note** (the impersonation posture) — doc-only;
   unblocks honest consumer review. Pair with the U-3(4) permanent
   probe so the documented behavior is pinned from both sides.
2. **U-3's handshake-level tests (1)(2)(3)** — turns the fail-closed /
   pin / raw-key language into executed behavior; `tcp`-feature-gated,
   no new deps.
3. **U-2's cheap unit closes** (resolvers' `resolve`, the non-Ed25519
   pin-verifier arm, pem parse-error arm, fallback-loop seam) — one
   pass.
4. **C-2 + C-3 + N-6 + N-7 trivia batch** — the ALPN dedup guard (or
   doc pin), the empty-domains validation (or doc pin), the exclude
   additions, the `https://` doc line.
5. **U-1's ACME event-loop test** — the fake-directory integration
   test; medium effort, real value (the acme feature's only runtime
   surface).
6. **Consider-before-first-consumer** (N-3): whether
   `TlsServerConfig: Clone` under non-acme features is worth the API
   split; decide with the rewrite's consumer in hand.