Add private key cli arg #35
Conversation
src/portalnet/discovery.rs
Outdated
| let enr_key = CombinedKey::generate_secp256k1(); | ||
| let enr_key = match config.private_key { | ||
| Some(val) => { | ||
| CombinedKey::secp256k1_from_bytes(val.clone().as_mut_slice()).unwrap() |
There was a problem hiding this comment.
This pr took way longer than it should have, but most of that time was wasted b/c I was using b'0' * 32 as my test private key, but for whatever reason this library rejects the 0 bytestring as an invalid private key. Is this just a security thing? Is this desired behavior? b'0' * 31 + b'1' is accepted, along with all other private keys i've used
There was a problem hiding this comment.
Yup, I don't remember the details of why, but 0 is an invalid key. Rejecting that key is the correct behavior AIUI.
njgheorghita
force-pushed
the
private-key-cli-flag
branch
from
c9dcdcd to
d0ba276
Compare
njgheorghita
force-pushed
the
private-key-cli-flag
branch
from
d0ba276 to
5162d8f
Compare
src/cli.rs
Outdated
| .arg( | ||
| Arg::with_name("private_key") | ||
| .long("private-key") | ||
| .help("Hex encoded 32 byte private key") | ||
| .takes_value(true), | ||
| ) |
There was a problem hiding this comment.
Private keys should not be taken as command line arguments, since they end up in terminal history and in the /proc file system.
Probably should pass a path, and read from there?
There was a problem hiding this comment.
Yup, this makes sense and I def agree. Any objections to me just creating an issue to track the update for now? And consider the update "critical" before any kind of initial release. This pr is mostly to help with simplicity of spinning up a testnet and all the various nodes - which becomes more complex if we require keyfiles. All of the private keys I'm using are not even real (00.01, 00.02, etc).
There was a problem hiding this comment.
Maybe rename the argument to unsafe_private_key?
In any case, my review is nonbinding, I'm only a contributor 🤣
There was a problem hiding this comment.
Hahaha no way, all contributors matter. I updated the long param to unsafe-private-key. left the name private_key as is, since that's internal to the code. Tracking the update here: https://github.com/carver/trin/issues/36
njgheorghita
force-pushed
the
private-key-cli-flag
branch
from
5162d8f to
440fed2
Compare
njgheorghita
force-pushed
the
private-key-cli-flag
branch
from
440fed2 to
752ee61
Compare
README.md
Outdated
| --discovery-port <discovery_port> The UDP port to listen on. [default: 9000] | ||
| --external-address <external_addr> The public IP address and port under which this node is accessible | ||
| --pool-size <pool_size> max size of threadpool [default: 2] | ||
| --private-key <private_key> Hex encoded 32 byte private key |
src/cli.rs
Outdated
| .arg( | ||
| Arg::with_name("private_key") | ||
| .long("unsafe-private-key") | ||
| .help("Hex encoded 32 byte private key (unsafe)") |
There was a problem hiding this comment.
Probably explain the "unsafe" bit in the description for the arg.
| let hex_private_key = value_t!(matches.value_of("private_key"), String)?; | ||
| match hex_private_key.len() { | ||
| 64 => (), | ||
| val => panic!( |
There was a problem hiding this comment.
Maybe could return error instead of panic?
There was a problem hiding this comment.
From what I read - panic! is for unrecoverable errors and Err is for recoverable errors. My assumption was that if a user enters an invalid key - we want to terminate and report. Similarly, we panic with other invalid cli args.
If I understand your suggestion correctly, you're suggesting that we want to use a recoverable error? Is this just for code cleanliness/understanding? Seems like we don't want trin to continue if the key is invalid, since trin'll generate a random key if the user-provided key is invalid, and then the node has a different identity from what the user is expecting.
In terms of ux, imo it seems like the panic! output is slightly less muddled than Err
panic!
thread 'main' panicked at 'Invalid private key length: 6, expected 32 byte hexstring', src/cli.rs:110:24
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
Err
thread 'main' panicked at 'called `Result::unwrap()` on an `Err` value: "Invalid private key length: 6, expected 32 byte hexstring"', src/cli.rs:121:13
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
There was a problem hiding this comment.
Merging for now, but i'll circle back to this in a follow up pr if we want to improve the cli-arg validation strategy
Add a cli arg for custom private keys.