Browse Source

Fix S3 bucket policy ARN validation to accept AWS ARNs and simplified formats (#7409)

* Fix S3 bucket policy ARN validation to accept AWS ARNs and simplified formats

Fixes #7252

The bucket policy validation was rejecting valid AWS-style ARNs and
simplified resource formats, causing validation failures with the
error 'resource X does not match bucket X' even when they were
identical strings.

Changes:
- Updated validateResourceForBucket() to accept three formats:
  1. AWS-style ARNs: arn:aws:s3:::bucket-name[/*|/path]
  2. SeaweedFS ARNs: arn:seaweed:s3:::bucket-name[/*|/path]
  3. Simplified formats: bucket-name[/*|/path]

- Added comprehensive test coverage for all three formats
- Added specific test cases from issue #7252 to prevent regression

This ensures compatibility with standard AWS S3 bucket policies
while maintaining support for SeaweedFS-specific ARN format.

* Refactor validateResourceForBucket to reduce code duplication

Simplified the validation logic by stripping ARN prefixes first,
then performing validation on the remaining resource path.
This reduces code duplication and improves maintainability while
maintaining identical functionality.

Addresses review feedback from Gemini Code Assist.

* Use strings.CutPrefix for cleaner ARN prefix stripping

Replace strings.HasPrefix checks with strings.CutPrefix for more
idiomatic Go code. This function is available in Go 1.20+ and
provides cleaner conditional logic with the combined check and
extraction.

Addresses review feedback from Gemini Code Assist.
pull/7414/head
Chris Lu 5 days ago
committed by GitHub
parent
commit
d00a2a8707
No known key found for this signature in database GPG Key ID: B5690EEEBB952194
  1. 177
      weed/s3api/s3_bucket_policy_simple_test.go
  2. 36
      weed/s3api/s3api_bucket_policy_handlers.go

177
weed/s3api/s3_bucket_policy_simple_test.go

@ -143,42 +143,106 @@ func TestBucketResourceValidation(t *testing.T) {
bucket string
valid bool
}{
// SeaweedFS ARN format
{
name: "Exact bucket ARN",
name: "Exact bucket ARN (SeaweedFS)",
resource: "arn:seaweed:s3:::test-bucket",
bucket: "test-bucket",
valid: true,
},
{
name: "Bucket wildcard ARN",
name: "Bucket wildcard ARN (SeaweedFS)",
resource: "arn:seaweed:s3:::test-bucket/*",
bucket: "test-bucket",
valid: true,
},
{
name: "Specific object ARN",
name: "Specific object ARN (SeaweedFS)",
resource: "arn:seaweed:s3:::test-bucket/path/to/object.txt",
bucket: "test-bucket",
valid: true,
},
// AWS ARN format (compatibility)
{
name: "Different bucket ARN",
name: "Exact bucket ARN (AWS)",
resource: "arn:aws:s3:::test-bucket",
bucket: "test-bucket",
valid: true,
},
{
name: "Bucket wildcard ARN (AWS)",
resource: "arn:aws:s3:::test-bucket/*",
bucket: "test-bucket",
valid: true,
},
{
name: "Specific object ARN (AWS)",
resource: "arn:aws:s3:::test-bucket/path/to/object.txt",
bucket: "test-bucket",
valid: true,
},
// Simplified format (without ARN prefix)
{
name: "Simplified bucket name",
resource: "test-bucket",
bucket: "test-bucket",
valid: true,
},
{
name: "Simplified bucket wildcard",
resource: "test-bucket/*",
bucket: "test-bucket",
valid: true,
},
{
name: "Simplified specific object",
resource: "test-bucket/path/to/object.txt",
bucket: "test-bucket",
valid: true,
},
// Invalid cases
{
name: "Different bucket ARN (SeaweedFS)",
resource: "arn:seaweed:s3:::other-bucket/*",
bucket: "test-bucket",
valid: false,
},
{
name: "Global S3 wildcard",
name: "Different bucket ARN (AWS)",
resource: "arn:aws:s3:::other-bucket/*",
bucket: "test-bucket",
valid: false,
},
{
name: "Different bucket simplified",
resource: "other-bucket/*",
bucket: "test-bucket",
valid: false,
},
{
name: "Global S3 wildcard (SeaweedFS)",
resource: "arn:seaweed:s3:::*",
bucket: "test-bucket",
valid: false,
},
{
name: "Global S3 wildcard (AWS)",
resource: "arn:aws:s3:::*",
bucket: "test-bucket",
valid: false,
},
{
name: "Invalid ARN format",
resource: "invalid-arn",
bucket: "test-bucket",
valid: false,
},
{
name: "Bucket name prefix match but different bucket",
resource: "test-bucket-different/*",
bucket: "test-bucket",
valid: false,
},
}
for _, tt := range tests {
@ -226,3 +290,106 @@ func marshalPolicy(t *testing.T, policyDoc *policy.PolicyDocument) []byte {
require.NoError(t, err)
return data
}
// TestIssue7252Examples tests the specific examples from GitHub issue #7252
func TestIssue7252Examples(t *testing.T) {
s3Server := &S3ApiServer{}
tests := []struct {
name string
policy *policy.PolicyDocument
bucket string
expectedValid bool
description string
}{
{
name: "Issue #7252 - Standard ARN with wildcard",
policy: &policy.PolicyDocument{
Version: "2012-10-17",
Statement: []policy.Statement{
{
Effect: "Allow",
Principal: map[string]interface{}{
"AWS": "*",
},
Action: []string{"s3:GetObject"},
Resource: []string{"arn:aws:s3:::main-bucket/*"},
},
},
},
bucket: "main-bucket",
expectedValid: true,
description: "AWS ARN format should be accepted",
},
{
name: "Issue #7252 - Simplified resource with wildcard",
policy: &policy.PolicyDocument{
Version: "2012-10-17",
Statement: []policy.Statement{
{
Effect: "Allow",
Principal: map[string]interface{}{
"AWS": "*",
},
Action: []string{"s3:GetObject"},
Resource: []string{"main-bucket/*"},
},
},
},
bucket: "main-bucket",
expectedValid: true,
description: "Simplified format with wildcard should be accepted",
},
{
name: "Issue #7252 - Resource as exact bucket name",
policy: &policy.PolicyDocument{
Version: "2012-10-17",
Statement: []policy.Statement{
{
Effect: "Allow",
Principal: map[string]interface{}{
"AWS": "*",
},
Action: []string{"s3:GetObject"},
Resource: []string{"main-bucket"},
},
},
},
bucket: "main-bucket",
expectedValid: true,
description: "Exact bucket name should be accepted",
},
{
name: "Public read policy with AWS ARN",
policy: &policy.PolicyDocument{
Version: "2012-10-17",
Statement: []policy.Statement{
{
Sid: "PublicReadGetObject",
Effect: "Allow",
Principal: map[string]interface{}{
"AWS": "*",
},
Action: []string{"s3:GetObject"},
Resource: []string{"arn:aws:s3:::my-public-bucket/*"},
},
},
},
bucket: "my-public-bucket",
expectedValid: true,
description: "Standard public read policy with AWS ARN should work",
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
err := s3Server.validateBucketPolicy(tt.policy, tt.bucket)
if tt.expectedValid {
assert.NoError(t, err, "Policy should be valid: %s", tt.description)
} else {
assert.Error(t, err, "Policy should be invalid: %s", tt.description)
}
})
}
}

36
weed/s3api/s3api_bucket_policy_handlers.go

@ -274,18 +274,38 @@ func (s3a *S3ApiServer) validateBucketPolicy(policyDoc *policy.PolicyDocument, b
// validateResourceForBucket checks if a resource ARN is valid for the given bucket
func (s3a *S3ApiServer) validateResourceForBucket(resource, bucket string) bool {
// Expected formats:
// Accepted formats for S3 bucket policies:
// AWS-style ARNs:
// arn:aws:s3:::bucket-name
// arn:aws:s3:::bucket-name/*
// arn:aws:s3:::bucket-name/path/to/object
// SeaweedFS ARNs:
// arn:seaweed:s3:::bucket-name
// arn:seaweed:s3:::bucket-name/*
// arn:seaweed:s3:::bucket-name/path/to/object
// Simplified formats (for convenience):
// bucket-name
// bucket-name/*
// bucket-name/path/to/object
var resourcePath string
const awsPrefix = "arn:aws:s3:::"
const seaweedPrefix = "arn:seaweed:s3:::"
// Strip the optional ARN prefix to get the resource path
if path, ok := strings.CutPrefix(resource, awsPrefix); ok {
resourcePath = path
} else if path, ok := strings.CutPrefix(resource, seaweedPrefix); ok {
resourcePath = path
} else {
resourcePath = resource
}
expectedBucketArn := fmt.Sprintf("arn:seaweed:s3:::%s", bucket)
expectedBucketWildcard := fmt.Sprintf("arn:seaweed:s3:::%s/*", bucket)
expectedBucketPath := fmt.Sprintf("arn:seaweed:s3:::%s/", bucket)
return resource == expectedBucketArn ||
resource == expectedBucketWildcard ||
strings.HasPrefix(resource, expectedBucketPath)
// After stripping the optional ARN prefix, the resource path must
// either match the bucket name exactly, or be a path within the bucket.
return resourcePath == bucket ||
resourcePath == bucket+"/*" ||
strings.HasPrefix(resourcePath, bucket+"/")
}
// IAM integration functions

Loading…
Cancel
Save