Repository navigation
Zeroize ViewKey and redact secrets from Debug output - #324
Merged
Merged
Conversation
There was a problem hiding this comment.
🟢 Approval recommended
The implementation matches the stated security requirements and includes focused regression coverage.
0 open findings
What changed in this PR
Adds secret zeroization and prevents sensitive key/circuit witnesses from leaking through Debug.
Changes:
- Makes
ViewKeyzeroize on drop and removesCopy/Clone. - Redacts secret keys and private circuit witnesses from debug output.
- Adds regression tests and changelog entries.
| File | Description |
|---|---|
core/src/keys/view.rs |
Adds zeroization and redacted debug output. |
core/src/keys/secret.rs |
Redacts secret-key debug output. |
core/tests/keys.rs |
Tests zeroization and redaction. |
core/CHANGELOG.md |
Documents breaking and security-related changes. |
circuits/src/lib.rs |
Restricts debug output to public inputs. |
circuits/tests/serialization.rs |
Tests witness redaction. |
circuits/CHANGELOG.md |
Documents circuit debug redaction. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
HDauven
force-pushed
the
phoenix/secret-hygiene
branch
from
October 10, 2026 23:13
e05cb22 to
5a76aa0
Compare
Member
Author
|
Force-pushed: rebased on the amended stack. No changes. |
HDauven
force-pushed
the
phoenix/secret-hygiene
branch
from
October 11, 2026 01:32
5a76aa0 to
fd1f904
Compare
Member
Author
|
Force-pushed: rebased on the amended stack. Only changelog context changed. |
HDauven
marked this pull request as ready for review
October 11, 2026 02:02
moCello
approved these changes
Oct 11, 2026
moCello
left a comment
moCello
left a comment
Member
There was a problem hiding this comment.
APPROVE — The view key wipes its secret on drop, and the debug output of the keys and the circuit inputs shows only redacted or public data. One small note below on a test import.
- Issue #285 asks to reconsider
Copyon the view key. The pull request removes bothCopyandClone.
HDauven
force-pushed
the
phoenix/secret-hygiene
branch
4 times, most recently
from
October 13, 2026 11:07
310e7fc to
48db841
Compare
`ViewKey` holds the secret `a` that recognises the owner's notes and decrypts their values, but it was `Copy` and could not be wiped, so a wallet zeroizing its secret keys left the view keys in memory (#285). Derive `Zeroize` and `ZeroizeOnDrop`, which wipe `a` and skip the public `B`, and remove `Copy` and `Clone`, so that a second copy of a view key has to be derived again. Neither Phoenix nor its callers in Rusk clone one. `SecretKey` stays as it is: it derives only `Zeroize`, for the reasons in #244.
The derived `Debug` of `SecretKey` printed both secret scalars, and that of `ViewKey` its secret `a`, so a key formatted into a log line or a panic message carried them. Print `SecretKey(REDACTED)` and `ViewKey(REDACTED)` instead, as jubjub-schnorr 0.7.0 does for its secret keys. Of the other secret-holding types, the note secret key that `SecretKey::gen_note_sk` returns is jubjub-schnorr's `SecretKey`, already redacted, and the crate's remaining `Debug` types hold public data. The test checks that none of the three outputs carries its scalars, neither in their `Debug` form nor as hex bytes in either order.
The derived `Debug` of `TxCircuit`, `InputNoteInfo` and `OutputNoteInfo` printed every private witness: the input notes and their openings, the values and value blinders, the sender blinders, the signatures and the sender public key, so a circuit formatted into a log line or a panic message carried them all. Print the public inputs of the circuit only, and mark the omitted fields with `..`: the nullifier of each input, the value commitment, note public key and encrypted sender of each output, and the payload hash, root, deposit and maximum fee. The test checks the exact output for a random circuit, and that none of its blinders, signature responses, values or sender key show.
HDauven
force-pushed
the
phoenix/secret-hygiene
branch
from
October 13, 2026 11:07
48db841 to
2601e22
Compare
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.
Stacked on #323.
ViewKeyimplementsZeroizeandZeroizeOnDrop, and no longer implementsCopyorClone(breaking).SecretKeyandViewKeyprintSecretKey(REDACTED)andViewKey(REDACTED)throughDebug.TxCircuit,InputNoteInfoandOutputNoteInfoprint only the public inputs of the circuit throughDebug: nullifiers, output commitments, note public keys and sender encryptions, payload hash, root, deposit and maximum fee.Resolves #285
Resolves #316