Skip to content

Commit

Permalink
Problem: zksync eth_signer private key loading security concerns (#44)
Browse files Browse the repository at this point in the history
    Co-authored-by: Calvin Lau <[email protected]>
  • Loading branch information
JayT106 committed Jul 11, 2024
1 parent 9cdee2c commit fd683a1
Show file tree
Hide file tree
Showing 10 changed files with 1,762 additions and 938 deletions.
2,363 changes: 1,459 additions & 904 deletions Cargo.lock

Large diffs are not rendered by default.

3 changes: 2 additions & 1 deletion core/bin/zksync_server/src/node_builder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ use zksync_node_framework::{
main_node_strategy::MainNodeInitStrategyLayer, NodeStorageInitializerLayer,
},
object_store::ObjectStoreLayer,
pk_signing_eth_client::PKSigningEthClientLayer,
pk_signing_eth_client::{PKSigningEthClientLayer, SigningEthClientType},
pools_layer::PoolsLayerBuilder,
postgres_metrics::PostgresMetricsLayer,
prometheus_exporter::PrometheusExporterLayer,
Expand Down Expand Up @@ -146,6 +146,7 @@ impl MainNodeBuilder {
self.contracts_config.clone(),
self.genesis_config.l1_chain_id,
wallets,
SigningEthClientType::PKSigningEthClient,
));
Ok(self)
}
Expand Down
2 changes: 1 addition & 1 deletion core/lib/eth_client/src/clients/http/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ use vise::{
Buckets, Counter, EncodeLabelSet, EncodeLabelValue, Family, Histogram, LabeledFamily, Metrics,
};

pub use self::signing::{PKSigningClient, SigningClient};
pub use self::signing::{GKMSSigningClient, PKSigningClient, SigningClient};

mod decl;
mod query;
Expand Down
33 changes: 32 additions & 1 deletion core/lib/eth_client/src/clients/http/signing.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ use std::{fmt, sync::Arc};

use async_trait::async_trait;
use zksync_contracts::hyperchain_contract;
use zksync_eth_signer::{EthereumSigner, PrivateKeySigner, TransactionParameters};
use zksync_eth_signer::{EthereumSigner, GKMSSigner, PrivateKeySigner, TransactionParameters};
use zksync_types::{
ethabi, web3, Address, K256PrivateKey, L1ChainId, EIP_4844_TX_TYPE, H160, U256,
};
Expand Down Expand Up @@ -40,6 +40,37 @@ impl PKSigningClient {
}
}

pub type GKMSSigningClient = SigningClient<GKMSSigner>;

impl GKMSSigningClient {
pub async fn new_raw(
diamond_proxy_addr: Address,
default_priority_fee_per_gas: u64,
l1_chain_id: L1ChainId,
query_client: Box<DynClient<L1>>,
key_name: String,
) -> Self {
let signer = match GKMSSigner::new(key_name, l1_chain_id.0).await {
Ok(s) => s,
Err(e) => panic!("Failed to create GKMSSigner: {:?}", e),
};

SigningClient::new(
query_client,
hyperchain_contract(),
signer.get_address().await.unwrap(),
signer,
diamond_proxy_addr,
default_priority_fee_per_gas.into(),
l1_chain_id,
)
}

pub fn get_address(&self) -> Address {
self.inner.sender_account
}
}

/// Gas limit value to be used in transaction if for some reason
/// gas limit was not set for it.
///
Expand Down
2 changes: 1 addition & 1 deletion core/lib/eth_client/src/clients/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,6 @@ mod mock;
pub use zksync_web3_decl::client::{Client, DynClient, L1};

pub use self::{
http::{PKSigningClient, SigningClient},
http::{GKMSSigningClient, PKSigningClient, SigningClient},
mock::{MockEthereum, MockEthereumBuilder},
};
5 changes: 5 additions & 0 deletions core/lib/eth_signer/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,11 @@ zksync_types.workspace = true
rlp.workspace = true
thiserror.workspace = true
async-trait.workspace = true
google-cloud-kms = { git="https://github.com/yoshidan/google-cloud-rust.git", tag="v20240627", features=["eth"]}
google-cloud-gax = { git="https://github.com/yoshidan/google-cloud-rust.git", tag="v20240627"}
hex = "0.4.3"
tracing = "0.1"
ethers-signers = "2.0"

[dev-dependencies]
tokio = { workspace = true, features = ["full"] }
161 changes: 161 additions & 0 deletions core/lib/eth_signer/src/gkms_signer.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,161 @@
use std::result::Result;

use ethers_signers::Signer as EthSigner;
use google_cloud_gax::retry::RetrySetting;
use google_cloud_kms::{
client::{Client, ClientConfig},
signer::ethereum::Signer,
};
use hex;
use tracing::{self};
use zksync_types::{
web3::{keccak256, Signature},
Address, EIP712TypedStructure, Eip712Domain, PackedEthSignature, H256, U256,
};

use crate::{
raw_ethereum_tx::{Transaction, TransactionParameters},
EthereumSigner, SignerError,
};

pub const GOOGLE_KMS_OP_KEY_NAME: &str = "GOOGLE_KMS_OP_KEY_NAME";
pub const GOOGLE_KMS_OP_BLOB_KEY_NAME: &str = "GOOGLE_KMS_OP_BLOB_KEY_NAME";

#[derive(Debug, Clone)]
pub struct GKMSSigner {
signer: Signer,
}

impl GKMSSigner {
pub async fn new(key_name: String, _chain_id: u64) -> Result<Self, SignerError> {
let config = ClientConfig::default()
.with_auth()
.await
.map_err(|e| SignerError::SigningFailed(e.to_string()))?;

let client = Client::new(config)
.await
.map_err(|e| SignerError::SigningFailed(e.to_string()))?;

let signer = Signer::new(client, &key_name, _chain_id, Some(RetrySetting::default()))
.await
.map_err(|e| SignerError::SigningFailed(e.to_string()))?;

tracing::info!("KMS signer address: {:?}", hex::encode(signer.address()));

Ok(GKMSSigner { signer })
}

fn u256_to_h256(u: U256) -> H256 {
let mut bytes = [0u8; 32];
u.to_big_endian(&mut bytes);
H256::from(bytes)
}
}

#[async_trait::async_trait]
impl EthereumSigner for GKMSSigner {
/// Get Ethereum address that matches the private key.
async fn get_address(&self) -> Result<Address, SignerError> {
Ok(self.signer.address())
}

/// Signs typed struct using Ethereum private key by EIP-712 signature standard.
/// Result of this function is the equivalent of RPC calling `eth_signTypedData`.
async fn sign_typed_data<S: EIP712TypedStructure + Sync>(
&self,
domain: &Eip712Domain,
typed_struct: &S,
) -> Result<PackedEthSignature, SignerError> {
let digest =
H256::from(PackedEthSignature::typed_data_to_signed_bytes(domain, typed_struct).0);

let signature = self
.signer
.sign_digest(digest.as_bytes())
.await
.map_err(|e| SignerError::SigningFailed(e.to_string()))?;

// Convert the signature components to the appropriate format.
let r_h256 = GKMSSigner::u256_to_h256(signature.r);
let s_h256 = GKMSSigner::u256_to_h256(signature.s);

// Ensure the `v` component is in the correct byte format.
let v_byte = match signature.v.try_into() {
Ok(v) => v,
Err(_) => {
return Err(SignerError::SigningFailed(
"V value conversion failed".to_string(),
))
}
};

// Construct the Ethereum signature from the R, S, and V components.
let eth_sig = PackedEthSignature::from_rsv(&r_h256, &s_h256, v_byte);

Ok(eth_sig)
}

/// Signs and returns the RLP-encoded transaction.
async fn sign_transaction(
&self,
raw_tx: TransactionParameters,
) -> Result<Vec<u8>, SignerError> {
// According to the code in web3 <https://docs.rs/web3/latest/src/web3/api/accounts.rs.html#86>
// We should use `max_fee_per_gas` as `gas_price` if we use EIP1559
let gas_price = raw_tx.max_fee_per_gas;
let max_priority_fee_per_gas = raw_tx.max_priority_fee_per_gas;

let tx = Transaction {
to: raw_tx.to,
nonce: raw_tx.nonce,
gas: raw_tx.gas,
gas_price,
value: raw_tx.value,
data: raw_tx.data,
transaction_type: raw_tx.transaction_type,
access_list: raw_tx.access_list.unwrap_or_default(),
max_priority_fee_per_gas,
max_fee_per_blob_gas: raw_tx.max_fee_per_blob_gas,
blob_versioned_hashes: raw_tx.blob_versioned_hashes,
};

let encoded = tx.encode_pub(raw_tx.chain_id, None);
let digest = H256(keccak256(encoded.as_ref()));

let signature = self
.signer
.sign_digest(digest.as_bytes())
.await
.map_err(|e| SignerError::SigningFailed(e.to_string()))?;

let adjusted_v = if let Some(transaction_type) = tx.transaction_type.map(|t| t.as_u64()) {
match transaction_type {
0 => signature.v + raw_tx.chain_id * 2 + 35, // EIP-155
_ => signature.v, // EIP-2930 and others
}
} else {
signature.v + raw_tx.chain_id * 2 + 35 // EIP-155
};

let r_h256 = GKMSSigner::u256_to_h256(signature.r);
let s_h256 = GKMSSigner::u256_to_h256(signature.s);

tracing::debug!(
"KMS sign_transaction signature: v: {}, r: {}, s: {}",
adjusted_v,
hex::encode(r_h256),
hex::encode(s_h256),
);

let web3_sig = Signature {
v: adjusted_v,
r: r_h256,
s: s_h256,
};

let signed = tx.encode_pub(raw_tx.chain_id, Some(&web3_sig));

return Ok(signed);
}
}
5 changes: 4 additions & 1 deletion core/lib/eth_signer/src/lib.rs
Original file line number Diff line number Diff line change
@@ -1,8 +1,11 @@
use async_trait::async_trait;
use zksync_types::{Address, EIP712TypedStructure, Eip712Domain, PackedEthSignature};

pub use crate::{pk_signer::PrivateKeySigner, raw_ethereum_tx::TransactionParameters};
pub use crate::{
gkms_signer::GKMSSigner, pk_signer::PrivateKeySigner, raw_ethereum_tx::TransactionParameters,
};

mod gkms_signer;
mod pk_signer;
mod raw_ethereum_tx;

Expand Down
4 changes: 4 additions & 0 deletions core/lib/eth_signer/src/raw_ethereum_tx.rs
Original file line number Diff line number Diff line change
Expand Up @@ -268,4 +268,8 @@ impl Transaction {
transaction_hash,
}
}

pub fn encode_pub(&self, chain_id: u64, signature: Option<&Signature>) -> Vec<u8> {
self.encode(chain_id, signature)
}
}
Loading

0 comments on commit fd683a1

Please sign in to comment.