[clap-v3-utils] Deprecate signer source validation (#33802)

* make `SignerSource` implement `Clone`

* add `SignerSourceParserBuilder`

* deprecate `is_keypair`

* deprecate `is_keypair_or_ask_keyword`

* deprecate `is_prompt_signer_source`

* deprecate `is_pubkey_or_keypair`, `is_valid_pubkey`, and `is_valid_signer`

* deprecate `is_pubkey`

* bump deprecation version to 1.17.2

* temporarily allow deprecation for build

* Apply suggestions from code review

Co-authored-by: Tyera <teulberg@gmail.com>

* fix typo `SignerSourceParseBuilder` --> `SignerSourceParserBuilder`

* add `allow_` prefix to `SignerSourceParserBuilder` fields

* remove `SignerSourceParserBuilder::new()` and replace it with `Default`

* Update keygen/src/keygen.rs

Co-authored-by: Trent Nelson <trent.a.b.nelson@gmail.com>

* update deprecated version to `1.18.0`

---------

Co-authored-by: Tyera <teulberg@gmail.com>
Co-authored-by: Trent Nelson <trent.a.b.nelson@gmail.com>
This commit is contained in:
samkim-crypto 2024-01-06 08:19:54 +09:00 committed by GitHub
parent 08082df879
commit 6a9a89064b
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
8 changed files with 379 additions and 4 deletions

View File

@ -11,6 +11,7 @@ pub const FEE_PAYER_ARG: ArgConstant<'static> = ArgConstant {
is also passed. Defaults to the client keypair.",
};
#[allow(deprecated)]
pub fn fee_payer_arg<'a>() -> Arg<'a> {
Arg::new(FEE_PAYER_ARG.name)
.long(FEE_PAYER_ARG.long)

View File

@ -1,9 +1,12 @@
use {
crate::{
input_parsers::{keypair_of, keypairs_of, pubkey_of, pubkeys_of},
keypair::{pubkey_from_path, resolve_signer_from_path, signer_from_path},
keypair::{
parse_signer_source, pubkey_from_path, resolve_signer_from_path, signer_from_path,
SignerSource, SignerSourceError, SignerSourceKind,
},
},
clap::ArgMatches,
clap::{builder::ValueParser, ArgMatches},
solana_remote_wallet::remote_wallet::RemoteWalletManager,
solana_sdk::{
pubkey::Pubkey,
@ -15,6 +18,77 @@ use {
// Sentinel value used to indicate to write to screen instead of file
pub const STDOUT_OUTFILE_TOKEN: &str = "-";
#[derive(Debug, Default)]
pub struct SignerSourceParserBuilder {
allow_prompt: bool,
allow_file_path: bool,
allow_usb: bool,
allow_stdin: bool,
allow_pubkey: bool,
allow_legacy: bool,
}
impl SignerSourceParserBuilder {
pub fn allow_all(mut self) -> Self {
self.allow_prompt = true;
self.allow_file_path = true;
self.allow_usb = true;
self.allow_stdin = true;
self.allow_pubkey = true;
self.allow_legacy = true;
self
}
pub fn allow_prompt(mut self) -> Self {
self.allow_prompt = true;
self
}
pub fn allow_file_path(mut self) -> Self {
self.allow_file_path = true;
self
}
pub fn allow_usb(mut self) -> Self {
self.allow_usb = true;
self
}
pub fn allow_stdin(mut self) -> Self {
self.allow_stdin = true;
self
}
pub fn allow_pubkey(mut self) -> Self {
self.allow_pubkey = true;
self
}
pub fn allow_legacy(mut self) -> Self {
self.allow_legacy = true;
self
}
pub fn build(self) -> ValueParser {
ValueParser::from(
move |arg: &str| -> Result<SignerSource, SignerSourceError> {
let signer_source = parse_signer_source(arg)?;
if !self.allow_legacy && signer_source.legacy {
return Err(SignerSourceError::UnsupportedSource);
}
match signer_source.kind {
SignerSourceKind::Prompt if self.allow_prompt => Ok(signer_source),
SignerSourceKind::Filepath(_) if self.allow_file_path => Ok(signer_source),
SignerSourceKind::Usb(_) if self.allow_usb => Ok(signer_source),
SignerSourceKind::Stdin if self.allow_stdin => Ok(signer_source),
SignerSourceKind::Pubkey(_) if self.allow_pubkey => Ok(signer_source),
_ => Err(SignerSourceError::UnsupportedSource),
}
},
)
}
}
// Return the keypair for an argument with filename `name` or `None` if not present wrapped inside `Result`.
pub fn try_keypair_of(
matches: &ArgMatches,
@ -164,9 +238,11 @@ impl FromStr for PubkeySignature {
mod tests {
use {
super::*,
assert_matches::assert_matches,
clap::{Arg, Command},
solana_sdk::signature::write_keypair_file,
std::fs,
tempfile::NamedTempFile,
};
fn app<'ab>() -> Command<'ab> {
@ -301,4 +377,262 @@ mod tests {
.unwrap_err();
assert_eq!(matches_error.kind, clap::error::ErrorKind::ValueValidation);
}
#[test]
fn test_parse_keypair_source() {
let command = Command::new("test").arg(
Arg::new("keypair")
.long("keypair")
.takes_value(true)
.value_parser(
SignerSourceParserBuilder::default()
.allow_file_path()
.build(),
),
);
// success case
let file0 = NamedTempFile::new().unwrap();
let path = file0.path();
let path_str = path.to_str().unwrap();
let matches = command
.clone()
.try_get_matches_from(vec!["test", "--keypair", path_str])
.unwrap();
let signer_source = matches.get_one::<SignerSource>("keypair").unwrap();
assert!(matches!(signer_source, SignerSource {
kind: SignerSourceKind::Filepath(p),
derivation_path: None,
legacy: false,
}
if p == path_str));
// failure cases
let matches_error = command
.clone()
.try_get_matches_from(vec!["test", "--keypair", "-"])
.unwrap_err();
assert_eq!(matches_error.kind, clap::error::ErrorKind::ValueValidation);
let matches_error = command
.clone()
.try_get_matches_from(vec!["test", "--keypair", "usb://ledger"])
.unwrap_err();
assert_eq!(matches_error.kind, clap::error::ErrorKind::ValueValidation);
}
#[test]
fn test_parse_keypair_or_ask_keyword_source() {
// allow `ASK` keyword
let command = Command::new("test").arg(
Arg::new("keypair")
.long("keypair")
.takes_value(true)
.value_parser(
SignerSourceParserBuilder::default()
.allow_file_path()
.allow_prompt()
.allow_legacy()
.build(),
),
);
// success cases
let file0 = NamedTempFile::new().unwrap();
let path = file0.path();
let path_str = path.to_str().unwrap();
let matches = command
.clone()
.try_get_matches_from(vec!["test", "--keypair", path_str])
.unwrap();
let signer_source = matches.get_one::<SignerSource>("keypair").unwrap();
assert!(matches!(signer_source, SignerSource {
kind: SignerSourceKind::Filepath(p),
derivation_path: None,
legacy: false,
}
if p == path_str));
let matches = command
.clone()
.try_get_matches_from(vec!["test", "--keypair", "ASK"])
.unwrap();
let signer_source = matches.get_one::<SignerSource>("keypair").unwrap();
assert_matches!(
signer_source,
SignerSource {
kind: SignerSourceKind::Prompt,
derivation_path: None,
legacy: true,
}
);
let matches = command
.clone()
.try_get_matches_from(vec!["test", "--keypair", "prompt:"])
.unwrap();
let signer_source = matches.get_one::<SignerSource>("keypair").unwrap();
assert_matches!(
signer_source,
SignerSource {
kind: SignerSourceKind::Prompt,
derivation_path: None,
legacy: false,
}
);
// failure cases
let matches_error = command
.clone()
.try_get_matches_from(vec!["test", "--keypair", "-"])
.unwrap_err();
assert_eq!(matches_error.kind, clap::error::ErrorKind::ValueValidation);
let matches_error = command
.clone()
.try_get_matches_from(vec!["test", "--keypair", "usb://ledger"])
.unwrap_err();
assert_eq!(matches_error.kind, clap::error::ErrorKind::ValueValidation);
// disallow `ASK` keyword
let command = Command::new("test").arg(
Arg::new("keypair")
.long("keypair")
.takes_value(true)
.value_parser(
SignerSourceParserBuilder::default()
.allow_file_path()
.allow_prompt()
.build(),
),
);
let matches_error = command
.clone()
.try_get_matches_from(vec!["test", "--keypair", "ASK"])
.unwrap_err();
assert_eq!(matches_error.kind, clap::error::ErrorKind::ValueValidation);
}
#[test]
fn test_parse_prompt_signer_source() {
let command = Command::new("test").arg(
Arg::new("keypair")
.long("keypair")
.takes_value(true)
.value_parser(
SignerSourceParserBuilder::default()
.allow_prompt()
.allow_legacy()
.build(),
),
);
// success case
let matches = command
.clone()
.try_get_matches_from(vec!["test", "--keypair", "ASK"])
.unwrap();
let signer_source = matches.get_one::<SignerSource>("keypair").unwrap();
assert_matches!(
signer_source,
SignerSource {
kind: SignerSourceKind::Prompt,
derivation_path: None,
legacy: true,
}
);
let matches = command
.clone()
.try_get_matches_from(vec!["test", "--keypair", "prompt:"])
.unwrap();
let signer_source = matches.get_one::<SignerSource>("keypair").unwrap();
assert_matches!(
signer_source,
SignerSource {
kind: SignerSourceKind::Prompt,
derivation_path: None,
legacy: false,
}
);
// failure cases
let matches_error = command
.clone()
.try_get_matches_from(vec!["test", "--keypair", "-"])
.unwrap_err();
assert_eq!(matches_error.kind, clap::error::ErrorKind::ValueValidation);
let matches_error = command
.clone()
.try_get_matches_from(vec!["test", "--keypair", "usb://ledger"])
.unwrap_err();
assert_eq!(matches_error.kind, clap::error::ErrorKind::ValueValidation);
}
#[test]
fn test_parse_pubkey_or_keypair_signer_source() {
let command = Command::new("test").arg(
Arg::new("signer")
.long("signer")
.takes_value(true)
.value_parser(
SignerSourceParserBuilder::default()
.allow_pubkey()
.allow_file_path()
.build(),
),
);
// success cases
let pubkey = Pubkey::new_unique();
let matches = command
.clone()
.try_get_matches_from(vec!["test", "--signer", &pubkey.to_string()])
.unwrap();
let signer_source = matches.get_one::<SignerSource>("signer").unwrap();
assert!(matches!(
signer_source,
SignerSource {
kind: SignerSourceKind::Pubkey(p),
derivation_path: None,
legacy: false,
}
if *p == pubkey));
let file0 = NamedTempFile::new().unwrap();
let path = file0.path();
let path_str = path.to_str().unwrap();
let matches = command
.clone()
.try_get_matches_from(vec!["test", "--signer", path_str])
.unwrap();
let signer_source = matches.get_one::<SignerSource>("signer").unwrap();
assert!(matches!(
signer_source,
SignerSource {
kind: SignerSourceKind::Filepath(p),
derivation_path: None,
legacy: false,
}
if p == path_str));
// failure cases
let matches_error = command
.clone()
.try_get_matches_from(vec!["test", "--signer", "-"])
.unwrap_err();
assert_eq!(matches_error.kind, clap::error::ErrorKind::ValueValidation);
let matches_error = command
.clone()
.try_get_matches_from(vec!["test", "--signer", "usb://ledger"])
.unwrap_err();
assert_eq!(matches_error.kind, clap::error::ErrorKind::ValueValidation);
}
}

View File

@ -59,6 +59,10 @@ where
}
// Return an error if a pubkey cannot be parsed.
#[deprecated(
since = "1.18.0",
note = "please use `clap::value_parser!(Pubkey)` instead"
)]
pub fn is_pubkey(string: &str) -> Result<(), String> {
is_parsable_generic::<Pubkey, _>(string)
}
@ -76,6 +80,10 @@ where
}
// Return an error if a keypair file cannot be parsed.
#[deprecated(
since = "1.18.0",
note = "please use `SignerSourceParserBuilder::default().allow_file_path().build()` instead"
)]
pub fn is_keypair<T>(string: T) -> Result<(), String>
where
T: AsRef<str> + Display,
@ -86,6 +94,10 @@ where
}
// Return an error if a keypair file cannot be parsed
#[deprecated(
since = "1.18.0",
note = "please use `SignerSourceParserBuilder::default().allow_file_path().allow_prompt().allow_legacy().build()` instead"
)]
pub fn is_keypair_or_ask_keyword<T>(string: T) -> Result<(), String>
where
T: AsRef<str> + Display,
@ -99,6 +111,10 @@ where
}
// Return an error if a `SignerSourceKind::Prompt` cannot be parsed
#[deprecated(
since = "1.18.0",
note = "please use `SignerSourceParserBuilder::default().allow_prompt().allow_legacy().build()` instead"
)]
pub fn is_prompt_signer_source(string: &str) -> Result<(), String> {
if string == ASK_KEYWORD {
return Ok(());
@ -115,6 +131,11 @@ pub fn is_prompt_signer_source(string: &str) -> Result<(), String> {
}
// Return an error if string cannot be parsed as pubkey string or keypair file location
#[deprecated(
since = "1.18.0",
note = "please use `SignerSourceParserBuilder::default().allow_pubkey().allow_file_path().build()` instead"
)]
#[allow(deprecated)]
pub fn is_pubkey_or_keypair<T>(string: T) -> Result<(), String>
where
T: AsRef<str> + Display,
@ -124,6 +145,11 @@ where
// Return an error if string cannot be parsed as a pubkey string, or a valid Signer that can
// produce a pubkey()
#[deprecated(
since = "1.18.0",
note = "please use `SignerSourceParserBuilder::default().allow_pubkey().allow_file_path().build()` instead"
)]
#[allow(deprecated)]
pub fn is_valid_pubkey<T>(string: T) -> Result<(), String>
where
T: AsRef<str> + Display,
@ -145,6 +171,11 @@ where
// when paired with an offline `--signer` argument to provide a Presigner (pubkey + signature).
// Clap validators can't check multiple fields at once, so the verification that a `--signer` is
// also provided and correct happens in parsing, not in validation.
#[deprecated(
since = "1.18.0",
note = "please use `SignerSourceParserBuilder::default().build()` instead"
)]
#[allow(deprecated)]
pub fn is_valid_signer<T>(string: T) -> Result<(), String>
where
T: AsRef<str> + Display,
@ -157,6 +188,7 @@ where
since = "1.17.0",
note = "please use `clap::value_parser!(PubkeySignature)` instead"
)]
#[allow(deprecated)]
pub fn is_pubkey_sig<T>(string: T) -> Result<(), String>
where
T: AsRef<str> + Display,

View File

@ -371,7 +371,7 @@ impl DefaultSigner {
}
}
#[derive(Debug)]
#[derive(Debug, Clone)]
pub(crate) struct SignerSource {
pub kind: SignerSourceKind,
pub derivation_path: Option<DerivationPath>,
@ -402,6 +402,7 @@ const SIGNER_SOURCE_USB: &str = "usb";
const SIGNER_SOURCE_STDIN: &str = "stdin";
const SIGNER_SOURCE_PUBKEY: &str = "pubkey";
#[derive(Clone)]
pub(crate) enum SignerSourceKind {
Prompt,
Filepath(String),
@ -439,6 +440,8 @@ pub(crate) enum SignerSourceError {
DerivationPathError(#[from] DerivationPathError),
#[error(transparent)]
IoError(#[from] std::io::Error),
#[error("unsupported source")]
UnsupportedSource,
}
pub(crate) fn parse_signer_source<S: AsRef<str>>(

View File

@ -18,6 +18,7 @@ pub const NONCE_AUTHORITY_ARG: ArgConstant<'static> = ArgConstant {
help: "Provide the nonce authority keypair to use when signing a nonced transaction",
};
#[allow(deprecated)]
fn nonce_arg<'a>() -> Arg<'a> {
Arg::new(NONCE_ARG.name)
.long(NONCE_ARG.long)
@ -27,6 +28,7 @@ fn nonce_arg<'a>() -> Arg<'a> {
.help(NONCE_ARG.help)
}
#[allow(deprecated)]
pub fn nonce_authority_arg<'a>() -> Arg<'a> {
Arg::new(NONCE_AUTHORITY_ARG.name)
.long(NONCE_AUTHORITY_ARG.long)

View File

@ -1,4 +1,5 @@
#![allow(clippy::arithmetic_side_effects)]
#![allow(deprecated)]
use {
bip39::{Mnemonic, MnemonicType, Seed},
clap::{crate_description, crate_name, value_parser, Arg, ArgMatches, Command},

View File

@ -87,7 +87,7 @@ impl From<Infallible> for LocatorError {
}
}
#[derive(Debug, PartialEq, Eq)]
#[derive(Clone, Debug, PartialEq, Eq)]
pub struct Locator {
pub manufacturer: Manufacturer,
pub pubkey: Option<Pubkey>,

View File

@ -1,3 +1,5 @@
#![allow(deprecated)]
use {
bip39::{Mnemonic, MnemonicType, Seed},
clap::{crate_description, crate_name, Arg, ArgMatches, Command},