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

@@ -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"

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<u8>);
+
+/// Derive Serialize manually to avoid leaking the secret.
+impl Serialize for ApiSecret {
+    fn serialize<S: serde::Serializer>(
+        &self,
+        serializer: S,
+    ) -> Result<S::Ok, S::Error> {
+        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<u8>,
+    ) -> eyre::Result<Self> {
+        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<S::Ok, S::Error> {
-        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<Self, Self::Err> {
         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<String> {
         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::<Vec<_>>();
 
-        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::<ApiKey>().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]

src/app.rs

@@ -81,7 +81,10 @@ impl App {
         api_token: &ApiKey,
     ) -> eyre::Result<bool> {
         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
     }
 }

src/client.rs

@@ -86,8 +86,11 @@ impl TxSitterClient {
         api_key: &ApiKey,
         req: &SendTxRequest,
     ) -> eyre::Result<SendTxResponse> {
-        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<GetTxResponse> {
         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

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<Arc<App>>,
     Path(api_token): Path<ApiKey>,
@@ -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 }))

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<Arc<App>>,
     Path(api_token): Path<ApiKey>,
@@ -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<Arc<App>>,
     Path((api_token, tx_id)): Path<(ApiKey, String)>,

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?;
 

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::<Url>()?));