fix(iam): preserve actions/resources in GetUserPolicy fallback (#9009)

* fix(iam): preserve actions/resources in GetUserPolicy fallback (#9008)

When GetUserPolicy cannot find a stored inline policy document and falls
back to reconstructing one from the aggregated ident.Actions, it produced
mangled output: bare-bucket paths like "b-le*/*" got another "/*" appended
(becoming "b-le*/*/*"), and distinct s3 actions that map to the same
coarse verb (e.g. s3:GetObject and s3:GetBucketLocation -> s3:Get*) were
emitted multiple times in the same statement.

- Use SplitN so paths containing ':' are not shredded.
- Only append "/*" to bare bucket patterns; paths already containing '/'
  are used as-is.
- Dedupe reconstructed actions per resource.

Adds a regression test using the exact reproducer from the issue.

* fix(iam): preserve bucket-level ARNs in fallback reconstruction

Addresses CodeRabbit review feedback on #9009:

- Use stored path verbatim in the GetUserPolicy fallback so bucket-level
  resources (e.g. arn:aws:s3:::b-le*) are not rewritten to object-level
  ARNs (arn:aws:s3:::b-le*/*). Previously bare bucket patterns had "/*"
  appended, conflating bucket and object resources.
- Extend TestPutGetUserPolicyIssue9008 to also exercise the fallback
  reconstruction path by clearing the persisted inline policy between
  the two GetUserPolicy calls, validating that bucket and object
  resources stay distinct.

* chore: revert accidental scheduled_tasks.lock change
This commit is contained in:
Chris Lu
2026-04-09 11:48:51 -07:00
committed by GitHub
parent dd203769b1
commit e4bcfb96d8
2 changed files with 93 additions and 6 deletions

View File

@@ -458,17 +458,33 @@ func (iama *IamApiServer) GetUserPolicy(s3cfg *iam_pb.S3ApiConfiguration, values
policyDocument := policy_engine.PolicyDocument{Version: policyDocumentVersion}
statements := make(map[string][]string)
seenAction := make(map[string]map[string]bool)
for _, action := range ident.Actions {
// parse "Read:EXAMPLE-BUCKET"
act := strings.Split(action, ":")
// parse "Read:EXAMPLE-BUCKET" or "Read:EXAMPLE-BUCKET/prefix/*"
// Use SplitN so the path component (which may contain ':') is preserved intact.
act := strings.SplitN(action, ":", 2)
resource := "*"
if len(act) == 2 {
resource = fmt.Sprintf("arn:aws:s3:::%s/*", act[1])
// Preserve the stored path verbatim so bucket-level and
// object-level resources remain distinguishable. GetActions
// stores the path exactly as parsed from the original ARN
// (e.g. "b-le*" for the bucket, "b-le*/*" for objects), and
// reconstruction should not rewrite one into the other.
resource = fmt.Sprintf("arn:aws:s3:::%s", act[1])
}
statements[resource] = append(statements[resource],
fmt.Sprintf("s3:%s", MapToIdentitiesAction(act[0])),
)
s3Action := fmt.Sprintf("s3:%s", MapToIdentitiesAction(act[0]))
// Dedupe actions per resource: the Read/Write/List internal verbs map to
// coarse wildcards (s3:Get*, s3:Put*, s3:List*), so multiple distinct
// original actions can collapse to the same reconstructed verb.
if seenAction[resource] == nil {
seenAction[resource] = make(map[string]bool)
}
if seenAction[resource][s3Action] {
continue
}
seenAction[resource][s3Action] = true
statements[resource] = append(statements[resource], s3Action)
}
for resource, actions := range statements {
isEqAction := false

View File

@@ -186,6 +186,77 @@ func TestPutGetUserPolicyPreservesStatements(t *testing.T) {
assert.True(t, deleteObjectFound, "s3:DeleteObject action was lost")
}
// TestPutGetUserPolicyIssue9008 is a regression test for
// https://github.com/seaweedfs/seaweedfs/issues/9008: put-user-policy followed
// by get-user-policy must return the same policy document that was submitted,
// with Action and Resource lists intact (no duplication, no collapsing).
func TestPutGetUserPolicyIssue9008(t *testing.T) {
s3cfg := &iam_pb.S3ApiConfiguration{
Identities: []*iam_pb.Identity{{Name: "steward"}},
}
policyJSON := `{
"Version": "2012-10-17",
"Statement": [{
"Effect": "Allow",
"Action": ["s3:GetObject", "s3:PutObject", "s3:ListBucket"],
"Resource": ["arn:aws:s3:::b-le*", "arn:aws:s3:::b-le*/*"]
}]
}`
mockCfg := &mockIamS3ApiConfig{}
iama := &IamApiServer{s3ApiConfig: mockCfg}
_, iamErr := iama.PutUserPolicy(s3cfg, url.Values{
"UserName": []string{"steward"},
"PolicyName": []string{"steward_policy"},
"PolicyDocument": []string{policyJSON},
})
assert.Nil(t, iamErr)
// Part 1: verbatim round-trip. GetUserPolicy returns the exact document
// that was persisted, with Action and Resource lists intact.
resp, iamErr := iama.GetUserPolicy(s3cfg, url.Values{
"UserName": []string{"steward"},
"PolicyName": []string{"steward_policy"},
})
assert.Nil(t, iamErr)
var got policy_engine.PolicyDocument
assert.NoError(t, json.Unmarshal([]byte(resp.GetUserPolicyResult.PolicyDocument), &got))
assert.Equal(t, "2012-10-17", got.Version)
assert.Equal(t, 1, len(got.Statement))
stmt := got.Statement[0]
assert.Equal(t, policy_engine.PolicyEffectAllow, stmt.Effect)
assert.ElementsMatch(t, []string{"s3:GetObject", "s3:PutObject", "s3:ListBucket"}, stmt.Action.Strings())
assert.ElementsMatch(t, []string{"arn:aws:s3:::b-le*", "arn:aws:s3:::b-le*/*"}, stmt.Resource.Strings())
// Part 2: fallback reconstruction. Clear the persisted inline policy so
// GetUserPolicy must rebuild the document from ident.Actions. The fallback
// is lossy (distinct S3 verbs collapse to wildcards like s3:Get*), but it
// must not duplicate actions nor conflate bucket-level and object-level
// resources.
mockCfg.policies = Policies{}
resp, iamErr = iama.GetUserPolicy(s3cfg, url.Values{
"UserName": []string{"steward"},
"PolicyName": []string{"steward_policy"},
})
assert.Nil(t, iamErr)
var fallback policy_engine.PolicyDocument
assert.NoError(t, json.Unmarshal([]byte(resp.GetUserPolicyResult.PolicyDocument), &fallback))
assert.Equal(t, 1, len(fallback.Statement), "fallback should merge equal-action statements")
fstmt := fallback.Statement[0]
// Each coarse verb appears exactly once (no duplication from the
// Read/Write/List -> s3:Get*/s3:Put*/s3:List* expansion).
assert.ElementsMatch(t, []string{"s3:Get*", "s3:Put*", "s3:List*"}, fstmt.Action.Strings())
// Bucket-level and object-level resources stay distinct — the bare bucket
// pattern must not be rewritten to an object ARN.
assert.ElementsMatch(t, []string{"arn:aws:s3:::b-le*", "arn:aws:s3:::b-le*/*"}, fstmt.Resource.Strings())
}
func TestMultipleInlinePoliciesAggregateActions(t *testing.T) {
s3cfg := &iam_pb.S3ApiConfiguration{
Identities: []*iam_pb.Identity{{Name: "alice"}},