Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix statement.IsAllowed when bucket disallowed #132

Merged
merged 1 commit into from
Sep 21, 2024

Conversation

vfauth
Copy link
Contributor

@vfauth vfauth commented Sep 18, 2024

Resolves minio/minio#20449

I also added some tests.

By the way, some existing tests seem to be duplicate of each other, can somebody understand what their use is? If not, I may remove them.
Because:

  • anonGetBucketLocationArgs and getBucketLocationArgs are exactly the same
  • anonPutObjectActionArgs and putObjectActionArgs are exactly the same
  • anonGetObjectActionArgs and gutObjectActionArgs are exactly the same

@harshavardhana
Copy link
Member

@donatello @vadmeste please review this carefully, as its a breaking change.

Copy link
Contributor

@taran-p taran-p left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change fixes problem in mentioned issue. Doesn't break anything unexpected in my testing, so as long as the expected breaking changes are ok, it LGTM.

@harshavardhana harshavardhana merged commit 4f16e1e into minio:main Sep 21, 2024
3 checks passed
@harshavardhana
Copy link
Member

Thank you @taran-p - onwards.

@harshavardhana
Copy link
Member

This PR does actually break this test

        // 2.2 create and associate policy to user                                                                                                                                                                                            
        policy := "mypolicy-test-user-update"                                                                                                                                                                                                 
        policyBytes := []byte(fmt.Sprintf(`{                                                                                                                                                                                                  
 "Version": "2012-10-17",                                                                                                                                                                                                                     
 "Statement": [                                                                                                                                                                                                                               
  {                                                                                                                                                                                                                                           
   "Effect": "Allow",                                                                                                                                                                                                                         
   "Action": [                                                                                                                                                                                                                                
    "s3:PutObject",                                                                                                                                                                                                                           
    "s3:GetObject",                                                                                                                                                                                                                           
    "s3:ListBucket"                                                                                                                                                                                                                           
   ],                                                                                                                                                                                                                                         
   "Resource": [                                                                                                                                                                                                                              
    "arn:aws:s3:::%s/*"                                                                                                                                                                                                                       
   ]                                                                                                                                                                                                                                          
  }                                                                                                                                                                                                                                           
 ]                                                                                                                                                                                                                                            
}`, bucket))                                                                                                                                                                                                                                  
        err = s.adm.AddCannedPolicy(ctx, policy, policyBytes)                                                                                                                                                                                 
        if err != nil {                                                                                                                                                                                                                       
                c.Fatalf("policy add error: %v", err)                                                                                                                                                                                         
        }                                                                                                                                                                                                                                     
        _, err = s.adm.AttachPolicy(ctx, madmin.PolicyAssociationReq{                                                                                                                                                                         
                Policies: []string{policy},                                                                                                                                                                                                   
                User:     accessKey,                                                                                                                                                                                                          
        })                                                                                                                                                                                                                                    
        if err != nil {                                                                                                                                                                                                                       
                c.Fatalf("unable to attach policy: %v", err)                                                                                                                                                                                  
        }                                                                                                                                                                                                                                     
        // 2.3 check user has access to bucket                                                                                                                                                                                                
        c.mustListObjects(ctx, uClient, bucket)    

@harshavardhana
Copy link
Member

harshavardhana commented Sep 21, 2024

This PR also breaks a policy like this where ListObjects() must work but fails

        // Create policy                                                                                                                                                                      
        policy := "mypolicy-username"                                                                                                                                                         
        policyBytes := []byte(`{                                                                                                                                                              
 "Version": "2012-10-17",                                                                                                                                                                     
 "Statement": [                                                                                                                                                                               
  {                                                                                                                                                                                           
   "Effect": "Allow",                                                                                                                                                                         
   "Action": [                                                                                                                                                                                
    "s3:*"                                                                                                                                                                                    
   ],                                                                                                                                                                                         
   "Resource": [                                                                                                                                                                              
    "arn:aws:s3:::${aws:username}-*"                                                                                                                                                          
   ]                                                                                                                                                                                          
  }                                                                                                                                                                                           
 ]                                                                                                                                                                                            
}`)    

The PR is reverted.

harshavardhana added a commit that referenced this pull request Sep 21, 2024
harshavardhana added a commit that referenced this pull request Sep 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Policy allow action on bucket even when it should only be allowed on objects
3 participants