diff --git a/crates/alknet-call/src/protocol/adapter.rs b/crates/alknet-call/src/protocol/adapter.rs index fa96cf9..712284c 100644 --- a/crates/alknet-call/src/protocol/adapter.rs +++ b/crates/alknet-call/src/protocol/adapter.rs @@ -16,10 +16,11 @@ use serde_json::Value; use tokio::task::JoinHandle; use tracing::{debug, warn}; +use super::abort::AbortCascade; use super::connection::CallConnection; use super::wire::{ CallError, EventEnvelope, FrameFramedReader, FrameFramedWriter, ResponseEnvelope, - EVENT_REQUESTED, + EVENT_ABORTED, EVENT_REQUESTED, }; use crate::registry::context::{AbortPolicy, OperationContext, ScopedOperationEnv}; use crate::registry::env::{CompositeOperationEnv, LocalOperationEnv, OperationEnv}; @@ -207,22 +208,34 @@ impl CallAdapter { } }; - if envelope.r#type != EVENT_REQUESTED { - debug!(event_type = %envelope.r#type, id = %envelope.id, "ignoring non-requested event on inbound stream"); - continue; - } + match envelope.r#type.as_str() { + EVENT_REQUESTED => { + let request_id = envelope.id.clone(); + let payload = envelope.payload.clone(); - let request_id = envelope.id.clone(); - let payload = envelope.payload.clone(); + let response = self + .dispatch_requested(&connection, request_id.clone(), payload) + .await; - let response = self - .dispatch_requested(&connection, request_id.clone(), payload) - .await; - - let event: EventEnvelope = response.into(); - if let Err(err) = writer.write_frame(&event).await { - warn!(error = %err, "failed to write response frame; closing stream"); - break; + let event: EventEnvelope = response.into(); + if let Err(err) = writer.write_frame(&event).await { + warn!(error = %err, "failed to write response frame; closing stream"); + break; + } + } + EVENT_ABORTED => { + let request_id = envelope.id.clone(); + let mut pending = connection.pending().lock(); + let mut cascade = AbortCascade::new(&mut pending); + let aborted = cascade.cascade_abort(&request_id, AbortPolicy::AbortDependents); + pending.handle_aborted(&request_id); + if !aborted.is_empty() { + debug!(count = aborted.len(), "abort cascade evicted descendants"); + } + } + other => { + debug!(event_type = %other, id = %envelope.id, "ignoring non-requested/non-aborted event on inbound stream"); + } } } } @@ -312,6 +325,7 @@ mod tests { use std::collections::HashMap; use std::net::{IpAddr, Ipv4Addr, SocketAddr}; use std::sync::Mutex as StdMutex; + use std::time::{Duration, Instant}; struct StaticIdentityProvider { tokens: StdMutex>, @@ -946,4 +960,92 @@ mod tests { other => panic!("expected NOT_FOUND, got {other:?}"), } } + + fn encode_frame(envelope: &EventEnvelope) -> Vec { + let body = serde_json::to_vec(envelope).unwrap(); + let mut buf = (body.len() as u32).to_be_bytes().to_vec(); + buf.extend_from_slice(&body); + buf + } + + #[tokio::test] + async fn handle_stream_aborted_cascades_parent_and_child() { + let registry = registry_with( + "parent/run", + Visibility::External, + AccessControl::default(), + echo_handler(), + ); + let provider: Arc = Arc::new(StaticIdentityProvider::new()); + let adapter = CallAdapter::new(registry, provider); + let conn = Arc::new(CallConnection::new(stub_connection())); + + { + let mut pending = conn.pending().lock(); + pending.register_call( + "parent-1".to_string(), + Instant::now() + Duration::from_secs(30), + None, + ); + pending.register_call( + "child-1".to_string(), + Instant::now() + Duration::from_secs(30), + Some("parent-1".to_string()), + ); + } + + let frame = encode_frame(&EventEnvelope::aborted("parent-1")); + let recv = tokio::io::BufReader::new(std::io::Cursor::new(frame)); + let (send, _recv_sink) = tokio::io::duplex(64); + let send = alknet_core::types::SendStream::from_mock(send); + let recv = alknet_core::types::RecvStream::from_mock(recv); + + adapter.handle_stream(conn.clone(), send, recv).await; + + let pending = conn.pending().lock(); + assert!( + !pending.contains("parent-1"), + "parent entry must be removed after abort" + ); + assert!( + !pending.contains("child-1"), + "child entry must be removed by cascade" + ); + } + + #[tokio::test] + async fn handle_stream_aborted_unknown_request_id_is_noop() { + let registry = registry_with( + "parent/run", + Visibility::External, + AccessControl::default(), + echo_handler(), + ); + let provider: Arc = Arc::new(StaticIdentityProvider::new()); + let adapter = CallAdapter::new(registry, provider); + let conn = Arc::new(CallConnection::new(stub_connection())); + + { + let mut pending = conn.pending().lock(); + pending.register_call( + "unrelated-1".to_string(), + Instant::now() + Duration::from_secs(30), + None, + ); + } + + let frame = encode_frame(&EventEnvelope::aborted("does-not-exist")); + let recv = tokio::io::BufReader::new(std::io::Cursor::new(frame)); + let (send, _recv_sink) = tokio::io::duplex(64); + let send = alknet_core::types::SendStream::from_mock(send); + let recv = alknet_core::types::RecvStream::from_mock(recv); + + adapter.handle_stream(conn.clone(), send, recv).await; + + let pending = conn.pending().lock(); + assert!( + pending.contains("unrelated-1"), + "unrelated entry must survive abort of unknown id" + ); + } } diff --git a/crates/alknet-core/src/config.rs b/crates/alknet-core/src/config.rs index 6821fba..1142c56 100644 --- a/crates/alknet-core/src/config.rs +++ b/crates/alknet-core/src/config.rs @@ -318,4 +318,52 @@ mod tests { let s = format!("{e}"); assert!(s.starts_with("tls config error:")); } + + #[test] + fn resolve_api_key_returns_empty_resources() { + use sha2::{Digest, Sha256}; + let token = "alk_test_secret"; + let mut hasher = Sha256::new(); + hasher.update(token.as_bytes()); + let hash = format!("sha256:{}", hex::encode(hasher.finalize())); + + let entry = ApiKeyEntry { + prefix: "alk_tes".to_string(), + hash, + scopes: vec!["admin".to_string()], + description: "test key".to_string(), + expires_at: None, + }; + let policy = AuthPolicy { + authorized_fingerprints: HashSet::new(), + api_keys: vec![entry], + }; + + let identity = policy.resolve_api_key(token); + assert!(identity.is_some(), "api key with matching prefix and hash should resolve"); + let identity = identity.unwrap(); + assert_eq!(identity.id, "alk_tes"); + assert_eq!(identity.scopes, vec!["admin"]); + assert!( + identity.resources.is_empty(), + "token-resolved identities must have empty resources (Option B — scopes only)" + ); + } + + #[test] + fn resolve_identity_from_fingerprint_returns_empty_resources() { + let policy = AuthPolicy { + authorized_fingerprints: HashSet::from(["SHA256:known".to_string()]), + api_keys: vec![], + }; + + let identity = policy + .resolve_identity_from_fingerprint("SHA256:known") + .expect("known fingerprint should resolve"); + assert_eq!(identity.id, "SHA256:known"); + assert!( + identity.resources.is_empty(), + "fingerprint-resolved identities must have empty resources (Option B — scopes only)" + ); + } } diff --git a/crates/alknet-core/src/endpoint.rs b/crates/alknet-core/src/endpoint.rs index 0f0aa02..2e25d19 100644 --- a/crates/alknet-core/src/endpoint.rs +++ b/crates/alknet-core/src/endpoint.rs @@ -303,7 +303,8 @@ fn dispatch_quinn( }; let remote_addr = Some(connection.remote_address()); - let auth = build_auth_context(&alpn, remote_addr, None, identity_provider); + let fingerprint = extract_quinn_client_fingerprint(&connection); + let auth = build_auth_context(&alpn, remote_addr, fingerprint, identity_provider); let conn = Connection::from_quinn_with_alpn(connection, alpn.clone()); tokio::spawn(async move { if let Err(e) = handler.handle(conn, &auth).await { @@ -325,6 +326,25 @@ fn extract_quinn_alpn(connection: &quinn::Connection) -> Vec { Vec::new() } +#[cfg(feature = "quinn")] +fn extract_quinn_client_fingerprint(connection: &quinn::Connection) -> Option { + let identity = connection.peer_identity()?; + let certs = identity + .downcast::>() + .ok()?; + let leaf = certs.first()?; + fingerprint_from_cert_der(leaf.as_ref()) +} + +#[cfg(any(feature = "quinn", feature = "iroh"))] +fn fingerprint_from_cert_der(cert_der: &[u8]) -> Option { + use sha2::{Digest, Sha256}; + let mut hasher = Sha256::new(); + hasher.update(cert_der); + let digest = hasher.finalize(); + Some(format!("SHA256:{}", hex::encode(digest))) +} + #[cfg(feature = "iroh")] async fn run_iroh_accept_loop( iroh: iroh::Endpoint, @@ -393,7 +413,8 @@ fn dispatch_iroh( } }; - let auth = build_auth_context(&alpn, None, None, identity_provider); + let fingerprint = extract_iroh_client_fingerprint(&connection); + let auth = build_auth_context(&alpn, None, fingerprint, identity_provider); let conn = Connection::from_iroh(connection); tokio::spawn(async move { if let Err(e) = handler.handle(conn, &auth).await { @@ -402,6 +423,12 @@ fn dispatch_iroh( }); } +#[cfg(feature = "iroh")] +fn extract_iroh_client_fingerprint(connection: &iroh::endpoint::Connection) -> Option { + let node_id = connection.remote_node_id().ok()?; + Some(format!("ed25519:{}", node_id)) +} + #[cfg(any(feature = "quinn", feature = "iroh"))] fn build_auth_context( alpn: &[u8], @@ -979,4 +1006,40 @@ mod tests { let unknown = registry.get(b"alknet/unknown"); assert!(unknown.is_none(), "unknown ALPN has no handler"); } + + #[cfg(any(feature = "quinn", feature = "iroh"))] + #[test] + fn fingerprint_from_cert_der_produces_sha256_hex_format() { + let cert_der = b"fake-leaf-cert-der-bytes"; + let fp = fingerprint_from_cert_der(cert_der).expect("non-empty cert produces fingerprint"); + assert!( + fp.starts_with("SHA256:"), + "fingerprint must be SHA256-prefixed, got: {fp}" + ); + let hex_part = &fp["SHA256:".len()..]; + assert_eq!( + hex_part.len(), + 64, + "hex digest must be 64 chars (32 bytes), got: {fp}" + ); + assert!( + hex_part.chars().all(|c| c.is_ascii_hexdigit()), + "hex part must be lowercase hex, got: {fp}" + ); + + use sha2::{Digest, Sha256}; + let mut hasher = Sha256::new(); + hasher.update(cert_der); + let expected = format!("SHA256:{}", hex::encode(hasher.finalize())); + assert_eq!(fp, expected, "fingerprint must match SHA-256 of cert DER"); + } + + #[cfg(any(feature = "quinn", feature = "iroh"))] + #[test] + fn fingerprint_from_cert_der_deterministic() { + let cert = b"some-cert"; + let a = fingerprint_from_cert_der(cert).unwrap(); + let b = fingerprint_from_cert_der(cert).unwrap(); + assert_eq!(a, b, "same cert DER must produce same fingerprint"); + } } diff --git a/crates/alknet-vault/src/mnemonic.rs b/crates/alknet-vault/src/mnemonic.rs index c6968d5..e57bcf7 100644 --- a/crates/alknet-vault/src/mnemonic.rs +++ b/crates/alknet-vault/src/mnemonic.rs @@ -32,12 +32,19 @@ impl From for bip39::Language { /// /// Wraps the `bip39` crate's `Mnemonic` type and provides seed derivation. /// The internal phrase is zeroized on drop. -#[derive(Debug)] pub struct Mnemonic { inner: Bip39Mnemonic, phrase: String, } +impl std::fmt::Debug for Mnemonic { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + f.debug_struct("Mnemonic") + .field("phrase", &"[REDACTED]") + .finish() + } +} + impl Mnemonic { /// Generate a new random mnemonic with the given word count. /// @@ -163,4 +170,20 @@ mod tests { assert_eq!(seed.len(), 64); assert!(!seed.is_empty()); } + + #[test] + fn test_mnemonic_debug_redacts_phrase() { + let mnemonic = Mnemonic::generate(24).unwrap(); + let debug_output = format!("{:?}", mnemonic); + assert!( + debug_output.contains("[REDACTED]"), + "Debug must show [REDACTED] for phrase, got: {debug_output}" + ); + for word in mnemonic.phrase().split_whitespace() { + assert!( + !debug_output.contains(word), + "Debug must not leak phrase word '{word}', got: {debug_output}" + ); + } + } } diff --git a/docs/architecture/crates/core/auth.md b/docs/architecture/crates/core/auth.md index 482ceec..e67922a 100644 --- a/docs/architecture/crates/core/auth.md +++ b/docs/architecture/crates/core/auth.md @@ -150,10 +150,41 @@ The "Config" prefix indicates that identities are resolved from configuration (a How it resolves: - **Fingerprint**: Look up in `DynamicConfig::auth::authorized_keys_fingerprints`. If found, return `Identity { id: fingerprint, scopes: ["relay:connect"], resources: {} }`. -- **Token**: Parse as UTF-8. If it starts with `alk_`, look up in `DynamicConfig::auth::api_keys` by prefix match + SHA-256 hash. If found and not expired, return `Identity { id: prefix, scopes: entry.scopes, resources: entry.resources }`. +- **Token**: Parse as UTF-8. If it starts with `alk_`, look up in `DynamicConfig::auth::api_keys` by prefix match + SHA-256 hash. If found and not expired, return `Identity { id: prefix, scopes: entry.scopes, resources: {} }`. + +> **Resource-scoped ACLs and external identities.** `Identity.resources` is +> populated only by the composition path (`CompositionAuthority::as_identity`, +> ADR-015/022) — never by token or fingerprint resolvers. API keys and +> fingerprints grant **scopes only**; resource-scoped access is an +> internal-composition concern. An `OperationSpec` that declares +> `resource_type`/`resource_action` will return `FORBIDDEN` when the caller +> authenticated via token or fingerprint, because `Identity.resources` is +> empty. This is a documented limitation, not a bug: if a future crate needs +> per-key resource binding, it must earn a dedicated ADR that adds a +> `resources` field to `ApiKeyEntry` and the fingerprint config path, rather +> than silently widening the external-auth contract. Changes to `DynamicConfig` via `ConfigReloadHandle` are reflected immediately — `ConfigIdentityProvider` reads from `ArcSwap` on every call. +### Fingerprint string format + +`tls_client_fingerprint` and `authorized_fingerprints` use a prefixed-hex +format. The prefix identifies the key type; the body is the hex-encoded +hash or raw key bytes. `AuthPolicy::resolve_identity_from_fingerprint` +does a literal `HashSet::contains()` — no normalization — so the extractor +and the operator config must use the same format. + +| Transport | Source | Format | +|-----------|--------|--------| +| quinn (X.509) | leaf client cert DER | `SHA256:` | +| iroh (raw Ed25519) | peer `NodeId` | `ed25519:` | + +When no client cert is presented (the current default — server uses +`with_no_client_auth()`), the fingerprint is `None` and identity remains +unresolved at the endpoint layer. A follow-up task will switch the server +config to request-but-not-require client certs so fingerprints flow for +peers that present them. + ## Resolution Flow ### Endpoint-level (before `handle()`) diff --git a/docs/reviews/004-post-implementation-sanity-check.md b/docs/reviews/004-post-implementation-sanity-check.md index b563248..b44cb36 100644 --- a/docs/reviews/004-post-implementation-sanity-check.md +++ b/docs/reviews/004-post-implementation-sanity-check.md @@ -1,6 +1,6 @@ --- -status: open -last_updated: 2026-06-23 +status: resolved +last_updated: 2026-06-24 reviewed_artifacts: - crates/alknet-vault/src/{lib,cache,derivation,encryption,ethereum,mnemonic,protocol,service}.rs - crates/alknet-core/src/{lib,auth,config,endpoint,types}.rs @@ -566,4 +566,44 @@ flagged them as high-risk, and the implementation got them right: Review #004 is open. Zero critical findings; four warnings, all local; five suggestions. The implementation is sound, the spec drift is bounded, and the one wiring gap (W1) has all the hard logic already written and tested — it -just needs to be called. \ No newline at end of file +just needs to be called. + +--- + +## Resolution (2026-06-24) + +All four warnings (W1–W4) resolved. Workspace green: +`cargo build --workspace --all-features`, `cargo test --workspace +--all-features` (326 tests, 0 failures), `cargo clippy --workspace +--all-features --all-targets` (0 warnings). + +- **W1 (abort cascade wiring)**: `CallAdapter::handle_stream` now + matches `EVENT_ABORTED`, invokes `AbortCascade::cascade_abort` with + `AbortPolicy::AbortDependents`, and aborts the root. No descendant + `call.aborted` frames sent on the wire (ADR-016 Decision 2). Two + integration tests cover the cascade + unknown-id no-op paths. + (`tasks/call/protocol/abort-cascade-wiring.md` → completed) + +- **W2 (fingerprint extraction)**: `dispatch_quinn` extracts the leaf + client cert DER via `peer_identity()` → `SHA256:`; `dispatch_iroh` + extracts the peer `NodeId` → `ed25519:`. Fingerprint format + documented in `auth.md`. Server config still uses + `with_no_client_auth()` — extraction is a safe no-op until the + follow-up task `core/endpoint-request-client-cert` switches to + request-but-don't-require. Two unit tests cover fingerprint format + + determinism. + (`tasks/core/endpoint-client-fingerprint.md` → completed) + +- **W3 (Mnemonic Debug redaction)**: `#[derive(Debug)]` replaced with + manual redacting impl matching the `DerivedKey` pattern. `Seed` + confirmed to have no `Debug` impl. Test asserts no phrase word leaks. + (`tasks/vault/mnemonic-debug-redaction.md` → completed) + +- **W4 (ApiKeyEntry resources)**: Option B chosen — spec corrected to + drop `entry.resources`; `auth.md` now documents that external + identities (token/fingerprint) grant scopes only, resource-scoped + ACLs are a composition-internal concern (ADR-015/022). Two tests + confirm both resolvers return empty resources. + (`tasks/core/auth-apikey-resources.md` → completed) + +S1–S5 (suggestions) remain opportunistic; not gated by this review. \ No newline at end of file diff --git a/tasks/call/protocol/abort-cascade-wiring.md b/tasks/call/protocol/abort-cascade-wiring.md index 8af80a2..9438446 100644 --- a/tasks/call/protocol/abort-cascade-wiring.md +++ b/tasks/call/protocol/abort-cascade-wiring.md @@ -1,7 +1,7 @@ --- id: call/protocol/abort-cascade-wiring name: Wire AbortCascade into CallAdapter inbound event path (ADR-016) -status: pending +status: completed depends_on: [call/protocol/abort-cascade] scope: narrow risk: medium @@ -127,4 +127,14 @@ frame actually reaches `cascade_abort`. > criteria likewise omitted "inbound `call.aborted` triggers cascade." > This task closes that integration gap — all the hard logic already > exists and is tested; this task adds the ~30-line bolt and the one -> integration test that would have caught the gap. \ No newline at end of file +> integration test that would have caught the gap. + +## Summary + +`handle_stream` now matches `EVENT_ABORTED` → invokes +`AbortCascade::cascade_abort` with `AbortPolicy::AbortDependents`, then +aborts the root. Non-requested/non-aborted events still log at `debug!`. +No descendant `call.aborted` frames sent on the wire. Two integration +tests: cascade removes parent + child from `PendingRequestMap`; unknown +request_id is a no-op. `cargo test -p alknet-call` (161 tests) and +clippy clean. \ No newline at end of file diff --git a/tasks/core/auth-apikey-resources.md b/tasks/core/auth-apikey-resources.md index b6a6ddf..ccba5b1 100644 --- a/tasks/core/auth-apikey-resources.md +++ b/tasks/core/auth-apikey-resources.md @@ -1,7 +1,7 @@ --- id: core/auth-apikey-resources name: Reconcile ApiKeyEntry.resources — add field to type and populate in resolve_api_key, or drop from spec -status: pending +status: completed depends_on: [] scope: narrow risk: low @@ -114,4 +114,17 @@ applied to handler-internal composition identities > decision first, then implement. If the decision is A and the > implementation is more than ~30 lines, split a follow-up > `level: implementation` task (`core/auth-apikey-resources-impl`) -> depending on this one. \ No newline at end of file +> depending on this one. + +## Summary + +Decision: **Option B** — dropped `entry.resources` from the spec. +Rationale: `Identity.resources` is populated only by +`CompositionAuthority::as_identity` (the composition path, ADR-015/022). +All architecture examples use scope-based ACLs for external identities +(`fs:read`, `vastai:query`, `llm:call`). Adding a second +resource-population path for API keys would muddy the external/internal +separation without a demonstrated downstream need. `auth.md:153` +corrected to `resources: {}`; documented limitation added. Two tests +confirm both `resolve_api_key` and `resolve_identity_from_fingerprint` +return empty resources. `cargo test -p alknet-core` and clippy clean. \ No newline at end of file diff --git a/tasks/core/endpoint-client-fingerprint.md b/tasks/core/endpoint-client-fingerprint.md index 1bd7202..b5771d6 100644 --- a/tasks/core/endpoint-client-fingerprint.md +++ b/tasks/core/endpoint-client-fingerprint.md @@ -1,7 +1,7 @@ --- id: core/endpoint-client-fingerprint name: Extract TLS client certificate fingerprint in endpoint dispatch (ADR-004) -status: pending +status: completed depends_on: [] scope: narrow risk: medium @@ -115,4 +115,17 @@ in the codebase before committing. > client certs. The extraction code is correct either way — it returns > `None` when no cert was presented, which is the current behavior, so > landing extraction first is a safe no-op until the server config -> changes. \ No newline at end of file +> changes. + +## Summary + +Added `extract_quinn_client_fingerprint` (leaf cert DER → `SHA256:` +via `peer_identity()` downcast) and `extract_iroh_client_fingerprint` +(peer `NodeId` → `ed25519:`). Both dispatch functions now pass the +extracted fingerprint to `build_auth_context`. Fingerprint format +documented in `auth.md` (table: quinn X.509 vs iroh raw Ed25519). +Server config still uses `with_no_client_auth()` — extraction is a safe +no-op. Follow-up task `core/endpoint-request-client-cert` created for the +server config change. Two unit tests cover fingerprint format + +determinism. `cargo test -p alknet-core --all-features` (59 tests) and +clippy clean. \ No newline at end of file diff --git a/tasks/core/endpoint-request-client-cert.md b/tasks/core/endpoint-request-client-cert.md new file mode 100644 index 0000000..2b43392 --- /dev/null +++ b/tasks/core/endpoint-request-client-cert.md @@ -0,0 +1,90 @@ +--- +id: core/endpoint-request-client-cert +name: Switch rustls ServerConfig from with_no_client_auth to request-but-don't-require client certs +status: pending +depends_on: [core/endpoint-client-fingerprint] +scope: narrow +risk: medium +impact: component +level: implementation +--- + +## Description + +`core/endpoint-client-fingerprint` landed the extraction logic: when a +client certificate *is* presented, `dispatch_quinn` / `dispatch_iroh` +extract the fingerprint and populate `AuthContext`. However, the server +still builds `rustls::ServerConfig` with `with_no_client_auth()` in all +three `TlsIdentity` branches (`endpoint.rs:477`, `490`, `501`), so the +server never *requests* a client cert. Extraction is a safe no-op until +this task changes the server-side TLS config. + +This follow-up switches from `with_no_client_auth()` to a +request-but-don't-require mode so that peers presenting a client cert +(X.509 or RFC 7250 raw Ed25519 key) flow through the extraction path +landed in the predecessor task, while peers without a cert still connect +without regression. + +### Design decision: how to request-but-not-require + +rustls does not have a direct `with_optional_client_auth()` builder. +The standard approach is: + +1. Build the config with `.with_client_auth(verifier)` where `verifier` + is a custom `ServerCertVerifier` that accepts any presentation (returns + `Ok(Certified::yes())` when a cert is presented, `Ok(Certified::no())` + when none is presented — rustls 0.23's `WebPkiServerVerifier` cannot + be used directly for optional auth). +2. Alternatively, use `rustls::server::WebPkiServerVerifier` with a + `NoClientAuth` fallback — check the exact rustls API available in the + pinned version before implementing. + +Read the rustls API docs for the pinned version +(`rustls::server::ServerConfig::builder_with_provider`) and confirm the +correct verifier construction. The key property: a peer *may* present a +cert, and if it does, `peer_identity()` returns it; if it doesn't, the +connection still succeeds. + +### iroh path + +iroh's `Endpoint` builder uses its own TLS session internally. For the +raw-key path (`TlsIdentity::RawKey`), iroh already advertises +`only_raw_public_keys()` via `RawKeyCertResolver` — the server-side half +of RFC 7250. The client-side presentation is set by the client's +`rustls::ClientConfig`, not the server. So the iroh path may already +receive peer identities when the client is an iroh node (the `NodeId` is +always in the TLS cert). Verify: does `Connection::remote_node_id()` +already work for iroh connections today, or does it require the server to +request client certs? If iroh always presents a cert (raw-key mode), no +server-side change is needed for the iroh path — only quinn/X.509 needs +this task. Confirm before implementing. + +## Acceptance Criteria + +- [ ] `build_rustls_server_config` uses request-but-don't-require client auth (not `with_no_client_auth()`) for at least the X.509 path +- [ ] Peer presenting a client cert: `peer_identity()` returns the cert chain → fingerprint extraction works end-to-end +- [ ] Peer without a client cert: connection still succeeds, `tls_client_fingerprint` is `None` (no regression) +- [ ] iroh path: confirm whether a server-side change is needed; if yes, apply it; if no, document why +- [ ] Integration test: quinn endpoint with a client that presents a cert → `AuthContext.tls_client_fingerprint` is `Some(SHA256:...)` +- [ ] Integration test: quinn endpoint with a client that presents no cert → `AuthContext.tls_client_fingerprint` is `None` and connection succeeds +- [ ] `cargo test -p alknet-core --all-features` succeeds +- [ ] `cargo clippy -p alknet-core --all-features --all-targets` succeeds with no warnings +- [ ] `auth.md` updated: server-config decision documented (request-but-don't-require, not no-client-auth) + +## References + +- tasks/core/endpoint-client-fingerprint.md — predecessor task (landed extraction, deferred this config change) +- crates/alknet-core/src/endpoint.rs:477, 490, 501 — the three `with_no_client_auth()` sites +- crates/alknet-core/src/endpoint.rs — `extract_quinn_client_fingerprint` / `extract_iroh_client_fingerprint` (already landed, waiting for certs to flow) +- docs/architecture/crates/core/auth.md — fingerprint format table and endpoint-level resolution flow +- docs/architecture/decisions/004-auth-as-shared-core.md — ADR-004 (hybrid resolution) +- docs/architecture/open-questions.md — OQ-12 (TLS identity provisioning) + +## Notes + +> Split from `core/endpoint-client-fingerprint` per the task's own +> suggestion: extraction is correct either way (returns `None` when no +> cert), so landing it first is a safe no-op. This task is the +> behavioral change that makes fingerprints actually flow. The risk is +> medium because it alters the TLS handshake for every connection — +> ensure the no-cert-peer case has explicit test coverage. \ No newline at end of file diff --git a/tasks/review-post-impl-fixes.md b/tasks/review-post-impl-fixes.md index a3bdcba..06ad016 100644 --- a/tasks/review-post-impl-fixes.md +++ b/tasks/review-post-impl-fixes.md @@ -1,7 +1,7 @@ --- id: review-post-impl-fixes name: Review the four post-implementation sanity-check #004 fixes for spec conformance -status: pending +status: completed depends_on: [call/protocol/abort-cascade-wiring, core/endpoint-client-fingerprint, vault/mnemonic-debug-redaction, core/auth-apikey-resources] scope: moderate risk: low @@ -93,4 +93,23 @@ check.md`, does not introduce new spec drift, and is adequately tested. > at the end of a fix batch, with `scope: moderate`, `risk: low`, > `impact: phase`. It does not need to re-derive the findings — review > #004 already did that work. It only needs to confirm the fixes land -> correctly and the workspace stays green. \ No newline at end of file +> correctly and the workspace stays green. + +## Summary + +All four fixes verified against acceptance criteria: +- W1: `handle_stream` handles `EVENT_ABORTED`, cascades with + `AbortDependents`, no descendant frames on wire, root removed, two + integration tests pass. +- W2: both dispatch paths extract fingerprints, format documented in + `auth.md`, no-cert case returns `None` (no regression), server-config + change deferred to `core/endpoint-request-client-cert`. +- W3: `Mnemonic` has manual redacting `Debug`, `Seed` has no `Debug`, + redaction test passes. +- W4: Option B — spec corrected, limitation documented, both resolvers + return empty resources, tests pass. + +Workspace green: `cargo build --workspace --all-features` ✓, `cargo test +--workspace --all-features` (326 tests, 0 failures) ✓, `cargo clippy +--workspace --all-features --all-targets` (0 warnings) ✓. Review #004 +status updated to `resolved`. \ No newline at end of file diff --git a/tasks/vault/mnemonic-debug-redaction.md b/tasks/vault/mnemonic-debug-redaction.md index ba63b1e..722d43f 100644 --- a/tasks/vault/mnemonic-debug-redaction.md +++ b/tasks/vault/mnemonic-debug-redaction.md @@ -1,7 +1,7 @@ --- id: vault/mnemonic-debug-redaction name: Replace Mnemonic derive(Debug) with redacting impl to prevent seed phrase leak -status: pending +status: completed depends_on: [] scope: single risk: low @@ -107,4 +107,12 @@ fn test_mnemonic_debug_redacts_phrase() { > Small fix, but eliminates a latent root-of-trust leak. The same > pattern (custom redacting `Debug`) is already established in three -> other places in this codebase — this task brings `Mnemonic` in line. \ No newline at end of file +> other places in this codebase — this task brings `Mnemonic` in line. + +## Summary + +Replaced `#[derive(Debug)]` on `Mnemonic` with a manual redacting impl +(`phrase: "[REDACTED]"`). Added `test_mnemonic_debug_redacts_phrase` +asserting no phrase word appears in `format!("{:?}", mnemonic)`. +Confirmed `Seed` has no `Debug` impl (derives only `Clone, Zeroize`). +`cargo test -p alknet-vault` and clippy clean. \ No newline at end of file