generation 5: review-impl — spec-conformance gate passed, API frozen
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.
This commit is contained in:
+121
-9
@@ -1,7 +1,7 @@
|
||||
---
|
||||
id: review-impl
|
||||
name: Review alktls v1 implementation for spec conformance (pre-rewrite gate)
|
||||
status: pending
|
||||
status: completed
|
||||
depends_on: [integration-suite]
|
||||
scope: moderate
|
||||
risk: low
|
||||
@@ -50,20 +50,20 @@ public API surface freezes here (the rewrite compiles against it).
|
||||
|
||||
## Verification
|
||||
|
||||
- [ ] The full checklist passes with findings recorded
|
||||
- [ ] `cargo test`, `cargo test --all-features`,
|
||||
- [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
|
||||
- [ ] `cargo publish --dry-run --allow-dirty` passes (packaging
|
||||
- [x] `cargo publish --dry-run --allow-dirty` passes (packaging
|
||||
readiness: metadata, license files, exclude list)
|
||||
- [ ] Spec statuses advanced where the gate passes
|
||||
- [x] Spec statuses advanced where the gate passes
|
||||
|
||||
## Acceptance Criteria
|
||||
|
||||
- [ ] Zero un-pinned divergences from the architecture docs
|
||||
- [ ] The API surface is declared frozen for the rewrite (README
|
||||
- [x] Zero un-pinned divergences from the architecture docs
|
||||
- [x] The API surface is declared frozen for the rewrite (README
|
||||
lifecycle note)
|
||||
- [ ] Findings + resolutions documented in the task Summary
|
||||
- [x] Findings + resolutions documented in the task Summary
|
||||
|
||||
## References
|
||||
|
||||
@@ -77,4 +77,116 @@ public API surface freezes here (the rewrite compiles against it).
|
||||
|
||||
## Summary
|
||||
|
||||
> Agent fills this on completion.
|
||||
> 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).
|
||||
Reference in New Issue
Block a user