Harden nsec storage with SecretString and one-time key init#772
Harden nsec storage with SecretString and one-time key init#772arkanoider wants to merge 4 commits into
Conversation
Wrap the Nostr private key in secrecy/zeroize, parse Keys once at startup, wipe nsec from global Settings, and stop re-parsing on every get_keys call. Co-authored-by: Cursor <cursoragent@cursor.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
WalkthroughThis PR refactors Nostr private key handling to use secret-aware types and centralizes key initialization. Dependencies for ChangesNostr Secret Handling Refactor
🎯 4 (Complex) | ⏱️ ~45 minutes Possibly Related PRs
Suggested Reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
|
I did a strict pass on this PR and I do not currently see a functional blocker. What I specifically checked:
From what I can see, the design is internally consistent:
I also appreciate that the PR description is honest about the residual exposure that still exists by design ( So from my side this looks like a sound hardening step with a clear security win, even if it is intentionally not the final word on secret handling. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
src/util.rs (1)
820-826: ⚡ Quick winDocument the
get_keysinitialization contract.This public API now has a non-obvious precondition (
NOSTR_KEYSmust be initialized) and a specific failure mode; please add///docs so callers know the lifecycle and error semantics.✍️ Suggested doc block
+/// Returns the globally initialized Mostro Nostr signing keys. +/// +/// # Errors +/// Returns `MostroError` if keys have not been initialized yet +/// (i.e. before settings/bootstrap finishes). pub fn get_keys() -> Result<&'static Keys, MostroError> {As per coding guidelines, "Document non-obvious public APIs with
///doc comments".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/util.rs` around lines 820 - 826, Add a doc comment to the public function get_keys that documents its initialization contract and failure semantics: state that callers must ensure NOSTR_KEYS (or the underlying provider initialized by crate::config::get_mostro_keys) is set before calling, describe that it returns a &'static Keys on success and returns a MostroError via MostroInternalErr(ServiceError::NostrError(...)) when keys are not initialized, and include any recommended lifecycle guidance (when/where to call initialization) so callers understand preconditions and error handling for get_keys.Source: Coding guidelines
src/app/context.rs (1)
233-238: ⚡ Quick winKeep test settings on the same post-init contract as production.
The builder still parses
settings.nostr.nsec_privkeydirectly and then stores that same populatedSettingsinsideArc. Production goes throughtake_nsec_for_init()and clears the settings copy, so tests can currently mask code that still reads the secret fromsettingsinstead ofctx.keys().Based on PR objectives, shared settings clones should no longer retain a duplicate plaintext nsec copy.
Also applies to: 263-265
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/app/context.rs` around lines 233 - 238, The TestContextBuilder currently parses settings.nostr.nsec_privkey directly and then passes the original Settings into AppContext, leaving a plaintext nsec in the shared Settings; instead call the same helper used in production (take_nsec_for_init or equivalent) to extract and clear the nsec into Keys and then pass the sanitized Settings into AppContext (replace the unwrap_or_else parsing with code that invokes take_nsec_for_init on settings to populate keys and return a Settings without the nsec). Apply the same change to the similar block around lines 263-265 so tests mirror production behavior and no Settings clone retains the plaintext nsec.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/config/secret.rs`:
- Around line 22-29: The env path in read_nsec_env_var leaves the raw String
buffer in memory; make the initial nsec_from_env mutable, import
zeroize::Zeroize, build the SecretString from a trimmed owned copy (e.g.
SecretString::from(nsec_from_env.trim().to_owned())), then immediately call
nsec_from_env.zeroize() before returning; this wipes the original env buffer
while still returning the SecretString from the trimmed owned data.
In `@src/config/settings.rs`:
- Around line 37-45: Change init_mostro_settings to return a Result (e.g.,
Result<(), anyhow::Error> or your crate's error type) instead of panicking:
replace the expect() calls around take_nsec_for_init, NOSTR_KEYS.set, and
MOSTRO_CONFIG.set with early-return Err variants containing contextual errors;
keep using take_nsec_for_init, NOSTR_KEYS.set, and MOSTRO_CONFIG.set to detect
failures but propagate them via Err(...) (with context) and return Ok(()) on
success; then update callers (e.g., the call site that currently invokes
init_mostro_settings(settings)) to use the ? operator so failures flow through
the existing init_configuration_file() Result path.
In `@src/config/wizard.rs`:
- Around line 158-165: Replace the visible text input for existing nsec in
prompt_nostr_settings: use a non-echoing prompt
(Password::new().with_prompt(...).validate_with(|s: &String|
validate_nsec(s)).interact_password() or the equivalent non-echoing API) instead
of Input::new().interact_text(), then wrap the result in Zeroizing and construct
SecretString::from(...) as before. Also stop printing the raw nsec to stdout
when generating a new keypair; only display the public key (npub) and instruct
the user to securely store the private key (or write it to a secure
file/clipboard) so the nsec is never printed in plain text by the wizard.
---
Nitpick comments:
In `@src/app/context.rs`:
- Around line 233-238: The TestContextBuilder currently parses
settings.nostr.nsec_privkey directly and then passes the original Settings into
AppContext, leaving a plaintext nsec in the shared Settings; instead call the
same helper used in production (take_nsec_for_init or equivalent) to extract and
clear the nsec into Keys and then pass the sanitized Settings into AppContext
(replace the unwrap_or_else parsing with code that invokes take_nsec_for_init on
settings to populate keys and return a Settings without the nsec). Apply the
same change to the similar block around lines 263-265 so tests mirror production
behavior and no Settings clone retains the plaintext nsec.
In `@src/util.rs`:
- Around line 820-826: Add a doc comment to the public function get_keys that
documents its initialization contract and failure semantics: state that callers
must ensure NOSTR_KEYS (or the underlying provider initialized by
crate::config::get_mostro_keys) is set before calling, describe that it returns
a &'static Keys on success and returns a MostroError via
MostroInternalErr(ServiceError::NostrError(...)) when keys are not initialized,
and include any recommended lifecycle guidance (when/where to call
initialization) so callers understand preconditions and error handling for
get_keys.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9d61c4e5-3201-46a2-9cb8-1d4d9f769416
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (13)
Cargo.tomlsrc/app/bond/flow.rssrc/app/context.rssrc/bitcoin_price.rssrc/config/mod.rssrc/config/secret.rssrc/config/settings.rssrc/config/types.rssrc/config/util.rssrc/config/wizard.rssrc/db.rssrc/main.rssrc/util.rs
|
Reviewed this PR — the SecretString/Zeroizing refactor looks clean, and I did not spot any blocking issues. Good move parsing the Nostr keys once at startup and wiping the duplicate plaintext from global settings. |
codaMW
left a comment
codaMW
left a comment
There was a problem hiding this comment.
Tested on regtest, and verified the specific hardening claims rather than just "nothing regressed." ACK.
Build + suite
• Builds clean (the get_keys() owned->borrowed change and the updated call sites in bond/flow.rs, main.rs, db.rs, price/manager.rs all compile).
• Full suite: 403 pass. The single failure is test_lnurl_validation_with_test_server (AddrInUse / port bind) it's in the Lightning subsystem this PR doesn't touch and fails identically on main, so it's a pre-existing local-env flake, not from this PR.
Runtime (no regression)
• Ran the daemon on the branch: starts cleanly (one-time NOSTR_KEYS OnceLock init via take_nsec_for_init), and signs + publishes to Nostr fine (saw a kind-30078 price event published). So signing through the refactored cached get_keys() works after the nsec is parsed once and the Settings copy is cleared no "keys not initialized" / NostrError / signing failures in the log.
Hardening claims actually verified
• nsec wiped from Settings after init: take_nsec_for_init does std::mem::take(&mut nostr.nsec_privkey) then overwrites with SecretString::default(); the test take_nsec_clears_settings_field asserts nsec_privkey is empty afterward passes here.
• redaction: the field is secrecy::SecretString (types.rs:284), so an accidental {:?} of NostrSettings prints [REDACTED] rather than the key.
• no plaintext leak: grep finds zero expose_secret() calls in any logging/print/format path; the wizard prints only the npub ("Generated new Nostr keypair: npub: …"), never the nsec.
• bonus: serialize_omits_secret_fields also passes secrets aren't serialized out of the bond model.
Scope (honest): I verified the behaviors the PR claims Settings is cleared post-init, secrets are redacted in Debug, and nothing logs the plaintext via the included tests + code paths, plus a runtime no-regression check. I did not do a raw memory dump to prove the zeroize buffers are scrubbed from RAM.
Housekeeping: the branch currently shows merge conflicts against main needs a rebase before merge.
2 participants
Summary
nsec_privkeyinsecrecy::SecretStringwithzeroizefor transient buffers (TOML load, wizard input, env reads).Keysonce at startup intoNOSTR_KEYS, then wipe the nsec from globalSettingssoMOSTRO_CONFIG/Arc<Settings>clones no longer retain a duplicate plaintext copy.get_keys()to return the cached&'static Keysinstead of re-parsing on every call.Residual exposure (unchanged)
MOSTRO_NSEC_PRIVKEY) and on-disk.env/settings.tomlremain plaintext by design.nostr_sdk::Keysinternal representation is not zeroized (upstream).Summary by CodeRabbit
Security Improvements
New Features
Bug Fixes