fix(faucet): keep keystore password in a SecretString so Debug cannot leak it#829
Conversation
… leak it The faucet Cli derives Debug and held the keystore password as a plain String for the whole process lifetime, so printing or tracing the Cli struct would expose the plaintext password. Wrap it in a reliakit-secret SecretString, which redacts itself in Debug and Display, and read it only at the keystore.decrypt call site via expose_str(). Adds a regression test asserting the password does not appear in the Debug output of the parsed Cli.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
📝 WalkthroughWalkthroughThe faucet CLI is updated to handle the keystore password as a Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@bin/sentrix-faucet/src/main.rs`:
- Around line 541-548: The test at the Cli::try_parse_from call is not hermetic
because it only provides explicit values for --keystore and --password while
leaving 8 other env-backed options (--rpc-url, --listen, --drip-amount,
--chain-id, --ip-window-secs, --ip-max-drips, --addr-cooldown-secs, --tx-fee) to
be read from process environment variables. Add explicit command-line arguments
with appropriate test values for all 8 missing env-backed options to the
Cli::try_parse_from array so the test is completely isolated from the process
environment and runs predictably.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 08de6016-0e4b-4098-b7f8-0f9a986f288b
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock,!**/*.lock
📒 Files selected for processing (2)
bin/sentrix-faucet/Cargo.tomlbin/sentrix-faucet/src/main.rs
| let cli = Cli::try_parse_from([ | ||
| "sentrix-faucet", | ||
| "--keystore", | ||
| "k.json", | ||
| "--password", | ||
| "hunter2-super-secret", | ||
| ]) | ||
| .expect("parse"); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor
🧩 Analysis chain
🏁 Script executed:
# First, let's look at the test code at the specified lines
cd bin/sentrix-faucet && sed -n '541,548p' src/main.rsRepository: sentrix-labs/sentrix
Length of output: 375
🏁 Script executed:
# Find the Cli struct definition to see which fields have env attributes
cd bin/sentrix-faucet && grep -n "struct Cli" src/main.rs | head -5Repository: sentrix-labs/sentrix
Length of output: 174
🏁 Script executed:
# Search for all env = ... attributes in the Cli struct
cd bin/sentrix-faucet && ast-grep outline src/main.rs | grep -A 50 "Cli"Repository: sentrix-labs/sentrix
Length of output: 1738
🏁 Script executed:
# Get the full Cli struct definition to see all fields and their attributes
cd bin/sentrix-faucet && sed -n '51,111p' src/main.rsRepository: sentrix-labs/sentrix
Length of output: 2269
🏁 Script executed:
# Look for the test function that uses Cli::try_parse_from around line 541
cd bin/sentrix-faucet && sed -n '530,560p' src/main.rsRepository: sentrix-labs/sentrix
Length of output: 979
🏁 Script executed:
# Verify clap's behavior with env attributes by checking the clap documentation or source
# Let's also check if there's a MIN_TX_FEE constant defined
cd bin/sentrix-faucet && grep -n "MIN_TX_FEE" src/main.rs | head -10Repository: sentrix-labs/sentrix
Length of output: 378
🌐 Web query:
clap Arg env attribute behavior when not provided on command line
💡 Result:
In the clap crate, the env attribute (or the Arg::env method) specifies that if an argument is not provided on the command line, the value should be retrieved from the specified environment variable, if available [1]. If the argument is not provided on the command line and the environment variable is not set, clap will proceed to use any default values defined for the argument [1]. If no default value is defined and the argument is not marked as required, the argument will simply remain unset (or have a None value, depending on the implementation) [1][2]. Key behaviors regarding the env attribute include: 1. Integration: The env attribute requires the env feature to be enabled for the clap crate [3][4]. 2. Precedence: Values provided via the command line always take precedence over values retrieved from environment variables [1][4]. 3. Reporting: If the user does not provide the argument on the command line but the environment variable is set, clap's ArgMatches::is_present method will return true [1]. 4. Detection: To determine whether the argument was actually provided at runtime (specifically via the command line), you should use ArgMatches::occurrences_of, which will return 0 if the argument was not passed on the command line, even if it was retrieved from an environment variable or set via a default value [1].
Citations:
- 1: https://docs.rs/clap-v3/latest/clap_v3/struct.Arg.html
- 2: Values from environment variables clap-rs/clap#814
- 3: https://docs.rs/clap/latest/aarch64-unknown-linux-gnu/src/clap/_derive/mod.rs.html
- 4: https://rust.code-maven.com/clap/clap-and-environment-variables.html
Make the parser regression test hermetic by providing explicit values for all env-backed options.
Cli::try_parse_from reads every omitted env = ... option from the process environment; clap checks environment variables when arguments are not provided on the command line. The test only provides --keystore and --password, leaving 8 other env-backed fields (--rpc-url, --listen, --drip-amount, --chain-id, --ip-window-secs, --ip-max-drips, --addr-cooldown-secs, --tx-fee) to potentially read from SENTRIX_FAUCET_* environment variables. Stray values in the process environment could cause the test to fail unpredictably before it validates password redaction.
Proposed fix: pass explicit values for all env-backed options
- let cli = Cli::try_parse_from([
- "sentrix-faucet",
- "--keystore",
- "k.json",
- "--password",
- "hunter2-super-secret",
- ])
+ let cli = Cli::try_parse_from(vec![
+ "sentrix-faucet".to_owned(),
+ "--keystore".to_owned(),
+ "k.json".to_owned(),
+ "--password".to_owned(),
+ "hunter2-super-secret".to_owned(),
+ "--rpc-url".to_owned(),
+ "http://127.0.0.1:8545".to_owned(),
+ "--listen".to_owned(),
+ "127.0.0.1:8546".to_owned(),
+ "--drip-amount".to_owned(),
+ (100 * 100_000_000u64).to_string(),
+ "--chain-id".to_owned(),
+ "7120".to_owned(),
+ "--ip-window-secs".to_owned(),
+ "3600".to_owned(),
+ "--ip-max-drips".to_owned(),
+ "3".to_owned(),
+ "--addr-cooldown-secs".to_owned(),
+ "86400".to_owned(),
+ "--tx-fee".to_owned(),
+ MIN_TX_FEE.to_string(),
+ ])
.expect("parse");📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let cli = Cli::try_parse_from([ | |
| "sentrix-faucet", | |
| "--keystore", | |
| "k.json", | |
| "--password", | |
| "hunter2-super-secret", | |
| ]) | |
| .expect("parse"); | |
| let cli = Cli::try_parse_from(vec![ | |
| "sentrix-faucet".to_owned(), | |
| "--keystore".to_owned(), | |
| "k.json".to_owned(), | |
| "--password".to_owned(), | |
| "hunter2-super-secret".to_owned(), | |
| "--rpc-url".to_owned(), | |
| "http://127.0.0.1:8545".to_owned(), | |
| "--listen".to_owned(), | |
| "127.0.0.1:8546".to_owned(), | |
| "--drip-amount".to_owned(), | |
| (100 * 100_000_000u64).to_string(), | |
| "--chain-id".to_owned(), | |
| "7120".to_owned(), | |
| "--ip-window-secs".to_owned(), | |
| "3600".to_owned(), | |
| "--ip-max-drips".to_owned(), | |
| "3".to_owned(), | |
| "--addr-cooldown-secs".to_owned(), | |
| "86400".to_owned(), | |
| "--tx-fee".to_owned(), | |
| MIN_TX_FEE.to_string(), | |
| ]) | |
| .expect("parse"); |
🤖 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 `@bin/sentrix-faucet/src/main.rs` around lines 541 - 548, The test at the
Cli::try_parse_from call is not hermetic because it only provides explicit
values for --keystore and --password while leaving 8 other env-backed options
(--rpc-url, --listen, --drip-amount, --chain-id, --ip-window-secs,
--ip-max-drips, --addr-cooldown-secs, --tx-fee) to be read from process
environment variables. Add explicit command-line arguments with appropriate test
values for all 8 missing env-backed options to the Cli::try_parse_from array so
the test is completely isolated from the process environment and runs
predictably.
1 participant
Problem
The faucet
CliderivesDebugand held the keystore password as a plainStringfor the whole process lifetime. Any{:?}formatting ortracingof theClistruct would print the plaintext password.Fix
Wrap the password in a
reliakit-secretSecretString, which redacts itself inDebug/Display. The plaintext is read only at thekeystore.decrypt(...)call site viaexpose_str().SecretStringdoes not implementFromStr, so a smallwrap_secretclapvalue_parserwraps the raw arg at parse time. Behavior is otherwise unchanged: same--passwordflag, sameSENTRIX_FAUCET_PASSWORDenv var, samehide_env_values.Test
Adds
cli_debug_does_not_leak_password, which parses aCliwith a known password and asserts the plaintext does not appear in theDebugoutput. All faucet tests pass.Notes
This does not change memory-zeroization behavior;
reliakit-secretis redaction-only. The keystore password is consumed once at startup and the decrypted wallet is what lives for the process lifetime.Summary by CodeRabbit
Bug Fixes
Tests