diff --git a/crates/credentials/src/credentials.rs b/crates/credentials/src/credentials.rs index c06024115..b8d256f78 100644 --- a/crates/credentials/src/credentials.rs +++ b/crates/credentials/src/credentials.rs @@ -29,6 +29,12 @@ static GLOBAL_ACTIVE_CRED: OnceLock = OnceLock::new(); /// Global RPC authentication token pub static GLOBAL_RUSTFS_RPC_SECRET: OnceLock = OnceLock::new(); +/// Public error returned when RPC authentication is not safely configured. +pub const RPC_SECRET_REQUIRED_MESSAGE: &str = "RPC authentication secret is not configured"; + +/// Operator-facing guidance for configuring RPC authentication safely. +pub const RPC_SECRET_REQUIRED_OPERATOR_MESSAGE: &str = "RUSTFS_RPC_SECRET must be set to a non-default value or RUSTFS_SECRET_KEY must be changed from the default for RPC authentication"; + /// Error type for credentials operations #[derive(Debug)] pub enum CredentialsError { @@ -216,13 +222,39 @@ pub fn gen_secret_key(length: usize) -> std::io::Result { /// # Returns /// * `String` - The RPC authentication token /// +fn resolve_rpc_secret(env_secret: Option<&str>, global_secret: Option<&str>) -> Option { + if let Some(secret) = env_secret.map(str::trim).filter(|secret| !secret.is_empty()) { + return (secret != DEFAULT_SECRET_KEY).then(|| secret.to_string()); + } + + global_secret + .map(str::trim) + .filter(|secret| !secret.is_empty() && *secret != DEFAULT_SECRET_KEY) + .map(ToOwned::to_owned) +} + +pub fn try_get_rpc_token() -> std::io::Result { + if let Some(secret) = GLOBAL_RUSTFS_RPC_SECRET.get() { + return resolve_rpc_secret(None, Some(secret)).ok_or_else(|| Error::other(RPC_SECRET_REQUIRED_MESSAGE)); + } + + let env_secret = env::var(ENV_RPC_SECRET).ok(); + let global_secret = get_global_secret_key_opt(); + let secret = resolve_rpc_secret(env_secret.as_deref(), global_secret.as_deref()) + .ok_or_else(|| Error::other(RPC_SECRET_REQUIRED_MESSAGE))?; + + match GLOBAL_RUSTFS_RPC_SECRET.set(secret.clone()) { + Ok(()) => Ok(secret), + Err(_) => GLOBAL_RUSTFS_RPC_SECRET + .get() + .and_then(|stored| resolve_rpc_secret(None, Some(stored))) + .ok_or_else(|| Error::other(RPC_SECRET_REQUIRED_MESSAGE)), + } +} + +#[deprecated(note = "use try_get_rpc_token to handle missing RPC secrets explicitly")] pub fn get_rpc_token() -> String { - GLOBAL_RUSTFS_RPC_SECRET - .get_or_init(|| { - env::var(ENV_RPC_SECRET) - .unwrap_or_else(|_| get_global_secret_key_opt().unwrap_or_else(|| DEFAULT_SECRET_KEY.to_string())) - }) - .clone() + try_get_rpc_token().expect(RPC_SECRET_REQUIRED_MESSAGE) } /// A wrapper struct for masking sensitive strings in Debug implementations. @@ -301,7 +333,7 @@ impl fmt::Debug for Credentials { f.debug_struct("Credentials") .field("access_key", &self.access_key) .field("secret_key", &Masked(Some(&self.secret_key))) - .field("session_token", &self.session_token) + .field("session_token", &Masked(Some(&self.session_token))) .field("expiration", &self.expiration) .field("status", &self.status) .field("parent_user", &self.parent_user) @@ -495,6 +527,37 @@ mod tests { assert!(!key.contains('=')); } + #[test] + fn test_resolve_rpc_secret_rejects_default_fallback() { + assert!(resolve_rpc_secret(None, None).is_none()); + assert!(resolve_rpc_secret(None, Some(DEFAULT_SECRET_KEY)).is_none()); + assert!(resolve_rpc_secret(Some(DEFAULT_SECRET_KEY), Some("custom-global-secret")).is_none()); + } + + #[test] + fn test_rpc_secret_public_error_omits_configuration_details() { + assert!(!RPC_SECRET_REQUIRED_MESSAGE.contains("RUSTFS_")); + assert!(!RPC_SECRET_REQUIRED_MESSAGE.contains(DEFAULT_SECRET_KEY)); + assert!(RPC_SECRET_REQUIRED_OPERATOR_MESSAGE.contains("RUSTFS_RPC_SECRET")); + } + + #[allow(deprecated)] + #[test] + fn test_get_rpc_token_preserves_string_return_type() { + fn assert_string_return(_: fn() -> String) {} + + assert_string_return(get_rpc_token); + } + + #[test] + fn test_resolve_rpc_secret_accepts_non_default_secret() { + assert_eq!(resolve_rpc_secret(Some("custom-rpc-secret"), None).as_deref(), Some("custom-rpc-secret")); + assert_eq!( + resolve_rpc_secret(None, Some("custom-global-secret")).as_deref(), + Some("custom-global-secret") + ); + } + #[test] fn test_masked_debug() { // Test None @@ -524,6 +587,24 @@ mod tests { assert_eq!(format!("{:?}", Masked(Some("中文测试"))), "中***试|4"); } + #[test] + fn test_credentials_debug_masks_sensitive_fields() { + let cred = Credentials { + access_key: "debug-access-key".to_string(), + secret_key: "debug-secret-key".to_string(), + session_token: "debug-session-token".to_string(), + parent_user: "parent-user".to_string(), + ..Default::default() + }; + + let output = format!("{cred:?}"); + + assert!(output.contains("debug-access-key")); + assert!(output.contains("parent-user")); + assert!(!output.contains("debug-secret-key")); + assert!(!output.contains("debug-session-token")); + } + #[test] fn test_credentials_expiration_serialize_as_rfc3339() { use time::OffsetDateTime; diff --git a/crates/ecstore/src/rpc/client.rs b/crates/ecstore/src/rpc/client.rs index 43a1c37f0..2138345b6 100644 --- a/crates/ecstore/src/rpc/client.rs +++ b/crates/ecstore/src/rpc/client.rs @@ -96,7 +96,8 @@ pub struct TonicSignatureInterceptor; impl tonic::service::Interceptor for TonicSignatureInterceptor { fn call(&mut self, mut req: tonic::Request<()>) -> Result, tonic::Status> { - let headers = gen_signature_headers(TONIC_RPC_PREFIX, &Method::GET); + let headers = gen_signature_headers(TONIC_RPC_PREFIX, &Method::GET) + .map_err(|_| tonic::Status::unauthenticated("No valid auth token"))?; req.metadata_mut().as_mut().extend(headers); inject_trace_context_into_metadata(req.metadata_mut()); inject_request_id_into_metadata(req.metadata_mut()); @@ -135,8 +136,13 @@ mod tests { use super::*; use tonic::service::Interceptor; + fn ensure_test_rpc_secret() { + let _ = rustfs_credentials::GLOBAL_RUSTFS_RPC_SECRET.set("test-rpc-secret".to_string()); + } + #[test] fn test_signature_interceptor_keeps_auth_headers() { + ensure_test_rpc_secret(); let mut interceptor = TonicSignatureInterceptor; let req = tonic::Request::new(()); @@ -148,6 +154,7 @@ mod tests { #[test] fn test_signature_interceptor_may_inject_request_id() { + ensure_test_rpc_secret(); let mut interceptor = TonicSignatureInterceptor; let req = tonic::Request::new(()); diff --git a/crates/ecstore/src/rpc/http_auth.rs b/crates/ecstore/src/rpc/http_auth.rs index 9f267d248..c494b54c7 100644 --- a/crates/ecstore/src/rpc/http_auth.rs +++ b/crates/ecstore/src/rpc/http_auth.rs @@ -17,8 +17,11 @@ use base64::Engine as _; use base64::engine::general_purpose; use hmac::{Hmac, KeyInit, Mac}; use http::{HeaderMap, HeaderValue, Method, Uri}; -use rustfs_credentials::{DEFAULT_SECRET_KEY, ENV_RPC_SECRET, get_global_secret_key_opt}; +#[cfg(test)] +use rustfs_credentials::{DEFAULT_SECRET_KEY, RPC_SECRET_REQUIRED_MESSAGE}; +use rustfs_credentials::{RPC_SECRET_REQUIRED_OPERATOR_MESSAGE, try_get_rpc_token}; use sha2::Sha256; +use std::sync::Once; use time::OffsetDateTime; use tracing::error; @@ -28,19 +31,31 @@ const SIGNATURE_HEADER: &str = "x-rustfs-signature"; const TIMESTAMP_HEADER: &str = "x-rustfs-timestamp"; const SIGNATURE_VALID_DURATION: i64 = 300; // 5 minutes pub const TONIC_RPC_PREFIX: &str = "/node_service.NodeService"; +static RPC_SECRET_RESOLUTION_LOG_ONCE: Once = Once::new(); /// Get the shared secret for HMAC signing -fn get_shared_secret() -> String { - rustfs_credentials::GLOBAL_RUSTFS_RPC_SECRET - .get_or_init(|| { - rustfs_utils::get_env_str( - ENV_RPC_SECRET, - get_global_secret_key_opt() - .unwrap_or_else(|| DEFAULT_SECRET_KEY.to_string()) - .as_str(), - ) - }) - .clone() +#[cfg(test)] +fn resolve_shared_secret(env_secret: Option<&str>, global_secret: Option<&str>) -> std::io::Result { + if let Some(secret) = env_secret.map(str::trim).filter(|secret| !secret.is_empty()) { + return (secret != DEFAULT_SECRET_KEY) + .then(|| secret.to_string()) + .ok_or_else(|| std::io::Error::other(RPC_SECRET_REQUIRED_MESSAGE)); + } + + global_secret + .map(str::trim) + .filter(|secret| !secret.is_empty() && *secret != DEFAULT_SECRET_KEY) + .map(ToOwned::to_owned) + .ok_or_else(|| std::io::Error::other(RPC_SECRET_REQUIRED_MESSAGE)) +} + +fn get_shared_secret() -> std::io::Result { + try_get_rpc_token().map_err(|err| { + RPC_SECRET_RESOLUTION_LOG_ONCE.call_once(|| { + error!("RPC auth secret resolution failed: {}; {}", err, RPC_SECRET_REQUIRED_OPERATOR_MESSAGE); + }); + err + }) } /// Generate HMAC-SHA256 signature for the given data @@ -59,16 +74,17 @@ fn generate_signature(secret: &str, url: &str, method: &Method, timestamp: i64) } /// Build headers with authentication signature -pub fn build_auth_headers(url: &str, method: &Method, headers: &mut HeaderMap) { - let auth_headers = gen_signature_headers(url, method); +pub fn build_auth_headers(url: &str, method: &Method, headers: &mut HeaderMap) -> std::io::Result<()> { + let auth_headers = gen_signature_headers(url, method)?; headers.extend(auth_headers); inject_trace_context_into_http_headers(headers); inject_request_id_into_http_headers(headers); + Ok(()) } -pub fn gen_signature_headers(url: &str, method: &Method) -> HeaderMap { - let secret = get_shared_secret(); +pub fn gen_signature_headers(url: &str, method: &Method) -> std::io::Result { + let secret = get_shared_secret()?; let timestamp = OffsetDateTime::now_utc().unix_timestamp(); let signature = generate_signature(&secret, url, method, timestamp); @@ -80,13 +96,11 @@ pub fn gen_signature_headers(url: &str, method: &Method) -> HeaderMap { HeaderValue::from_str(×tamp.to_string()).expect("Invalid header value"), ); - headers + Ok(headers) } /// Verify the request signature for RPC requests pub fn verify_rpc_signature(url: &str, method: &Method, headers: &HeaderMap) -> std::io::Result<()> { - let secret = get_shared_secret(); - // Get signature from header let signature = headers .get(SIGNATURE_HEADER) @@ -106,24 +120,24 @@ pub fn verify_rpc_signature(url: &str, method: &Method, headers: &HeaderMap) -> // Check timestamp validity (prevent replay attacks) let current_time = OffsetDateTime::now_utc().unix_timestamp(); - if current_time.saturating_sub(timestamp) > SIGNATURE_VALID_DURATION { + if current_time.saturating_sub(timestamp) > SIGNATURE_VALID_DURATION + || timestamp.saturating_sub(current_time) > SIGNATURE_VALID_DURATION + { return Err(std::io::Error::other("Request timestamp expired")); } // Generate expected signature + let secret = get_shared_secret()?; let expected_signature = generate_signature(&secret, url, method, timestamp); // Compare signatures if signature != expected_signature { error!( - "verify_rpc_signature: Invalid signature: url {}, method {}, timestamp {}, signature {}, expected_signature: {}***{}|{}", + "verify_rpc_signature: Invalid signature: url {}, method {}, timestamp {}, signature_len {}", url, method, timestamp, - signature, - expected_signature.chars().next().unwrap_or('*'), - expected_signature.chars().last().unwrap_or('*'), - expected_signature.len() + signature.len() ); return Err(std::io::Error::other("Invalid signature")); @@ -137,18 +151,79 @@ mod tests { use super::*; use crate::rpc::context_propagation::REQUEST_ID_HEADER; use http::{HeaderMap, Method}; + use std::io::{self, Write}; + use std::sync::{Arc, Mutex}; use time::OffsetDateTime; + use tracing_subscriber::fmt::MakeWriter; + + #[derive(Clone, Default)] + struct CapturedLogs { + buffer: Arc>>, + } + + struct CapturedLogWriter { + buffer: Arc>>, + } + + impl CapturedLogs { + fn contents(&self) -> String { + let buffer = self + .buffer + .lock() + .expect("captured logs mutex should not be poisoned") + .clone(); + String::from_utf8(buffer).expect("captured logs should be valid UTF-8") + } + } + + impl Write for CapturedLogWriter { + fn write(&mut self, buf: &[u8]) -> io::Result { + self.buffer + .lock() + .expect("captured logs mutex should not be poisoned") + .extend_from_slice(buf); + Ok(buf.len()) + } + + fn flush(&mut self) -> io::Result<()> { + Ok(()) + } + } + + impl<'a> MakeWriter<'a> for CapturedLogs { + type Writer = CapturedLogWriter; + + fn make_writer(&'a self) -> Self::Writer { + CapturedLogWriter { + buffer: Arc::clone(&self.buffer), + } + } + } + + fn ensure_test_rpc_secret() { + let _ = rustfs_credentials::GLOBAL_RUSTFS_RPC_SECRET.set("test-rpc-secret".to_string()); + } + + #[test] + fn test_resolve_shared_secret_rejects_default_fallback() { + let err = resolve_shared_secret(None, None).expect_err("default fallback must be rejected"); + assert_eq!(err.to_string(), RPC_SECRET_REQUIRED_MESSAGE); + + let err = resolve_shared_secret(None, Some(DEFAULT_SECRET_KEY)).expect_err("default global secret must be rejected"); + assert_eq!(err.to_string(), RPC_SECRET_REQUIRED_MESSAGE); + } #[test] fn test_get_shared_secret() { - let secret = get_shared_secret(); + ensure_test_rpc_secret(); + let secret = get_shared_secret().expect("test RPC secret should resolve"); assert!(!secret.is_empty(), "Secret should not be empty"); let url = "http://node1:7000/rustfs/rpc/read_file_stream?disk=http%3A%2F%2Fnode1%3A7000%2Fdata%2Frustfs3&volume=.rustfs.sys&path=pool.bin%2Fdd0fd773-a962-4265-b543-783ce83953e9%2Fpart.1&offset=0&length=44"; let method = Method::GET; let mut headers = HeaderMap::new(); - build_auth_headers(url, &method, &mut headers); + build_auth_headers(url, &method, &mut headers).expect("auth headers should build"); let url = "/rustfs/rpc/read_file_stream?disk=http%3A%2F%2Fnode1%3A7000%2Fdata%2Frustfs3&volume=.rustfs.sys&path=pool.bin%2Fdd0fd773-a962-4265-b543-783ce83953e9%2Fpart.1&offset=0&length=44"; @@ -189,11 +264,12 @@ mod tests { #[test] fn test_build_auth_headers() { + ensure_test_rpc_secret(); let url = "http://example.com/api/test"; let method = Method::POST; let mut headers = HeaderMap::new(); - build_auth_headers(url, &method, &mut headers); + build_auth_headers(url, &method, &mut headers).expect("auth headers should build"); // Verify headers are present assert!(headers.contains_key(SIGNATURE_HEADER), "Should contain signature header"); @@ -216,25 +292,27 @@ mod tests { #[test] fn test_build_auth_headers_preserves_existing_request_id() { + ensure_test_rpc_secret(); let url = "http://example.com/api/test"; let method = Method::GET; let mut headers = HeaderMap::new(); headers.insert(REQUEST_ID_HEADER, HeaderValue::from_static("req-upstream-123")); - build_auth_headers(url, &method, &mut headers); + build_auth_headers(url, &method, &mut headers).expect("auth headers should build"); assert_eq!(headers.get(REQUEST_ID_HEADER).and_then(|v| v.to_str().ok()), Some("req-upstream-123")); } #[test] fn test_build_auth_headers_may_set_request_id_from_trace_id() { + ensure_test_rpc_secret(); let url = "http://example.com/api/test"; let method = Method::GET; let mut headers = HeaderMap::new(); let span = tracing::info_span!("rpc-test-span"); let _guard = span.enter(); - build_auth_headers(url, &method, &mut headers); + build_auth_headers(url, &method, &mut headers).expect("auth headers should build"); if let Some(value) = headers.get(REQUEST_ID_HEADER).and_then(|v| v.to_str().ok()) { assert!(!value.is_empty(), "request id should not be empty"); @@ -243,12 +321,13 @@ mod tests { #[test] fn test_verify_rpc_signature_success() { + ensure_test_rpc_secret(); let url = "http://example.com/api/test"; let method = Method::GET; let mut headers = HeaderMap::new(); // Build headers with valid signature - build_auth_headers(url, &method, &mut headers); + build_auth_headers(url, &method, &mut headers).expect("auth headers should build"); // Verify should succeed let result = verify_rpc_signature(url, &method, &headers); @@ -257,12 +336,13 @@ mod tests { #[test] fn test_verify_rpc_signature_invalid_signature() { + ensure_test_rpc_secret(); let url = "http://example.com/api/test"; let method = Method::GET; let mut headers = HeaderMap::new(); // Build headers with valid signature first - build_auth_headers(url, &method, &mut headers); + build_auth_headers(url, &method, &mut headers).expect("auth headers should build"); // Tamper with the signature headers.insert(SIGNATURE_HEADER, HeaderValue::from_str("invalid-signature").unwrap()); @@ -275,15 +355,49 @@ mod tests { assert_eq!(error.to_string(), "Invalid signature"); } + #[test] + fn test_invalid_signature_log_contract_excludes_secrets() { + ensure_test_rpc_secret(); + let url = "http://example.com/api/test"; + let method = Method::GET; + let timestamp = OffsetDateTime::now_utc().unix_timestamp(); + let secret = get_shared_secret().expect("test RPC secret should resolve"); + let expected_signature = generate_signature(&secret, url, &method, timestamp); + let invalid_signature = "invalid-signature"; + let logs = CapturedLogs::default(); + let subscriber = tracing_subscriber::fmt() + .with_max_level(tracing::Level::ERROR) + .with_writer(logs.clone()) + .with_ansi(false) + .without_time() + .finish(); + + let mut headers = HeaderMap::new(); + headers.insert(SIGNATURE_HEADER, HeaderValue::from_str(invalid_signature).unwrap()); + headers.insert(TIMESTAMP_HEADER, HeaderValue::from_str(×tamp.to_string()).unwrap()); + + tracing::subscriber::with_default(subscriber, || { + let result = verify_rpc_signature(url, &method, &headers); + assert!(result.is_err(), "Invalid signature should fail verification"); + }); + + let captured = logs.contents(); + assert!(captured.contains("Invalid signature")); + assert!(!captured.contains(&secret)); + assert!(!captured.contains(&expected_signature)); + assert!(!captured.contains(invalid_signature)); + } + #[test] fn test_verify_rpc_signature_expired_timestamp() { + ensure_test_rpc_secret(); let url = "http://example.com/api/test"; let method = Method::GET; let mut headers = HeaderMap::new(); // Set expired timestamp (older than SIGNATURE_VALID_DURATION) let expired_timestamp = OffsetDateTime::now_utc().unix_timestamp() - SIGNATURE_VALID_DURATION - 10; - let secret = get_shared_secret(); + let secret = get_shared_secret().expect("test RPC secret should resolve"); let signature = generate_signature(&secret, url, &method, expired_timestamp); headers.insert(SIGNATURE_HEADER, HeaderValue::from_str(&signature).unwrap()); @@ -297,6 +411,27 @@ mod tests { assert_eq!(error.to_string(), "Request timestamp expired"); } + #[test] + fn test_verify_rpc_signature_future_timestamp_outside_window() { + ensure_test_rpc_secret(); + let url = "http://example.com/api/test"; + let method = Method::GET; + let mut headers = HeaderMap::new(); + + let future_timestamp = OffsetDateTime::now_utc().unix_timestamp() + SIGNATURE_VALID_DURATION + 10; + let secret = get_shared_secret().expect("test RPC secret should resolve"); + let signature = generate_signature(&secret, url, &method, future_timestamp); + + headers.insert(SIGNATURE_HEADER, HeaderValue::from_str(&signature).unwrap()); + headers.insert(TIMESTAMP_HEADER, HeaderValue::from_str(&future_timestamp.to_string()).unwrap()); + + let result = verify_rpc_signature(url, &method, &headers); + assert!(result.is_err(), "Future timestamp outside valid window should fail verification"); + + let error = result.unwrap_err(); + assert_eq!(error.to_string(), "Request timestamp expired"); + } + #[test] fn test_verify_rpc_signature_missing_signature_header() { let url = "http://example.com/api/test"; @@ -351,13 +486,14 @@ mod tests { #[test] fn test_verify_rpc_signature_url_mismatch() { + ensure_test_rpc_secret(); let original_url = "http://example.com/api/test"; let different_url = "http://example.com/api/different"; let method = Method::GET; let mut headers = HeaderMap::new(); // Build headers for one URL - build_auth_headers(original_url, &method, &mut headers); + build_auth_headers(original_url, &method, &mut headers).expect("auth headers should build"); // Try to verify with a different URL let result = verify_rpc_signature(different_url, &method, &headers); @@ -369,13 +505,14 @@ mod tests { #[test] fn test_verify_rpc_signature_method_mismatch() { + ensure_test_rpc_secret(); let url = "http://example.com/api/test"; let original_method = Method::GET; let different_method = Method::POST; let mut headers = HeaderMap::new(); // Build headers for one method - build_auth_headers(url, &original_method, &mut headers); + build_auth_headers(url, &original_method, &mut headers).expect("auth headers should build"); // Try to verify with a different method let result = verify_rpc_signature(url, &different_method, &headers); @@ -387,9 +524,10 @@ mod tests { #[test] fn test_signature_valid_duration_boundary() { + ensure_test_rpc_secret(); let url = "http://example.com/api/test"; let method = Method::GET; - let secret = get_shared_secret(); + let secret = get_shared_secret().expect("test RPC secret should resolve"); let mut headers = HeaderMap::new(); let current_time = OffsetDateTime::now_utc().unix_timestamp(); @@ -418,6 +556,7 @@ mod tests { #[test] fn test_round_trip_authentication() { + ensure_test_rpc_secret(); let test_cases = vec![ ("http://example.com/api/test", Method::GET), ("https://api.rustfs.com/v1/bucket", Method::POST), @@ -429,7 +568,7 @@ mod tests { let mut headers = HeaderMap::new(); // Build authentication headers - build_auth_headers(url, &method, &mut headers); + build_auth_headers(url, &method, &mut headers).expect("auth headers should build"); // Verify the signature should succeed let result = verify_rpc_signature(url, &method, &headers); diff --git a/crates/ecstore/src/rpc/remote_disk.rs b/crates/ecstore/src/rpc/remote_disk.rs index eb58b0fd6..96aeaf577 100644 --- a/crates/ecstore/src/rpc/remote_disk.rs +++ b/crates/ecstore/src/rpc/remote_disk.rs @@ -1143,7 +1143,7 @@ impl DiskAPI for RemoteDisk { let mut headers = HeaderMap::new(); headers.insert(CONTENT_TYPE, HeaderValue::from_static("application/json")); - build_auth_headers(&url, &Method::GET, &mut headers); + build_auth_headers(&url, &Method::GET, &mut headers)?; let mut reader = HttpReader::new_with_stall_timeout( url, @@ -1196,7 +1196,7 @@ impl DiskAPI for RemoteDisk { let mut headers = HeaderMap::new(); headers.insert(CONTENT_TYPE, HeaderValue::from_static("application/json")); - build_auth_headers(&url, &Method::GET, &mut headers); + build_auth_headers(&url, &Method::GET, &mut headers)?; Ok(Box::new(HttpReader::new(url, Method::GET, headers, None).await?)) } @@ -1239,7 +1239,7 @@ impl DiskAPI for RemoteDisk { let mut headers = HeaderMap::new(); headers.insert(CONTENT_TYPE, HeaderValue::from_static("application/json")); - build_auth_headers(&url, &Method::PUT, &mut headers); + build_auth_headers(&url, &Method::PUT, &mut headers)?; Ok(Box::new(HttpWriter::new(url, Method::PUT, headers).await?)) } @@ -1270,7 +1270,7 @@ impl DiskAPI for RemoteDisk { let mut headers = HeaderMap::new(); headers.insert(CONTENT_TYPE, HeaderValue::from_static("application/json")); - build_auth_headers(&url, &Method::PUT, &mut headers); + build_auth_headers(&url, &Method::PUT, &mut headers)?; Ok(Box::new(HttpWriter::new(url, Method::PUT, headers).await?)) } diff --git a/crates/ecstore/src/rpc/remote_locker.rs b/crates/ecstore/src/rpc/remote_locker.rs index b39c1699a..82bd84a71 100644 --- a/crates/ecstore/src/rpc/remote_locker.rs +++ b/crates/ecstore/src/rpc/remote_locker.rs @@ -483,6 +483,10 @@ mod tests { GLOBAL_CONN_MAP.write().await.insert(addr.to_string(), channel); } + fn ensure_test_rpc_secret() { + let _ = rustfs_credentials::GLOBAL_RUSTFS_RPC_SECRET.set("test-rpc-secret".to_string()); + } + fn test_lock_request(timeout_duration: Duration) -> LockRequest { LockRequest::new(ObjectKey::new("bucket", "object"), LockType::Exclusive, "owner-a") .with_acquire_timeout(timeout_duration) @@ -491,6 +495,7 @@ mod tests { #[tokio::test] async fn test_remote_client_acquire_lock_respects_request_timeout_and_evicts_connection() { + ensure_test_rpc_secret(); let (addr, accept_task) = spawn_hanging_listener().await; cache_lazy_channel(&addr).await; assert!(GLOBAL_CONN_MAP.read().await.contains_key(&addr)); @@ -517,6 +522,7 @@ mod tests { #[tokio::test] async fn test_remote_client_acquire_locks_batch_respects_request_timeout_and_evicts_connection() { + ensure_test_rpc_secret(); let (addr, accept_task) = spawn_hanging_listener().await; cache_lazy_channel(&addr).await; assert!(GLOBAL_CONN_MAP.read().await.contains_key(&addr)); diff --git a/rustfs/src/admin/handlers/profile.rs b/rustfs/src/admin/handlers/profile.rs index b8ee18b77..6f6ebb29a 100644 --- a/rustfs/src/admin/handlers/profile.rs +++ b/rustfs/src/admin/handlers/profile.rs @@ -12,17 +12,41 @@ // See the License for the specific language governing permissions and // limitations under the License. -use crate::admin::router::Operation; +use crate::admin::{auth::validate_admin_request, router::Operation}; +use crate::auth::{check_key_valid, get_session_token}; +use crate::server::RemoteAddr; use http::header::CONTENT_TYPE; use http::{HeaderMap, StatusCode}; use matchit::Params; -use s3s::{Body, S3Request, S3Response, S3Result}; +use rustfs_policy::policy::action::{Action, AdminAction}; +use s3s::{Body, S3Request, S3Response, S3Result, s3_error}; use tracing::info; +pub(super) async fn authorize_profile_request(req: &S3Request) -> S3Result<()> { + let Some(input_cred) = req.credentials.as_ref() else { + return Err(s3_error!(AccessDenied, "Signature is required")); + }; + + let (cred, owner) = + check_key_valid(get_session_token(&req.uri, &req.headers).unwrap_or_default(), &input_cred.access_key).await?; + let remote_addr = req.extensions.get::>().and_then(|opt| opt.map(|a| a.0)); + + validate_admin_request( + &req.headers, + &cred, + owner, + false, + vec![Action::AdminAction(AdminAction::ProfilingAdminAction)], + remote_addr, + ) + .await +} + pub struct TriggerProfileCPU {} #[async_trait::async_trait] impl Operation for TriggerProfileCPU { - async fn call(&self, _req: S3Request, _params: Params<'_, '_>) -> S3Result> { + async fn call(&self, req: S3Request, _params: Params<'_, '_>) -> S3Result> { + authorize_profile_request(&req).await?; info!("Triggering CPU profile dump via S3 request..."); let dur = std::time::Duration::from_secs(60); @@ -40,7 +64,8 @@ impl Operation for TriggerProfileCPU { pub struct TriggerProfileMemory {} #[async_trait::async_trait] impl Operation for TriggerProfileMemory { - async fn call(&self, _req: S3Request, _params: Params<'_, '_>) -> S3Result> { + async fn call(&self, req: S3Request, _params: Params<'_, '_>) -> S3Result> { + authorize_profile_request(&req).await?; info!("Triggering Memory profile dump via S3 request..."); match crate::profiling::dump_memory_pprof_now().await { diff --git a/rustfs/src/admin/handlers/profile_admin.rs b/rustfs/src/admin/handlers/profile_admin.rs index 9198070d9..4e2d830d6 100644 --- a/rustfs/src/admin/handlers/profile_admin.rs +++ b/rustfs/src/admin/handlers/profile_admin.rs @@ -12,6 +12,7 @@ // See the License for the specific language governing permissions and // limitations under the License. +use super::profile::authorize_profile_request; use crate::admin::router::{AdminOperation, Operation, S3Router}; use crate::server::ADMIN_PREFIX; use http::{HeaderMap, HeaderValue, Uri}; @@ -56,6 +57,8 @@ pub struct ProfileHandler {} #[async_trait::async_trait] impl Operation for ProfileHandler { async fn call(&self, req: S3Request, _params: Params<'_, '_>) -> S3Result> { + authorize_profile_request(&req).await?; + #[cfg(not(all(target_os = "linux", target_env = "gnu", target_arch = "x86_64")))] { let requested_url = req.uri.to_string(); @@ -151,7 +154,9 @@ pub struct ProfileStatusHandler {} #[async_trait::async_trait] impl Operation for ProfileStatusHandler { - async fn call(&self, _req: S3Request, _params: Params<'_, '_>) -> S3Result> { + async fn call(&self, req: S3Request, _params: Params<'_, '_>) -> S3Result> { + authorize_profile_request(&req).await?; + #[cfg(not(all(target_os = "linux", target_env = "gnu", target_arch = "x86_64")))] let message = format!("CPU profiling is not supported on {} platform", std::env::consts::OS); #[cfg(not(all(target_os = "linux", target_env = "gnu", target_arch = "x86_64")))] @@ -204,8 +209,26 @@ impl Operation for ProfileStatusHandler { #[cfg(test)] mod tests { - use super::extract_query_params; - use http::Uri; + use super::{ProfileHandler, ProfileStatusHandler, extract_query_params}; + use crate::admin::router::Operation; + use http::{Extensions, HeaderMap, Uri}; + use hyper::Method; + use matchit::Params; + use s3s::{Body, S3ErrorCode, S3Request}; + + fn build_profile_request(uri: &'static str) -> S3Request { + S3Request { + input: Body::empty(), + method: Method::GET, + uri: Uri::from_static(uri), + headers: HeaderMap::new(), + extensions: Extensions::new(), + credentials: None, + region: None, + service: None, + trailing_headers: None, + } + } #[test] fn test_extract_query_params_decodes_percent_encoded_values() { @@ -217,4 +240,32 @@ mod tests { assert_eq!(params.get("format"), Some(&"flamegraph".to_string())); assert_eq!(params.get("note"), Some(&"a+b value".to_string())); } + + #[tokio::test] + async fn profile_handler_rejects_missing_credentials() { + let result = ProfileHandler {} + .call(build_profile_request("/rustfs/admin/debug/pprof/profile?format=protobuf"), Params::new()) + .await; + let err = match result { + Ok(_) => panic!("profile handler must reject unauthenticated requests"), + Err(err) => err, + }; + + assert_eq!(err.code(), &S3ErrorCode::AccessDenied); + assert_eq!(err.message(), Some("Signature is required")); + } + + #[tokio::test] + async fn profile_status_handler_rejects_missing_credentials() { + let result = ProfileStatusHandler {} + .call(build_profile_request("/rustfs/admin/debug/pprof/status"), Params::new()) + .await; + let err = match result { + Ok(_) => panic!("profile status handler must reject unauthenticated requests"), + Err(err) => err, + }; + + assert_eq!(err.code(), &S3ErrorCode::AccessDenied); + assert_eq!(err.message(), Some("Signature is required")); + } } diff --git a/rustfs/src/admin/handlers/sts.rs b/rustfs/src/admin/handlers/sts.rs index bc72f2a92..b669f8ff4 100644 --- a/rustfs/src/admin/handlers/sts.rs +++ b/rustfs/src/admin/handlers/sts.rs @@ -250,7 +250,12 @@ async fn handle_assume_role( new_cred.parent_user = cred.access_key.clone(); - debug!("AssumeRole get new_cred {:?}", &new_cred); + debug!( + access_key = %new_cred.access_key, + parent_user = %new_cred.parent_user, + expiration = ?new_cred.expiration, + "AssumeRole generated temporary credentials" + ); let updated_at = iam_store .set_temp_user(&new_cred.access_key, &new_cred, None) diff --git a/rustfs/src/admin/handlers/user.rs b/rustfs/src/admin/handlers/user.rs index 18e553744..cd46a1112 100644 --- a/rustfs/src/admin/handlers/user.rs +++ b/rustfs/src/admin/handlers/user.rs @@ -139,6 +139,36 @@ fn imported_service_account_status(status: &str) -> Option { None } +const SERVICE_ACCOUNT_PARENT_SCOPE_ERROR: &str = "service account parent is outside requester scope"; + +fn imported_service_account_parent_allowed(parent: &str, requester: &Credentials, owner: bool) -> bool { + if parent.is_empty() { + return false; + } + + if owner { + return true; + } + + if requester.is_temp() || requester.is_service_account() { + return temp_identity_parent(requester).is_some_and(|requester_parent| requester_parent == parent); + } + + requester.parent_user.is_empty() && requester.access_key == parent +} + +fn imported_service_account_parent_scope_failure( + access_key: &str, + parent: &str, + requester: &Credentials, + owner: bool, +) -> Option { + (!imported_service_account_parent_allowed(parent, requester, owner)).then(|| IAMErrEntity { + name: access_key.to_string(), + error: SERVICE_ACCOUNT_PARENT_SCOPE_ERROR.to_string(), + }) +} + pub struct AddUser {} #[async_trait::async_trait] impl Operation for AddUser { @@ -984,6 +1014,11 @@ impl Operation for ImportIam { return Err(s3_error!(InvalidArgument, "has space be {ak}")); } + if let Some(err) = imported_service_account_parent_scope_failure(&ak, &req.parent, &cred, owner) { + failed.service_accounts.push(err); + continue; + } + let mut update = true; if let Err(e) = iam_store.get_service_account(&req.access_key).await { @@ -1216,8 +1251,9 @@ impl Operation for ImportIam { #[cfg(test)] mod tests { use super::{ - GROUP_POLICY_MAPPING_USER_TYPE, imported_service_account_status, should_check_deny_only, should_reject_group_import_name, - should_restore_group_as_disabled, + GROUP_POLICY_MAPPING_USER_TYPE, SERVICE_ACCOUNT_PARENT_SCOPE_ERROR, imported_service_account_parent_allowed, + imported_service_account_parent_scope_failure, imported_service_account_status, should_check_deny_only, + should_reject_group_import_name, should_restore_group_as_disabled, }; use rustfs_credentials::{Credentials, IAM_POLICY_CLAIM_NAME_SA}; use rustfs_iam::error::Error as IamError; @@ -1337,6 +1373,65 @@ mod tests { assert!(imported_service_account_status("unknown").is_none()); } + #[test] + fn test_import_service_account_parent_rejects_other_parent_for_non_owner() { + let requester = Credentials { + access_key: "delegated-importer".to_string(), + ..Default::default() + }; + + assert!(!imported_service_account_parent_allowed("root-access-key", &requester, false)); + } + + #[test] + fn test_service_account_parent_scope_failure_records_import_error() { + let requester = Credentials { + access_key: "delegated-importer".to_string(), + ..Default::default() + }; + let err = imported_service_account_parent_scope_failure("svc-access-key", "root-access-key", &requester, false) + .expect("non-owner must not import a service account for another parent"); + + assert_eq!(err.name, "svc-access-key"); + assert_eq!(err.error, SERVICE_ACCOUNT_PARENT_SCOPE_ERROR); + assert!( + imported_service_account_parent_scope_failure("svc-access-key", "delegated-importer", &requester, false).is_none() + ); + } + + #[test] + fn test_import_service_account_parent_allows_owner_restore() { + let requester = Credentials { + access_key: "root-access-key".to_string(), + ..Default::default() + }; + + assert!(imported_service_account_parent_allowed("any-imported-parent", &requester, true)); + } + + #[test] + fn test_import_service_account_parent_allows_requester_self_parent() { + let requester = Credentials { + access_key: "delegated-importer".to_string(), + ..Default::default() + }; + + assert!(imported_service_account_parent_allowed("delegated-importer", &requester, false)); + } + + #[test] + fn test_import_service_account_parent_allows_derived_requester_parent() { + let requester = Credentials { + access_key: "derived-access-key".to_string(), + parent_user: "parent-user".to_string(), + session_token: "session-token".to_string(), + ..Default::default() + }; + + assert!(imported_service_account_parent_allowed("parent-user", &requester, false)); + assert!(!imported_service_account_parent_allowed("other-parent", &requester, false)); + } + #[test] fn test_service_account_import_accepts_null_groups_and_epoch_expiration() { let payload = r#"{ diff --git a/rustfs/src/admin/router.rs b/rustfs/src/admin/router.rs index f9d4eb079..37edd3804 100644 --- a/rustfs/src/admin/router.rs +++ b/rustfs/src/admin/router.rs @@ -2331,11 +2331,6 @@ where // Allow unauthenticated access to health check let path = req.uri.path(); - // Profiling endpoints - if req.method == Method::GET && (path == PROFILE_CPU_PATH || path == PROFILE_MEMORY_PATH) { - return Ok(()); - } - // Health check if (req.method == Method::HEAD || req.method == Method::GET) && is_public_health_path(path) { return Ok(()); @@ -3747,6 +3742,28 @@ mod tests { assert_eq!(err.code(), &S3ErrorCode::AccessDenied); } + #[tokio::test] + async fn check_access_rejects_anonymous_profile_request() { + let router: S3Router = S3Router::new(false); + let mut req = S3Request { + input: Body::from(String::new()), + method: Method::GET, + uri: PROFILE_CPU_PATH.parse().expect("uri should parse"), + headers: HeaderMap::new(), + extensions: http::Extensions::new(), + credentials: None, + region: None, + service: None, + trailing_headers: None, + }; + + let err = router + .check_access(&mut req) + .await + .expect_err("anonymous profile request must be denied"); + assert_eq!(err.code(), &S3ErrorCode::AccessDenied); + } + #[test] fn listen_notification_keepalive_plan_defaults_to_space_keepalive() { let uri: Uri = "/demo-bucket?events=s3:ObjectCreated:Put".parse().expect("uri should parse");