record S-1/N-1 remediation status + the N-4 parenthetical correction
- review 001: §Status carries the remediation status (S-1 + N-1 landed
in e86b8ba, OQ-TLS-09 open, probe permanent) and the N-4 correction
summary; §S-1 remediation-task line updated; §N-1 marked remediated;
§N-4 gains the corrected mechanism chain (rustls 0.23.41/0.23.44 send
[RawPublicKey] iff the resolver's only_raw_public_keys() is true —
no "offers both types" behavior exists; a raw-key client fails
IncorrectCertificateTypeExtension against this verifier; the S-1
probe presents the SPKI under the default X.509 offer instead)
- task fix-accept-any-cert-verifier-posture: drop the placeholder
fill-in lines (Notes/Summary carry the actual record)
- task docs-pin-c1-c4-n3-n4: N-4 work item rewritten from the
corrected chain so the implementing agent does not re-derive it;
references point at the rustls source lines and the probe's
resolver choice
This commit is contained in:
@@ -8,6 +8,31 @@ 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`).
|
||||
|
||||
**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
|
||||
@@ -240,10 +265,16 @@ vulnerable *now*.
|
||||
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.)
|
||||
**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
|
||||
|
||||
@@ -468,6 +499,12 @@ 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`
|
||||
@@ -522,6 +559,39 @@ explicitly: **a raw-key client must include `X509` in its
|
||||
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
|
||||
|
||||
@@ -42,18 +42,39 @@ question):
|
||||
reasoning in the type's doc (one sentence) and close the question
|
||||
— revisit only with a concrete consumer demanding it.
|
||||
4. **N-4** — the cert-type negotiation interaction needs an explicit
|
||||
doc note on `AcceptAnyCertVerifier`: `requires_raw_public_keys()`
|
||||
stays `false` (both cert types accepted), and — the subtle part,
|
||||
verified against rustls 0.23.44 `validate_client_cert_type_extension`
|
||||
in the decomposition session — **a client offering ONLY
|
||||
`RawPublicKey` fails the handshake** with
|
||||
`IncorrectCertificateTypeExtension`; it must offer both
|
||||
`[X509, RawPublicKey]` (or the verifier would have to set
|
||||
`requires_raw_public_keys() == true`). The extracted alknet client
|
||||
resolver offers both types, which is why production works. Without
|
||||
the note, the next reader may "fix" the verifier to `true` and
|
||||
break X.509 clients, or mis-diagnose the raw-only-client failure as
|
||||
a bug.
|
||||
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.
|
||||
|
||||
## Work
|
||||
|
||||
@@ -87,10 +108,18 @@ question):
|
||||
## 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` `validate_client_cert_type_extension`
|
||||
(the negotiation table N-4 documents)
|
||||
- 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)
|
||||
|
||||
## Notes
|
||||
|
||||
|
||||
@@ -109,8 +109,6 @@ finding, not a code-change mandate. What's missing:
|
||||
|
||||
## Notes
|
||||
|
||||
> Agent fills this during implementation.
|
||||
|
||||
- The probe used a tokio duplex pair + `TlsConnector`/`TlsAcceptor`
|
||||
(tokio-rustls, `tcp`-gated) rather than raw `complete_io` — same
|
||||
shape, cleaner await-based flow. The attacker client uses a
|
||||
@@ -139,8 +137,6 @@ finding, not a code-change mandate. What's missing:
|
||||
|
||||
## Summary
|
||||
|
||||
> Agent fills this on completion.
|
||||
|
||||
**Landed (this task):**
|
||||
|
||||
1. `src/server.rs` — `AcceptAnyCertVerifier` doc rewritten: a
|
||||
|
||||
Reference in New Issue
Block a user