From 18da0233149e6a0c547ca1dbb0786e3d67eb45c1 Mon Sep 17 00:00:00 2001 From: Eric Woolsey Date: Mon, 22 Jan 2024 08:33:50 -0800 Subject: [PATCH] Api Key Rework - PRO-467 (#17) * shortened api key and fixed predefined toml * dynamic length api key * Fix for security issue where api keys were being exposed in logs and traces --- config.toml | 12 ++- src/api_key.rs | 166 +++++++++++++++++++++++-------- src/app.rs | 5 +- src/client.rs | 11 +- src/server/routes/relayer.rs | 6 +- src/server/routes/transaction.rs | 12 +-- src/service.rs | 4 +- tests/rpc_access.rs | 7 +- 8 files changed, 161 insertions(+), 62 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..06a1216 100644 --- a/src/api_key.rs +++ b/src/api_key.rs @@ -3,40 +3,79 @@ 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}; -#[derive(Debug, Clone, PartialEq, Eq)] +const DEFAULT_SECRET_LEN: usize = 16; +const MIN_SECRET_LEN: usize = 16; +const MAX_SECRET_LEN: usize = 32; +const UUID_LEN: usize = 16; + +#[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 { - pub relayer_id: String, - pub api_key: [u8; 32], + relayer_id: String, + secret: ApiSecret, } impl ApiKey { - pub fn new(relayer_id: impl ToString, key: [u8; 32]) -> 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 { + Ok(Self { relayer_id, - api_key: key, - } + secret: ApiSecret(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: ApiSecret(OsRng.gen::<[u8; DEFAULT_SECRET_LEN]>().into()), } } - pub fn api_key_hash(&self) -> [u8; 32] { - Sha3_256::digest(self.api_key).into() + pub fn api_key_secret_hash(&self) -> [u8; 32] { + Sha3_256::digest(self.secret.0.clone()).into() + } + + pub fn relayer_id(&self) -> &str { + &self.relayer_id } } @@ -45,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)?) } } @@ -66,64 +106,112 @@ impl FromStr for ApiKey { fn from_str(s: &str) -> Result { let buffer = base64::prelude::BASE64_URL_SAFE.decode(s)?; - if buffer.len() != 48 { - 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[..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 secret = ApiSecret(buffer[UUID_LEN..].into()); - Ok(Self { - relayer_id, - 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; 48]; - +impl ApiKey { + pub fn reveal(&self) -> eyre::Result { 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); + let bytes = relayer_id + .as_bytes() + .iter() + .cloned() + .chain(self.secret.0.iter().cloned()) + .collect::>(); - let encoded = base64::prelude::BASE64_URL_SAFE.encode(buffer); - - write!(f, "{}", encoded) + Ok(base64::prelude::BASE64_URL_SAFE.encode(bytes)) } } #[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(), + 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: ApiSecret(buf.into()), + } + } - ApiKey::new(uuid::Uuid::new_v4().to_string(), api_key) + 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: ApiSecret(buf.into()), + } } #[test] 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.0.clone(), + ) + .expect_err("long api key should be invalid"); + + 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.0.clone(), + ) + .expect_err("short api key should be invalid"); + + let _ = ApiKey::from_str(&short_api_key.reveal().unwrap()) + .expect_err("short api key should be invalid"); } #[test] 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/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 88d923e..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, @@ -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..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, @@ -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 = @@ -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/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?; 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::()?));