From 67863630b2367fef30b9b3bc9acf263f958fb740 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E5=AE=89=E6=AD=A3=E8=B6=85?= Date: Sat, 4 Apr 2026 08:36:14 +0800 Subject: [PATCH] fix(auth): reject ambiguous case-insensitive claim matches (#2386) --- crates/iam/src/oidc.rs | 42 ++++++++++------- crates/policy/src/policy.rs | 1 + crates/policy/src/policy/policy.rs | 73 ++++++++++++++++++------------ crates/policy/src/policy/utils.rs | 71 ++++++++++++++++++++++++++++- 4 files changed, 142 insertions(+), 45 deletions(-) diff --git a/crates/iam/src/oidc.rs b/crates/iam/src/oidc.rs index cac4c7bb6..923e99c99 100644 --- a/crates/iam/src/oidc.rs +++ b/crates/iam/src/oidc.rs @@ -27,6 +27,7 @@ use openidconnect::{ use rustfs_config::oidc::*; use rustfs_config::{DEFAULT_DELIMITER, ENABLE_KEY, EnableState}; use rustfs_ecstore::config::{Config as ServerConfig, KVS, get_global_server_config}; +use rustfs_policy::policy::{ClaimLookup, get_claim_case_insensitive}; use serde::{Deserialize, Serialize}; use std::borrow::Cow; use std::collections::HashMap; @@ -1007,29 +1008,19 @@ pub(crate) fn decode_jwt_payload(token: &str) -> HashMap(claims: &'a HashMap, key: &str) -> Option<&'a serde_json::Value> { - if let Some(v) = claims.get(key) { - return Some(v); - } - let key_lower = key.to_lowercase(); - claims.iter().find(|(k, _)| k.to_lowercase() == key_lower).map(|(_, v)| v) -} - /// Extract a string claim from raw claims with case-insensitive fallback. fn extract_string_claim(claims: &HashMap, key: &str) -> String { - get_claim_case_insensitive(claims, key) - .and_then(serde_json::Value::as_str) - .unwrap_or_default() - .to_string() + match get_claim_case_insensitive(claims, key) { + ClaimLookup::Found(value) => value.as_str().unwrap_or_default().to_string(), + ClaimLookup::Missing | ClaimLookup::Ambiguous => String::new(), + } } /// Extract a groups/array claim from raw claims with case-insensitive fallback. Handles both string arrays and single strings. fn extract_groups_claim(claims: &HashMap, key: &str) -> Vec { match get_claim_case_insensitive(claims, key) { - Some(serde_json::Value::Array(arr)) => arr.iter().filter_map(|v| v.as_str().map(String::from)).collect(), - Some(serde_json::Value::String(s)) => s.split(',').map(|s| s.trim().to_string()).collect(), + ClaimLookup::Found(serde_json::Value::Array(arr)) => arr.iter().filter_map(|v| v.as_str().map(String::from)).collect(), + ClaimLookup::Found(serde_json::Value::String(s)) => s.split(',').map(|s| s.trim().to_string()).collect(), _ => vec![], } } @@ -1117,6 +1108,25 @@ mod tests { assert_eq!(groups, vec!["exact_match"]); } + #[test] + fn test_extract_string_claim_ambiguous_case_insensitive_match_returns_empty() { + let mut claims = HashMap::new(); + claims.insert("Policy".to_string(), serde_json::json!("exact_match")); + claims.insert("policy".to_string(), serde_json::json!("lowercase")); + + assert_eq!(extract_string_claim(&claims, "POLICY"), ""); + } + + #[test] + fn test_extract_groups_claim_ambiguous_case_insensitive_match_returns_empty() { + let mut claims = HashMap::new(); + claims.insert("Policy".to_string(), serde_json::json!(["exact_match"])); + claims.insert("policy".to_string(), serde_json::json!(["lowercase"])); + + let groups = extract_groups_claim(&claims, "POLICY"); + assert!(groups.is_empty()); + } + #[test] fn test_decode_jwt_payload() { let payload = r#"{"sub":"user123","email":"user@example.com"}"#; diff --git a/crates/policy/src/policy.rs b/crates/policy/src/policy.rs index 92329983f..756f44a2e 100644 --- a/crates/policy/src/policy.rs +++ b/crates/policy/src/policy.rs @@ -35,6 +35,7 @@ pub use policy::*; pub use principal::Principal; pub use resource::ResourceSet; pub use statement::Statement; +pub use utils::{ClaimLookup, get_claim_case_insensitive}; #[derive(thiserror::Error, Debug)] #[cfg_attr(test, derive(Eq, PartialEq))] diff --git a/crates/policy/src/policy/policy.rs b/crates/policy/src/policy/policy.rs index 88201e66b..1176b862b 100644 --- a/crates/policy/src/policy/policy.rs +++ b/crates/policy/src/policy/policy.rs @@ -13,8 +13,8 @@ // limitations under the License. use super::{ - Effect, Error as IamError, Functions, ID, Statement, action::Action, statement::BPStatement, - statement::variable_resolver_for_policy_args, + ClaimLookup, Effect, Error as IamError, Functions, ID, Statement, action::Action, get_claim_case_insensitive, + statement::BPStatement, statement::variable_resolver_for_policy_args, }; use crate::error::{Error, Result}; use serde::{Deserialize, Serialize}; @@ -239,42 +239,37 @@ impl Validator for BucketPolicy { } } -fn get_claim_case_insensitive<'a>(claims: &'a HashMap, claim_name: &str) -> Option<&'a Value> { - if let Some(v) = claims.get(claim_name) { - return Some(v); - } - let claim_name_lower = claim_name.to_lowercase(); - claims - .iter() - .find(|(k, _)| k.to_lowercase() == claim_name_lower) - .map(|(_, v)| v) -} - fn get_values_from_claims(claims: &HashMap, claim_name: &str) -> (HashSet, bool) { let mut s = HashSet::new(); - if let Some(pname) = get_claim_case_insensitive(claims, claim_name) { - if let Some(pnames) = pname.as_array() { - for pname in pnames { - if let Some(pname_str) = pname.as_str() { - for pname in pname_str.split(',') { - let pname = pname.trim(); - if !pname.is_empty() { - s.insert(pname.to_string()); + match get_claim_case_insensitive(claims, claim_name) { + ClaimLookup::Found(pname) => { + if let Some(pnames) = pname.as_array() { + for pname in pnames { + if let Some(pname_str) = pname.as_str() { + for pname in pname_str.split(',') { + let pname = pname.trim(); + if !pname.is_empty() { + s.insert(pname.to_string()); + } } } } + return (s, true); } - return (s, true); - } else if let Some(pname_str) = pname.as_str() { - for pname in pname_str.split(',') { - let pname = pname.trim(); - if !pname.is_empty() { - s.insert(pname.to_string()); + + if let Some(pname_str) = pname.as_str() { + for pname in pname_str.split(',') { + let pname = pname.trim(); + if !pname.is_empty() { + s.insert(pname.to_string()); + } } + return (s, true); } - return (s, true); } + ClaimLookup::Missing | ClaimLookup::Ambiguous => {} } + (s, false) } @@ -1744,4 +1739,26 @@ mod test { assert!(policies.contains("consoleAdmin")); assert!(policies.contains("readwrite")); } + + #[test] + fn test_get_values_from_claims_ambiguous_case_insensitive_match_returns_missing() { + let mut claims = HashMap::new(); + claims.insert("Policy".to_string(), Value::Array(vec![Value::String("exact_match".to_string())])); + claims.insert("policy".to_string(), Value::Array(vec![Value::String("lowercase".to_string())])); + + let (policies, found) = get_values_from_claims(&claims, "POLICY"); + assert!(!found); + assert!(policies.is_empty()); + } + + #[test] + fn test_get_policies_from_claims_ambiguous_case_insensitive_match_returns_missing() { + let mut claims = HashMap::new(); + claims.insert("Policy".to_string(), Value::String("consoleAdmin".to_string())); + claims.insert("policy".to_string(), Value::String("readwrite".to_string())); + + let (policies, found) = get_policies_from_claims(&claims, "POLICY"); + assert!(!found); + assert!(policies.is_empty()); + } } diff --git a/crates/policy/src/policy/utils.rs b/crates/policy/src/policy/utils.rs index 832f6f5a6..32e95c858 100644 --- a/crates/policy/src/policy/utils.rs +++ b/crates/policy/src/policy/utils.rs @@ -19,6 +19,41 @@ use serde_json::Value; pub mod path; pub mod wildcard; +#[derive(Clone, Copy, Debug, Eq, PartialEq)] +pub enum ClaimLookup<'a> { + Missing, + Found(&'a Value), + Ambiguous, +} + +fn case_insensitive_eq(left: &str, right: &str) -> bool { + left.chars() + .flat_map(char::to_lowercase) + .eq(right.chars().flat_map(char::to_lowercase)) +} + +pub fn get_claim_case_insensitive<'a>(claims: &'a HashMap, claim_name: &str) -> ClaimLookup<'a> { + if let Some(value) = claims.get(claim_name) { + return ClaimLookup::Found(value); + } + + let mut matched = None; + + for (candidate, value) in claims { + if case_insensitive_eq(candidate, claim_name) { + if matched.is_some() { + return ClaimLookup::Ambiguous; + } + matched = Some(value); + } + } + + match matched { + Some(value) => ClaimLookup::Found(value), + None => ClaimLookup::Missing, + } +} + pub fn _get_values_from_claims(claim: &HashMap, chaim_name: &str) -> (Vec, bool) { let mut result = vec![]; let Some(pname) = claim.get(chaim_name) else { @@ -77,7 +112,9 @@ pub fn _split_path(path: &str, second_index: bool) -> (&str, &str) { #[cfg(test)] mod tests { - use super::_split_path; + use super::{_split_path, ClaimLookup, get_claim_case_insensitive}; + use serde_json::{Value, json}; + use std::collections::HashMap; #[test_case::test_case("format.json", false => ("format.json", ""))] #[test_case::test_case("users/tester.json", false => ("users/", "tester.json"))] @@ -98,4 +135,36 @@ mod tests { fn test_split_path(path: &str, second_index: bool) -> (&str, &str) { _split_path(path, second_index) } + + #[test] + fn test_get_claim_case_insensitive_prefers_exact_match() { + let mut claims = HashMap::new(); + claims.insert("Policy".to_string(), json!("exact_match")); + claims.insert("policy".to_string(), json!("lowercase")); + + assert_eq!( + get_claim_case_insensitive(&claims, "Policy"), + ClaimLookup::Found(&Value::String("exact_match".to_string())) + ); + } + + #[test] + fn test_get_claim_case_insensitive_returns_ambiguous_for_multiple_folded_matches() { + let mut claims = HashMap::new(); + claims.insert("Policy".to_string(), json!("exact_match")); + claims.insert("policy".to_string(), json!("lowercase")); + + assert_eq!(get_claim_case_insensitive(&claims, "POLICY"), ClaimLookup::Ambiguous); + } + + #[test] + fn test_get_claim_case_insensitive_matches_unicode_without_allocation() { + let mut claims = HashMap::new(); + claims.insert("Straße".to_string(), json!("value")); + + assert_eq!( + get_claim_case_insensitive(&claims, "straße"), + ClaimLookup::Found(&Value::String("value".to_string())) + ); + } }