alkdev/alktls
6
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

review 001: alktls v1 implementation — correctness, security, coverage

Browse Source 
Independent post-port review (behavior-preservation + security +
coverage focus; the alknet sources consulted only as prior art).

Findings:
- S-1 [major, security posture]: AcceptAnyCertVerifier does not verify
  the client CertificateVerify (no proof-of-possession) — executable
  probe confirms a victim-cert + attacker-key handshake completes and
  the server extracts the victim's fingerprint (both X.509 and RFC
  7250 SPKI variants). Doc/ADR remediation, not a behavior change
  (the shape is the alknet-inherited design).
- C-1..C-4 [minor]: self-signed cert validity 1975-4096 undocumented;
  acme-tls/1 duplicated if caller supplies it; empty ACME domains
  construct without validation; malformed/case-shifted fingerprint
  pins reject at handshake with no config-layer doc.
- U-1..U-3 [minor, coverage]: ACME event-loop body uncovered (23
  lines); resolvers' resolve() methods uncovered; zero
  handshake-level tests (fail-closed/pin/raw-key paths asserted only
  structurally); webpki-roots fallback loop nondeterministically
  covered.
- N-1..N-7: doc nits, two non-findings (dangerous()-downgrade suspect
  retracted; zeroize marker verified inert via ed25519-dalek default
  features), packaging noise (tasks/ + docs/architecture/ ship),
  requires_raw_public_keys interaction note (raw-only clients fail
  against request-but-don't-require), http:// custom ACME directory
  warning.
- Behavior preservation: full src/ diff vs alknet-tls + alknet-core —
  zero unrecorded behavioral divergences (Part B).

Verification: cargo test 68 default / 75 --all-features; clippy -D
warnings (both configs); fmt --check; cargo doc --no-deps;
cargo publish --dry-run --allow-dirty; cargo llvm-cov --all-features
95.32% line / 94.48% functions, uncovered-line inventory in the doc;
temporary review probes (fingerprint pins, DER adversarial battery,
empty-PEM, ACME construction edges, impersonation handshake) all run
and deleted after the run.
This commit is contained in:
glm-5.3-flash
2026-09-10 16:14:05 +00:00
parent 8a15978c41
commit bb0d060135
1 changed files with 690 additions and 0 deletions
Download Patch File Download Diff File Expand all files Collapse all files
docs/reviews/001-implementation-review.md
+690
View File
@@ -0,0 +1,690 @@
# 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).
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; if adopted, land as
`tasks/fix-accept-any-cert-verifier-posture.md` alongside the ADR/OQ
above. (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.
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
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.
## 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.
## 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.)
## 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.