feat(delegate): host-side secret-key enumeration (#4355)

[iduartgomez]

Status: Ready for review. freenet-stdlib 0.8.2 is published to crates.io (freenet/freenet-stdlib#80 merged); Cargo.toml/Cargo.lock resolve it from crates.io with a checksum. There is no [patch.crates-io] / path-dep coupling — stdlib-first release policy honored.

Problem

The delegate secret-storage API is opaque key→value with no enumeration, so apps storing dynamic key families (e.g. River's room:<id>) cannot discover stored keys — which strands them across a delegate-WASM rebuild (new delegate key = empty storage, migration can only probe a hardcoded set). See #4355 and freenet/river#345.

Solution

Host-side handling for the new stdlib list_secrets operation (the API surface is in freenet/freenet-stdlib#80):

• Two new hostcalls under freenet_delegate_secrets, mirroring the proven get_secret two-call (len-then-fill) ABI pattern, with refresh_mem_addr_from_caller + validate_and_compute_ptr per the contracts rules.
• A new encrypted .keys registry alongside the secret store, reusing the scope DEK with the same [VERSION_V1][nonce][AEAD] layout, per-write nonce, owner-only perms, and tmp+rename discipline (no new plaintext-at-rest), count-capped at 4096/scope to bound the amplification class.
• Optional prefix filtering; returns raw key bytes (not hashes), deduplicated.

The registry is best-effort and independent of the value path — written strictly after the durable value+index commit, so it can never lose a secret VALUE. Reads are fail-safe: a transient IO error or a corrupt/undecryptable .keys blob leaves the on-disk registry untouched (rather than overwriting it from empty), so a momentary hiccup cannot permanently shrink the enumerable key set.

Truncation note: a key family larger than the 4096/scope cap enumerates a bounded, truncated (non-evicting) view — over-cap keys are still stored and readable as values but are not listed. A migration consumer of a >4096-key family must not assume the list is complete.

Testing

Store layer: list_secret_keys_enumerates_and_filters, list_secret_keys_scope_isolation, list_secret_keys_at_cap (cap boundary), list_secret_keys_persist_across_restart (Local) + list_secret_keys_user_scope_persist_across_restart (User-scope DEK path), key_registry_tmp_path_is_correct, plus empty-store / prefix cases.

Data-integrity (fail-safe) regressions: corrupt_registry_does_not_shrink_enumerable_set and unreadable_registry_does_not_shrink_enumerable_set — both assert the secret VALUE write still succeeds AND the existing registry is preserved on a read error (both fail before the fail-safe fix).

Host-ABI: list_secrets_abi_tests exercises the list_secrets / list_secrets_len parameter-validation arms (negative out_len/prefix_len → ERR_INVALID_PARAM, outside-process() → ERR_NOT_IN_PROCESS) with no real WASM module. End-to-end through a real delegate WASM remains deferred (the test-delegate fixture pins published stdlib and cannot drive the new ABI yet) — tracked as a follow-up.

Fixes

Fixes #4355 (core half; stdlib half = freenet/freenet-stdlib#80)

[github-actions]

I have all the information I need. Let me compile the review.

---

Rule Review: No blocking issues; two INFO observations

Rules checked: git-workflow.md, code-style.md, testing.md, contracts.md
Files reviewed: 6 (Cargo.lock, Cargo.toml, wasmtime_engine.rs, native_api.rs, secrets_store.rs, docs/secrets-at-rest.md)

---

Warnings

None.

All checked rules pass:

• Conventional commits ✓ (feat:, fix:, build: prefixes, subject ≤72 chars)
• Both fix: commits have regression tests: key_registry_tmp_path_is_correct (tmp-path fix) and corrupt_registry_does_not_shrink_enumerable_set / unreadable_registry_does_not_shrink_enumerable_set (fail-safe fix) ✓
• freenet-stdlib = "0.8.2" is a published crates.io release — no git override, no path dep ✓
• Both new func_wrap registrations in wasmtime_engine.rs call refresh_mem_addr_from_caller before delegating to native_api ✓
• No .unwrap() in production code (only unwrap_or_else / unwrap_or_default) ✓
• No fire-and-forget spawns, no retry loops, no unguarded biased; ✓
• GC/cleanup logic: no exemptions introduced ✓
• Documentation updated in docs/secrets-at-rest.md ✓

---

Info

• crates/core/src/wasm_runtime/secrets_store.rs:1238 — write_key_registry calls XChaCha20Poly1305::generate_nonce(&mut OsRng) without a load-bearing comment explaining why OsRng is the correct choice over GlobalRng. Per code-style.md, the helper function must include a comment citing the cryptographic-key-material reason. The existing store_secret call site (line 819) has this pattern (// reuse would be catastrophic (keystream XOR + Poly1305 key recovery)) and ensure_kek_loaded (line 497–502) explicitly cites the code-style.md exception. The write_key_registry docstring mentions "per-write random nonce" but stops short of the required cryptographic justification. (rule: code-style.md — OsRng exception)

• crates/core/src/wasm_runtime/native_api.rs — list_secrets's ERR_BUFFER_TOO_SMALL and memory-write success paths are not exercised at the unit-test level. The list_secrets_abi_tests module explicitly documents why: these paths require a registered MEM_ADDR + DELEGATE_ENV (live delegate WASM instance), and notes store-layer coverage via secrets_store::list_secret_keys_*. The parameter-validation error paths (negative lengths, outside-process) ARE covered. Given the explicit rationale and the store-layer coverage, this is informational rather than blocking, but testing.md asks that error paths be tested. (rule: testing.md — error path coverage)

---

Rule review against .claude/rules/. WARNING findings block merge.

[iduartgomez]

Comprehensive PR Review: #4523

Summary

• PR Title: feat(delegate): host-side secret-key enumeration (#4355)
• Type: feat (delegate secret-storage key enumeration / list_secrets)
• CI Status: all green (Clippy, Fmt, Unit & Integration, Simulation, Windows, NAT Validation, macOS bundle-swap E2E, conventional-commits, rule checks, snyk, CLA)
• Linked Issue: #4355 (approved, T-feature/P-high, In Progress); River use case freenet/river#345; stdlib half freenet/freenet-stdlib#80 (published as 0.8.2)
• Review tier: Full — touches cryptography/secrets-at-rest, WASM host ABI, and a stdlib wire-format codec.
• Reviewers run: code-first, testing, skeptical, big-picture (all four freenet: subagents) + my own independent crypto/ABI verification. External model (codex) was unavailable — see note below.
• HEAD SHA reviewed: 53647fe7efae6cbfc1a842506e7057824d3e51a0
• Local build/test: cargo test -p freenet --lib wasm_runtime::secrets_store → 51 passed, 0 failed (incl. all 6 new tests and the existing per-write-nonce-uniqueness guard).

External-model substitution (disclosure): codex review failed with a hard auth error (refresh_token_reused, 401 — token expired), and gemini is not installed. Per the pr-review skill's fallback policy, the independent pass was substituted with the four distinct Claude-lens subagents (skeptical bug-hunt, code-first, testing, big-picture), each blind to the others, plus a manual crypto-at-rest + WASM-ABI verification pass by the orchestrator. When the external quota resets, re-running codex review --base origin/main before merge would add a non-correlated signal; not a blocker.

Scope note: a stale local main in the working worktree made a naive git diff main...HEAD show ~101 files plus two stray binaries (arr, trait). Those are NOT part of this PR. The authoritative diff (gh pr diff 4523) is exactly 5 files, +708/-13: Cargo.toml, Cargo.lock, wasm_runtime/engine/wasmtime_engine.rs, wasm_runtime/native_api.rs, wasm_runtime/secrets_store.rs. The review covers only those.

---

Code-First Analysis

Independent understanding: Adds host-side delegate secret-key enumeration in three layers — (1) two synchronous hostcalls under freenet_delegate_secrets (__frnt__delegate__list_secrets_len / __frnt__delegate__list_secrets) mirroring the proven len-then-fill get_secret ABI; (2) native_api::delegate_secrets::{collect_list_secrets, list_secrets_len, list_secrets} reading the prefix from WASM memory and serializing via the shared stdlib codec; (3) a per-scope encrypted .keys registry in secrets_store.rs that maps stored secrets back to their raw keys (the durable ReDb index keeps only the 32-byte Blake3 hash, and secret files are hash-named, so the raw key was recoverable nowhere). register_key/deregister_key maintain it best-effort, ordered AFTER the durable value+index commit; list_secret_keys reads + prefix-filters; capped at 4096/scope.

Stated intent: Give a delegate a way to enumerate its stored secret keys (optionally prefix-scoped) so apps with open-ended key families (River room:<owner_vk>) can rediscover storage after a delegate-WASM rebuild.

Alignment: Matches. Prefix-filtered, deduped (on insert), bounded, raw-key (not hash) enumeration; registry encrypted under the same scope DEK as values; scope-isolated; crash-safe ordering.

Gaps: No code-level gaps. One description-currency item: the PR body still opens with a "DRAFT — blocked on stdlib release" banner, but the PR is no longer a draft (isDraft=false), 0.8.2 is published, Cargo.lock resolves it from crates.io with a checksum, and CI is green. The banner is stale and should be removed so it doesn't mislead a reviewer. No [patch.crates-io]/path-dep coupling exists — stdlib-first policy honored. The PR adds no match sites on stdlib #[non_exhaustive] enums (only the codec + new imports), so no wildcard arms are needed.

---

Testing Assessment

Coverage level: Store-layer adequate and genuine (not coverage theater); host-ABI layer uncovered at unit level.

Test Type	Status	Notes
Unit (store)	PASS	empty/prefix, enumerate-raw-not-hash, dedup-on-restore, removal, scope isolation (Local vs User), at-cap boundary (+ over-cap value still readable), persist-across-restart (decrypt-from-disk), .keys-dotfile tmp-path regression. 6 new tests, all pass.
Unit (host ABI)	MISSING	collect_list_secrets/list_secrets/list_secrets_len have no unit coverage (negative-len, list-all vs prefix, buffer-too-small, len↔fill agreement) — and these are pure host logic that does NOT need a real WASM module to test.
Corrupt-registry path	MISSING	the documented decrypt-fail → empty → next-write-drops-keys data-loss transition is untested (trivially testable via the existing corrupt_versioned_blob_errors_cleanly template).
Integration / E2E (through real WASM)	DEFERRED (legitimate)	the test-delegate fixture pins published stdlib and can't drive the unpublished ABI; tracked as follow-up. Do not block on this.

Regression test: This is feat: (regression-test-mandatory rule is for fix:); the new tests satisfy the "test edge cases" expectation for the store layer.

Missing tests (see Recommendations): corrupt-registry transition; host-side ABI error/boundary arms.

---

Skeptical Findings

Risk level: Low — the change reuses proven crypto/permission/atomic-write machinery rather than inventing a parallel path.

Concern	Severity	Location (verified)	Details
Transient (non-NotFound) read error overwrites a valid registry from empty	Medium	secrets_store.rs:1149-1152 (read_key_registry) → register_key:1282-1295	A fs::read error that is NOT NotFound (EACCES, EIO, EMFILE/"too many open files", a .keys caught mid-rename) returns Vec::new() — indistinguishable from "file absent." The next register_key then appends only the one new key and write_key_registry overwrites .keys with a single-entry blob, permanently shrinking a registry that legitimately held thousands of valid, decryptable keys. This is sharper than the documented decrypt-failure case: there the blob is genuinely unusable; here the on-disk blob is perfectly valid and a momentary, recoverable IO error is amplified into permanent enumeration loss. The doc comment (:1262-1274) only justifies the "corrupt blob" case, not this one. Secret VALUES are never affected (registry is independent + written after value commit), which is why this is Should-Fix not blocking — but the fix is cheap. (Verified at :1149.)
Corrupt/undecryptable .keys blob silently shrinks the enumerable set	Low (documented, by-design)	secrets_store.rs:1154-1173 (read_key_registry), doc at :1262-1274	On malformed-header/decrypt failure, read returns empty (loud warn!), and the next register/deregister rewrites from empty, dropping prior keys. Deterministic/persistent (unlike M1). Secret VALUES unaffected. Thoroughly documented; acceptable trade-off. Worth explicit maintainer sign-off + a test.
>4096 key families enumerate a truncated view	Low (by-design, #3798 bound)	secrets_store.rs:1286	Over-cap keys are stored + readable but not enumerable; no LRU/eviction (first 4096 distinct keys win until removed). Cap enforced with >= (no off-by-one), warn-logged. A migration consumer of a >4096 family gets a partial list — deserves a one-line callout in the PR body / docs.
list_secrets re-fetches MEM_ADDR after collect_list_secrets	None (cosmetic)	native_api.rs:1566-1567	Harmless redundancy; refresh_mem_addr_from_caller already ran in the linker closure. No fix needed.
tmp-path comment says &mut self but helpers are &self	None (doc-accuracy nit)	secrets_store.rs:1232-1233	The fixed .keys.tmp suffix is safe because the single-writer invariant holds transitively: the only callers are store_secret/remove_secret, which take &mut self (documented at :296-300). The comment's wording is slightly loose; the conclusion is correct.

Verified-clean (the brief's focus areas):

• Crypto-at-rest: fresh per-write nonce via XChaCha20Poly1305::generate_nonce(&mut OsRng) (the sanctioned OsRng crypto exception), SAME scope DEK as values, identical [VERSION_V1][24-byte nonce][AEAD] layout, create_owner_only (0o600) + ensure_owner_only_tree + tmp+rename — byte-for-byte the existing store_secret discipline. No nonce reuse, no plaintext-at-rest (raw keys are AEAD-encrypted before disk; only hashes were ever plaintext, in ReDb). The per_write_nonce_makes_identical_plaintext_ciphertext_distinct guard passes.
• DEK read/write pairing: write side uses cipher_for/derive_user_dek (passed into register_key); read side uses cipher_for_read/derive_user_dek (encryption_for_scope_read). User path is identical; Local path matches by the same get_secret read/write invariant already proven for values. No spurious-decrypt-induced key loss for correctly-derived keys.
• WASM host ABI: both linker registrations read id from CURRENT_DELEGATE_INSTANCE and call refresh_mem_addr_from_caller(&mut caller, id) before delegating (contracts.md #3248 rule); logic lives in native_api.rs. validate_and_compute_ptr guards BOTH the prefix read and the output write; negative prefix_len/out_len rejected before the as usize casts; zero-len prefix takes a no-read "list all" path; ERR_BUFFER_TOO_SMALL when serialized > buffer; SAFETY comments are accurate.
• len↔fill consistency / TOCTOU: both calls share collect_list_secrets, so the advertised length matches the bytes written. A registry change between the two calls is impossible in practice (single synchronous delegate instance per thread-local), and is handled gracefully by ERR_BUFFER_TOO_SMALL if it ever occurred.
• Ordering: registry write is AFTER value+index commit (store) and value+index removal (remove); a crash only delays enumerability, never the value.

---

Big Picture Assessment

Goal alignment: Yes. Delivers the core (host) half of #4355; directly serves the River room: migration use case. The encrypted .keys registry is the load-bearing design decision (raw keys are otherwise unrecoverable) — justified scope, not creep.
Anti-patterns detected: None. The "best-effort / swallow" registry error handling is deliberate, documented, and correct (registry failure degrades only future enumeration). The 4096 cap is an amplification bound (#3798 class), not a magic number replacing dynamic logic; it's #[cfg(test)]-overridable.
Removed code concerns: None — the only deletions are the stdlib version churn and one import line expanded (not removed). No tests removed or weakened.
Scope assessment: Focused — one logical change, 5 files all in the delegate-secrets subsystem, with an approved issue. Stdlib-first policy fully compliant (published 0.8.2, no patch/path-dep).

---

Documentation

• Code docs: complete — new public list_secret_keys and the registry helpers are well-documented, including the honest corrupt-registry caveat.
• Architecture / operator docs: docs/secrets-at-rest.md is stale. Its on-disk file-permissions table (lines 246-257) enumerates <delegate>/<secret_id> and every .snapshots/ entry but does NOT list the new <secrets_dir>/<delegate>/.keys (and User-scope users/<id>/.keys) registry — a NEW on-disk artifact created per scope. Functionally it conforms to the documented posture (created 0o600 under an owner-only tree), and the export/import path walks the ReDb index (not a raw dir scan) so .keys won't be mis-ingested — but per AGENTS.md ("WHEN you discover missing documentation → fix it in the same PR") a table row + a one-paragraph encryption-format note should ride in this PR.
• .claude/rules/ / skills: no updates required (the registry reuses existing crypto-at-rest conventions; repo has no agents//skills/ trees affected).

---

Recommendations

Must Fix (Blocking)

None. No correctness, security, or data-integrity defect was found. Crypto-at-rest, WASM ABI, cap enforcement, wire-format, and ordering are all sound; CI is green and the local test run is clean.

Should Fix (Important)

1. (M1) Don't overwrite the registry from empty on a transient read error (secrets_store.rs:1149). Make read_key_registry tri-state: distinguish "definitively absent" (NotFound → safe to treat as empty + overwrite) from "unknown" (any other IO error, and arguably also decrypt failure → registry state is unknown). register_key/deregister_key should bail out without writing on the "unknown" case (logging), leaving the existing .keys intact for a later retry. This keeps the "value already committed, enumeration is advisory" contract while not amplifying a one-call IO hiccup into permanent enumeration loss. (Raised by skeptical-reviewer; independently verified at :1149-1152. Lower-severity because values are never affected, but the fix is small and removes a silent data-loss path the doc comment doesn't actually justify.)
2. Add a corrupt/unreadable-registry regression test (secrets_store.rs): store ≥2 keys, corrupt or make-unreadable the on-disk .keys blob (reuse the corrupt_versioned_blob_errors_cleanly template: VERSION_V1 + 24 zero bytes + bogus AEAD), register a new key, and assert the resulting enumerable set — pinning whatever behavior you settle on for M1 so a future refactor can't silently flip "drop" vs "preserve." (Template verified at secrets_store.rs:3234.)
3. Add host-side ABI unit tests (native_api.rs) for collect_list_secrets/list_secrets_len/list_secrets: negative prefix_len/out_len → ERR_INVALID_PARAM; list-all (len 0) vs prefix; ERR_BUFFER_TOO_SMALL; empty → 0; and the list_secrets_len == list_secrets bytes-written agreement invariant. These are pure host logic (no real WASM module needed) — the deferred end-to-end-through-WASM test remains legitimately deferred, but this host-logic layer should not be.
4. Add a User-scope restart test. list_secret_keys_persist_across_restart only covers Local; the User-scope registry is encrypted under derive_user_dek(delegate, dek_secret), the higher-risk DEK path, and is untested across restart. (DEK derivation verified deterministic and matching on both sides, so this is a coverage gap, not a suspected bug.)
5. Update docs/secrets-at-rest.md — add a <secrets_dir>/<delegate>/.keys (and User-scope) row to the permissions table and a short encryption-format note (same scope DEK, same [VERSION_V1][nonce][AEAD] layout, raw keys, 4096/scope cap).
6. Refresh the PR description — remove the stale "DRAFT — blocked on stdlib release" banner (0.8.2 is published, PR is not a draft, CI green), and add a one-line callout that >4096-key families enumerate a truncated (non-evicting) view, so a migration consumer doesn't assume completeness.

Consider (Suggestions)

7. Tighten the tmp-path comment at secrets_store.rs:1232-1233 to say the single-writer invariant comes from the &mut self public mutators (store_secret/remove_secret), since the helper itself is &self.
8. Optional micro: drop the redundant second MEM_ADDR.get(&id) in list_secrets (native_api.rs:1566) by threading info out of collect_list_secrets; purely cosmetic.
9. Add a one-line note that the i32::MAX length clamp in list_secrets_len (native_api.rs:1521) can make a >2 GiB encoded list permanently un-fetchable (guest sizes from the clamped value, then hits ERR_BUFFER_TOO_SMALL). Unreachable in practice given the 4096-key cap and realistic key sizes; degrades safely (error, no UB), so documentation-only.

---

Verdict

State: Ship with nits — Needs Changes — Light Re-Check Sufficient.

This is a clean, well-scoped, well-documented feature that correctly reuses the existing secrets-at-rest crypto and atomic-write discipline. No blocking defect, and critically no path can lose a secret VALUE — the registry is independent of, and written strictly after, the durable value+index commit. Crypto (per-write OsRng nonce, shared scope DEK, no plaintext-at-rest), the WASM host ABI (mem refresh + bounds-checked reads/writes, accurate SAFETY comments), the 4096 cap (enforced, no off-by-one, warn-logged), the shared wire-format codec, and the ordering all check out; CI plus a local 51-test run are green.

The one finding worth fixing before merge is M1: a transient (non-NotFound) IO read error on .keys is currently amplified into permanent loss of the enumerable key set (values stay safe). It's Should-Fix, not blocking, because it only affects enumeration and needs a rare FS error mid-store — but the fix is small (tri-state read + bail-on-unknown) and removes a silent data-loss path the doc comment doesn't actually justify. The remaining Should-Fix items are additive (three tests, one doc table, one PR-description refresh) and don't change existing logic.

Because M1 touches persistence/data-integrity (a high-risk area per the re-review policy), if M1 is fixed, do a focused re-check of the M1 diff + the new corrupt-registry test (a diff-of-the-diff pass is sufficient — a full re-review is not required unless that test surfaces a behavioral surprise). The merge decision rests with the maintainer.

HEAD SHA reviewed: 53647fe7efae6cbfc1a842506e7057824d3e51a0

[AI-assisted - Claude]