M6: Zeroize long-lived engine key material on drop #199

New Issue

2026-07-28T14:04:43Z

KofTwentyTwo commented

2026-07-28 14:04:43 +00:00

(Migrated from github.com)

Context

Story for milestone M6. Read docs/STORY-WORKFLOW.md first — it defines the process, gates, and patterns; this issue defines only the specific work. crates/nuncio-store/src/vault.rs already depends on zeroize and defines a ZeroizingSecret wrapper (vault.rs:30-44, Zeroize/ZeroizeOnDrop-derived), but it is decorative: nothing in the tree actually wraps real key material in it — it is only exercised by its own unit test (vault.rs:285-290). Verified in the tree: DatabaseEngine (crates/nuncio-store/src/db.rs:79-85) holds storage_key: [u8; 32], worm_key: Vec<u8>, and ledger_key: Vec<u8> as plain, non-zeroizing fields for the entire process lifetime (the engine is wrapped in Arc and shared across the whole daemon). WebhookDispatcher (crates/nuncio-filter/src/webhook.rs:29-32) similarly holds its HMAC secret_key: String as a plain field. None of these are zeroized on drop, so the AES-256-GCM storage key, the WORM/ledger HMAC keys, and the webhook signing key all remain recoverable in freed heap memory after the holding struct is dropped (reconciles #163).

Goal

Wrap every long-lived cryptographic key field identified above in a zeroizing container so its backing memory is overwritten when the field (or its owning struct) is dropped. This must not change any observed behavior of encryption/HMAC/signing — only add the zeroize-on-drop guarantee. Additionally, the transient account password handled in AccountsGrpcService::add_account (crates/nunciod/src/grpc.rs:308-340) should stop living as a bare String for longer than necessary before being handed to the OS keyring.

Files to touch / patterns to mirror

crates/nuncio-store/src/db.rs:79-85 (DatabaseEngine struct) — change:
```
pub struct DatabaseEngine {
    pool: SqlitePool,
    storage_key: [u8; 32],
    worm_key: Vec<u8>,
    ledger_key: Vec<u8>,
}
```
to wrap each key field in zeroize::Zeroizing<T> (already usable without new dependencies: zeroize is a workspace dependency with the derive feature in crates/nuncio-store/Cargo.toml:25, and Zeroizing<T> ships in the base zeroize crate). Zeroizing<[u8; 32]> and Zeroizing<Vec<u8>> both Deref/DerefMut to the inner type, so existing call sites that pass &self.storage_key / &self.worm_key / &self.ledger_key to PayloadCipher::{encrypt,decrypt}_bytes (cipher.rs) and HmacSha256::new_from_slice (worm_audit.rs, db.rs) continue to compile unchanged via deref coercion — verify this compiles as-is; if any call site needs an explicit &* deref, add it.
crates/nuncio-store/src/db.rs:96-116 (resolve_engine_keys) — update the return type / construction to produce the zeroizing wrapper types (e.g. Zeroizing::new(storage_key)), and the constructor(s) that build DatabaseEngine from this tuple (search resolve_engine_keys( call sites in db.rs, e.g. connect_file/connect_ephemeral).
crates/nuncio-filter/src/webhook.rs:29-32 (WebhookDispatcher struct) — change secret_key: String to secret_key: zeroize::Zeroizing<String> (add zeroize.workspace = true to crates/nuncio-filter/Cargo.toml's [dependencies], mirroring crates/nuncio-store/Cargo.toml:25). WebhookDispatcher::new (webhook.rs:36-47) wraps the incoming impl Into<String> in Zeroizing::new(..); dispatch_with_options (webhook.rs:50-98) already only reads self.secret_key.as_bytes() (webhook.rs:79), which works unchanged via deref coercion.
crates/nunciod/src/grpc.rs:308-340 (AccountsGrpcService::add_account) — after let req = request.into_inner(); (grpc.rs:312), move req.password into a zeroize::Zeroizing<String> (add zeroize.workspace = true to crates/nunciod/Cargo.toml if not already present — verify first) immediately, and pass &password_secret (deref-coerced to &str) to self.secrets.set_secret(&config.keyring_secret_key, &password_secret) at grpc.rs:331, so the plaintext password is zeroized as soon as it goes out of scope rather than relying on String's ordinary (non-wiping) Drop.
crates/nuncio-store/src/vault.rs:30-44 (ZeroizingSecret) — leave as-is (it's a general-purpose wrapper other stories may use), but note in the PR description that this story is what starts actually using zeroizing wrappers for real key material, since today it is unused decoration.

Implementation steps

In crates/nuncio-store/src/db.rs, change the three DatabaseEngine key fields to zeroize::Zeroizing<[u8; 32]> / zeroize::Zeroizing<Vec<u8>> and add use zeroize::Zeroizing; at the top of the file.
Update resolve_engine_keys (db.rs:100-116) and its callers to wrap the raw bytes in Zeroizing::new(..) at construction.
Fix any resulting compile errors from deref coercion gaps (add explicit &*self.storage_key etc. only where the compiler actually requires it — don't add unnecessary derefs).
In crates/nuncio-filter/src/webhook.rs, add the zeroize dependency to Cargo.toml, change secret_key: String to secret_key: zeroize::Zeroizing<String>, and update WebhookDispatcher::new accordingly.
In crates/nunciod/src/grpc.rs, wrap req.password in Zeroizing<String> immediately after extraction in add_account, before it's passed to set_secret.
Add tests per Acceptance Criteria.
Run cargo check-all to confirm zero new warnings from the type changes (in particular check Debug/Clone derives on any struct now containing a Zeroizing<T> field — Zeroizing<T> implements Debug by delegating to T's Debug, which is fine here since DatabaseEngine already has a manual Debug impl (db.rs:87-94) that never prints key material — do not let the manual impl regress into printing the zeroizing field).

Acceptance criteria

DatabaseEngine's storage_key, worm_key, and ledger_key fields are zeroize::Zeroizing<..>-wrapped; all existing nuncio-store tests (encryption roundtrip, WORM chain, ledger HMAC) pass unchanged.
WebhookDispatcher::secret_key is zeroize::Zeroizing<String>-wrapped; the existing test_blocked_private_ip_webhook test (webhook.rs:125-143) and any new dispatch tests still pass.
AccountsGrpcService::add_account's local password variable is zeroized on drop rather than relying on String's default (non-wiping) drop.
A test proves the zeroize wiring is real, not decorative: since inspecting freed memory requires unsafe (forbidden by this workspace's #![forbid(unsafe_code)]), assert the mechanism directly — e.g. a test that constructs a Zeroizing<[u8; 32]> with known non-zero bytes, calls .zeroize() on it explicitly (safe, from the Zeroize trait) without dropping it, and asserts the underlying bytes are now all-zero (proving the same method Zeroizing's Drop impl calls internally actually wipes the buffer). Add this as a unit test in crates/nuncio-store/src/db.rs or cipher.rs rather than relying solely on the existing no-op-looking zeroizing_secret_wipes_bytes_on_drop test in vault.rs:285-290 (that test drops a value and asserts nothing about its memory — it does not actually prove wiping; do not add another test with the same non-assertion shape).
Debug output for DatabaseEngine (already redacted, db.rs:87-94) and any newly-affected struct never prints raw key bytes — add/keep a test asserting this (mirror the existing manual Debug impl's intent).
cargo verify green (fmt + clippy -D warnings all-targets + test-all), 0 ignored.

Definition of done & process

Follow docs/STORY-WORKFLOW.md: no fabricated data; comment convention (intent-only, no story/GH/phase refs in .rs/.proto); mock-only tests. Branch story/m6-zeroize-keys off dev; PR to dev with the gate output pasted as evidence; the PO reviews before the issue is closed.

Dependencies / notes

Not a [PROTO] story. Reconciles #163. Touches crates/nuncio-store/src/db.rs (also touched by M6-S1 and M6-S3) and crates/nunciod/src/grpc.rs (also touched by M6-S4) — coordinate with the PO on landing order to minimize merge conflicts; none of these are proto-coordination conflicts (no .proto changes here). secrecy::SecretString was considered for the transient account password instead of zeroize::Zeroizing<String>; secrecy is not currently a workspace dependency (only age::secrecy is available transitively via the age crate used in cipher.rs:141-142, which is a differently-scoped API not worth pulling in cross-crate for this). Using zeroize::Zeroizing<String> achieves the same wipe-on-drop guarantee with an already-present dependency; do not add a new secrecy dependency for this story. Size: M.

## Context Story for milestone M6. **Read [`docs/STORY-WORKFLOW.md`](https://github.com/KofTwentyTwo/nuncio/blob/dev/docs/STORY-WORKFLOW.md) first** — it defines the process, gates, and patterns; this issue defines only the specific work. `crates/nuncio-store/src/vault.rs` already depends on `zeroize` and defines a `ZeroizingSecret` wrapper (`vault.rs:30-44`, `Zeroize`/`ZeroizeOnDrop`-derived), but it is decorative: nothing in the tree actually wraps real key material in it — it is only exercised by its own unit test (`vault.rs:285-290`). Verified in the tree: `DatabaseEngine` (`crates/nuncio-store/src/db.rs:79-85`) holds `storage_key: [u8; 32]`, `worm_key: Vec<u8>`, and `ledger_key: Vec<u8>` as plain, non-zeroizing fields for the *entire process lifetime* (the engine is wrapped in `Arc` and shared across the whole daemon). `WebhookDispatcher` (`crates/nuncio-filter/src/webhook.rs:29-32`) similarly holds its HMAC `secret_key: String` as a plain field. None of these are zeroized on drop, so the AES-256-GCM storage key, the WORM/ledger HMAC keys, and the webhook signing key all remain recoverable in freed heap memory after the holding struct is dropped (reconciles #163). ## Goal Wrap every long-lived cryptographic key field identified above in a zeroizing container so its backing memory is overwritten when the field (or its owning struct) is dropped. This must not change any observed behavior of encryption/HMAC/signing — only add the zeroize-on-drop guarantee. Additionally, the transient account password handled in `AccountsGrpcService::add_account` (`crates/nunciod/src/grpc.rs:308-340`) should stop living as a bare `String` for longer than necessary before being handed to the OS keyring. ## Files to touch / patterns to mirror - `crates/nuncio-store/src/db.rs:79-85` (`DatabaseEngine` struct) — change: ```rust pub struct DatabaseEngine { pool: SqlitePool, storage_key: [u8; 32], worm_key: Vec<u8>, ledger_key: Vec<u8>, } ``` to wrap each key field in `zeroize::Zeroizing<T>` (already usable without new dependencies: `zeroize` is a workspace dependency with the `derive` feature in `crates/nuncio-store/Cargo.toml:25`, and `Zeroizing<T>` ships in the base `zeroize` crate). `Zeroizing<[u8; 32]>` and `Zeroizing<Vec<u8>>` both `Deref`/`DerefMut` to the inner type, so existing call sites that pass `&self.storage_key` / `&self.worm_key` / `&self.ledger_key` to `PayloadCipher::{encrypt,decrypt}_bytes` (`cipher.rs`) and `HmacSha256::new_from_slice` (`worm_audit.rs`, `db.rs`) continue to compile unchanged via deref coercion — verify this compiles as-is; if any call site needs an explicit `&*` deref, add it. - `crates/nuncio-store/src/db.rs:96-116` (`resolve_engine_keys`) — update the return type / construction to produce the zeroizing wrapper types (e.g. `Zeroizing::new(storage_key)`), and the constructor(s) that build `DatabaseEngine` from this tuple (search `resolve_engine_keys(` call sites in `db.rs`, e.g. `connect_file`/`connect_ephemeral`). - `crates/nuncio-filter/src/webhook.rs:29-32` (`WebhookDispatcher` struct) — change `secret_key: String` to `secret_key: zeroize::Zeroizing<String>` (add `zeroize.workspace = true` to `crates/nuncio-filter/Cargo.toml`'s `[dependencies]`, mirroring `crates/nuncio-store/Cargo.toml:25`). `WebhookDispatcher::new` (`webhook.rs:36-47`) wraps the incoming `impl Into<String>` in `Zeroizing::new(..)`; `dispatch_with_options` (`webhook.rs:50-98`) already only reads `self.secret_key.as_bytes()` (`webhook.rs:79`), which works unchanged via deref coercion. - `crates/nunciod/src/grpc.rs:308-340` (`AccountsGrpcService::add_account`) — after `let req = request.into_inner();` (`grpc.rs:312`), move `req.password` into a `zeroize::Zeroizing<String>` (add `zeroize.workspace = true` to `crates/nunciod/Cargo.toml` if not already present — verify first) immediately, and pass `&password_secret` (deref-coerced to `&str`) to `self.secrets.set_secret(&config.keyring_secret_key, &password_secret)` at `grpc.rs:331`, so the plaintext password is zeroized as soon as it goes out of scope rather than relying on `String`'s ordinary (non-wiping) `Drop`. - `crates/nuncio-store/src/vault.rs:30-44` (`ZeroizingSecret`) — leave as-is (it's a general-purpose wrapper other stories may use), but note in the PR description that this story is what starts actually using zeroizing wrappers for real key material, since today it is unused decoration. ## Implementation steps 1. In `crates/nuncio-store/src/db.rs`, change the three `DatabaseEngine` key fields to `zeroize::Zeroizing<[u8; 32]>` / `zeroize::Zeroizing<Vec<u8>>` and add `use zeroize::Zeroizing;` at the top of the file. 2. Update `resolve_engine_keys` (`db.rs:100-116`) and its callers to wrap the raw bytes in `Zeroizing::new(..)` at construction. 3. Fix any resulting compile errors from deref coercion gaps (add explicit `&*self.storage_key` etc. only where the compiler actually requires it — don't add unnecessary derefs). 4. In `crates/nuncio-filter/src/webhook.rs`, add the `zeroize` dependency to `Cargo.toml`, change `secret_key: String` to `secret_key: zeroize::Zeroizing<String>`, and update `WebhookDispatcher::new` accordingly. 5. In `crates/nunciod/src/grpc.rs`, wrap `req.password` in `Zeroizing<String>` immediately after extraction in `add_account`, before it's passed to `set_secret`. 6. Add tests per Acceptance Criteria. 7. Run `cargo check-all` to confirm zero new warnings from the type changes (in particular check `Debug`/`Clone` derives on any struct now containing a `Zeroizing<T>` field — `Zeroizing<T>` implements `Debug` by delegating to `T`'s `Debug`, which is fine here since `DatabaseEngine` already has a manual `Debug` impl (`db.rs:87-94`) that never prints key material — do not let the manual impl regress into printing the zeroizing field). ## Acceptance criteria - [ ] `DatabaseEngine`'s `storage_key`, `worm_key`, and `ledger_key` fields are `zeroize::Zeroizing<..>`-wrapped; all existing `nuncio-store` tests (encryption roundtrip, WORM chain, ledger HMAC) pass unchanged. - [ ] `WebhookDispatcher::secret_key` is `zeroize::Zeroizing<String>`-wrapped; the existing `test_blocked_private_ip_webhook` test (`webhook.rs:125-143`) and any new dispatch tests still pass. - [ ] `AccountsGrpcService::add_account`'s local password variable is zeroized on drop rather than relying on `String`'s default (non-wiping) drop. - [ ] A test proves the zeroize wiring is real, not decorative: since inspecting freed memory requires `unsafe` (forbidden by this workspace's `#![forbid(unsafe_code)]`), assert the *mechanism* directly — e.g. a test that constructs a `Zeroizing<[u8; 32]>` with known non-zero bytes, calls `.zeroize()` on it explicitly (safe, from the `Zeroize` trait) without dropping it, and asserts the underlying bytes are now all-zero (proving the same method `Zeroizing`'s `Drop` impl calls internally actually wipes the buffer). Add this as a unit test in `crates/nuncio-store/src/db.rs` or `cipher.rs` rather than relying solely on the existing no-op-looking `zeroizing_secret_wipes_bytes_on_drop` test in `vault.rs:285-290` (that test drops a value and asserts nothing about its memory — it does not actually prove wiping; do not add another test with the same non-assertion shape). - [ ] `Debug` output for `DatabaseEngine` (already redacted, `db.rs:87-94`) and any newly-affected struct never prints raw key bytes — add/keep a test asserting this (mirror the existing manual `Debug` impl's intent). - [ ] `cargo verify` green (fmt + clippy `-D warnings` all-targets + test-all), 0 ignored. ## Definition of done & process Follow [`docs/STORY-WORKFLOW.md`](https://github.com/KofTwentyTwo/nuncio/blob/dev/docs/STORY-WORKFLOW.md): no fabricated data; comment convention (intent-only, no story/GH/phase refs in `.rs`/`.proto`); mock-only tests. Branch `story/m6-zeroize-keys` off `dev`; PR to `dev` with the gate output pasted as evidence; the PO reviews before the issue is closed. ## Dependencies / notes Not a [PROTO] story. Reconciles #163. Touches `crates/nuncio-store/src/db.rs` (also touched by M6-S1 and M6-S3) and `crates/nunciod/src/grpc.rs` (also touched by M6-S4) — coordinate with the PO on landing order to minimize merge conflicts; none of these are proto-coordination conflicts (no `.proto` changes here). `secrecy::SecretString` was considered for the transient account password instead of `zeroize::Zeroizing<String>`; `secrecy` is not currently a workspace dependency (only `age::secrecy` is available transitively via the `age` crate used in `cipher.rs:141-142`, which is a differently-scoped API not worth pulling in cross-crate for this). Using `zeroize::Zeroizing<String>` achieves the same wipe-on-drop guarantee with an already-present dependency; do not add a new `secrecy` dependency for this story. Size: M.

KofTwentyTwo commented

2026-07-30 03:10:04 +00:00

(Migrated from github.com)

Delivered via PR #238 (squash-merged to dev). DatabaseEngine's storage_key/worm_key/ledger_key, WebhookDispatcher's HMAC secret_key, and add_account's transient password are now zeroize::Zeroizing-wrapped so their backing memory is wiped on drop; call sites deref-coerce so no crypto/HMAC/signing behavior changed. Honest mechanism test asserts .zeroize() actually zeroes the buffer (not the prior decorative no-op-drop shape); Debug stays redacting. This is what starts actually using zeroizing wrappers for real key material (vault.rs's ZeroizingSecret was decorative). Reconciles #163.

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: KofTwentyTwo/nuncio#199