-
Notifications
You must be signed in to change notification settings - Fork 84
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
keys: add command warns on poor key practices #1809
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||
---|---|---|---|---|---|---|---|---|
|
@@ -43,29 +43,30 @@ pub struct Args { | |||||||
|
||||||||
impl Args { | ||||||||
pub fn read_secret(&self) -> Result<Secret, Error> { | ||||||||
let print = Print::new(false); | ||||||||
if let Ok(secret_key) = std::env::var("SOROBAN_SECRET_KEY") { | ||||||||
print.infoln("Read secret key from environment variable SOROBAN_SECRET_KEY"); | ||||||||
Ok(Secret::SecretKey { secret_key }) | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just realized there is no check here that it is a valid secret key. That's what we get for using a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yup, see: |
||||||||
} else if self.secret_key { | ||||||||
println!("Type a secret key: "); | ||||||||
print.kbln("Enter a secret key (starts with S):"); | ||||||||
let secret_key = read_password()?; | ||||||||
let secret_key = PrivateKey::from_string(&secret_key) | ||||||||
.map_err(|_| Error::InvalidSecretKey)? | ||||||||
.to_string(); | ||||||||
Ok(Secret::SecretKey { secret_key }) | ||||||||
} else if self.seed_phrase { | ||||||||
println!("Type a 12 word seed phrase: "); | ||||||||
print.kbln("Enter a 24 word seed phrase:"); | ||||||||
let seed_phrase = read_password()?; | ||||||||
let seed_phrase: Vec<&str> = seed_phrase.split_whitespace().collect(); | ||||||||
// if seed_phrase.len() != 12 { | ||||||||
// let len = seed_phrase.len(); | ||||||||
// return Err(Error::InvalidSeedPhrase { len }); | ||||||||
// } | ||||||||
let seed_words = seed_phrase | ||||||||
.split_whitespace() | ||||||||
.map(ToString::to_string) | ||||||||
.collect::<Vec<_>>(); | ||||||||
let seed_words_len = seed_words.len(); | ||||||||
if seed_words_len < 24 { | ||||||||
print.warnln("Warning, seed phrases containing less than 24 words may not be secure. It is safer to use a 24 word seed. To generate a new key and a 24 word seed phrase use the `stellar keys generate` command."); | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🎉 |
||||||||
} | ||||||||
Ok(Secret::SeedPhrase { | ||||||||
seed_phrase: seed_phrase | ||||||||
.into_iter() | ||||||||
.map(ToString::to_string) | ||||||||
.collect::<Vec<_>>() | ||||||||
.join(" "), | ||||||||
seed_phrase: seed_words.join(" "), | ||||||||
}) | ||||||||
} else { | ||||||||
Err(Error::PasswordRead {}) | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I just noticed that when a user doesn't pass
Perhaps we can add this, or just add a new error message for this case.
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah, just noticed that this PR is closed. I can open a new PR with this small change. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm removing the error in another PR anyway: There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nice, that's even better! |
||||||||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit