From ea48886ed640433577aee80f5b78f8c77d00e061 Mon Sep 17 00:00:00 2001 From: Joe Clapis Date: Thu, 18 Sep 2025 09:10:22 -0400 Subject: [PATCH 01/13] Renamed a misnamed variable --- crates/common/src/signer/store.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/crates/common/src/signer/store.rs b/crates/common/src/signer/store.rs index bd4aa103..d70ea8a0 100644 --- a/crates/common/src/signer/store.rs +++ b/crates/common/src/signer/store.rs @@ -244,14 +244,14 @@ impl ProxyStore { serde_json::from_str(&file_content)?; let signer = EcdsaSigner::new_from_bytes(&key_and_delegation.secret)?; - let pubkey = signer.address(); + let address = signer.address(); let proxy_signer = EcdsaProxySigner { signer, delegation: key_and_delegation.delegation, }; - proxy_signers.ecdsa_signers.insert(pubkey, proxy_signer); - ecdsa_map.entry(module_id.clone()).or_default().push(pubkey); + proxy_signers.ecdsa_signers.insert(address, proxy_signer); + ecdsa_map.entry(module_id.clone()).or_default().push(address); } } } From 351c2c90e265b5e1b82b7ffc2d0c2605c7b68231 Mon Sep 17 00:00:00 2001 From: Joe Clapis Date: Thu, 18 Sep 2025 10:12:57 -0400 Subject: [PATCH 02/13] Added rate limit flagging to admin JWT auth --- crates/signer/src/service.rs | 19 +++++++++++------ tests/tests/signer_jwt_auth.rs | 38 ++++++++++++++++++++++++++++++++++ 2 files changed, 51 insertions(+), 6 deletions(-) diff --git a/crates/signer/src/service.rs b/crates/signer/src/service.rs index eb284289..ef0c7770 100644 --- a/crates/signer/src/service.rs +++ b/crates/signer/src/service.rs @@ -173,6 +173,16 @@ impl SigningService { } } +/// Marks a JWT authentication failure for a given client IP +fn mark_jwt_failure(state: &SigningState, client_ip: IpAddr) { + let mut failures = state.jwt_auth_failures.write(); + let failure_info = failures + .entry(client_ip) + .or_insert(JwtAuthFailureInfo { failure_count: 0, last_failure: Instant::now() }); + failure_info.failure_count += 1; + failure_info.last_failure = Instant::now(); +} + /// Authentication middleware layer async fn jwt_auth( State(state): State, @@ -200,12 +210,7 @@ async fn jwt_auth( Ok(next.run(req).await) } Err(SignerModuleError::Unauthorized) => { - let mut failures = state.jwt_auth_failures.write(); - let failure_info = failures - .entry(client_ip) - .or_insert(JwtAuthFailureInfo { failure_count: 0, last_failure: Instant::now() }); - failure_info.failure_count += 1; - failure_info.last_failure = Instant::now(); + mark_jwt_failure(&state, client_ip); Err(SignerModuleError::Unauthorized) } Err(err) => Err(err), @@ -320,11 +325,13 @@ async fn admin_auth( // Skip payload hash comparison for requests without a body validate_admin_jwt(jwt, &state.admin_secret.read(), None).map_err(|e| { error!("Unauthorized request. Invalid JWT: {e}"); + mark_jwt_failure(&state, client_ip); SignerModuleError::Unauthorized })?; } else { validate_admin_jwt(jwt, &state.admin_secret.read(), Some(&bytes)).map_err(|e| { error!("Unauthorized request. Invalid payload hash in JWT claims: {e}"); + mark_jwt_failure(&state, client_ip); SignerModuleError::Unauthorized })?; } diff --git a/tests/tests/signer_jwt_auth.rs b/tests/tests/signer_jwt_auth.rs index 37561428..c7689ddb 100644 --- a/tests/tests/signer_jwt_auth.rs +++ b/tests/tests/signer_jwt_auth.rs @@ -170,3 +170,41 @@ async fn test_signer_only_admin_can_revoke() -> Result<()> { Ok(()) } + +#[tokio::test] +async fn test_signer_admin_jwt_rate_limit() -> Result<()> { + setup_test_env(); + let admin_secret = ADMIN_SECRET.to_string(); + let module_id = ModuleId(JWT_MODULE.to_string()); + let mod_cfgs = create_mod_signing_configs().await; + let start_config = start_server(20500, &mod_cfgs, admin_secret.clone(), false).await?; + + let revoke_body = RevokeModuleRequest { module_id: ModuleId(JWT_MODULE.to_string()) }; + let body_bytes = serde_json::to_vec(&revoke_body)?; + + // Run as many pubkeys requests as the fail limit + let jwt = create_jwt(&module_id, JWT_SECRET, Some(&body_bytes))?; + let client = reqwest::Client::new(); + let url = format!("http://{}{}", start_config.endpoint, REVOKE_MODULE_PATH); + + // Module JWT shouldn't be able to revoke modules + for _ in 0..start_config.jwt_auth_fail_limit { + let response = client.post(&url).json(&revoke_body).bearer_auth(&jwt).send().await?; + assert!(response.status() == StatusCode::UNAUTHORIZED); + } + + // Run another request - this should fail due to rate limiting now + let admin_jwt = create_admin_jwt(admin_secret, Some(&body_bytes))?; + let response = client.post(&url).json(&revoke_body).bearer_auth(&admin_jwt).send().await?; + assert!(response.status() == StatusCode::TOO_MANY_REQUESTS); + + // Wait for the rate limit timeout + tokio::time::sleep(Duration::from_secs(start_config.jwt_auth_fail_timeout_seconds as u64)) + .await; + + // Now the next request should succeed + let response = client.post(&url).json(&revoke_body).bearer_auth(&admin_jwt).send().await?; + assert!(response.status() == StatusCode::OK); + + Ok(()) +} From d2bb6c4f8d4902ef81e3f86b4dc54ba6842f1b58 Mon Sep 17 00:00:00 2001 From: Joe Clapis Date: Thu, 18 Sep 2025 10:40:27 -0400 Subject: [PATCH 03/13] Added retry to AWS-LC provider setup, moved into the TLS-only branch --- crates/signer/src/service.rs | 32 ++++++++++++++++++++++++++------ 1 file changed, 26 insertions(+), 6 deletions(-) diff --git a/crates/signer/src/service.rs b/crates/signer/src/service.rs index ef0c7770..207b4f63 100644 --- a/crates/signer/src/service.rs +++ b/crates/signer/src/service.rs @@ -144,13 +144,33 @@ impl SigningService { .route_layer(middleware::from_fn(log_request)) .route(STATUS_PATH, get(handle_status)); - if CryptoProvider::get_default().is_none() { - aws_lc_rs::default_provider() - .install_default() - .map_err(|_| eyre::eyre!("Failed to install TLS provider"))?; - } - let server_result = if let Some(tls_config) = config.tls_certificates { + if CryptoProvider::get_default().is_none() { + // Install the AWS-LC provider if no default is set, usually for CI + debug!("Installing AWS-LC as default TLS provider"); + let mut attempts = 0; + loop { + match aws_lc_rs::default_provider().install_default() { + Ok(_) => { + debug!("Successfully installed AWS-LC as default TLS provider"); + break; + } + Err(e) => { + error!( + "Failed to install AWS-LC as default TLS provider: {e:?}. Retrying..." + ); + if attempts >= 3 { + error!( + "Exceeded maximum attempts to install AWS-LC as default TLS provider" + ); + break; + } + attempts += 1; + } + } + } + } + let tls_config = RustlsConfig::from_pem(tls_config.0, tls_config.1).await?; axum_server::bind_rustls(config.endpoint, tls_config) .serve( From ad33d02d9777c9ee7887f7ed2a75b30e33200b72 Mon Sep 17 00:00:00 2001 From: Joe Clapis Date: Thu, 18 Sep 2025 11:36:55 -0400 Subject: [PATCH 04/13] Fixed a conflicting port --- tests/tests/signer_jwt_auth.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/tests/signer_jwt_auth.rs b/tests/tests/signer_jwt_auth.rs index c7689ddb..c4eb4598 100644 --- a/tests/tests/signer_jwt_auth.rs +++ b/tests/tests/signer_jwt_auth.rs @@ -177,7 +177,7 @@ async fn test_signer_admin_jwt_rate_limit() -> Result<()> { let admin_secret = ADMIN_SECRET.to_string(); let module_id = ModuleId(JWT_MODULE.to_string()); let mod_cfgs = create_mod_signing_configs().await; - let start_config = start_server(20500, &mod_cfgs, admin_secret.clone(), false).await?; + let start_config = start_server(20510, &mod_cfgs, admin_secret.clone(), false).await?; let revoke_body = RevokeModuleRequest { module_id: ModuleId(JWT_MODULE.to_string()) }; let body_bytes = serde_json::to_vec(&revoke_body)?; From 70b4980c1468cadc7139b47e96e78c0ba5693e85 Mon Sep 17 00:00:00 2001 From: Joe Clapis Date: Thu, 18 Sep 2025 12:52:35 -0400 Subject: [PATCH 05/13] Added true IP retrieval to the JWT auth checks --- crates/signer/src/service.rs | 29 ++++++++++++++++++++++++++--- 1 file changed, 26 insertions(+), 3 deletions(-) diff --git a/crates/signer/src/service.rs b/crates/signer/src/service.rs index 207b4f63..9154e905 100644 --- a/crates/signer/src/service.rs +++ b/crates/signer/src/service.rs @@ -10,7 +10,7 @@ use axum::{ Extension, Json, body::{Body, to_bytes}, extract::{ConnectInfo, Request, State}, - http::StatusCode, + http::{HeaderMap, StatusCode}, middleware::{self, Next}, response::{IntoResponse, Response}, routing::{get, post}, @@ -203,16 +203,38 @@ fn mark_jwt_failure(state: &SigningState, client_ip: IpAddr) { failure_info.last_failure = Instant::now(); } +fn get_true_ip(req_headers: &HeaderMap, addr: &SocketAddr) -> IpAddr { + // Try the X-Forwarded-For header first + if let Some(true_ip) = req_headers.get("x-forwarded-for") && + let Ok(true_ip) = true_ip.to_str() && + let Ok(true_ip) = true_ip.parse() + { + return true_ip; + } + + // Then try the X-Real-IP header + if let Some(true_ip) = req_headers.get("x-real-ip") && + let Ok(true_ip) = true_ip.to_str() && + let Ok(true_ip) = true_ip.parse() + { + return true_ip; + } + + // Fallback to the socket IP + addr.ip() +} + /// Authentication middleware layer async fn jwt_auth( State(state): State, + req_headers: HeaderMap, TypedHeader(auth): TypedHeader>, addr: ConnectInfo, req: Request, next: Next, ) -> Result { // Check if the request needs to be rate limited - let client_ip = addr.ip(); + let client_ip = get_true_ip(&req_headers, &addr); check_jwt_rate_limit(&state, &client_ip)?; // Clone the request so we can read the body @@ -322,13 +344,14 @@ fn check_jwt_auth( async fn admin_auth( State(state): State, + req_headers: HeaderMap, TypedHeader(auth): TypedHeader>, addr: ConnectInfo, req: Request, next: Next, ) -> Result { // Check if the request needs to be rate limited - let client_ip = addr.ip(); + let client_ip = get_true_ip(&req_headers, &addr); check_jwt_rate_limit(&state, &client_ip)?; // Clone the request so we can read the body From 79f005189406a92622ef4fcad5b90a2d049c97a4 Mon Sep 17 00:00:00 2001 From: Joe Clapis Date: Thu, 18 Sep 2025 14:35:47 -0400 Subject: [PATCH 06/13] Made handle_reload only update the state once all branches have succeeded --- crates/signer/src/service.rs | 29 +++++++++++++++-------------- 1 file changed, 15 insertions(+), 14 deletions(-) diff --git a/crates/signer/src/service.rs b/crates/signer/src/service.rs index 9154e905..227e86cb 100644 --- a/crates/signer/src/service.rs +++ b/crates/signer/src/service.rs @@ -203,6 +203,8 @@ fn mark_jwt_failure(state: &SigningState, client_ip: IpAddr) { failure_info.last_failure = Instant::now(); } +/// Get the true client IP from the request headers or fallback to the socket +/// address fn get_true_ip(req_headers: &HeaderMap, addr: &SocketAddr) -> IpAddr { // Try the X-Forwarded-For header first if let Some(true_ip) = req_headers.get("x-forwarded-for") && @@ -648,6 +650,7 @@ async fn handle_reload( debug!(event = "reload", ?req_id, "New request"); + // Regenerate the config let config = match StartSignerConfig::load_from_env() { Ok(config) => config, Err(err) => { @@ -656,6 +659,16 @@ async fn handle_reload( } }; + // Start a new manager with the updated config + let new_manager = match start_manager(config).await { + Ok(manager) => manager, + Err(err) => { + error!(event = "reload", ?req_id, error = ?err, "Failed to reload manager"); + return Err(SignerModuleError::Internal("failed to reload config".to_string())); + } + }; + + // Update the JWT configs if provided in the request if let Some(jwt_secrets) = request.jwt_secrets { let mut jwt_configs = state.jwts.write(); let mut new_configs = HashMap::new(); @@ -677,23 +690,11 @@ async fn handle_reload( *jwt_configs = new_configs; } + // Update the rest of the state once everything has passed if let Some(admin_secret) = request.admin_secret { *state.admin_secret.write() = admin_secret; } - - let new_manager = match start_manager(config).await { - Ok(manager) => manager, - Err(err) => { - error!(event = "reload", ?req_id, error = ?err, "Failed to reload manager"); - return Err(SignerModuleError::Internal("failed to reload config".to_string())); - } - }; - - // Replace the contents of the manager RwLock - { - let mut manager_guard = state.manager.write().await; - *manager_guard = new_manager; - } + *state.manager.write().await = new_manager; Ok(StatusCode::OK) } From a9e247912941e9afc3f382453eb38c9fe1f4eff9 Mon Sep 17 00:00:00 2001 From: Joe Clapis Date: Fri, 19 Sep 2025 10:00:06 -0400 Subject: [PATCH 07/13] Fixed a race between reading JWT auth fails and writing to it --- crates/signer/src/service.rs | 63 ++++++++++++++++++++---------------- 1 file changed, 35 insertions(+), 28 deletions(-) diff --git a/crates/signer/src/service.rs b/crates/signer/src/service.rs index 227e86cb..50872150 100644 --- a/crates/signer/src/service.rs +++ b/crates/signer/src/service.rs @@ -38,9 +38,8 @@ use cb_common::{ use cb_metrics::provider::MetricsProvider; use eyre::Context; use headers::{Authorization, authorization::Bearer}; -use parking_lot::RwLock as ParkingRwLock; use rustls::crypto::{CryptoProvider, aws_lc_rs}; -use tokio::sync::RwLock; +use tokio::sync::{RwLock, RwLockWriteGuard}; use tracing::{debug, error, info, warn}; use uuid::Uuid; @@ -71,13 +70,13 @@ struct SigningState { /// Map of modules ids to JWT configurations. This also acts as registry of /// all modules running - jwts: Arc>>, + jwts: Arc>>, /// Secret for the admin JWT - admin_secret: Arc>, + admin_secret: Arc>, /// Map of JWT failures per peer - jwt_auth_failures: Arc>>, + jwt_auth_failures: Arc>>, // JWT auth failure settings jwt_auth_fail_limit: u32, @@ -96,9 +95,9 @@ impl SigningService { let state = SigningState { manager: Arc::new(RwLock::new(start_manager(config.clone()).await?)), - jwts: Arc::new(ParkingRwLock::new(config.mod_signing_configs)), - admin_secret: Arc::new(ParkingRwLock::new(config.admin_secret)), - jwt_auth_failures: Arc::new(ParkingRwLock::new(HashMap::new())), + jwts: Arc::new(RwLock::new(config.mod_signing_configs)), + admin_secret: Arc::new(RwLock::new(config.admin_secret)), + jwt_auth_failures: Arc::new(RwLock::new(HashMap::new())), jwt_auth_fail_limit: config.jwt_auth_fail_limit, jwt_auth_fail_timeout: Duration::from_secs(config.jwt_auth_fail_timeout_seconds as u64), }; @@ -194,8 +193,10 @@ impl SigningService { } /// Marks a JWT authentication failure for a given client IP -fn mark_jwt_failure(state: &SigningState, client_ip: IpAddr) { - let mut failures = state.jwt_auth_failures.write(); +fn mark_jwt_failure( + client_ip: IpAddr, + failures: &mut RwLockWriteGuard>, +) { let failure_info = failures .entry(client_ip) .or_insert(JwtAuthFailureInfo { failure_count: 0, last_failure: Instant::now() }); @@ -235,9 +236,11 @@ async fn jwt_auth( req: Request, next: Next, ) -> Result { + let mut failures = state.jwt_auth_failures.write().await; + // Check if the request needs to be rate limited let client_ip = get_true_ip(&req_headers, &addr); - check_jwt_rate_limit(&state, &client_ip)?; + check_jwt_rate_limit(&state, &client_ip, &mut failures)?; // Clone the request so we can read the body let (parts, body) = req.into_parts(); @@ -247,14 +250,14 @@ async fn jwt_auth( })?; // Process JWT authorization - match check_jwt_auth(&auth, &state, &bytes) { + match check_jwt_auth(&auth, &state, &bytes).await { Ok(module_id) => { let mut req = Request::from_parts(parts, Body::from(bytes)); req.extensions_mut().insert(module_id); Ok(next.run(req).await) } Err(SignerModuleError::Unauthorized) => { - mark_jwt_failure(&state, client_ip); + mark_jwt_failure(client_ip, &mut failures); Err(SignerModuleError::Unauthorized) } Err(err) => Err(err), @@ -263,9 +266,11 @@ async fn jwt_auth( /// Checks if the incoming request needs to be rate limited due to previous JWT /// authentication failures -fn check_jwt_rate_limit(state: &SigningState, client_ip: &IpAddr) -> Result<(), SignerModuleError> { - let mut failures = state.jwt_auth_failures.write(); - +fn check_jwt_rate_limit( + state: &SigningState, + client_ip: &IpAddr, + failures: &mut RwLockWriteGuard>, +) -> Result<(), SignerModuleError> { // Ignore clients that don't have any failures if let Some(failure_info) = failures.get(client_ip) { // If the last failure was more than the timeout ago, remove this entry so it's @@ -299,7 +304,7 @@ fn check_jwt_rate_limit(state: &SigningState, client_ip: &IpAddr) -> Result<(), } /// Checks if a request can successfully authenticate with the JWT secret -fn check_jwt_auth( +async fn check_jwt_auth( auth: &Authorization, state: &SigningState, body: &[u8], @@ -313,7 +318,7 @@ fn check_jwt_auth( SignerModuleError::Unauthorized })?; - let guard = state.jwts.read(); + let guard = state.jwts.read().await; let jwt_config = guard.get(&claims.module).ok_or_else(|| { error!("Unauthorized request. Was the module started correctly?"); SignerModuleError::Unauthorized @@ -352,9 +357,11 @@ async fn admin_auth( req: Request, next: Next, ) -> Result { + let mut failures = state.jwt_auth_failures.write().await; + // Check if the request needs to be rate limited let client_ip = get_true_ip(&req_headers, &addr); - check_jwt_rate_limit(&state, &client_ip)?; + check_jwt_rate_limit(&state, &client_ip, &mut failures)?; // Clone the request so we can read the body let (parts, body) = req.into_parts(); @@ -368,15 +375,15 @@ async fn admin_auth( // Validate the admin JWT if bytes.is_empty() { // Skip payload hash comparison for requests without a body - validate_admin_jwt(jwt, &state.admin_secret.read(), None).map_err(|e| { + validate_admin_jwt(jwt, &state.admin_secret.read().await, None).map_err(|e| { error!("Unauthorized request. Invalid JWT: {e}"); - mark_jwt_failure(&state, client_ip); + mark_jwt_failure(client_ip, &mut failures); SignerModuleError::Unauthorized })?; } else { - validate_admin_jwt(jwt, &state.admin_secret.read(), Some(&bytes)).map_err(|e| { + validate_admin_jwt(jwt, &state.admin_secret.read().await, Some(&bytes)).map_err(|e| { error!("Unauthorized request. Invalid payload hash in JWT claims: {e}"); - mark_jwt_failure(&state, client_ip); + mark_jwt_failure(client_ip, &mut failures); SignerModuleError::Unauthorized })?; } @@ -470,7 +477,7 @@ async fn handle_request_signature_bls_impl( object_root: B256, nonce: u64, ) -> Result { - let Some(signing_id) = state.jwts.read().get(&module_id).map(|m| m.signing_id) else { + let Some(signing_id) = state.jwts.read().await.get(&module_id).map(|m| m.signing_id) else { error!( event = "proxy_bls_request_signature", ?module_id, @@ -548,7 +555,7 @@ async fn handle_request_signature_proxy_ecdsa( Json(request): Json>, ) -> Result { let req_id = Uuid::new_v4(); - let Some(signing_id) = state.jwts.read().get(&module_id).map(|m| m.signing_id) else { + let Some(signing_id) = state.jwts.read().await.get(&module_id).map(|m| m.signing_id) else { error!( event = "proxy_ecdsa_request_signature", ?module_id, @@ -670,7 +677,7 @@ async fn handle_reload( // Update the JWT configs if provided in the request if let Some(jwt_secrets) = request.jwt_secrets { - let mut jwt_configs = state.jwts.write(); + let mut jwt_configs = state.jwts.write().await; let mut new_configs = HashMap::new(); for (module_id, jwt_secret) in jwt_secrets { if let Some(signing_id) = jwt_configs.get(&module_id).map(|cfg| cfg.signing_id) { @@ -692,7 +699,7 @@ async fn handle_reload( // Update the rest of the state once everything has passed if let Some(admin_secret) = request.admin_secret { - *state.admin_secret.write() = admin_secret; + *state.admin_secret.write().await = admin_secret; } *state.manager.write().await = new_manager; @@ -703,7 +710,7 @@ async fn handle_revoke_module( State(state): State, Json(request): Json, ) -> Result { - let mut guard = state.jwts.write(); + let mut guard = state.jwts.write().await; guard .remove(&request.module_id) .ok_or(SignerModuleError::ModuleIdNotFound) From afb6a5002b1318060bcbdf441573373cd5db5956 Mon Sep 17 00:00:00 2001 From: Joe Clapis Date: Fri, 19 Sep 2025 13:00:02 -0400 Subject: [PATCH 08/13] Added path / route to JWT claims --- api/signer-api.yml | 5 +++ crates/common/src/commit/client.rs | 36 ++++----------- crates/common/src/types.rs | 2 + crates/common/src/utils.rs | 71 +++++++++++++++++++++++------- crates/signer/src/service.rs | 33 +++++++------- tests/tests/signer_jwt_auth.rs | 20 ++++----- tests/tests/signer_request_sig.rs | 23 ++++++++-- tests/tests/signer_tls.rs | 2 +- 8 files changed, 118 insertions(+), 74 deletions(-) diff --git a/api/signer-api.yml b/api/signer-api.yml index 9e11da34..95897ecd 100644 --- a/api/signer-api.yml +++ b/api/signer-api.yml @@ -15,6 +15,7 @@ paths: The token **must include** the following claims: - `exp` (integer): Expiration timestamp + - `route` (string): The route being requested (must be `/signer/v1/get_pubkeys` for this endpoint). - `module` (string): The ID of the module making the request, which must match a module ID in the Commit-Boost configuration file. tags: - Signer @@ -73,6 +74,7 @@ paths: The token **must include** the following claims: - `exp` (integer): Expiration timestamp - `module` (string): The ID of the module making the request, which must match a module ID in the Commit-Boost configuration file. + - `route` (string): The route being requested (must be `/signer/v1/request_signature/bls` for this endpoint). - `payload_hash` (string): The Keccak-256 hash of the JSON-encoded request body, with optional `0x` prefix. This is required to prevent JWT replay attacks. tags: - Signer @@ -220,6 +222,7 @@ paths: The token **must include** the following claims: - `exp` (integer): Expiration timestamp - `module` (string): The ID of the module making the request, which must match a module ID in the Commit-Boost configuration file. + - `route` (string): The route being requested (must be `/signer/v1/request_signature/proxy-bls` for this endpoint). - `payload_hash` (string): The Keccak-256 hash of the JSON-encoded request body, with optional `0x` prefix. This is required to prevent JWT replay attacks. tags: - Signer @@ -367,6 +370,7 @@ paths: The token **must include** the following claims: - `exp` (integer): Expiration timestamp - `module` (string): The ID of the module making the request, which must match a module ID in the Commit-Boost configuration file. + - `route` (string): The route being requested (must be `/signer/v1/request_signature/proxy-ecdsa` for this endpoint). - `payload_hash` (string): The Keccak-256 hash of the JSON-encoded request body, with optional `0x` prefix. This is required to prevent JWT replay attacks. tags: - Signer @@ -514,6 +518,7 @@ paths: The token **must include** the following claims: - `exp` (integer): Expiration timestamp - `module` (string): The ID of the module making the request, which must match a module ID in the Commit-Boost configuration file. + - `route` (string): The route being requested (must be `/signer/v1/generate_proxy_key` for this endpoint). - `payload_hash` (string): The Keccak-256 hash of the JSON-encoded request body, with optional `0x` prefix. This is required to prevent JWT replay attacks. tags: - Signer diff --git a/crates/common/src/commit/client.rs b/crates/common/src/commit/client.rs index 1151eb6f..98d8c26d 100644 --- a/crates/common/src/commit/client.rs +++ b/crates/common/src/commit/client.rs @@ -2,10 +2,7 @@ use std::path::PathBuf; use alloy::primitives::Address; use eyre::WrapErr; -use reqwest::{ - Certificate, - header::{AUTHORIZATION, HeaderMap, HeaderValue}, -}; +use reqwest::Certificate; use serde::{Deserialize, Serialize}; use url::Url; @@ -60,30 +57,13 @@ impl SignerClient { Ok(Self { url: signer_server_url, client: builder.build()?, module_id, jwt_secret }) } - fn refresh_jwt(&mut self) -> Result<(), SignerClientError> { - let jwt = create_jwt(&self.module_id, &self.jwt_secret, None)?; - - let mut auth_value = - HeaderValue::from_str(&format!("Bearer {jwt}")).wrap_err("invalid jwt")?; - auth_value.set_sensitive(true); - - let mut headers = HeaderMap::new(); - headers.insert(AUTHORIZATION, auth_value); - - self.client = reqwest::Client::builder() - .timeout(DEFAULT_REQUEST_TIMEOUT) - .default_headers(headers) - .build()?; - - Ok(()) - } - fn create_jwt_for_payload( &mut self, + route: &str, payload: &T, ) -> Result { let payload_vec = serde_json::to_vec(payload)?; - create_jwt(&self.module_id, &self.jwt_secret, Some(&payload_vec)) + create_jwt(&self.module_id, &self.jwt_secret, route, Some(&payload_vec)) .wrap_err("failed to create JWT for payload") .map_err(SignerClientError::JWTError) } @@ -92,10 +72,12 @@ impl SignerClient { /// requested. // TODO: add more docs on how proxy keys work pub async fn get_pubkeys(&mut self) -> Result { - self.refresh_jwt()?; + let jwt = create_jwt(&self.module_id, &self.jwt_secret, GET_PUBKEYS_PATH, None) + .wrap_err("failed to create JWT for payload") + .map_err(SignerClientError::JWTError)?; let url = self.url.join(GET_PUBKEYS_PATH)?; - let res = self.client.get(url).send().await?; + let res = self.client.get(url).bearer_auth(jwt).send().await?; if !res.status().is_success() { return Err(SignerClientError::FailedRequest { @@ -117,7 +99,7 @@ impl SignerClient { Q: Serialize, T: for<'de> Deserialize<'de>, { - let jwt = self.create_jwt_for_payload(request)?; + let jwt = self.create_jwt_for_payload(route, request)?; let url = self.url.join(route)?; let res = self.client.post(url).json(&request).bearer_auth(jwt).send().await?; @@ -165,7 +147,7 @@ impl SignerClient { where T: ProxyId + for<'de> Deserialize<'de>, { - let jwt = self.create_jwt_for_payload(request)?; + let jwt = self.create_jwt_for_payload(GENERATE_PROXY_KEY_PATH, request)?; let url = self.url.join(GENERATE_PROXY_KEY_PATH)?; let res = self.client.post(url).json(&request).bearer_auth(jwt).send().await?; diff --git a/crates/common/src/types.rs b/crates/common/src/types.rs index 13c6b501..9fa3b40b 100644 --- a/crates/common/src/types.rs +++ b/crates/common/src/types.rs @@ -26,6 +26,7 @@ pub struct Jwt(pub String); pub struct JwtClaims { pub exp: u64, pub module: ModuleId, + pub route: String, pub payload_hash: Option, } @@ -33,6 +34,7 @@ pub struct JwtClaims { pub struct JwtAdminClaims { pub exp: u64, pub admin: bool, + pub route: String, pub payload_hash: Option, } diff --git a/crates/common/src/utils.rs b/crates/common/src/utils.rs index 91c3b11a..bb26edb5 100644 --- a/crates/common/src/utils.rs +++ b/crates/common/src/utils.rs @@ -346,11 +346,17 @@ pub fn print_logo() { } /// Create a JWT for the given module id with expiration -pub fn create_jwt(module_id: &ModuleId, secret: &str, payload: Option<&[u8]>) -> eyre::Result { +pub fn create_jwt( + module_id: &ModuleId, + secret: &str, + route: &str, + payload: Option<&[u8]>, +) -> eyre::Result { jsonwebtoken::encode( &jsonwebtoken::Header::default(), &JwtClaims { module: module_id.clone(), + route: route.to_string(), exp: jsonwebtoken::get_current_timestamp() + SIGNER_JWT_EXPIRATION, payload_hash: payload.map(keccak256), }, @@ -361,11 +367,16 @@ pub fn create_jwt(module_id: &ModuleId, secret: &str, payload: Option<&[u8]>) -> } // Creates a JWT for module administration -pub fn create_admin_jwt(admin_secret: String, payload: Option<&[u8]>) -> eyre::Result { +pub fn create_admin_jwt( + admin_secret: String, + route: &str, + payload: Option<&[u8]>, +) -> eyre::Result { jsonwebtoken::encode( &jsonwebtoken::Header::default(), &JwtAdminClaims { admin: true, + route: route.to_string(), exp: jsonwebtoken::get_current_timestamp() + SIGNER_JWT_EXPIRATION, payload_hash: payload.map(keccak256), }, @@ -408,7 +419,12 @@ pub fn decode_admin_jwt(jwt: Jwt) -> eyre::Result { } /// Validate a JWT with the given secret -pub fn validate_jwt(jwt: Jwt, secret: &str, payload: Option<&[u8]>) -> eyre::Result<()> { +pub fn validate_jwt( + jwt: Jwt, + secret: &str, + route: &str, + payload: Option<&[u8]>, +) -> eyre::Result<()> { let mut validation = jsonwebtoken::Validation::default(); validation.leeway = 10; @@ -419,6 +435,11 @@ pub fn validate_jwt(jwt: Jwt, secret: &str, payload: Option<&[u8]>) -> eyre::Res )? .claims; + // Validate the route + if claims.route != route { + eyre::bail!("Token route does not match"); + } + // Validate the payload hash if provided if let Some(payload_bytes) = payload { if let Some(expected_hash) = claims.payload_hash { @@ -436,7 +457,12 @@ pub fn validate_jwt(jwt: Jwt, secret: &str, payload: Option<&[u8]>) -> eyre::Res } /// Validate an admin JWT with the given secret -pub fn validate_admin_jwt(jwt: Jwt, secret: &str, payload: Option<&[u8]>) -> eyre::Result<()> { +pub fn validate_admin_jwt( + jwt: Jwt, + secret: &str, + route: &str, + payload: Option<&[u8]>, +) -> eyre::Result<()> { let mut validation = jsonwebtoken::Validation::default(); validation.leeway = 10; @@ -451,6 +477,11 @@ pub fn validate_admin_jwt(jwt: Jwt, secret: &str, payload: Option<&[u8]>) -> eyr eyre::bail!("Token is not admin") } + // Validate the route + if claims.route != route { + eyre::bail!("Token route does not match"); + } + // Validate the payload hash if provided if let Some(payload_bytes) = payload { if let Some(expected_hash) = claims.payload_hash { @@ -546,24 +577,25 @@ mod test { #[test] fn test_jwt_validation_no_payload_hash() { // Check valid JWT - let jwt = create_jwt(&ModuleId("DA_COMMIT".to_string()), "secret", None).unwrap(); + let jwt = + create_jwt(&ModuleId("DA_COMMIT".to_string()), "secret", "/test/route", None).unwrap(); let claims = decode_jwt(jwt.clone()).unwrap(); let module_id = claims.module; let payload_hash = claims.payload_hash; assert_eq!(module_id, ModuleId("DA_COMMIT".to_string())); assert!(payload_hash.is_none()); - let response = validate_jwt(jwt, "secret", None); + let response = validate_jwt(jwt, "secret", "/test/route", None); assert!(response.is_ok()); // Check expired JWT - let expired_jwt = Jwt::from("eyJ0eXAiOiJKV1QiLCJhbGciOiJIUzI1NiJ9.eyJleHAiOjE3NDI5OTU5NDYsIm1vZHVsZSI6IkRBX0NPTU1JVCJ9.iiq4Z2ed2hk3c3c-cn2QOQJWE5XUOc5BoaIPT-I8q-s".to_string()); - let response = validate_jwt(expired_jwt, "secret", None); + let expired_jwt = Jwt::from("eyJ0eXAiOiJKV1QiLCJhbGciOiJIUzI1NiJ9.eyJleHAiOjE3NTgyOTkxNzIsIm1vZHVsZSI6IkRBX0NPTU1JVCIsInJvdXRlIjoiL3Rlc3Qvcm91dGUiLCJwYXlsb2FkX2hhc2giOm51bGx9._OBsNC67KLkk6f6ZQ2_CDbhYUJ2OtZ9egKAmi1L-ymA".to_string()); + let response = validate_jwt(expired_jwt, "secret", "/test/route", None); assert!(response.is_err()); assert_eq!(response.unwrap_err().to_string(), "ExpiredSignature"); // Check invalid signature JWT - let invalid_jwt = Jwt::from("eyJ0eXAiOiJKV1QiLCJhbGciOiJIUzI1NiJ9.eyJleHAiOjE3NDI5OTU5NDYsIm1vZHVsZSI6IkRBX0NPTU1JVCJ9.w9WYdDNzgDjYTvjBkk4GGzywGNBYPxnzU2uJWzPUT1s".to_string()); - let response = validate_jwt(invalid_jwt, "secret", None); + let invalid_jwt = Jwt::from("eyJ0eXAiOiJKV1QiLCJhbGciOiJIUzI1NiJ9.eyJleHAiOjE3NTgyOTkxMzQsIm1vZHVsZSI6IkRBX0NPTU1JVCIsInJvdXRlIjoiL3Rlc3Qvcm91dGUiLCJwYXlsb2FkX2hhc2giOm51bGx9.58QXayg2XeX5lXhIPw-a8kl04DWBEj5wBsqsedTeClo".to_string()); + let response = validate_jwt(invalid_jwt, "secret", "/test/route", None); assert!(response.is_err()); assert_eq!(response.unwrap_err().to_string(), "InvalidSignature"); } @@ -577,25 +609,30 @@ mod test { let payload_bytes = serde_json::to_vec(&payload).unwrap(); // Check valid JWT - let jwt = - create_jwt(&ModuleId("DA_COMMIT".to_string()), "secret", Some(&payload_bytes)).unwrap(); + let jwt = create_jwt( + &ModuleId("DA_COMMIT".to_string()), + "secret", + "/test/route", + Some(&payload_bytes), + ) + .unwrap(); let claims = decode_jwt(jwt.clone()).unwrap(); let module_id = claims.module; let payload_hash = claims.payload_hash; assert_eq!(module_id, ModuleId("DA_COMMIT".to_string())); assert_eq!(payload_hash, Some(keccak256(&payload_bytes))); - let response = validate_jwt(jwt, "secret", Some(&payload_bytes)); + let response = validate_jwt(jwt, "secret", "/test/route", Some(&payload_bytes)); assert!(response.is_ok()); // Check expired JWT - let expired_jwt = Jwt::from("eyJ0eXAiOiJKV1QiLCJhbGciOiJIUzI1NiJ9.eyJleHAiOjE3NDI5OTU5NDYsIm1vZHVsZSI6IkRBX0NPTU1JVCJ9.iiq4Z2ed2hk3c3c-cn2QOQJWE5XUOc5BoaIPT-I8q-s".to_string()); - let response = validate_jwt(expired_jwt, "secret", Some(&payload_bytes)); + let expired_jwt = Jwt::from("eyJ0eXAiOiJKV1QiLCJhbGciOiJIUzI1NiJ9.eyJleHAiOjE3NTgyOTgzNDQsIm1vZHVsZSI6IkRBX0NPTU1JVCIsInJvdXRlIjoiL3Rlc3Qvcm91dGUiLCJwYXlsb2FkX2hhc2giOiIweGFmODk2MjY0MzUzNTFmYzIwMDBkYmEwM2JiNTlhYjcyZWE0ODJiOWEwMDBmZWQzNmNkMjBlMDU0YjE2NjZmZjEifQ.PYrSxLXadKBgYZlmLam8RBSL32I1T_zAxlZpG6xnnII".to_string()); + let response = validate_jwt(expired_jwt, "secret", "/test/route", Some(&payload_bytes)); assert!(response.is_err()); assert_eq!(response.unwrap_err().to_string(), "ExpiredSignature"); // Check invalid signature JWT - let invalid_jwt = Jwt::from("eyJ0eXAiOiJKV1QiLCJhbGciOiJIUzI1NiJ9.eyJleHAiOjE3NDI5OTU5NDYsIm1vZHVsZSI6IkRBX0NPTU1JVCJ9.w9WYdDNzgDjYTvjBkk4GGzywGNBYPxnzU2uJWzPUT1s".to_string()); - let response = validate_jwt(invalid_jwt, "secret", Some(&payload_bytes)); + let invalid_jwt = Jwt::from("eyJ0eXAiOiJKV1QiLCJhbGciOiJIUzI1NiJ9.eyJleHAiOjE3NTgyOTkwMDAsIm1vZHVsZSI6IkRBX0NPTU1JVCIsInJvdXRlIjoiL3Rlc3Qvcm91dGUiLCJwYXlsb2FkX2hhc2giOiIweGFmODk2MjY0MzUzNTFmYzIwMDBkYmEwM2JiNTlhYjcyZWE0ODJiOWEwMDBmZWQzNmNkMjBlMDU0YjE2NjZmZjEifQ.mnC-AexkLlR9l98SJbln3DmV6r9XyHYdbjcUVcWdi_8".to_string()); + let response = validate_jwt(invalid_jwt, "secret", "/test/route", Some(&payload_bytes)); assert!(response.is_err()); assert_eq!(response.unwrap_err().to_string(), "InvalidSignature"); } diff --git a/crates/signer/src/service.rs b/crates/signer/src/service.rs index 50872150..49b56f3b 100644 --- a/crates/signer/src/service.rs +++ b/crates/signer/src/service.rs @@ -244,13 +244,14 @@ async fn jwt_auth( // Clone the request so we can read the body let (parts, body) = req.into_parts(); + let path = parts.uri.path(); let bytes = to_bytes(body, REQUEST_MAX_BODY_LENGTH).await.map_err(|e| { error!("Failed to read request body: {e}"); SignerModuleError::RequestError(e.to_string()) })?; // Process JWT authorization - match check_jwt_auth(&auth, &state, &bytes).await { + match check_jwt_auth(&auth, &state, path, &bytes).await { Ok(module_id) => { let mut req = Request::from_parts(parts, Body::from(bytes)); req.extensions_mut().insert(module_id); @@ -307,6 +308,7 @@ fn check_jwt_rate_limit( async fn check_jwt_auth( auth: &Authorization, state: &SigningState, + path: &str, body: &[u8], ) -> Result { let jwt: Jwt = auth.token().to_string().into(); @@ -326,23 +328,21 @@ async fn check_jwt_auth( if body.is_empty() { // Skip payload hash comparison for requests without a body - validate_jwt(jwt, &jwt_config.jwt_secret, None).map_err(|e| { + validate_jwt(jwt, &jwt_config.jwt_secret, path, None).map_err(|e| { error!("Unauthorized request. Invalid JWT: {e}"); SignerModuleError::Unauthorized })?; } else { - validate_jwt(jwt, &jwt_config.jwt_secret, Some(body)).map_err(|e| { + validate_jwt(jwt, &jwt_config.jwt_secret, path, Some(body)).map_err(|e| { error!("Unauthorized request. Invalid JWT: {e}"); SignerModuleError::Unauthorized })?; // Make sure the request contains a hash of the payload in its claims - if !body.is_empty() { - let payload_hash = keccak256(body); - if claims.payload_hash.is_none() || claims.payload_hash != Some(payload_hash) { - error!("Unauthorized request. Invalid payload hash in JWT claims"); - return Err(SignerModuleError::Unauthorized); - } + let payload_hash = keccak256(body); + if claims.payload_hash.is_none() || claims.payload_hash != Some(payload_hash) { + error!("Unauthorized request. Invalid payload hash in JWT claims"); + return Err(SignerModuleError::Unauthorized); } } @@ -365,6 +365,7 @@ async fn admin_auth( // Clone the request so we can read the body let (parts, body) = req.into_parts(); + let path = parts.uri.path(); let bytes = to_bytes(body, REQUEST_MAX_BODY_LENGTH).await.map_err(|e| { error!("Failed to read request body: {e}"); SignerModuleError::RequestError(e.to_string()) @@ -375,17 +376,19 @@ async fn admin_auth( // Validate the admin JWT if bytes.is_empty() { // Skip payload hash comparison for requests without a body - validate_admin_jwt(jwt, &state.admin_secret.read().await, None).map_err(|e| { + validate_admin_jwt(jwt, &state.admin_secret.read().await, path, None).map_err(|e| { error!("Unauthorized request. Invalid JWT: {e}"); mark_jwt_failure(client_ip, &mut failures); SignerModuleError::Unauthorized })?; } else { - validate_admin_jwt(jwt, &state.admin_secret.read().await, Some(&bytes)).map_err(|e| { - error!("Unauthorized request. Invalid payload hash in JWT claims: {e}"); - mark_jwt_failure(client_ip, &mut failures); - SignerModuleError::Unauthorized - })?; + validate_admin_jwt(jwt, &state.admin_secret.read().await, path, Some(&bytes)).map_err( + |e| { + error!("Unauthorized request. Invalid payload hash in JWT claims: {e}"); + mark_jwt_failure(client_ip, &mut failures); + SignerModuleError::Unauthorized + }, + )?; } let req = Request::from_parts(parts, Body::from(bytes)); diff --git a/tests/tests/signer_jwt_auth.rs b/tests/tests/signer_jwt_auth.rs index c4eb4598..d1b65b3f 100644 --- a/tests/tests/signer_jwt_auth.rs +++ b/tests/tests/signer_jwt_auth.rs @@ -45,7 +45,7 @@ async fn test_signer_jwt_auth_success() -> Result<()> { let jwt_config = mod_cfgs.get(&module_id).expect("JWT config for test module not found"); // Run a pubkeys request - let jwt = create_jwt(&module_id, &jwt_config.jwt_secret, None)?; + let jwt = create_jwt(&module_id, &jwt_config.jwt_secret, GET_PUBKEYS_PATH, None)?; let client = reqwest::Client::new(); let url = format!("http://{}{}", start_config.endpoint, GET_PUBKEYS_PATH); let response = client.get(&url).bearer_auth(&jwt).send().await?; @@ -64,7 +64,7 @@ async fn test_signer_jwt_auth_fail() -> Result<()> { let start_config = start_server(20101, &mod_cfgs, ADMIN_SECRET.to_string(), false).await?; // Run a pubkeys request - this should fail due to invalid JWT - let jwt = create_jwt(&module_id, "incorrect secret", None)?; + let jwt = create_jwt(&module_id, "incorrect secret", GET_PUBKEYS_PATH, None)?; let client = reqwest::Client::new(); let url = format!("http://{}{}", start_config.endpoint, GET_PUBKEYS_PATH); let response = client.get(&url).bearer_auth(&jwt).send().await?; @@ -86,7 +86,7 @@ async fn test_signer_jwt_rate_limit() -> Result<()> { let mod_cfg = mod_cfgs.get(&module_id).expect("JWT config for test module not found"); // Run as many pubkeys requests as the fail limit - let jwt = create_jwt(&module_id, "incorrect secret", None)?; + let jwt = create_jwt(&module_id, "incorrect secret", GET_PUBKEYS_PATH, None)?; let client = reqwest::Client::new(); let url = format!("http://{}{}", start_config.endpoint, GET_PUBKEYS_PATH); for _ in 0..start_config.jwt_auth_fail_limit { @@ -95,7 +95,7 @@ async fn test_signer_jwt_rate_limit() -> Result<()> { } // Run another request - this should fail due to rate limiting now - let jwt = create_jwt(&module_id, &mod_cfg.jwt_secret, None)?; + let jwt = create_jwt(&module_id, &mod_cfg.jwt_secret, GET_PUBKEYS_PATH, None)?; let response = client.get(&url).bearer_auth(&jwt).send().await?; assert!(response.status() == StatusCode::TOO_MANY_REQUESTS); @@ -119,7 +119,7 @@ async fn test_signer_revoked_jwt_fail() -> Result<()> { let start_config = start_server(20400, &mod_cfgs, admin_secret.clone(), false).await?; // Run as many pubkeys requests as the fail limit - let jwt = create_jwt(&module_id, JWT_SECRET, None)?; + let jwt = create_jwt(&module_id, JWT_SECRET, GET_PUBKEYS_PATH, None)?; let client = reqwest::Client::new(); // At first, test module should be allowed to request pubkeys @@ -129,7 +129,7 @@ async fn test_signer_revoked_jwt_fail() -> Result<()> { let revoke_body = RevokeModuleRequest { module_id: ModuleId(JWT_MODULE.to_string()) }; let body_bytes = serde_json::to_vec(&revoke_body)?; - let admin_jwt = create_admin_jwt(admin_secret, Some(&body_bytes))?; + let admin_jwt = create_admin_jwt(admin_secret, REVOKE_MODULE_PATH, Some(&body_bytes))?; let revoke_url = format!("http://{}{}", start_config.endpoint, REVOKE_MODULE_PATH); let response = @@ -155,7 +155,7 @@ async fn test_signer_only_admin_can_revoke() -> Result<()> { let body_bytes = serde_json::to_vec(&revoke_body)?; // Run as many pubkeys requests as the fail limit - let jwt = create_jwt(&module_id, JWT_SECRET, Some(&body_bytes))?; + let jwt = create_jwt(&module_id, JWT_SECRET, REVOKE_MODULE_PATH, Some(&body_bytes))?; let client = reqwest::Client::new(); let url = format!("http://{}{}", start_config.endpoint, REVOKE_MODULE_PATH); @@ -164,7 +164,7 @@ async fn test_signer_only_admin_can_revoke() -> Result<()> { assert!(response.status() == StatusCode::UNAUTHORIZED); // Admin should be able to revoke modules - let admin_jwt = create_admin_jwt(admin_secret, Some(&body_bytes))?; + let admin_jwt = create_admin_jwt(admin_secret, REVOKE_MODULE_PATH, Some(&body_bytes))?; let response = client.post(&url).json(&revoke_body).bearer_auth(&admin_jwt).send().await?; assert!(response.status() == StatusCode::OK); @@ -183,7 +183,7 @@ async fn test_signer_admin_jwt_rate_limit() -> Result<()> { let body_bytes = serde_json::to_vec(&revoke_body)?; // Run as many pubkeys requests as the fail limit - let jwt = create_jwt(&module_id, JWT_SECRET, Some(&body_bytes))?; + let jwt = create_jwt(&module_id, JWT_SECRET, REVOKE_MODULE_PATH, Some(&body_bytes))?; let client = reqwest::Client::new(); let url = format!("http://{}{}", start_config.endpoint, REVOKE_MODULE_PATH); @@ -194,7 +194,7 @@ async fn test_signer_admin_jwt_rate_limit() -> Result<()> { } // Run another request - this should fail due to rate limiting now - let admin_jwt = create_admin_jwt(admin_secret, Some(&body_bytes))?; + let admin_jwt = create_admin_jwt(admin_secret, REVOKE_MODULE_PATH, Some(&body_bytes))?; let response = client.post(&url).json(&revoke_body).bearer_auth(&admin_jwt).send().await?; assert!(response.status() == StatusCode::TOO_MANY_REQUESTS); diff --git a/tests/tests/signer_request_sig.rs b/tests/tests/signer_request_sig.rs index 15680587..78efbf9e 100644 --- a/tests/tests/signer_request_sig.rs +++ b/tests/tests/signer_request_sig.rs @@ -62,7 +62,12 @@ async fn test_signer_sign_request_good() -> Result<()> { let pubkey = BlsPublicKey::deserialize(&PUBKEY_1).unwrap(); let request = SignConsensusRequest { pubkey: pubkey.clone(), object_root, nonce }; let payload_bytes = serde_json::to_vec(&request)?; - let jwt = create_jwt(&module_id, &jwt_config.jwt_secret, Some(&payload_bytes))?; + let jwt = create_jwt( + &module_id, + &jwt_config.jwt_secret, + REQUEST_SIGNATURE_BLS_PATH, + Some(&payload_bytes), + )?; let client = reqwest::Client::new(); let url = format!("http://{}{}", start_config.endpoint, REQUEST_SIGNATURE_BLS_PATH); let response = client.post(&url).json(&request).bearer_auth(&jwt).send().await?; @@ -100,7 +105,12 @@ async fn test_signer_sign_request_different_module() -> Result<()> { let pubkey = BlsPublicKey::deserialize(&PUBKEY_1).unwrap(); let request = SignConsensusRequest { pubkey: pubkey.clone(), object_root, nonce }; let payload_bytes = serde_json::to_vec(&request)?; - let jwt = create_jwt(&module_id, &jwt_config.jwt_secret, Some(&payload_bytes))?; + let jwt = create_jwt( + &module_id, + &jwt_config.jwt_secret, + REQUEST_SIGNATURE_BLS_PATH, + Some(&payload_bytes), + )?; let client = reqwest::Client::new(); let url = format!("http://{}{}", start_config.endpoint, REQUEST_SIGNATURE_BLS_PATH); let response = client.post(&url).json(&request).bearer_auth(&jwt).send().await?; @@ -146,7 +156,12 @@ async fn test_signer_sign_request_incorrect_hash() -> Result<()> { let true_object_root = b256!("0x0123456789012345678901234567890123456789012345678901234567890123"); let true_request = SignConsensusRequest { pubkey, object_root: true_object_root, nonce }; - let jwt = create_jwt(&module_id, &jwt_config.jwt_secret, Some(&fake_payload_bytes))?; + let jwt = create_jwt( + &module_id, + &jwt_config.jwt_secret, + REQUEST_SIGNATURE_BLS_PATH, + Some(&fake_payload_bytes), + )?; let client = reqwest::Client::new(); let url = format!("http://{}{}", start_config.endpoint, REQUEST_SIGNATURE_BLS_PATH); let response = client.post(&url).json(&true_request).bearer_auth(&jwt).send().await?; @@ -171,7 +186,7 @@ async fn test_signer_sign_request_missing_hash() -> Result<()> { let pubkey = BlsPublicKey::deserialize(&PUBKEY_1).unwrap(); let object_root = b256!("0x0123456789012345678901234567890123456789012345678901234567890123"); let request = SignConsensusRequest { pubkey, object_root, nonce }; - let jwt = create_jwt(&module_id, &jwt_config.jwt_secret, None)?; + let jwt = create_jwt(&module_id, &jwt_config.jwt_secret, REQUEST_SIGNATURE_BLS_PATH, None)?; let client = reqwest::Client::new(); let url = format!("http://{}{}", start_config.endpoint, REQUEST_SIGNATURE_BLS_PATH); let response = client.post(&url).json(&request).bearer_auth(&jwt).send().await?; diff --git a/tests/tests/signer_tls.rs b/tests/tests/signer_tls.rs index 4f53bb92..2df98d73 100644 --- a/tests/tests/signer_tls.rs +++ b/tests/tests/signer_tls.rs @@ -41,7 +41,7 @@ async fn test_signer_tls() -> Result<()> { let jwt_config = mod_cfgs.get(&module_id).expect("JWT config for test module not found"); // Run a pubkeys request - let jwt = create_jwt(&module_id, &jwt_config.jwt_secret, None)?; + let jwt = create_jwt(&module_id, &jwt_config.jwt_secret, GET_PUBKEYS_PATH, None)?; let cert = match start_config.tls_certificates { Some(ref certificates) => &certificates.0, None => bail!("TLS certificates not found in start config"), From 9392c590205b3b1eebc9f24eb479f5e8e38f955e Mon Sep 17 00:00:00 2001 From: Joe Clapis Date: Fri, 19 Sep 2025 13:06:21 -0400 Subject: [PATCH 09/13] Cleaned up some duplicate validation --- crates/signer/src/service.rs | 48 +++++++++--------------------------- 1 file changed, 12 insertions(+), 36 deletions(-) diff --git a/crates/signer/src/service.rs b/crates/signer/src/service.rs index 49b56f3b..e59ca920 100644 --- a/crates/signer/src/service.rs +++ b/crates/signer/src/service.rs @@ -5,7 +5,7 @@ use std::{ time::{Duration, Instant}, }; -use alloy::primitives::{Address, B256, U256, keccak256}; +use alloy::primitives::{Address, B256, U256}; use axum::{ Extension, Json, body::{Body, to_bytes}, @@ -326,25 +326,11 @@ async fn check_jwt_auth( SignerModuleError::Unauthorized })?; - if body.is_empty() { - // Skip payload hash comparison for requests without a body - validate_jwt(jwt, &jwt_config.jwt_secret, path, None).map_err(|e| { - error!("Unauthorized request. Invalid JWT: {e}"); - SignerModuleError::Unauthorized - })?; - } else { - validate_jwt(jwt, &jwt_config.jwt_secret, path, Some(body)).map_err(|e| { - error!("Unauthorized request. Invalid JWT: {e}"); - SignerModuleError::Unauthorized - })?; - - // Make sure the request contains a hash of the payload in its claims - let payload_hash = keccak256(body); - if claims.payload_hash.is_none() || claims.payload_hash != Some(payload_hash) { - error!("Unauthorized request. Invalid payload hash in JWT claims"); - return Err(SignerModuleError::Unauthorized); - } - } + let body_bytes = if body.is_empty() { None } else { Some(body) }; + validate_jwt(jwt, &jwt_config.jwt_secret, path, body_bytes).map_err(|e| { + error!("Unauthorized request. Invalid JWT: {e}"); + SignerModuleError::Unauthorized + })?; Ok(claims.module) } @@ -374,22 +360,12 @@ async fn admin_auth( let jwt: Jwt = auth.token().to_string().into(); // Validate the admin JWT - if bytes.is_empty() { - // Skip payload hash comparison for requests without a body - validate_admin_jwt(jwt, &state.admin_secret.read().await, path, None).map_err(|e| { - error!("Unauthorized request. Invalid JWT: {e}"); - mark_jwt_failure(client_ip, &mut failures); - SignerModuleError::Unauthorized - })?; - } else { - validate_admin_jwt(jwt, &state.admin_secret.read().await, path, Some(&bytes)).map_err( - |e| { - error!("Unauthorized request. Invalid payload hash in JWT claims: {e}"); - mark_jwt_failure(client_ip, &mut failures); - SignerModuleError::Unauthorized - }, - )?; - } + let body_bytes: Option<&[u8]> = if bytes.is_empty() { None } else { Some(&bytes) }; + validate_admin_jwt(jwt, &state.admin_secret.read().await, path, body_bytes).map_err(|e| { + error!("Unauthorized request. Invalid JWT: {e}"); + mark_jwt_failure(client_ip, &mut failures); + SignerModuleError::Unauthorized + })?; let req = Request::from_parts(parts, Body::from(bytes)); Ok(next.run(req).await) From 2a9bdf95231c9e37f97e50983d7f4f99673e3420 Mon Sep 17 00:00:00 2001 From: Joe Clapis Date: Tue, 30 Sep 2025 14:17:50 -0400 Subject: [PATCH 10/13] Add a task to clean up expired JWT failure rate limits (#380) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Manuel Iñaki Bilbao --- Cargo.lock | 22 ++++++++ Cargo.toml | 1 + config.example.toml | 3 +- crates/common/src/config/signer.rs | 3 +- crates/signer/src/service.rs | 20 ++++++++ tests/Cargo.toml | 3 ++ tests/tests/signer_jwt_auth_cleanup.rs | 70 ++++++++++++++++++++++++++ 7 files changed, 120 insertions(+), 2 deletions(-) create mode 100644 tests/tests/signer_jwt_auth_cleanup.rs diff --git a/Cargo.lock b/Cargo.lock index 0dd0e763..e06d7ff0 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1716,6 +1716,7 @@ dependencies = [ "tokio", "tracing", "tracing-subscriber", + "tracing-test", "tree_hash", "url", ] @@ -6193,6 +6194,27 @@ dependencies = [ "tracing-serde", ] +[[package]] +name = "tracing-test" +version = "0.2.5" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "557b891436fe0d5e0e363427fc7f217abf9ccd510d5136549847bdcbcd011d68" +dependencies = [ + "tracing-core", + "tracing-subscriber", + "tracing-test-macro", +] + +[[package]] +name = "tracing-test-macro" +version = "0.2.5" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "04659ddb06c87d233c566112c1c9c5b9e98256d9af50ec3bc9c8327f873a7568" +dependencies = [ + "quote", + "syn 2.0.106", +] + [[package]] name = "tree_hash" version = "0.9.1" diff --git a/Cargo.toml b/Cargo.toml index dc5ee88d..6ea8ba96 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -74,6 +74,7 @@ tower-http = { version = "0.6", features = ["trace"] } tracing = "0.1.40" tracing-appender = "0.2.3" tracing-subscriber = { version = "0.3.18", features = ["env-filter", "json"] } +tracing-test = { version = "0.2.5", features = ["no-env-filter"] } tree_hash = "0.9" tree_hash_derive = "0.9" typenum = "1.17.0" diff --git a/config.example.toml b/config.example.toml index 67085409..5b69f108 100644 --- a/config.example.toml +++ b/config.example.toml @@ -165,7 +165,8 @@ port = 20000 # Number of JWT authentication attempts a client can fail before blocking that client temporarily from Signer access # OPTIONAL, DEFAULT: 3 jwt_auth_fail_limit = 3 -# How long to block a client from Signer access, in seconds, if it failed JWT authentication too many times +# How long to block a client from Signer access, in seconds, if it failed JWT authentication too many times. +# This also defines the interval at which failed attempts are regularly checked and expired ones are cleaned up. # OPTIONAL, DEFAULT: 300 jwt_auth_fail_timeout_seconds = 300 diff --git a/crates/common/src/config/signer.rs b/crates/common/src/config/signer.rs index 4e040701..b4c5db16 100644 --- a/crates/common/src/config/signer.rs +++ b/crates/common/src/config/signer.rs @@ -88,7 +88,8 @@ pub struct SignerConfig { pub jwt_auth_fail_limit: u32, /// Duration in seconds to rate limit an endpoint after the JWT auth failure - /// limit has been reached + /// limit has been reached. This also defines the interval at which failed + /// attempts are regularly checked and expired ones are cleaned up. #[serde(default = "default_u32::")] pub jwt_auth_fail_timeout_seconds: u32, diff --git a/crates/signer/src/service.rs b/crates/signer/src/service.rs index e59ca920..e0dd6de6 100644 --- a/crates/signer/src/service.rs +++ b/crates/signer/src/service.rs @@ -143,6 +143,22 @@ impl SigningService { .route_layer(middleware::from_fn(log_request)) .route(STATUS_PATH, get(handle_status)); + // Run the JWT cleaning task + let jwt_cleaning_task = tokio::spawn(async move { + let mut interval = tokio::time::interval(state.jwt_auth_fail_timeout); + loop { + interval.tick().await; + let mut failures = state.jwt_auth_failures.write().await; + let before = failures.len(); + failures + .retain(|_, info| info.last_failure.elapsed() < state.jwt_auth_fail_timeout); + let after = failures.len(); + if before != after { + debug!("Cleaned up {} old JWT auth failure entries", before - after); + } + } + }); + let server_result = if let Some(tls_config) = config.tls_certificates { if CryptoProvider::get_default().is_none() { // Install the AWS-LC provider if no default is set, usually for CI @@ -184,6 +200,10 @@ impl SigningService { ) .await }; + + // Shutdown the JWT cleaning task + jwt_cleaning_task.abort(); + server_result.wrap_err("signer service exited") } diff --git a/tests/Cargo.toml b/tests/Cargo.toml index 6cd2b829..5b373706 100644 --- a/tests/Cargo.toml +++ b/tests/Cargo.toml @@ -21,3 +21,6 @@ tracing.workspace = true tracing-subscriber.workspace = true tree_hash.workspace = true url.workspace = true + +[dev-dependencies] +tracing-test.workspace = true diff --git a/tests/tests/signer_jwt_auth_cleanup.rs b/tests/tests/signer_jwt_auth_cleanup.rs new file mode 100644 index 00000000..d6fde2a4 --- /dev/null +++ b/tests/tests/signer_jwt_auth_cleanup.rs @@ -0,0 +1,70 @@ +use std::{collections::HashMap, time::Duration}; + +use alloy::primitives::b256; +use cb_common::{ + commit::constants::GET_PUBKEYS_PATH, + config::{ModuleSigningConfig, load_module_signing_configs}, + types::ModuleId, + utils::create_jwt, +}; +use cb_tests::{ + signer_service::start_server, + utils::{self}, +}; +use eyre::Result; +use reqwest::StatusCode; + +const JWT_MODULE: &str = "test-module"; +const JWT_SECRET: &str = "test-jwt-secret"; +const ADMIN_SECRET: &str = "test-admin-secret"; + +async fn create_mod_signing_configs() -> HashMap { + let mut cfg = + utils::get_commit_boost_config(utils::get_pbs_static_config(utils::get_pbs_config(0))); + + let module_id = ModuleId(JWT_MODULE.to_string()); + let signing_id = b256!("0101010101010101010101010101010101010101010101010101010101010101"); + + cfg.modules = Some(vec![utils::create_module_config(module_id.clone(), signing_id)]); + + let jwts = HashMap::from([(module_id.clone(), JWT_SECRET.to_string())]); + + load_module_signing_configs(&cfg, &jwts).unwrap() +} + +#[tokio::test] +#[tracing_test::traced_test] +async fn test_signer_jwt_fail_cleanup() -> Result<()> { + // setup_test_env() isn't used because we want to capture logs with tracing_test + let module_id = ModuleId(JWT_MODULE.to_string()); + let mod_cfgs = create_mod_signing_configs().await; + let start_config = start_server(20102, &mod_cfgs, ADMIN_SECRET.to_string(), false).await?; + let mod_cfg = mod_cfgs.get(&module_id).expect("JWT config for test module not found"); + + // Run as many pubkeys requests as the fail limit + let jwt = create_jwt(&module_id, "incorrect secret", GET_PUBKEYS_PATH, None)?; + let client = reqwest::Client::new(); + let url = format!("http://{}{}", start_config.endpoint, GET_PUBKEYS_PATH); + for _ in 0..start_config.jwt_auth_fail_limit { + let response = client.get(&url).bearer_auth(&jwt).send().await?; + assert!(response.status() == StatusCode::UNAUTHORIZED); + } + + // Run another request - this should fail due to rate limiting now + let jwt = create_jwt(&module_id, &mod_cfg.jwt_secret, GET_PUBKEYS_PATH, None)?; + let response = client.get(&url).bearer_auth(&jwt).send().await?; + assert!(response.status() == StatusCode::TOO_MANY_REQUESTS); + + // Wait until the cleanup task should have run properly, takes a while for the + // timing to work out + tokio::time::sleep(Duration::from_secs( + (start_config.jwt_auth_fail_timeout_seconds * 3) as u64, + )) + .await; + + // Make sure the cleanup message was logged - it's all internal state so without + // refactoring or exposing it, this is the easiest way to check if it triggered + assert!(logs_contain("Cleaned up 1 old JWT auth failure entries")); + + Ok(()) +} From 6f3eaaf6cc87a1a18ea2a840785e1198daa9b6c1 Mon Sep 17 00:00:00 2001 From: Joe Clapis Date: Tue, 30 Sep 2025 14:40:06 -0400 Subject: [PATCH 11/13] Cleaned up an argument type --- crates/signer/src/service.rs | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/crates/signer/src/service.rs b/crates/signer/src/service.rs index e0dd6de6..528e79d5 100644 --- a/crates/signer/src/service.rs +++ b/crates/signer/src/service.rs @@ -213,10 +213,7 @@ impl SigningService { } /// Marks a JWT authentication failure for a given client IP -fn mark_jwt_failure( - client_ip: IpAddr, - failures: &mut RwLockWriteGuard>, -) { +fn mark_jwt_failure(client_ip: IpAddr, failures: &mut HashMap) { let failure_info = failures .entry(client_ip) .or_insert(JwtAuthFailureInfo { failure_count: 0, last_failure: Instant::now() }); From a29a3668ea084bfae060ff734d46260163be9676 Mon Sep 17 00:00:00 2001 From: Joe Clapis Date: Tue, 30 Sep 2025 14:50:47 -0400 Subject: [PATCH 12/13] Revert "Fixed a race between reading JWT auth fails and writing to it" This reverts commit a9e247912941e9afc3f382453eb38c9fe1f4eff9. --- crates/signer/src/service.rs | 58 +++++++++++++++++------------------- 1 file changed, 27 insertions(+), 31 deletions(-) diff --git a/crates/signer/src/service.rs b/crates/signer/src/service.rs index 528e79d5..facc06ef 100644 --- a/crates/signer/src/service.rs +++ b/crates/signer/src/service.rs @@ -38,8 +38,9 @@ use cb_common::{ use cb_metrics::provider::MetricsProvider; use eyre::Context; use headers::{Authorization, authorization::Bearer}; +use parking_lot::RwLock as ParkingRwLock; use rustls::crypto::{CryptoProvider, aws_lc_rs}; -use tokio::sync::{RwLock, RwLockWriteGuard}; +use tokio::sync::RwLock; use tracing::{debug, error, info, warn}; use uuid::Uuid; @@ -70,13 +71,13 @@ struct SigningState { /// Map of modules ids to JWT configurations. This also acts as registry of /// all modules running - jwts: Arc>>, + jwts: Arc>>, /// Secret for the admin JWT - admin_secret: Arc>, + admin_secret: Arc>, /// Map of JWT failures per peer - jwt_auth_failures: Arc>>, + jwt_auth_failures: Arc>>, // JWT auth failure settings jwt_auth_fail_limit: u32, @@ -95,9 +96,9 @@ impl SigningService { let state = SigningState { manager: Arc::new(RwLock::new(start_manager(config.clone()).await?)), - jwts: Arc::new(RwLock::new(config.mod_signing_configs)), - admin_secret: Arc::new(RwLock::new(config.admin_secret)), - jwt_auth_failures: Arc::new(RwLock::new(HashMap::new())), + jwts: Arc::new(ParkingRwLock::new(config.mod_signing_configs)), + admin_secret: Arc::new(ParkingRwLock::new(config.admin_secret)), + jwt_auth_failures: Arc::new(ParkingRwLock::new(HashMap::new())), jwt_auth_fail_limit: config.jwt_auth_fail_limit, jwt_auth_fail_timeout: Duration::from_secs(config.jwt_auth_fail_timeout_seconds as u64), }; @@ -148,7 +149,7 @@ impl SigningService { let mut interval = tokio::time::interval(state.jwt_auth_fail_timeout); loop { interval.tick().await; - let mut failures = state.jwt_auth_failures.write().await; + let mut failures = state.jwt_auth_failures.write(); let before = failures.len(); failures .retain(|_, info| info.last_failure.elapsed() < state.jwt_auth_fail_timeout); @@ -213,7 +214,8 @@ impl SigningService { } /// Marks a JWT authentication failure for a given client IP -fn mark_jwt_failure(client_ip: IpAddr, failures: &mut HashMap) { +fn mark_jwt_failure(state: &SigningState, client_ip: IpAddr) { + let mut failures = state.jwt_auth_failures.write(); let failure_info = failures .entry(client_ip) .or_insert(JwtAuthFailureInfo { failure_count: 0, last_failure: Instant::now() }); @@ -253,11 +255,9 @@ async fn jwt_auth( req: Request, next: Next, ) -> Result { - let mut failures = state.jwt_auth_failures.write().await; - // Check if the request needs to be rate limited let client_ip = get_true_ip(&req_headers, &addr); - check_jwt_rate_limit(&state, &client_ip, &mut failures)?; + check_jwt_rate_limit(&state, &client_ip)?; // Clone the request so we can read the body let (parts, body) = req.into_parts(); @@ -268,14 +268,14 @@ async fn jwt_auth( })?; // Process JWT authorization - match check_jwt_auth(&auth, &state, path, &bytes).await { + match check_jwt_auth(&auth, &state, path, &bytes) { Ok(module_id) => { let mut req = Request::from_parts(parts, Body::from(bytes)); req.extensions_mut().insert(module_id); Ok(next.run(req).await) } Err(SignerModuleError::Unauthorized) => { - mark_jwt_failure(client_ip, &mut failures); + mark_jwt_failure(&state, client_ip); Err(SignerModuleError::Unauthorized) } Err(err) => Err(err), @@ -284,11 +284,9 @@ async fn jwt_auth( /// Checks if the incoming request needs to be rate limited due to previous JWT /// authentication failures -fn check_jwt_rate_limit( - state: &SigningState, - client_ip: &IpAddr, - failures: &mut RwLockWriteGuard>, -) -> Result<(), SignerModuleError> { +fn check_jwt_rate_limit(state: &SigningState, client_ip: &IpAddr) -> Result<(), SignerModuleError> { + let mut failures = state.jwt_auth_failures.write(); + // Ignore clients that don't have any failures if let Some(failure_info) = failures.get(client_ip) { // If the last failure was more than the timeout ago, remove this entry so it's @@ -322,7 +320,7 @@ fn check_jwt_rate_limit( } /// Checks if a request can successfully authenticate with the JWT secret -async fn check_jwt_auth( +fn check_jwt_auth( auth: &Authorization, state: &SigningState, path: &str, @@ -337,7 +335,7 @@ async fn check_jwt_auth( SignerModuleError::Unauthorized })?; - let guard = state.jwts.read().await; + let guard = state.jwts.read(); let jwt_config = guard.get(&claims.module).ok_or_else(|| { error!("Unauthorized request. Was the module started correctly?"); SignerModuleError::Unauthorized @@ -360,11 +358,9 @@ async fn admin_auth( req: Request, next: Next, ) -> Result { - let mut failures = state.jwt_auth_failures.write().await; - // Check if the request needs to be rate limited let client_ip = get_true_ip(&req_headers, &addr); - check_jwt_rate_limit(&state, &client_ip, &mut failures)?; + check_jwt_rate_limit(&state, &client_ip)?; // Clone the request so we can read the body let (parts, body) = req.into_parts(); @@ -378,9 +374,9 @@ async fn admin_auth( // Validate the admin JWT let body_bytes: Option<&[u8]> = if bytes.is_empty() { None } else { Some(&bytes) }; - validate_admin_jwt(jwt, &state.admin_secret.read().await, path, body_bytes).map_err(|e| { + validate_admin_jwt(jwt, &state.admin_secret.read(), path, body_bytes).map_err(|e| { error!("Unauthorized request. Invalid JWT: {e}"); - mark_jwt_failure(client_ip, &mut failures); + mark_jwt_failure(&state, client_ip); SignerModuleError::Unauthorized })?; @@ -473,7 +469,7 @@ async fn handle_request_signature_bls_impl( object_root: B256, nonce: u64, ) -> Result { - let Some(signing_id) = state.jwts.read().await.get(&module_id).map(|m| m.signing_id) else { + let Some(signing_id) = state.jwts.read().get(&module_id).map(|m| m.signing_id) else { error!( event = "proxy_bls_request_signature", ?module_id, @@ -551,7 +547,7 @@ async fn handle_request_signature_proxy_ecdsa( Json(request): Json>, ) -> Result { let req_id = Uuid::new_v4(); - let Some(signing_id) = state.jwts.read().await.get(&module_id).map(|m| m.signing_id) else { + let Some(signing_id) = state.jwts.read().get(&module_id).map(|m| m.signing_id) else { error!( event = "proxy_ecdsa_request_signature", ?module_id, @@ -673,7 +669,7 @@ async fn handle_reload( // Update the JWT configs if provided in the request if let Some(jwt_secrets) = request.jwt_secrets { - let mut jwt_configs = state.jwts.write().await; + let mut jwt_configs = state.jwts.write(); let mut new_configs = HashMap::new(); for (module_id, jwt_secret) in jwt_secrets { if let Some(signing_id) = jwt_configs.get(&module_id).map(|cfg| cfg.signing_id) { @@ -695,7 +691,7 @@ async fn handle_reload( // Update the rest of the state once everything has passed if let Some(admin_secret) = request.admin_secret { - *state.admin_secret.write().await = admin_secret; + *state.admin_secret.write() = admin_secret; } *state.manager.write().await = new_manager; @@ -706,7 +702,7 @@ async fn handle_revoke_module( State(state): State, Json(request): Json, ) -> Result { - let mut guard = state.jwts.write().await; + let mut guard = state.jwts.write(); guard .remove(&request.module_id) .ok_or(SignerModuleError::ModuleIdNotFound) From 157aec770aea7fe759b7960060e3ad4ebf78afba Mon Sep 17 00:00:00 2001 From: Joe Clapis Date: Tue, 30 Sep 2025 16:31:42 -0400 Subject: [PATCH 13/13] Swapped to client-ip for proxy header IP checking --- Cargo.lock | 27 +++++++++++++++++++ Cargo.toml | 1 + crates/signer/Cargo.toml | 1 + crates/signer/src/service.rs | 52 +++++++++++++++++++++++------------- 4 files changed, 63 insertions(+), 18 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index e06d7ff0..285bbd94 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1679,6 +1679,7 @@ dependencies = [ "blsful", "cb-common", "cb-metrics", + "client-ip", "eyre", "futures", "headers", @@ -1837,6 +1838,16 @@ version = "0.7.5" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "b94f61472cee1439c0b966b47e3aca9ae07e45d070759512cd390ea2bebc6675" +[[package]] +name = "client-ip" +version = "0.1.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "31211fc26899744f5b22521fdc971e5f3875991d8880537537470685a0e9552d" +dependencies = [ + "forwarded-header-value", + "http", +] + [[package]] name = "cmake" version = "0.1.54" @@ -2829,6 +2840,16 @@ dependencies = [ "percent-encoding", ] +[[package]] +name = "forwarded-header-value" +version = "0.1.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "8835f84f38484cc86f110a805655697908257fb9a7af005234060891557198e9" +dependencies = [ + "nonempty", + "thiserror 1.0.69", +] + [[package]] name = "fs-err" version = "3.1.0" @@ -3949,6 +3970,12 @@ dependencies = [ "minimal-lexical", ] +[[package]] +name = "nonempty" +version = "0.7.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "e9e591e719385e6ebaeb5ce5d3887f7d5676fceca6411d1925ccc95745f3d6f7" + [[package]] name = "nu-ansi-term" version = "0.50.1" diff --git a/Cargo.toml b/Cargo.toml index 6ea8ba96..b0533144 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -33,6 +33,7 @@ cb-pbs = { path = "crates/pbs" } cb-signer = { path = "crates/signer" } cipher = "0.4" clap = { version = "4.5.4", features = ["derive", "env"] } +client-ip = { version = "0.1.1", features = [ "forwarded-header" ] } color-eyre = "0.6.3" const_format = "0.2.34" ctr = "0.9.2" diff --git a/crates/signer/Cargo.toml b/crates/signer/Cargo.toml index 7c6e63fa..1a688e1b 100644 --- a/crates/signer/Cargo.toml +++ b/crates/signer/Cargo.toml @@ -14,6 +14,7 @@ bimap.workspace = true blsful.workspace = true cb-common.workspace = true cb-metrics.workspace = true +client-ip.workspace = true eyre.workspace = true futures.workspace = true headers.workspace = true diff --git a/crates/signer/src/service.rs b/crates/signer/src/service.rs index facc06ef..e9480db1 100644 --- a/crates/signer/src/service.rs +++ b/crates/signer/src/service.rs @@ -36,6 +36,7 @@ use cb_common::{ utils::{decode_jwt, validate_admin_jwt, validate_jwt}, }; use cb_metrics::provider::MetricsProvider; +use client_ip::*; use eyre::Context; use headers::{Authorization, authorization::Bearer}; use parking_lot::RwLock as ParkingRwLock; @@ -225,25 +226,34 @@ fn mark_jwt_failure(state: &SigningState, client_ip: IpAddr) { /// Get the true client IP from the request headers or fallback to the socket /// address -fn get_true_ip(req_headers: &HeaderMap, addr: &SocketAddr) -> IpAddr { - // Try the X-Forwarded-For header first - if let Some(true_ip) = req_headers.get("x-forwarded-for") && - let Ok(true_ip) = true_ip.to_str() && - let Ok(true_ip) = true_ip.parse() - { - return true_ip; - } - - // Then try the X-Real-IP header - if let Some(true_ip) = req_headers.get("x-real-ip") && - let Ok(true_ip) = true_ip.to_str() && - let Ok(true_ip) = true_ip.parse() - { - return true_ip; +fn get_true_ip(req_headers: &HeaderMap, addr: &SocketAddr) -> eyre::Result { + let ip_extractors = [ + cf_connecting_ip, + cloudfront_viewer_address, + fly_client_ip, + rightmost_forwarded, + rightmost_x_forwarded_for, + true_client_ip, + x_real_ip, + ]; + + // Run each extractor in order and return the first valid IP found + for extractor in ip_extractors { + match extractor(req_headers) { + Ok(true_ip) => { + return Ok(true_ip); + } + Err(e) => { + match e { + Error::AbsentHeader { .. } => continue, // Missing headers are fine + _ => return Err(eyre::eyre!(e.to_string())), // Report anything else + } + } + } } // Fallback to the socket IP - addr.ip() + Ok(addr.ip()) } /// Authentication middleware layer @@ -256,7 +266,10 @@ async fn jwt_auth( next: Next, ) -> Result { // Check if the request needs to be rate limited - let client_ip = get_true_ip(&req_headers, &addr); + let client_ip = get_true_ip(&req_headers, &addr).map_err(|e| { + error!("Failed to get client IP: {e}"); + SignerModuleError::RequestError("failed to get client IP".to_string()) + })?; check_jwt_rate_limit(&state, &client_ip)?; // Clone the request so we can read the body @@ -359,7 +372,10 @@ async fn admin_auth( next: Next, ) -> Result { // Check if the request needs to be rate limited - let client_ip = get_true_ip(&req_headers, &addr); + let client_ip = get_true_ip(&req_headers, &addr).map_err(|e| { + error!("Failed to get client IP: {e}"); + SignerModuleError::RequestError("failed to get client IP".to_string()) + })?; check_jwt_rate_limit(&state, &client_ip)?; // Clone the request so we can read the body