identity+crypto: atomic 0600 key file and zeroize secrets on drop#136
Merged
Conversation
Hardens on-disk identity storage and in-memory secret handling:
* `Identity::load_or_generate` now writes the key file atomically:
open a sibling temp file with `O_CREAT|O_EXCL` and mode `0o600`,
fsync, then rename into place. A crash mid-write can no longer
leave a half-written or world-readable key on disk.
* On load, the file mode is checked on Unix; any group/other access
is rejected with a new `IdentityError::InsecurePermissions { path,
mode }` error variant rather than silently using a leaked key.
* `ChannelKey` and `Identity` both derive `ZeroizeOnDrop` so secret
bytes are wiped from memory when dropped. `iroh_base::SecretKey`
already zeroizes internally, so `Identity` uses `#[zeroize(skip)]`
on its `secret_key` field; the derive is kept for the type-level
guarantee enforced by the new `_is_zeroize_on_drop` compile tests.
* Added `Identity::with_secret_bytes(|bytes| ...)`, a callback API
that exposes the raw 32-byte secret only inside the closure and
zeroizes the buffer afterwards. `atomic_write_key` uses it so the
intermediate write buffer is wiped immediately. `Identity::to_bytes`
is marked `#[must_use]` with a doc comment urging callers to
zeroize the returned `Vec<u8>` (or use `with_secret_bytes`).
The on-disk key file format is unchanged (raw 32 Ed25519 bytes), and
nothing in the public API of `ChannelKey` changes — the `pub(crate)`
field stays put, so `willow-channel`/`willow-state` consumers don't
need updates.
Tests cover: 0o600 mode on freshly-created files, rejection of
loose-perm files with `InsecurePermissions`, round-trip via
`load_or_generate`, `with_secret_bytes` exposing the same 32 bytes
as `to_bytes`, and compile-time `ZeroizeOnDrop` assertions for both
`Identity` and `ChannelKey`.
Closes #126
Closes #127
Progresses #108
Audit nit: the regression test that verifies `with_secret_bytes` actually exposes the raw 32-byte secret was itself leaking two unzeroized `Vec<u8>` copies of the secret on the heap — ironic given the PR's purpose. Wrap both locals in `zeroize::Zeroizing` so the test zeroizes the scratch buffers on drop like the rest of the codebase. No behavior change; `zeroize` is already a workspace dep with `derive`.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #126
Closes #127
Progresses #108
Summary
Hardens on-disk identity storage and in-memory secret handling for Willow's identity / crypto layers.
Issue #126 — atomic key file write, 0600 perms, perm validation
Identity::load_or_generatenow writes the key file atomically: opens a sibling temp file withO_CREAT|O_EXCLand (on Unix) mode0o600, writes viawith_secret_bytesso the buffer is zeroized,fsyncs, andrenames into place. A crash mid-write can no longer leave a half-written or world-readable key on disk.mode & 0o077 != 0) is rejected with a newIdentityError::InsecurePermissions { path, mode }variant rather than silently using a leaked key.#[cfg(not(target_arch = "wasm32"))]; Unix-only bits sit under#[cfg(unix)]so Windows and WASM still build.OpenOptions::new().write(true).create_new(true)(plus a sibling.<file>.tmp.<pid>.<nanos>name) catches the concurrent-startup race the issue called out.Issue #127 — zeroize SecretKey and ChannelKey on drop
ChannelKeyderivesZeroizeOnDrop(#[zeroize] pub(crate) [u8; 32]). Public API (as_bytes,from_bytes, thepub(crate)field) is unchanged, sowillow-channel/willow-state/willow-clientconsumers don't need updates.iroh_base::SecretKeyalready deriveszeroize::ZeroizeOnDropinternally (verified by readingiroh-base-0.97.0/src/key.rs:235), soIdentityderivesZeroizeOnDropwith#[zeroize(skip)]on thesecret_keyfield. The derive is kept for the type-level guarantee that's enforced by the new compile-time_is_zeroize_on_droptests — if the inner representation ever changes to a non-zeroizing type, the build will fail.Identity::with_secret_bytes(|bytes| ...)callback API that exposes the raw 32-byte secret only inside the closure and zeroizes the buffer on the way out.atomic_write_keyuses this to avoid leaking the key bytes through a heap-allocatedVec<u8>.Identity::to_bytesis marked#[must_use]with a doc comment urging callers to either zeroize the returnedVec<u8>or preferwith_secret_bytes. Existing call sites incrates/agentandcrates/clientare out-of-scope for this PR (per the task guardrails) and are unaffected becausemust_useonly fires on truly-unused returns.Scope guardrails respected
crates/identity/,crates/crypto/(justChannelKey+ workspace dep wiring), and the workspaceCargo.tomlare touched.Test Plan
cargo test -p willow-identity— 20 tests pass, including:load_or_generate_creates_file_with_0600_on_unix(gated#[cfg(unix)])load_existing_with_loose_perms_returns_insecure_permissions(gated#[cfg(unix)])load_existing_with_secure_perms_succeedsload_or_generate_persists_identity(round-trip)identity_is_zeroize_on_drop(compile-time assertion)with_secret_bytes_exposes_full_secretcargo test -p willow-crypto— 31 tests pass, including the newchannel_key_is_zeroize_on_dropcompile-time assertion.cargo clippy --workspace -- -D warnings— clean.cargo fmt --check— clean.cargo check --target wasm32-unknown-unknownfor the standard library set — clean.willow-identity,willow-crypto,willow-channel,willow-client,willow-state,willow-worker(worker'sload_or_generate_*tests still pass against the new error path).Note: three pre-existing flakes in
willow-actor(send_dead_actor_returns_send_error,ask_dead_actor_returns_closed,recipient_dead_actor) intermittently race when the workspace is run under heavy parallelism. They pass in isolation and reproduce onorigin/mainwithout these changes — unrelated to this PR.