From c46526822b8d8891942bc0ada4e2c0b37bc8c6d1 Mon Sep 17 00:00:00 2001 From: Chris Lu Date: Thu, 2 Jul 2026 09:14:54 -0700 Subject: [PATCH] s3: embedded IAM inline policy honors prefix-scoped resources (#10192) * s3: embedded IAM inline policy honors prefix-scoped resources getActions stripped the trailing wildcard from a resource like arn:aws:s3:::bucket/prefix/*, producing a non-wildcard action (Write:bucket/prefix) that CanDo only ever matched at bucket level, so PutObject under the prefix was denied. Preserve the object path with its wildcard (Write:bucket/prefix/*) to match objects under the prefix, matching the standalone iamapi behavior. * s3: prune bucket-confined wildcard actions on bucket delete actionScopedToBucket treated any wildcard as multi-bucket, so a prefix-scoped action like Write:bucket/prefix/* survived deletion of its own bucket and could re-grant access if the bucket was recreated. Scope the wildcard check to the bucket segment only: a wildcard in the object path stays scoped to its bucket, while one in the bucket segment does not. --- weed/s3api/auth_credentials.go | 11 +++++--- weed/s3api/auth_prune_bucket_test.go | 26 +++++++++++++++++++ weed/s3api/s3api_embedded_iam.go | 14 ++++------ weed/s3api/s3api_embedded_iam_test.go | 37 +++++++++++++++++++++++++++ 4 files changed, 75 insertions(+), 13 deletions(-) create mode 100644 weed/s3api/auth_prune_bucket_test.go diff --git a/weed/s3api/auth_credentials.go b/weed/s3api/auth_credentials.go index 235f1335b..0226aea11 100644 --- a/weed/s3api/auth_credentials.go +++ b/weed/s3api/auth_credentials.go @@ -2059,18 +2059,21 @@ func (iam *IdentityAccessManagement) PruneBucketFromConfiguration(ctx context.Co } // actionScopedToBucket reports whether a configured action string like -// "Read:bucket" or "Write:bucket/prefix" is scoped exclusively to the given -// bucket. Wildcard resources are never considered scoped to a single bucket. +// "Read:bucket", "Write:bucket/prefix" or "Write:bucket/prefix/*" is scoped +// exclusively to the given bucket. A wildcard in the bucket segment (e.g. "*" +// or "buck*/x") may cover other buckets and is never single-bucket scoped; a +// wildcard confined to the object path stays scoped to this bucket. func actionScopedToBucket(action, bucket string) bool { idx := strings.Index(action, ":") if idx < 0 { return false } resource := action[idx+1:] - if strings.ContainsAny(resource, "*?") { + bucketSeg, _, _ := strings.Cut(resource, "/") + if strings.ContainsAny(bucketSeg, "*?") { return false } - return resource == bucket || strings.HasPrefix(resource, bucket+"/") + return bucketSeg == bucket } // LoadS3ApiConfigurationFromCredentialManager loads configuration using the credential manager diff --git a/weed/s3api/auth_prune_bucket_test.go b/weed/s3api/auth_prune_bucket_test.go new file mode 100644 index 000000000..e9c2b2644 --- /dev/null +++ b/weed/s3api/auth_prune_bucket_test.go @@ -0,0 +1,26 @@ +package s3api + +import "testing" + +func TestActionScopedToBucket(t *testing.T) { + cases := []struct { + action string + bucket string + want bool + }{ + {"Read:bucket", "bucket", true}, + {"Write:bucket/prefix", "bucket", true}, + {"Write:bucket/prefix/*", "bucket", true}, + {"Write:bucket/*", "bucket", true}, + {"Read:other/prefix/*", "bucket", false}, + {"Write:*", "bucket", false}, + {"Write:buck*/x", "bucket", false}, + {"Write:bucketother", "bucket", false}, + {"Admin", "bucket", false}, + } + for _, c := range cases { + if got := actionScopedToBucket(c.action, c.bucket); got != c.want { + t.Errorf("actionScopedToBucket(%q, %q) = %v, want %v", c.action, c.bucket, got, c.want) + } + } +} diff --git a/weed/s3api/s3api_embedded_iam.go b/weed/s3api/s3api_embedded_iam.go index 751a339ca..8dcea749a 100644 --- a/weed/s3api/s3api_embedded_iam.go +++ b/weed/s3api/s3api_embedded_iam.go @@ -989,15 +989,11 @@ func (e *EmbeddedIamApi) getActions(policy *policy_engine.PolicyDocument) ([]str // Bucket-level or bucket/* - use just bucket name actions = append(actions, fmt.Sprintf("%s:%s", statementAction, bucket)) } else { - // Path-specific: bucket/path/* -> Action:bucket/path - // Remove trailing /* if present for cleaner action format - objectPath = strings.TrimSuffix(objectPath, "/*") - objectPath = strings.TrimSuffix(objectPath, "*") - if objectPath == "" { - actions = append(actions, fmt.Sprintf("%s:%s", statementAction, bucket)) - } else { - actions = append(actions, fmt.Sprintf("%s:%s/%s", statementAction, bucket, objectPath)) - } + // Path-specific: preserve the object path, including any + // trailing wildcard, so CanDo can match objects under the + // prefix. Stripping the wildcard yielded a non-wildcard + // action that only matched at bucket level. + actions = append(actions, fmt.Sprintf("%s:%s/%s", statementAction, bucket, objectPath)) } } } diff --git a/weed/s3api/s3api_embedded_iam_test.go b/weed/s3api/s3api_embedded_iam_test.go index 6701e5e17..4e82baaa3 100644 --- a/weed/s3api/s3api_embedded_iam_test.go +++ b/weed/s3api/s3api_embedded_iam_test.go @@ -2095,6 +2095,43 @@ func TestEmbeddedIamGetActionsFromPolicy(t *testing.T) { assert.Contains(t, actions, "Write:mybucket") } +// TestEmbeddedIamGetActionsPrefixScopedResource verifies that a prefix-scoped +// resource keeps its trailing wildcard so CanDo authorizes objects under the +// prefix. Stripping the wildcard produced a non-wildcard action that only +// matched at bucket level, denying every PutObject under the prefix. +func TestEmbeddedIamGetActionsPrefixScopedResource(t *testing.T) { + api := NewEmbeddedIamApiForTest() + + policyDoc := `{ + "Version": "2012-10-17", + "Statement": [{ + "Effect": "Allow", + "Action": ["s3:Get*", "s3:Put*", "s3:DeleteObject"], + "Resource": ["arn:aws:s3:::mcp-upload/mcp/TMS/api_keys/example/*"] + }] + }` + + policy, err := api.GetPolicyDocument(&policyDoc) + require.NoError(t, err) + + actions, err := api.getActions(&policy) + require.NoError(t, err) + assert.Contains(t, actions, "Read:mcp-upload/mcp/TMS/api_keys/example/*") + assert.Contains(t, actions, "Write:mcp-upload/mcp/TMS/api_keys/example/*") + + identity := &Identity{Name: "prefix-user"} + for _, a := range actions { + identity.Actions = append(identity.Actions, Action(a)) + } + + assert.True(t, identity.CanDo(ACTION_WRITE, "mcp-upload", "mcp/TMS/api_keys/example/logo.png"), + "PutObject under the allowed prefix should be authorized") + assert.True(t, identity.CanDo(ACTION_READ, "mcp-upload", "mcp/TMS/api_keys/example/logo.png"), + "GetObject under the allowed prefix should be authorized") + assert.False(t, identity.CanDo(ACTION_WRITE, "mcp-upload", "other/logo.png"), + "PutObject outside the allowed prefix should be denied") +} + // TestEmbeddedIamPutUserPolicyAllResourceWildcard reproduces issue #9209: // an AWS-style policy using "Action":"s3:*" with a bare "Resource":"*" is // valid AWS IAM syntax (meaning "any resource") but was rejected with