fix(crypto): redact key wrapper debug formatting - #36
presempathy-awb wants to merge 2 commits into
Conversation
farhan-syah
left a comment
There was a problem hiding this comment.
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.
|
|
||
| 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] |
There was a problem hiding this comment.
Conflicts with main. main renamed [0.1.0] to [Unreleased], so this adds a second [Unreleased] section. Rebase.
|
|
||
| ### 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. |
There was a problem hiding this comment.
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.
| /// | ||
| /// 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) { |
There was a problem hiding this comment.
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.
Summary
Prevent PageDB's three key-wrapper types from exposing raw key bytes through Rust debug formatting.
SecretKey,MasterKey, andDerivedKeynow implement manual, constant-shapeDebugoutput that contains only the wrapper name and<redacted>. The inner byte arrays forMasterKeyandDerivedKeyalso become private, closing the crate-internal spelling that could bypass the wrapper and formatZeroizing<[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>implementsDebugwhenTdoes, and a byte array'sDebugrepresentation contains every byte.Before this change,
MasterKeyandDerivedKeyexposed their inner field aspub(crate). That made this valid anywhere in the crate:Adding only a redacted
Debugimplementation to the wrapper would not close that route: callers could still reach through the wrapper and format the innerZeroizingvalue. Conversely, making the field private without implementingDebugwould 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:
Change
Debugcontract for all three wrapper types.TypeName(<redacted>), for normal and alternate debug formatting.MasterKeyandDerivedKeytuple fields private. All legitimate byte access remains through the existingpub(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.SecretKeyalready held its bytes privately; it receives the same formatter so all key types behave consistently.Regression coverage
Five focused tests cover the formatting boundary:
SecretKeynormal debug formatting;MasterKeynormal debug formatting;DerivedKeynormal debug formatting;Debugstructure.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
Debugautomatically.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
Debugimplementations; no removed public API. The affected tuple fields werepub(crate), never public to downstream crates..0access outsidecrypto::keysis intentionally removed. The full crate compiles with the field private, demonstrating that production code already uses the reviewed accessors.Debug.Alternatives considered
Derive
DebugRejected. Derivation would print the
Zeroizing<[u8; 32]>field and therefore the secret itself.Omit
DebugentirelyInsufficient. 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 reviewRejected. 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
DisplayNot needed. Key material has no user-facing textual representation. Adding
Displaywould increase the formatting surface without a concrete use case.Verification
Run on the exact upstream base plus this commit:
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: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:
Debugimplementation is manual and constant-shape.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.