fix(policy): substitute jwt and ldap variables in policies (#8927)

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")]

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, Vec<String>>) -> 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<StringFuncValue> {
     async fn eval(
         &self,
@@ -110,17 +124,7 @@ impl FuncKeyValue<StringFuncValue> {
         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::<Set<_>>();
 
@@ -159,17 +163,7 @@ impl FuncKeyValue<StringFuncValue> {
                 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")]

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;