From 3bf0d1ebf31e7a27d902bad6e8b234d2987b8ceb Mon Sep 17 00:00:00 2001 From: VibeSWE <338779468+VibeSWE@users.noreply.github.com> Date: Sun, 25 Oct 2026 18:06:53 -0500 Subject: [PATCH 1/2] fix: return errors for malformed request headers and log bodies --- Cargo.lock | 1 + Cargo.toml | 1 + src/alerts/mod.rs | 19 + src/event/format/known_schema.rs | 149 ++- src/handlers/http/alert_target_policy.rs | 4 +- src/handlers/http/alerts.rs | 18 +- src/handlers/http/cluster/mod.rs | 172 ++- src/handlers/http/demo_data.rs | 2 +- src/handlers/http/health_check.rs | 24 +- src/handlers/http/ingest.rs | 167 ++- src/handlers/http/logstream.rs | 93 +- src/handlers/http/middleware.rs | 1060 +++++++++++++++-- .../http/modal/ingest/ingestor_logstream.rs | 6 +- .../http/modal/ingest/ingestor_rbac.rs | 10 +- .../http/modal/ingest/ingestor_role.rs | 4 +- .../http/modal/query/querier_logstream.rs | 217 +++- src/handlers/http/modal/query/querier_rbac.rs | 10 +- src/handlers/http/modal/query/querier_role.rs | 4 +- src/handlers/http/modal/utils/ingest_utils.rs | 2 +- .../http/modal/utils/logstream_utils.rs | 204 +++- src/handlers/http/oidc.rs | 3 +- src/handlers/http/otel_generator.rs | 28 +- src/handlers/http/prism_logstream.rs | 4 +- src/handlers/http/promql.rs | 17 +- src/handlers/http/query.rs | 62 +- src/handlers/http/query_context.rs | 2 +- src/handlers/http/rbac.rs | 109 +- src/handlers/http/role.rs | 68 +- src/handlers/http/targets.rs | 10 +- src/handlers/http/traces.rs | 59 +- src/handlers/http/users/dashboards.rs | 32 +- src/parseable/mod.rs | 2 +- src/prism/logstream/mod.rs | 45 + src/sse/mod.rs | 2 +- src/storage/field_stats.rs | 137 ++- src/tool_executor.rs | 29 +- src/users/dashboard_runtime.rs | 34 +- src/users/dashboard_tools.rs | 26 +- src/utils/header_parsing.rs | 171 +++ src/utils/mod.rs | 300 ++++- 40 files changed, 2916 insertions(+), 391 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 0726e1b08..002c99c52 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -4772,6 +4772,7 @@ dependencies = [ "prometheus", "prometheus-parse", "promql-parser", + "proptest", "prost 0.13.5", "rand 0.8.6", "rayon", diff --git a/Cargo.toml b/Cargo.toml index f8c6459f9..87d216f30 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -221,6 +221,7 @@ anyhow = "1.0" procfs = { version = "0.18.0", default-features = false } [dev-dependencies] +proptest = { version = "=1.9.0", default-features = false, features = ["std"] } rstest = "0.26.1" arrow = "59.2.0" temp-dir = "0.1.14" diff --git a/src/alerts/mod.rs b/src/alerts/mod.rs index 36323f6f0..7c185ff1c 100644 --- a/src/alerts/mod.rs +++ b/src/alerts/mod.rs @@ -1237,6 +1237,8 @@ pub enum AlertError { ValidationFailure(String), #[error(transparent)] MetastoreError(#[from] MetastoreError), + #[error("{0}")] + Header(#[from] crate::utils::header_parsing::ParseHeaderError), } impl actix_web::ResponseError for AlertError { @@ -1266,6 +1268,7 @@ impl actix_web::ResponseError for AlertError { Self::NotPresentInOSS(_) => StatusCode::BAD_REQUEST, Self::MetastoreError(_) => StatusCode::INTERNAL_SERVER_ERROR, Self::OutboundPolicy(_) => StatusCode::BAD_REQUEST, + Self::Header(_) => StatusCode::BAD_REQUEST, } } @@ -1848,3 +1851,19 @@ mod tests { ); } } + +#[cfg(test)] +mod header_error_tests { + use actix_web::ResponseError; + + use super::*; + use crate::utils::header_parsing::ParseHeaderError; + + #[test] + fn header_error_is_bad_request() { + assert_eq!( + AlertError::from(ParseHeaderError::InvalidTenantId).status_code(), + StatusCode::BAD_REQUEST + ); + } +} diff --git a/src/event/format/known_schema.rs b/src/event/format/known_schema.rs index 2ac32a5b4..311bb0c5a 100644 --- a/src/event/format/known_schema.rs +++ b/src/event/format/known_schema.rs @@ -43,6 +43,10 @@ pub enum Error { "Unsupported log format: '{0}'. This format cannot be parsed by the current version. Please create an issue on our GitHub repository (github.com/parseablehq/parseable) with a sample log event, or reach out in the #support-channel on our Slack community for assistance. Include this error message and your log sample to help us improve compatibility." )] Unknown(String), + #[error( + "Event shape is not supported for known log formats. Expected a JSON object or an array of JSON objects." + )] + InvalidShape, } /// Deserializes a string pattern into a compiled Regex @@ -193,6 +197,8 @@ impl EventProcessor { /// # Returns /// * `Ok` - The original JSON will now contain extracted fields /// * `Err(Unacceptable)` - JSON provided is acceptable for the known format + /// * `Err(Unknown)` - `log_source` is not a known format + /// * `Err(InvalidShape)` - JSON is neither an object nor an array pub fn extract_from_inline_log( &self, json: &mut Value, @@ -227,7 +233,9 @@ impl EventProcessor { p_custom_fields.insert(FORMAT_VERIFY_KEY.to_string(), "false".to_string()); } } - _ => unreachable!("We don't accept events of the form: {json}"), + // Scalars (number, string, bool, null) can't carry log fields. + // The value is deliberately not echoed back in the error. + _ => return Err(Error::InvalidShape), } Ok(fields) @@ -470,6 +478,145 @@ mod tests { ); } + #[test] + fn test_extract_from_inline_log_rejects_scalars() { + let processor = EventProcessor::new(TEST_CONFIG); + + for scalar in [ + json!(42), + json!(1.5), + json!("[ERROR] [2023-10-10T13:55:36Z] do-not-echo-this"), + json!(true), + Value::Null, + ] { + let mut json_value = scalar.clone(); + let mut p_custom_fields = HashMap::new(); + let result = processor.extract_from_inline_log( + &mut json_value, + &mut p_custom_fields, + "custom_app_log", + Some("raw_log"), + ); + + let err = result.expect_err("scalar events must be rejected"); + assert!(matches!(err, Error::InvalidShape), "{scalar} gave {err:?}"); + assert!(!err.to_string().contains("do-not-echo-this")); + assert_eq!(json_value, scalar, "scalar event must be left untouched"); + assert!(p_custom_fields.is_empty()); + assert_eq!( + actix_web::ResponseError::status_code( + &crate::handlers::http::ingest::PostError::from(err) + ), + actix_web::http::StatusCode::BAD_REQUEST + ); + } + } + + #[test] + fn test_extract_from_inline_log_unknown_source_before_shape() { + let processor = EventProcessor::new(TEST_CONFIG); + let mut json_value = json!(42); + let mut p_custom_fields = HashMap::new(); + + let result = processor.extract_from_inline_log( + &mut json_value, + &mut p_custom_fields, + "nonexistent_format", + Some("raw_log"), + ); + + assert!(matches!(result, Err(Error::Unknown(name)) if name == "nonexistent_format")); + } + + #[test] + fn test_extract_from_inline_log_object_via_processor() { + let processor = EventProcessor::new(TEST_CONFIG); + let mut json_value = json!({ + "raw_log": "[ERROR] [2023-10-10T13:55:36Z] Failed to connect to database" + }); + let mut p_custom_fields = HashMap::new(); + + let fields = processor + .extract_from_inline_log( + &mut json_value, + &mut p_custom_fields, + "custom_app_log", + Some("raw_log"), + ) + .unwrap(); + + assert!(fields.contains("level")); + assert_eq!(json_value["level"], "ERROR"); + assert!(p_custom_fields.is_empty()); + } + + #[test] + fn test_extract_from_inline_log_object_no_match_marks_unverified() { + let processor = EventProcessor::new(TEST_CONFIG); + let mut json_value = json!({ "raw_log": "not a known format" }); + let mut p_custom_fields = HashMap::new(); + + let fields = processor + .extract_from_inline_log( + &mut json_value, + &mut p_custom_fields, + "custom_app_log", + Some("raw_log"), + ) + .unwrap(); + + assert!(fields.is_empty()); + assert_eq!( + p_custom_fields.get(FORMAT_VERIFY_KEY).map(String::as_str), + Some("false") + ); + } + + #[test] + fn test_extract_from_inline_log_array_skips_non_objects() { + let processor = EventProcessor::new(TEST_CONFIG); + let mut json_value = json!([ + 42, + "plain string", + null, + { "raw_log": "[INFO] [2023-10-10T13:55:40Z] Application started" } + ]); + let mut p_custom_fields = HashMap::new(); + + let fields = processor + .extract_from_inline_log( + &mut json_value, + &mut p_custom_fields, + "custom_app_log", + Some("raw_log"), + ) + .unwrap(); + + assert!(fields.contains("message")); + assert_eq!(json_value[0], json!(42)); + assert_eq!(json_value[3]["level"], "INFO"); + assert!(p_custom_fields.is_empty()); + } + + #[test] + fn test_extract_from_inline_log_empty_array() { + let processor = EventProcessor::new(TEST_CONFIG); + let mut json_value = json!([]); + let mut p_custom_fields = HashMap::new(); + + let fields = processor + .extract_from_inline_log( + &mut json_value, + &mut p_custom_fields, + "custom_app_log", + Some("raw_log"), + ) + .unwrap(); + + assert!(fields.is_empty()); + assert!(p_custom_fields.is_empty()); + } + #[test] fn test_unknown_log_format() { let processor = EventProcessor::new(TEST_CONFIG); diff --git a/src/handlers/http/alert_target_policy.rs b/src/handlers/http/alert_target_policy.rs index 78a1d2160..fd2baeb79 100644 --- a/src/handlers/http/alert_target_policy.rs +++ b/src/handlers/http/alert_target_policy.rs @@ -13,7 +13,7 @@ use crate::utils::get_tenant_id_from_request; use actix_web::HttpRequest; pub async fn get(req: HttpRequest) -> Result { - let tenant_id = get_tenant_id_from_request(&req); + let tenant_id = get_tenant_id_from_request(&req)?; let policy = active_policy(&tenant_id).await?; Ok(web::Json(policy)) } @@ -23,7 +23,7 @@ pub async fn put( Json(policy): Json, ) -> Result { // validate before replacing so a bad policy never becomes active - let tenant_id = get_tenant_id_from_request(&req); + let tenant_id = get_tenant_id_from_request(&req)?; validate_policy(&policy)?; replace_policy(&tenant_id, policy.clone()).await?; Ok(web::Json(policy)) diff --git a/src/handlers/http/alerts.rs b/src/handlers/http/alerts.rs index 650f6728a..19e57d709 100644 --- a/src/handlers/http/alerts.rs +++ b/src/handlers/http/alerts.rs @@ -260,7 +260,7 @@ pub async fn post( req: HttpRequest, Json(alert): Json, ) -> Result { - let tenant_id = get_tenant_id_from_request(&req); + let tenant_id = get_tenant_id_from_request(&req)?; let session_key = extract_session_key_from_req(&req)?; Ok(web::Json( post_internal(alert, &session_key, &tenant_id).await?, @@ -356,7 +356,7 @@ pub async fn post_internal( pub async fn get(req: HttpRequest, alert_id: Path) -> Result { let session_key = extract_session_key_from_req(&req)?; let alert_id = alert_id.into_inner(); - let tenant_id = get_tenant_id_from_request(&req); + let tenant_id = get_tenant_id_from_request(&req)?; Ok(web::Json( get_internal(&session_key, alert_id, &tenant_id).await?, )) @@ -388,7 +388,7 @@ pub async fn delete(req: HttpRequest, alert_id: Path) -> Result NotificationState::Notify, "indefinite" => NotificationState::Mute("indefinite".into()), @@ -492,7 +492,7 @@ pub async fn disable_alert( ) -> Result { let session_key = extract_session_key_from_req(&req)?; let alert_id = alert_id.into_inner(); - let tenant_id = get_tenant_id_from_request(&req); + let tenant_id = get_tenant_id_from_request(&req)?; Ok(web::Json( disable_alert_internal(&session_key, alert_id, &tenant_id).await?, )) @@ -534,7 +534,7 @@ pub async fn enable_alert( ) -> Result { let session_key = extract_session_key_from_req(&req)?; let alert_id = alert_id.into_inner(); - let tenant_id = get_tenant_id_from_request(&req); + let tenant_id = get_tenant_id_from_request(&req)?; Ok(web::Json( enable_alert_internal(&session_key, alert_id, &tenant_id).await?, )) @@ -591,7 +591,7 @@ pub async fn modify_alert( let session_key = extract_session_key_from_req(&req)?; let alert_id = alert_id.into_inner(); let _lifecycle = crate::alerts::promql_alerts::lock_lifecycle(alert_id).await; - let tenant_id = get_tenant_id_from_request(&req); + let tenant_id = get_tenant_id_from_request(&req)?; // Get alerts manager reference without holding the global lock let alerts = { let guard = ALERTS.read().await; @@ -688,7 +688,7 @@ pub async fn evaluate_alert( ) -> Result { let session_key = extract_session_key_from_req(&req)?; let alert_id = alert_id.into_inner(); - let tenant_id = get_tenant_id_from_request(&req); + let tenant_id = get_tenant_id_from_request(&req)?; Ok(Json( evaluate_alert_internal(&session_key, alert_id, &tenant_id).await?, )) @@ -724,7 +724,7 @@ pub async fn evaluate_alert_internal( } pub async fn list_tags(req: HttpRequest) -> Result { - let tenant_id = get_tenant_id_from_request(&req); + let tenant_id = get_tenant_id_from_request(&req)?; Ok(web::Json(list_tags_internal(&tenant_id).await?)) } diff --git a/src/handlers/http/cluster/mod.rs b/src/handlers/http/cluster/mod.rs index 8ecc5538f..ef7c6e048 100644 --- a/src/handlers/http/cluster/mod.rs +++ b/src/handlers/http/cluster/mod.rs @@ -52,6 +52,7 @@ use crate::rbac::role::model::Role; use crate::rbac::user::User; use crate::stats::Stats; use crate::storage::{ObjectStorageError, ObjectStoreFormat}; +use crate::utils::header_parsing::{HeaderBuildError, generated_text_reqwest_header_value}; use crate::utils::{create_intracluster_auth_headermap, get_tenant_id_from_request}; use super::base_path_without_preceding_slash; @@ -387,6 +388,22 @@ where Ok(()) } +pub const INTRA_CLUSTER_USERID: &str = "intra-cluster-userid"; + +/// The `intra-cluster-userid` that [`sync_streams_with_ingestors`] adds when the +/// forwarded headers don't already carry one: the configured username. +/// `put_stream` calls this before mutating the stream so that a username that +/// can't be sent fails the request before any local change or send. +pub fn fallback_intracluster_userid( + has_userid: bool, + username: &str, +) -> Result, HeaderBuildError> { + if has_userid { + return Ok(None); + } + generated_text_reqwest_header_value(username, "configured username").map(Some) +} + // forward the create/update stream request to all ingestors to keep them in sync pub async fn sync_streams_with_ingestors( headers: HeaderMap, @@ -405,6 +422,16 @@ pub async fn sync_streams_with_ingestors( } } + if let Some(userid) = fallback_intracluster_userid( + reqwest_headers.contains_key(INTRA_CLUSTER_USERID), + &PARSEABLE.options.username, + )? { + reqwest_headers.insert( + reqwest::header::HeaderName::from_static(INTRA_CLUSTER_USERID), + userid, + ); + } + let body_clone = body.clone(); let stream_name = stream_name.to_string(); let reqwest_headers_clone = reqwest_headers.clone(); @@ -417,14 +444,7 @@ pub async fn sync_streams_with_ingestors( base_path_without_preceding_slash(), stream_name ); - let mut headers = reqwest_headers_clone.clone(); - - if !headers.contains_key("intra-cluster-userid") { - headers.insert( - reqwest::header::HeaderName::from_static("intra-cluster-userid"), - reqwest::header::HeaderValue::from_str(&PARSEABLE.options.username).unwrap(), - ); - } + let headers = reqwest_headers_clone.clone(); let body = body_clone.clone(); async move { let res = INTRA_CLUSTER_CLIENT @@ -490,6 +510,7 @@ pub async fn sync_users_with_roles_with_ingestors( let headermap = create_intracluster_auth_headermap(&headers, &ingestor.token, &caller_userid); async move { + let headermap = headermap?; let res = INTRA_CLUSTER_CLIENT .patch(url) .headers(headermap) @@ -540,6 +561,7 @@ pub async fn sync_user_deletion_with_ingestors( let headermap = create_intracluster_auth_headermap(&headers, &ingestor.token, &caller_userid); async move { + let headermap = headermap?; let res = INTRA_CLUSTER_CLIENT .delete(url) // .header(header::AUTHORIZATION, &ingestor.token) @@ -602,6 +624,7 @@ pub async fn sync_user_creation( let user_data = user_data.clone(); async move { + let headermap = headermap?; let res = INTRA_CLUSTER_CLIENT .post(url) .headers(headermap) @@ -638,7 +661,7 @@ pub async fn sync_password_reset_with_ingestors( caller_userid: &str, ) -> Result<(), RBACError> { let userid = username.to_owned(); - let tenant_id = get_tenant_id_from_request(&req); + let tenant_id = get_tenant_id_from_request(&req)?; let caller_userid = caller_userid.to_owned(); let headers = req.headers().clone(); for_each_live_node(&tenant_id, move |ingestor| { @@ -651,6 +674,7 @@ pub async fn sync_password_reset_with_ingestors( let headermap = create_intracluster_auth_headermap(&headers, &ingestor.token, &caller_userid); async move { + let headermap = headermap?; let res = INTRA_CLUSTER_CLIENT .post(url) .headers(headermap) @@ -702,6 +726,7 @@ pub async fn sync_role_update( let headermap = create_intracluster_auth_headermap(&headers, &node.token, &userid); let tenant_id = tenant.clone(); async move { + let headermap = headermap?; let res = INTRA_CLUSTER_CLIENT .put(url) .headers(headermap) @@ -750,6 +775,7 @@ pub async fn sync_role_delete( let headermap = create_intracluster_auth_headermap(&headers, &node.token, &userid); async move { + let headermap = headermap?; let res = INTRA_CLUSTER_CLIENT .delete(url) .headers(headermap) @@ -939,7 +965,7 @@ pub async fn send_retention_cleanup_request( /// Fetches cluster information for all nodes (ingestor, indexer, querier and prism) pub async fn get_cluster_info(req: HttpRequest) -> Result { - let tenant_id = get_tenant_id_from_request(&req); + let tenant_id = get_tenant_id_from_request(&req)?; Ok(actix_web::HttpResponse::Ok().json(get_cluster_info_internal(&tenant_id).await?)) } @@ -1092,7 +1118,7 @@ async fn fetch_nodes_info( } pub async fn get_cluster_metrics(req: HttpRequest) -> Result { - let tenant_id = get_tenant_id_from_request(&req); + let tenant_id = get_tenant_id_from_request(&req)?; Ok(actix_web::HttpResponse::Ok().json(get_cluster_metrics_internal(&tenant_id).await?)) } @@ -1922,6 +1948,19 @@ pub async fn mark_querier_available(domain_name: &str) { } } +/// Extracts the user id from a `Basic ` node token as a +/// header value. The token is server-generated, so any failure is internal. +fn basic_token_userid_header( + token: &str, +) -> Result { + let internal = || HeaderBuildError::Internal("querier token"); + let encoded = token.split(' ').nth(1).ok_or_else(internal)?; + let decoded = BASE64_STANDARD.decode(encoded).map_err(|_| internal())?; + let decoded = String::from_utf8(decoded).map_err(|_| internal())?; + let (user, _) = decoded.split_once(':').ok_or_else(internal)?; + generated_text_reqwest_header_value(user, "querier token") +} + pub async fn send_query_request( auth_token: Option, query_request: &Query, @@ -1958,20 +1997,30 @@ pub async fn send_query_request( } } } else { - map.insert( - reqwest::header::AUTHORIZATION, - reqwest::header::HeaderValue::from_str(&querier.token).unwrap(), - ); + match generated_text_reqwest_header_value(&querier.token, "querier token") { + Ok(token) => { + map.insert(reqwest::header::AUTHORIZATION, token); + } + Err(err) => { + mark_querier_available(&domain_name).await; + return Err(err.into()); + } + } }; if map.get("intra-cluster-userid").is_none() { - let token: Vec<&str> = querier.token().split(' ').collect(); - let decode = BASE64_STANDARD.decode(token[1]).unwrap(); - let user = String::from_utf8(decode).unwrap(); - let user = user.split_once(':').unwrap(); - map.insert( - reqwest::header::HeaderName::from_static("intra-cluster-userid"), - reqwest::header::HeaderValue::from_str(user.0).unwrap(), - ); + match basic_token_userid_header(querier.token()) { + Ok(userid) => { + map.insert( + reqwest::header::HeaderName::from_static("intra-cluster-userid"), + userid, + ); + } + Err(err) => { + mark_querier_available(&domain_name).await; + error!("failed to derive intra-cluster user id from querier token"); + return Err(err.into()); + } + } } let res = match INTRA_CLUSTER_CLIENT .post(uri) @@ -2017,3 +2066,80 @@ pub async fn send_query_request( Err(QueryError::JsonParse(err_text)) } } + +#[cfg(test)] +mod header_build_tests { + use super::*; + + #[test] + fn basic_token_userid_header_extracts_user() { + let token = format!("Basic {}", BASE64_STANDARD.encode("alice:secret")); + assert_eq!(basic_token_userid_header(&token).unwrap(), "alice"); + } + + #[test] + fn basic_token_userid_header_rejects_malformed_tokens_as_internal() { + let not_utf8 = format!("Basic {}", BASE64_STANDARD.encode([0xff, 0xfe])); + let no_colon = format!("Basic {}", BASE64_STANDARD.encode("alice")); + let bad_user = format!("Basic {}", BASE64_STANDARD.encode("al\nice:secret")); + // `HeaderValue::from_str` accepts these but the receiver's `to_str` doesn't + let unicode_user = format!("Basic {}", BASE64_STANDARD.encode("josé:secret")); + let cjk_user = format!("Basic {}", BASE64_STANDARD.encode("用户:secret")); + let control_user = format!("Basic {}", BASE64_STANDARD.encode("al\u{1}ice:secret")); + for token in [ + "Basic", + "Basic !!!", + not_utf8.as_str(), + no_colon.as_str(), + bad_user.as_str(), + unicode_user.as_str(), + cjk_user.as_str(), + control_user.as_str(), + ] { + let err = basic_token_userid_header(token).unwrap_err(); + assert!(matches!(err, HeaderBuildError::Internal("querier token"))); + assert!(!err.to_string().contains("secret")); + assert_eq!( + actix_web::ResponseError::status_code(&QueryError::from(err)), + actix_web::http::StatusCode::INTERNAL_SERVER_ERROR + ); + } + } + + #[test] + fn fallback_intracluster_userid_uses_configured_username() { + let userid = fallback_intracluster_userid(false, "admin").unwrap(); + assert_eq!(userid.unwrap(), "admin"); + } + + #[test] + fn fallback_intracluster_userid_defers_to_existing_userid() { + // an already-present user id wins, so the username is never encoded + assert!( + fallback_intracluster_userid(true, "josé") + .unwrap() + .is_none() + ); + assert!( + fallback_intracluster_userid(true, "ad\nmin") + .unwrap() + .is_none() + ); + } + + #[test] + fn fallback_intracluster_userid_rejects_unreadable_username_as_internal() { + for username in ["ad\nmin", "ad\rmin", "ad\u{7f}min", "josé", "用户"] { + let err = fallback_intracluster_userid(false, username).unwrap_err(); + assert!(matches!( + err, + HeaderBuildError::Internal("configured username") + )); + assert!(!err.to_string().contains(username)); + assert_eq!( + actix_web::ResponseError::status_code(&StreamError::from(err)), + actix_web::http::StatusCode::INTERNAL_SERVER_ERROR + ); + } + } +} diff --git a/src/handlers/http/demo_data.rs b/src/handlers/http/demo_data.rs index 06514056a..2df4d8373 100644 --- a/src/handlers/http/demo_data.rs +++ b/src/handlers/http/demo_data.rs @@ -53,7 +53,7 @@ pub async fn get_demo_data(req: HttpRequest) -> Result let password = PARSEABLE.options.password.clone(); let scheme = PARSEABLE.options.get_scheme(); let standalone_url = format!("{scheme}://{}", PARSEABLE.options.address); - let tenant_id = get_tenant_id_from_request(&req); + let tenant_id = get_tenant_id_from_request(&req)?; match action.as_str() { "ingest" => match PARSEABLE.options.mode { Mode::All => { diff --git a/src/handlers/http/health_check.rs b/src/handlers/http/health_check.rs index 7ea5eea32..3f06d8c67 100644 --- a/src/handlers/http/health_check.rs +++ b/src/handlers/http/health_check.rs @@ -110,7 +110,10 @@ async fn perform_object_store_sync() { } pub async fn readiness(req: HttpRequest) -> HttpResponse { - let tenant_id = get_tenant_id_from_request(&req); + let tenant_id = match get_tenant_id_from_request(&req) { + Ok(tenant_id) => tenant_id, + Err(error) => return HttpResponse::BadRequest().body(error.to_string()), + }; HttpResponse::new(readiness_internal(&tenant_id).await) } @@ -128,3 +131,22 @@ pub async fn readiness_internal(tenant_id: &Option) -> StatusCode { StatusCode::SERVICE_UNAVAILABLE } } + +#[cfg(test)] +mod header_error_tests { + use actix_web::{http::header::HeaderValue, test::TestRequest}; + + use super::*; + use crate::handlers::TENANT_ID; + + #[actix_web::test] + async fn readiness_rejects_malformed_tenant_with_400() { + let req = TestRequest::default() + .insert_header(( + TENANT_ID, + HeaderValue::from_bytes(b"t\xff").expect("obs-text is a valid header value"), + )) + .to_http_request(); + assert_eq!(readiness(req).await.status(), StatusCode::BAD_REQUEST); + } +} diff --git a/src/handlers/http/ingest.rs b/src/handlers/http/ingest.rs index 2f79ba105..84a0327f6 100644 --- a/src/handlers/http/ingest.rs +++ b/src/handlers/http/ingest.rs @@ -49,7 +49,7 @@ use crate::otel::traces::OTEL_TRACES_KNOWN_FIELD_LIST; use crate::parseable::{PARSEABLE, StreamNotFound}; use crate::storage::{ObjectStorageError, StreamType}; use crate::utils::get_tenant_id_from_request; -use crate::utils::header_parsing::ParseHeaderError; +use crate::utils::header_parsing::{ParseHeaderError, optional_header_str}; use crate::utils::json::{flatten::JsonFlattenError, strict::StrictValue}; use super::logstream::error::{CreateStreamError, StreamError}; @@ -70,11 +70,11 @@ pub async fn ingest( req: HttpRequest, Json(json): Json, ) -> Result { - let Some(stream_name) = req.headers().get(STREAM_NAME_HEADER_KEY) else { + let Some(stream_name) = optional_header_str(req.headers(), STREAM_NAME_HEADER_KEY)? else { return Err(PostError::Header(ParseHeaderError::MissingStreamName)); }; - let tenant_id = get_tenant_id_from_request(&req); - let stream_name = stream_name.to_str().unwrap().to_owned(); + let stream_name = stream_name.to_owned(); + let tenant_id = get_tenant_id_from_request(&req)?; let internal_stream_names = PARSEABLE.streams.list_internal_streams(&tenant_id); if internal_stream_names.contains(&stream_name) { return Err(PostError::InternalStream(stream_name)); @@ -229,18 +229,18 @@ pub async fn setup_otel_stream( dataset_tags: Vec, dataset_labels: Vec, ) -> Result<(String, LogSource, LogSourceEntry, Option), PostError> { - let Some(stream_name) = req.headers().get(STREAM_NAME_HEADER_KEY) else { + let Some(stream_name) = optional_header_str(req.headers(), STREAM_NAME_HEADER_KEY)? else { return Err(PostError::Header(ParseHeaderError::MissingStreamName)); }; - let stream_name = stream_name.to_str().unwrap().to_owned(); + let stream_name = stream_name.to_owned(); let log_source_entry = LogSourceEntry::new( log_source.clone(), known_fields.iter().map(|&s| s.to_string()).collect(), ); - let tenant_id = get_tenant_id_from_request(req); + let tenant_id = get_tenant_id_from_request(req)?; PARSEABLE .create_stream_if_not_exists( &stream_name, @@ -385,7 +385,7 @@ pub async fn post_event( Json(json): Json, ) -> Result { let stream_name = logstream.into_inner(); - let tenant_id = get_tenant_id_from_request(&req); + let tenant_id = get_tenant_id_from_request(&req)?; let internal_stream_names = PARSEABLE.streams.list_internal_streams(&tenant_id); if internal_stream_names.contains(&stream_name) { return Err(PostError::InternalStream(stream_name)); @@ -560,8 +560,14 @@ impl actix_web::ResponseError for PostError { | InvalidQueryParameter | MissingQueryParameter | CreateStream(CreateStreamError::StreamNameValidation(_)) + | StreamError(super::logstream::error::StreamError::Header(_)) + | DashboardError(super::users::dashboards::DashboardError::Header(_)) | OtelNotSupported(_) => StatusCode::BAD_REQUEST, + StreamError(super::logstream::error::StreamError::HeaderBuild(e)) => { + actix_web::ResponseError::status_code(e) + } + Event(_) | CreateStream(_) | CustomError(_) @@ -1130,3 +1136,148 @@ mod tests { ); } } + +#[cfg(test)] +mod header_error_tests { + use actix_web::{ + ResponseError, + http::{StatusCode, header::HeaderValue}, + test::TestRequest, + web::Json, + }; + + use super::*; + use crate::handlers::TENANT_ID; + use crate::utils::header_parsing::HeaderBuildError; + + fn non_ascii() -> HeaderValue { + HeaderValue::from_bytes(b"name\xff").expect("obs-text is a valid header value") + } + + fn empty_body() -> Json { + Json(serde_json::from_str("{}").expect("empty object is strict JSON")) + } + + #[actix_web::test] + async fn ingest_rejects_non_ascii_stream_header_with_400() { + let req = TestRequest::default() + .insert_header((STREAM_NAME_HEADER_KEY, non_ascii())) + .to_http_request(); + let err = ingest(req, empty_body()).await.unwrap_err(); + assert!(matches!( + err, + PostError::Header(ParseHeaderError::InvalidValue) + )); + assert_eq!(err.status_code(), StatusCode::BAD_REQUEST); + } + + #[actix_web::test] + async fn ingest_missing_stream_header_is_still_missing() { + let req = TestRequest::default().to_http_request(); + let err = ingest(req, empty_body()).await.unwrap_err(); + assert!(matches!( + err, + PostError::Header(ParseHeaderError::MissingStreamName) + )); + } + + #[actix_web::test] + async fn ingest_rejects_malformed_tenant_with_400() { + let req = TestRequest::default() + .insert_header((STREAM_NAME_HEADER_KEY, "app")) + .insert_header((TENANT_ID, non_ascii())) + .to_http_request(); + let err = ingest(req, empty_body()).await.unwrap_err(); + assert!(matches!( + err, + PostError::Header(ParseHeaderError::InvalidTenantId) + )); + assert_eq!(err.status_code(), StatusCode::BAD_REQUEST); + } + + #[actix_web::test] + async fn post_event_rejects_malformed_tenant_with_400() { + let req = TestRequest::default() + .insert_header((TENANT_ID, non_ascii())) + .to_http_request(); + let err = post_event(req, Path::from("app".to_owned()), empty_body()) + .await + .unwrap_err(); + assert!(matches!( + err, + PostError::Header(ParseHeaderError::InvalidTenantId) + )); + } + + #[actix_web::test] + async fn setup_otel_stream_rejects_bad_stream_and_tenant_headers() { + let req = TestRequest::default() + .insert_header((STREAM_NAME_HEADER_KEY, non_ascii())) + .to_http_request(); + let Err(err) = setup_otel_stream( + &req, + LogSource::OtelLogs, + &[], + TelemetryType::Logs, + vec![], + vec![], + ) + .await + else { + panic!("bad header must be rejected"); + }; + assert!(matches!( + err, + PostError::Header(ParseHeaderError::InvalidValue) + )); + + let req = TestRequest::default() + .insert_header((STREAM_NAME_HEADER_KEY, "otel")) + .insert_header((TENANT_ID, non_ascii())) + .to_http_request(); + let Err(err) = setup_otel_stream( + &req, + LogSource::OtelLogs, + &[], + TelemetryType::Logs, + vec![], + vec![], + ) + .await + else { + panic!("bad header must be rejected"); + }; + assert!(matches!( + err, + PostError::Header(ParseHeaderError::InvalidTenantId) + )); + } + + #[test] + fn nested_header_errors_keep_their_status() { + let stream_header = + PostError::StreamError(StreamError::Header(ParseHeaderError::InvalidTenantId)); + assert_eq!(stream_header.status_code(), StatusCode::BAD_REQUEST); + + let stream_input = PostError::StreamError(StreamError::HeaderBuild( + HeaderBuildError::Input(ParseHeaderError::InvalidTenantId), + )); + assert_eq!(stream_input.status_code(), StatusCode::BAD_REQUEST); + + let stream_internal = PostError::StreamError(StreamError::HeaderBuild( + HeaderBuildError::Internal("user id"), + )); + assert_eq!( + stream_internal.status_code(), + StatusCode::INTERNAL_SERVER_ERROR + ); + + let dashboard_header = + PostError::DashboardError(DashboardError::Header(ParseHeaderError::InvalidTenantId)); + assert_eq!(dashboard_header.status_code(), StatusCode::BAD_REQUEST); + + // unrelated nested errors keep their existing 500 mapping + let other = PostError::StreamError(StreamError::UninitializedLogstream); + assert_eq!(other.status_code(), StatusCode::INTERNAL_SERVER_ERROR); + } +} diff --git a/src/handlers/http/logstream.rs b/src/handlers/http/logstream.rs index 82c58bd70..08df8e953 100644 --- a/src/handlers/http/logstream.rs +++ b/src/handlers/http/logstream.rs @@ -57,7 +57,7 @@ pub async fn delete( ) -> Result { let stream_name = logstream.into_inner(); // Error out if stream doesn't exist in memory, or in the case of query node, in storage as well - let tenant_id = get_tenant_id_from_request(&req); + let tenant_id = get_tenant_id_from_request(&req)?; if !PARSEABLE .check_or_load_stream(&stream_name, &tenant_id) .await @@ -99,7 +99,7 @@ pub async fn list(req: HttpRequest) -> Result { let key = extract_session_key_from_req(&req) .map_err(|err| StreamError::Anyhow(anyhow::Error::msg(err.to_string())))?; - let tenant_id = get_tenant_id_from_request(&req); + let tenant_id = get_tenant_id_from_request(&req)?; // list all streams from storage let res = PARSEABLE .metastore @@ -292,7 +292,7 @@ pub async fn get_schema( query: Query, ) -> Result { let stream_name = logstream.into_inner(); - let tenant_id = get_tenant_id_from_request(&req); + let tenant_id = get_tenant_id_from_request(&req)?; let schema = get_schema_internal(&stream_name, &tenant_id).await?; Ok(schema_response(&schema, query.format)) } @@ -329,7 +329,7 @@ pub async fn put_stream( body: Bytes, ) -> Result { let stream_name = logstream.into_inner(); - let tenant_id = get_tenant_id_from_request(&req); + let tenant_id = get_tenant_id_from_request(&req)?; PARSEABLE .create_update_stream(req.headers(), &body, &stream_name, &tenant_id) @@ -343,7 +343,7 @@ pub async fn get_retention( stream_name: Path, ) -> Result { let stream_name = stream_name.into_inner(); - let tenant_id = get_tenant_id_from_request(&req); + let tenant_id = get_tenant_id_from_request(&req)?; let retention = get_retention_internal(&stream_name, &tenant_id).await?; Ok((web::Json(retention), StatusCode::OK)) } @@ -372,7 +372,7 @@ pub async fn put_retention( Json(retention): Json, ) -> Result { let stream_name = stream_name.into_inner(); - let tenant_id = get_tenant_id_from_request(&req); + let tenant_id = get_tenant_id_from_request(&req)?; // For query mode, if the stream not found in memory map, //check if it exists in the storage //create stream and schema from storage @@ -433,7 +433,7 @@ pub async fn get_stats( logstream: Path, ) -> Result { let stream_name = logstream.into_inner(); - let tenant_id = get_tenant_id_from_request(&req); + let tenant_id = get_tenant_id_from_request(&req)?; // For query mode, if the stream not found in memory map, //check if it exists in the storage //create stream and schema from storage @@ -504,7 +504,7 @@ pub async fn get_stream_info( logstream: Path, ) -> Result { let stream_name = logstream.into_inner(); - let tenant_id = get_tenant_id_from_request(&req); + let tenant_id = get_tenant_id_from_request(&req)?; // For query mode, if the stream not found in memory map, //check if it exists in the storage //create stream and schema from storage @@ -566,7 +566,7 @@ pub async fn put_stream_hot_tier( Json(mut hottier): Json, ) -> Result { let stream_name = logstream.into_inner(); - let tenant_id = get_tenant_id_from_request(&req); + let tenant_id = get_tenant_id_from_request(&req)?; let current_span = tracing::Span::current(); current_span .record("stream", tracing::field::display(&stream_name)) @@ -642,7 +642,7 @@ pub async fn get_stream_hot_tier( logstream: Path, ) -> Result { let stream_name = logstream.into_inner(); - let tenant_id = get_tenant_id_from_request(&req); + let tenant_id = get_tenant_id_from_request(&req)?; tracing::Span::current() .record("stream", tracing::field::display(&stream_name)) .record("tenant", tracing::field::debug(&tenant_id)); @@ -679,7 +679,7 @@ pub async fn delete_stream_hot_tier( logstream: Path, ) -> Result { let stream_name = logstream.into_inner(); - let tenant_id = get_tenant_id_from_request(&req); + let tenant_id = get_tenant_id_from_request(&req)?; let current_span = tracing::Span::current(); current_span .record("stream", tracing::field::display(&stream_name)) @@ -755,6 +755,7 @@ pub mod error { parseable::StreamNotFound, storage::ObjectStorageError, tenants::TenantNotFound, + utils::header_parsing::{HeaderBuildError, ParseHeaderError}, validator::error::{ AlertValidationError, HotTierValidationError, StreamNameValidationError, }, @@ -829,6 +830,10 @@ pub mod error { MetastoreError(#[from] MetastoreError), #[error("{0}")] TenantNotFoundError(#[from] TenantNotFound), + #[error("{0}")] + Header(#[from] ParseHeaderError), + #[error("{0}")] + HeaderBuild(#[from] HeaderBuildError), } impl actix_web::ResponseError for StreamError { @@ -868,6 +873,8 @@ pub mod error { StreamError::HotTierError(_) => StatusCode::INTERNAL_SERVER_ERROR, StreamError::InvalidQueryParameter(_) => StatusCode::BAD_REQUEST, StreamError::MetastoreError(e) => e.status_code(), + StreamError::Header(_) => StatusCode::BAD_REQUEST, + StreamError::HeaderBuild(e) => actix_web::ResponseError::status_code(e), } } @@ -1107,7 +1114,8 @@ mod tests { #[actix_web::test] async fn header_without_log_source() { let req = TestRequest::default().to_http_request(); - let PutStreamHeaders { log_source, .. } = req.headers().into(); + let PutStreamHeaders { log_source, .. } = + PutStreamHeaders::try_from(req.headers()).unwrap(); assert_eq!(log_source, LogSource::Json); } @@ -1116,19 +1124,22 @@ mod tests { let mut req = TestRequest::default() .insert_header(("X-P-Log-Source", "pmeta")) .to_http_request(); - let PutStreamHeaders { log_source, .. } = req.headers().into(); + let PutStreamHeaders { log_source, .. } = + PutStreamHeaders::try_from(req.headers()).unwrap(); assert_eq!(log_source, LogSource::Pmeta); req = TestRequest::default() .insert_header(("X-P-Log-Source", "otel-logs")) .to_http_request(); - let PutStreamHeaders { log_source, .. } = req.headers().into(); + let PutStreamHeaders { log_source, .. } = + PutStreamHeaders::try_from(req.headers()).unwrap(); assert_eq!(log_source, LogSource::OtelLogs); req = TestRequest::default() .insert_header(("X-P-Log-Source", "kinesis")) .to_http_request(); - let PutStreamHeaders { log_source, .. } = req.headers().into(); + let PutStreamHeaders { log_source, .. } = + PutStreamHeaders::try_from(req.headers()).unwrap(); assert_eq!(log_source, LogSource::Kinesis); } @@ -1137,10 +1148,60 @@ mod tests { let req = TestRequest::default() .insert_header(("X-P-Log-Source", "teststream")) .to_http_request(); - let PutStreamHeaders { log_source, .. } = req.headers().into(); + let PutStreamHeaders { log_source, .. } = + PutStreamHeaders::try_from(req.headers()).unwrap(); matches!( log_source, LogSource::Custom(src) if src == "teststream" ); } } + +#[cfg(test)] +mod header_error_tests { + use actix_web::{ + ResponseError, + http::{StatusCode, header::HeaderValue}, + test::TestRequest, + web::Path, + }; + + use super::error::StreamError; + use crate::handlers::TENANT_ID; + use crate::utils::header_parsing::{HeaderBuildError, ParseHeaderError}; + + #[test] + fn header_errors_map_to_expected_status() { + assert_eq!( + StreamError::from(ParseHeaderError::InvalidTenantId).status_code(), + StatusCode::BAD_REQUEST + ); + assert_eq!( + StreamError::from(HeaderBuildError::Input(ParseHeaderError::InvalidTenantId)) + .status_code(), + StatusCode::BAD_REQUEST + ); + assert_eq!( + StreamError::from(HeaderBuildError::Internal("user id")).status_code(), + StatusCode::INTERNAL_SERVER_ERROR + ); + } + + #[actix_web::test] + async fn delete_rejects_malformed_tenant_before_touching_storage() { + let req = TestRequest::default() + .insert_header(( + TENANT_ID, + HeaderValue::from_bytes(b"t\xff").expect("obs-text is a valid header value"), + )) + .to_http_request(); + let Err(err) = super::delete(req, Path::from("app".to_owned())).await else { + panic!("malformed tenant must be rejected"); + }; + assert!(matches!( + err, + StreamError::Header(ParseHeaderError::InvalidTenantId) + )); + assert_eq!(err.status_code(), StatusCode::BAD_REQUEST); + } +} diff --git a/src/handlers/http/middleware.rs b/src/handlers/http/middleware.rs index cc6d2bc02..e8dce8771 100644 --- a/src/handlers/http/middleware.rs +++ b/src/handlers/http/middleware.rs @@ -40,7 +40,10 @@ use crate::{ handlers::{ AUTHORIZATION_KEY, KINESIS_COMMON_ATTRIBUTES_KEY, LOG_SOURCE_KEY, LOG_SOURCE_KINESIS, STREAM_NAME_HEADER_KEY, TENANT_ID, - http::{ingest::PostError, modal::OIDC_CLIENT, rbac::RBACError}, + http::{ + base_path, ingest::PostError, modal::OIDC_CLIENT, prism_base_path, promql, + rbac::RBACError, traces::TraceError, + }, }, option::Mode, parseable::{DEFAULT_TENANT, PARSEABLE}, @@ -51,7 +54,12 @@ use crate::{ user::UserType, }, tenants::TENANT_METADATA, - utils::{get_user_and_tenant_from_request, get_user_from_request}, + tool_executor::ToolCallHeaderError, + users::dashboard_runtime, + utils::{ + get_user_and_tenant_from_request, get_user_from_request, + header_parsing::{HeaderBuildError, ParseHeaderError, optional_header_str}, + }, }; use crate::{ rbac::Users, @@ -62,6 +70,8 @@ use crate::{ use serde::{Deserialize, Serialize}; pub const CLUSTER_SECRET_HEADER: &str = "x-p-cluster-secret"; +const INTRA_CLUSTER_TENANT_HEADER: &str = "intra-cluster-tenant"; +const INTRA_CLUSTER_USERID_HEADER: &str = "intra-cluster-userid"; pub static CLUSTER_SECRET: OnceCell<(String, String)> = OnceCell::new(); #[derive(Serialize, Deserialize, Debug)] @@ -168,18 +178,23 @@ where // Capture the incoming tenant header value before `get_user_and_tenant` // potentially mutates it, so the API-key branch below sees the original - // client-supplied value. - let tenant_id = req - .headers() - .get(TENANT_ID) - .or_else(|| req.headers().get("intra-cluster-tenant")) - .and_then(|v| v.to_str().ok()) - .filter(|tenant| !tenant.eq(&DEFAULT_TENANT)) - .map(String::from); + // client-supplied value. A malformed value is rejected here, before it + // can be replaced or any identity or session is resolved. + let tenant_id = match api_key_tenant_hint(req.headers()) { + Ok(tenant_id) => tenant_id, + Err(error) => { + let error = tenant_header_error(&req, error); + return Box::pin(async move { Err(error) }); + } + }; // an optional error to track the presence of CORRECT tenant header in case of ingestion let mut header_error = None; - let user_and_tenant_id = get_user_and_tenant(&self.action, &mut req, &mut header_error); + let user_and_tenant_id = + match get_user_and_tenant(&self.action, &mut req, &mut header_error) { + Ok(user_and_tenant_id) => user_and_tenant_id, + Err(error) => return Box::pin(async move { Err(error) }), + }; // If an API key is present in X-API-KEY or as a Bearer token, resolve it to the backing // `UserType::ApiKey` user and register an ephemeral session whose @@ -193,6 +208,13 @@ where let Some(api_key) = user.as_api_key() else { return Box::pin(async { Err(ErrorUnauthorized("Invalid API key")) }); }; + // Fallible header work happens before the session is + // tracked so a failure cannot leave a session behind. + if let Err(error) = + insert_api_key_tenant_header(&mut req, &user, is_multi_tenant()) + { + return Box::pin(async move { Err(Error::from(error)) }); + } let tenant = user.tenant.as_deref().unwrap_or(DEFAULT_TENANT); let permissions = roles_to_permission(user.roles(), tenant); let session_id = Ulid::new(); @@ -204,7 +226,6 @@ where permissions, &user.tenant, ); - insert_api_key_tenant_header(&mut req, &user, is_multi_tenant()); req.extensions_mut().insert(session_key); Some(session_id) } @@ -243,6 +264,9 @@ where } else { None }; + // Owned from here so the session is removed even if the returned + // future is dropped before it is first polled. + let api_key_guard = EphemeralSessionGuard(api_key_session_id); let key: Result = extract_session_key(&mut req); @@ -250,8 +274,7 @@ where if self.action.eq(&Action::Ingest) && let Ok(key) = &key && let SessionKey::BasicAuth { username, password } = &key - && let Ok((_, tenant)) = &user_and_tenant_id - && let Some(tenant) = tenant.as_ref() + && let (_, Some(tenant)) = &user_and_tenant_id && !Users.validate_basic_user_tenant_id(username, password, tenant) { header_error = Some(actix_web::Error::from(PostError::Header( @@ -265,17 +288,9 @@ where let auth_method = self.auth_method; let action = self.action; Box::pin(async move { - // Guard cleans up the ephemeral session created for an API-key - // request when this future finishes (success OR failure). - struct ApiKeySessionGuard(Option); - impl Drop for ApiKeySessionGuard { - fn drop(&mut self) { - if let Some(sid) = self.0 { - mut_sessions().remove_session(&SessionKey::SessionId(sid)); - } - } - } - let _api_key_guard = ApiKeySessionGuard(api_key_session_id); + // Cleans up the ephemeral session created for an API-key request + // when this future finishes (success OR failure) or is dropped. + let _api_key_guard = api_key_guard; let Ok(key) = key else { return Err(ErrorUnauthorized( @@ -292,7 +307,7 @@ where // Basic auth retains its existing precedence when both mechanisms // are supplied. Only the selected non-API session may be refreshed. if api_key_session.as_ref() != Some(&key) && sessions().is_session_expired(&key) { - refresh_token(user_and_tenant_id, &key, headers).await?; + refresh_token(Ok(user_and_tenant_id), &key, headers).await?; } if let Some(api_key_session) = &api_key_session { // This per-request session is capped by the credential's expiry. @@ -377,19 +392,82 @@ fn extract_api_key(req: &ServiceRequest) -> Option { .map(String::from) } +/// Removes an ephemeral session when dropped, so the session is cleaned up +/// whether the request future completes, fails or is cancelled. +struct EphemeralSessionGuard(Option); + +impl Drop for EphemeralSessionGuard { + fn drop(&mut self) { + if let Some(sid) = self.0 { + mut_sessions().remove_session(&SessionKey::SessionId(sid)); + } + } +} + +/// The client-supplied tenant used to scope an API-key lookup: `x-p-tenant`, +/// or `intra-cluster-tenant` when `x-p-tenant` is absent. A missing header is +/// `Ok(None)`; a present but non visible-ASCII value is an error, never `None`. +fn api_key_tenant_hint(headers: &HeaderMap) -> Result, ParseHeaderError> { + let tenant = match optional_header_str(headers, TENANT_ID) { + Ok(Some(tenant)) => Some(tenant), + Ok(None) => optional_header_str(headers, INTRA_CLUSTER_TENANT_HEADER) + .map_err(|_| ParseHeaderError::InvalidTenantId)?, + Err(_) => return Err(ParseHeaderError::InvalidTenantId), + }; + Ok(tenant + .filter(|tenant| !tenant.eq(&DEFAULT_TENANT)) + .map(String::from)) +} + +/// The rejection for a malformed tenant header. This happens before the +/// handler runs, so endpoints that answer errors in a JSON envelope get theirs +/// from the handler's own constructor; every other endpoint keeps the plain +/// text error. Endpoints are recognized by the registered route pattern, found +/// from the path as the router decoded it, so path parameters and +/// percent-encoded spellings of a route resolve to the same endpoint. +fn tenant_header_error(req: &ServiceRequest, error: ParseHeaderError) -> Error { + let Some(pattern) = req.resource_map().match_pattern(req.match_info().as_str()) else { + return PostError::Header(error).into(); + }; + // Registered by `promql::scope` and by the server's dashboards, llm and + // traces scopes. + if pattern.starts_with(&format!("{}/", promql::SCOPE)) { + promql::ApiError::invalid_header(error).into() + } else if pattern.starts_with(&format!("{}/services/traces/", prism_base_path())) { + TraceError::invalid_header(error).into() + } else if pattern == format!("{}/dashboards/preview", base_path()) { + dashboard_runtime::invalid_header(error).into() + } else if pattern == format!("{}/llm/tools/call", prism_base_path()) { + ToolCallHeaderError(error).into() + } else { + PostError::Header(error).into() + } +} + +/// Encode a tenant id the server resolved from its own user records. +/// The value must read back as text, since handlers re-read the header; +/// anything else is corrupt stored data and a server error. +fn stored_tenant_header_value(tenant: &str) -> Result { + HeaderValue::from_str(tenant) + .ok() + .filter(|value| value.to_str().is_ok()) + .ok_or(HeaderBuildError::Internal("stored tenant id")) +} + /// Add the tenant resolved from an API-key user to a multi-tenant request. +/// The request is left unchanged on error. fn insert_api_key_tenant_header( req: &mut ServiceRequest, user: &user::User, is_multi_tenant: bool, -) { +) -> Result<(), HeaderBuildError> { if is_multi_tenant { let tenant = user.tenant.as_deref().unwrap_or(DEFAULT_TENANT); - req.headers_mut().insert( - HeaderName::from_static(TENANT_ID), - HeaderValue::from_str(tenant).unwrap(), - ); + let value = stored_tenant_header_value(tenant)?; + req.headers_mut() + .insert(HeaderName::from_static(TENANT_ID), value); } + Ok(()) } /// Unit tests cannot initialize `PARSEABLE`, which parses the process arguments, @@ -430,18 +508,21 @@ fn find_api_key_user(api_key_value: &str, tenant_id: &Option) -> Option< }) } +/// Resolve the requesting user and tenant. Client header problems are +/// recorded in `header_error`; an `Err` is a server-side failure that must +/// fail the request immediately. #[inline] fn get_user_and_tenant( action: &Action, request: &mut ServiceRequest, header_error: &mut Option, -) -> Result<(Result, Option), RBACError> { +) -> Result<(Result, Option), Error> { if is_multi_tenant() { // if ingestion then tenant MUST be present and should not be DEFAULT_TENANT let tenant = if action.eq(&Action::Ingest) { - if let Some(tenant) = request.headers().get(TENANT_ID) - && let Ok(tenant) = tenant.to_str() - { + let tenant = optional_header_str(request.headers(), TENANT_ID) + .map_err(|_| PostError::Header(ParseHeaderError::InvalidTenantId))?; + if let Some(tenant) = tenant { if tenant.eq(DEFAULT_TENANT) { *header_error = Some(actix_web::Error::from(PostError::Header( crate::utils::header_parsing::ParseHeaderError::InvalidTenantId, @@ -463,10 +544,10 @@ fn get_user_and_tenant( if let Ok((_, tenant)) = get_user_and_tenant_from_request(request.request()) && let Some(tid) = tenant.as_ref() { - request.headers_mut().insert( - HeaderName::from_static(TENANT_ID), - HeaderValue::from_str(tid).unwrap(), - ); + let value = stored_tenant_header_value(tid)?; + request + .headers_mut() + .insert(HeaderName::from_static(TENANT_ID), value); t = tenant; } else { // remove the header if already present @@ -672,7 +753,7 @@ pub fn auth_resource_context( let creds = extract_session_key(req); let usergroup = req.match_info().get("usergroup"); let llmid = req.match_info().get("llmid"); - let mut stream = req.match_info().get("logstream"); + let stream = req.match_info().get("logstream"); if let Some(usergroup) = usergroup { creds.map(|key| Users.authorize(key, action, Some(usergroup), None)) } else if let Some(llmid) = llmid { @@ -680,9 +761,9 @@ pub fn auth_resource_context( } else if let Some(stream) = stream { creds.map(|key| Users.authorize(key, action, Some(stream), None)) } else { - if let Some(stream_name) = req.headers().get(STREAM_NAME_HEADER_KEY) { - stream = Some(stream_name.to_str().unwrap()); - } + // Path resources take precedence, so the header is only read here. + let stream = optional_header_str(req.headers(), STREAM_NAME_HEADER_KEY) + .map_err(PostError::Header)?; creds.map(|key| Users.authorize(key, action, stream, None)) } } @@ -793,86 +874,89 @@ where forward_ready!(service); fn call(&self, mut req: ServiceRequest) -> Self::Future { - let (err, id) = if let Some((secret, _)) = CLUSTER_SECRET.get() { - if let Some(header) = req.headers().get(CLUSTER_SECRET_HEADER) - && let Some(tenant) = req.headers().get("intra-cluster-tenant") - && let Some(userid) = req.headers().get("intra-cluster-userid") - && let Ok(incoming_secret) = header.to_str() - && let Ok(tenant) = tenant.to_str() - && let Ok(userid) = userid.to_str() - { - // validate the incoming header value - let parsed_hash = PasswordHash::new(incoming_secret).unwrap(); - if Argon2::default() - .verify_password(secret.as_bytes(), &parsed_hash) - .is_ok() - { - // create a user session (how to remove that later?) - let tenant_id = if tenant.eq(DEFAULT_TENANT) { - None - } else { - Some(tenant.to_owned()) - }; - let user = Users.get_user(userid, &tenant_id); - // Forwarded work must not outlive the API key that authorized it. - if user - .as_ref() - .and_then(|user| user.as_api_key()) - .is_some_and(|api_key| api_key.is_expired_at(Utc::now())) - { - return Box::pin(async { Err(ErrorUnauthorized("Invalid API key")) }); - } - let id = if let Some(user) = user { - let id = Ulid::new(); - req.headers_mut().insert( - header::COOKIE, - HeaderValue::from_str(&format!("session={}", id)).unwrap(), - ); - - // remove basic auth header - req.headers_mut().remove(header::AUTHORIZATION); - - let session = SessionKey::SessionId(id); - req.extensions_mut().insert(session.clone()); - Users.new_session(&user, session, TimeDelta::seconds(20)); - Some(id) - } else { - None - }; - (None, id) - } else { - ( - Some("Incoming intra-cluster request validation failed"), - None, - ) - } - } else { - ( - Some( - "Incoming intra-cluster request doesn't contain the proper header or the server was started without P_CLUSTER_SECRET", - ), - None, - ) - } - } else { - (None, None) + // Without a configured secret the request passes through unchanged. + let session_guard = match CLUSTER_SECRET.get() { + Some((secret, _)) => match authenticate_intra_cluster_request(&mut req, secret) { + Ok(guard) => guard, + // Rejected before the inner service sees the request. + Err(error) => return Box::pin(async move { Err(error) }), + }, + None => EphemeralSessionGuard(None), }; let fut = self.service.call(req); Box::pin(async move { - if let Some(err) = err { - return Err(ErrorUnauthorized(err)); - } - let res = fut.await; - if let Some(id) = id { - mut_sessions().remove_session(&SessionKey::SessionId(id)); - } - res + // The forwarded user's session ends with this request. + let _session_guard = session_guard; + fut.await }) } } +/// Verify the forwarded cluster credentials and give the forwarded user a +/// short-lived session. Credentials present but not visible ASCII, or a +/// secret that is not a PHC password hash, are 400; missing or wrong +/// credentials are 401. Submitted values never appear in errors. +fn authenticate_intra_cluster_request( + req: &mut ServiceRequest, + secret: &str, +) -> Result { + let headers = req.headers(); + let incoming_secret = optional_header_str(headers, CLUSTER_SECRET_HEADER)?; + let tenant = optional_header_str(headers, INTRA_CLUSTER_TENANT_HEADER)?; + let userid = optional_header_str(headers, INTRA_CLUSTER_USERID_HEADER)?; + let (Some(incoming_secret), Some(tenant), Some(userid)) = (incoming_secret, tenant, userid) + else { + return Err(ErrorUnauthorized( + "Incoming intra-cluster request doesn't contain the proper header or the server was started without P_CLUSTER_SECRET", + )); + }; + + // validate the incoming header value + let parsed_hash = PasswordHash::new(incoming_secret) + .map_err(|_| ErrorBadRequest("Incoming intra-cluster secret is malformed"))?; + if Argon2::default() + .verify_password(secret.as_bytes(), &parsed_hash) + .is_err() + { + return Err(ErrorUnauthorized( + "Incoming intra-cluster request validation failed", + )); + } + + let tenant_id = if tenant.eq(DEFAULT_TENANT) { + None + } else { + Some(tenant.to_owned()) + }; + let Some(user) = Users.get_user(userid, &tenant_id) else { + return Ok(EphemeralSessionGuard(None)); + }; + // Forwarded work must not outlive the API key that authorized it. + if user + .as_api_key() + .is_some_and(|api_key| api_key.is_expired_at(Utc::now())) + { + return Err(ErrorUnauthorized("Invalid API key")); + } + + // Build the cookie before registering the session so a failure here + // cannot leave a session behind. + let id = Ulid::new(); + let cookie = HeaderValue::from_str(&format!("session={id}")) + .map_err(|_| HeaderBuildError::Internal("intra-cluster session cookie"))?; + req.headers_mut().insert(header::COOKIE, cookie); + + // remove basic auth header + req.headers_mut().remove(header::AUTHORIZATION); + + let session = SessionKey::SessionId(id); + req.extensions_mut().insert(session.clone()); + Users.new_session(&user, session, TimeDelta::seconds(20)); + Ok(EphemeralSessionGuard(Some(id))) +} + /// ModeFilterMiddleware factory pub struct ModeFilter; @@ -1103,21 +1187,116 @@ mod api_key_header_tests { ); let mut req = actix_test::TestRequest::default().to_srv_request(); - insert_api_key_tenant_header(&mut req, &user, true); + insert_api_key_tenant_header(&mut req, &user, true).unwrap(); assert_eq!(req.headers().get(TENANT_ID).unwrap(), "acme"); } + + #[test] + fn unencodable_stored_api_key_tenant_is_a_server_error() { + // Stored data that cannot be sent as a header, or would not read + // back as text, is the server's fault and must not panic. + for tenant in ["bad\ntenant", "tenänt"] { + let user = user::User::new_api_key( + Ulid::new(), + "valid-api-key".to_owned(), + "test-key".to_owned(), + HashSet::new(), + "admin".to_owned(), + Some(tenant.to_owned()), + ); + let mut req = actix_test::TestRequest::default().to_srv_request(); + + let error = insert_api_key_tenant_header(&mut req, &user, true).unwrap_err(); + + assert!(matches!(error, HeaderBuildError::Internal(_))); + assert_eq!( + Error::from(error).as_response_error().status_code(), + actix_web::http::StatusCode::INTERNAL_SERVER_ERROR + ); + assert!(req.headers().get(TENANT_ID).is_none()); + // Single-tenant requests never carry the header. + insert_api_key_tenant_header(&mut req, &user, false).unwrap(); + assert!(req.headers().get(TENANT_ID).is_none()); + } + } + + fn header_map(pairs: &[(&'static str, &[u8])]) -> HeaderMap { + let mut headers = HeaderMap::new(); + for &(name, value) in pairs { + headers.insert( + HeaderName::from_static(name), + HeaderValue::from_bytes(value).unwrap(), + ); + } + headers + } + + #[test] + fn api_key_tenant_hint_rejects_malformed_headers() { + let invalid = b"\xff".as_slice(); + for (pairs, expected) in [ + (vec![], Ok(None)), + (vec![(TENANT_ID, b"acme".as_slice())], Ok(Some("acme"))), + (vec![(TENANT_ID, DEFAULT_TENANT.as_bytes())], Ok(None)), + (vec![(TENANT_ID, invalid)], Err(())), + (vec![(TENANT_ID, b"caf\xc3\xa9".as_slice())], Err(())), + // A malformed client tenant is not replaced by the fallback. + ( + vec![ + (TENANT_ID, invalid), + (INTRA_CLUSTER_TENANT_HEADER, b"acme".as_slice()), + ], + Err(()), + ), + ( + vec![(INTRA_CLUSTER_TENANT_HEADER, b"acme".as_slice())], + Ok(Some("acme")), + ), + (vec![(INTRA_CLUSTER_TENANT_HEADER, invalid)], Err(())), + // The fallback is only read when `x-p-tenant` is absent. + ( + vec![ + (TENANT_ID, b"acme".as_slice()), + (INTRA_CLUSTER_TENANT_HEADER, invalid), + ], + Ok(Some("acme")), + ), + ] { + let result = api_key_tenant_hint(&header_map(&pairs)); + match expected { + Ok(expected) => assert_eq!(result.unwrap().as_deref(), expected), + Err(()) => assert!(matches!(result, Err(ParseHeaderError::InvalidTenantId))), + } + } + } } #[cfg(test)] mod api_key_auth_tests { - use std::collections::{HashMap, HashSet}; + use std::{ + collections::{HashMap, HashSet}, + sync::atomic::{AtomicUsize, Ordering}, + }; - use actix_web::{App, HttpResponse, http::StatusCode, test as actix_test, web}; + use actix_web::{ + App, HttpResponse, + body::{MessageBody, to_bytes}, + dev::fn_service, + http::{Method, StatusCode}, + test as actix_test, web, + }; use argon2::{PasswordHasher, password_hash::SaltString}; use base64::{Engine, prelude::BASE64_STANDARD}; + use bytes::Bytes; use chrono::DateTime; use parking_lot::RwLock; + use proptest::{ + collection::vec, + prelude::*, + test_runner::{Config, TestRunner}, + }; + use serde_json::{Value, json}; use tokio::sync::MutexGuard; use super::*; @@ -1218,27 +1397,119 @@ mod api_key_auth_tests { intra_cluster: bool, headers: &[(&'static str, String)], ) -> StatusCode { - let route = web::get() - .to(|| async { HttpResponse::Ok().finish() }) - .wrap(Auth { - action: Action::Query, - method, - }); - let app = if intra_cluster { - actix_test::init_service(App::new().route("/", route.wrap(IntraClusterRequest))).await - } else { - actix_test::init_service(App::new().route("/", route)).await + let headers: Vec<_> = headers + .iter() + .map(|(name, value)| (*name, HeaderValue::from_str(value).unwrap())) + .collect(); + call_app(method, intra_cluster, "/", &headers).await.status + } + + /// Calls to `counting_auth`. `Auth` always runs its method synchronously + /// in `call` unless it rejects the request first, so an unchanged count + /// shows an outer middleware never called into it. Tests only compare + /// counts while holding `TEST_SERIAL`. + static AUTH_CALLS: AtomicUsize = AtomicUsize::new(0); + + fn counting_auth(req: &mut ServiceRequest, action: Action) -> Result { + AUTH_CALLS.fetch_add(1, Ordering::SeqCst); + auth_no_context(req, action) + } + + struct Outcome { + status: StatusCode, + error: Option, + handled: usize, + auth_calls: usize, + } + + /// Serves `/` and `/logstream/{logstream}` behind `Auth` (and optionally + /// `IntraClusterRequest`), accepting raw header values. + async fn call_app( + method: AuthMethod, + intra_cluster: bool, + uri: &str, + headers: &[(&'static str, HeaderValue)], + ) -> Outcome { + let handled = Arc::new(AtomicUsize::new(0)); + let route = |handled: Arc| { + let route = web::get() + .to(move || { + handled.fetch_add(1, Ordering::SeqCst); + async { HttpResponse::Ok().finish() } + }) + .wrap(Auth { + action: Action::Query, + method, + }); + if intra_cluster { + route.wrap(IntraClusterRequest) + } else { + route + } }; - let mut req = actix_test::TestRequest::get().uri("/"); + let app = actix_test::init_service( + App::new() + .route("/", route(Arc::clone(&handled))) + .route("/logstream/{logstream}", route(Arc::clone(&handled))), + ) + .await; + let mut req = actix_test::TestRequest::get().uri(uri); for header in headers { req = req.insert_header(header.clone()); } - match actix_test::try_call_service(&app, req.to_request()).await { - Ok(response) => response.status(), - Err(error) => error.as_response_error().status_code(), + let auth_calls = AUTH_CALLS.load(Ordering::SeqCst); + let (status, error) = match actix_test::try_call_service(&app, req.to_request()).await { + Ok(response) => (response.status(), None), + Err(error) => ( + error.as_response_error().status_code(), + Some(error.to_string()), + ), + }; + Outcome { + status, + error, + handled: handled.load(Ordering::SeqCst), + auth_calls: AUTH_CALLS.load(Ordering::SeqCst) - auth_calls, } } + fn basic_admin() -> (&'static str, HeaderValue) { + let username = format!("header-test-{}", Ulid::new()).to_lowercase(); + let (mut basic, password) = user::User::new_basic(username.clone(), None, false); + basic.roles.insert(ROLE.to_owned()); + mut_users().insert(basic); + let credentials = BASE64_STANDARD.encode(format!("{username}:{password}")); + ( + "authorization", + HeaderValue::from_str(&format!("Basic {credentials}")).unwrap(), + ) + } + + fn raw(value: &[u8]) -> HeaderValue { + HeaderValue::from_bytes(value).unwrap() + } + + fn text(value: &str) -> HeaderValue { + HeaderValue::from_str(value).unwrap() + } + + fn raw_cluster_headers(user: &user::User) -> Vec<(&'static str, HeaderValue)> { + cluster_headers(user) + .into_iter() + .map(|(name, value)| (name, text(&value))) + .collect() + } + + fn with_header( + mut headers: Vec<(&'static str, HeaderValue)>, + name: &'static str, + value: Option, + ) -> Vec<(&'static str, HeaderValue)> { + headers.retain(|(header, _)| *header != name); + headers.extend(value.map(|value| (name, value))); + headers + } + /// Simulates the key's deadline passing after its session was registered. fn expire_session_then_authorize( req: &mut ServiceRequest, @@ -1378,4 +1649,531 @@ mod api_key_auth_tests { mut_sessions().remove_session(&session); } } + + #[actix_web::test] + async fn malformed_stream_header_is_rejected_before_authorization() { + let _state = auth_state().await; + let auth = basic_admin(); + let stream = |value: HeaderValue| vec![auth.clone(), (STREAM_NAME_HEADER_KEY, value)]; + + for value in [raw(b"\xff"), raw(b"caf\xc3\xa9")] { + let outcome = call_app(auth_resource_context, false, "/", &stream(value)).await; + assert_eq!(outcome.status, StatusCode::BAD_REQUEST); + assert!(outcome.error.unwrap().contains("visible ASCII")); + assert_eq!(outcome.handled, 0); + } + + let outcome = call_app(auth_resource_context, false, "/", &stream(text("app-logs"))).await; + assert_eq!(outcome.status, StatusCode::OK); + assert_eq!(outcome.handled, 1); + } + + #[actix_web::test] + async fn path_stream_takes_precedence_over_malformed_stream_header() { + let _state = auth_state().await; + let headers = [basic_admin(), (STREAM_NAME_HEADER_KEY, raw(b"\xff"))]; + + let outcome = call_app( + auth_resource_context, + false, + "/logstream/app-logs", + &headers, + ) + .await; + + assert_eq!(outcome.status, StatusCode::OK); + assert_eq!(outcome.handled, 1); + } + + #[actix_web::test] + async fn malformed_tenant_header_is_rejected_before_api_key_session() { + let _state = auth_state().await; + let (user, secret) = api_key_user(None); + let api_key = ("x-api-key", text(&secret)); + + for tenant_header in [TENANT_ID, INTRA_CLUSTER_TENANT_HEADER] { + let headers = [api_key.clone(), (tenant_header, raw(b"\xff"))]; + let outcome = call_app(counting_auth, false, "/", &headers).await; + assert_eq!(outcome.status, StatusCode::BAD_REQUEST); + // Rejected as malformed up front, not as an unexpected header + // after the API key resolved an identity. + assert!(outcome.error.unwrap().contains("Invalid tenant id")); + assert_eq!((outcome.handled, outcome.auth_calls), (0, 0)); + assert!(!has_sessions(&user)); + } + + let outcome = call_app(counting_auth, false, "/", &[api_key]).await; + assert_eq!(outcome.status, StatusCode::OK); + assert_eq!(outcome.handled, 1); + assert!(!has_sessions(&user)); + } + + /// Status, content type and body the client receives, whether the app + /// answered or failed with an error. + async fn read_reply(app: &S, req: R) -> (StatusCode, Option, Bytes) + where + S: Service, Error = Error>, + B: MessageBody + 'static, + { + let response = match actix_test::try_call_service(app, req).await { + Ok(response) => response.map_into_boxed_body().into_parts().1, + Err(error) => error.error_response(), + }; + let content_type = response + .headers() + .get(header::CONTENT_TYPE) + .and_then(|value| value.to_str().ok()) + .map(str::to_owned); + let status = response.status(); + let body = to_bytes(response.into_body()).await.expect("buffered body"); + (status, content_type, body) + } + + /// The tenant error for each endpoint family, in its handler's envelope. + fn expected_tenant_error(family: &str) -> Option { + let message = ParseHeaderError::InvalidTenantId.to_string(); + match family { + "promql" => Some(json!({"status":"error","errorType":"bad_data","error":message})), + "traces" => Some(json!({"error":message})), + "preview" => Some(json!({ + "error": {"code":"INVALID_HEADER","message":message,"details":{}}, + "message": message, + })), + "tools" => Some(json!({ + "content": [{"type":"text","text":message}], + "structuredContent": {"error":{"message":message}}, + "isError": true, + })), + _ => None, + } + } + + /// Asserts a malformed-tenant 400 in `family`'s format that does not echo + /// the submitted value. + fn assert_tenant_rejection( + family: &str, + context: &str, + (status, content_type, body): (StatusCode, Option, Bytes), + ) { + assert_eq!(status, StatusCode::BAD_REQUEST, "{context}"); + assert!( + !body + .windows(SUBMITTED_TENANT.len()) + .any(|w| w == SUBMITTED_TENANT), + "{context}: the submitted tenant is echoed" + ); + match expected_tenant_error(family) { + Some(expected) => { + assert_eq!( + content_type.as_deref(), + Some("application/json"), + "{context}" + ); + let body: Value = serde_json::from_slice(&body).expect("JSON error body"); + assert_eq!(body, expected, "{context}"); + } + None => { + assert_eq!( + content_type.as_deref(), + Some("text/plain; charset=utf-8"), + "{context}" + ); + assert_eq!( + body, + format!("Header Error: {}", ParseHeaderError::InvalidTenantId), + "{context}" + ); + } + } + } + + const SUBMITTED_TENANT: &[u8] = b"acme-tenant"; + + /// Present but not visible ASCII, carrying a recognizable submitted value. + fn malformed_tenants() -> [HeaderValue; 2] { + [raw(b"acme-tenant\xff"), raw(b"acme-tenant\xc3\xa9")] + } + + #[actix_web::test] + async fn malformed_tenant_is_rejected_in_each_endpoint_error_format() { + let _state = auth_state().await; + let (api_user, secret) = api_key_user(None); + let credentials = [ + ("basic", basic_admin()), + ("api key", ("x-api-key", text(&secret))), + ]; + + // Counting stubs behind `Auth` at the paths the servers register, + // so only the middleware and the production error constructors run. + let handled = Arc::new(AtomicUsize::new(0)); + let stub = |route: Route| { + let handled = Arc::clone(&handled); + route + .to(move || { + handled.fetch_add(1, Ordering::SeqCst); + async { HttpResponse::Ok().finish() } + }) + .wrap(Auth { + action: Action::Query, + method: counting_auth, + }) + }; + let app = actix_test::init_service( + App::new() + .service( + web::scope(promql::SCOPE) + .route("/query", stub(web::get())) + .route("/query_range", stub(web::post())) + .route("/label/{name}/values", stub(web::get())), + ) + .service( + web::scope(&base_path()) + .route("/query", stub(web::post())) + .service( + web::scope("/dashboards") + .route("/preview", stub(web::post())) + .route("/{dashboard_id}", stub(web::post())), + ), + ) + .service( + web::scope(&prism_base_path()) + .service( + web::scope("/llm") + .route("/tools/list", stub(web::post())) + .route("/tools/call", stub(web::post())), + ) + .service( + web::scope("/services/traces") + .route("/list", stub(web::post())) + .route("/detail", stub(web::post())), + ), + ), + ) + .await; + + let cases = [ + (Method::GET, "/prometheus/api/v1/query", "promql"), + (Method::POST, "/prometheus/api/v1/query_range", "promql"), + (Method::GET, "/prometheus/api/v1/label/job/values", "promql"), + (Method::POST, "/api/prism/v1/services/traces/list", "traces"), + ( + Method::POST, + "/api/prism/v1/services/traces/detail", + "traces", + ), + ( + Method::POST, + "/api/prism/v1/services/traces/%6cist", + "traces", + ), + (Method::POST, "/api/v1/dashboards/preview", "preview"), + // Routed to `/preview`, though the raw path would match `/{dashboard_id}`. + (Method::POST, "/api/v1/dashboards/%70review", "preview"), + (Method::POST, "/api/prism/v1/llm/tools/call", "tools"), + (Method::POST, "/api/prism/v1/llm/tools/%63all", "tools"), + // Ordinary endpoints keep the plain-text error, including the + // SQL endpoint whose path is a suffix of a PromQL one. + (Method::POST, "/api/v1/query", "plain"), + (Method::POST, "/api/v1/dashboards/abc", "plain"), + (Method::POST, "/api/prism/v1/llm/tools/list", "plain"), + ]; + let active_sessions = sessions().get_active_sessions().len(); + for (method, uri, family) in &cases { + for (credential, auth) in &credentials { + for tenant_header in [TENANT_ID, INTRA_CLUSTER_TENANT_HEADER] { + for tenant in malformed_tenants() { + let context = + format!("{method} {uri} with {credential} and {tenant_header}"); + let handled_before = handled.load(Ordering::SeqCst); + let auth_calls = AUTH_CALLS.load(Ordering::SeqCst); + let req = actix_test::TestRequest::default() + .method(method.clone()) + .uri(uri) + .insert_header(auth.clone()) + .insert_header((tenant_header, tenant)) + .to_request(); + assert_tenant_rejection(family, &context, read_reply(&app, req).await); + assert_eq!(handled.load(Ordering::SeqCst), handled_before, "{context}"); + assert_eq!(AUTH_CALLS.load(Ordering::SeqCst), auth_calls, "{context}"); + assert_eq!( + sessions().get_active_sessions().len(), + active_sessions, + "{context}: a session was created" + ); + } + } + } + } + assert!(!has_sessions(&api_user)); + + // The same requests without a tenant header reach the handler, so + // every path above is a registered route. + for (method, uri, _) in &cases { + let handled_before = handled.load(Ordering::SeqCst); + let req = actix_test::TestRequest::default() + .method(method.clone()) + .uri(uri) + .insert_header(credentials[1].1.clone()) + .to_request(); + let (status, _, _) = read_reply(&app, req).await; + assert_eq!(status, StatusCode::OK, "{method} {uri}"); + assert_eq!( + handled.load(Ordering::SeqCst), + handled_before + 1, + "{method} {uri}" + ); + } + assert!(!has_sessions(&api_user)); + } + + #[actix_web::test] + async fn production_promql_and_traces_routes_reject_malformed_tenant_as_json() { + let _state = auth_state().await; + let (api_user, secret) = api_key_user(None); + // Mounted exactly as the servers' `configure_routes` mount them. + let app = actix_test::init_service( + App::new().service(promql::scope()).service( + web::scope(&prism_base_path()) + .service(crate::handlers::http::modal::server::Server::get_traces_webscope()), + ), + ) + .await; + + let cases = [ + (Method::GET, "/prometheus/api/v1/query", "promql"), + (Method::POST, "/prometheus/api/v1/query", "promql"), + (Method::GET, "/prometheus/api/v1/query_range", "promql"), + (Method::GET, "/prometheus/api/v1/labels", "promql"), + ( + Method::GET, + "/prometheus/api/v1/label/__name__/values", + "promql", + ), + (Method::POST, "/api/prism/v1/services/traces/list", "traces"), + ( + Method::POST, + "/api/prism/v1/services/traces/detail", + "traces", + ), + ]; + let active_sessions = sessions().get_active_sessions().len(); + for (method, uri, family) in cases { + for tenant_header in [TENANT_ID, INTRA_CLUSTER_TENANT_HEADER] { + for tenant in malformed_tenants() { + let req = actix_test::TestRequest::default() + .method(method.clone()) + .uri(uri) + .insert_header(("x-api-key", text(&secret))) + .insert_header((tenant_header, tenant)) + .to_request(); + let context = format!("{method} {uri} with {tenant_header}"); + assert_tenant_rejection(family, &context, read_reply(&app, req).await); + assert_eq!(sessions().get_active_sessions().len(), active_sessions); + } + } + } + assert!(!has_sessions(&api_user)); + } + + #[actix_web::test] + async fn intra_cluster_rejects_malformed_credentials_before_inner_service() { + let _state = auth_state().await; + let (user, _) = api_key_user(None); + let valid = raw_cluster_headers(&user); + let malformed_hash = "not-a-phc-hash"; + + // The messages show `IntraClusterRequest` rejected the request, not + // the inner `Auth`, which reports a malformed tenant differently. + for (name, value, message) in [ + (CLUSTER_SECRET_HEADER, raw(b"\xff"), "visible ASCII"), + (INTRA_CLUSTER_TENANT_HEADER, raw(b"\xff"), "visible ASCII"), + ( + INTRA_CLUSTER_USERID_HEADER, + raw(b"caf\xc3\xa9"), + "visible ASCII", + ), + (CLUSTER_SECRET_HEADER, text(malformed_hash), "malformed"), + ] { + let headers = with_header(valid.clone(), name, Some(value)); + let outcome = call_app(counting_auth, true, "/", &headers).await; + assert_eq!(outcome.status, StatusCode::BAD_REQUEST, "{name}"); + let error = outcome.error.unwrap(); + assert!(error.contains(message), "{name}: {error}"); + assert!(!error.contains(malformed_hash)); + assert_eq!((outcome.handled, outcome.auth_calls), (0, 0), "{name}"); + assert!(!has_sessions(&user)); + } + + let outcome = call_app(counting_auth, true, "/", &valid).await; + assert_eq!(outcome.status, StatusCode::OK); + assert_eq!((outcome.handled, outcome.auth_calls), (1, 2)); + assert!(!has_sessions(&user)); + } + + #[actix_web::test] + async fn intra_cluster_rejects_missing_or_wrong_credentials() { + let _state = auth_state().await; + let (user, _) = api_key_user(None); + let valid = raw_cluster_headers(&user); + let salt = SaltString::encode_b64(b"wrong-cluster-salt").unwrap(); + let wrong_hash = Argon2::default() + .hash_password(b"wrong-cluster-secret", &salt) + .unwrap() + .to_string(); + + let mut cases: Vec<_> = [ + CLUSTER_SECRET_HEADER, + INTRA_CLUSTER_TENANT_HEADER, + INTRA_CLUSTER_USERID_HEADER, + ] + .into_iter() + .map(|name| with_header(valid.clone(), name, None)) + .collect(); + cases.push(with_header( + valid.clone(), + CLUSTER_SECRET_HEADER, + Some(text(&wrong_hash)), + )); + + for headers in cases { + let outcome = call_app(counting_auth, true, "/", &headers).await; + assert_eq!(outcome.status, StatusCode::UNAUTHORIZED); + assert_eq!((outcome.handled, outcome.auth_calls), (0, 0)); + assert!(!has_sessions(&user)); + } + + let outcome = call_app(counting_auth, true, "/", &valid).await; + assert_eq!(outcome.status, StatusCode::OK); + assert_eq!(outcome.handled, 1); + assert!(!has_sessions(&user)); + } + + #[actix_web::test] + async fn api_key_session_is_removed_when_unpolled_future_is_dropped() { + let _state = auth_state().await; + let (user, secret) = api_key_user(Some(Utc::now() + TimeDelta::hours(1))); + let handled = Arc::new(AtomicUsize::new(0)); + let inner = { + let handled = Arc::clone(&handled); + fn_service(move |req: ServiceRequest| { + handled.fetch_add(1, Ordering::SeqCst); + async move { Ok::<_, Error>(req.into_response(HttpResponse::Ok().finish())) } + }) + }; + let middleware = AuthMiddleware { + action: Action::Query, + auth_method: counting_auth, + service: Rc::new(inner), + }; + let req = actix_test::TestRequest::default() + .insert_header(("x-api-key", secret)) + .to_srv_request(); + + // `call` registers the API-key session before returning its future. + let future = middleware.call(req); + let session = sessions() + .get_active_sessions() + .into_iter() + .find_map(|(key, userid, _)| (userid == user.userid()).then_some(key)) + .expect("API-key session is registered by `call`"); + assert!(sessions().get(&session).is_some()); + assert!(!sessions().is_session_expired(&session)); + + // A client disconnect can drop the future before it is ever polled. + drop(future); + assert!(!has_sessions(&user)); + assert_eq!(handled.load(Ordering::SeqCst), 0); + } + + /// Bytes `HeaderValue` accepts, so every case can reach the server: + /// printable ASCII (possibly empty), or visible ASCII mixed with at least + /// one obs-text byte (0x80..=0xff) that `to_str` rejects. At most 256 bytes. + fn accepted_header_bytes() -> impl Strategy> { + let printable = vec(0x20u8..=0x7e, 0..=256); + let obs_text = ( + vec(prop_oneof![0x20u8..=0x7e, 0x80u8..=0xff], 0..=255), + 0x80u8..=0xff, + any::(), + ) + .prop_map(|(mut bytes, obs, at)| { + let at = at.index(bytes.len() + 1); + bytes.insert(at, obs); + bytes + }); + prop_oneof![1 => printable, 1 => obs_text] + } + + #[test] + fn header_extractors_never_panic_on_accepted_bytes() { + let _state = blocking_auth_state(); + let auth = basic_admin(); + let bad_request = |error: Error| error.as_response_error().status_code(); + let (text_cases, obs_text_cases) = (AtomicUsize::new(0), AtomicUsize::new(0)); + + let mut runner = TestRunner::new(Config { + cases: 256, + failure_persistence: None, + ..Config::default() + }); + runner + .run(&accepted_header_bytes(), |bytes| { + prop_assert!(bytes.len() <= 256); + let value = HeaderValue::from_bytes(&bytes) + .expect("generator only yields bytes `HeaderValue` accepts"); + let accepted = value.to_str().ok(); + let printable = bytes.iter().all(|byte| (0x20..=0x7e).contains(byte)); + prop_assert_eq!(accepted.is_some(), printable); + let category = if printable { + &text_cases + } else { + &obs_text_cases + }; + category.fetch_add(1, Ordering::SeqCst); + + let mut headers = HeaderMap::new(); + headers.insert(HeaderName::from_static(TENANT_ID), value.clone()); + match (api_key_tenant_hint(&headers), accepted) { + (Ok(hint), Some(text)) => { + prop_assert_eq!(hint.as_deref(), (text != DEFAULT_TENANT).then_some(text)) + } + (Err(error), None) => { + prop_assert_eq!(bad_request(error.into()), StatusCode::BAD_REQUEST) + } + (result, _) => prop_assert!(false, "unexpected tenant hint {result:?}"), + } + + let mut req = actix_test::TestRequest::default() + .insert_header(auth.clone()) + .insert_header((STREAM_NAME_HEADER_KEY, value.clone())) + .to_srv_request(); + match (auth_resource_context(&mut req, Action::Query), accepted) { + (Ok(rbac::Response::Authorized), Some(_)) => {} + (Err(error), None) => { + prop_assert_eq!(bad_request(error), StatusCode::BAD_REQUEST) + } + (result, _) => prop_assert!(false, "unexpected auth result {result:?}"), + } + + let mut req = actix_test::TestRequest::default() + .insert_header((CLUSTER_SECRET_HEADER, value.clone())) + .insert_header((INTRA_CLUSTER_TENANT_HEADER, DEFAULT_TENANT)) + .insert_header((INTRA_CLUSTER_USERID_HEADER, "header-proptest")) + .to_srv_request(); + let status = authenticate_intra_cluster_request(&mut req, CLUSTER_TEST_SECRET) + .err() + .map(bad_request); + match accepted { + None => prop_assert_eq!(status, Some(StatusCode::BAD_REQUEST)), + Some(_) => prop_assert!(matches!( + status, + Some(StatusCode::BAD_REQUEST | StatusCode::UNAUTHORIZED) + )), + } + Ok(()) + }) + .unwrap(); + // Both categories ran, so neither branch above went untested. + assert!(text_cases.load(Ordering::SeqCst) > 0); + assert!(obs_text_cases.load(Ordering::SeqCst) > 0); + } } diff --git a/src/handlers/http/modal/ingest/ingestor_logstream.rs b/src/handlers/http/modal/ingest/ingestor_logstream.rs index 9f7414baa..a07597cff 100644 --- a/src/handlers/http/modal/ingest/ingestor_logstream.rs +++ b/src/handlers/http/modal/ingest/ingestor_logstream.rs @@ -42,7 +42,7 @@ pub async fn retention_cleanup( ) -> Result { let stream_name = stream_name.into_inner(); let storage = PARSEABLE.storage().get_object_store(); - let tenant_id = get_tenant_id_from_request(&req); + let tenant_id = get_tenant_id_from_request(&req)?; // if the stream not found in memory map, //check if it exists in the storage //create stream and schema from storage @@ -75,7 +75,7 @@ pub async fn delete( stream_name: Path, ) -> Result { let stream_name = stream_name.into_inner(); - let tenant_id = get_tenant_id_from_request(&req); + let tenant_id = get_tenant_id_from_request(&req)?; // Delete from staging let stream_dir = PARSEABLE.get_stream(&stream_name, &tenant_id)?; @@ -105,7 +105,7 @@ pub async fn put_stream( body: Bytes, ) -> Result { let stream_name = stream_name.into_inner(); - let tenant_id = get_tenant_id_from_request(&req); + let tenant_id = get_tenant_id_from_request(&req)?; PARSEABLE .create_update_stream(req.headers(), &body, &stream_name, &tenant_id) .await?; diff --git a/src/handlers/http/modal/ingest/ingestor_rbac.rs b/src/handlers/http/modal/ingest/ingestor_rbac.rs index 486b365d7..8a32d8897 100644 --- a/src/handlers/http/modal/ingest/ingestor_rbac.rs +++ b/src/handlers/http/modal/ingest/ingestor_rbac.rs @@ -49,7 +49,7 @@ pub async fn post_user( if let Some(body) = body { let user: ParseableUser = serde_json::from_value(body.into_inner())?; - let req_tenant_id = get_tenant_id_from_request(&req); + let req_tenant_id = get_tenant_id_from_request(&req)?; let req_tenant = req_tenant_id.as_deref().unwrap_or(DEFAULT_TENANT); if req_tenant.ne(DEFAULT_TENANT) && (req_tenant.ne(user.tenant.as_deref().unwrap_or(DEFAULT_TENANT))) @@ -70,7 +70,7 @@ pub async fn delete_user( userid: web::Path, ) -> Result { let userid = userid.into_inner(); - let tenant_id = get_tenant_id_from_request(&req); + let tenant_id = get_tenant_id_from_request(&req)?; sync_stored_user(&userid, &tenant_id).await?; Ok(HttpResponse::Ok().status(StatusCode::OK).finish()) } @@ -83,7 +83,7 @@ pub async fn add_roles_to_user( _roles_to_add: web::Json>, ) -> Result { let userid = userid.into_inner(); - let tenant_id = get_tenant_id_from_request(&req); + let tenant_id = get_tenant_id_from_request(&req)?; sync_stored_user(&userid, &tenant_id).await?; Ok(HttpResponse::Ok().status(StatusCode::OK).finish()) } @@ -96,7 +96,7 @@ pub async fn remove_roles_from_user( _roles_to_remove: web::Json>, ) -> Result { let userid = userid.into_inner(); - let tenant_id = get_tenant_id_from_request(&req); + let tenant_id = get_tenant_id_from_request(&req)?; sync_stored_user(&userid, &tenant_id).await?; Ok(HttpResponse::Ok().status(StatusCode::OK).finish()) } @@ -108,7 +108,7 @@ pub async fn post_gen_password( userid: web::Path, ) -> Result { let username = userid.into_inner(); - let tenant_id = get_tenant_id_from_request(&req); + let tenant_id = get_tenant_id_from_request(&req)?; sync_stored_user(&username, &tenant_id).await?; Ok(HttpResponse::Ok().status(StatusCode::OK).finish()) } diff --git a/src/handlers/http/modal/ingest/ingestor_role.rs b/src/handlers/http/modal/ingest/ingestor_role.rs index 67112abed..15b6336e6 100644 --- a/src/handlers/http/modal/ingest/ingestor_role.rs +++ b/src/handlers/http/modal/ingest/ingestor_role.rs @@ -46,7 +46,7 @@ pub async fn put( Json(sync_req): Json, ) -> Result { let name = name.into_inner(); - let req_tenant_id = get_tenant_id_from_request(&req); + let req_tenant_id = get_tenant_id_from_request(&req)?; let req_tenant = req_tenant_id.as_deref().unwrap_or(DEFAULT_TENANT); if req_tenant.ne(DEFAULT_TENANT) && (req_tenant_id.ne(&sync_req.tenant_id)) { return Err(RoleError::Anyhow(anyhow::Error::msg( @@ -126,7 +126,7 @@ pub async fn delete( name: web::Path, ) -> Result { let name = name.into_inner(); - let req_tenant_id = get_tenant_id_from_request(&req); + let req_tenant_id = get_tenant_id_from_request(&req)?; let mut metadata = get_metadata(&req_tenant_id).await?; metadata.roles.remove(&name); diff --git a/src/handlers/http/modal/query/querier_logstream.rs b/src/handlers/http/modal/query/querier_logstream.rs index ec6365e97..5226fbe07 100644 --- a/src/handlers/http/modal/query/querier_logstream.rs +++ b/src/handlers/http/modal/query/querier_logstream.rs @@ -20,6 +20,7 @@ use core::str; use std::{collections::HashMap, fs}; use actix_web::http::StatusCode; +use actix_web::http::header::{HeaderMap, HeaderName, HeaderValue}; use actix_web::{ HttpRequest, Responder, web::{self, Path}, @@ -35,13 +36,17 @@ use crate::handlers::http::middleware::{CLUSTER_SECRET, CLUSTER_SECRET_HEADER}; use crate::hottier::GLOBAL_HOTTIER; use crate::parseable::DEFAULT_TENANT; use crate::utils::get_user_from_request; +use crate::utils::header_parsing::{ + HeaderBuildError, ParseHeaderError, generated_text_header_value, optional_header_str, +}; use crate::{ handlers::{ UPDATE_STREAM_KEY, http::{ base_path_without_preceding_slash, cluster::{ - self, fetch_daily_stats, fetch_stats_from_ingestors, sync_streams_with_ingestors, + self, INTRA_CLUSTER_USERID, fetch_daily_stats, fetch_stats_from_ingestors, + sync_streams_with_ingestors, utils::{IngestionStats, QueriedStats, StorageStats, merge_queried_stats}, }, logstream::error::StreamError, @@ -60,7 +65,7 @@ pub async fn delete( stream_name: Path, ) -> Result { let stream_name = stream_name.into_inner(); - let tenant_id = get_tenant_id_from_request(&req); + let tenant_id = get_tenant_id_from_request(&req)?; // if the stream not found in memory map, //check if it exists in the storage //create stream and schema from storage @@ -127,39 +132,41 @@ pub async fn put_stream( body: Bytes, ) -> Result { let stream_name = stream_name.into_inner(); - let tenant_id = get_tenant_id_from_request(&req); + let tenant_id = get_tenant_id_from_request(&req)?; + let is_update = + optional_header_str(req.headers(), UPDATE_STREAM_KEY)?.is_some_and(|value| value == "true"); + + // Build the forwarding headers before mutating the stream so that a bad + // header can't leave the stream created locally but never synced. + let intracluster_headers = match CLUSTER_SECRET.get() { + Some((_, hash)) => { + let userid = get_user_from_request(&req).map_err(|_| StreamError::Custom { + msg: "Unauthorized".to_owned(), + status: StatusCode::UNAUTHORIZED, + })?; + intracluster_stream_headers(hash, &tenant_id, &userid)? + } + None => Vec::new(), + }; + check_sync_userid_fallback( + req.headers(), + &intracluster_headers, + &PARSEABLE.options.username, + )?; + let _guard = CREATE_STREAM_LOCK.lock().await; let mut headers = PARSEABLE .create_update_stream(req.headers(), &body, &stream_name, &tenant_id) .await?; - - let is_update = if let Some(val) = headers.get(UPDATE_STREAM_KEY) { - val.to_str().unwrap() == "true" - } else { - false - }; - - if let Some((_, hash)) = CLUSTER_SECRET.get() { - let userid = get_user_from_request(&req).unwrap(); - headers.insert( - actix_web::http::header::HeaderName::from_static(CLUSTER_SECRET_HEADER), - actix_web::http::header::HeaderValue::from_str(hash).unwrap(), - ); - headers.insert( - actix_web::http::header::HeaderName::from_static("intra-cluster-tenant"), - actix_web::http::header::HeaderValue::from_str( - tenant_id.as_deref().unwrap_or(DEFAULT_TENANT), - ) - .unwrap(), - ); - headers.insert( - actix_web::http::header::HeaderName::from_static("intra-cluster-userid"), - actix_web::http::header::HeaderValue::from_str(&userid).unwrap(), - ); + for (name, value) in intracluster_headers { + headers.insert(name, value); + } + match sync_streams_with_ingestors(headers, body, &stream_name, &tenant_id).await { + Ok(()) => {} + // checked above, so this is a server bug rather than a sync failure + Err(e @ StreamError::HeaderBuild(_)) => return Err(e), + Err(e) => tracing::error!("{e}"), } - if let Err(e) = sync_streams_with_ingestors(headers, body, &stream_name, &tenant_id).await { - tracing::error!("{e}"); - }; if is_update { Ok(("Log stream updated", StatusCode::OK)) @@ -168,12 +175,46 @@ pub async fn put_stream( } } +/// Headers that authenticate a forwarded stream create/update as an +/// intra-cluster request. A tenant that can't be encoded is a client error; +/// a cluster secret hash or user id that can't be encoded is internal. +fn intracluster_stream_headers( + hash: &str, + tenant_id: &Option, + userid: &str, +) -> Result, HeaderBuildError> { + let tenant = HeaderValue::from_str(tenant_id.as_deref().unwrap_or(DEFAULT_TENANT)) + .map_err(|_| HeaderBuildError::Input(ParseHeaderError::InvalidTenantId))?; + let hash = generated_text_header_value(hash, "cluster secret hash")?; + let userid = generated_text_header_value(userid, "user id")?; + Ok(vec![ + (HeaderName::from_static(CLUSTER_SECRET_HEADER), hash), + (HeaderName::from_static("intra-cluster-tenant"), tenant), + (HeaderName::from_static(INTRA_CLUSTER_USERID), userid), + ]) +} + +/// `sync_streams_with_ingestors` sends the configured username as the user id +/// when the forwarded headers carry none. Check that it can be encoded before +/// the stream is mutated, using the same helper so the two can't drift. +fn check_sync_userid_fallback( + req_headers: &HeaderMap, + intracluster_headers: &[(HeaderName, HeaderValue)], + username: &str, +) -> Result<(), HeaderBuildError> { + let has_userid = req_headers.contains_key(INTRA_CLUSTER_USERID) + || intracluster_headers + .iter() + .any(|(name, _)| name == INTRA_CLUSTER_USERID); + cluster::fallback_intracluster_userid(has_userid, username).map(|_| ()) +} + pub async fn get_stats( req: HttpRequest, stream_name: Path, ) -> Result { let stream_name = stream_name.into_inner(); - let tenant_id = get_tenant_id_from_request(&req); + let tenant_id = get_tenant_id_from_request(&req)?; let query_map = web::Query::>::from_query(req.query_string()) .map_err(|_| StreamError::InvalidQueryParameter(STATS_DATE_QUERY_PARAM.to_string()))?; let date = if query_map.is_empty() { @@ -284,3 +325,117 @@ pub async fn get_stats_internal( Ok(stats) } + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn intracluster_stream_headers_carry_secret_tenant_and_user() { + let headers = intracluster_stream_headers("hash", &Some("acme".to_owned()), "alice") + .expect("valid values build headers"); + let get = |name: &str| { + headers + .iter() + .find(|(key, _)| key == name) + .map(|(_, value)| value.to_str().unwrap().to_owned()) + }; + assert_eq!(get(CLUSTER_SECRET_HEADER).as_deref(), Some("hash")); + assert_eq!(get("intra-cluster-tenant").as_deref(), Some("acme")); + assert_eq!(get("intra-cluster-userid").as_deref(), Some("alice")); + } + + #[test] + fn intracluster_stream_headers_default_missing_tenant() { + let headers = intracluster_stream_headers("hash", &None, "alice").unwrap(); + assert!( + headers + .iter() + .any(|(key, value)| key == "intra-cluster-tenant" && value == DEFAULT_TENANT) + ); + } + + #[test] + fn intracluster_stream_headers_reject_invalid_tenant_as_input() { + let err = intracluster_stream_headers("hash", &Some("bad\ntenant".to_owned()), "alice") + .unwrap_err(); + assert!(matches!( + err, + HeaderBuildError::Input(ParseHeaderError::InvalidTenantId) + )); + assert_eq!( + actix_web::ResponseError::status_code(&StreamError::from(err)), + StatusCode::BAD_REQUEST + ); + } + + #[test] + fn intracluster_stream_headers_reject_invalid_generated_values_as_internal() { + let err = intracluster_stream_headers("se\ncret", &None, "alice").unwrap_err(); + assert!(matches!( + err, + HeaderBuildError::Internal("cluster secret hash") + )); + assert!(!err.to_string().contains("se\ncret")); + let err = intracluster_stream_headers("hash", &None, "al\rice").unwrap_err(); + assert!(matches!(err, HeaderBuildError::Internal("user id"))); + assert_eq!( + actix_web::ResponseError::status_code(&StreamError::from(err)), + StatusCode::INTERNAL_SERVER_ERROR + ); + } + + // `HeaderValue::from_str` accepts these but the ingestor's `to_str` doesn't + const UNREADABLE_TEXT: [&str; 3] = ["josé", "用户", "ctl\u{1}"]; + + fn assert_internal_500(err: HeaderBuildError, field: &str, value: &str) { + assert!(matches!(err, HeaderBuildError::Internal(f) if f == field)); + assert!(!err.to_string().contains(value)); + assert_eq!( + actix_web::ResponseError::status_code(&StreamError::from(err)), + StatusCode::INTERNAL_SERVER_ERROR + ); + } + + #[test] + fn intracluster_stream_headers_reject_unreadable_generated_text() { + for value in UNREADABLE_TEXT { + let err = intracluster_stream_headers(value, &None, "alice").unwrap_err(); + assert_internal_500(err, "cluster secret hash", value); + let err = intracluster_stream_headers("hash", &None, value).unwrap_err(); + assert_internal_500(err, "user id", value); + } + } + + fn userid_header(value: &'static str) -> HeaderMap { + let mut headers = HeaderMap::new(); + headers.insert( + HeaderName::from_static(INTRA_CLUSTER_USERID), + HeaderValue::from_static(value), + ); + headers + } + + #[test] + fn sync_userid_fallback_accepts_valid_username() { + check_sync_userid_fallback(&HeaderMap::new(), &[], "admin").unwrap(); + } + + #[test] + fn sync_userid_fallback_rejects_unreadable_username_before_mutation() { + // no cluster secret and no user id on the request: the sync would send + // the configured username, so it must fail here before the stream changes + for username in ["ad\nmin", "ad\u{7f}min", "josé", "用户"] { + let err = check_sync_userid_fallback(&HeaderMap::new(), &[], username).unwrap_err(); + assert_internal_500(err, "configured username", username); + } + } + + #[test] + fn sync_userid_fallback_skipped_when_userid_present() { + // the sync keeps an already-present user id, so the username is never sent + check_sync_userid_fallback(&userid_header("alice"), &[], "ad\nmin").unwrap(); + let intracluster = intracluster_stream_headers("hash", &None, "alice").unwrap(); + check_sync_userid_fallback(&HeaderMap::new(), &intracluster, "josé").unwrap(); + } +} diff --git a/src/handlers/http/modal/query/querier_rbac.rs b/src/handlers/http/modal/query/querier_rbac.rs index 4cf624eb2..cbd6af62b 100644 --- a/src/handlers/http/modal/query/querier_rbac.rs +++ b/src/handlers/http/modal/query/querier_rbac.rs @@ -49,7 +49,7 @@ pub async fn post_user( body: Option>, ) -> Result { let username = userid.into_inner(); - let tenant_id = get_tenant_id_from_request(&req); + let tenant_id = get_tenant_id_from_request(&req)?; let caller_userid = get_user_from_request(&req)?; validator::user_role_name(&username)?; let _guard = UPDATE_LOCK.lock().await; @@ -120,7 +120,7 @@ pub async fn delete_user( userid: web::Path, ) -> Result { let userid = userid.into_inner(); - let tenant_id = get_tenant_id_from_request(&req); + let tenant_id = get_tenant_id_from_request(&req)?; let caller_userid = get_user_from_request(&req)?; let tenant = tenant_id.as_deref().unwrap_or(DEFAULT_TENANT); let _guard = UPDATE_LOCK.lock().await; @@ -234,7 +234,7 @@ pub async fn add_roles_to_user( ) -> Result { let userid = userid.into_inner(); let roles_to_add = roles_to_add.into_inner(); - let tenant_id = get_tenant_id_from_request(&req); + let tenant_id = get_tenant_id_from_request(&req)?; let _guard = UPDATE_LOCK.lock().await; let caller_userid = get_user_from_request(&req)?; if !Users.contains(&userid, &tenant_id) { @@ -330,7 +330,7 @@ pub async fn remove_roles_from_user( ) -> Result { let userid = userid.into_inner(); let roles_to_remove = roles_to_remove.into_inner(); - let tenant_id = get_tenant_id_from_request(&req); + let tenant_id = get_tenant_id_from_request(&req)?; let caller_userid = get_user_from_request(&req)?; let tenant = tenant_id.as_deref().unwrap_or(DEFAULT_TENANT); let _guard = UPDATE_LOCK.lock().await; @@ -445,7 +445,7 @@ pub async fn post_gen_password( let userid = userid.into_inner(); let mut new_password = String::default(); let mut new_hash = String::default(); - let tenant_id = get_tenant_id_from_request(&req); + let tenant_id = get_tenant_id_from_request(&req)?; let caller_userid = get_user_from_request(&req)?; if Users.is_protected(&userid, &tenant_id) == Some(true) { return Err(RBACError::ProtectedUser); diff --git a/src/handlers/http/modal/query/querier_role.rs b/src/handlers/http/modal/query/querier_role.rs index 205e3d796..53c3c294b 100644 --- a/src/handlers/http/modal/query/querier_role.rs +++ b/src/handlers/http/modal/query/querier_role.rs @@ -56,7 +56,7 @@ pub async fn put( return Err(RoleError::SuperAdminPrivilege); } let name = name.into_inner(); - let tenant_id = get_tenant_id_from_request(&req); + let tenant_id = get_tenant_id_from_request(&req)?; let tenant = tenant_id.as_deref().unwrap_or(DEFAULT_TENANT); // extract caller userid early, before any session mutations let caller_userid = @@ -131,7 +131,7 @@ pub async fn delete( ) -> Result { let guard = UPDATE_LOCK.lock().await; let name = name.into_inner(); - let tenant_id = get_tenant_id_from_request(&req); + let tenant_id = get_tenant_id_from_request(&req)?; let tenant = tenant_id.as_deref().unwrap_or(DEFAULT_TENANT); // extract caller userid early, before any session mutations let caller_userid = diff --git a/src/handlers/http/modal/utils/ingest_utils.rs b/src/handlers/http/modal/utils/ingest_utils.rs index 93636d5a0..906b83c37 100644 --- a/src/handlers/http/modal/utils/ingest_utils.rs +++ b/src/handlers/http/modal/utils/ingest_utils.rs @@ -96,7 +96,7 @@ pub async fn process_otel_content( .get("Content-Type") .and_then(|h| h.to_str().ok()) .map(str::to_owned); - let tenant_id = get_tenant_id_from_request(req); + let tenant_id = get_tenant_id_from_request(req)?; let (s, r) = oneshot::channel(); let stream_name = stream_name.to_string(); let log_source = log_source.clone(); diff --git a/src/handlers/http/modal/utils/logstream_utils.rs b/src/handlers/http/modal/utils/logstream_utils.rs index 0a7b1b433..69df5391d 100644 --- a/src/handlers/http/modal/utils/logstream_utils.rs +++ b/src/handlers/http/modal/utils/logstream_utils.rs @@ -27,6 +27,7 @@ use crate::{ parse_dataset_labels, parse_dataset_tags, }, storage::StreamType, + utils::header_parsing::{ParseHeaderError, optional_header_str}, }; #[derive(Debug)] @@ -64,8 +65,39 @@ impl Default for PutStreamHeaders { } } -impl From<&HeaderMap> for PutStreamHeaders { - fn from(headers: &HeaderMap) -> Self { +impl TryFrom<&HeaderMap> for PutStreamHeaders { + type Error = ParseHeaderError; + + /// Parses the stream creation/update headers. + /// + /// Absent headers fall back to their defaults, but a present header whose + /// value is not visible ASCII is rejected instead of panicking or being + /// silently treated as absent. + fn try_from(headers: &HeaderMap) -> Result { + let time_partition_limit = optional_header_str(headers, TIME_PARTITION_LIMIT_KEY)? + .unwrap_or_default() + .to_string(); + let custom_partition = + optional_header_str(headers, CUSTOM_PARTITION_KEY)?.map(str::to_string); + let static_schema_flag = + optional_header_str(headers, STATIC_SCHEMA_FLAG)?.is_some_and(|v| v == "true"); + let update_stream_flag = + optional_header_str(headers, UPDATE_STREAM_KEY)?.is_some_and(|v| v == "true"); + // `StreamType::from` panics on unknown values, so match here and + // reject anything else with a 400 that does not echo the value. + let stream_type = match optional_header_str(headers, STREAM_TYPE_KEY)? { + None => StreamType::default(), + Some("UserDefined") => StreamType::UserDefined, + Some("Internal") => StreamType::Internal, + Some(_) => { + return Err(ParseHeaderError::UnexpectedHeader( + STREAM_TYPE_KEY.to_string(), + )); + } + }; + let log_source = + optional_header_str(headers, LOG_SOURCE_KEY)?.map_or(LogSource::default(), Into::into); + // Track raw header presence first so a present-but-unparseable header // (non-UTF-8 bytes, etc.) still trips downstream validation gates // instead of silently being treated as absent. @@ -77,27 +109,13 @@ impl From<&HeaderMap> for PutStreamHeaders { let telemetry_type_value = headers .get(TELEMETRY_TYPE_KEY) .and_then(|v| v.to_str().ok()); - PutStreamHeaders { - time_partition_limit: headers - .get(TIME_PARTITION_LIMIT_KEY) - .map_or("", |v| v.to_str().unwrap()) - .to_string(), - custom_partition: headers - .get(CUSTOM_PARTITION_KEY) - .map(|v| v.to_str().unwrap().to_string()), - static_schema_flag: headers - .get(STATIC_SCHEMA_FLAG) - .is_some_and(|v| v.to_str().unwrap() == "true"), - update_stream_flag: headers - .get(UPDATE_STREAM_KEY) - .is_some_and(|v| v.to_str().unwrap() == "true"), - stream_type: headers - .get(STREAM_TYPE_KEY) - .map(|v| StreamType::from(v.to_str().unwrap())) - .unwrap_or_default(), - log_source: headers - .get(LOG_SOURCE_KEY) - .map_or(LogSource::default(), |v| v.to_str().unwrap().into()), + Ok(PutStreamHeaders { + time_partition_limit, + custom_partition, + static_schema_flag, + update_stream_flag, + stream_type, + log_source, telemetry_type: telemetry_type_value.map_or(TelemetryType::Logs, TelemetryType::from), telemetry_type_set, dataset_tags: headers @@ -113,7 +131,7 @@ impl From<&HeaderMap> for PutStreamHeaders { .unwrap_or_default(), infer_timestamp: infer_timestamp_value.is_none_or(|v| !v.eq_ignore_ascii_case("false")), infer_timestamp_set, - } + }) } } @@ -122,42 +140,70 @@ mod tests { use actix_web::http::header::{HeaderMap, HeaderName, HeaderValue}; use super::PutStreamHeaders; - use crate::handlers::{INFER_TIMESTAMP_KEY, TELEMETRY_TYPE_KEY, TelemetryType}; + use crate::{ + event::format::LogSource, + handlers::{ + CUSTOM_PARTITION_KEY, INFER_TIMESTAMP_KEY, LOG_SOURCE_KEY, STATIC_SCHEMA_FLAG, + STREAM_TYPE_KEY, TELEMETRY_TYPE_KEY, TIME_PARTITION_LIMIT_KEY, TelemetryType, + UPDATE_STREAM_KEY, + }, + storage::StreamType, + utils::header_parsing::ParseHeaderError, + }; - fn headers_with_infer(value: &str) -> HeaderMap { + fn parse(headers: &HeaderMap) -> PutStreamHeaders { + PutStreamHeaders::try_from(headers).expect("headers should parse") + } + + fn headers_with(name: &'static str, value: HeaderValue) -> HeaderMap { let mut map = HeaderMap::new(); - map.insert( - HeaderName::from_static(INFER_TIMESTAMP_KEY), - HeaderValue::from_str(value).unwrap(), - ); + map.insert(HeaderName::from_static(name), value); map } + fn headers_with_infer(value: &str) -> HeaderMap { + headers_with(INFER_TIMESTAMP_KEY, HeaderValue::from_str(value).unwrap()) + } + + fn non_utf8() -> HeaderValue { + HeaderValue::from_bytes(&[0xff, 0xfe]).unwrap() + } + + /// Headers whose value used to be `to_str().unwrap()`ed. + const STRICT_HEADERS: [&str; 6] = [ + TIME_PARTITION_LIMIT_KEY, + CUSTOM_PARTITION_KEY, + STATIC_SCHEMA_FLAG, + UPDATE_STREAM_KEY, + STREAM_TYPE_KEY, + LOG_SOURCE_KEY, + ]; + #[test] fn defaults_to_true_when_header_absent() { let headers = HeaderMap::new(); - let parsed = PutStreamHeaders::from(&headers); + let parsed = parse(&headers); assert!(parsed.infer_timestamp); assert!(!parsed.infer_timestamp_set); } #[test] fn parses_false_value() { - let parsed = PutStreamHeaders::from(&headers_with_infer("false")); + let parsed = parse(&headers_with_infer("false")); assert!(!parsed.infer_timestamp); assert!(parsed.infer_timestamp_set); } #[test] fn parses_true_value() { - let parsed = PutStreamHeaders::from(&headers_with_infer("true")); + let parsed = parse(&headers_with_infer("true")); assert!(parsed.infer_timestamp); assert!(parsed.infer_timestamp_set); } #[test] fn case_insensitive_false() { - let parsed = PutStreamHeaders::from(&headers_with_infer("FALSE")); + let parsed = parse(&headers_with_infer("FALSE")); assert!(!parsed.infer_timestamp); assert!(parsed.infer_timestamp_set); } @@ -166,7 +212,7 @@ mod tests { fn unknown_value_treated_as_true() { // Anything not matching "false" (case-insensitive) keeps the default behavior // of inferring timestamps; flag is still considered explicitly set. - let parsed = PutStreamHeaders::from(&headers_with_infer("yes")); + let parsed = parse(&headers_with_infer("yes")); assert!(parsed.infer_timestamp); assert!(parsed.infer_timestamp_set); } @@ -174,7 +220,7 @@ mod tests { #[test] fn telemetry_type_set_flag_tracks_header_presence() { // Header absent -> flag is false, value defaults to Logs. - let parsed = PutStreamHeaders::from(&HeaderMap::new()); + let parsed = parse(&HeaderMap::new()); assert!(!parsed.telemetry_type_set); assert_eq!(parsed.telemetry_type, TelemetryType::Logs); @@ -184,7 +230,7 @@ mod tests { HeaderName::from_static(TELEMETRY_TYPE_KEY), HeaderValue::from_static("metrics"), ); - let parsed = PutStreamHeaders::from(&h); + let parsed = parse(&h); assert!(parsed.telemetry_type_set); assert_eq!(parsed.telemetry_type, TelemetryType::Metrics); } @@ -200,7 +246,7 @@ mod tests { HeaderName::from_static(INFER_TIMESTAMP_KEY), HeaderValue::from_bytes(&[0xff, 0xfe]).unwrap(), ); - let parsed = PutStreamHeaders::from(&h); + let parsed = parse(&h); assert!(parsed.infer_timestamp_set); // Value couldn't be parsed -> falls back to default (true), but the // _set flag still ensures downstream gates fire. @@ -214,8 +260,88 @@ mod tests { HeaderName::from_static(TELEMETRY_TYPE_KEY), HeaderValue::from_bytes(&[0xff, 0xfe]).unwrap(), ); - let parsed = PutStreamHeaders::from(&h); + let parsed = parse(&h); assert!(parsed.telemetry_type_set); assert_eq!(parsed.telemetry_type, TelemetryType::Logs); } + + #[test] + fn absent_strict_headers_use_defaults() { + let parsed = parse(&HeaderMap::new()); + assert_eq!(parsed.time_partition_limit, ""); + assert_eq!(parsed.custom_partition, None); + assert!(!parsed.static_schema_flag); + assert!(!parsed.update_stream_flag); + assert_eq!(parsed.stream_type, StreamType::UserDefined); + assert_eq!(parsed.log_source, LogSource::Json); + } + + #[test] + fn valid_strict_headers_are_parsed() { + let mut h = HeaderMap::new(); + for (name, value) in [ + (TIME_PARTITION_LIMIT_KEY, "30d"), + (CUSTOM_PARTITION_KEY, "region"), + (STATIC_SCHEMA_FLAG, "true"), + (UPDATE_STREAM_KEY, "true"), + (STREAM_TYPE_KEY, "Internal"), + (LOG_SOURCE_KEY, "otel-logs"), + ] { + h.insert( + HeaderName::from_static(name), + HeaderValue::from_static(value), + ); + } + let parsed = parse(&h); + assert_eq!(parsed.time_partition_limit, "30d"); + assert_eq!(parsed.custom_partition.as_deref(), Some("region")); + assert!(parsed.static_schema_flag); + assert!(parsed.update_stream_flag); + assert_eq!(parsed.stream_type, StreamType::Internal); + assert_eq!(parsed.log_source, LogSource::OtelLogs); + } + + #[test] + fn boolean_flags_require_exact_true() { + for flag in [STATIC_SCHEMA_FLAG, UPDATE_STREAM_KEY] { + for value in ["TRUE", "yes", "false", ""] { + let parsed = parse(&headers_with(flag, HeaderValue::from_static(value))); + assert!( + !parsed.static_schema_flag && !parsed.update_stream_flag, + "{flag}={value:?} must not enable the flag" + ); + } + } + } + + #[test] + fn custom_log_source_is_preserved() { + let parsed = parse(&headers_with( + LOG_SOURCE_KEY, + HeaderValue::from_static("Syslog"), + )); + assert_eq!(parsed.log_source, LogSource::Custom("syslog".to_string())); + } + + #[test] + fn malformed_strict_header_is_rejected() { + for name in STRICT_HEADERS { + let result = PutStreamHeaders::try_from(&headers_with(name, non_utf8())); + assert!( + matches!(result, Err(ParseHeaderError::InvalidValue)), + "{name} with non-UTF-8 bytes must be rejected, got {result:?}" + ); + } + } + + #[test] + fn unknown_stream_type_is_rejected_without_echo() { + let result = PutStreamHeaders::try_from(&headers_with( + STREAM_TYPE_KEY, + HeaderValue::from_static("secret-ish-value"), + )); + let err = result.expect_err("unknown stream type must be rejected"); + assert!(matches!(err, ParseHeaderError::UnexpectedHeader(_))); + assert!(!err.to_string().contains("secret-ish-value")); + } } diff --git a/src/handlers/http/oidc.rs b/src/handlers/http/oidc.rs index b9a7a420a..b0721e0bf 100644 --- a/src/handlers/http/oidc.rs +++ b/src/handlers/http/oidc.rs @@ -206,7 +206,8 @@ pub async fn reply_login( login_query: web::Query, ) -> Result { let oidc_client = OIDC_CLIENT.get().ok_or(OIDCError::Unauthorized)?; - let tenant_id = get_tenant_id_from_request(&req); + let tenant_id = get_tenant_id_from_request(&req) + .map_err(|error| OIDCError::BadRequest(error.to_string()))?; // An XHR caller forwards the code itself and receives the session as JSON; // a browser arrives from the IdP and is redirected on. Check either caller // before the code is redeemed: a forged callback must not create a session. diff --git a/src/handlers/http/otel_generator.rs b/src/handlers/http/otel_generator.rs index 7d77c73d8..8a23c5abb 100644 --- a/src/handlers/http/otel_generator.rs +++ b/src/handlers/http/otel_generator.rs @@ -136,7 +136,11 @@ pub async fn get_otel_generator_status(req: HttpRequest) -> HttpResponse { } fn request_tenant(req: &HttpRequest) -> Result, HttpResponse> { - let tenant_id = get_tenant_id_from_request(req); + let tenant_id = get_tenant_id_from_request(req).map_err(|error| { + HttpResponse::BadRequest().json(OtelGeneratorErrorResponse { + error: error.to_string(), + }) + })?; if PARSEABLE.options.is_multi_tenant() && tenant_id.is_none() { return Err(HttpResponse::BadRequest().json(OtelGeneratorErrorResponse { error: "X-P-Tenant header is required when multi-tenancy is enabled".to_string(), @@ -289,3 +293,25 @@ fn unavailable_in_mode() -> HttpResponse { ), }) } + +#[cfg(test)] +mod header_error_tests { + use actix_web::{http::header::HeaderValue, test::TestRequest}; + + use super::*; + use crate::handlers::TENANT_ID; + + #[test] + fn request_tenant_rejects_malformed_tenant_with_400() { + let req = TestRequest::default() + .insert_header(( + TENANT_ID, + HeaderValue::from_bytes(b"t\xff").expect("obs-text is a valid header value"), + )) + .to_http_request(); + let Err(response) = request_tenant(&req) else { + panic!("malformed tenant must be rejected"); + }; + assert_eq!(response.status(), actix_web::http::StatusCode::BAD_REQUEST); + } +} diff --git a/src/handlers/http/prism_logstream.rs b/src/handlers/http/prism_logstream.rs index 4f71941b0..395642def 100644 --- a/src/handlers/http/prism_logstream.rs +++ b/src/handlers/http/prism_logstream.rs @@ -32,7 +32,7 @@ pub async fn get_info( stream_name: Path, ) -> Result { let prism_logstream_info = - get_prism_logstream_info(&stream_name, &get_tenant_id_from_request(&req)).await?; + get_prism_logstream_info(&stream_name, &get_tenant_id_from_request(&req)?).await?; Ok(web::Json(prism_logstream_info)) } @@ -43,7 +43,7 @@ pub async fn post_datasets( req: HttpRequest, ) -> Result { let session_key = extract_session_key_from_req(&req)?; - let tenant_id = get_tenant_id_from_request(&req); + let tenant_id = get_tenant_id_from_request(&req)?; let dataset = dataset_req .map(|Json(r)| r) .unwrap_or_default() diff --git a/src/handlers/http/promql.rs b/src/handlers/http/promql.rs index 66473f8f4..0bd57905a 100644 --- a/src/handlers/http/promql.rs +++ b/src/handlers/http/promql.rs @@ -20,7 +20,7 @@ use crate::promql::{self, Labels, QueryPlan, Sample, Series}; use crate::rbac::{Users, role::Action}; use crate::utils::{ actix::extract_session_key_from_req, arrow::record_batches_to_json, get_tenant_id_from_request, - user_auth_for_datasets, + header_parsing::ParseHeaderError, user_auth_for_datasets, }; const MAX_RANGE_MS: i64 = 31 * 24 * 60 * 60 * 1000; @@ -43,8 +43,11 @@ static PREVIEW_QUERY_SLOTS: Lazy> = /// Alerts evaluate in their own slots so dashboard load cannot starve them. static ALERT_SLOTS: Lazy> = Lazy::new(|| Arc::new(Semaphore::new(ALERT_CAPACITY))); +/// Path prefix of every PromQL route; all of them answer errors as [`ApiError`]. +pub(crate) const SCOPE: &str = "/prometheus/api/v1"; + pub fn scope() -> Scope { - web::scope("/prometheus/api/v1") + web::scope(SCOPE) .service( web::resource("/query") .route(web::get().to(instant_get).authorize(Action::Query)) @@ -92,6 +95,10 @@ impl ApiError { message: message.into(), } } + /// A malformed request header. The message never includes the value. + pub(crate) fn invalid_header(error: ParseHeaderError) -> Self { + Self::bad(error.to_string()) + } fn auth(error: actix_web::Error) -> Self { Self { status: error.as_response_error().status_code(), @@ -338,7 +345,7 @@ async fn metadata(req: HttpRequest, name: Option) -> Result Result { - let tenant_id = get_tenant_id_from_request(&req); + let tenant_id = get_tenant_id_from_request(&req)?; let creds = extract_session_key_from_req(&req)?; query_internal(query_request, &creds, &tenant_id).await } @@ -423,7 +424,7 @@ pub async fn get_counts( ) -> Result { let creds = extract_session_key_from_req(&req)?; let permissions = Users.get_permissions(&creds); - let tenant_id = get_tenant_id_from_request(&req); + let tenant_id = get_tenant_id_from_request(&req)?; let body = counts_request.into_inner(); // does user have access to table? @@ -742,6 +743,10 @@ Description: {0}"# ParserError(#[from] ParserError), #[error(transparent)] MetastoreError(#[from] MetastoreError), + #[error("{0}")] + Header(#[from] ParseHeaderError), + #[error("{0}")] + HeaderBuild(#[from] HeaderBuildError), } impl actix_web::ResponseError for QueryError { @@ -750,6 +755,8 @@ impl actix_web::ResponseError for QueryError { QueryError::JsonParse(_) => StatusCode::INTERNAL_SERVER_ERROR, QueryError::Execute(e) => e.status_code(), QueryError::MetastoreError(e) => e.status_code(), + QueryError::Header(_) => StatusCode::BAD_REQUEST, + QueryError::HeaderBuild(e) => actix_web::ResponseError::status_code(e), _ => StatusCode::BAD_REQUEST, } } @@ -863,3 +870,54 @@ mod response_tests { } } } + +#[cfg(test)] +mod header_error_tests { + use actix_web::{ResponseError, http::header::HeaderValue, test::TestRequest}; + + use super::*; + use crate::handlers::TENANT_ID; + + #[test] + fn header_errors_map_to_expected_status() { + assert_eq!( + QueryError::from(ParseHeaderError::InvalidTenantId).status_code(), + StatusCode::BAD_REQUEST + ); + assert_eq!( + QueryError::from(HeaderBuildError::Input(ParseHeaderError::InvalidTenantId)) + .status_code(), + StatusCode::BAD_REQUEST + ); + assert_eq!( + QueryError::from(HeaderBuildError::Internal("querier token")).status_code(), + StatusCode::INTERNAL_SERVER_ERROR + ); + } + + #[actix_web::test] + async fn query_rejects_malformed_tenant() { + let req = TestRequest::default() + .insert_header(( + TENANT_ID, + HeaderValue::from_bytes(b"t\xff").expect("obs-text is a valid header value"), + )) + .to_http_request(); + let query_request = Query { + query: "SELECT 1".to_owned(), + start_time: "1h".to_owned(), + end_time: "now".to_owned(), + send_null: false, + fields: false, + streaming: false, + filter_tags: None, + }; + let Err(err) = query(req, query_request).await else { + panic!("malformed tenant must be rejected"); + }; + assert!(matches!( + err, + QueryError::Header(ParseHeaderError::InvalidTenantId) + )); + } +} diff --git a/src/handlers/http/query_context.rs b/src/handlers/http/query_context.rs index 23e768ddd..c2d163a0d 100644 --- a/src/handlers/http/query_context.rs +++ b/src/handlers/http/query_context.rs @@ -122,7 +122,7 @@ pub async fn query_context( context_request: Json, ) -> Result { let context_request = context_request.into_inner(); - let tenant_id = get_tenant_id_from_request(&req); + let tenant_id = get_tenant_id_from_request(&req)?; let span = Span::current(); span.record("dataset", tracing::field::display(&context_request.dataset)); span.record("tenant", tracing::field::debug(&tenant_id)); diff --git a/src/handlers/http/rbac.rs b/src/handlers/http/rbac.rs index b3a2b5f8f..1c107288a 100644 --- a/src/handlers/http/rbac.rs +++ b/src/handlers/http/rbac.rs @@ -28,7 +28,10 @@ use crate::{ utils::to_prism_user, }, storage::ObjectStorageError, - utils::get_tenant_id_from_request, + utils::{ + get_tenant_id_from_request, + header_parsing::{HeaderBuildError, ParseHeaderError}, + }, validator::{self, error::UsernameValidationError}, }; use actix_web::http::StatusCode; @@ -70,9 +73,9 @@ impl From<&user::User> for User { // Handler for GET /api/v1/user // returns list of all registered users -pub async fn list_users(req: HttpRequest) -> impl Responder { - let tenant_id = get_tenant_id_from_request(&req); - web::Json(list_users_internal(&tenant_id)) +pub async fn list_users(req: HttpRequest) -> Result { + let tenant_id = get_tenant_id_from_request(&req)?; + Ok(web::Json(list_users_internal(&tenant_id))) } /// Shared user-list implementation for HTTP handlers and in-process callers. @@ -82,8 +85,8 @@ pub fn list_users_internal(tenant_id: &Option) -> serde_json::Value { /// Handler for GET /api/v1/users /// returns list of all registered users along with their roles and other info -pub async fn list_users_prism(req: HttpRequest) -> impl Responder { - let tenant_id = get_tenant_id_from_request(&req); +pub async fn list_users_prism(req: HttpRequest) -> Result { + let tenant_id = get_tenant_id_from_request(&req)?; let tenant_id = tenant_id.as_deref().unwrap_or(DEFAULT_TENANT); // get all users let prism_users = match rbac::map::users().get(tenant_id) { @@ -100,7 +103,7 @@ pub async fn list_users_prism(req: HttpRequest) -> impl Responder { .collect_vec(), None => vec![], }; - web::Json(prism_users) + Ok(web::Json(prism_users)) } /// Function for GET /users/{userid} @@ -109,7 +112,7 @@ pub async fn get_prism_user( userid: Path, ) -> Result { let userid = userid.into_inner(); - let tenant_id = get_tenant_id_from_request(&req); + let tenant_id = get_tenant_id_from_request(&req)?; Ok(web::Json(get_user_internal(&userid, &tenant_id)?)) } @@ -138,7 +141,7 @@ pub async fn post_user( body: Option>, ) -> Result { let userid = userid.into_inner(); - let tenant_id = get_tenant_id_from_request(&req); + let tenant_id = get_tenant_id_from_request(&req)?; validator::user_role_name(&userid)?; let _guard = UPDATE_LOCK.lock().await; let mut metadata = get_metadata(&tenant_id).await?; @@ -205,7 +208,7 @@ pub async fn patch_user( web::Json(update): web::Json, ) -> Result { let userid = userid.into_inner(); - let tenant_id = get_tenant_id_from_request(&req); + let tenant_id = get_tenant_id_from_request(&req)?; if let Some(p) = Users.is_protected(&userid, &tenant_id) && p @@ -243,7 +246,7 @@ pub async fn post_gen_password( userid: web::Path, ) -> Result { let userid = userid.into_inner(); - let tenant_id = get_tenant_id_from_request(&req); + let tenant_id = get_tenant_id_from_request(&req)?; // fail this request if the user is protected if let Some(p) = Users.is_protected(&userid, &tenant_id) && p @@ -284,7 +287,7 @@ pub async fn get_role( userid: web::Path, ) -> Result { let userid = userid.into_inner(); - let tenant_id = get_tenant_id_from_request(&req); + let tenant_id = get_tenant_id_from_request(&req)?; Ok(web::Json(get_role_internal(&userid, &tenant_id)?)) } @@ -365,7 +368,7 @@ pub async fn delete_user( userid: web::Path, ) -> Result { let userid = userid.into_inner(); - let tenant_id = get_tenant_id_from_request(&req); + let tenant_id = get_tenant_id_from_request(&req)?; let _guard = UPDATE_LOCK.lock().await; // if user is a part of any groups then don't allow deletion if !Users.get_user_groups(&userid, &tenant_id).is_empty() { @@ -412,7 +415,7 @@ pub async fn add_roles_to_user( ) -> Result { let userid = userid.into_inner(); let roles_to_add = roles_to_add.into_inner(); - let tenant_id = get_tenant_id_from_request(&req); + let tenant_id = get_tenant_id_from_request(&req)?; let _guard = UPDATE_LOCK.lock().await; let tenant_str = tenant_id.as_deref().unwrap_or(DEFAULT_TENANT); if !Users.contains(&userid, &tenant_id) { @@ -503,7 +506,7 @@ pub async fn remove_roles_from_user( ) -> Result { let userid = userid.into_inner(); let roles_to_remove = roles_to_remove.into_inner(); - let tenant_id = get_tenant_id_from_request(&req); + let tenant_id = get_tenant_id_from_request(&req)?; let _guard = UPDATE_LOCK.lock().await; if !Users.contains(&userid, &tenant_id) { return Err(RBACError::UserDoesNotExist); @@ -631,6 +634,10 @@ pub enum RBACError { ResourceInUse(String), #[error("{0}")] InvalidDeletionRequest(String), + #[error("{0}")] + Header(#[from] ParseHeaderError), + #[error("{0}")] + HeaderBuild(#[from] HeaderBuildError), } impl actix_web::ResponseError for RBACError { @@ -656,6 +663,8 @@ impl actix_web::ResponseError for RBACError { Self::UserGroupNotEmpty(_) => StatusCode::BAD_REQUEST, Self::ResourceInUse(_) => StatusCode::BAD_REQUEST, Self::InvalidDeletionRequest(_) => StatusCode::BAD_REQUEST, + Self::Header(_) => StatusCode::BAD_REQUEST, + Self::HeaderBuild(e) => actix_web::ResponseError::status_code(e), } } @@ -724,3 +733,73 @@ mod tests { ); } } + +#[cfg(test)] +mod header_error_tests { + use actix_web::{ResponseError, http::header::HeaderValue, test::TestRequest}; + + use super::*; + use crate::handlers::TENANT_ID; + + fn malformed_tenant_request() -> HttpRequest { + TestRequest::default() + .insert_header(( + TENANT_ID, + HeaderValue::from_bytes(b"t\xff").expect("obs-text is a valid header value"), + )) + .to_http_request() + } + + #[test] + fn header_errors_map_to_expected_status() { + assert_eq!( + RBACError::from(ParseHeaderError::InvalidTenantId).status_code(), + StatusCode::BAD_REQUEST + ); + assert_eq!( + RBACError::from(HeaderBuildError::Input(ParseHeaderError::InvalidTenantId)) + .status_code(), + StatusCode::BAD_REQUEST + ); + assert_eq!( + RBACError::from(HeaderBuildError::Internal("node token")).status_code(), + StatusCode::INTERNAL_SERVER_ERROR + ); + } + + #[actix_web::test] + async fn list_users_rejects_malformed_tenant() { + let Err(err) = list_users(malformed_tenant_request()).await else { + panic!("malformed tenant must be rejected"); + }; + assert!(matches!( + err, + RBACError::Header(ParseHeaderError::InvalidTenantId) + )); + assert_eq!(err.status_code(), StatusCode::BAD_REQUEST); + } + + #[actix_web::test] + async fn list_users_prism_rejects_malformed_tenant() { + let Err(err) = list_users_prism(malformed_tenant_request()).await else { + panic!("malformed tenant must be rejected"); + }; + assert!(matches!( + err, + RBACError::Header(ParseHeaderError::InvalidTenantId) + )); + } + + #[actix_web::test] + async fn get_prism_user_rejects_malformed_tenant() { + let Err(err) = + get_prism_user(malformed_tenant_request(), Path::from("alice".to_owned())).await + else { + panic!("malformed tenant must be rejected"); + }; + assert!(matches!( + err, + RBACError::Header(ParseHeaderError::InvalidTenantId) + )); + } +} diff --git a/src/handlers/http/role.rs b/src/handlers/http/role.rs index 994b01f1d..832b350d2 100644 --- a/src/handlers/http/role.rs +++ b/src/handlers/http/role.rs @@ -36,7 +36,10 @@ use crate::{ user::UserType, }, storage::{ObjectStorageError, StorageMetadata}, - utils::get_tenant_id_from_request, + utils::{ + get_tenant_id_from_request, + header_parsing::{HeaderBuildError, ParseHeaderError}, + }, validator::{self, error::UsernameValidationError}, }; @@ -56,7 +59,7 @@ pub async fn put( return Err(RoleError::SuperAdminPrivilege); } let name = name.into_inner(); - let tenant_id = get_tenant_id_from_request(&req); + let tenant_id = get_tenant_id_from_request(&req)?; // validate the role name validator::user_role_name(&name).map_err(RoleError::ValidationError)?; @@ -111,7 +114,7 @@ pub async fn put( // Fetch role by name pub async fn get(req: HttpRequest, name: web::Path) -> Result { let name = name.into_inner(); - let tenant_id = get_tenant_id_from_request(&req); + let tenant_id = get_tenant_id_from_request(&req)?; Ok(web::Json(get_internal(&name, &tenant_id).await?)) } @@ -128,7 +131,7 @@ pub async fn get_internal(name: &str, tenant_id: &Option) -> Result Result { - let tenant_id = get_tenant_id_from_request(&req); + let tenant_id = get_tenant_id_from_request(&req)?; Ok(web::Json(list_internal(&tenant_id).await?)) } @@ -155,7 +158,7 @@ pub async fn delete( ) -> Result { let _guard = super::rbac::UPDATE_LOCK.lock().await; let name = name.into_inner(); - let tenant_id = get_tenant_id_from_request(&req); + let tenant_id = get_tenant_id_from_request(&req)?; let tenant = tenant_id.as_deref().unwrap_or(DEFAULT_TENANT); if let Some(tenant_roles) = roles().get(tenant) && let Some(role) = tenant_roles.get(&name) @@ -197,14 +200,14 @@ pub async fn put_default( req: HttpRequest, name: web::Json, ) -> Result { - let tenant_id = get_tenant_id_from_request(&req); + let tenant_id = get_tenant_id_from_request(&req)?; set_default(Some(name.into_inner()), &tenant_id).await?; Ok(HttpResponse::Ok().finish()) } // Handler for DELETE /api/v1/role/default pub async fn delete_default(req: HttpRequest) -> Result { - let tenant_id = get_tenant_id_from_request(&req); + let tenant_id = get_tenant_id_from_request(&req)?; set_default(None, &tenant_id).await?; Ok(HttpResponse::Ok().finish()) } @@ -262,7 +265,7 @@ async fn set_default(name: Option, tenant_id: &Option) -> Result // Handler for GET /api/v1/role/default // Delete existing role pub async fn get_default(req: HttpRequest) -> Result { - let tenant_id = get_tenant_id_from_request(&req); + let tenant_id = get_tenant_id_from_request(&req)?; Ok(web::Json(get_default_internal(&tenant_id))) } @@ -325,6 +328,10 @@ pub enum RoleError { ValidationError(#[from] UsernameValidationError), #[error("Cannot create a role with superadmin privilege.")] SuperAdminPrivilege, + #[error("{0}")] + Header(#[from] ParseHeaderError), + #[error("{0}")] + HeaderBuild(#[from] HeaderBuildError), } impl actix_web::ResponseError for RoleError { @@ -339,6 +346,8 @@ impl actix_web::ResponseError for RoleError { Self::SerdeError(_) => StatusCode::BAD_REQUEST, Self::Network(_) => StatusCode::BAD_GATEWAY, Self::ValidationError(_) => StatusCode::BAD_REQUEST, + Self::Header(_) => StatusCode::BAD_REQUEST, + Self::HeaderBuild(e) => actix_web::ResponseError::status_code(e), } } @@ -438,3 +447,46 @@ mod put_tests { ); } } + +#[cfg(test)] +mod header_error_tests { + use actix_web::{ResponseError, http::header::HeaderValue, test::TestRequest}; + + use super::*; + use crate::handlers::TENANT_ID; + + #[test] + fn header_errors_map_to_expected_status() { + assert_eq!( + RoleError::from(ParseHeaderError::InvalidTenantId).status_code(), + StatusCode::BAD_REQUEST + ); + assert_eq!( + RoleError::from(HeaderBuildError::Input(ParseHeaderError::InvalidTenantId)) + .status_code(), + StatusCode::BAD_REQUEST + ); + assert_eq!( + RoleError::from(HeaderBuildError::Internal("cluster secret hash")).status_code(), + StatusCode::INTERNAL_SERVER_ERROR + ); + } + + #[actix_web::test] + async fn list_rejects_malformed_tenant() { + let req = TestRequest::default() + .insert_header(( + TENANT_ID, + HeaderValue::from_bytes(b"t\xff").expect("obs-text is a valid header value"), + )) + .to_http_request(); + let Err(err) = list(req).await else { + panic!("malformed tenant must be rejected"); + }; + assert!(matches!( + err, + RoleError::Header(ParseHeaderError::InvalidTenantId) + )); + assert_eq!(err.status_code(), StatusCode::BAD_REQUEST); + } +} diff --git a/src/handlers/http/targets.rs b/src/handlers/http/targets.rs index ae9d312ea..369ad63fc 100644 --- a/src/handlers/http/targets.rs +++ b/src/handlers/http/targets.rs @@ -38,7 +38,7 @@ pub async fn post( req: HttpRequest, Json(target): Json, ) -> Result { - let tenant_id = get_tenant_id_from_request(&req); + let tenant_id = get_tenant_id_from_request(&req)?; Ok(web::Json(post_internal(target, &tenant_id).await?)) } @@ -58,7 +58,7 @@ pub async fn post_internal( // GET /targets pub async fn list(req: HttpRequest) -> Result { - let tenant_id = get_tenant_id_from_request(&req); + let tenant_id = get_tenant_id_from_request(&req)?; Ok(web::Json(list_internal(&tenant_id).await?)) } @@ -103,7 +103,7 @@ pub async fn list_internal( // GET /targets/{target_id} pub async fn get(req: HttpRequest, target_id: Path) -> Result { let target_id = target_id.into_inner(); - let tenant_id = get_tenant_id_from_request(&req); + let tenant_id = get_tenant_id_from_request(&req)?; Ok(web::Json(get_internal(target_id, &tenant_id).await?)) } @@ -139,7 +139,7 @@ pub async fn update( Json(mut target): Json, ) -> Result { let target_id = target_id.into_inner(); - let tenant_id = get_tenant_id_from_request(&req); + let tenant_id = get_tenant_id_from_request(&req)?; // if target_id does not exist, error let old_target = TARGETS.get_target_by_id(&target_id, &tenant_id).await?; @@ -164,7 +164,7 @@ pub async fn update( // DELETE /targets/{target_id} pub async fn delete(req: HttpRequest, target_id: Path) -> Result { let target_id = target_id.into_inner(); - let tenant_id = get_tenant_id_from_request(&req); + let tenant_id = get_tenant_id_from_request(&req)?; let target = TARGETS.delete(&target_id, &tenant_id).await?; Ok(web::Json(target.mask())) diff --git a/src/handlers/http/traces.rs b/src/handlers/http/traces.rs index 960b7a53f..3adb9cde5 100644 --- a/src/handlers/http/traces.rs +++ b/src/handlers/http/traces.rs @@ -37,6 +37,7 @@ use crate::{ actix::extract_session_key_from_req, arrow::record_batches_to_json, get_tenant_id_from_request, get_user_from_request, + header_parsing::ParseHeaderError, time::{TimeRange, parse_start_time_expression, parse_time_expression, truncate_to_minute}, }, }; @@ -166,9 +167,18 @@ pub enum TraceError { TraceNotFound(String), } +impl TraceError { + /// A malformed request header. The message never includes the value. + pub(crate) fn invalid_header(error: ParseHeaderError) -> Self { + Self::BadRequest(error.to_string()) + } +} + impl actix_web::ResponseError for TraceError { fn status_code(&self) -> StatusCode { match self { + Self::Query(QueryError::Header(_)) => StatusCode::BAD_REQUEST, + Self::Query(QueryError::HeaderBuild(e)) => actix_web::ResponseError::status_code(e), Self::Query(_) | Self::Internal(_) => StatusCode::INTERNAL_SERVER_ERROR, Self::TimeParse(_) | Self::BadRequest(_) => StatusCode::BAD_REQUEST, Self::StreamNotFound(_) | Self::TraceNotFound(_) => StatusCode::NOT_FOUND, @@ -185,7 +195,7 @@ pub async fn list_traces( req: HttpRequest, Json(body): Json, ) -> Result { - let tenant_id = get_tenant_id_from_request(&req); + let tenant_id = get_tenant_id_from_request(&req).map_err(TraceError::invalid_header)?; let target = query_target(&req, &tenant_id)?; let response = list_traces_with_target(body, &tenant_id, target).await?; Ok(HttpResponse::Ok().json(response)) @@ -280,7 +290,7 @@ pub async fn get_trace_detail( req: HttpRequest, Json(body): Json, ) -> Result { - let tenant_id = get_tenant_id_from_request(&req); + let tenant_id = get_tenant_id_from_request(&req).map_err(TraceError::invalid_header)?; let target = query_target(&req, &tenant_id)?; let response = get_trace_detail_with_target(body, &tenant_id, target).await?; Ok(HttpResponse::Ok().json(response)) @@ -1256,3 +1266,48 @@ FROM (VALUES assert_eq!(context.table, "\"traces\"\"archive\""); } } + +#[cfg(test)] +mod header_error_tests { + use actix_web::{ResponseError, http::header::HeaderValue, test::TestRequest}; + + use super::*; + use crate::handlers::TENANT_ID; + use crate::utils::header_parsing::{HeaderBuildError, ParseHeaderError}; + + #[test] + fn nested_query_header_errors_keep_their_status() { + assert_eq!( + TraceError::Query(QueryError::Header(ParseHeaderError::InvalidTenantId)).status_code(), + StatusCode::BAD_REQUEST + ); + assert_eq!( + TraceError::Query(QueryError::HeaderBuild(HeaderBuildError::Internal( + "querier token" + ))) + .status_code(), + StatusCode::INTERNAL_SERVER_ERROR + ); + } + + #[actix_web::test] + async fn list_traces_rejects_malformed_tenant_with_400() { + let req = TestRequest::default() + .insert_header(( + TENANT_ID, + HeaderValue::from_bytes(b"t\xff").expect("obs-text is a valid header value"), + )) + .to_http_request(); + let body: TraceListRequest = serde_json::from_value(serde_json::json!({ + "dataset": "traces", + "startTime": "1h", + "endTime": "now" + })) + .expect("minimal trace list request"); + let Err(err) = list_traces(req, Json(body)).await else { + panic!("malformed tenant must be rejected"); + }; + assert!(matches!(err, TraceError::BadRequest(_))); + assert_eq!(err.status_code(), StatusCode::BAD_REQUEST); + } +} diff --git a/src/handlers/http/users/dashboards.rs b/src/handlers/http/users/dashboards.rs index b773fd8e6..225e01347 100644 --- a/src/handlers/http/users/dashboards.rs +++ b/src/handlers/http/users/dashboards.rs @@ -27,7 +27,10 @@ use crate::{ dashboard_service::{self, DashboardActor, DashboardMutation, DashboardWriteOptions}, dashboards::{Dashboard, Tile, validate_dashboard_id}, }, - utils::{get_hash, get_tenant_id_from_request, get_user_and_tenant_from_request, is_admin}, + utils::{ + get_hash, get_tenant_id_from_request, get_user_and_tenant_from_request, + header_parsing::ParseHeaderError, is_admin, + }, }; use actix_web::{ HttpRequest, HttpResponse, Responder, @@ -108,7 +111,7 @@ pub async fn list_dashboards(req: HttpRequest) -> Result Result { Ok(web::Json( - list_tags_internal(&get_tenant_id_from_request(&req)).await?, + list_tags_internal(&get_tenant_id_from_request(&req)?).await?, )) } @@ -459,6 +462,8 @@ pub enum DashboardError { message: String, details: Value, }, + #[error("{0}")] + Header(#[from] ParseHeaderError), } impl DashboardError { pub fn api( @@ -490,7 +495,7 @@ impl actix_web::ResponseError for DashboardError { fn status_code(&self) -> StatusCode { match self { Self::ObjectStorage(_) | Self::Custom(_) => StatusCode::INTERNAL_SERVER_ERROR, - Self::Serde(_) | Self::Metadata(_) | Self::InvalidQueryParameter => { + Self::Serde(_) | Self::Metadata(_) | Self::InvalidQueryParameter | Self::Header(_) => { StatusCode::BAD_REQUEST } Self::UserDoesNotExist(_) => StatusCode::NOT_FOUND, @@ -725,3 +730,20 @@ mod compatibility_tests { ); } } + +#[cfg(test)] +mod header_error_tests { + use actix_web::ResponseError; + + use super::*; + + #[test] + fn header_error_is_plaintext_bad_request() { + let err = DashboardError::from(ParseHeaderError::InvalidTenantId); + assert_eq!(err.status_code(), StatusCode::BAD_REQUEST); + assert_eq!( + err.to_string(), + "Invalid tenant id found in header [tenant]" + ); + } +} diff --git a/src/parseable/mod.rs b/src/parseable/mod.rs index cd127adbf..06bb5a5c7 100644 --- a/src/parseable/mod.rs +++ b/src/parseable/mod.rs @@ -691,7 +691,7 @@ impl Parseable { dataset_labels, infer_timestamp, infer_timestamp_set, - } = headers.into(); + } = PutStreamHeaders::try_from(headers)?; // x-p-infer-timestamp can only be set during stream creation, not on update. // It is only valid for otel-metrics datasets. diff --git a/src/prism/logstream/mod.rs b/src/prism/logstream/mod.rs index ec7e62fab..9f9a6d87c 100644 --- a/src/prism/logstream/mod.rs +++ b/src/prism/logstream/mod.rs @@ -376,6 +376,8 @@ pub enum PrismLogstreamError { SerdeError(#[from] serde_json::Error), #[error("ReqwestError: {0}")] ReqwestError(#[from] reqwest::Error), + #[error("Header: {0}")] + Header(#[from] crate::utils::header_parsing::ParseHeaderError), } impl actix_web::ResponseError for PrismLogstreamError { @@ -385,12 +387,17 @@ impl actix_web::ResponseError for PrismLogstreamError { PrismLogstreamError::StreamError(e) => e.status_code(), PrismLogstreamError::StreamNotFound(_) => StatusCode::NOT_FOUND, PrismLogstreamError::Hottier(_) => StatusCode::INTERNAL_SERVER_ERROR, + PrismLogstreamError::Query(QueryError::Header(_)) => StatusCode::BAD_REQUEST, + PrismLogstreamError::Query(QueryError::HeaderBuild(e)) => { + actix_web::ResponseError::status_code(e) + } PrismLogstreamError::Query(_) => StatusCode::INTERNAL_SERVER_ERROR, PrismLogstreamError::TimeParse(_) => StatusCode::NOT_FOUND, PrismLogstreamError::Execute(_) => StatusCode::INTERNAL_SERVER_ERROR, PrismLogstreamError::SerdeError(_) => StatusCode::INTERNAL_SERVER_ERROR, PrismLogstreamError::ReqwestError(_) => StatusCode::INTERNAL_SERVER_ERROR, PrismLogstreamError::Auth(_) => StatusCode::UNAUTHORIZED, + PrismLogstreamError::Header(_) => StatusCode::BAD_REQUEST, } } @@ -400,3 +407,41 @@ impl actix_web::ResponseError for PrismLogstreamError { .body(self.to_string()) } } + +#[cfg(test)] +mod header_error_tests { + use actix_web::ResponseError; + + use super::*; + use crate::utils::header_parsing::{HeaderBuildError, ParseHeaderError}; + + #[test] + fn header_errors_map_to_expected_status() { + assert_eq!( + PrismLogstreamError::from(ParseHeaderError::InvalidTenantId).status_code(), + StatusCode::BAD_REQUEST + ); + assert_eq!( + PrismLogstreamError::from(QueryError::Header(ParseHeaderError::InvalidTenantId)) + .status_code(), + StatusCode::BAD_REQUEST + ); + assert_eq!( + PrismLogstreamError::from(QueryError::HeaderBuild(HeaderBuildError::Internal( + "querier token" + ))) + .status_code(), + StatusCode::INTERNAL_SERVER_ERROR + ); + assert_eq!( + PrismLogstreamError::from(StreamError::Header(ParseHeaderError::InvalidTenantId)) + .status_code(), + StatusCode::BAD_REQUEST + ); + // unrelated query errors keep their existing 500 mapping + assert_eq!( + PrismLogstreamError::from(QueryError::EmptyQuery).status_code(), + StatusCode::INTERNAL_SERVER_ERROR + ); + } +} diff --git a/src/sse/mod.rs b/src/sse/mod.rs index 3b8b36345..6d41133d2 100644 --- a/src/sse/mod.rs +++ b/src/sse/mod.rs @@ -183,7 +183,7 @@ impl Broadcaster { pub async fn register_sse_client( req: HttpRequest, ) -> Result>>, actix_web::Error> { - let tenant_id = get_tenant_id_from_request(&req); + let tenant_id = get_tenant_id_from_request(&req)?; let session = extract_session_key_from_req(&req).unwrap(); let sessionid = match session { SessionKey::SessionId(ulid) => ulid, diff --git a/src/storage/field_stats.rs b/src/storage/field_stats.rs index 3f81d2dbe..ee6304c86 100644 --- a/src/storage/field_stats.rs +++ b/src/storage/field_stats.rs @@ -39,6 +39,9 @@ use crate::tenants::TENANT_METADATA; use crate::utils::create_intracluster_auth_headermap; use crate::utils::get_tenant_id_from_request; use crate::utils::get_user_from_request; +use crate::utils::header_parsing::{ + HeaderBuildError, ParseHeaderError, generated_text_header_value, +}; use crate::utils::json::apply_generic_flattening_for_partition; use actix_web::HttpRequest; use actix_web::HttpResponse; @@ -856,7 +859,7 @@ pub async fn get_dataset_stats( ) -> Result { let offset = dataset_stats_request.offset.unwrap_or(0); let limit = dataset_stats_request.limit.unwrap_or(5); - let tenant_id = get_tenant_id_from_request(&req); + let tenant_id = get_tenant_id_from_request(&req)?; let sql = if dataset_stats_request.fields.is_empty() { build_stats_sql(&dataset_stats_request.dataset_name, None, offset, limit) } else { @@ -893,34 +896,16 @@ pub async fn get_dataset_stats( } Mode::Prism => { let tenant = tenant_id.as_deref().unwrap_or(DEFAULT_TENANT); + let userid = get_user_from_request(&req).map_err(|_| QueryError::Unauthorized)?; let auth = if let Some((_, hash)) = CLUSTER_SECRET.get() { - let mut map = actix_web::http::header::HeaderMap::new(); - if let Some(header) = TENANT_METADATA.get_global_query_auth(tenant) { - map.insert( - HeaderName::from_static("authorization"), - HeaderValue::from_str(&header).unwrap(), - ); - } - let userid = get_user_from_request(&req).unwrap(); - map.insert( - HeaderName::from_static(CLUSTER_SECRET_HEADER), - HeaderValue::from_str(hash).unwrap(), - ); - map.insert( - HeaderName::from_static("intra-cluster-tenant"), - HeaderValue::from_str(tenant).unwrap(), - ); - map.insert( - HeaderName::from_static("intra-cluster-userid"), - HeaderValue::from_str(&userid).unwrap(), - ); - Some(map) + Some(prism_forwarding_headers( + hash, + tenant, + &userid, + TENANT_METADATA.get_global_query_auth(tenant).as_deref(), + )?) } else { - let auth = create_intracluster_auth_headermap( - req.headers(), - "", - &get_user_from_request(&req).unwrap(), - ); + let auth = create_intracluster_auth_headermap(req.headers(), "", &userid)?; let mut map = HeaderMap::new(); for (key, value) in auth.iter() { @@ -955,6 +940,39 @@ pub async fn get_dataset_stats( Ok(response) } +/// Headers for forwarding a Prism dataset-stats query to a querier with the +/// cluster secret. All values are server-side (stored global query auth, +/// cluster secret hash, the authenticated user id) apart from the tenant, +/// which was already validated from the request header. +fn prism_forwarding_headers( + hash: &str, + tenant: &str, + userid: &str, + global_query_auth: Option<&str>, +) -> Result { + let mut map = HeaderMap::new(); + if let Some(header) = global_query_auth { + map.insert( + HeaderName::from_static("authorization"), + generated_text_header_value(header, "global query auth")?, + ); + } + map.insert( + HeaderName::from_static(CLUSTER_SECRET_HEADER), + generated_text_header_value(hash, "cluster secret hash")?, + ); + map.insert( + HeaderName::from_static("intra-cluster-tenant"), + HeaderValue::from_str(tenant) + .map_err(|_| HeaderBuildError::Input(ParseHeaderError::InvalidTenantId))?, + ); + map.insert( + HeaderName::from_static("intra-cluster-userid"), + generated_text_header_value(userid, "user id")?, + ); + Ok(map) +} + pub fn transform_query_results( rows: Vec, ) -> Result, QueryError> { @@ -1495,3 +1513,68 @@ mod tests { ); } } + +#[cfg(test)] +mod forwarding_header_tests { + use super::*; + + #[test] + fn prism_forwarding_headers_include_auth_secret_tenant_and_user() { + let map = prism_forwarding_headers("hash", "acme", "alice", Some("Basic abc")) + .expect("valid values build headers"); + assert_eq!(map.get("authorization").unwrap(), "Basic abc"); + assert_eq!(map.get(CLUSTER_SECRET_HEADER).unwrap(), "hash"); + assert_eq!(map.get("intra-cluster-tenant").unwrap(), "acme"); + assert_eq!(map.get("intra-cluster-userid").unwrap(), "alice"); + } + + #[test] + fn prism_forwarding_headers_skip_missing_global_auth() { + let map = prism_forwarding_headers("hash", "acme", "alice", None).unwrap(); + assert!(map.get("authorization").is_none()); + } + + #[test] + fn prism_forwarding_headers_reject_invalid_stored_values_as_internal() { + for (hash, user, auth, field) in [ + ("ha\nsh", "alice", None, "cluster secret hash"), + ("hash", "al\nice", None, "user id"), + ("hash", "alice", Some("Basic\r\nx"), "global query auth"), + ] { + let err = prism_forwarding_headers(hash, "acme", user, auth).unwrap_err(); + assert!(matches!(err, HeaderBuildError::Internal(f) if f == field)); + let status = actix_web::ResponseError::status_code(&QueryError::from(err)); + assert_eq!(status, actix_web::http::StatusCode::INTERNAL_SERVER_ERROR); + } + } + + #[test] + fn prism_forwarding_headers_reject_unreadable_stored_text_as_internal() { + // `HeaderValue::from_str` accepts the non-ASCII values but the querier's `to_str` doesn't + for value in ["josé", "用户", "ctl\u{1}"] { + let auth = format!("Basic {value}"); + for (hash, user, auth, field) in [ + (value, "alice", None, "cluster secret hash"), + ("hash", value, None, "user id"), + ("hash", "alice", Some(auth.as_str()), "global query auth"), + ] { + let err = prism_forwarding_headers(hash, "acme", user, auth).unwrap_err(); + assert!(matches!(err, HeaderBuildError::Internal(f) if f == field)); + assert!(!err.to_string().contains(value)); + let status = actix_web::ResponseError::status_code(&QueryError::from(err)); + assert_eq!(status, actix_web::http::StatusCode::INTERNAL_SERVER_ERROR); + } + } + } + + #[test] + fn prism_forwarding_headers_reject_invalid_tenant_as_input() { + let err = prism_forwarding_headers("hash", "ac\nme", "alice", None).unwrap_err(); + assert!(matches!( + err, + HeaderBuildError::Input(ParseHeaderError::InvalidTenantId) + )); + let status = actix_web::ResponseError::status_code(&QueryError::from(err)); + assert_eq!(status, actix_web::http::StatusCode::BAD_REQUEST); + } +} diff --git a/src/tool_executor.rs b/src/tool_executor.rs index 17ed3e416..5779152c0 100644 --- a/src/tool_executor.rs +++ b/src/tool_executor.rs @@ -1,6 +1,11 @@ use std::{collections::HashMap, ops::ControlFlow}; -use actix_web::{HttpRequest, HttpResponse, body::to_bytes_limited, http::header::HeaderMap, web}; +use actix_web::{ + HttpRequest, HttpResponse, ResponseError, + body::to_bytes_limited, + http::{StatusCode, header::HeaderMap}, + web, +}; use chrono::{DateTime, Duration, LocalResult, NaiveDateTime, Offset, TimeZone, Utc}; use chrono_tz::Tz; use futures::{StreamExt, stream}; @@ -34,6 +39,7 @@ use crate::{ utils::{ actix::extract_session_key_from_req, get_tenant_id_from_request, + header_parsing::ParseHeaderError, time::{TimeRange, parse_start_time_expression, parse_time_expression, truncate_to_minute}, }, }; @@ -2039,6 +2045,22 @@ pub async fn execute_oss_tool_with_query_headers( execute_oss_tool_inner(name, arguments, session_key, tenant_id, query_headers).await } +/// A malformed request header on a tool call, answered in the same +/// `ToolCallResult` envelope as the endpoint's other errors. The message +/// never includes the value. +#[derive(Debug, thiserror::Error)] +#[error(transparent)] +pub(crate) struct ToolCallHeaderError(pub(crate) ParseHeaderError); + +impl ResponseError for ToolCallHeaderError { + fn status_code(&self) -> StatusCode { + StatusCode::BAD_REQUEST + } + fn error_response(&self) -> HttpResponse { + HttpResponse::build(self.status_code()).json(ToolCallResult::error(self.0.to_string())) + } +} + /// Generic execution endpoint used by external MCP adapters. pub async fn call_tool_registry( req: HttpRequest, @@ -2069,7 +2091,10 @@ pub async fn call_tool_registry( if let Err(error) = spec.validate_arguments(&call.arguments) { return HttpResponse::BadRequest().json(ToolCallResult::error(error)); } - let tenant_id = get_tenant_id_from_request(&req); + let tenant_id = match get_tenant_id_from_request(&req) { + Ok(tenant_id) => tenant_id, + Err(error) => return ToolCallHeaderError(error).error_response(), + }; if crate::users::dashboard_tools::is_dashboard_tool(spec.name) { return match crate::users::dashboard_tools::execute(&req, spec.name, &call.arguments).await { diff --git a/src/users/dashboard_runtime.rs b/src/users/dashboard_runtime.rs index 3e0632c6f..624c254d4 100644 --- a/src/users/dashboard_runtime.rs +++ b/src/users/dashboard_runtime.rs @@ -26,6 +26,7 @@ use crate::users::{ }; use crate::utils::{ actix::extract_session_key_from_req, arrow::record_batches_to_json, get_tenant_id_from_request, + header_parsing::ParseHeaderError, }; use crate::{ handlers::http::middleware, @@ -64,6 +65,10 @@ struct PreviewRequest { fn bad(message: impl Into) -> DashboardError { DashboardError::api(400, "INVALID_PREVIEW", message, json!({})) } +/// A malformed request header. The message never includes the value. +pub(crate) fn invalid_header(error: ParseHeaderError) -> DashboardError { + DashboardError::api(400, "INVALID_HEADER", error.to_string(), json!({})) +} fn text(value: &Value) -> &str { value.as_str().unwrap_or("") } @@ -755,7 +760,8 @@ async fn sql_rows( return Err("This tile has no SQL query".into()); } let creds = extract_session_key_from_req(req).map_err(|e| e.to_string())?; - let tenant = get_tenant_id_from_request(req); + // callers run authorize_runtime first, which rejects a malformed tenant with 400 + let tenant = get_tenant_id_from_request(req).map_err(|e| e.to_string())?; let query = Query { query: bounded_sql(sql, limit), start_time: start.into(), @@ -1131,7 +1137,7 @@ fn authorize_runtime(req: &HttpRequest, action: Action) -> Result<(), DashboardE json!({}), ) })?; - let tenant = get_tenant_id_from_request(req); + let tenant = get_tenant_id_from_request(req).map_err(invalid_header)?; if let Response::Suspended(message) = middleware::check_suspension_for_tenant(tenant.as_deref(), action) { @@ -1176,7 +1182,7 @@ async fn preview_inner(req: &HttpRequest, request: Value) -> Result Result<(), DashboardError> { ) })?; let action = required_action(name); - let tenant = crate::utils::get_tenant_id_from_request(req); + let tenant = crate::utils::get_tenant_id_from_request(req).map_err(|error| { + DashboardError::api(400, "INVALID_HEADER", error.to_string(), json!({})) + })?; if let Response::Suspended(message) = middleware::check_suspension_for_tenant(tenant.as_deref(), action) { @@ -385,3 +387,25 @@ pub fn output_schema(name: &str) -> Option { json!({"type":"object","properties":{"result":result,"error":{"type":"object","properties":{"code":{"type":"string"},"message":{"type":"string"},"status":{"type":"integer"}},"required":["message"]}},"anyOf":[{"required":["result"]},{"required":["error"]}]}), ) } + +#[cfg(test)] +mod header_error_tests { + use actix_web::{ResponseError, http::header::HeaderValue, test::TestRequest}; + + use super::*; + use crate::handlers::TENANT_ID; + + #[test] + fn authorize_rejects_malformed_tenant_with_json_400() { + let req = TestRequest::default() + .insert_header(("authorization", "Basic YWxpY2U6cGFzcw==")) + .insert_header(( + TENANT_ID, + HeaderValue::from_bytes(b"t\xff").expect("obs-text is a valid header value"), + )) + .to_http_request(); + let err = authorize(&req, "list_dashboards").unwrap_err(); + assert_eq!(err.status_code(), actix_web::http::StatusCode::BAD_REQUEST); + assert_eq!(err.detail()["code"], "INVALID_HEADER"); + } +} diff --git a/src/utils/header_parsing.rs b/src/utils/header_parsing.rs index 83b9c340a..f4cb18329 100644 --- a/src/utils/header_parsing.rs +++ b/src/utils/header_parsing.rs @@ -17,6 +17,7 @@ */ use actix_web::http::StatusCode; +use actix_web::http::header::HeaderMap; use actix_web::{HttpResponse, ResponseError}; #[derive(Debug, thiserror::Error)] @@ -50,3 +51,173 @@ impl ResponseError for ParseHeaderError { HttpResponse::build(self.status_code()).body(self.to_string()) } } + +/// Reads an optional header as visible-ASCII text. +/// +/// A missing header is `Ok(None)`; a header that is present but cannot be +/// represented as visible ASCII is `Err(ParseHeaderError::InvalidValue)`. +/// The offending value is never included in the error. +pub fn optional_header_str<'a>( + headers: &'a HeaderMap, + name: &str, +) -> Result, ParseHeaderError> { + headers + .get(name) + .map(|value| value.to_str().map_err(|_| ParseHeaderError::InvalidValue)) + .transpose() +} + +/// Failure while building headers for an outgoing (intra-cluster) request. +/// +/// `Input` wraps a problem with the incoming client request and maps to 400. +/// `Internal` covers values the server stored or generated itself (cluster +/// secret hash, user id, node token) and maps to a generic 500; the static +/// string only names the field and never contains the value. +#[derive(Debug, thiserror::Error)] +pub enum HeaderBuildError { + #[error(transparent)] + Input(#[from] ParseHeaderError), + #[error("Failed to build internal request headers")] + Internal(&'static str), +} + +impl ResponseError for HeaderBuildError { + fn status_code(&self) -> StatusCode { + match self { + HeaderBuildError::Input(e) => e.status_code(), + HeaderBuildError::Internal(_) => StatusCode::INTERNAL_SERVER_ERROR, + } + } + + fn error_response(&self) -> HttpResponse { + HttpResponse::build(self.status_code()).body(self.to_string()) + } +} + +fn generated_text_error(field: &'static str) -> HeaderBuildError { + tracing::error!("failed to encode {field} as an intra-cluster header value"); + HeaderBuildError::Internal(field) +} + +/// Encodes text the server stored or generated (user id, cluster secret hash, +/// node token, ...) as a header value for another node. +/// +/// `HeaderValue::from_str` also accepts obs-text such as `é`, which the +/// receiving node then fails to read back with `to_str`, so the value must +/// also pass `to_str` here. Any failure is `HeaderBuildError::Internal`; the +/// value is never logged or included in the error. Opaque bytes copied from +/// the incoming request (Authorization, Cookie) must not go through this. +pub fn generated_text_header_value( + value: &str, + field: &'static str, +) -> Result { + actix_web::http::header::HeaderValue::from_str(value) + .ok() + .filter(|value| value.to_str().is_ok()) + .ok_or_else(|| generated_text_error(field)) +} + +/// [`generated_text_header_value`] for a `reqwest` request. +pub fn generated_text_reqwest_header_value( + value: &str, + field: &'static str, +) -> Result { + reqwest::header::HeaderValue::from_str(value) + .ok() + .filter(|value| value.to_str().is_ok()) + .ok_or_else(|| generated_text_error(field)) +} + +#[cfg(test)] +mod tests { + use super::*; + use actix_web::http::header::{HeaderName, HeaderValue}; + + fn headers_with(name: &'static str, value: HeaderValue) -> HeaderMap { + let mut headers = HeaderMap::new(); + headers.insert(HeaderName::from_static(name), value); + headers + } + + #[test] + fn optional_header_str_missing_is_none() { + let headers = HeaderMap::new(); + assert!(matches!( + optional_header_str(&headers, "x-p-test"), + Ok(None) + )); + } + + #[test] + fn optional_header_str_valid_is_some() { + let headers = headers_with("x-p-test", HeaderValue::from_static("value")); + assert!(matches!( + optional_header_str(&headers, "x-p-test"), + Ok(Some("value")) + )); + } + + #[test] + fn optional_header_str_non_ascii_is_invalid_value() { + let headers = headers_with( + "x-p-test", + HeaderValue::from_bytes(b"caf\xc3\xa9").expect("obs-text is a valid header value"), + ); + assert!(matches!( + optional_header_str(&headers, "x-p-test"), + Err(ParseHeaderError::InvalidValue) + )); + } + + #[test] + fn header_build_error_status_codes() { + assert_eq!( + HeaderBuildError::Input(ParseHeaderError::InvalidTenantId).status_code(), + StatusCode::BAD_REQUEST + ); + assert_eq!( + HeaderBuildError::Input(ParseHeaderError::InvalidValue).status_code(), + StatusCode::BAD_REQUEST + ); + assert_eq!( + HeaderBuildError::Internal("node token").status_code(), + StatusCode::INTERNAL_SERVER_ERROR + ); + } + + #[test] + fn header_build_error_internal_body_is_generic() { + let err = HeaderBuildError::Internal("cluster secret hash"); + assert_eq!(err.to_string(), "Failed to build internal request headers"); + } + + // `from_str` alone accepts the non-ASCII values; the receiver's `to_str` doesn't + const UNREADABLE_TEXT: [&str; 5] = ["josé", "用户", "c1\u{80}", "nl\nx", "del\u{7f}"]; + + #[test] + fn generated_text_header_value_accepts_visible_ascii() { + for value in ["alice", "Basic dXNlcjpwYXNz", "a b\tc!~"] { + let actix = generated_text_header_value(value, "user id").unwrap(); + assert_eq!(actix.to_str().unwrap(), value); + let reqwest = generated_text_reqwest_header_value(value, "user id").unwrap(); + assert_eq!(reqwest.to_str().unwrap(), value); + } + } + + #[test] + fn generated_text_header_value_rejects_unreadable_text_as_internal() { + assert!(HeaderValue::from_str("josé").is_ok()); + for value in UNREADABLE_TEXT { + for err in [ + generated_text_header_value(value, "user id").unwrap_err(), + generated_text_reqwest_header_value(value, "user id").unwrap_err(), + ] { + assert!(matches!(err, HeaderBuildError::Internal("user id"))); + assert_eq!(err.status_code(), StatusCode::INTERNAL_SERVER_ERROR); + let body = err.to_string(); + assert!(!body.contains(value)); + assert!(!format!("{err:?}").contains(value)); + } + } + } +} diff --git a/src/utils/mod.rs b/src/utils/mod.rs index bbf2de6e2..325f61a2b 100644 --- a/src/utils/mod.rs +++ b/src/utils/mod.rs @@ -35,6 +35,9 @@ use crate::query::resolve_stream_names; use crate::rbac::Users; use crate::rbac::map::{SessionKey, sessions}; use crate::rbac::role::{Action, ParseableResourceType, Permission}; +use crate::utils::header_parsing::{ + HeaderBuildError, ParseHeaderError, generated_text_reqwest_header_value, optional_header_str, +}; use actix::extract_session_key_from_req; use actix_web::dev::ServiceRequest; use actix_web::http::header::HeaderMap; @@ -76,7 +79,7 @@ pub fn mutate_request_with_tenant(req: &mut ServiceRequest) { let creds = BasicAuth::extract(req.request()).into_inner(); if let Ok(basic) = &creds { - Users.mutate_request_with_basic_user(basic.user_id(), basic.password().unwrap(), req); + Users.mutate_request_with_basic_user(basic.user_id(), basic.password().unwrap_or(""), req); } else if let Some(cookie) = req.cookie("session") && let Ok(ulid) = ulid::Ulid::from_string(cookie.value()) { @@ -87,11 +90,9 @@ pub fn mutate_request_with_tenant(req: &mut ServiceRequest) { pub fn get_user_from_request(req: &HttpRequest) -> Result { let session_key = extract_session_key_from_req(req).map_err(|_| RBACError::UserDoesNotExist)?; - let user_id = Users.get_userid_from_session(&session_key); - if user_id.is_none() { - return Err(RBACError::UserDoesNotExist); - } - let (user_id, _) = user_id.unwrap(); + let (user_id, _) = Users + .get_userid_from_session(&session_key) + .ok_or(RBACError::UserDoesNotExist)?; Ok(user_id) } @@ -121,10 +122,19 @@ pub fn get_user_and_tenant_from_request( } } -pub fn get_tenant_id_from_request(req: &HttpRequest) -> Option { - req.headers() - .get(TENANT_ID) - .map(|tenant_value| tenant_value.to_str().unwrap().to_owned()) +/// Returns the tenant id from the `x-p-tenant` request header. +/// +/// A missing header is `Ok(None)`. A header that is present but not visible +/// ASCII is rejected with `ParseHeaderError::InvalidTenantId` (400) instead of +/// silently falling back to the default tenant. +pub fn get_tenant_id_from_request(req: &HttpRequest) -> Result, ParseHeaderError> { + tenant_id_from_headers(req.headers()) +} + +fn tenant_id_from_headers(headers: &HeaderMap) -> Result, ParseHeaderError> { + optional_header_str(headers, TENANT_ID) + .map(|tenant| tenant.map(str::to_owned)) + .map_err(|_| ParseHeaderError::InvalidTenantId) } pub fn get_tenant_id_from_key(key: &SessionKey) -> Option { @@ -253,48 +263,280 @@ pub fn is_admin_for_session(session_key: &SessionKey) -> bool { false } +/// Builds the auth headers used to forward a request to another node. +/// +/// A malformed tenant header on the incoming request is a client error +/// (`HeaderBuildError::Input`, 400). Values the server stored or generated +/// itself (cluster secret hash, user id, node token) failing to encode is a +/// server error (`HeaderBuildError::Internal`, 500). Header values are never +/// included in the error. pub fn create_intracluster_auth_headermap( req: &HeaderMap, node_token: &str, userid: &str, -) -> reqwest::header::HeaderMap { +) -> Result { + build_intracluster_auth_headermap( + req, + node_token, + userid, + CLUSTER_SECRET.get().map(|(_, hash)| hash.as_str()), + ) +} + +fn copied_header_value( + value: &actix_web::http::header::HeaderValue, +) -> Result { + reqwest::header::HeaderValue::from_bytes(value.as_bytes()) + .map_err(|_| HeaderBuildError::Input(ParseHeaderError::InvalidValue)) +} + +fn build_intracluster_auth_headermap( + req: &HeaderMap, + node_token: &str, + userid: &str, + cluster_secret_hash: Option<&str>, +) -> Result { let mut map = reqwest::header::HeaderMap::new(); - let tenant_id = req - .get(TENANT_ID) - .map(|tenant_value| tenant_value.to_str().unwrap().to_owned()); - if let Some((_, hash)) = CLUSTER_SECRET.get() { + // reject a malformed client-supplied tenant regardless of the auth branch + let tenant_id = tenant_id_from_headers(req)?; + if let Some(hash) = cluster_secret_hash { + let tenant_value = + reqwest::header::HeaderValue::from_str(tenant_id.as_deref().unwrap_or(DEFAULT_TENANT)) + .map_err(|_| HeaderBuildError::Input(ParseHeaderError::InvalidTenantId))?; // also insert server secret along with other required headers for query forwarding map.insert( reqwest::header::HeaderName::from_static(CLUSTER_SECRET_HEADER), - reqwest::header::HeaderValue::from_str(hash).unwrap(), + generated_text_reqwest_header_value(hash, "cluster secret hash")?, ); map.insert( reqwest::header::HeaderName::from_static("intra-cluster-tenant"), - reqwest::header::HeaderValue::from_str(tenant_id.as_deref().unwrap_or(DEFAULT_TENANT)) - .unwrap(), + tenant_value, ); map.insert( reqwest::header::HeaderName::from_static("intra-cluster-userid"), - reqwest::header::HeaderValue::from_str(userid).unwrap(), + generated_text_reqwest_header_value(userid, "user id")?, ); } else if let Some(auth) = req.get(actix_web::http::header::AUTHORIZATION) { - map.insert( - reqwest::header::AUTHORIZATION, - reqwest::header::HeaderValue::from_bytes(auth.as_bytes()).unwrap(), - ); + map.insert(reqwest::header::AUTHORIZATION, copied_header_value(auth)?); } else if req.contains_key(actix_web::http::header::COOKIE) { // multiple cookies for cookie in req.get_all(actix_web::http::header::COOKIE) { - map.insert( - reqwest::header::COOKIE, - reqwest::header::HeaderValue::from_bytes(cookie.as_bytes()).unwrap(), - ); + map.insert(reqwest::header::COOKIE, copied_header_value(cookie)?); } } else { map.insert( reqwest::header::AUTHORIZATION, - reqwest::header::HeaderValue::from_str(node_token).unwrap(), + generated_text_reqwest_header_value(node_token, "node token")?, + ); + } + Ok(map) +} + +#[cfg(test)] +mod tests { + use super::*; + use actix_web::http::header::{HeaderName, HeaderValue}; + use actix_web::test::TestRequest; + + fn invalid_tenant_value() -> HeaderValue { + HeaderValue::from_bytes(b"tenant\xff").expect("obs-text is a valid header value") + } + + fn headers(pairs: &[(&'static str, HeaderValue)]) -> HeaderMap { + let mut map = HeaderMap::new(); + for (name, value) in pairs { + map.append(HeaderName::from_static(name), value.clone()); + } + map + } + + #[test] + fn tenant_id_missing_is_none() { + let req = TestRequest::default().to_http_request(); + assert!(matches!(get_tenant_id_from_request(&req), Ok(None))); + } + + #[test] + fn tenant_id_valid_is_some() { + let req = TestRequest::default() + .insert_header((TENANT_ID, "acme")) + .to_http_request(); + assert_eq!( + get_tenant_id_from_request(&req).unwrap().as_deref(), + Some("acme") + ); + } + + #[test] + fn tenant_id_invalid_bytes_is_rejected() { + let req = TestRequest::default() + .insert_header((TENANT_ID, invalid_tenant_value())) + .to_http_request(); + assert!(matches!( + get_tenant_id_from_request(&req), + Err(ParseHeaderError::InvalidTenantId) + )); + } + + #[test] + fn forwarding_with_cluster_secret_sets_intracluster_headers() { + let req = headers(&[(TENANT_ID, HeaderValue::from_static("acme"))]); + let map = build_intracluster_auth_headermap(&req, "Basic token", "alice", Some("hash")) + .expect("valid inputs build headers"); + assert_eq!(map.get(CLUSTER_SECRET_HEADER).unwrap(), "hash"); + assert_eq!(map.get("intra-cluster-tenant").unwrap(), "acme"); + assert_eq!(map.get("intra-cluster-userid").unwrap(), "alice"); + assert!(map.get(reqwest::header::AUTHORIZATION).is_none()); + } + + #[test] + fn forwarding_with_cluster_secret_defaults_missing_tenant() { + let map = build_intracluster_auth_headermap(&HeaderMap::new(), "t", "alice", Some("hash")) + .expect("valid inputs build headers"); + assert_eq!(map.get("intra-cluster-tenant").unwrap(), DEFAULT_TENANT); + } + + #[test] + fn forwarding_rejects_malformed_tenant_as_input_error() { + let req = headers(&[(TENANT_ID, invalid_tenant_value())]); + for secret in [Some("hash"), None] { + let err = build_intracluster_auth_headermap(&req, "t", "alice", secret).unwrap_err(); + assert!(matches!( + err, + HeaderBuildError::Input(ParseHeaderError::InvalidTenantId) + )); + assert_eq!( + actix_web::ResponseError::status_code(&err), + actix_web::http::StatusCode::BAD_REQUEST + ); + } + } + + #[test] + fn forwarding_rejects_invalid_secret_hash_as_internal_without_leaking() { + let err = build_intracluster_auth_headermap( + &HeaderMap::new(), + "t", + "alice", + Some("secret\nvalue"), + ) + .unwrap_err(); + assert!(matches!( + err, + HeaderBuildError::Internal("cluster secret hash") + )); + assert_eq!( + actix_web::ResponseError::status_code(&err), + actix_web::http::StatusCode::INTERNAL_SERVER_ERROR + ); + assert!(!err.to_string().contains("secret\nvalue")); + assert!(!format!("{err:?}").contains("secret\nvalue")); + } + + #[test] + fn forwarding_rejects_invalid_userid_as_internal() { + let err = + build_intracluster_auth_headermap(&HeaderMap::new(), "t", "al\u{7f}ice", Some("hash")) + .unwrap_err(); + assert!(matches!(err, HeaderBuildError::Internal("user id"))); + } + + #[test] + fn forwarding_copies_authorization_header() { + let req = headers(&[( + "authorization", + HeaderValue::from_static("Basic dXNlcjpwYXNz"), + )]); + let map = build_intracluster_auth_headermap(&req, "node", "alice", None).unwrap(); + assert_eq!( + map.get(reqwest::header::AUTHORIZATION).unwrap(), + "Basic dXNlcjpwYXNz" + ); + assert!(map.get(reqwest::header::COOKIE).is_none()); + } + + #[test] + fn forwarding_copies_cookie_header() { + let req = headers(&[("cookie", HeaderValue::from_static("session=abc"))]); + let map = build_intracluster_auth_headermap(&req, "node", "alice", None).unwrap(); + assert_eq!(map.get(reqwest::header::COOKIE).unwrap(), "session=abc"); + assert!(map.get(reqwest::header::AUTHORIZATION).is_none()); + } + + #[test] + fn forwarding_copies_non_ascii_authorization_bytes() { + // obs-text is valid in both http versions and must not be rejected + let req = headers(&[( + "authorization", + HeaderValue::from_bytes(b"Basic \xe9").unwrap(), + )]); + let map = build_intracluster_auth_headermap(&req, "node", "alice", None).unwrap(); + assert_eq!( + map.get(reqwest::header::AUTHORIZATION).unwrap().as_bytes(), + b"Basic \xe9" + ); + } + + #[test] + fn forwarding_falls_back_to_node_token() { + let map = build_intracluster_auth_headermap(&HeaderMap::new(), "Basic node", "alice", None) + .unwrap(); + assert_eq!( + map.get(reqwest::header::AUTHORIZATION).unwrap(), + "Basic node" + ); + } + + #[test] + fn forwarding_rejects_invalid_node_token_as_internal_without_leaking() { + let err = build_intracluster_auth_headermap(&HeaderMap::new(), "tok\r\nen", "alice", None) + .unwrap_err(); + assert!(matches!(err, HeaderBuildError::Internal("node token"))); + assert!(!err.to_string().contains("tok")); + } + + // text the receiving node can't read with `to_str`; `from_str` accepts the non-ASCII ones + const UNREADABLE_TEXT: [&str; 3] = ["josé", "用户", "ctl\u{1}"]; + + fn assert_internal_without_leaking(err: HeaderBuildError, field: &str, value: &str) { + assert!(matches!(err, HeaderBuildError::Internal(f) if f == field)); + assert_eq!( + actix_web::ResponseError::status_code(&err), + actix_web::http::StatusCode::INTERNAL_SERVER_ERROR + ); + assert!(!err.to_string().contains(value)); + assert!(!format!("{err:?}").contains(value)); + } + + #[test] + fn forwarding_rejects_unreadable_generated_text_as_internal() { + for value in UNREADABLE_TEXT { + let err = + build_intracluster_auth_headermap(&HeaderMap::new(), "t", "alice", Some(value)) + .unwrap_err(); + assert_internal_without_leaking(err, "cluster secret hash", value); + + let err = + build_intracluster_auth_headermap(&HeaderMap::new(), "t", value, Some("hash")) + .unwrap_err(); + assert_internal_without_leaking(err, "user id", value); + + let token = format!("Basic {value}"); + let err = build_intracluster_auth_headermap(&HeaderMap::new(), &token, "alice", None) + .unwrap_err(); + assert_internal_without_leaking(err, "node token", value); + } + } + + #[test] + fn forwarding_ignores_unused_generated_text() { + // a copied credential wins over the node token, so a bad token is never encoded + let req = headers(&[("cookie", HeaderValue::from_bytes(b"s=caf\xc3\xa9").unwrap())]); + let map = build_intracluster_auth_headermap(&req, "Basic josé", "josé", None).unwrap(); + assert_eq!( + map.get(reqwest::header::COOKIE).unwrap().as_bytes(), + b"s=caf\xc3\xa9" ); + assert!(map.get("intra-cluster-userid").is_none()); } - map } From baa4b6e3b6c3170f7b0a4a63dae864dc3aaa9c4f Mon Sep 17 00:00:00 2001 From: VibeSWE <338779468+VibeSWE@users.noreply.github.com> Date: Sun, 25 Oct 2026 18:22:52 -0500 Subject: [PATCH 2/2] fix: preserve header validation errors across API and sync paths --- src/apikeys.rs | 28 ++++ src/handlers/http/apikeys.rs | 8 +- src/handlers/http/cluster/mod.rs | 88 ++++++++++- src/handlers/http/middleware.rs | 121 ++++++++++++++- src/handlers/http/modal/query/querier_rbac.rs | 140 +++++++++++++----- src/handlers/http/modal/query/querier_role.rs | 80 +++++++++- src/handlers/http/oidc.rs | 88 +++++++++-- src/handlers/http/otel_generator.rs | 29 +++- 8 files changed, 509 insertions(+), 73 deletions(-) diff --git a/src/apikeys.rs b/src/apikeys.rs index b6dc03a4f..f43b340b9 100644 --- a/src/apikeys.rs +++ b/src/apikeys.rs @@ -98,8 +98,36 @@ impl actix_web::ResponseError for ApiKeyError { #[cfg(test)] mod tests { use super::*; + use crate::{ + handlers::http::rbac::RBACError, + utils::header_parsing::{HeaderBuildError, ParseHeaderError}, + }; + use actix_web::{ResponseError, http::StatusCode}; use serde_json::json; + #[test] + fn sync_header_errors_preserve_status_without_revocation_wrapping() { + let error = ApiKeyError::from(RBACError::HeaderBuild(HeaderBuildError::Internal( + "configured username", + ))); + assert_eq!(error.status_code(), StatusCode::INTERNAL_SERVER_ERROR); + assert_eq!( + error.to_string(), + "Failed to build internal request headers" + ); + + let error = ApiKeyError::from(RBACError::HeaderBuild(HeaderBuildError::Input( + ParseHeaderError::InvalidTenantId, + ))); + assert_eq!(error.status_code(), StatusCode::BAD_REQUEST); + + let network_failure = ApiKeyError::RevocationSyncFailed("network failure".into()); + assert_eq!( + network_failure.status_code(), + StatusCode::SERVICE_UNAVAILABLE + ); + } + #[test] fn ttl_accepts_inclusive_boundaries_and_no_expiration() { for ttl in [json!(null), json!(1800), json!(7_776_000)] { diff --git a/src/handlers/http/apikeys.rs b/src/handlers/http/apikeys.rs index 57573ba30..003bcfec7 100644 --- a/src/handlers/http/apikeys.rs +++ b/src/handlers/http/apikeys.rs @@ -331,7 +331,7 @@ pub async fn create_api_key( // can be slow and must not block other create/delete requests. drop(guard); - // Best-effort sync to other live nodes (reuses the standard user sync). + // Node delivery is best effort; invalid forwarding headers still fail the request. let caller_userid = user .as_api_key() .map(|k| k.created_by.clone()) @@ -345,6 +345,9 @@ pub async fn create_api_key( ) .await { + if matches!(&e, RBACError::HeaderBuild(_)) { + return Err(e.into()); + } tracing::error!("Failed to sync API-key user creation: {e}"); } @@ -393,6 +396,9 @@ pub async fn delete_api_key( ) .await { + if matches!(&e, RBACError::HeaderBuild(_)) { + return Err(e.into()); + } tracing::error!("Failed to sync API-key user deletion: {e}"); return Err(ApiKeyError::RevocationSyncFailed(e.to_string())); } diff --git a/src/handlers/http/cluster/mod.rs b/src/handlers/http/cluster/mod.rs index ef7c6e048..1130557d5 100644 --- a/src/handlers/http/cluster/mod.rs +++ b/src/handlers/http/cluster/mod.rs @@ -342,7 +342,7 @@ pub async fn for_each_live_node(tenant_id: &Option, api_fn: F where F: Fn(NodeMetadata) -> Fut + Clone + Send + Sync + 'static, Fut: Future> + Send, - E: From + Send + Sync + 'static, + E: From + std::error::Error + Send + Sync + 'static, { let mut nodes = Vec::new(); @@ -380,12 +380,34 @@ where })) .await; - // collect results + collect_node_results(results) +} + +/// A best-effort caller may ignore ordinary node failures, but must still see +/// a header validation failure from any node. Preserve first-error ordering +/// when none of the nodes failed header validation. +fn collect_node_results( + results: impl IntoIterator>, +) -> Result<(), E> { + let mut first_error = None; for result in results { - result?; + if let Err(error) = result { + let mut cause: &(dyn std::error::Error + 'static) = &error; + loop { + if cause.is::() { + return Err(error); + } + match cause.source() { + Some(source) => cause = source, + None => break, + } + } + if first_error.is_none() { + first_error = Some(error); + } + } } - - Ok(()) + first_error.map_or(Ok(()), Err) } pub const INTRA_CLUSTER_USERID: &str = "intra-cluster-userid"; @@ -2071,6 +2093,62 @@ pub async fn send_query_request( mod header_build_tests { use super::*; + #[test] + fn node_header_failure_wins_over_earlier_network_failure() { + for header_first in [false, true] { + let network = RBACError::Network( + reqwest::Client::new() + .get("invalid url") + .build() + .unwrap_err(), + ); + let header = RBACError::HeaderBuild(HeaderBuildError::Internal("node token")); + let results = if header_first { + vec![Err(header), Err(network)] + } else { + vec![Err(network), Err(header)] + }; + let error = collect_node_results(results).unwrap_err(); + assert!(matches!(error, RBACError::HeaderBuild(_))); + assert_eq!( + actix_web::ResponseError::status_code(&error), + StatusCode::INTERNAL_SERVER_ERROR + ); + assert_eq!( + error.to_string(), + "Failed to build internal request headers" + ); + } + } + + #[test] + fn node_header_input_failure_preserves_400_through_role_wrapper() { + let error = collect_node_results(vec![ + Err(RoleError::Anyhow(anyhow::anyhow!("node unavailable"))), + Err(RoleError::HeaderBuild(HeaderBuildError::Input( + crate::utils::header_parsing::ParseHeaderError::InvalidTenantId, + ))), + ]) + .unwrap_err(); + assert_eq!( + actix_web::ResponseError::status_code(&error), + StatusCode::BAD_REQUEST + ); + assert!(matches!(error, RoleError::HeaderBuild(_))); + } + + #[test] + fn node_results_preserve_first_ordinary_error_and_success() { + let error = collect_node_results(vec![ + Err(RBACError::Anyhow(anyhow::anyhow!("first node failed"))), + Err(RBACError::UserDoesNotExist), + ]) + .unwrap_err(); + assert!(matches!(error, RBACError::Anyhow(_))); + assert_eq!(error.to_string(), "Error: first node failed"); + assert!(collect_node_results::([Ok(()), Ok(())]).is_ok()); + } + #[test] fn basic_token_userid_header_extracts_user() { let token = format!("Basic {}", BASE64_STANDARD.encode("alice:secret")); diff --git a/src/handlers/http/middleware.rs b/src/handlers/http/middleware.rs index e8dce8771..9f5ce104a 100644 --- a/src/handlers/http/middleware.rs +++ b/src/handlers/http/middleware.rs @@ -41,8 +41,9 @@ use crate::{ AUTHORIZATION_KEY, KINESIS_COMMON_ATTRIBUTES_KEY, LOG_SOURCE_KEY, LOG_SOURCE_KINESIS, STREAM_NAME_HEADER_KEY, TENANT_ID, http::{ - base_path, ingest::PostError, modal::OIDC_CLIENT, prism_base_path, promql, - rbac::RBACError, traces::TraceError, + base_path, ingest::PostError, modal::OIDC_CLIENT, + otel_generator::OtelGeneratorHeaderError, prism_base_path, promql, rbac::RBACError, + traces::TraceError, }, }, option::Mode, @@ -429,8 +430,8 @@ fn tenant_header_error(req: &ServiceRequest, error: ParseHeaderError) -> Error { let Some(pattern) = req.resource_map().match_pattern(req.match_info().as_str()) else { return PostError::Header(error).into(); }; - // Registered by `promql::scope` and by the server's dashboards, llm and - // traces scopes. + // Registered by `promql::scope` and by the server's dashboards, llm, + // traces and OTel generator scopes (the generator under both prefixes). if pattern.starts_with(&format!("{}/", promql::SCOPE)) { promql::ApiError::invalid_header(error).into() } else if pattern.starts_with(&format!("{}/services/traces/", prism_base_path())) { @@ -439,6 +440,10 @@ fn tenant_header_error(req: &ServiceRequest, error: ParseHeaderError) -> Error { dashboard_runtime::invalid_header(error).into() } else if pattern == format!("{}/llm/tools/call", prism_base_path()) { ToolCallHeaderError(error).into() + } else if pattern == format!("{}/otel_generator", prism_base_path()) + || pattern == format!("{}/otel_generator", base_path()) + { + OtelGeneratorHeaderError(error).into() } else { PostError::Header(error).into() } @@ -1719,6 +1724,11 @@ mod api_key_auth_tests { Ok(response) => response.map_into_boxed_body().into_parts().1, Err(error) => error.error_response(), }; + reply_parts(response).await + } + + /// Status, content type and body of `response`. + async fn reply_parts(response: HttpResponse) -> (StatusCode, Option, Bytes) { let content_type = response .headers() .get(header::CONTENT_TYPE) @@ -1734,7 +1744,7 @@ mod api_key_auth_tests { let message = ParseHeaderError::InvalidTenantId.to_string(); match family { "promql" => Some(json!({"status":"error","errorType":"bad_data","error":message})), - "traces" => Some(json!({"error":message})), + "traces" | "otel" => Some(json!({"error":message})), "preview" => Some(json!({ "error": {"code":"INVALID_HEADER","message":message,"details":{}}, "message": message, @@ -1818,6 +1828,12 @@ mod api_key_auth_tests { method: counting_auth, }) }; + let otel_stub = || { + web::resource("") + .route(stub(web::get())) + .route(stub(web::post())) + .route(stub(web::delete())) + }; let app = actix_test::init_service( App::new() .service( @@ -1833,6 +1849,11 @@ mod api_key_auth_tests { web::scope("/dashboards") .route("/preview", stub(web::post())) .route("/{dashboard_id}", stub(web::post())), + ) + .service( + web::scope("/otel_generator") + .service(otel_stub()) + .route("/status", stub(web::get())), ), ) .service( @@ -1846,7 +1867,8 @@ mod api_key_auth_tests { web::scope("/services/traces") .route("/list", stub(web::post())) .route("/detail", stub(web::post())), - ), + ) + .service(web::scope("/otel_generator").service(otel_stub())), ), ) .await; @@ -1871,11 +1893,23 @@ mod api_key_auth_tests { (Method::POST, "/api/v1/dashboards/%70review", "preview"), (Method::POST, "/api/prism/v1/llm/tools/call", "tools"), (Method::POST, "/api/prism/v1/llm/tools/%63all", "tools"), + // The generator is served under the prism prefix by query + // servers and under the base prefix by ingestors. + (Method::GET, "/api/prism/v1/otel_generator", "otel"), + (Method::POST, "/api/prism/v1/otel_generator", "otel"), + (Method::DELETE, "/api/prism/v1/otel_generator", "otel"), + (Method::GET, "/api/prism/v1/%6ftel_generator", "otel"), + (Method::GET, "/api/v1/otel_generator", "otel"), + (Method::POST, "/api/v1/otel_generator", "otel"), + (Method::DELETE, "/api/v1/otel_generator", "otel"), + (Method::DELETE, "/api/v1/otel_%67enerator", "otel"), // Ordinary endpoints keep the plain-text error, including the - // SQL endpoint whose path is a suffix of a PromQL one. + // SQL endpoint whose path is a suffix of a PromQL one, and a + // route nested under the generator's. (Method::POST, "/api/v1/query", "plain"), (Method::POST, "/api/v1/dashboards/abc", "plain"), (Method::POST, "/api/prism/v1/llm/tools/list", "plain"), + (Method::GET, "/api/v1/otel_generator/status", "plain"), ]; let active_sessions = sessions().get_active_sessions().len(); for (method, uri, family) in &cases { @@ -1975,6 +2009,79 @@ mod api_key_auth_tests { assert!(!has_sessions(&api_user)); } + #[actix_web::test] + async fn production_otel_generator_routes_reject_malformed_tenant_as_json() { + use crate::handlers::http::modal::server::Server; + + let _state = auth_state().await; + let (api_user, secret) = api_key_user(None); + let credentials = [ + ("basic", basic_admin()), + ("api key", ("x-api-key", text(&secret))), + ]; + // Mounted exactly as the query and ingest servers mount them. The + // handlers need `PARSEABLE`, so every request must stop in `Auth`. + let app = actix_test::init_service( + App::new() + .service( + web::scope(&prism_base_path()).service(Server::get_otel_generator_webscope()), + ) + .service( + web::scope(&base_path()).service(Server::get_otel_generator_ingest_webscope()), + ), + ) + .await; + + let cluster = raw_cluster_headers(&api_user); + let mut cases = Vec::new(); + for method in [Method::GET, Method::POST, Method::DELETE] { + for uri in [ + "/api/prism/v1/otel_generator", + "/api/prism/v1/%6ftel_generator", + ] { + for tenant_header in [TENANT_ID, INTRA_CLUSTER_TENANT_HEADER] { + cases.push((method.clone(), uri, tenant_header, Vec::new())); + } + } + // The ingest scope first verifies the forwarded cluster + // credentials, which carry a well-formed `intra-cluster-tenant`. + for uri in ["/api/v1/otel_generator", "/api/v1/otel_%67enerator"] { + cases.push((method.clone(), uri, TENANT_ID, cluster.clone())); + } + } + let active_sessions = sessions().get_active_sessions().len(); + for (method, uri, tenant_header, forwarded) in &cases { + for (credential, auth) in &credentials { + for tenant in malformed_tenants() { + let context = format!("{method} {uri} with {credential} and {tenant_header}"); + let mut req = actix_test::TestRequest::default() + .method(method.clone()) + .uri(uri) + .insert_header(auth.clone()); + for header in forwarded { + req = req.insert_header(header.clone()); + } + let req = req.insert_header((*tenant_header, tenant)).to_request(); + let error = actix_test::try_call_service(&app, req) + .await + .err() + .unwrap_or_else(|| panic!("{context}: reached the handler")); + assert_tenant_rejection( + "otel", + &context, + reply_parts(error.error_response()).await, + ); + assert_eq!( + sessions().get_active_sessions().len(), + active_sessions, + "{context}: a session was left behind" + ); + } + } + } + assert!(!has_sessions(&api_user)); + } + #[actix_web::test] async fn intra_cluster_rejects_malformed_credentials_before_inner_service() { let _state = auth_state().await; diff --git a/src/handlers/http/modal/query/querier_rbac.rs b/src/handlers/http/modal/query/querier_rbac.rs index cbd6af62b..c42eb8931 100644 --- a/src/handlers/http/modal/query/querier_rbac.rs +++ b/src/handlers/http/modal/query/querier_rbac.rs @@ -107,9 +107,10 @@ pub async fn post_user( // let created_role = user_roles.clone(); Users.put_user(user.clone()); - if let Err(e) = sync_user_creation(&req, user, &None, &tenant_id, &caller_userid).await { - tracing::error!("{e}"); - }; + best_effort_sync( + sync_user_creation(&req, user, &None, &tenant_id, &caller_userid).await, + "Failed to sync user creation to cluster nodes", + )?; Ok(password) } @@ -164,14 +165,14 @@ pub async fn delete_user( put_metadata(&metadata, &tenant_id).await?; apply_user_removal_to_groups(&userid, &tenant_id, &groups_to_update); - if let Err(e) = - sync_user_deletion_with_ingestors(&req, &userid, &tenant_id, &caller_userid).await - { - tracing::error!("{e}"); - }; + let synced = best_effort_sync( + sync_user_deletion_with_ingestors(&req, &userid, &tenant_id, &caller_userid).await, + "Failed to sync user deletion to cluster nodes", + ); // update in mem table Users.delete_user(&userid, &tenant_id); + synced?; Ok(HttpResponse::Ok().json(format!("deleted user: {username}"))) } @@ -303,21 +304,22 @@ pub async fn add_roles_to_user( put_metadata(&metadata, &tenant_id).await?; // sync to other nodes before updating in-memory (which invalidates sessions) - if let Err(e) = sync_users_with_roles_with_ingestors( - &req, - &userid, - &roles_to_add, - "add", - &tenant_id, - &caller_userid, - ) - .await - { - tracing::error!("Failed to sync role addition to cluster nodes: {e}"); - } + let synced = best_effort_sync( + sync_users_with_roles_with_ingestors( + &req, + &userid, + &roles_to_add, + "add", + &tenant_id, + &caller_userid, + ) + .await, + "Failed to sync role addition to cluster nodes", + ); // update in mem table (this invalidates the user's session) Users.add_roles(&userid.clone(), roles_to_add.clone(), &tenant_id); + synced?; Ok(format!("Roles updated successfully for {username}")) } @@ -417,21 +419,22 @@ pub async fn remove_roles_from_user( put_metadata(&metadata, &tenant_id).await?; // sync to other nodes before updating in-memory (which invalidates sessions) - if let Err(e) = sync_users_with_roles_with_ingestors( - &req, - &userid, - &roles_to_remove, - "remove", - &tenant_id, - &caller_userid, - ) - .await - { - tracing::error!("Failed to sync role removal to cluster nodes: {e}"); - } + let synced = best_effort_sync( + sync_users_with_roles_with_ingestors( + &req, + &userid, + &roles_to_remove, + "remove", + &tenant_id, + &caller_userid, + ) + .await, + "Failed to sync role removal to cluster nodes", + ); // update in mem table (this invalidates the user's session) Users.remove_roles(&userid.clone(), roles_to_remove.clone(), &tenant_id); + synced?; Ok(HttpResponse::Ok().json(format!("Roles updated successfully for {username}"))) } @@ -472,13 +475,30 @@ pub async fn post_gen_password( put_metadata(&metadata, &tenant_id).await?; Users.change_password_hash(&userid, &new_hash, &tenant_id); - if let Err(e) = sync_password_reset_with_ingestors(req, &userid, &caller_userid).await { - tracing::error!("{e}"); - }; + best_effort_sync( + sync_password_reset_with_ingestors(req, &userid, &caller_userid).await, + "Failed to sync password reset to cluster nodes", + )?; Ok(new_password) } +/// Forwarding a change to the other nodes is best effort: a node or network +/// failure is logged and the local change still succeeds. A `HeaderBuild` +/// error means a node's forwarding headers could not be built; it retains its +/// status: 400 for a malformed client header, a generic 500 for a stored or +/// generated value. Callers apply the local in-memory change first. +fn best_effort_sync(result: Result<(), RBACError>, context: &str) -> Result<(), RBACError> { + match result { + Ok(()) => Ok(()), + Err(e @ RBACError::HeaderBuild(_)) => Err(e), + Err(e) => { + tracing::error!("{context}: {e}"); + Ok(()) + } + } +} + #[cfg(test)] mod role_escalation { // Removing a protected check lets the handler reach `get_metadata`, which @@ -812,3 +832,53 @@ mod role_escalation { rejects_addition(Role::create_user_role(vec![DefaultPrivilege::SuperAdmin])).await; } } + +#[cfg(test)] +mod sync_policy { + use actix_web::{ResponseError, body::to_bytes, http::StatusCode}; + + use super::*; + use crate::utils::header_parsing::{HeaderBuildError, ParseHeaderError}; + + // a request builder error, so no network is touched + fn network_error() -> reqwest::Error { + reqwest::Client::new().get("not a url").build().unwrap_err() + } + + #[actix_web::test] + async fn header_build_input_fails_the_request_with_400() { + let err = best_effort_sync( + Err(HeaderBuildError::Input(ParseHeaderError::InvalidTenantId).into()), + "sync", + ) + .unwrap_err(); + assert!(matches!( + err, + RBACError::HeaderBuild(HeaderBuildError::Input(ParseHeaderError::InvalidTenantId)) + )); + assert_eq!(err.status_code(), StatusCode::BAD_REQUEST); + } + + #[actix_web::test] + async fn header_build_internal_fails_the_request_with_generic_500() { + let err = best_effort_sync( + Err(HeaderBuildError::Internal("configured username").into()), + "sync", + ) + .unwrap_err(); + assert!(matches!( + err, + RBACError::HeaderBuild(HeaderBuildError::Internal("configured username")) + )); + assert_eq!(err.status_code(), StatusCode::INTERNAL_SERVER_ERROR); + let body = to_bytes(err.error_response().into_body()).await.unwrap(); + assert_eq!(body, "Failed to build internal request headers"); + } + + #[actix_web::test] + async fn node_and_network_failures_stay_best_effort() { + assert!(best_effort_sync(Ok(()), "sync").is_ok()); + assert!(best_effort_sync(Err(RBACError::Network(network_error())), "sync").is_ok()); + assert!(best_effort_sync(Err(anyhow::anyhow!("node info").into()), "sync").is_ok()); + } +} diff --git a/src/handlers/http/modal/query/querier_role.rs b/src/handlers/http/modal/query/querier_role.rs index 53c3c294b..8e675649e 100644 --- a/src/handlers/http/modal/query/querier_role.rs +++ b/src/handlers/http/modal/query/querier_role.rs @@ -116,9 +116,10 @@ pub async fn put( drop(guard); - if let Err(e) = sync_role_update(&req, name.clone(), role, &tenant_id, &caller_userid).await { - tracing::error!("Failed to sync role update to cluster nodes: {e}"); - } + best_effort_sync( + sync_role_update(&req, name.clone(), role, &tenant_id, &caller_userid).await, + "Failed to sync role update to cluster nodes", + )?; Ok(HttpResponse::Ok().finish()) } @@ -202,13 +203,30 @@ pub async fn delete( drop(guard); - if let Err(e) = sync_role_delete(&req, name.clone(), &tenant_id, &caller_userid).await { - tracing::error!("Failed to sync role deletion to cluster nodes: {e}"); - } + best_effort_sync( + sync_role_delete(&req, name.clone(), &tenant_id, &caller_userid).await, + "Failed to sync role deletion to cluster nodes", + )?; Ok(HttpResponse::Ok().finish()) } +/// Forwarding a change to the other nodes is best effort: a node or network +/// failure is logged and the local change still succeeds. A `HeaderBuild` +/// error means a node's forwarding headers could not be built; it retains its +/// status: 400 for a malformed client header, a generic 500 for a stored or +/// generated value. +fn best_effort_sync(result: Result<(), RoleError>, context: &str) -> Result<(), RoleError> { + match result { + Ok(()) => Ok(()), + Err(e @ RoleError::HeaderBuild(_)) => Err(e), + Err(e) => { + tracing::error!("{context}: {e}"); + Ok(()) + } + } +} + #[cfg(test)] mod tests { // Removing a protected-role check lets the handler reach storage and initialize @@ -383,3 +401,53 @@ mod tests { } } } + +#[cfg(test)] +mod sync_policy { + use actix_web::{ResponseError, body::to_bytes, http::StatusCode}; + + use super::*; + use crate::utils::header_parsing::{HeaderBuildError, ParseHeaderError}; + + // a request builder error, so no network is touched + fn network_error() -> reqwest::Error { + reqwest::Client::new().get("not a url").build().unwrap_err() + } + + #[actix_web::test] + async fn header_build_input_fails_the_request_with_400() { + let err = best_effort_sync( + Err(HeaderBuildError::Input(ParseHeaderError::InvalidTenantId).into()), + "sync", + ) + .unwrap_err(); + assert!(matches!( + err, + RoleError::HeaderBuild(HeaderBuildError::Input(ParseHeaderError::InvalidTenantId)) + )); + assert_eq!(err.status_code(), StatusCode::BAD_REQUEST); + } + + #[actix_web::test] + async fn header_build_internal_fails_the_request_with_generic_500() { + let err = best_effort_sync( + Err(HeaderBuildError::Internal("configured username").into()), + "sync", + ) + .unwrap_err(); + assert!(matches!( + err, + RoleError::HeaderBuild(HeaderBuildError::Internal("configured username")) + )); + assert_eq!(err.status_code(), StatusCode::INTERNAL_SERVER_ERROR); + let body = to_bytes(err.error_response().into_body()).await.unwrap(); + assert_eq!(body, "Failed to build internal request headers"); + } + + #[actix_web::test] + async fn node_and_network_failures_stay_best_effort() { + assert!(best_effort_sync(Ok(()), "sync").is_ok()); + assert!(best_effort_sync(Err(RoleError::Network(network_error())), "sync").is_ok()); + assert!(best_effort_sync(Err(anyhow::anyhow!("node info").into()), "sync").is_ok()); + } +} diff --git a/src/handlers/http/oidc.rs b/src/handlers/http/oidc.rs index b0721e0bf..316e6b027 100644 --- a/src/handlers/http/oidc.rs +++ b/src/handlers/http/oidc.rs @@ -50,7 +50,7 @@ use crate::{ API_BASE_PATH, API_VERSION, cluster::sync_user_creation, modal::{OIDC_CLIENT, utils::rbac_utils::put_metadata}, - rbac::UPDATE_LOCK, + rbac::{RBACError, UPDATE_LOCK}, }, }, oauth::{OAuthProvider, OAuthSession}, @@ -63,6 +63,7 @@ use crate::{ storage::{ObjectStorageError, StorageMetadata}, utils::{ actix::extract_session_key_from_req, get_tenant_id_from_key, get_tenant_id_from_request, + header_parsing::HeaderBuildError, }, }; @@ -240,17 +241,16 @@ pub async fn reply_login( if let UserType::OAuth(oauth) = &mut cluster_user.ty { oauth.bearer = None; } - if let Err(e) = sync_user_creation( - &req, - cluster_user, - &roles, - &tenant_id, - &PARSEABLE.options.username, - ) - .await - { - tracing::error!("Failed to sync OAuth user with roles to cluster nodes: {e}"); - } + best_effort_user_sync( + sync_user_creation( + &req, + cluster_user, + &roles, + &tenant_id, + &PARSEABLE.options.username, + ) + .await, + )?; } let cookies = [ @@ -264,6 +264,21 @@ pub async fn reply_login( )) } +/// Syncing the OAuth user to other nodes is best effort: a node or network +/// failure is logged and login continues. Forwarding headers that can't be +/// built on any node fail the login rather than being ignored, retaining +/// its own status (400 client header, generic 500 stored or generated value). +fn best_effort_user_sync(result: Result<(), RBACError>) -> Result<(), OIDCError> { + match result { + Ok(()) => Ok(()), + Err(RBACError::HeaderBuild(e)) => Err(e.into()), + Err(e) => { + tracing::error!("Failed to sync OAuth user with roles to cluster nodes: {e}"); + Ok(()) + } + } +} + /// Extract username, user_id, and UserInfo from the OAuth session. fn extract_identity(session: &OAuthSession) -> Result<(String, String, user::UserInfo), OIDCError> { let user_info = &session.userinfo; @@ -838,6 +853,8 @@ pub enum OIDCError { BadRequest(String), #[error("Unauthorized")] Unauthorized, + #[error("{0}")] + HeaderBuild(#[from] HeaderBuildError), } impl actix_web::ResponseError for OIDCError { @@ -847,6 +864,7 @@ impl actix_web::ResponseError for OIDCError { Self::Serde(_) => StatusCode::INTERNAL_SERVER_ERROR, Self::BadRequest(_) => StatusCode::BAD_REQUEST, Self::Unauthorized => StatusCode::UNAUTHORIZED, + Self::HeaderBuild(e) => actix_web::ResponseError::status_code(e), } } @@ -1089,4 +1107,50 @@ mod tests { ); } } + + // a request builder error, so no network is touched + fn network_error() -> reqwest::Error { + reqwest::Client::new().get("not a url").build().unwrap_err() + } + + #[actix_web::test] + async fn user_sync_header_build_input_fails_login_with_400() { + let err = best_effort_user_sync(Err(HeaderBuildError::Input( + crate::utils::header_parsing::ParseHeaderError::InvalidTenantId, + ) + .into())) + .unwrap_err(); + assert!(matches!( + err, + OIDCError::HeaderBuild(HeaderBuildError::Input(_)) + )); + assert_eq!( + actix_web::ResponseError::status_code(&err), + StatusCode::BAD_REQUEST + ); + } + + #[actix_web::test] + async fn user_sync_header_build_internal_fails_login_with_generic_500() { + let err = + best_effort_user_sync(Err(HeaderBuildError::Internal("configured username").into())) + .unwrap_err(); + assert!(matches!( + err, + OIDCError::HeaderBuild(HeaderBuildError::Internal("configured username")) + )); + let response = actix_web::ResponseError::error_response(&err); + assert_eq!(response.status(), StatusCode::INTERNAL_SERVER_ERROR); + let body = actix_web::body::to_bytes(response.into_body()) + .await + .unwrap(); + assert_eq!(body, "Failed to build internal request headers"); + } + + #[actix_web::test] + async fn user_sync_node_and_network_failures_stay_best_effort() { + assert!(best_effort_user_sync(Ok(())).is_ok()); + assert!(best_effort_user_sync(Err(RBACError::Network(network_error()))).is_ok()); + assert!(best_effort_user_sync(Err(anyhow::anyhow!("node info").into())).is_ok()); + } } diff --git a/src/handlers/http/otel_generator.rs b/src/handlers/http/otel_generator.rs index 8a23c5abb..8849f8de4 100644 --- a/src/handlers/http/otel_generator.rs +++ b/src/handlers/http/otel_generator.rs @@ -7,7 +7,7 @@ * License, or (at your option) any later version. */ -use actix_web::{HttpRequest, HttpResponse, web}; +use actix_web::{HttpRequest, HttpResponse, ResponseError, http::StatusCode, web}; use base64::{Engine, prelude::BASE64_STANDARD}; use reqwest::Method; use serde::{Deserialize, Serialize}; @@ -19,7 +19,7 @@ use crate::{ option::Mode, otel_generator::OTEL_GENERATOR, parseable::{DEFAULT_TENANT, PARSEABLE}, - utils::{get_tenant_id_from_request, get_user_from_request}, + utils::{get_tenant_id_from_request, get_user_from_request, header_parsing::ParseHeaderError}, }; use super::{ @@ -47,6 +47,24 @@ struct OtelGeneratorErrorResponse { error: String, } +/// A malformed request header on a generator request, answered in the same +/// envelope as the endpoint's other errors. The message never includes the +/// value. +#[derive(Debug, thiserror::Error)] +#[error(transparent)] +pub(crate) struct OtelGeneratorHeaderError(pub(crate) ParseHeaderError); + +impl ResponseError for OtelGeneratorHeaderError { + fn status_code(&self) -> StatusCode { + StatusCode::BAD_REQUEST + } + fn error_response(&self) -> HttpResponse { + HttpResponse::build(self.status_code()).json(OtelGeneratorErrorResponse { + error: self.0.to_string(), + }) + } +} + pub async fn start_otel_generator( req: HttpRequest, body: Option>, @@ -136,11 +154,8 @@ pub async fn get_otel_generator_status(req: HttpRequest) -> HttpResponse { } fn request_tenant(req: &HttpRequest) -> Result, HttpResponse> { - let tenant_id = get_tenant_id_from_request(req).map_err(|error| { - HttpResponse::BadRequest().json(OtelGeneratorErrorResponse { - error: error.to_string(), - }) - })?; + let tenant_id = get_tenant_id_from_request(req) + .map_err(|error| OtelGeneratorHeaderError(error).error_response())?; if PARSEABLE.options.is_multi_tenant() && tenant_id.is_none() { return Err(HttpResponse::BadRequest().json(OtelGeneratorErrorResponse { error: "X-P-Tenant header is required when multi-tenancy is enabled".to_string(),