diff --git a/crates/policy/src/policy/function/key_name.rs b/crates/policy/src/policy/function/key_name.rs index 807653e02..777ba1993 100644 --- a/crates/policy/src/policy/function/key_name.rs +++ b/crates/policy/src/policy/function/key_name.rs @@ -139,15 +139,9 @@ impl KeyName { Self::COMMON_KEYS.iter().filter(|k| k.is_server_derived()).map(|k| k.name()) } + /// Returns the policy variable form of the key, e.g. `${jwt:sub}`. pub fn var_name(&self) -> String { - match self { - KeyName::Aws(s) => format!("${{aws:{}}}", Into::<&str>::into(s)), - KeyName::Jwt(s) => format!("${{jwt:{}}}", Into::<&str>::into(s)), - KeyName::Ldap(s) => format!("${{ldap:{}}}", Into::<&str>::into(s)), - KeyName::Sts(s) => format!("${{sts:{}}}", Into::<&str>::into(s)), - KeyName::Svc(s) => format!("${{svc:{}}}", Into::<&str>::into(s)), - KeyName::S3(s) => format!("${{s3:{}}}", Into::<&str>::into(s)), - } + format!("${{{}}}", Into::<&str>::into(self)) } } @@ -414,6 +408,14 @@ mod tests { ); } + #[test_case(KeyName::Aws(AwsKeyName::AWSSourceIP), "${aws:SourceIp}")] + #[test_case(KeyName::Jwt(JwtKeyName::JWTPrefUsername), "${jwt:preferred_username}")] + #[test_case(KeyName::Ldap(LdapKeyName::Username), "${ldap:username}")] + #[test_case(KeyName::S3(S3KeyName::S3LocationConstraint), "${s3:LocationConstraint}")] + fn var_name_matches_policy_variable_syntax(key: KeyName, expected: &str) { + assert_eq!(key.var_name(), expected); + } + #[test_case("s3:x-amz-copy-source", KeyName::S3(S3KeyName::S3XAmzCopySource))] #[test_case("s3:VersionId", KeyName::S3(S3KeyName::S3VersionId) ; "aws_version_id")] #[test_case("s3:versionid", KeyName::S3(S3KeyName::S3VersionId) ; "minio_version_id")] diff --git a/crates/policy/src/policy/function/string.rs b/crates/policy/src/policy/function/string.rs index 983f79d13..2c9a03948 100644 --- a/crates/policy/src/policy/function/string.rs +++ b/crates/policy/src/policy/function/string.rs @@ -73,6 +73,20 @@ impl StringFunc { } } +/// Replaces every `${...}` reference to a common condition key that has a +/// non-empty value in the request, not only the first such key. +fn substitute_common_keys(mut c: String, values: &HashMap>) -> String { + for key in KeyName::COMMON_KEYS { + if let Some(v) = values.get(key.name()).and_then(|x| x.first()) + && !v.is_empty() + { + c = c.replace(&key.var_name(), v); + } + } + + c +} + impl FuncKeyValue { async fn eval( &self, @@ -110,17 +124,7 @@ impl FuncKeyValue { let fvalues = resolved_values .into_iter() .flatten() - .map(|resolved_c| { - let mut c = Cow::from(resolved_c); - for key in KeyName::COMMON_KEYS { - match values.get(key.name()).and_then(|x| x.first()) { - Some(v) if !v.is_empty() => return Cow::Owned(c.to_mut().replace(&key.var_name(), v)), - _ => continue, - }; - } - - c - }) + .map(|resolved_c| Cow::from(substitute_common_keys(resolved_c, values))) .map(|x| if ignore_case { Cow::Owned(x.to_lowercase()) } else { x }) .collect::>(); @@ -159,17 +163,7 @@ impl FuncKeyValue { let matched = resolved_values .into_iter() .flatten() - .map(|resolved_c| { - let mut c = Cow::from(resolved_c); - for key in KeyName::COMMON_KEYS { - match values.get(key.name()).and_then(|x| x.first()) { - Some(v) if !v.is_empty() => return Cow::Owned(c.to_mut().replace(&key.var_name(), v)), - _ => continue, - }; - } - - c - }) + .map(|resolved_c| substitute_common_keys(resolved_c, values)) .any(|x| wildcard::is_match(x, v)); // Unqualified evaluation reports the plain aggregate match and lets the @@ -389,6 +383,44 @@ mod tests { )); } + // Real requests always carry earlier common keys such as `signatureversion` + // and `SourceIp`; substitution must still reach later keys like `jwt:*`. + #[test] + fn test_condition_values_substitute_variables_after_other_common_keys() { + let request = || { + vec![ + ("signatureversion", vec!["AWS4-HMAC-SHA256"]), + ("SourceIp", vec!["192.0.2.10"]), + ("preferred_username", vec!["alice"]), + ("prefix", vec!["alice/photos/"]), + ] + }; + + assert!(test_eval_like( + new_fkv("s3:prefix", vec!["${jwt:preferred_username}/*"]), + false, + false, + request() + )); + assert!(test_eval( + new_fkv("s3:prefix", vec!["${jwt:preferred_username}/photos/"]), + false, + false, + false, + request() + )); + assert!(!test_eval_like( + new_fkv("s3:prefix", vec!["${jwt:preferred_username}/*"]), + false, + false, + vec![ + ("signatureversion", vec!["AWS4-HMAC-SHA256"]), + ("preferred_username", vec!["bob"]), + ("prefix", vec!["alice/photos/"]), + ] + )); + } + #[test_case(new_fkv("s3:x-amz-copy-source", vec!["mybucket/myobject"]), false, vec![("x-amz-copy-source", vec!["mybucket/myobject"])] => true ; "1")] #[test_case(new_fkv("s3:x-amz-copy-source", vec!["mybucket/myobject"]), false, vec![("x-amz-copy-source", vec!["yourbucket/myobject"])] => false ; "2")] #[test_case(new_fkv("s3:x-amz-copy-source", vec!["mybucket/myobject"]), false, vec![] => false ; "3")] diff --git a/crates/policy/src/policy/resource.rs b/crates/policy/src/policy/resource.rs index 6d203accc..d60603fc6 100644 --- a/crates/policy/src/policy/resource.rs +++ b/crates/policy/src/policy/resource.rs @@ -352,6 +352,19 @@ mod tests { pollster::block_on(resource.is_match(object, &HashMap::new())) } + #[test_case("arn:aws:s3:::mybucket/${jwt:preferred_username}/*", "mybucket/alice/photo.jpg" => true; "jwt variable")] + #[test_case("arn:aws:s3:::mybucket/${jwt:preferred_username}/*", "mybucket/bob/photo.jpg" => false; "jwt variable other user")] + #[test_case("arn:aws:s3:::mybucket/${ldap:username}/*", "mybucket/alice-ldap/photo.jpg" => true; "ldap variable")] + fn test_resource_is_match_substitutes_condition_variables(resource: &str, object: &str) -> bool { + let resource: Resource = resource.try_into().unwrap(); + let conditions = HashMap::from([ + ("preferred_username".to_string(), vec!["alice".to_string()]), + ("username".to_string(), vec!["alice-ldap".to_string()]), + ]); + + pollster::block_on(resource.is_match(object, &conditions)) + } + #[test] fn test_wildcard_resource_serialization_round_trip() { use crate::policy::resource::ResourceSet;