From f4012ca04406c0941d2322db436e7959661d5ee3 Mon Sep 17 00:00:00 2001 From: Matthew Smith Date: Tue, 18 Oct 2022 23:11:27 +0700 Subject: [PATCH 01/24] Use aws mod --- chains/nomad-substrate/src/lib.rs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/chains/nomad-substrate/src/lib.rs b/chains/nomad-substrate/src/lib.rs index 98a644e8..3dac25b3 100644 --- a/chains/nomad-substrate/src/lib.rs +++ b/chains/nomad-substrate/src/lib.rs @@ -28,6 +28,9 @@ pub use client::*; mod signer; pub use signer::*; +mod aws; +pub use aws::*; + #[macro_use] mod macros; pub use macros::*; From b8f5e525781b0a94740327837e2185c3e6a9c0e2 Mon Sep 17 00:00:00 2001 From: Matthew Smith Date: Tue, 18 Oct 2022 23:15:08 +0700 Subject: [PATCH 02/24] Add AWS signer init and trait bound --- chains/nomad-substrate/src/signer.rs | 24 +++++++++++++++++++----- 1 file changed, 19 insertions(+), 5 deletions(-) diff --git a/chains/nomad-substrate/src/signer.rs b/chains/nomad-substrate/src/signer.rs index 0a2310dc..beb0bf22 100644 --- a/chains/nomad-substrate/src/signer.rs +++ b/chains/nomad-substrate/src/signer.rs @@ -1,3 +1,4 @@ +use crate::aws::FromAwsId; use async_trait::async_trait; use color_eyre::{eyre::bail, Result}; use nomad_core::FromSignerConf; @@ -10,7 +11,7 @@ use subxt::{ Config, }; -/// Error types for EthereumSigners +/// Error types for SubstrateSigners #[derive(Debug, thiserror::Error)] pub enum SubstrateSignersError { /// Local signer configuration error @@ -25,9 +26,11 @@ impl From for SubstrateSignersError { } /// Substrate signer variants -pub enum SubstrateSigners { +pub enum SubstrateSigners { /// Local signer, instantiated from local private key Local(PairSigner), + /// A signer using a key stored in AWS KMS + Aws(PairSigner), } #[async_trait] @@ -39,7 +42,7 @@ where ::AccountId: Into<::Address>, ::Address: std::fmt::Display, ::AccountId: std::fmt::Display, - P: Pair, + P: Pair + FromAwsId, P::Public: std::fmt::Display, { async fn try_from_signer_conf(conf: &SignerConf) -> Result { @@ -55,13 +58,20 @@ where Ok(Self::Local(pair_signer)) } - SignerConf::Aws { .. } => bail!("No AWS signer support"), + SignerConf::Aws { id } => { + let pair = P::from_aws_id(id); + let pair_signer = PairSigner::::new(pair); + let account_id = pair_signer.account_id(); + tracing::info!("Tx signer AccountId: {}", account_id); + + Ok(Self::Aws(pair_signer)) + } SignerConf::Node => bail!("No node signer support"), } } } -impl Signer for SubstrateSigners +impl Signer for SubstrateSigners where T: Config, T::Signature: From, @@ -73,24 +83,28 @@ where fn nonce(&self) -> Option<::Index> { match self { SubstrateSigners::Local(signer) => signer.nonce(), + SubstrateSigners::Aws(signer) => signer.nonce(), } } fn account_id(&self) -> &::AccountId { match self { SubstrateSigners::Local(signer) => signer.account_id(), + SubstrateSigners::Aws(signer) => signer.account_id(), } } fn address(&self) -> ::Address { match self { SubstrateSigners::Local(signer) => signer.address(), + SubstrateSigners::Aws(signer) => signer.address(), } } fn sign(&self, signer_payload: &[u8]) -> ::Signature { match self { SubstrateSigners::Local(signer) => signer.sign(signer_payload), + SubstrateSigners::Aws(signer) => signer.sign(signer_payload), } } } From c5cfaaed6a61913831838e02aebdea48302446c1 Mon Sep 17 00:00:00 2001 From: Matthew Smith Date: Wed, 19 Oct 2022 20:48:46 +0700 Subject: [PATCH 03/24] Update dependencies --- Cargo.lock | 3 +++ chains/nomad-substrate/Cargo.toml | 3 +++ 2 files changed, 6 insertions(+) diff --git a/Cargo.lock b/Cargo.lock index 78532187..48d65d79 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3334,6 +3334,7 @@ dependencies = [ "color-eyre", "env_logger", "ethers-core", + "ethers-signers", "futures", "hex", "nomad-core", @@ -3343,6 +3344,8 @@ dependencies = [ "parity-scale-codec", "parity-util-mem", "primitive-types 0.11.1 (git+https://github.com/paritytech/parity-common.git?branch=master)", + "rusoto_core", + "rusoto_kms", "scale-info", "serde 1.0.137", "serde_json", diff --git a/chains/nomad-substrate/Cargo.toml b/chains/nomad-substrate/Cargo.toml index b1da4315..5df02129 100644 --- a/chains/nomad-substrate/Cargo.toml +++ b/chains/nomad-substrate/Cargo.toml @@ -28,9 +28,12 @@ tracing-futures = "0.2.5" thiserror = "1.0.30" once_cell = "1.8.0" primitive-types = { git = "https://github.com/paritytech/parity-common.git", branch = "master", features = ["serde"] } +rusoto_core = "0.48.0" +rusoto_kms = "0.48.0" nomad-xyz-configuration = { path = "../../configuration" } nomad-core = { path = "../../nomad-core" } nomad-types = { path = "../../nomad-types" } ethers-core = { git = "https://github.com/gakonst/ethers-rs", branch = "master" } +ethers-signers = { git = "https://github.com/gakonst/ethers-rs", branch = "master", features = ["aws"] } From c49f8327bb0f7a1eaedab7337b4522e4f2039086 Mon Sep 17 00:00:00 2001 From: Matthew Smith Date: Wed, 19 Oct 2022 20:49:01 +0700 Subject: [PATCH 04/24] Stub out AWS Pair --- chains/nomad-substrate/src/aws.rs | 192 ++++++++++++++++++++++++++++++ 1 file changed, 192 insertions(+) create mode 100644 chains/nomad-substrate/src/aws.rs diff --git a/chains/nomad-substrate/src/aws.rs b/chains/nomad-substrate/src/aws.rs new file mode 100644 index 00000000..1c12eeea --- /dev/null +++ b/chains/nomad-substrate/src/aws.rs @@ -0,0 +1,192 @@ +use subxt::error::SecretStringError; +use subxt::ext::{ + sp_core::{ + crypto::{CryptoTypePublicPair, Derive}, + ecdsa, ByteArray, DeriveJunction, Pair as TraitPair, Public as TraitPublic, + }, + sp_runtime::{CryptoType, MultiSignature, MultiSigner}, +}; + +/// A partially implemented Pair (`subxt::ext::sp_core::Pair`) that +/// will support a remote AWS signer using ECDSA +#[derive(Clone, Debug)] +pub struct Pair; + +impl Pair { + /// Create a new AWS Pair from an AWS id + pub fn new>(_id: T) -> Self { + todo!() + } +} + +/// To make `Pair` init from an AWS id while keeping our internal signer +/// generic over all `subxt::ext::sp_core::Pair` and `subxt::Config`. +/// This will be implemented as a noop for `subxt::ext::sp_core::ecdsa::Pair` +/// and other core implementations +pub trait FromAwsId { + /// Create an AWS-compatible signer from an AWS id + fn from_aws_id>(_id: T) -> Self; +} + +impl FromAwsId for Pair { + fn from_aws_id>(id: T) -> Self { + Pair::new(id) + } +} + +impl FromAwsId for ecdsa::Pair { + fn from_aws_id>(_id: T) -> Self { + unimplemented!("For compatibility only, ecdsa::Pair cannot be created from an AWS id") + } +} + +/// A `Public` key that is compatible with `subxt::ext::sp_core::Pair` +/// and AWS's ECDSA KMS signer +#[derive(Clone, Copy, Eq, PartialEq, PartialOrd, Ord, Hash)] +pub struct Public(pub [u8; 33]); + +impl Derive for Public {} + +impl CryptoType for Public { + type Pair = Pair; +} + +impl ByteArray for Public { + const LEN: usize = 33; +} + +impl std::fmt::Display for Public { + fn fmt(&self, _f: &mut std::fmt::Formatter) -> std::fmt::Result { + todo!() + } +} + +impl AsRef<[u8]> for Public { + fn as_ref(&self) -> &[u8] { + todo!() + } +} + +impl AsMut<[u8]> for Public { + fn as_mut(&mut self) -> &mut [u8] { + todo!() + } +} + +impl TryFrom<&[u8]> for Public { + type Error = (); + + fn try_from(_value: &[u8]) -> Result { + todo!() + } +} + +impl TraitPublic for Public { + fn to_public_crypto_pair(&self) -> CryptoTypePublicPair { + todo!() + } +} + +impl From for MultiSigner { + fn from(_x: Public) -> Self { + todo!() + } +} + +/// A `Signature` that is compatible with `subxt::ext::sp_core::Pair` +/// and AWS's ECDSA KMS signer +#[derive(PartialEq, Eq, Hash)] +pub struct Signature(pub [u8; 65]); + +impl From for MultiSignature { + fn from(_x: Signature) -> Self { + todo!() + } +} + +impl AsRef<[u8]> for Signature { + fn as_ref(&self) -> &[u8] { + &self.0[..] + } +} + +/// We only need this to satisfy the associated type on +/// `subxt::ext::sp_core::Pair`, so we'll make it a seed of length zero +type DummySeed = [u8; 0]; + +/// The trait `subxt::ext::sp_core::Pair` handles signing, verification and the creation +/// of keypairs form local key material (mnemonics, random bytes, etc.). With a remote +/// AWS signer keypair creation is handled externally so we will only partially implement +/// `Pair` to reflect this. +impl TraitPair for Pair { + type Public = Public; + type Seed = DummySeed; + type Signature = Signature; + type DeriveError = (); + + /// Our `Public` key + fn public(&self) -> Self::Public { + Public([0; 33]) + } + + /// Sign a message of arbitrary bytes to return a `Signature` + fn sign(&self, _message: &[u8]) -> Self::Signature { + // let rt = tokio::runtime::Builder::new_current_thread() + // .enable_all() + // .build() + // .expect("TODO"); + // rt.block_on(async { + // // call async sign + // }); + Signature([0; 65]) + } + + fn verify>(_sig: &Self::Signature, _message: M, _pubkey: &Self::Public) -> bool { + todo!() + } + + fn verify_weak, M: AsRef<[u8]>>(_sig: &[u8], _message: M, _pubkey: P) -> bool { + todo!() + } + + /// Not implemented for AWS Pair + fn generate_with_phrase(_password: Option<&str>) -> (Self, String, Self::Seed) { + unimplemented!("Pair cannot be created with local key material") + } + + /// Not implemented for AWS Pair + fn from_phrase( + _phrase: &str, + _password: Option<&str>, + ) -> Result<(Self, Self::Seed), SecretStringError> { + unimplemented!("Pair cannot be created with local key material") + } + + /// Not implemented for AWS Pair + fn derive>( + &self, + _path: Iter, + _seed: Option, + ) -> Result<(Self, Option), Self::DeriveError> { + unimplemented!("Pair does not support derivation") + } + + /// Not implemented for AWS Pair + fn from_seed(_seed: &Self::Seed) -> Self { + unimplemented!("Pair cannot be created with local key material") + } + + /// Not implemented for AWS Pair + fn from_seed_slice(_seed: &[u8]) -> Result { + unimplemented!("Pair cannot be created with local key material") + } + + /// Not implemented for AWS Pair + fn to_raw_vec(&self) -> Vec { + unimplemented!("Pair does not have access to key material") + } +} + +impl CryptoType for Pair { + type Pair = Pair; +} From cfc4fc2425d2c48f6309409da9c0fd8b79c1eadb Mon Sep 17 00:00:00 2001 From: Matthew Smith Date: Wed, 19 Oct 2022 21:29:43 +0700 Subject: [PATCH 05/24] Stub out AWS pair impl --- chains/nomad-substrate/src/aws.rs | 23 +++++++++++++---------- 1 file changed, 13 insertions(+), 10 deletions(-) diff --git a/chains/nomad-substrate/src/aws.rs b/chains/nomad-substrate/src/aws.rs index 1c12eeea..1bd2a26e 100644 --- a/chains/nomad-substrate/src/aws.rs +++ b/chains/nomad-substrate/src/aws.rs @@ -17,6 +17,16 @@ impl Pair { pub fn new>(_id: T) -> Self { todo!() } + + fn public_remote(&self) -> Public { + todo!() + } + + // TODO: Since Pair::sign is infallible, we will have to have a retry count + // TODO: followed by a panic here if we can't remote sign + fn sign_remote(&self, message: &[u8]) -> Signature { + todo!() + } } /// To make `Pair` init from an AWS id while keeping our internal signer @@ -126,19 +136,12 @@ impl TraitPair for Pair { /// Our `Public` key fn public(&self) -> Self::Public { - Public([0; 33]) + self.public_remote() } /// Sign a message of arbitrary bytes to return a `Signature` - fn sign(&self, _message: &[u8]) -> Self::Signature { - // let rt = tokio::runtime::Builder::new_current_thread() - // .enable_all() - // .build() - // .expect("TODO"); - // rt.block_on(async { - // // call async sign - // }); - Signature([0; 65]) + fn sign(&self, message: &[u8]) -> Self::Signature { + self.sign_remote(message) } fn verify>(_sig: &Self::Signature, _message: M, _pubkey: &Self::Public) -> bool { From 3d86880c1745d2b060dbeb97cd148a51dd09939d Mon Sep 17 00:00:00 2001 From: Matthew Smith Date: Wed, 19 Oct 2022 22:04:39 +0700 Subject: [PATCH 06/24] Make FromAwsId async, Sized and Send --- chains/nomad-substrate/src/aws.rs | 33 ++++++++++++++++++++++------ chains/nomad-substrate/src/signer.rs | 2 +- 2 files changed, 27 insertions(+), 8 deletions(-) diff --git a/chains/nomad-substrate/src/aws.rs b/chains/nomad-substrate/src/aws.rs index 1bd2a26e..0a82b9ac 100644 --- a/chains/nomad-substrate/src/aws.rs +++ b/chains/nomad-substrate/src/aws.rs @@ -1,3 +1,6 @@ +use async_trait::async_trait; +use color_eyre::Result; +use ethers_signers::AwsSigner as EthersAwsSigner; use subxt::error::SecretStringError; use subxt::ext::{ sp_core::{ @@ -9,12 +12,16 @@ use subxt::ext::{ /// A partially implemented Pair (`subxt::ext::sp_core::Pair`) that /// will support a remote AWS signer using ECDSA -#[derive(Clone, Debug)] +#[derive(Clone)] pub struct Pair; impl Pair { /// Create a new AWS Pair from an AWS id - pub fn new>(_id: T) -> Self { + pub async fn new(id: T) -> Result + where + T: AsRef + Send, + { + // let signer = EthersAwsSigner::new().await?; todo!() } @@ -24,7 +31,7 @@ impl Pair { // TODO: Since Pair::sign is infallible, we will have to have a retry count // TODO: followed by a panic here if we can't remote sign - fn sign_remote(&self, message: &[u8]) -> Signature { + fn sign_remote(&self, _message: &[u8]) -> Signature { todo!() } } @@ -33,19 +40,31 @@ impl Pair { /// generic over all `subxt::ext::sp_core::Pair` and `subxt::Config`. /// This will be implemented as a noop for `subxt::ext::sp_core::ecdsa::Pair` /// and other core implementations +#[async_trait] pub trait FromAwsId { /// Create an AWS-compatible signer from an AWS id - fn from_aws_id>(_id: T) -> Self; + async fn from_aws_id(id: T) -> Result + where + Self: Sized, + T: AsRef + Send; } +#[async_trait] impl FromAwsId for Pair { - fn from_aws_id>(id: T) -> Self { - Pair::new(id) + async fn from_aws_id(id: T) -> Result + where + T: AsRef + Send, + { + Pair::new(id).await } } +#[async_trait] impl FromAwsId for ecdsa::Pair { - fn from_aws_id>(_id: T) -> Self { + async fn from_aws_id>(_id: T) -> Result + where + T: AsRef + Send, + { unimplemented!("For compatibility only, ecdsa::Pair cannot be created from an AWS id") } } diff --git a/chains/nomad-substrate/src/signer.rs b/chains/nomad-substrate/src/signer.rs index beb0bf22..fa7b70ed 100644 --- a/chains/nomad-substrate/src/signer.rs +++ b/chains/nomad-substrate/src/signer.rs @@ -59,7 +59,7 @@ where Ok(Self::Local(pair_signer)) } SignerConf::Aws { id } => { - let pair = P::from_aws_id(id); + let pair = P::from_aws_id(id).await?; let pair_signer = PairSigner::::new(pair); let account_id = pair_signer.account_id(); tracing::info!("Tx signer AccountId: {}", account_id); From f1834c922a0c56fa4af153a03c41b7c9be5b318c Mon Sep 17 00:00:00 2001 From: Matthew Smith Date: Wed, 19 Oct 2022 22:08:27 +0700 Subject: [PATCH 07/24] Make FromAwsId Sync --- chains/nomad-substrate/src/aws.rs | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/chains/nomad-substrate/src/aws.rs b/chains/nomad-substrate/src/aws.rs index 0a82b9ac..00cb601a 100644 --- a/chains/nomad-substrate/src/aws.rs +++ b/chains/nomad-substrate/src/aws.rs @@ -1,6 +1,7 @@ use async_trait::async_trait; use color_eyre::Result; use ethers_signers::AwsSigner as EthersAwsSigner; +use nomad_core::aws::get_kms_client; use subxt::error::SecretStringError; use subxt::ext::{ sp_core::{ @@ -19,9 +20,10 @@ impl Pair { /// Create a new AWS Pair from an AWS id pub async fn new(id: T) -> Result where - T: AsRef + Send, + T: AsRef + Send + Sync, { - // let signer = EthersAwsSigner::new().await?; + let kms_client = get_kms_client().await; + let _signer = EthersAwsSigner::new(kms_client, id, 0).await?; todo!() } @@ -45,15 +47,15 @@ pub trait FromAwsId { /// Create an AWS-compatible signer from an AWS id async fn from_aws_id(id: T) -> Result where - Self: Sized, - T: AsRef + Send; + T: AsRef + Send + Sync, + Self: Sized; } #[async_trait] impl FromAwsId for Pair { async fn from_aws_id(id: T) -> Result where - T: AsRef + Send, + T: AsRef + Send + Sync, { Pair::new(id).await } @@ -63,7 +65,7 @@ impl FromAwsId for Pair { impl FromAwsId for ecdsa::Pair { async fn from_aws_id>(_id: T) -> Result where - T: AsRef + Send, + T: AsRef + Send + Sync, { unimplemented!("For compatibility only, ecdsa::Pair cannot be created from an AWS id") } From 1bd2e3f5f584083029acb9b7fb3788ce120d8b3d Mon Sep 17 00:00:00 2001 From: Matthew Smith Date: Wed, 19 Oct 2022 23:12:04 +0700 Subject: [PATCH 08/24] Add pubkey support --- chains/nomad-substrate/src/aws.rs | 52 ++++++++++++++++++++++++++----- 1 file changed, 45 insertions(+), 7 deletions(-) diff --git a/chains/nomad-substrate/src/aws.rs b/chains/nomad-substrate/src/aws.rs index 00cb601a..07b664ae 100644 --- a/chains/nomad-substrate/src/aws.rs +++ b/chains/nomad-substrate/src/aws.rs @@ -1,20 +1,31 @@ use async_trait::async_trait; use color_eyre::Result; +use ethers_core::k256::ecdsa::VerifyingKey as EthersVerifyingKey; use ethers_signers::AwsSigner as EthersAwsSigner; use nomad_core::aws::get_kms_client; use subxt::error::SecretStringError; use subxt::ext::{ sp_core::{ - crypto::{CryptoTypePublicPair, Derive}, + crypto::{CryptoTypePublicPair, Derive, UncheckedFrom}, ecdsa, ByteArray, DeriveJunction, Pair as TraitPair, Public as TraitPublic, }, sp_runtime::{CryptoType, MultiSignature, MultiSigner}, }; +#[derive(Debug, thiserror::Error)] +pub enum AwsPairError { + /// Dummy error + #[error("Dummy error")] + DummyError(), +} + /// A partially implemented Pair (`subxt::ext::sp_core::Pair`) that /// will support a remote AWS signer using ECDSA #[derive(Clone)] -pub struct Pair; +pub struct Pair { + signer: EthersAwsSigner<'static>, + pubkey: Public, +} impl Pair { /// Create a new AWS Pair from an AWS id @@ -23,12 +34,19 @@ impl Pair { T: AsRef + Send + Sync, { let kms_client = get_kms_client().await; - let _signer = EthersAwsSigner::new(kms_client, id, 0).await?; - todo!() + let signer = EthersAwsSigner::new(kms_client, id, 0) + .await + .map_err(|_| AwsPairError::DummyError())?; + let pubkey = signer + .get_pubkey() + .await + .map_err(|_| AwsPairError::DummyError())?; + let pubkey = pubkey.try_into().map_err(|_| AwsPairError::DummyError())?; + Ok(Self { signer, pubkey }) } fn public_remote(&self) -> Public { - todo!() + self.pubkey.clone() } // TODO: Since Pair::sign is infallible, we will have to have a retry count @@ -76,6 +94,12 @@ impl FromAwsId for ecdsa::Pair { #[derive(Clone, Copy, Eq, PartialEq, PartialOrd, Ord, Hash)] pub struct Public(pub [u8; 33]); +impl UncheckedFrom<[u8; 33]> for Public { + fn unchecked_from(x: [u8; 33]) -> Self { + Public(x) + } +} + impl Derive for Public {} impl CryptoType for Public { @@ -104,11 +128,25 @@ impl AsMut<[u8]> for Public { } } +impl TryFrom for Public { + type Error = (); + + fn try_from(data: EthersVerifyingKey) -> Result { + let data = data.to_bytes(); + TryFrom::<&[u8]>::try_from(data.as_slice()) + } +} + impl TryFrom<&[u8]> for Public { type Error = (); - fn try_from(_value: &[u8]) -> Result { - todo!() + fn try_from(data: &[u8]) -> Result { + if data.len() != Self::LEN { + return Err(()); + } + let mut r = [0u8; Self::LEN]; + r.copy_from_slice(data); + Ok(Self::unchecked_from(r)) } } From b70ed7c67f145e0e57f9f6bf9af9948ed004dbcb Mon Sep 17 00:00:00 2001 From: Matthew Smith Date: Wed, 19 Oct 2022 23:33:23 +0700 Subject: [PATCH 09/24] Clean up lint errors --- chains/nomad-substrate/src/aws.rs | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/chains/nomad-substrate/src/aws.rs b/chains/nomad-substrate/src/aws.rs index 07b664ae..60b01c8d 100644 --- a/chains/nomad-substrate/src/aws.rs +++ b/chains/nomad-substrate/src/aws.rs @@ -12,6 +12,9 @@ use subxt::ext::{ sp_runtime::{CryptoType, MultiSignature, MultiSigner}, }; +// TODO: Rename things + +/// Error types for AWS `Pair` #[derive(Debug, thiserror::Error)] pub enum AwsPairError { /// Dummy error @@ -23,7 +26,7 @@ pub enum AwsPairError { /// will support a remote AWS signer using ECDSA #[derive(Clone)] pub struct Pair { - signer: EthersAwsSigner<'static>, + _signer: EthersAwsSigner<'static>, pubkey: Public, } @@ -42,11 +45,14 @@ impl Pair { .await .map_err(|_| AwsPairError::DummyError())?; let pubkey = pubkey.try_into().map_err(|_| AwsPairError::DummyError())?; - Ok(Self { signer, pubkey }) + Ok(Self { + _signer: signer, + pubkey, + }) } fn public_remote(&self) -> Public { - self.pubkey.clone() + self.pubkey } // TODO: Since Pair::sign is infallible, we will have to have a retry count From ef9115a1a9e7e2af7079e2f87090f399eb624891 Mon Sep 17 00:00:00 2001 From: Matthew Smith Date: Thu, 20 Oct 2022 19:40:56 +0700 Subject: [PATCH 10/24] Add sync retrying remote signer with exponential backoff --- chains/nomad-substrate/src/aws.rs | 65 +++++++++++++++++++++++++++---- 1 file changed, 58 insertions(+), 7 deletions(-) diff --git a/chains/nomad-substrate/src/aws.rs b/chains/nomad-substrate/src/aws.rs index 60b01c8d..c4f90f21 100644 --- a/chains/nomad-substrate/src/aws.rs +++ b/chains/nomad-substrate/src/aws.rs @@ -1,8 +1,9 @@ use async_trait::async_trait; use color_eyre::Result; -use ethers_core::k256::ecdsa::VerifyingKey as EthersVerifyingKey; +use ethers_core::k256::ecdsa::{Signature as EthersSignature, VerifyingKey as EthersVerifyingKey}; use ethers_signers::AwsSigner as EthersAwsSigner; use nomad_core::aws::get_kms_client; +use std::time::Duration; use subxt::error::SecretStringError; use subxt::ext::{ sp_core::{ @@ -11,6 +12,7 @@ use subxt::ext::{ }, sp_runtime::{CryptoType, MultiSignature, MultiSigner}, }; +use tokio::time::sleep; // TODO: Rename things @@ -26,8 +28,10 @@ pub enum AwsPairError { /// will support a remote AWS signer using ECDSA #[derive(Clone)] pub struct Pair { - _signer: EthersAwsSigner<'static>, + signer: EthersAwsSigner<'static>, pubkey: Public, + max_retries: u32, + min_retry_ms: u64, } impl Pair { @@ -45,20 +49,61 @@ impl Pair { .await .map_err(|_| AwsPairError::DummyError())?; let pubkey = pubkey.try_into().map_err(|_| AwsPairError::DummyError())?; + let max_retries = 5; + let min_retry_ms = 1000; Ok(Self { - _signer: signer, + signer, pubkey, + max_retries, + min_retry_ms, }) } + /// Our `Public` key fn public_remote(&self) -> Public { self.pubkey } - // TODO: Since Pair::sign is infallible, we will have to have a retry count - // TODO: followed by a panic here if we can't remote sign - fn sign_remote(&self, _message: &[u8]) -> Signature { - todo!() + /// Try to sign `message` using our remote signer. Since we can't recover + /// from an error here, we'll discard it in favor of an `Option` + async fn try_sign_remote(&self, _message: &[u8], delay: Duration) -> Option { + sleep(delay).await; + let dummy = [0u8; 32]; // TODO: + self.signer + .sign_digest(dummy) + .await + .ok() + .map(Into::::into) + } + + /// Try to sign `message` `max_retries` times with an exponential backoff between tries. + /// If we hit `max_retries` `panic` since we're unable to return an error here. + fn sign_remote(&self, message: &[u8]) -> Signature { + let mut times_attempted = 0; + let mut delay = Duration::from_millis(0); + tokio::runtime::Builder::new_current_thread() + .enable_all() + .build() + .expect("unable to create tokio::runtime (this should never happen)") + .block_on(async { + loop { + match self.try_sign_remote(message, delay).await { + Some(signature) => return signature, + None => { + delay = Duration::from_millis(self.min_retry_ms.pow({ + times_attempted += 1; + times_attempted + })) + } + } + if times_attempted == self.max_retries { + panic!( + "giving up after attempting to sign message {} times", + times_attempted + ) + } + } + }) } } @@ -173,6 +218,12 @@ impl From for MultiSigner { #[derive(PartialEq, Eq, Hash)] pub struct Signature(pub [u8; 65]); +impl From for Signature { + fn from(_ethers_signature: EthersSignature) -> Self { + todo!() + } +} + impl From for MultiSignature { fn from(_x: Signature) -> Self { todo!() From 048559300ba678cc82232b8ca0b0468744d6db9c Mon Sep 17 00:00:00 2001 From: Matthew Smith Date: Thu, 20 Oct 2022 19:43:29 +0700 Subject: [PATCH 11/24] Misc cleanup --- chains/nomad-substrate/src/aws.rs | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/chains/nomad-substrate/src/aws.rs b/chains/nomad-substrate/src/aws.rs index c4f90f21..a5063ed2 100644 --- a/chains/nomad-substrate/src/aws.rs +++ b/chains/nomad-substrate/src/aws.rs @@ -4,13 +4,15 @@ use ethers_core::k256::ecdsa::{Signature as EthersSignature, VerifyingKey as Eth use ethers_signers::AwsSigner as EthersAwsSigner; use nomad_core::aws::get_kms_client; use std::time::Duration; -use subxt::error::SecretStringError; -use subxt::ext::{ - sp_core::{ - crypto::{CryptoTypePublicPair, Derive, UncheckedFrom}, - ecdsa, ByteArray, DeriveJunction, Pair as TraitPair, Public as TraitPublic, +use subxt::{ + error::SecretStringError, + ext::{ + sp_core::{ + crypto::{CryptoTypePublicPair, Derive, UncheckedFrom}, + ecdsa, ByteArray, DeriveJunction, Pair as TraitPair, Public as TraitPublic, + }, + sp_runtime::{CryptoType, MultiSignature, MultiSigner}, }, - sp_runtime::{CryptoType, MultiSignature, MultiSigner}, }; use tokio::time::sleep; From a7138f8ed442a5d00a0cfabdfeee4e755b873004 Mon Sep 17 00:00:00 2001 From: Matthew Smith Date: Thu, 20 Oct 2022 19:54:02 +0700 Subject: [PATCH 12/24] Signer should include last error message when it panics --- chains/nomad-substrate/src/aws.rs | 36 +++++++++++++++---------------- 1 file changed, 18 insertions(+), 18 deletions(-) diff --git a/chains/nomad-substrate/src/aws.rs b/chains/nomad-substrate/src/aws.rs index a5063ed2..f1c7ce66 100644 --- a/chains/nomad-substrate/src/aws.rs +++ b/chains/nomad-substrate/src/aws.rs @@ -1,7 +1,7 @@ use async_trait::async_trait; use color_eyre::Result; use ethers_core::k256::ecdsa::{Signature as EthersSignature, VerifyingKey as EthersVerifyingKey}; -use ethers_signers::AwsSigner as EthersAwsSigner; +use ethers_signers::{AwsSigner as EthersAwsSigner, AwsSignerError}; use nomad_core::aws::get_kms_client; use std::time::Duration; use subxt::{ @@ -66,19 +66,21 @@ impl Pair { self.pubkey } - /// Try to sign `message` using our remote signer. Since we can't recover - /// from an error here, we'll discard it in favor of an `Option` - async fn try_sign_remote(&self, _message: &[u8], delay: Duration) -> Option { + /// Try to sign `message` using our remote signer + async fn try_sign_remote( + &self, + _message: &[u8], + delay: Duration, + ) -> Result { sleep(delay).await; let dummy = [0u8; 32]; // TODO: self.signer .sign_digest(dummy) .await - .ok() .map(Into::::into) } - /// Try to sign `message` `max_retries` times with an exponential backoff between tries. + /// Try to sign `message` `max_retries` times with an exponential backoff between attempts. /// If we hit `max_retries` `panic` since we're unable to return an error here. fn sign_remote(&self, message: &[u8]) -> Signature { let mut times_attempted = 0; @@ -90,20 +92,18 @@ impl Pair { .block_on(async { loop { match self.try_sign_remote(message, delay).await { - Some(signature) => return signature, - None => { - delay = Duration::from_millis(self.min_retry_ms.pow({ - times_attempted += 1; - times_attempted - })) + Ok(signature) => return signature, + Err(error) => { + times_attempted += 1; + delay = Duration::from_millis(self.min_retry_ms.pow(times_attempted)); + if times_attempted == self.max_retries { + panic!( + "giving up after attempting to sign message {} times: {:?}", + times_attempted, error, + ) + } } } - if times_attempted == self.max_retries { - panic!( - "giving up after attempting to sign message {} times", - times_attempted - ) - } } }) } From 8e4e53cb6ba49d23345e924f698db211ad0a8976 Mon Sep 17 00:00:00 2001 From: Matthew Smith Date: Fri, 21 Oct 2022 23:30:26 +0700 Subject: [PATCH 13/24] Implement message signing --- chains/nomad-substrate/src/aws.rs | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/chains/nomad-substrate/src/aws.rs b/chains/nomad-substrate/src/aws.rs index f1c7ce66..5e1f9598 100644 --- a/chains/nomad-substrate/src/aws.rs +++ b/chains/nomad-substrate/src/aws.rs @@ -1,7 +1,9 @@ use async_trait::async_trait; use color_eyre::Result; -use ethers_core::k256::ecdsa::{Signature as EthersSignature, VerifyingKey as EthersVerifyingKey}; -use ethers_signers::{AwsSigner as EthersAwsSigner, AwsSignerError}; +use ethers_core::{ + k256::ecdsa::VerifyingKey as EthersVerifyingKey, types::Signature as EthersSignature, +}; +use ethers_signers::{AwsSigner as EthersAwsSigner, AwsSignerError, Signer}; use nomad_core::aws::get_kms_client; use std::time::Duration; use subxt::{ @@ -69,13 +71,12 @@ impl Pair { /// Try to sign `message` using our remote signer async fn try_sign_remote( &self, - _message: &[u8], + message: &[u8], delay: Duration, ) -> Result { sleep(delay).await; - let dummy = [0u8; 32]; // TODO: self.signer - .sign_digest(dummy) + .sign_message(message) .await .map(Into::::into) } @@ -221,8 +222,8 @@ impl From for MultiSigner { pub struct Signature(pub [u8; 65]); impl From for Signature { - fn from(_ethers_signature: EthersSignature) -> Self { - todo!() + fn from(signature: EthersSignature) -> Self { + Signature(signature.into()) } } From c7d43e872b727755836d1279e6e11aeddc6552b4 Mon Sep 17 00:00:00 2001 From: Matthew Smith Date: Sat, 22 Oct 2022 01:32:56 +0700 Subject: [PATCH 14/24] Implement supertraits --- chains/nomad-substrate/src/aws.rs | 18 ++++++------------ 1 file changed, 6 insertions(+), 12 deletions(-) diff --git a/chains/nomad-substrate/src/aws.rs b/chains/nomad-substrate/src/aws.rs index 5e1f9598..6aa686b8 100644 --- a/chains/nomad-substrate/src/aws.rs +++ b/chains/nomad-substrate/src/aws.rs @@ -13,7 +13,7 @@ use subxt::{ crypto::{CryptoTypePublicPair, Derive, UncheckedFrom}, ecdsa, ByteArray, DeriveJunction, Pair as TraitPair, Public as TraitPublic, }, - sp_runtime::{CryptoType, MultiSignature, MultiSigner}, + sp_runtime::{CryptoType, MultiSignature}, }, }; use tokio::time::sleep; @@ -165,20 +165,20 @@ impl ByteArray for Public { } impl std::fmt::Display for Public { - fn fmt(&self, _f: &mut std::fmt::Formatter) -> std::fmt::Result { - todo!() + fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result { + write!(f, "{}", hex::encode(self.as_ref())) } } impl AsRef<[u8]> for Public { fn as_ref(&self) -> &[u8] { - todo!() + &self.0[..] } } impl AsMut<[u8]> for Public { fn as_mut(&mut self) -> &mut [u8] { - todo!() + &mut self.0[..] } } @@ -206,13 +206,7 @@ impl TryFrom<&[u8]> for Public { impl TraitPublic for Public { fn to_public_crypto_pair(&self) -> CryptoTypePublicPair { - todo!() - } -} - -impl From for MultiSigner { - fn from(_x: Public) -> Self { - todo!() + CryptoTypePublicPair(ecdsa::CRYPTO_ID, self.as_ref().to_vec()) } } From 98501dd091cd87eadec4cd085d0210cd839ca238 Mon Sep 17 00:00:00 2001 From: Matthew Smith Date: Mon, 24 Oct 2022 21:52:22 +0700 Subject: [PATCH 15/24] Bring naming in line with project conventions --- chains/nomad-substrate/src/aws.rs | 84 +++++++++++++++---------------- 1 file changed, 41 insertions(+), 43 deletions(-) diff --git a/chains/nomad-substrate/src/aws.rs b/chains/nomad-substrate/src/aws.rs index 6aa686b8..f382cc2b 100644 --- a/chains/nomad-substrate/src/aws.rs +++ b/chains/nomad-substrate/src/aws.rs @@ -18,9 +18,7 @@ use subxt::{ }; use tokio::time::sleep; -// TODO: Rename things - -/// Error types for AWS `Pair` +/// Error types for AwsPair` #[derive(Debug, thiserror::Error)] pub enum AwsPairError { /// Dummy error @@ -28,18 +26,18 @@ pub enum AwsPairError { DummyError(), } -/// A partially implemented Pair (`subxt::ext::sp_core::Pair`) that +/// A partially implemented `subxt::ext::sp_core::Pair` that /// will support a remote AWS signer using ECDSA #[derive(Clone)] -pub struct Pair { +pub struct AwsPair { signer: EthersAwsSigner<'static>, - pubkey: Public, + pubkey: AwsPublic, max_retries: u32, min_retry_ms: u64, } -impl Pair { - /// Create a new AWS Pair from an AWS id +impl AwsPair { + /// Create a new `AwsPair` from an AWS id pub async fn new(id: T) -> Result where T: AsRef + Send + Sync, @@ -64,7 +62,7 @@ impl Pair { } /// Our `Public` key - fn public_remote(&self) -> Public { + fn public_remote(&self) -> AwsPublic { self.pubkey } @@ -73,17 +71,17 @@ impl Pair { &self, message: &[u8], delay: Duration, - ) -> Result { + ) -> Result { sleep(delay).await; self.signer .sign_message(message) .await - .map(Into::::into) + .map(Into::::into) } /// Try to sign `message` `max_retries` times with an exponential backoff between attempts. /// If we hit `max_retries` `panic` since we're unable to return an error here. - fn sign_remote(&self, message: &[u8]) -> Signature { + fn sign_remote(&self, message: &[u8]) -> AwsSignature { let mut times_attempted = 0; let mut delay = Duration::from_millis(0); tokio::runtime::Builder::new_current_thread() @@ -110,7 +108,7 @@ impl Pair { } } -/// To make `Pair` init from an AWS id while keeping our internal signer +/// To make `AwsPair` init from an AWS id while keeping our internal signer /// generic over all `subxt::ext::sp_core::Pair` and `subxt::Config`. /// This will be implemented as a noop for `subxt::ext::sp_core::ecdsa::Pair` /// and other core implementations @@ -124,12 +122,12 @@ pub trait FromAwsId { } #[async_trait] -impl FromAwsId for Pair { +impl FromAwsId for AwsPair { async fn from_aws_id(id: T) -> Result where T: AsRef + Send + Sync, { - Pair::new(id).await + AwsPair::new(id).await } } @@ -143,46 +141,46 @@ impl FromAwsId for ecdsa::Pair { } } -/// A `Public` key that is compatible with `subxt::ext::sp_core::Pair` -/// and AWS's ECDSA KMS signer +/// A `subxt::ext::sp_core::Public` key that is compatible with +/// `subxt::ext::sp_core::Pair` and AWS's ECDSA KMS signer #[derive(Clone, Copy, Eq, PartialEq, PartialOrd, Ord, Hash)] -pub struct Public(pub [u8; 33]); +pub struct AwsPublic(pub [u8; 33]); -impl UncheckedFrom<[u8; 33]> for Public { +impl UncheckedFrom<[u8; 33]> for AwsPublic { fn unchecked_from(x: [u8; 33]) -> Self { - Public(x) + AwsPublic(x) } } -impl Derive for Public {} +impl Derive for AwsPublic {} -impl CryptoType for Public { - type Pair = Pair; +impl CryptoType for AwsPublic { + type Pair = AwsPair; } -impl ByteArray for Public { +impl ByteArray for AwsPublic { const LEN: usize = 33; } -impl std::fmt::Display for Public { +impl std::fmt::Display for AwsPublic { fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result { write!(f, "{}", hex::encode(self.as_ref())) } } -impl AsRef<[u8]> for Public { +impl AsRef<[u8]> for AwsPublic { fn as_ref(&self) -> &[u8] { &self.0[..] } } -impl AsMut<[u8]> for Public { +impl AsMut<[u8]> for AwsPublic { fn as_mut(&mut self) -> &mut [u8] { &mut self.0[..] } } -impl TryFrom for Public { +impl TryFrom for AwsPublic { type Error = (); fn try_from(data: EthersVerifyingKey) -> Result { @@ -191,7 +189,7 @@ impl TryFrom for Public { } } -impl TryFrom<&[u8]> for Public { +impl TryFrom<&[u8]> for AwsPublic { type Error = (); fn try_from(data: &[u8]) -> Result { @@ -204,7 +202,7 @@ impl TryFrom<&[u8]> for Public { } } -impl TraitPublic for Public { +impl TraitPublic for AwsPublic { fn to_public_crypto_pair(&self) -> CryptoTypePublicPair { CryptoTypePublicPair(ecdsa::CRYPTO_ID, self.as_ref().to_vec()) } @@ -213,21 +211,21 @@ impl TraitPublic for Public { /// A `Signature` that is compatible with `subxt::ext::sp_core::Pair` /// and AWS's ECDSA KMS signer #[derive(PartialEq, Eq, Hash)] -pub struct Signature(pub [u8; 65]); +pub struct AwsSignature(pub [u8; 65]); -impl From for Signature { +impl From for AwsSignature { fn from(signature: EthersSignature) -> Self { - Signature(signature.into()) + AwsSignature(signature.into()) } } -impl From for MultiSignature { - fn from(_x: Signature) -> Self { +impl From for MultiSignature { + fn from(_x: AwsSignature) -> Self { todo!() } } -impl AsRef<[u8]> for Signature { +impl AsRef<[u8]> for AwsSignature { fn as_ref(&self) -> &[u8] { &self.0[..] } @@ -238,13 +236,13 @@ impl AsRef<[u8]> for Signature { type DummySeed = [u8; 0]; /// The trait `subxt::ext::sp_core::Pair` handles signing, verification and the creation -/// of keypairs form local key material (mnemonics, random bytes, etc.). With a remote -/// AWS signer keypair creation is handled externally so we will only partially implement +/// of keypairs from local key material (mnemonics, random bytes, etc.). With a remote +/// AWS signer, keypair creation is handled externally so we will only partially implement /// `Pair` to reflect this. -impl TraitPair for Pair { - type Public = Public; +impl TraitPair for AwsPair { + type Public = AwsPublic; type Seed = DummySeed; - type Signature = Signature; + type Signature = AwsSignature; type DeriveError = (); /// Our `Public` key @@ -303,6 +301,6 @@ impl TraitPair for Pair { } } -impl CryptoType for Pair { - type Pair = Pair; +impl CryptoType for AwsPair { + type Pair = AwsPair; } From 5ca5afaf5e7464bc8acf8f8d90d17d93be69470f Mon Sep 17 00:00:00 2001 From: Matthew Smith Date: Mon, 24 Oct 2022 22:21:10 +0700 Subject: [PATCH 16/24] Implement error handling --- chains/nomad-substrate/src/aws.rs | 21 +++++++++++++-------- 1 file changed, 13 insertions(+), 8 deletions(-) diff --git a/chains/nomad-substrate/src/aws.rs b/chains/nomad-substrate/src/aws.rs index f382cc2b..6f6f5298 100644 --- a/chains/nomad-substrate/src/aws.rs +++ b/chains/nomad-substrate/src/aws.rs @@ -18,12 +18,15 @@ use subxt::{ }; use tokio::time::sleep; -/// Error types for AwsPair` +/// Error types for `AwsPair` #[derive(Debug, thiserror::Error)] pub enum AwsPairError { - /// Dummy error - #[error("Dummy error")] - DummyError(), + /// AWS Signer Error + #[error("Error from EthersAwsSigner: {0}")] + AwsSignerError(#[from] AwsSignerError), + /// Public key length error + #[error("EthersAwsSigner returned a bad public key length")] + PubKeyBadLength, } /// A partially implemented `subxt::ext::sp_core::Pair` that @@ -45,12 +48,14 @@ impl AwsPair { let kms_client = get_kms_client().await; let signer = EthersAwsSigner::new(kms_client, id, 0) .await - .map_err(|_| AwsPairError::DummyError())?; + .map_err(AwsPairError::AwsSignerError)?; let pubkey = signer .get_pubkey() .await - .map_err(|_| AwsPairError::DummyError())?; - let pubkey = pubkey.try_into().map_err(|_| AwsPairError::DummyError())?; + .map_err(AwsPairError::AwsSignerError)?; + let pubkey = pubkey + .try_into() + .map_err(|_| AwsPairError::PubKeyBadLength)?; let max_retries = 5; let min_retry_ms = 1000; Ok(Self { @@ -61,7 +66,7 @@ impl AwsPair { }) } - /// Our `Public` key + /// Our `AwsPublic` key fn public_remote(&self) -> AwsPublic { self.pubkey } From c3caee79be8e422eabf87dbac8c8743412a18483 Mon Sep 17 00:00:00 2001 From: Matthew Smith Date: Mon, 24 Oct 2022 22:22:55 +0700 Subject: [PATCH 17/24] Move signer settings to constants --- chains/nomad-substrate/src/aws.rs | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/chains/nomad-substrate/src/aws.rs b/chains/nomad-substrate/src/aws.rs index 6f6f5298..b14852b9 100644 --- a/chains/nomad-substrate/src/aws.rs +++ b/chains/nomad-substrate/src/aws.rs @@ -18,6 +18,9 @@ use subxt::{ }; use tokio::time::sleep; +const AWS_SIGNER_MAX_RETRIES: u32 = 5; +const AWS_SINGER_MIN_RETRY_DELAY_MS: u64 = 1000; + /// Error types for `AwsPair` #[derive(Debug, thiserror::Error)] pub enum AwsPairError { @@ -56,13 +59,11 @@ impl AwsPair { let pubkey = pubkey .try_into() .map_err(|_| AwsPairError::PubKeyBadLength)?; - let max_retries = 5; - let min_retry_ms = 1000; Ok(Self { signer, pubkey, - max_retries, - min_retry_ms, + max_retries: AWS_SIGNER_MAX_RETRIES, + min_retry_ms: AWS_SINGER_MIN_RETRY_DELAY_MS, }) } From 79f5ea1c789187846828a5c8be0c7fe940f96f7a Mon Sep 17 00:00:00 2001 From: Matthew Smith Date: Mon, 24 Oct 2022 22:30:44 +0700 Subject: [PATCH 18/24] Improve docs --- chains/nomad-substrate/src/aws.rs | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/chains/nomad-substrate/src/aws.rs b/chains/nomad-substrate/src/aws.rs index b14852b9..56b80817 100644 --- a/chains/nomad-substrate/src/aws.rs +++ b/chains/nomad-substrate/src/aws.rs @@ -24,7 +24,7 @@ const AWS_SINGER_MIN_RETRY_DELAY_MS: u64 = 1000; /// Error types for `AwsPair` #[derive(Debug, thiserror::Error)] pub enum AwsPairError { - /// AWS Signer Error + /// AWS signer error #[error("Error from EthersAwsSigner: {0}")] AwsSignerError(#[from] AwsSignerError), /// Public key length error @@ -48,14 +48,19 @@ impl AwsPair { where T: AsRef + Send + Sync, { + // Shared AWS client let kms_client = get_kms_client().await; + // Init our remote signer let signer = EthersAwsSigner::new(kms_client, id, 0) .await .map_err(AwsPairError::AwsSignerError)?; + // Get the pubkey from our remote keypair let pubkey = signer .get_pubkey() .await .map_err(AwsPairError::AwsSignerError)?; + // Map our AWS pubkey to our Substrate-compatible one + // These are both 33-byte ECDSA Secp256k1 compressed points let pubkey = pubkey .try_into() .map_err(|_| AwsPairError::PubKeyBadLength)?; @@ -72,13 +77,15 @@ impl AwsPair { self.pubkey } - /// Try to sign `message` using our remote signer + /// Try to sign `message` using our remote signer. Accept a `delay` so that + /// this can be called repeatedly with a backoff async fn try_sign_remote( &self, message: &[u8], delay: Duration, ) -> Result { sleep(delay).await; + // Sign and map between our remote and local 65-byte ECDSA sigs self.signer .sign_message(message) .await From 1702d656ba4c87995eee414bbe2c6036e58ae8b0 Mon Sep 17 00:00:00 2001 From: Matthew Smith Date: Mon, 24 Oct 2022 23:04:08 +0700 Subject: [PATCH 19/24] Refactor sign_remote --- chains/nomad-substrate/src/aws.rs | 50 ++++++++++++------------------- 1 file changed, 19 insertions(+), 31 deletions(-) diff --git a/chains/nomad-substrate/src/aws.rs b/chains/nomad-substrate/src/aws.rs index 56b80817..aaa49a89 100644 --- a/chains/nomad-substrate/src/aws.rs +++ b/chains/nomad-substrate/src/aws.rs @@ -77,46 +77,34 @@ impl AwsPair { self.pubkey } - /// Try to sign `message` using our remote signer. Accept a `delay` so that - /// this can be called repeatedly with a backoff - async fn try_sign_remote( - &self, - message: &[u8], - delay: Duration, - ) -> Result { - sleep(delay).await; - // Sign and map between our remote and local 65-byte ECDSA sigs - self.signer - .sign_message(message) - .await - .map(Into::::into) - } - /// Try to sign `message` `max_retries` times with an exponential backoff between attempts. /// If we hit `max_retries` `panic` since we're unable to return an error here. fn sign_remote(&self, message: &[u8]) -> AwsSignature { - let mut times_attempted = 0; - let mut delay = Duration::from_millis(0); tokio::runtime::Builder::new_current_thread() .enable_all() .build() .expect("unable to create tokio::runtime (this should never happen)") .block_on(async { - loop { - match self.try_sign_remote(message, delay).await { - Ok(signature) => return signature, - Err(error) => { - times_attempted += 1; - delay = Duration::from_millis(self.min_retry_ms.pow(times_attempted)); - if times_attempted == self.max_retries { - panic!( - "giving up after attempting to sign message {} times: {:?}", - times_attempted, error, - ) - } - } - } + let mut error = None; + for i in 0..self.max_retries { + sleep(Duration::from_millis(self.min_retry_ms.pow(i))).await; + error = Some( + match self + .signer + .sign_message(message) + .await + .map(Into::::into) + { + Ok(signature) => return signature, + Err(error) => error, + }, + ); } + panic!( + "giving up after attempting to sign message {} times: {:?}", + self.max_retries, + error.unwrap(), + ); }) } } From 871a0de1bacfc0945584be2945877866c66db8e5 Mon Sep 17 00:00:00 2001 From: Matthew Smith Date: Mon, 24 Oct 2022 23:08:31 +0700 Subject: [PATCH 20/24] Prefer signing over singing --- chains/nomad-substrate/src/aws.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/chains/nomad-substrate/src/aws.rs b/chains/nomad-substrate/src/aws.rs index aaa49a89..43490797 100644 --- a/chains/nomad-substrate/src/aws.rs +++ b/chains/nomad-substrate/src/aws.rs @@ -19,7 +19,7 @@ use subxt::{ use tokio::time::sleep; const AWS_SIGNER_MAX_RETRIES: u32 = 5; -const AWS_SINGER_MIN_RETRY_DELAY_MS: u64 = 1000; +const AWS_SIGNER_MIN_RETRY_DELAY_MS: u64 = 1000; /// Error types for `AwsPair` #[derive(Debug, thiserror::Error)] @@ -68,7 +68,7 @@ impl AwsPair { signer, pubkey, max_retries: AWS_SIGNER_MAX_RETRIES, - min_retry_ms: AWS_SINGER_MIN_RETRY_DELAY_MS, + min_retry_ms: AWS_SIGNER_MIN_RETRY_DELAY_MS, }) } From 601d97873641df692002447d513135fd64584f59 Mon Sep 17 00:00:00 2001 From: Matthew Smith Date: Mon, 24 Oct 2022 23:41:49 +0700 Subject: [PATCH 21/24] Implement missing supertrait --- chains/nomad-substrate/src/aws.rs | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/chains/nomad-substrate/src/aws.rs b/chains/nomad-substrate/src/aws.rs index 43490797..b6d41667 100644 --- a/chains/nomad-substrate/src/aws.rs +++ b/chains/nomad-substrate/src/aws.rs @@ -16,6 +16,7 @@ use subxt::{ sp_runtime::{CryptoType, MultiSignature}, }, }; +use subxt::ext::sp_runtime::MultiSigner; use tokio::time::sleep; const AWS_SIGNER_MAX_RETRIES: u32 = 5; @@ -209,6 +210,12 @@ impl TraitPublic for AwsPublic { } } +impl From for MultiSigner { + fn from(pubkey: AwsPublic) -> Self { + Self::Ecdsa(ecdsa::Public::from_raw(pubkey.0)) + } +} + /// A `Signature` that is compatible with `subxt::ext::sp_core::Pair` /// and AWS's ECDSA KMS signer #[derive(PartialEq, Eq, Hash)] From 36110f8a91e827dd1a75df697436b0d10721d9a6 Mon Sep 17 00:00:00 2001 From: Matthew Smith Date: Thu, 27 Oct 2022 03:18:39 +0700 Subject: [PATCH 22/24] Update test deps --- Cargo.lock | 28 ++++++++++++++++++++++++++++ chains/nomad-substrate/Cargo.toml | 4 ++++ 2 files changed, 32 insertions(+) diff --git a/Cargo.lock b/Cargo.lock index 48d65d79..3d277b17 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -733,6 +733,7 @@ dependencies = [ "num-integer", "num-traits 0.2.15", "serde 1.0.137", + "time", "winapi", ] @@ -3331,6 +3332,7 @@ dependencies = [ "affix", "async-std", "async-trait", + "base64 0.13.0", "color-eyre", "env_logger", "ethers-core", @@ -3346,6 +3348,7 @@ dependencies = [ "primitive-types 0.11.1 (git+https://github.com/paritytech/parity-common.git?branch=master)", "rusoto_core", "rusoto_kms", + "rusoto_mock", "scale-info", "serde 1.0.137", "serde_json", @@ -4587,6 +4590,21 @@ dependencies = [ "serde_json", ] +[[package]] +name = "rusoto_mock" +version = "0.48.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "5a384880f3c6d514e9499e6df75490bef5f6f39237bc24844e3933dfc09e9e55" +dependencies = [ + "async-trait", + "chrono", + "futures", + "http", + "rusoto_core", + "serde 1.0.137", + "serde_json", +] + [[package]] name = "rusoto_s3" version = "0.48.0" @@ -5931,6 +5949,16 @@ dependencies = [ "once_cell", ] +[[package]] +name = "time" +version = "0.1.43" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "ca8a50ef2360fbd1eeb0ecd46795a87a19024eb4b53c5dc916ca1fd95fe62438" +dependencies = [ + "libc", + "winapi", +] + [[package]] name = "tiny-bip39" version = "0.8.2" diff --git a/chains/nomad-substrate/Cargo.toml b/chains/nomad-substrate/Cargo.toml index 5df02129..b3fc7105 100644 --- a/chains/nomad-substrate/Cargo.toml +++ b/chains/nomad-substrate/Cargo.toml @@ -37,3 +37,7 @@ nomad-types = { path = "../../nomad-types" } ethers-core = { git = "https://github.com/gakonst/ethers-rs", branch = "master" } ethers-signers = { git = "https://github.com/gakonst/ethers-rs", branch = "master", features = ["aws"] } + +[dev-dependencies] +rusoto_mock = "0.48.0" +base64 = "0.13.0" \ No newline at end of file From 61e7db90ecc4a118d938bf4d1836b5e518cff365 Mon Sep 17 00:00:00 2001 From: Matthew Smith Date: Thu, 27 Oct 2022 03:19:16 +0700 Subject: [PATCH 23/24] Add AwsPairError --- chains/nomad-substrate/src/signer.rs | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/chains/nomad-substrate/src/signer.rs b/chains/nomad-substrate/src/signer.rs index fa7b70ed..afac607c 100644 --- a/chains/nomad-substrate/src/signer.rs +++ b/chains/nomad-substrate/src/signer.rs @@ -1,4 +1,4 @@ -use crate::aws::FromAwsId; +use crate::aws::{AwsPairError, FromAwsId}; use async_trait::async_trait; use color_eyre::{eyre::bail, Result}; use nomad_core::FromSignerConf; @@ -14,6 +14,9 @@ use subxt::{ /// Error types for SubstrateSigners #[derive(Debug, thiserror::Error)] pub enum SubstrateSignersError { + /// AWS signer configuration error + #[error("Failed to configure AWS signer: {0}")] + AwsPairConfiguration(#[from] AwsPairError), /// Local signer configuration error #[error("Failed to configure local signer from secret: {0:?}")] LocalSignerConfiguration(SecretStringError), @@ -59,7 +62,9 @@ where Ok(Self::Local(pair_signer)) } SignerConf::Aws { id } => { - let pair = P::from_aws_id(id).await?; + let pair = P::from_aws_id(id) + .await + .map_err(SubstrateSignersError::AwsPairConfiguration)?; let pair_signer = PairSigner::::new(pair); let account_id = pair_signer.account_id(); tracing::info!("Tx signer AccountId: {}", account_id); From 9bce6950c87437d1b8980bc2ac3f3a2547e62800 Mon Sep 17 00:00:00 2001 From: Matthew Smith Date: Thu, 27 Oct 2022 03:19:55 +0700 Subject: [PATCH 24/24] Fix async issues and add sign test --- chains/nomad-substrate/src/aws.rs | 137 ++++++++++++++++++++++++------ 1 file changed, 113 insertions(+), 24 deletions(-) diff --git a/chains/nomad-substrate/src/aws.rs b/chains/nomad-substrate/src/aws.rs index b6d41667..e93b7fea 100644 --- a/chains/nomad-substrate/src/aws.rs +++ b/chains/nomad-substrate/src/aws.rs @@ -5,7 +5,9 @@ use ethers_core::{ }; use ethers_signers::{AwsSigner as EthersAwsSigner, AwsSignerError, Signer}; use nomad_core::aws::get_kms_client; -use std::time::Duration; +use rusoto_kms::KmsClient; +use std::{thread, time::Duration}; +use subxt::ext::sp_runtime::MultiSigner; use subxt::{ error::SecretStringError, ext::{ @@ -16,11 +18,9 @@ use subxt::{ sp_runtime::{CryptoType, MultiSignature}, }, }; -use subxt::ext::sp_runtime::MultiSigner; -use tokio::time::sleep; +use tokio::{runtime, time::sleep}; const AWS_SIGNER_MAX_RETRIES: u32 = 5; -const AWS_SIGNER_MIN_RETRY_DELAY_MS: u64 = 1000; /// Error types for `AwsPair` #[derive(Debug, thiserror::Error)] @@ -40,17 +40,27 @@ pub struct AwsPair { signer: EthersAwsSigner<'static>, pubkey: AwsPublic, max_retries: u32, - min_retry_ms: u64, } impl AwsPair { /// Create a new `AwsPair` from an AWS id - pub async fn new(id: T) -> Result + pub async fn new(id: T) -> Result where T: AsRef + Send + Sync, { // Shared AWS client let kms_client = get_kms_client().await; + Self::new_with_client(id, kms_client).await + } + + /// Create a new `AwsPair` from a `rusoto_kms::KmsClient` and an AWS id + pub async fn new_with_client( + id: T, + kms_client: &'static KmsClient, + ) -> Result + where + T: AsRef + Send + Sync, + { // Init our remote signer let signer = EthersAwsSigner::new(kms_client, id, 0) .await @@ -69,7 +79,6 @@ impl AwsPair { signer, pubkey, max_retries: AWS_SIGNER_MAX_RETRIES, - min_retry_ms: AWS_SIGNER_MIN_RETRY_DELAY_MS, }) } @@ -80,19 +89,32 @@ impl AwsPair { /// Try to sign `message` `max_retries` times with an exponential backoff between attempts. /// If we hit `max_retries` `panic` since we're unable to return an error here. - fn sign_remote(&self, message: &[u8]) -> AwsSignature { - tokio::runtime::Builder::new_current_thread() - .enable_all() - .build() - .expect("unable to create tokio::runtime (this should never happen)") - .block_on(async { + fn sign_remote_sync(&self, message: &[u8]) -> AwsSignature { + let message = message.to_owned(); + let Self { + signer, + max_retries, + .. + } = self.clone(); + // We may be running this inside an async func, so we want to grab the current + // runtime instead of spawning a new one. + let handle = match runtime::Handle::try_current() { + Ok(handle) => handle, + Err(_) => runtime::Builder::new_current_thread() + .enable_all() + .build() + .expect("unable to create new tokio::runtime (this should never happen)") + .handle() + .clone(), + }; + // We're spawning a new thread here to accommodate `tokio::runtime::Handle::block_on` + thread::spawn(move || { + handle.block_on(async { let mut error = None; - for i in 0..self.max_retries { - sleep(Duration::from_millis(self.min_retry_ms.pow(i))).await; + for i in 0..max_retries { error = Some( - match self - .signer - .sign_message(message) + match signer + .sign_message(&message) .await .map(Into::::into) { @@ -100,13 +122,16 @@ impl AwsPair { Err(error) => error, }, ); + sleep(Duration::from_secs(2u64.pow(i))).await; } panic!( "giving up after attempting to sign message {} times: {:?}", - self.max_retries, - error.unwrap(), + max_retries, error, ); }) + }) + .join() + .unwrap() // Let our panic bubble up } } @@ -117,7 +142,7 @@ impl AwsPair { #[async_trait] pub trait FromAwsId { /// Create an AWS-compatible signer from an AWS id - async fn from_aws_id(id: T) -> Result + async fn from_aws_id(id: T) -> Result where T: AsRef + Send + Sync, Self: Sized; @@ -125,7 +150,7 @@ pub trait FromAwsId { #[async_trait] impl FromAwsId for AwsPair { - async fn from_aws_id(id: T) -> Result + async fn from_aws_id(id: T) -> Result where T: AsRef + Send + Sync, { @@ -135,7 +160,7 @@ impl FromAwsId for AwsPair { #[async_trait] impl FromAwsId for ecdsa::Pair { - async fn from_aws_id>(_id: T) -> Result + async fn from_aws_id>(_id: T) -> Result where T: AsRef + Send + Sync, { @@ -260,7 +285,7 @@ impl TraitPair for AwsPair { /// Sign a message of arbitrary bytes to return a `Signature` fn sign(&self, message: &[u8]) -> Self::Signature { - self.sign_remote(message) + self.sign_remote_sync(message) } fn verify>(_sig: &Self::Signature, _message: M, _pubkey: &Self::Public) -> bool { @@ -312,3 +337,67 @@ impl TraitPair for AwsPair { impl CryptoType for AwsPair { type Pair = AwsPair; } + +#[cfg(test)] +mod test { + use super::*; + use once_cell::sync::OnceCell; + use rusoto_mock::{ + MockCredentialsProvider, MockRequestDispatcher, MultipleMockRequestDispatcher, + }; + + static MOCK_KMS_CLIENT: OnceCell = OnceCell::new(); + + fn mock_kms_client() -> &'static KmsClient { + MOCK_KMS_CLIENT.get_or_init(|| { + // aws kms get-public-key --key-id + let pubkey_response = r#"{ + "KeyId": "arn:aws:kms:ap-southeast-1:000000000000:key/XXXXXXXX-XXXX-XXXX-XXXX-XXXXXXXXXXXX", + "PublicKey": "MFYwEAYHKoZIzj0CAQYFK4EEAAoDQgAEhn6q/sPAS/tU0M49HnbT2N/o2ApVcOxg8RZmtbTrQKUZ8t2s6bi2/AJ+OcVbtavZzqCRttJG6kS/pyEa53AytQ==", + "CustomerMasterKeySpec": "ECC_SECG_P256K1", + "KeySpec": "ECC_SECG_P256K1", + "KeyUsage": "SIGN_VERIFY", + "SigningAlgorithms": [ + "ECDSA_SHA_256" + ] + }"#; + // aws kms sign --key-id --message ZKoCXIP1QLeb/r/mmWQwUS6UHyfxS6KYfu+RJwaRG2c= \ + // --signing-algorithm ECDSA_SHA_256 --message-type DIGEST + // NB: `message` here is `[0u8; 128]` hashed with `ethers_core::utils::hash_message` + let sign_response = r#"{ + "KeyId": "arn:aws:kms:ap-southeast-1:000000000000:key/XXXXXXXX-XXXX-XXXX-XXXX-XXXXXXXXXXXX", + "Signature": "MEYCIQCzqO4YPbzgw9LlRVB+X040Rb+e7rqNMZf2DqWe5SmY+gIhANi0TTSPDM4FUwrY7hRUZsDcBFHptIdUEak/fOod6UQQ", + "SigningAlgorithm": "ECDSA_SHA_256" + }"#; + let request_dispatcher = MultipleMockRequestDispatcher::new([ + MockRequestDispatcher::default().with_body(pubkey_response.clone()), + MockRequestDispatcher::default().with_body(pubkey_response), + MockRequestDispatcher::default().with_body(sign_response), + ]); + KmsClient::new_with( + request_dispatcher, + MockCredentialsProvider, + Default::default(), + ) + }) + } + + #[tokio::test(flavor = "multi_thread", worker_threads = 1)] + async fn it_instantiates_and_signs() { + let id = "XXXXXXXX-XXXX-XXXX-XXXX-XXXXXXXXXXXX"; + let kms_client = mock_kms_client(); + + let signer = AwsPair::new_with_client(id, kms_client).await; + + assert!(signer.is_ok()); + + let signer = signer.unwrap(); + let message = [0u8; 128]; + let signature = signer.sign(&message); + + assert_eq!( + base64::encode(signature), + "s6juGD284MPS5UVQfl9ONEW/nu66jTGX9g6lnuUpmPonS7LLcPMx+qz1JxHrq5k93qqK/PrBTCoWkuGiskz9MSM=" + ); + } +}