From 95b50cbe62db16db24091cf5b74778d29698bbba Mon Sep 17 00:00:00 2001 From: plaidfinch Date: Tue, 12 Mar 2024 17:35:32 -0400 Subject: [PATCH] parent 3a9db32d7696f29afaf9cf7a9a65012037dbdd5e author plaidfinch 1710279332 -0400 committer finch 1710280707 -0400 Permit validator voting across an airgap (#3985) This addresses the first changeset described in #3813, permitting validator definitions and votes to be signed separately and those signatures to be broadcast from a potentially-unrelated penumbra account. This is a breaking change to the CLI for all workflows that use the `validator vote` subcommand, because it redefines that command to `validator vote cast` in order to make room for `validator vote sign`. It is purely a client-side change, and as such is not consensus-breaking. - [X] If this code contains consensus-breaking changes, I have added the "consensus-breaking" label. Delete unused import Make formatting a bit nicer, set validator vote sign to offline Allow initializing a separate governance subkey Add a validator command to emit the governance key Use the governance subkey correctly when present, in SoftKMS mode Use the governance key, if configured, in the validator template Fix mistake where the governance key was overzealously used It should only be used to sign *votes*, not *definitions*! Fix bug loading wrong config file path in dkg deal for governance keys --- crates/bin/pcli/src/command/init.rs | 226 +++++++++++++++++------ crates/bin/pcli/src/command/validator.rs | 80 +++++--- crates/bin/pcli/src/config.rs | 26 +++ 3 files changed, 254 insertions(+), 78 deletions(-) diff --git a/crates/bin/pcli/src/command/init.rs b/crates/bin/pcli/src/command/init.rs index c6de54d043..be24359c7b 100644 --- a/crates/bin/pcli/src/command/init.rs +++ b/crates/bin/pcli/src/command/init.rs @@ -11,14 +11,14 @@ use rand_core::OsRng; use url::Url; use crate::{ - config::{CustodyConfig, PcliConfig}, + config::{CustodyConfig, GovernanceCustodyConfig, PcliConfig}, terminal::ActualTerminal, }; #[derive(Debug, clap::Parser)] pub struct InitCmd { #[clap(subcommand)] - pub subcmd: InitSubCmd, + pub subcmd: InitTopSubCmd, /// The GRPC URL that will be used in the generated config. #[clap( long, @@ -33,26 +33,40 @@ pub struct InitCmd { grpc_url: Url, } -#[derive(Debug, clap::Subcommand)] -pub enum InitSubCmd { - /// Initialize `pcli` with a basic, file-based custody backend. - #[clap(subcommand, display_order = 100)] - SoftKms(SoftKmsInitCmd), - /// Initialize `pcli` with a manual threshold signing backend. - #[clap(subcommand, display_order = 150)] - Threshold(ThresholdInitCmd), +#[derive(Debug, Clone, clap::Subcommand)] +pub enum InitTopSubCmd { + #[clap(flatten)] + Spend(InitSubCmd), /// Initialize `pcli` in view-only mode, without spending keys. #[clap(display_order = 200)] ViewOnly { /// The full viewing key for the wallet to view. full_viewing_key: String, }, + /// Initialize a separate validator governance key for an existing `pcli` configuration (this + /// option is only meaningful for validators). + #[clap(subcommand, display_order = 300)] + ValidatorGovernanceSubkey(InitSubCmd), /// Wipe all `pcli` configuration and data, INCLUDING KEYS. #[clap(display_order = 900)] UnsafeWipe {}, } -#[derive(Debug, clap::Subcommand)] +#[derive(Debug, Clone, clap::Subcommand)] +pub enum InitSubCmd { + /// Initialize using a basic, file-based custody backend. + #[clap(subcommand, display_order = 100)] + SoftKms(SoftKmsInitCmd), + /// Initialize using a manual threshold signing backend. + #[clap(subcommand, display_order = 150)] + Threshold(ThresholdInitCmd), + // This is not accessible directly by the user, because it's impermissible to initialize the + // governance subkey as view-only. + #[clap(skip)] + ViewOnly { full_viewing_key: String }, +} + +#[derive(Debug, Clone, clap::Subcommand)] pub enum SoftKmsInitCmd { /// Generate a new seed phrase and import its corresponding key. #[clap(display_order = 100)] @@ -71,15 +85,20 @@ pub enum SoftKmsInitCmd { } impl SoftKmsInitCmd { - fn spend_key(&self) -> Result { + fn spend_key(&self, init_type: InitType) -> Result { Ok(match self { SoftKmsInitCmd::Generate => { let seed_phrase = SeedPhrase::generate(OsRng); - // xxx: Something better should be done here, this is in danger of being + // TODO: Something better should be done here, this is in danger of being // shared by users accidentally in log output. println!( - "YOUR PRIVATE SEED PHRASE:\n{seed_phrase}\nSave this in a safe place!\nDO NOT SHARE WITH ANYONE!" + "YOUR PRIVATE {}SEED PHRASE:\n\n {seed_phrase}\n\nSave this in a safe place!\nDO NOT SHARE WITH ANYONE!\n", + if let InitType::SpendKey = init_type { + "" + } else { + "GOVERNANCE " + }, ); let path = Bip44Path::new(0); @@ -117,7 +136,7 @@ impl SoftKmsInitCmd { } } -#[derive(Debug, clap::Subcommand)] +#[derive(Debug, Clone, clap::Subcommand)] pub enum ThresholdInitCmd { /// Use a centralized dealer to create config files for each signer. /// @@ -146,45 +165,111 @@ pub enum ThresholdInitCmd { }, } -fn exec_deal(threshold: u16, home: Vec, grpc_url: Url) -> Result<()> { +fn exec_deal( + init_type: InitType, + threshold: u16, + home: Vec, + grpc_url: Url, +) -> Result<()> { if threshold < 2 { anyhow::bail!("threshold must be >= 2"); } let n = home.len() as u16; + + // Check before doing anything to make sure that files don't exist (spend key case) or that the + // governance key is missing in all of them (governance key case) -- we do this check first so + // that we don't write partial results if we would fail partway through (though we *also* check + // partway through to reduce chances of a race where we'd overwrite data) + for config_path in home.iter() { + let config_path = config_path.join(crate::CONFIG_FILE_NAME); + if let InitType::GovernanceKey = init_type { + let config = PcliConfig::load(&config_path)?; + if config.governance_custody.is_some() { + anyhow::bail!( + "governance key already exists in config file at {:?}; refusing to overwrite it", + config_path + ); + } + } else if config_path.exists() { + anyhow::bail!( + "config file already exists at {:?}; refusing to overwrite it", + config_path + ); + } + } + println!("Generating {}-of-{} threshold config.", threshold, n); let configs = threshold::Config::deal(&mut OsRng, threshold, n)?; println!("Writing dealt config files..."); - for (i, (config, path)) in configs.into_iter().zip(home.iter()).enumerate() { + for (i, (config, config_path)) in configs.into_iter().zip(home.iter()).enumerate() { let full_viewing_key = config.fvk().clone(); - let config = PcliConfig { - custody: CustodyConfig::Threshold(config), - full_viewing_key, - grpc_url: grpc_url.clone(), - view_url: None, - disable_warning: false, + + let config = if let InitType::SpendKey = init_type { + PcliConfig { + custody: CustodyConfig::Threshold(config), + full_viewing_key, + grpc_url: grpc_url.clone(), + view_url: None, + disable_warning: false, + governance_custody: None, + } + } else { + let mut pcli_config = PcliConfig::load(config_path.join(crate::CONFIG_FILE_NAME))?; + if pcli_config.governance_custody.is_some() { + anyhow::bail!( + "governance key already exists in config file at {:?}; refusing to overwrite it", + config_path + ); + } + pcli_config.governance_custody = Some(GovernanceCustodyConfig::Threshold(config)); + pcli_config }; - println!(" Writing signer {} config to {}", i, path); - std::fs::create_dir_all(path)?; - config.save(path.join(crate::CONFIG_FILE_NAME))?; + + println!(" Writing signer {} config to {}", i, config_path); + std::fs::create_dir_all(config_path)?; + config.save(config_path.join(crate::CONFIG_FILE_NAME))?; } Ok(()) } +/// Which kind of initialization are we doing? +#[derive(Clone, Copy)] +enum InitType { + /// Initialize from scratch with a spend key. + SpendKey, + /// Add a governance key to an existing configuration. + GovernanceKey, +} + impl InitCmd { pub async fn exec(&self, home_dir: impl AsRef) -> Result<()> { - if let InitSubCmd::Threshold(ThresholdInitCmd::Deal { threshold, home }) = &self.subcmd { - exec_deal(threshold.clone(), home.clone(), self.grpc_url.clone())?; + let (init_type, subcmd) = match self.subcmd.clone() { + InitTopSubCmd::Spend(subcmd) => (InitType::SpendKey, subcmd), + InitTopSubCmd::ValidatorGovernanceSubkey(subcmd) => (InitType::GovernanceKey, subcmd), + InitTopSubCmd::ViewOnly { full_viewing_key } => ( + InitType::SpendKey, + InitSubCmd::ViewOnly { full_viewing_key }, + ), + InitTopSubCmd::UnsafeWipe {} => { + println!("Deleting all data in {}...", home_dir.as_ref()); + std::fs::remove_dir_all(home_dir.as_ref())?; + return Ok(()); + } + }; + + if let InitSubCmd::Threshold(ThresholdInitCmd::Deal { threshold, home }) = &subcmd { + exec_deal( + init_type, + threshold.clone(), + home.clone(), + self.grpc_url.clone(), + )?; return Ok(()); } let home_dir = home_dir.as_ref(); - match &self.subcmd { - InitSubCmd::UnsafeWipe {} => { - println!("Deleting all data in {}...", home_dir); - std::fs::remove_dir_all(home_dir)?; - return Ok(()); - } - _ => { + match &init_type { + InitType::SpendKey => { // Check that the data_dir is empty before running init: if home_dir.exists() && home_dir.read_dir()?.next().is_some() { anyhow::bail!( @@ -193,47 +278,80 @@ impl InitCmd { ); } } + InitType::GovernanceKey => { + // Check that there is no existing governance key before running init: + let config_path = home_dir.join(crate::CONFIG_FILE_NAME); + let config = PcliConfig::load(config_path)?; + if config.governance_custody.is_some() { + anyhow::bail!( + "governance key already exists in config file at {:?}; refusing to overwrite it", + home_dir + ); + } + } } - let (full_viewing_key, custody) = match &self.subcmd { - InitSubCmd::UnsafeWipe {} => unreachable!("this case is handled above"), - InitSubCmd::SoftKms(cmd) => { - let spend_key = cmd.spend_key()?; + let (full_viewing_key, custody) = match (&init_type, &subcmd) { + (_, InitSubCmd::SoftKms(cmd)) => { + let spend_key = cmd.spend_key(init_type)?; ( spend_key.full_viewing_key().clone(), CustodyConfig::SoftKms(spend_key.into()), ) } - InitSubCmd::Threshold(ThresholdInitCmd::Dkg { - threshold, - num_participants, - }) => { + ( + _, + InitSubCmd::Threshold(ThresholdInitCmd::Dkg { + threshold, + num_participants, + }), + ) => { let config = threshold::dkg(*threshold, *num_participants, &ActualTerminal).await?; (config.fvk().clone(), CustodyConfig::Threshold(config)) } - InitSubCmd::Threshold(ThresholdInitCmd::Deal { .. }) => { - panic!("this should already have been handled above") + (_, InitSubCmd::Threshold(ThresholdInitCmd::Deal { .. })) => { + unreachable!("this should already have been handled above") } - InitSubCmd::ViewOnly { full_viewing_key } => { + (InitType::SpendKey, InitSubCmd::ViewOnly { full_viewing_key }) => { let full_viewing_key = full_viewing_key.parse()?; (full_viewing_key, CustodyConfig::ViewOnly) } + (InitType::GovernanceKey, InitSubCmd::ViewOnly { .. }) => { + unreachable!("governance keys can't be initialized in view-only mode") + } }; - let config = PcliConfig { - custody, - full_viewing_key, - grpc_url: self.grpc_url.clone(), - view_url: None, - disable_warning: false, + let config = if let InitType::SpendKey = init_type { + PcliConfig { + custody, + full_viewing_key, + grpc_url: self.grpc_url.clone(), + view_url: None, + disable_warning: false, + governance_custody: None, + } + } else { + let config_path = home_dir.join(crate::CONFIG_FILE_NAME); + let mut config = PcliConfig::load(config_path)?; + let governance_custody = match custody { + CustodyConfig::SoftKms(config) => GovernanceCustodyConfig::SoftKms(config), + CustodyConfig::Threshold(config) => GovernanceCustodyConfig::Threshold(config), + _ => unreachable!("governance keys can't be initialized in view-only mode"), + }; + config.governance_custody = Some(governance_custody); + config }; - // Create the config directory, if - let config_path = home_dir.join(crate::CONFIG_FILE_NAME); println!("Writing generated config to {}", config_path); config.save(config_path)?; + if let InitType::GovernanceKey = init_type { + println!("\nIf you defined a validator on-chain before initializing this separate governance subkey, you need to update its definition to use your new public governance key:\n"); + println!(" governance_key = \"{}\"", config.governance_key()); + println!("\nUntil you do this, your validator will not be able to vote on governance proposals, so it's best to do it at your earliest convenience.") + } + Ok(()) } } diff --git a/crates/bin/pcli/src/command/validator.rs b/crates/bin/pcli/src/command/validator.rs index 56f6600182..3cab716788 100644 --- a/crates/bin/pcli/src/command/validator.rs +++ b/crates/bin/pcli/src/command/validator.rs @@ -20,11 +20,14 @@ use penumbra_proto::{ use penumbra_stake::{ validator, validator::{Validator, ValidatorToml}, - FundingStream, FundingStreams, GovernanceKey, IdentityKey, + FundingStream, FundingStreams, IdentityKey, }; use penumbra_wallet::plan; -use crate::{config::CustodyConfig, App}; +use crate::{ + config::{CustodyConfig, GovernanceCustodyConfig}, + App, +}; #[derive(Debug, clap::Subcommand)] pub enum ValidatorCmd { @@ -34,6 +37,12 @@ pub enum ValidatorCmd { #[clap(long)] base64: bool, }, + /// Display the validator's governance subkey derived from this wallet's governance seed. + GovernanceKey { + /// Use Base64 encoding for the governance key, rather than the default of Bech32. + #[clap(long)] + base64: bool, + }, /// Manage your validator's definition. #[clap(subcommand)] Definition(DefinitionCmd), @@ -125,7 +134,7 @@ pub enum DefinitionCmd { #[clap(short = 'k', long)] tendermint_validator_keyfile: Option, }, - /// Fetches the definition for your validator. + /// Fetches the definition for your validator Fetch { /// The JSON file to write the definition to [default: stdout]. #[clap(long)] @@ -137,6 +146,7 @@ impl ValidatorCmd { pub fn offline(&self) -> bool { match self { ValidatorCmd::Identity { .. } => true, + ValidatorCmd::GovernanceKey { .. } => true, ValidatorCmd::Definition( DefinitionCmd::Template { .. } | DefinitionCmd::Sign { .. }, ) => true, @@ -163,6 +173,16 @@ impl ValidatorCmd { println!("{ik}"); } } + ValidatorCmd::GovernanceKey { base64 } => { + let gk = app.config.governance_key(); + + if *base64 { + use base64::{display::Base64Display, engine::general_purpose::STANDARD}; + println!("{}", Base64Display::new(&gk.0.to_bytes(), &STANDARD)); + } else { + println!("{gk}"); + } + } ValidatorCmd::Definition(DefinitionCmd::Sign { file, signature_file, @@ -250,7 +270,9 @@ impl ValidatorCmd { let sk = match &app.config.custody { CustodyConfig::SoftKms(config) => config.spend_key.clone(), _ => { - anyhow::bail!("local validator definition signing currently requires SoftKMS backend"); + anyhow::bail!( + "local validator definition signing currently requires SoftKMS backend" + ); } }; sk.spend_auth_key().sign(OsRng, &v_bytes) @@ -283,10 +305,7 @@ impl ValidatorCmd { signature_file, }) => { let identity_key = IdentityKey(fvk.spend_verification_key().clone()); - - // Currently this is always just copied from the identity key - // TODO: support a separate governance key - let governance_key = GovernanceKey(identity_key.0); + let governance_key = app.config.governance_key(); let (proposal, vote): (u64, Vote) = (*vote).into(); @@ -303,12 +322,20 @@ impl ValidatorCmd { reason: ValidatorVoteReason(reason.clone()), }; - // TODO: support signing with a separate governance key rather than the spend auth key - let sk = match &app.config.custody { - CustodyConfig::SoftKms(config) => config.spend_key.clone(), - _ => { + // TODO: use the custody abstraction to sign + let sk = match &app.config.governance_custody { + None => match &app.config.custody { + CustodyConfig::SoftKms(config) => config.spend_key.clone(), + _ => { + anyhow::bail!( + "local validator definition signing currently requires SoftKMS backend" + ); + } + }, + Some(GovernanceCustodyConfig::SoftKms(config)) => config.spend_key.clone(), + Some(_) => { anyhow::bail!( - "local validator vote signing currently requires SoftKMS backend" + "local validator definition signing currently requires SoftKMS backend" ); } }; @@ -346,10 +373,7 @@ impl ValidatorCmd { signature, }) => { let identity_key = IdentityKey(fvk.spend_verification_key().clone()); - - // Currently this is always just copied from the identity key - // TODO: support a separate governance key - let governance_key = GovernanceKey(identity_key.0); + let governance_key = app.config.governance_key(); let (proposal, vote): (u64, Vote) = (*vote).into(); @@ -383,13 +407,21 @@ impl ValidatorCmd { ) .context("unable to parse decoded signature")? } else { - // TODO: support signing with a separate governance key rather than the spend auth key - let sk = match &app.config.custody { - CustodyConfig::SoftKms(config) => config.spend_key.clone(), - _ => { + // TODO: use the custody abstraction to sign + let sk = match &app.config.governance_custody { + None => match &app.config.custody { + CustodyConfig::SoftKms(config) => config.spend_key.clone(), + _ => { + anyhow::bail!( + "local validator definition signing currently requires SoftKMS backend" + ); + } + }, + Some(GovernanceCustodyConfig::SoftKms(config)) => config.spend_key.clone(), + Some(_) => { anyhow::bail!( - "local validator vote signing currently requires SoftKMS backend" - ); + "local validator definition signing currently requires SoftKMS backend" + ); } }; let governance_auth_key = sk.spend_auth_key(); @@ -427,7 +459,7 @@ impl ValidatorCmd { // By default, the template sets the governance key to the same verification key as // the identity key, but a validator can change this if they want to use different // key material. - let governance_key = GovernanceKey(identity_key.0); + let governance_key = app.config.governance_key(); // Honor the filepath to `priv_validator_key.json`, if set. Otherwise, generate // a random pubkey and emit a warning about it. diff --git a/crates/bin/pcli/src/config.rs b/crates/bin/pcli/src/config.rs index d9b1155082..eafb05ee60 100644 --- a/crates/bin/pcli/src/config.rs +++ b/crates/bin/pcli/src/config.rs @@ -1,6 +1,7 @@ use std::path::Path; use anyhow::{Context, Result}; +use penumbra_stake::GovernanceKey; use serde::{Deserialize, Serialize}; use serde_with::{serde_as, DisplayFromStr}; use url::Url; @@ -24,6 +25,8 @@ pub struct PcliConfig { pub full_viewing_key: FullViewingKey, /// The custody backend to use. pub custody: CustodyConfig, + /// The governance custody backend to use. + pub governance_custody: Option, } impl PcliConfig { @@ -40,6 +43,17 @@ impl PcliConfig { std::fs::write(path, contents)?; Ok(()) } + + pub fn governance_key(&self) -> GovernanceKey { + let fvk = match &self.governance_custody { + Some(GovernanceCustodyConfig::SoftKms(SoftKmsConfig { spend_key, .. })) => { + spend_key.full_viewing_key() + } + Some(GovernanceCustodyConfig::Threshold(threshold_config)) => threshold_config.fvk(), + None => &self.full_viewing_key, + }; + GovernanceKey(fvk.spend_verification_key().clone()) + } } /// The custody backend to use. @@ -55,6 +69,17 @@ pub enum CustodyConfig { Threshold(ThresholdConfig), } +/// The governance custody backend to use. +#[derive(Serialize, Deserialize, Clone, Debug, Eq, PartialEq)] +#[serde(tag = "backend")] +#[allow(clippy::large_enum_variant)] +pub enum GovernanceCustodyConfig { + /// A software key management service. + SoftKms(SoftKmsConfig), + /// A manual threshold custody service. + Threshold(ThresholdConfig), +} + impl Default for CustodyConfig { fn default() -> Self { Self::ViewOnly @@ -86,6 +111,7 @@ mod tests { custody: CustodyConfig::SoftKms(SoftKmsConfig::from( penumbra_keys::test_keys::SPEND_KEY.clone(), )), + governance_custody: None, }; let mut config2 = config.clone();