diff --git a/crates/auth/src/policy/document.rs b/crates/auth/src/policy/document.rs index dedcd5e2a..c227995a3 100755 --- a/crates/auth/src/policy/document.rs +++ b/crates/auth/src/policy/document.rs @@ -390,12 +390,12 @@ pub fn parse_conditions(value: &Value) -> Result, PolicyParseErro }; let mut conditions = Vec::new(); - for (op_name, keys_obj) in obj { + for (op_name, keys_obj) in by_name(obj) { let operator = parse_operator(op_name)?; let keys = keys_obj .as_object() .ok_or_else(|| PolicyParseError(format!("condition {op_name} must be an object")))?; - for (key, vals) in keys { + for (key, vals) in by_name(keys) { let values = parse_string_or_array(vals)?; conditions.push(Condition { operator: operator.clone(), @@ -407,6 +407,19 @@ pub fn parse_conditions(value: &Value) -> Result, PolicyParseErro Ok(conditions) } +/// The entries of `map` in name order. Any build that compiles the MongoDB +/// backend crate, which includes a build of the whole workspace, gets +/// serde_json's `preserve_order` feature through `bson`, so there a map keeps +/// the order it was written in; a build without that crate sorts it by name. +/// A parse or a check that stops at the first fault reports the same fault on +/// every build when it reads the entries in name order. +#[must_use] +pub fn by_name(map: &serde_json::Map) -> Vec<(&String, &Value)> { + let mut entries: Vec<_> = map.iter().collect(); + entries.sort_unstable_by(|a, b| a.0.cmp(b.0)); + entries +} + /// Parse a condition operator name, handling `ForAllValues:`, `ForAnyValue:` prefixes /// and `IfExists` suffix. fn parse_operator(name: &str) -> Result { @@ -512,6 +525,45 @@ mod tests { )); } + /// The fault reported for a Condition with two unknown operators is the + /// one whose name sorts first, in either order of the document (#780). + /// `cargo test --workspace` builds this crate with `preserve_order`, + /// which keeps the written order, so there the test tells the two apart. + #[test] + fn two_unknown_operators_report_the_first_in_name_order() { + for (first, second) in [("BadA", "BadB"), ("BadB", "BadA")] { + let json = format!( + r#"{{"Version":"2012-10-17","Statement":[{{"Effect":"Allow", + "Action":"dynamodb:GetItem","Resource":"*","Condition":{{ + "{first}":{{"aws:k":"v"}},"{second}":{{"aws:k":"v"}}}}}}]}}"# + ); + let err = PolicyDocument::from_json(&json).unwrap_err(); + assert_eq!(err.0, "unknown operator: BadA", "{first} before {second}"); + } + } + + /// Conditions come out in name order of operator, then key, in either + /// order of the document. + #[test] + fn conditions_are_read_in_name_order() { + for condition in [ + r#"{"StringLike":{"aws:b":"1","aws:a":"2"},"Bool":{"aws:c":"true"}}"#, + r#"{"Bool":{"aws:c":"true"},"StringLike":{"aws:a":"2","aws:b":"1"}}"#, + ] { + let json = format!( + r#"{{"Version":"2012-10-17","Statement":[{{"Effect":"Allow", + "Action":"dynamodb:GetItem","Resource":"*","Condition":{condition}}}]}}"# + ); + let doc = PolicyDocument::from_json(&json).unwrap(); + let keys: Vec<&str> = doc.statements[0] + .conditions + .iter() + .map(|c| c.key.as_str()) + .collect(); + assert_eq!(keys, ["aws:c", "aws:a", "aws:b"], "{condition}"); + } + } + #[test] fn parse_deny_with_not_action() { let json = r#"{ diff --git a/crates/server/src/management/assume_role.rs b/crates/server/src/management/assume_role.rs index 0be4b5295..edeeda75d 100755 --- a/crates/server/src/management/assume_role.rs +++ b/crates/server/src/management/assume_role.rs @@ -22,7 +22,7 @@ use super::crypto::{ }; use extenddb_auth::policy::condition::{ConditionMatch, evaluate_statement_conditions}; use extenddb_auth::policy::context::AssumeRoleContext; -use extenddb_auth::policy::document::parse_conditions; +use extenddb_auth::policy::document::{by_name, parse_conditions}; #[derive(Deserialize)] pub struct AssumeRoleRequest { @@ -624,7 +624,9 @@ fn session_tag_pairs(session_tags: &Value) -> Vec<(String, String)> { let mut pairs = Vec::new(); match session_tags { Value::Object(obj) => { - for (k, v) in obj { + // In name order, so the first bad key and the later of two keys + // that collide by case are the same on every build. + for (k, v) in by_name(obj) { if let Some(s) = scalar_to_string(v) { pairs.push((k.clone(), s)); } @@ -816,7 +818,9 @@ fn validate_trust_condition(cond: &Value) -> Result<(), String> { let obj = cond .as_object() .ok_or_else(|| "Condition must be an object".to_owned())?; - for (op_name, keys) in obj { + // In name order, so a block with two faults reports the same one on every + // build. + for (op_name, keys) in by_name(obj) { if !is_known_iam_condition_operator(op_name) { return Err(format!("invalid Condition operator: {op_name}")); } @@ -1664,6 +1668,45 @@ mod tests { } } + /// A trust policy `Condition` with two unknown operators reports the one + /// whose name sorts first, in either order of the document (#780). + /// `cargo test --workspace` builds `serde_json` with `preserve_order`, + /// which keeps the written order, so there the test tells the two apart. + #[test] + fn validate_reports_the_first_bad_operator_in_name_order() { + for (first, second) in [("BadA", "BadB"), ("BadB", "BadA")] { + let doc: Value = serde_json::from_str(&format!( + r#"{{"Version":"2012-10-17","Statement":[{{"Effect":"Allow", + "Principal":{{"AWS":"{CALLER}"}},"Action":"sts:AssumeRole", + "Condition":{{"{first}":{{"aws:k":"v"}},"{second}":{{"aws:k":"v"}}}}}}]}}"# + )) + .unwrap(); + assert_eq!( + validate_trust_policy(&doc).unwrap_err(), + "invalid Condition operator: BadA", + "{first} before {second}" + ); + } + } + + /// Object-form session tags are checked in name order: of two keys that + /// collide by case, the later in name order is the one reported, in either + /// order of the request. + #[test] + fn session_tag_pairs_are_in_name_order() { + for tags in [r#"{"team":"a","Team":"b"}"#, r#"{"Team":"b","team":"a"}"#] { + let tags: Value = serde_json::from_str(tags).unwrap(); + let pairs = session_tag_pairs(&tags); + let keys: Vec<&str> = pairs.iter().map(|(k, _)| k.as_str()).collect(); + assert_eq!(keys, ["Team", "team"], "{tags}"); + assert_eq!( + super::super::duplicate_tag_key_ci(&pairs).as_deref(), + Some("team"), + "{tags}" + ); + } + } + #[test] fn validate_accepts_alphanumeric_or_empty_sid() { for sid in ["Abc123", "", "aaaaaaaaaa"] {