glm-5.3-flash
8a15978c41
Checklist (all six PASS): 1. API surface == ADR-004 — every accessor signature verified verbatim 2. TlsError == ADR-002 — six variants, #[non_exhaustive], typed sources, AcmeConfig holds exactly the two config-mismatch cases 3. Invariants: all five server invariants + client 0-RTT half + fail-closed structure, each with a passing behavioral test at unit and integration level 4. Deltas vs extraction: all ADR-pinned; two surfaced divergences recorded as ADR amendments — zero un-pinned divergences remain 5. Feature hygiene: default = [] lean, tokio subset (no full), doc comments on public API, no inline // comments, no panics 6. Docs sync: ADR-002 + ADR-003 amendment notes; overview/server/client Draft → Reviewed; README carries the API-freeze lifecycle note 5 findings, all low severity, all resolved forward (table in task Notes) Verification: cargo test (81), cargo test --all-features (92), clippy -D warnings, fmt --check, doc --no-deps, publish --dry-run — all green. API FROZEN for the alknet rewrite.
192 lines
9.5 KiB
Markdown
192 lines
9.5 KiB
Markdown
---
|
||
id: review-impl
|
||
name: Review alktls v1 implementation for spec conformance (pre-rewrite gate)
|
||
status: completed
|
||
depends_on: [integration-suite]
|
||
scope: moderate
|
||
risk: low
|
||
impact: phase
|
||
level: review
|
||
tags: [review, phase-gate, spec-conformance]
|
||
---
|
||
|
||
## Description
|
||
|
||
Review the completed port against the architecture docs before the
|
||
alknet rewrite consumes the crate. This is the one-way-door gate: the
|
||
public API surface freezes here (the rewrite compiles against it).
|
||
|
||
### The checklist
|
||
|
||
1. **API surface == ADR-004** — every accessor signature matches the
|
||
ADR sketches exactly (`&self` server / `self` client; `for_tcp_tls`
|
||
infallible; `rustls_config` borrows). Re-export block is the
|
||
documented surface.
|
||
2. **`TlsError` == ADR-002** — six variants, `#[non_exhaustive]`, no
|
||
string catch-all, `#[source]` chains intact.
|
||
3. **Invariants == ADR-001/server.md/client.md** — all five server
|
||
invariants + the client 0-RTT half + fail-closed structure, each
|
||
with a passing test (check the test asserts the behavior, not
|
||
that a compile succeeded).
|
||
4. **Deltas vs the extraction source** — diff `src/` against
|
||
`crates/alknet-tls/src/` + the moved alknet-core modules; every
|
||
difference maps to an ADR-pinned delta (identity-type rewire,
|
||
error mapping, `for_noq`, borrowed accessors, no `unreachable!`).
|
||
Any un-pinned divergence is either fixed or recorded.
|
||
5. **Feature hygiene** — `default = []` lean (AGENTS.md convention 4);
|
||
tokio subset (`rt`, `sync`, `macros`) in `[dependencies]`; doc
|
||
comments on the public API; no inline `//` comments outside the
|
||
convention.
|
||
6. **Docs sync** — if the port revealed a spec mismatch, the ADR/spec
|
||
gets an amendment note (not silent divergence).
|
||
|
||
## Work
|
||
|
||
1. Run the checklist against the code.
|
||
2. File findings (fix-forward for small ones; blockers get Safe Exit
|
||
treatment).
|
||
3. Update `docs/architecture/README.md` statuses (Draft → Reviewed for
|
||
the specs) when the checklist passes.
|
||
|
||
## Verification
|
||
|
||
- [x] The full checklist passes with findings recorded
|
||
- [x] `cargo test`, `cargo test --all-features`,
|
||
`cargo clippy --all-targets -- -D warnings`,
|
||
`cargo fmt --check`, `cargo doc --no-deps` all green
|
||
- [x] `cargo publish --dry-run --allow-dirty` passes (packaging
|
||
readiness: metadata, license files, exclude list)
|
||
- [x] Spec statuses advanced where the gate passes
|
||
|
||
## Acceptance Criteria
|
||
|
||
- [x] Zero un-pinned divergences from the architecture docs
|
||
- [x] The API surface is declared frozen for the rewrite (README
|
||
lifecycle note)
|
||
- [x] Findings + resolutions documented in the task Summary
|
||
|
||
## References
|
||
|
||
- docs/architecture/ (all ADRs + specs — the conformance target)
|
||
- docs/research/phase-0.md §Prior art (the extraction deltas)
|
||
- AGENTS.md (conventions 1–12)
|
||
|
||
## Notes
|
||
|
||
> Agent fills this during implementation.
|
||
|
||
## Summary
|
||
|
||
> Agent fills this on completion.
|
||
|
||
### Checklist results (all six items)
|
||
|
||
1. **API surface == ADR-004** — PASS. `TlsServerConfig::new(identity:
|
||
&TlsIdentity, alpns: &[Vec<u8>]) -> Result<Self, TlsError>` (async),
|
||
`for_noq(&self) -> Result<noq::ServerConfig, TlsError>` (noq-gated),
|
||
`for_tcp_tls(&self) -> tokio_rustls::TlsAcceptor` (tcp-gated,
|
||
infallible), `rustls_config(&self) -> &rustls::ServerConfig`
|
||
(ungated); `TlsClientConfig::new(&ConnectionCredentials, alpn: &[u8])
|
||
-> Result<Self, TlsError>` (sync), `for_noq(self)`
|
||
(noq-gated, consuming), `into_rustls_config(self)` (consuming) —
|
||
every signature verified against the ADR-004 sketch verbatim
|
||
(src/server.rs:30,147,156,162; src/client.rs:29,52,60). The re-export
|
||
block is the documented surface (44 public items re-exported; no
|
||
`Config(String)` catch-all, no auth-layer types leaked).
|
||
|
||
2. **`TlsError` == ADR-002** — six variants, `#[non_exhaustive]`, no
|
||
string catch-all, typed `#[source]` chains (`#[from] io::Error` /
|
||
`rcgen::Error` / `rustls::Error` / `VerifierBuilderError` /
|
||
noq's `NoInitialCipherSuite`), string payloads only on `AcmeConfig`
|
||
(exactly the two config-mismatch cases). Compile-asserted by
|
||
`tests::tls_error_matches_the_adr_002_variant_set` + the noq-gated
|
||
`NoqWrap` existence test.
|
||
|
||
3. **Invariants == ADR-001/server.md/client.md** — all five server
|
||
invariants + the client 0-RTT half, each with a passing behavioral
|
||
test (not a compile assertion):
|
||
- `max_early_data_size = u32::MAX` per server path (unit:
|
||
`build_rustls_server_config_{raw_key,self_signed}_succeeds` +
|
||
ACME; integration: `tests/server_seams.rs` per variant +
|
||
`tests/invariant_pins.rs::server_paths_carry_max_early_data_and_alpn`).
|
||
- aws-lc-rs provider on all paths (`builder_with_provider`
|
||
everywhere; pinned by `tls_client_config_carries_aws_lc_rs_provider`).
|
||
- Nine-scheme list: exact-list vec equality pinned at both unit
|
||
(`accept_any_cert_verifier_supported_schemes_are_the_nine_pinned`)
|
||
and integration (`invariant_pins::nine_schemes_exact_list_pin`).
|
||
- `acme-tls/1` appended by the crate, ACME path only — asserted by
|
||
exact ALPN-list equality in `acme_lifecycle` (non-ACME paths
|
||
assert the bare list).
|
||
- Non-empty root store: `root_store_fallback_is_never_empty` +
|
||
`load_platform_root_cert_store_is_never_empty`.
|
||
- Client 0-RTT half: `enable_early_data == true` pinned
|
||
(`tls_client_config_pins_enable_early_data_true` + client_seams).
|
||
- Fail-closed structure: `None` installs `WebPkiServerVerifier`
|
||
(asserted via config Debug) — the structural fail-closed; no
|
||
fourth path. Verifier behavior
|
||
(offer/mandatory/empty-hints/accept-any) asserted; `RawKeyCertResolver`
|
||
`only_raw_public_keys() == true` asserted.
|
||
|
||
4. **Deltas vs the extraction source** — the src/ diff against
|
||
`crates/alknet-tls/src` + the moved alknet-core modules maps
|
||
entirely to ADR-pinned deltas: identity-type rewire (crate::identity
|
||
for alknet-core config.rs types), error mapping (every
|
||
`TlsError::Config(e.to_string())`/`Io`/`Cert` site → the typed
|
||
variants per ADR-002's refinement), `for_quinn` → `for_noq` (ADR-003),
|
||
borrowed server accessors (ADR-004), `unreachable!` →
|
||
`TlsError::AcmeConfig` (ADR-006), field rename `acme_state_handle` →
|
||
`acme_handle` (ADR-006), `SelfSigned`-client-path and
|
||
fingerprint-normalization docs carried over. Two un-pinned
|
||
divergences surfaced and were recorded as ADR amendments (below) —
|
||
**zero un-pinned divergences remain**.
|
||
|
||
5. **Feature hygiene** — `default = []` lean (verified: `cargo check`
|
||
pulls rustls + cert-material + tokio subset only); tokio is the
|
||
wasm-clean subset (`rt`, `sync`, `macros`, no `full`) in
|
||
`[dependencies]`, `full` only in dev-deps; `noq`/`tcp`/`acme` are
|
||
opt-in with `futures` correctly acme-gated (ADR-006). Doc comments
|
||
on the entire public API; zero inline `//` comments in non-test code
|
||
(the convention); zero `todo!`/`unimplemented!`/panics in library
|
||
code (all unwrap/expect sites are inside `#[cfg(test)]`).
|
||
|
||
6. **Docs sync** — two ADR amendments written:
|
||
- ADR-002 §Consequences: `VerifierBuild`'s source path is
|
||
`rustls::client::VerifierBuilderError` (the `rustls::webpki`
|
||
module is private at pinned 0.23.44; same type, public path).
|
||
- ADR-003 §Consequences: the `noq` dep gains `"aws-lc-rs"` in its
|
||
feature list (lockfile resolves noq-proto 1.3.0 where
|
||
`ServerConfig::with_crypto` is `#[cfg(any(aws-lc-rs, ring))]`;
|
||
the crate's explicit aws-lc-rs provider posture is unchanged).
|
||
Spec statuses advanced: overview.md / server.md / client.md
|
||
`Draft → Reviewed`; README.md carries the API-freeze lifecycle note.
|
||
|
||
### Findings + resolutions
|
||
|
||
| Finding | Severity | Resolution |
|
||
|---------|----------|------------|
|
||
| `VerifierBuilderError` path differs from ADR-002's sketch (`rustls::webpki` private at 0.23.44) | low | fixed at implementation (correct public path) + ADR-002 amendment |
|
||
| noq-proto 1.3's `with_crypto` needs a provider feature — ADR-003's TOML block was written against the 1.2 API | low | fixed (`features = ["rustls", "aws-lc-rs"]`) + ADR-003 amendment |
|
||
| scaffold's `acme` feature omitted the `dep:futures` half ADR-006 names | low | fixed (`acme = ["dep:rustls-acme", "dep:futures"]`) |
|
||
| `fingerprint_from_cert_der` empty-input: task text claimed `None` for empty input; extraction code always returns `Some` | low | port matches the code's actual behavior; deviation documented on the function's doc comment + task Notes |
|
||
| extracted `zeroize::ZeroizeOnDrop` impl on `Ed25519SecretKey` not ported (dep not in ADR-005's list) | low | accepted omission, recorded in tasks/port-identity-types.md — ed25519-dalek's default `zeroize` feature still zeroizes the inner key |
|
||
|
||
### Packaging
|
||
|
||
`cargo publish --dry-run --allow-dirty` passes: metadata complete
|
||
(description/keywords/categories/license), LICENSE-MIT + LICENSE-APACHE
|
||
included, exclude list keeps the SDD process docs out of the package.
|
||
|
||
### The frozen API surface
|
||
|
||
`lib.rs` re-exports (all-features view): `TlsServerConfig`,
|
||
`TlsClientConfig`, `TlsError`, `TlsIdentity`, `Ed25519SecretKey`,
|
||
`AcmeDirectory`, `ConnectionCredentials`, `RemoteIdentity`,
|
||
`fingerprint_from_cert_der`, `extract_ed25519_raw_key_from_spki`,
|
||
`load_cert_chain`, `load_private_key`, `Ed25519SigningKey`,
|
||
`build_client_auth`, `select_server_verifier`,
|
||
`load_platform_root_cert_store`, `build_rustls_server_config`,
|
||
`generate_self_signed_cert`, `AcceptAnyCertVerifier`,
|
||
`RawKeyCertResolver`, `SelfSignedCert`, `NoClientCertResolver`,
|
||
`RawKeyClientCertResolver`, `FingerprintPinVerifier` — plus the eight
|
||
`pub mod` declarations (ADR-006: modules pub for discoverability; the
|
||
re-export block is the documented surface). |