Skip to content

fix(core): route seed access through the zeroizing wrapper (#41)#64

Open
beardthelion wants to merge 1 commit into
mainfrom
fix/zeroize-seed-bytes
Open

fix(core): route seed access through the zeroizing wrapper (#41)#64
beardthelion wants to merge 1 commit into
mainfrom
fix/zeroize-seed-bytes

Conversation

@beardthelion

@beardthelion beardthelion commented Jun 20, 2026

Copy link
Copy Markdown
Collaborator

Summary

Keypair::seed_bytes() handed out a bare [u8; 32] copy of the Ed25519 private seed. Because [u8; 32] is Copy with no Drop, each caller's copy was released without being scrubbed, leaving secret key material in freed memory. This removes that accessor and routes seed access through the existing zeroizing wrapper.

Motivation & context

Closes #41

to_seed() -> Zeroizing<[u8; 32]> already existed right beside seed_bytes() and scrubs the copy on drop. There was no reason to keep an unzeroized accessor: it was duplicated, and the only two callers were both in gitlawb-core. Removing it makes to_seed() the single seed accessor and closes the exposure window to core dumps, swap, and memory-disclosure bugs.

Kind of change

  • Bug fix
  • Feature
  • Security fix
  • Docs
  • Tests / CI
  • Refactor (no behavior change)
  • Breaking or protocol change (issue required first)

What changed

Crate touched: gitlawb-core.

  • Removed Keypair::seed_bytes() (raw, unzeroized seed accessor) in identity.rs.
  • Migrated the two callers to to_seed(): the envelope-decryption path in encrypt.rs (open_blob) and a crypto agreement test. The migration is mechanical: Zeroizing<[u8; 32]> deref-coerces to &[u8; 32], which is what x25519_secret_from_seed already takes.
  • Added to_seed_is_sole_seed_accessor documenting and guarding the single-accessor invariant.

No behavior change to envelope encryption/decryption: the X25519 secret derived from the seed is byte-identical, covered by the existing round-trip tests.

How a reviewer can verify

cargo test --workspace
cargo clippy --workspace --all-targets -- -D warnings
# Confirm no raw accessor remains:
git grep -n '\.seed_bytes()' crates/   # only the unrelated p2p local var, no Keypair method

Before you request review

  • Scope is one logical change; no unrelated churn
  • cargo test --workspace passes locally
  • New behavior is covered by tests (required for fixes)
  • cargo fmt --all and cargo clippy --workspace --all-targets -- -D warnings are clean
  • Commit titles use Conventional Commits (feat(...), fix(...), docs(...))
  • Docs / .env.example updated if behavior or config changed (or N/A)
  • Checked existing PRs so this isn't a duplicate

Protocol & signing impact

  • Touches DID / did:key, Ed25519 / RFC 9421 signatures, UCAN, ref certs, or P2P wire formats
  • Discussed in an issue before implementation
  • Backward-compatible with existing nodes and previously signed history

This is in-memory hygiene only. It does not change the signature scheme, key format, DID derivation, or any wire format. The bytes produced and consumed are identical; only the container type of a transient changes.

Notes for reviewers

There is a related, deliberately out-of-scope leak one hop downstream: x25519_secret_from_seed returns the derived X25519 secret as a bare [u8; 32]. It will be filed as its own follow-up issue to keep this PR scoped to exactly #41.

Summary by CodeRabbit

  • Refactor

    • Enhanced the security of cryptographic seed handling to prevent sensitive data exposure and improve overall memory safety in key operations.
    • Updated internal encryption and decryption operations to use improved seed access patterns with stronger security guarantees.
  • Tests

    • Added comprehensive unit tests to validate seed determinism and verify proper key reconstruction from the derived seed.

Remove Keypair::seed_bytes(), which returned a bare [u8; 32] copy of the
Ed25519 private seed. Because [u8; 32] is Copy with no Drop, each caller's
copy was released without being scrubbed, leaving secret key material in
freed memory. The zeroizing accessor to_seed() -> Zeroizing<[u8; 32]>
already existed beside it; this makes it the sole seed accessor and migrates
the two in-crate callers (envelope open path and a crypto test) to it via
deref coercion. No behavior change to envelope encryption: the derived
X25519 secret is byte-identical, covered by the existing round-trip tests.

Adds an identity test pinning to_seed() as the only seed accessor.
@coderabbitai

coderabbitai Bot commented Jun 20, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 5e7ef14e-4bc9-403b-9bd7-f0e136909917

📥 Commits

Reviewing files that changed from the base of the PR and between e37ea7f and d453182.

📒 Files selected for processing (2)
  • crates/gitlawb-core/src/encrypt.rs
  • crates/gitlawb-core/src/identity.rs

📝 Walkthrough

Walkthrough

seed_bytes() returning a plain [u8; 32] is removed from Keypair; to_seed() returning Zeroizing<[u8; 32]> becomes the sole seed accessor. The open_blob function and its companion test in encrypt.rs are updated to call to_seed() instead.

Changes

Keypair Seed Zeroization Hardening

Layer / File(s) Summary
Keypair: remove seed_bytes, promote to_seed
crates/gitlawb-core/src/identity.rs
Removes pub fn seed_bytes(&self) -> [u8; 32], updates to_seed() docs to designate it the sole seed accessor returning Zeroizing<[u8; 32]>, and adds a test asserting the seed is stable across calls and can reconstruct the same DID via Keypair::from_seed.
open_blob and test: migrate to to_seed()
crates/gitlawb-core/src/encrypt.rs
Replaces keypair.seed_bytes() with keypair.to_seed() in open_blob for X25519 secret derivation, and updates the ed25519_to_x25519_keypair_agrees test to match.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

Poem

🐇 A seed once bare, now wrapped with care,
Zeroizing guards what must not linger there.
seed_bytes gone — no plain array remains,
to_seed alone now holds the reins.
The key forgets itself on drop — hooray!
Memory-safe hops make for a safer day. 🌿

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Linked Issues check ⚠️ Warning The PR addresses only 2 of the 3 required call sites from issue #41. The node seed handling in crates/gitlawb-node/src/api/repos.rs:672 was not migrated. Migrate the third call site in gitlawb-node/src/api/repos.rs:672 to use to_seed() to fully resolve issue #41.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title 'fix(core): route seed access through the zeroizing wrapper' accurately and concisely describes the main security fix: removing the unzeroized seed_bytes() accessor and routing access through the zeroizing wrapper.
Description check ✅ Passed The PR description comprehensively covers all template sections including summary, motivation, kind of change (security fix), what changed, verification steps, and pre-review checklist.
Out of Scope Changes check ✅ Passed All changes are directly scoped to removing seed_bytes() and migrating existing callers to to_seed() in gitlawb-core, plus a test asserting the single-accessor invariant.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/zeroize-seed-bytes

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Harden Keypair::seed_bytes: route seed access through the zeroizing wrapper

1 participant