feat: Add permission verification for account creation (#1401)

Co-authored-by: loverustfs <hello@rustfs.com>

crates/policy/src/policy/policy.rs

@@ -63,16 +63,23 @@ pub struct Policy {
 
 impl Policy {
     pub async fn is_allowed(&self, args: &Args<'_>) -> bool {
+        // First, check all Deny statements - if any Deny matches, deny the request
         for statement in self.statements.iter().filter(|s| matches!(s.effect, Effect::Deny)) {
             if !statement.is_allowed(args).await {
                 return false;
             }
         }
 
-        if args.deny_only || args.is_owner {
+        // Owner has all permissions
+        if args.is_owner {
             return true;
         }
 
+        if args.deny_only {
+            return false;
+        }
+
+        // Check Allow statements
         for statement in self.statements.iter().filter(|s| matches!(s.effect, Effect::Allow)) {
             if statement.is_allowed(args).await {
                 return true;
@@ -594,6 +601,102 @@ mod test {
         Ok(())
     }
 
+    #[tokio::test]
+    async fn test_deny_only_security_fix() -> Result<()> {
+        let data = r#"
+{
+  "Version": "2012-10-17",
+  "Statement": [
+    {
+      "Effect": "Allow",
+      "Action": ["s3:GetObject"],
+      "Resource": ["arn:aws:s3:::bucket1/*"]
+    }
+  ]
+}
+"#;
+
+        let policy = Policy::parse_config(data.as_bytes())?;
+        let conditions = HashMap::new();
+        let claims = HashMap::new();
+
+        // Test with deny_only=true but no matching Allow statement
+        let args_deny_only = Args {
+            account: "testuser",
+            groups: &None,
+            action: Action::S3Action(crate::policy::action::S3Action::PutObjectAction),
+            bucket: "bucket2",
+            conditions: &conditions,
+            is_owner: false,
+            object: "test.txt",
+            claims: &claims,
+            deny_only: true, // Should NOT automatically allow
+        };
+
+        // Should return false because deny_only=true, regardless of whether there's a matching Allow statement
+        assert!(
+            !policy.is_allowed(&args_deny_only).await,
+            "deny_only should return false when deny_only=true, regardless of Allow statements"
+        );
+
+        // Test with deny_only=true and matching Allow statement
+        let args_deny_only_allowed = Args {
+            account: "testuser",
+            groups: &None,
+            action: Action::S3Action(crate::policy::action::S3Action::GetObjectAction),
+            bucket: "bucket1",
+            conditions: &conditions,
+            is_owner: false,
+            object: "test.txt",
+            claims: &claims,
+            deny_only: true,
+        };
+
+        // Should return false because deny_only=true prevents checking Allow statements (unless is_owner=true)
+        assert!(
+            !policy.is_allowed(&args_deny_only_allowed).await,
+            "deny_only should return false even with matching Allow statement"
+        );
+
+        // Test with deny_only=false (normal case)
+        let args_normal = Args {
+            account: "testuser",
+            groups: &None,
+            action: Action::S3Action(crate::policy::action::S3Action::GetObjectAction),
+            bucket: "bucket1",
+            conditions: &conditions,
+            is_owner: false,
+            object: "test.txt",
+            claims: &claims,
+            deny_only: false,
+        };
+
+        // Should return true because there's an Allow statement
+        assert!(
+            policy.is_allowed(&args_normal).await,
+            "normal policy evaluation should allow with matching Allow statement"
+        );
+
+        let args_owner_deny_only = Args {
+            account: "testuser",
+            groups: &None,
+            action: Action::S3Action(crate::policy::action::S3Action::PutObjectAction),
+            bucket: "bucket2",
+            conditions: &conditions,
+            is_owner: true, // Owner has all permissions
+            object: "test.txt",
+            claims: &claims,
+            deny_only: true, // Even with deny_only=true, owner should be allowed
+        };
+
+        assert!(
+            policy.is_allowed(&args_owner_deny_only).await,
+            "owner should retain all permissions even when deny_only=true"
+        );
+
+        Ok(())
+    }
+
     #[tokio::test]
     async fn test_aws_username_policy_variable() -> Result<()> {
         let data = r#"

rustfs/src/admin/handlers/service_account.rs

@@ -112,8 +112,6 @@ impl Operation for AddServiceAccount {
             return Err(s3_error!(InvalidRequest, "iam not init"));
         };
 
-        let deny_only = constant_time_eq(&cred.access_key, &target_user) || constant_time_eq(&cred.parent_user, &target_user);
-
         if !iam_store
             .is_allowed(&Args {
                 account: &cred.access_key,
@@ -130,7 +128,7 @@ impl Operation for AddServiceAccount {
                 is_owner: owner,
                 object: "",
                 claims: cred.claims.as_ref().unwrap_or(&HashMap::new()),
-                deny_only,
+                deny_only: false, // Always require explicit Allow permission
             })
             .await
         {

rustfs/src/admin/handlers/user.rs

@@ -118,12 +118,14 @@ impl Operation for AddUser {
             return Err(s3_error!(InvalidArgument, "access key is not utf8"));
         }
 
-        let deny_only = ak == cred.access_key;
+        // Security fix: Always require explicit Allow permission for CreateUser
+        // Do not use deny_only to bypass permission checks, even when creating for self
+        // This ensures consistent security semantics and prevents privilege escalation
         validate_admin_request(
             &req.headers,
             &cred,
             owner,
-            deny_only,
+            false, // Always require explicit Allow permission
             vec![Action::AdminAction(AdminAction::CreateUserAdminAction)],
             req.extensions.get::<Option<RemoteAddr>>().and_then(|opt| opt.map(|a| a.0)),
         )
@@ -375,12 +377,14 @@ impl Operation for GetUserInfo {
         let (cred, owner) =
             check_key_valid(get_session_token(&req.uri, &req.headers).unwrap_or_default(), &input_cred.access_key).await?;
 
-        let deny_only = ak == cred.access_key;
+        // Security fix: Always require explicit Allow permission for GetUser
+        // Users should have explicit GetUser permission to view account information
+        // This ensures consistent security semantics across all admin operations
         validate_admin_request(
             &req.headers,
             &cred,
             owner,
-            deny_only,
+            false, // Always require explicit Allow permission
             vec![Action::AdminAction(AdminAction::GetUserAdminAction)],
             req.extensions.get::<Option<RemoteAddr>>().and_then(|opt| opt.map(|a| a.0)),
         )