Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Problem: zksync eth_signer private key loading security concerns #44

Merged
merged 10 commits into from
Jul 9, 2024
Merged
Show file tree
Hide file tree
Changes from 8 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
829 changes: 742 additions & 87 deletions Cargo.lock

Large diffs are not rendered by default.

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 @@ -6,7 +6,7 @@ use vise::{

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

mod decl;
Expand Down
35 changes: 34 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,9 @@ 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::{
g_kms_signer::GKMSSigner, EthereumSigner, PrivateKeySigner, TransactionParameters,
};
use zksync_types::{
ethabi, web3, Address, K256PrivateKey, L1ChainId, EIP_4844_TX_TYPE, H160, U256,
};
Expand Down Expand Up @@ -39,6 +41,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<dyn EthInterface>,
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 @@ -4,6 +4,6 @@ mod http;
mod mock;

pub use self::{
http::{PKSigningClient, QueryClient, SigningClient},
http::{GKMSSigningClient, PKSigningClient, QueryClient, SigningClient},
mock::MockEthereum,
};
6 changes: 6 additions & 0 deletions core/lib/eth_signer/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,12 @@ 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", rev="eb15032", features=["eth"]}
google-cloud-auth = { git="https://github.com/yoshidan/google-cloud-rust.git", rev="eb15032"}
google-cloud-gax = { git="https://github.com/yoshidan/google-cloud-rust.git", rev="eb15032"}
hex = "0.4.3"
tracing = "0.1"
ethers-signers = "2.0"

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

use ethers_signers::Signer as EthSigner;
use google_cloud_auth::credentials::CredentialsFile;
use google_cloud_gax::retry::RetrySetting;
use google_cloud_kms::{
client::{google_cloud_auth, 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,
};

const GOOGLE_APPLICATION_CREDENTIALS_PATH: &str = "GOOGLE_APPLICATION_CREDENTIALS";
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(Clone)]
pub struct GKMSSigner {
signer: Signer,
}

impl GKMSSigner {
pub async fn new(key_name: String, _chain_id: u64) -> Result<Self, SignerError> {
let credentials_path =
std::env::var(GOOGLE_APPLICATION_CREDENTIALS_PATH).map_err(|_| {
SignerError::SigningFailed(
"Environment variable GOOGLE_APPLICATION_CREDENTIALS not found".to_string(),
)
})?;

tracing::info!(
"KMS signer credentail path: {:?}",
std::env::var(GOOGLE_APPLICATION_CREDENTIALS_PATH)
);

let cf = CredentialsFile::new_from_file(credentials_path)
calvinaco marked this conversation as resolved.
Show resolved Hide resolved
.await
.map_err(|e| SignerError::SigningFailed(e.to_string()))?;

let config = ClientConfig::default()
.with_credentials(cf)
.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);
}
}
1 change: 1 addition & 0 deletions core/lib/eth_signer/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ use zksync_types::{Address, EIP712TypedStructure, Eip712Domain, PackedEthSignatu

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

pub mod g_kms_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
Loading