Skip to content

fix(crypto): redact key wrapper debug formatting - #36

Open
presempathy-awb wants to merge 2 commits into
NodeDB-Lab:mainfrom
presempathy-awb:draft/upstream-key-redaction-20260917
Open

presempathy-awb wants to merge 2 commits into
NodeDB-Lab:mainfrom
presempathy-awb:draft/upstream-key-redaction-20260917

Conversation

@presempathy-awb

Copy link
Copy Markdown
Contributor

Summary

Prevent PageDB's three key-wrapper types from exposing raw key bytes through Rust debug formatting.

SecretKey, MasterKey, and DerivedKey now implement manual, constant-shape Debug output that contains only the wrapper name and <redacted>. The inner byte arrays for MasterKey and DerivedKey also become private, closing the crate-internal spelling that could bypass the wrapper and format Zeroizing<[u8; 32]> directly.

This is defense in depth. There is no known incident or externally reachable logging path in this repository. The goal is to make a common future diagnostic mistake fail safely before one appears in an error, tracing field, panic context, or temporary troubleshooting statement.

Problem

The key hierarchy correctly stores bytes in Zeroizing<[u8; 32]>, so memory is cleared on drop. Zeroization does not control formatting while a value is alive, however. Zeroizing<T> implements Debug when T does, and a byte array's Debug representation contains every byte.

Before this change, MasterKey and DerivedKey exposed their inner field as pub(crate). That made this valid anywhere in the crate:

format!("{:?}", key.0)

Adding only a redacted Debug implementation to the wrapper would not close that route: callers could still reach through the wrapper and format the inner Zeroizing value. Conversely, making the field private without implementing Debug would turn ordinary diagnostic attempts into compile errors that a hurried developer might work around by adding an accessor or formatting a lower layer.

The safe invariant therefore has two parts:

  1. ordinary formatting of a key wrapper is harmless; and
  2. the underlying byte container is not directly format-accessible outside the defining module.

Change

  • Add a small local macro that implements the same manual Debug contract for all three wrapper types.
  • Render only TypeName(<redacted>), for normal and alternate debug formatting.
  • Make the MasterKey and DerivedKey tuple fields private. All legitimate byte access remains through the existing pub(crate) as_bytes() methods. Internal code can still deliberately format those bytes; this change prevents accidental wrapper/direct-field disclosure, not arbitrary logging of material obtained through an explicit accessor.
  • Document why both the private field and manual formatter are required.
  • Add an Unreleased security note so the hardening is visible without overstating it as a known compromise.

SecretKey already held its bytes privately; it receives the same formatter so all key types behave consistently.

Regression coverage

Five focused tests cover the formatting boundary:

  • SecretKey normal debug formatting;
  • MasterKey normal debug formatting;
  • DerivedKey normal debug formatting;
  • alternate/pretty debug formatting; and
  • formatting when a key is nested inside another derived Debug structure.

The fixture uses 32 distinct deterministic bytes. The assertion requires the explicit redaction marker and checks that neither the decimal array representation nor a plausible two-digit hexadecimal representation of those fixture bytes appears in the rendered result. The nested test protects the realistic case where an enclosing diagnostic structure derives Debug automatically.

The tests deliberately verify behavior rather than the exact complete string. That leaves room to improve wrapper labels later while preserving the security property.

Compatibility

  • Public API: additive redacted Debug implementations; no removed public API. The affected tuple fields were pub(crate), never public to downstream crates.
  • Internal API: direct .0 access outside crypto::keys is intentionally removed. The full crate compiles with the field private, demonstrating that production code already uses the reviewed accessors.
  • On-disk format: unchanged.
  • Cryptography and derivation: unchanged. Key bytes, lifetimes, cloning, derivation, and zeroization behavior are identical.
  • Existing databases: unaffected; no migration or rewrite occurs.
  • Performance: no database algorithm or I/O path changes; no new performance measurements are claimed. The formatter performs one fixed string write only when diagnostics explicitly request Debug.

Alternatives considered

Derive Debug

Rejected. Derivation would print the Zeroizing<[u8; 32]> field and therefore the secret itself.

Omit Debug entirely

Insufficient. It prevents one spelling but does not address the crate-visible inner field, and it encourages ad hoc workarounds when a key is included in a diagnostic structure. A safe formatter makes the obvious operation safe.

Keep the field pub(crate) and rely on review

Rejected. The type system can cheaply eliminate the bypass. Secret-handling boundaries should not depend solely on every future log statement receiving perfect review.

Expose a partially masked fingerprint

Rejected for this patch. Even a stable fingerprint creates a new correlation surface and would need a separate design for purpose, lifetime, and collision behavior. Callers that need key identity should use an explicit non-secret identifier, not derive one implicitly through Debug.

Implement Display

Not needed. Key material has no user-facing textual representation. Adding Display would increase the formatting surface without a concrete use case.

Verification

Run on the exact upstream base plus this commit:

cargo fmt --all -- --check
cargo test --locked -p pagedb --lib crypto::keys::tests

The targeted suite passes all five tests. This branch is based directly on current upstream 7d8ea435975fd63fa2a56434e2898dfb2c5c6aee, which already contains the separately merged Rust/Clippy compatibility fix (#35). The following gates were rerun on this refreshed candidate; there is no unsubmitted prerequisite stack:

cargo clippy --locked -p pagedb --all-targets --all-features -- -D warnings
cargo nextest run --locked -p pagedb --all-features --no-fail-fast
RUSTDOCFLAGS='-D warnings' cargo doc --locked -p pagedb --no-deps --all-features \
  --target x86_64-unknown-linux-gnu
cargo test --locked -p pagedb --doc --all-features
cargo check --locked -p pagedb --target wasm32-unknown-unknown --lib --features opfs
cargo check --locked -p pagedb --target wasm32-wasip1 --lib
cargo bench --locked --no-run -p pagedb

Results: strict Clippy passed; 791/791 tests passed with the repository's ten default-skipped slow tests unchanged; Linux-target documentation passed with warnings denied; doctests passed; both WASM/WASI checks passed; and all five PageDB benchmark executables built successfully. The Linux documentation target matches the hosted lint runner and avoids a pre-existing macOS-only rustdoc mismatch in an unchanged Linux-native VFS link.

The doctest command completed successfully but discovered zero runnable examples; it is not a runtime regression count. Native validation used Rust 1.98.1 on Apple Silicon macOS. The local toolchain emitted a non-fatal rust-objcopy/LLVM debug-info stripping warning during benchmark compilation; all five executables were produced. WASM and WASI checks are compilation evidence, not browser/runtime execution. No new external-engine dependency or prerequisite was added.

Review guide

The key review points are intentionally narrow:

  1. Confirm every wrapper's Debug implementation is manual and constant-shape.
  2. Confirm no raw-byte accessor was added to compensate for the private fields.
  3. Confirm production derivation and encryption code still accesses bytes only through the existing module-owned methods.
  4. Confirm the tests cover ordinary, alternate, and nested formatting rather than only one happy-path string.

Non-goals

This PR does not rotate keys, change key derivation, alter zeroization semantics, add logging infrastructure, or claim to remove secrets from crash dumps or process memory. It closes the Rust formatting path represented by these wrapper types and nothing broader.

@farhan-syah farhan-syah left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code is correct. Tests pass and clippy -D warnings is clean on the PR head. No .0 bypass exists outside crypto::keys.

Blocker: the changelog hunk. Remove it and rebase on main.

Comment thread CHANGELOG.md

All notable changes to this project are documented here. The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/), and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).

## [Unreleased]

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Conflicts with main. main renamed [0.1.0] to [Unreleased], so this adds a second [Unreleased] section. Rebase.

Comment thread CHANGELOG.md

### Security

- Key wrappers redact their `Debug` output; private tuple fields also prevent direct field formatting outside their module. Explicit internal byte access remains available for cryptographic operations and must not be logged.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove this entry. No version has shipped, so the changelog describes 0.1.0 as a whole, not deltas. A "now redacts" line has no baseline. If the property belongs in the release notes, add one bullet under the existing ### Security list: "Key wrappers (SecretKey, MasterKey, DerivedKey) format as <redacted>." Keep pub(crate) and as_bytes out of it: a changelog states user-visible behaviour only.

Comment thread src/crypto/keys.rs
///
/// Check every byte in both the decimal form an array's `Debug` uses and
/// the hexadecimal form a hand-written formatter might use.
fn assert_redacted(rendered: &str, label: &str) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit. The substring checks pass only because no fixture byte renders as ed, da, ac (from redacted) or 7 (the nested epoch). A fixture or label change fails the test without a leak. Assert equality with the exact expected string instead.

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.

2 participants