From 1887340dcf98c451fc3bd5e66a7a5e58cb66a8ff Mon Sep 17 00:00:00 2001 From: Eric Woolsey Date: Tue, 16 Jan 2024 18:58:28 -0800 Subject: [PATCH 1/3] shortened api key and fixed predefined toml --- config.toml | 12 +++++++----- src/api_key.rs | 42 +++++++++++++++++------------------------- 2 files changed, 24 insertions(+), 30 deletions(-) diff --git a/config.toml b/config.toml index 3977e73..a5c194f 100644 --- a/config.toml +++ b/config.toml @@ -3,16 +3,18 @@ escalation_interval = "1m" datadog_enabled = false statsd_enabled = false -[predefined.network] +[service.predefined.network] chain_id = 31337 -http_url = "http://127.0.0.1:8545" -ws_url = "ws://127.0.0.1:8545" +name = "predefined" +http_rpc = "http://127.0.0.1:8545" +ws_rpc = "ws://127.0.0.1:8545" -[predefined.relayer] +[service.predefined.relayer] id = "1b908a34-5dc1-4d2d-a146-5eb46e975830" +name = "predefined" chain_id = 31337 key_id = "d10607662a85424f02a33fb1e6d095bd0ac7154396ff09762e41f82ff2233aaa" -api_key = "G5CKNF3BTS2hRl60bpdYMNPqXvXsP-QZd2lrtmgctsnllwU9D3Z4D8gOt04M0QNH" +api_key = "G5CKNF3BTS2hRl60bpdYMNPqXvXsP-QZd2lrtmgctsk=" [server] host = "127.0.0.1:3000" diff --git a/src/api_key.rs b/src/api_key.rs index 02d5576..6a70b1d 100644 --- a/src/api_key.rs +++ b/src/api_key.rs @@ -3,40 +3,37 @@ use std::str::FromStr; use base64::Engine; use rand::rngs::OsRng; -use rand::RngCore; +use rand::Rng; use serde::Serialize; use sha3::{Digest, Sha3_256}; +const SECRET_LEN: usize = 16; +const UUID_LEN: usize = 16; + #[derive(Debug, Clone, PartialEq, Eq)] pub struct ApiKey { pub relayer_id: String, - pub api_key: [u8; 32], + pub secret: [u8; SECRET_LEN], } impl ApiKey { - pub fn new(relayer_id: impl ToString, key: [u8; 32]) -> Self { + pub fn new(relayer_id: impl ToString, secret: [u8; SECRET_LEN]) -> Self { let relayer_id = relayer_id.to_string(); - Self { - relayer_id, - api_key: key, - } + Self { relayer_id, secret } } pub fn random(relayer_id: impl ToString) -> Self { let relayer_id = relayer_id.to_string(); - let mut api_key = [0u8; 32]; - OsRng.fill_bytes(&mut api_key); - Self { relayer_id, - api_key, + secret: OsRng.gen(), } } pub fn api_key_hash(&self) -> [u8; 32] { - Sha3_256::digest(self.api_key).into() + Sha3_256::digest(self.secret).into() } } @@ -66,32 +63,31 @@ impl FromStr for ApiKey { fn from_str(s: &str) -> Result { let buffer = base64::prelude::BASE64_URL_SAFE.decode(s)?; - if buffer.len() != 48 { + if buffer.len() != UUID_LEN + SECRET_LEN { return Err(eyre::eyre!("invalid api key")); } - let relayer_id = uuid::Uuid::from_slice(&buffer[..16])?; + let relayer_id = uuid::Uuid::from_slice(&buffer[..UUID_LEN])?; let relayer_id = relayer_id.to_string(); - let mut api_key = [0u8; 32]; - api_key.copy_from_slice(&buffer[16..]); + let api_key = buffer[UUID_LEN..].try_into()?; Ok(Self { relayer_id, - api_key, + secret: api_key, }) } } impl std::fmt::Display for ApiKey { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - let mut buffer = [0u8; 48]; + let mut buffer = [0u8; 32]; let relayer_id = uuid::Uuid::parse_str(&self.relayer_id) .map_err(|_| std::fmt::Error)?; - buffer[..16].copy_from_slice(relayer_id.as_bytes()); - buffer[16..].copy_from_slice(&self.api_key); + buffer[..UUID_LEN].copy_from_slice(relayer_id.as_bytes()); + buffer[UUID_LEN..].copy_from_slice(&self.secret); let encoded = base64::prelude::BASE64_URL_SAFE.encode(buffer); @@ -102,15 +98,11 @@ impl std::fmt::Display for ApiKey { #[cfg(test)] mod tests { use rand::rngs::OsRng; - use rand::RngCore; use super::*; fn random_api_key() -> ApiKey { - let mut api_key = [0u8; 32]; - OsRng.fill_bytes(&mut api_key); - - ApiKey::new(uuid::Uuid::new_v4().to_string(), api_key) + ApiKey::new(uuid::Uuid::new_v4().to_string(), OsRng.gen()) } #[test] From 1c5268c13428b4ff8f66105e3308c70d78bb546f Mon Sep 17 00:00:00 2001 From: Eric Woolsey Date: Fri, 19 Jan 2024 12:55:11 -0800 Subject: [PATCH 2/3] dynamic length api key --- src/api_key.rs | 98 +++++++++++++++++++++++++------- src/app.rs | 5 +- src/server/routes/relayer.rs | 4 +- src/server/routes/transaction.rs | 8 +-- src/service.rs | 4 +- 5 files changed, 89 insertions(+), 30 deletions(-) diff --git a/src/api_key.rs b/src/api_key.rs index 6a70b1d..02881f4 100644 --- a/src/api_key.rs +++ b/src/api_key.rs @@ -7,20 +7,28 @@ use rand::Rng; use serde::Serialize; use sha3::{Digest, Sha3_256}; -const SECRET_LEN: usize = 16; +const DEFAULT_SECRET_LEN: usize = 16; +const MIN_SECRET_LEN: usize = 16; +const MAX_SECRET_LEN: usize = 32; const UUID_LEN: usize = 16; #[derive(Debug, Clone, PartialEq, Eq)] pub struct ApiKey { - pub relayer_id: String, - pub secret: [u8; SECRET_LEN], + relayer_id: String, + secret: Vec, } impl ApiKey { - pub fn new(relayer_id: impl ToString, secret: [u8; SECRET_LEN]) -> Self { + pub fn new( + relayer_id: impl ToString, + secret: Vec, + ) -> eyre::Result { + if secret.len() < MIN_SECRET_LEN || secret.len() > MAX_SECRET_LEN { + eyre::bail!("invalid api key"); + } let relayer_id = relayer_id.to_string(); - Self { relayer_id, secret } + Ok(Self { relayer_id, secret }) } pub fn random(relayer_id: impl ToString) -> Self { @@ -28,12 +36,16 @@ impl ApiKey { Self { relayer_id, - secret: OsRng.gen(), + secret: OsRng.gen::<[u8; DEFAULT_SECRET_LEN]>().into(), } } - pub fn api_key_hash(&self) -> [u8; 32] { - Sha3_256::digest(self.secret).into() + pub fn api_key_secret_hash(&self) -> [u8; 32] { + Sha3_256::digest(self.secret.clone()).into() + } + + pub fn relayer_id(&self) -> &str { + &self.relayer_id } } @@ -63,33 +75,34 @@ impl FromStr for ApiKey { fn from_str(s: &str) -> Result { let buffer = base64::prelude::BASE64_URL_SAFE.decode(s)?; - if buffer.len() != UUID_LEN + SECRET_LEN { - return Err(eyre::eyre!("invalid api key")); + if buffer.len() < UUID_LEN + MIN_SECRET_LEN + || buffer.len() > UUID_LEN + MAX_SECRET_LEN + { + eyre::bail!("invalid api key"); } let relayer_id = uuid::Uuid::from_slice(&buffer[..UUID_LEN])?; let relayer_id = relayer_id.to_string(); - let api_key = buffer[UUID_LEN..].try_into()?; + let secret = buffer[UUID_LEN..].into(); - Ok(Self { - relayer_id, - secret: api_key, - }) + Ok(Self { relayer_id, secret }) } } impl std::fmt::Display for ApiKey { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - let mut buffer = [0u8; 32]; - let relayer_id = uuid::Uuid::parse_str(&self.relayer_id) .map_err(|_| std::fmt::Error)?; - buffer[..UUID_LEN].copy_from_slice(relayer_id.as_bytes()); - buffer[UUID_LEN..].copy_from_slice(&self.secret); + let bytes = relayer_id + .as_bytes() + .iter() + .cloned() + .chain(self.secret.iter().cloned()) + .collect::>(); - let encoded = base64::prelude::BASE64_URL_SAFE.encode(buffer); + let encoded = base64::prelude::BASE64_URL_SAFE.encode(bytes); write!(f, "{}", encoded) } @@ -102,7 +115,29 @@ mod tests { use super::*; fn random_api_key() -> ApiKey { - ApiKey::new(uuid::Uuid::new_v4().to_string(), OsRng.gen()) + ApiKey::new( + uuid::Uuid::new_v4().to_string(), + OsRng.gen::<[u8; DEFAULT_SECRET_LEN]>().into(), + ) + .unwrap() + } + + fn invalid_short_api_key() -> ApiKey { + let mut buf = [0u8; MAX_SECRET_LEN + 1]; + OsRng.fill(&mut buf[..]); + ApiKey { + relayer_id: uuid::Uuid::new_v4().to_string(), + secret: buf.into(), + } + } + + fn invalid_long_api_key() -> ApiKey { + let mut buf = [0u8; MAX_SECRET_LEN + 1]; + OsRng.fill(&mut buf[..]); + ApiKey { + relayer_id: uuid::Uuid::new_v4().to_string(), + secret: buf.into(), + } } #[test] @@ -118,6 +153,27 @@ mod tests { assert_eq!(api_key, api_key_parsed); } + #[test] + fn assert_api_key_length_validation() { + let long_api_key = invalid_long_api_key(); + let _ = ApiKey::new( + long_api_key.relayer_id.clone(), + long_api_key.secret.clone(), + ) + .expect_err("long api key should be invalid"); + let _ = ApiKey::from_str(long_api_key.to_string().as_str()) + .expect_err("long api key should be invalid"); + + let short_api_key = invalid_short_api_key(); + let _ = ApiKey::new( + short_api_key.relayer_id.clone(), + short_api_key.secret.clone(), + ) + .expect_err("short api key should be invalid"); + let _ = ApiKey::from_str(short_api_key.to_string().as_str()) + .expect_err("short api key should be invalid"); + } + #[test] fn from_to_serde_json() { let api_key = random_api_key(); diff --git a/src/app.rs b/src/app.rs index 712423a..f3a1c49 100644 --- a/src/app.rs +++ b/src/app.rs @@ -81,7 +81,10 @@ impl App { api_token: &ApiKey, ) -> eyre::Result { self.db - .is_api_key_valid(&api_token.relayer_id, api_token.api_key_hash()) + .is_api_key_valid( + api_token.relayer_id(), + api_token.api_key_secret_hash(), + ) .await } } diff --git a/src/server/routes/relayer.rs b/src/server/routes/relayer.rs index 88d923e..52b5b77 100644 --- a/src/server/routes/relayer.rs +++ b/src/server/routes/relayer.rs @@ -131,7 +131,7 @@ pub async fn relayer_rpc( return Err(ApiError::Unauthorized); } - let relayer_info = app.db.get_relayer(&api_token.relayer_id).await?; + let relayer_info = app.db.get_relayer(api_token.relayer_id()).await?; // TODO: Cache? let http_provider = app.http_provider(relayer_info.chain_id).await?; @@ -161,7 +161,7 @@ pub async fn create_relayer_api_key( let api_key = ApiKey::random(&relayer_id); app.db - .create_api_key(&relayer_id, api_key.api_key_hash()) + .create_api_key(&relayer_id, api_key.api_key_secret_hash()) .await?; Ok(Json(CreateApiKeyResponse { api_key })) diff --git a/src/server/routes/transaction.rs b/src/server/routes/transaction.rs index 75dceec..46d46e2 100644 --- a/src/server/routes/transaction.rs +++ b/src/server/routes/transaction.rs @@ -98,7 +98,7 @@ pub async fn send_tx( req.value, req.gas_limit, req.priority, - &api_token.relayer_id, + api_token.relayer_id(), ) .await?; @@ -120,13 +120,13 @@ pub async fn get_txs( let txs = match query.status { Some(GetTxResponseStatus::TxStatus(status)) => { app.db - .read_txs(&api_token.relayer_id, Some(Some(status))) + .read_txs(api_token.relayer_id(), Some(Some(status))) .await? } Some(GetTxResponseStatus::Unsent(_)) => { - app.db.read_txs(&api_token.relayer_id, Some(None)).await? + app.db.read_txs(api_token.relayer_id(), Some(None)).await? } - None => app.db.read_txs(&api_token.relayer_id, None).await?, + None => app.db.read_txs(api_token.relayer_id(), None).await?, }; let txs = diff --git a/src/service.rs b/src/service.rs index f08b663..94260ef 100644 --- a/src/service.rs +++ b/src/service.rs @@ -125,8 +125,8 @@ async fn initialize_predefined_values( app.db .create_api_key( - &predefined.relayer.api_key.relayer_id, - predefined.relayer.api_key.api_key_hash(), + predefined.relayer.api_key.relayer_id(), + predefined.relayer.api_key.api_key_secret_hash(), ) .await?; From 38892de560f8de36c5813cd6e7f7fb5fbe945e51 Mon Sep 17 00:00:00 2001 From: Eric Woolsey Date: Fri, 19 Jan 2024 16:49:08 -0800 Subject: [PATCH 3/3] Fix for security issue where api keys were being exposed in logs and traces --- src/api_key.rs | 82 ++++++++++++++++++++++++-------- src/client.rs | 11 +++-- src/server/routes/relayer.rs | 2 +- src/server/routes/transaction.rs | 4 +- tests/rpc_access.rs | 7 ++- 5 files changed, 76 insertions(+), 30 deletions(-) diff --git a/src/api_key.rs b/src/api_key.rs index 02881f4..06a1216 100644 --- a/src/api_key.rs +++ b/src/api_key.rs @@ -12,10 +12,37 @@ const MIN_SECRET_LEN: usize = 16; const MAX_SECRET_LEN: usize = 32; const UUID_LEN: usize = 16; -#[derive(Debug, Clone, PartialEq, Eq)] +#[derive(Clone, Eq, PartialEq)] +struct ApiSecret(Vec); + +/// Derive Serialize manually to avoid leaking the secret. +impl Serialize for ApiSecret { + fn serialize( + &self, + serializer: S, + ) -> Result { + serializer.collect_str(&"***") + } +} + +/// Derive Debug manually to avoid leaking the secret. +impl std::fmt::Debug for ApiSecret { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + f.debug_tuple("ApiSecret").field(&"***").finish() + } +} + +/// Zero out the secret when dropped. +impl Drop for ApiSecret { + fn drop(&mut self) { + self.0.iter_mut().for_each(|b| *b = 0); + } +} + +#[derive(Clone, Debug, PartialEq, Eq)] pub struct ApiKey { relayer_id: String, - secret: Vec, + secret: ApiSecret, } impl ApiKey { @@ -28,7 +55,10 @@ impl ApiKey { } let relayer_id = relayer_id.to_string(); - Ok(Self { relayer_id, secret }) + Ok(Self { + relayer_id, + secret: ApiSecret(secret), + }) } pub fn random(relayer_id: impl ToString) -> Self { @@ -36,12 +66,12 @@ impl ApiKey { Self { relayer_id, - secret: OsRng.gen::<[u8; DEFAULT_SECRET_LEN]>().into(), + secret: ApiSecret(OsRng.gen::<[u8; DEFAULT_SECRET_LEN]>().into()), } } pub fn api_key_secret_hash(&self) -> [u8; 32] { - Sha3_256::digest(self.secret.clone()).into() + Sha3_256::digest(self.secret.0.clone()).into() } pub fn relayer_id(&self) -> &str { @@ -54,7 +84,8 @@ impl Serialize for ApiKey { &self, serializer: S, ) -> Result { - serializer.collect_str(self) + serializer + .serialize_str(&self.reveal().map_err(serde::ser::Error::custom)?) } } @@ -84,14 +115,14 @@ impl FromStr for ApiKey { let relayer_id = uuid::Uuid::from_slice(&buffer[..UUID_LEN])?; let relayer_id = relayer_id.to_string(); - let secret = buffer[UUID_LEN..].into(); + let secret = ApiSecret(buffer[UUID_LEN..].into()); Ok(Self { relayer_id, secret }) } } -impl std::fmt::Display for ApiKey { - fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { +impl ApiKey { + pub fn reveal(&self) -> eyre::Result { let relayer_id = uuid::Uuid::parse_str(&self.relayer_id) .map_err(|_| std::fmt::Error)?; @@ -99,12 +130,10 @@ impl std::fmt::Display for ApiKey { .as_bytes() .iter() .cloned() - .chain(self.secret.iter().cloned()) + .chain(self.secret.0.iter().cloned()) .collect::>(); - let encoded = base64::prelude::BASE64_URL_SAFE.encode(bytes); - - write!(f, "{}", encoded) + Ok(base64::prelude::BASE64_URL_SAFE.encode(bytes)) } } @@ -127,7 +156,7 @@ mod tests { OsRng.fill(&mut buf[..]); ApiKey { relayer_id: uuid::Uuid::new_v4().to_string(), - secret: buf.into(), + secret: ApiSecret(buf.into()), } } @@ -136,7 +165,7 @@ mod tests { OsRng.fill(&mut buf[..]); ApiKey { relayer_id: uuid::Uuid::new_v4().to_string(), - secret: buf.into(), + secret: ApiSecret(buf.into()), } } @@ -144,33 +173,44 @@ mod tests { fn from_to_str() { let api_key = random_api_key(); - let api_key_str = api_key.to_string(); + let api_key_str = api_key.reveal().unwrap(); println!("api_key_str = {api_key_str}"); let api_key_parsed = api_key_str.parse::().unwrap(); - assert_eq!(api_key, api_key_parsed); + assert_eq!(api_key.relayer_id, api_key_parsed.relayer_id); + assert_eq!(api_key.secret, api_key_parsed.secret); + } + + #[test] + fn assert_api_secret_debug() { + let api_secret = random_api_key().secret; + assert_eq!(&format!("{:?}", api_secret), "ApiSecret(\"***\")"); } #[test] fn assert_api_key_length_validation() { let long_api_key = invalid_long_api_key(); + let _ = ApiKey::new( long_api_key.relayer_id.clone(), - long_api_key.secret.clone(), + long_api_key.secret.0.clone(), ) .expect_err("long api key should be invalid"); - let _ = ApiKey::from_str(long_api_key.to_string().as_str()) + + let _ = ApiKey::from_str(&long_api_key.reveal().unwrap()) .expect_err("long api key should be invalid"); let short_api_key = invalid_short_api_key(); + let _ = ApiKey::new( short_api_key.relayer_id.clone(), - short_api_key.secret.clone(), + short_api_key.secret.0.clone(), ) .expect_err("short api key should be invalid"); - let _ = ApiKey::from_str(short_api_key.to_string().as_str()) + + let _ = ApiKey::from_str(&short_api_key.reveal().unwrap()) .expect_err("short api key should be invalid"); } diff --git a/src/client.rs b/src/client.rs index 49aa7d1..9f5c108 100644 --- a/src/client.rs +++ b/src/client.rs @@ -86,8 +86,11 @@ impl TxSitterClient { api_key: &ApiKey, req: &SendTxRequest, ) -> eyre::Result { - self.json_post(&format!("{}/1/api/{api_key}/tx", self.url), req) - .await + self.json_post( + &format!("{}/1/api/{}/tx", self.url, api_key.reveal()?), + req, + ) + .await } pub async fn get_tx( @@ -96,9 +99,9 @@ impl TxSitterClient { tx_id: &str, ) -> eyre::Result { self.json_get(&format!( - "{}/1/api/{api_key}/tx/{tx_id}", + "{}/1/api/{}/tx/{tx_id}", self.url, - api_key = api_key, + api_key.reveal()?, tx_id = tx_id )) .await diff --git a/src/server/routes/relayer.rs b/src/server/routes/relayer.rs index 52b5b77..da87148 100644 --- a/src/server/routes/relayer.rs +++ b/src/server/routes/relayer.rs @@ -121,7 +121,7 @@ pub async fn purge_unsent_txs( Ok(()) } -#[tracing::instrument(skip(app))] +#[tracing::instrument(skip(app, api_token))] pub async fn relayer_rpc( State(app): State>, Path(api_token): Path, diff --git a/src/server/routes/transaction.rs b/src/server/routes/transaction.rs index 46d46e2..a74cf30 100644 --- a/src/server/routes/transaction.rs +++ b/src/server/routes/transaction.rs @@ -74,7 +74,7 @@ pub enum UnsentStatus { Unsent, } -#[tracing::instrument(skip(app))] +#[tracing::instrument(skip(app, api_token))] pub async fn send_tx( State(app): State>, Path(api_token): Path, @@ -152,7 +152,7 @@ pub async fn get_txs( Ok(Json(txs)) } -#[tracing::instrument(skip(app))] +#[tracing::instrument(skip(app, api_token))] pub async fn get_tx( State(app): State>, Path((api_token, tx_id)): Path<(ApiKey, String)>, diff --git a/tests/rpc_access.rs b/tests/rpc_access.rs index f646210..15eed04 100644 --- a/tests/rpc_access.rs +++ b/tests/rpc_access.rs @@ -15,8 +15,11 @@ async fn rpc_access() -> eyre::Result<()> { let CreateApiKeyResponse { api_key } = client.create_relayer_api_key(DEFAULT_RELAYER_ID).await?; - let rpc_url = - format!("http://{}/1/api/{api_key}/rpc", service.local_addr()); + let rpc_url = format!( + "http://{}/1/api/{}/rpc", + service.local_addr(), + api_key.reveal()? + ); let provider = Provider::new(Http::new(rpc_url.parse::()?));