mirror of
https://github.com/seaweedfs/seaweedfs.git
synced 2026-09-29 11:15:34 +00:00
s3: do not promote ?prefix into the object for non-List actions (#11494)
* s3: do not promote ?prefix into the object for non-List actions authRequestWithAuthType mapped an empty object to the prefix parameter for every action, so PUT /bucket?versioning&prefix=x authorized as Write:bucket/x. An object-scoped grant (Write:bucket/*) could then change bucket versioning, lifecycle, cors, and object-lock configuration, and the promoted object also made ResolveS3Action report s3:PutObject to attached IAM policies. Generated with [Devin](https://devin.ai) Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com> * s3: treat GET ?uploads as a bucket listing for authorization Generated with [Devin](https://devin.ai) Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com> * s3: resolve the listing action through the bucket-level object resolveS3AuthTarget fed the promoted prefix to ResolveS3Action, so a bucket-level ?uploads request resolved as s3:GetObject on the prefix ARN in the admin explicit-deny check. Resolve both action and resource against the object the bucket listing actually scopes. Generated with [Devin](https://devin.ai) Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com> * s3: resolve the listing action through the bucket-level object in AuthorizeAction Generated with [Devin](https://devin.ai) Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com> * s3: drop the unreachable object-level uploads case from the resolver test Generated with [Devin](https://devin.ai) Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com> --------- Co-authored-by: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
This commit is contained in:
Chris Lu
co-authored by
Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
parent
4303b3aa4c
commit
f9289f0570
@@ -1780,13 +1780,13 @@ func (iam *IdentityAccessManagement) authRequestWithAuthType(r *http.Request, ac
|
||||
bucket, object := s3_constants.GetBucketAndObject(r)
|
||||
prefix := s3_constants.GetPrefix(r)
|
||||
|
||||
// For List operations, use prefix for permission checking if available
|
||||
if action == s3_constants.ACTION_LIST && object == "" && prefix != "" {
|
||||
// List operation with prefix - check permission for the prefix path
|
||||
object = prefix
|
||||
} else if (object == "/" || object == "") && prefix != "" {
|
||||
// Using the aws cli with s3, and s3api, and with boto3, the object is often set to "/" or empty
|
||||
// but the prefix is set to the actual object key for permission checking
|
||||
// For bucket listings, use prefix for permission checking if available:
|
||||
// the aws cli, s3api, and boto3 carry the key scope in ?prefix= rather
|
||||
// than the URL path. Other actions must not promote it — a bucket-level
|
||||
// request has no object, and a promoted prefix would let an object-scoped
|
||||
// grant (e.g. Write:bucket/*) authorize bucket-subresource operations
|
||||
// like ?versioning or ?lifecycle as if they were object writes.
|
||||
if isBucketListingRequest(r, action) && (object == "" || object == "/") && prefix != "" {
|
||||
object = prefix
|
||||
}
|
||||
|
||||
|
||||
@@ -124,3 +124,54 @@ func TestAttachedPolicyExplicitDenyOverridesNativeAdmin(t *testing.T) {
|
||||
assert.Equal(t, s3err.ErrAccessDenied, errCode,
|
||||
"explicit Deny in an attached policy must override native Admin")
|
||||
}
|
||||
|
||||
// GET ?uploads lists multipart uploads at bucket level but routes under Read,
|
||||
// so the prefix promoted into object must not change the evaluated action or
|
||||
// resource: an explicit Deny on s3:ListBucketMultipartUploads must still
|
||||
// constrain a native admin, and an Allow on the bucket ARN must satisfy a
|
||||
// plain attached-policy identity.
|
||||
func TestMultipartListingResolvesBucketAction(t *testing.T) {
|
||||
mgr := newTestIAMManager(t)
|
||||
iam := &IdentityAccessManagement{}
|
||||
iam.SetIAMIntegration(NewS3IAMIntegration(mgr, ""))
|
||||
|
||||
denyDoc, _ := json.Marshal(map[string]interface{}{
|
||||
"Version": "2012-10-17",
|
||||
"Statement": []map[string]interface{}{
|
||||
{"Effect": "Deny", "Action": "s3:ListBucketMultipartUploads", "Resource": "arn:aws:s3:::mybucket"},
|
||||
},
|
||||
})
|
||||
require.NoError(t, iam.PutPolicy("DenyUploadsListing", string(denyDoc)))
|
||||
allowDoc, _ := json.Marshal(map[string]interface{}{
|
||||
"Version": "2012-10-17",
|
||||
"Statement": []map[string]interface{}{
|
||||
{"Effect": "Allow", "Action": "s3:ListBucketMultipartUploads", "Resource": "arn:aws:s3:::mybucket"},
|
||||
},
|
||||
})
|
||||
require.NoError(t, iam.PutPolicy("AllowUploadsListing", string(allowDoc)))
|
||||
|
||||
uploadsReq := func() *http.Request {
|
||||
return httptest.NewRequest(http.MethodGet, "/mybucket?uploads&prefix=x/", nil)
|
||||
}
|
||||
admin := &Identity{
|
||||
Name: "admin",
|
||||
Account: &Account{DisplayName: "admin", Id: "admin"},
|
||||
Actions: []Action{s3_constants.ACTION_ADMIN},
|
||||
PolicyNames: []string{"DenyUploadsListing"},
|
||||
PrincipalArn: "arn:aws:iam::111122223333:user/admin",
|
||||
}
|
||||
reader := &Identity{
|
||||
Name: "reader",
|
||||
Account: &Account{DisplayName: "reader", Id: "reader"},
|
||||
PolicyNames: []string{"AllowUploadsListing"},
|
||||
PrincipalArn: "arn:aws:iam::111122223333:user/reader",
|
||||
}
|
||||
|
||||
// object carries the promoted prefix, matching authRequestWithAuthType
|
||||
assert.Equal(t, s3err.ErrAccessDenied,
|
||||
iam.VerifyActionPermission(uploadsReq(), admin, s3_constants.ACTION_READ, "mybucket", "x/"),
|
||||
"explicit Deny on the uploads listing must override native Admin")
|
||||
assert.Equal(t, s3err.ErrNone,
|
||||
iam.VerifyActionPermission(uploadsReq(), reader, s3_constants.ACTION_READ, "mybucket", "x/"),
|
||||
"s3:ListBucketMultipartUploads on the bucket must allow the uploads listing")
|
||||
}
|
||||
|
||||
@@ -12,6 +12,7 @@ import (
|
||||
|
||||
"github.com/aws/aws-sdk-go-v2/aws"
|
||||
v4 "github.com/aws/aws-sdk-go-v2/aws/signer/v4"
|
||||
"github.com/gorilla/mux"
|
||||
"github.com/seaweedfs/seaweedfs/weed/s3api/s3_constants"
|
||||
"github.com/seaweedfs/seaweedfs/weed/s3api/s3err"
|
||||
"github.com/stretchr/testify/assert"
|
||||
@@ -557,3 +558,108 @@ func TestRealSDKSignerWithForwardedHeaders(t *testing.T) {
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
// GHSA-8rrx-349w-6396: ?prefix= was promoted into the object for every action,
|
||||
// so PUT /cache?versioning&prefix=x authorized as Write:cache/x. An identity
|
||||
// holding only Write:cache/* could then change bucket versioning, lifecycle,
|
||||
// cors, and object-lock configuration.
|
||||
func TestPrefixParameterDoesNotEscalateBucketActions(t *testing.T) {
|
||||
resetMemoryStore()
|
||||
defer resetMemoryStore()
|
||||
|
||||
configContent := `{
|
||||
"identities": [
|
||||
{"name":"admin","credentials":[{"accessKey":"ADMINKEY","secretKey":"adminsecret0000000000000000000001"}],"actions":["Admin"]},
|
||||
{"name":"writer","credentials":[{"accessKey":"WRITERKEY","secretKey":"writersecret000000000000000000001"}],"actions":["Read:cache","Write:cache/*","List:cache"]},
|
||||
{"name":"uploadreader","credentials":[{"accessKey":"UPREADERKEY","secretKey":"upreadersecret00000000000000001"}],"actions":["Read:cache/uploads/*"]},
|
||||
{"name":"policywriter","credentials":[{"accessKey":"POLICYKEY","secretKey":"policysecret00000000000000000001"}],"policyNames":["WriterPolicy"]},
|
||||
{"name":"uploadlister","credentials":[{"accessKey":"UPLISTERKEY","secretKey":"uplistersecret00000000000000001"}],"policyNames":["UploadsPolicy"]},
|
||||
{"name":"getonly","credentials":[{"accessKey":"GETONLYKEY","secretKey":"getonlysecret0000000000000000001"}],"policyNames":["GetOnlyPolicy"]}
|
||||
],
|
||||
"policies":[
|
||||
{"name":"WriterPolicy","content":"{\"Version\":\"2012-10-17\",\"Statement\":[{\"Effect\":\"Allow\",\"Action\":[\"s3:PutObject\"],\"Resource\":[\"arn:aws:s3:::cache/*\"]}]}"},
|
||||
{"name":"UploadsPolicy","content":"{\"Version\":\"2012-10-17\",\"Statement\":[{\"Effect\":\"Allow\",\"Action\":[\"s3:ListBucketMultipartUploads\"],\"Resource\":[\"arn:aws:s3:::cache\"]}]}"},
|
||||
{"name":"GetOnlyPolicy","content":"{\"Version\":\"2012-10-17\",\"Statement\":[{\"Effect\":\"Allow\",\"Action\":[\"s3:GetObject\"],\"Resource\":[\"arn:aws:s3:::cache/*\"]}]}"}
|
||||
]
|
||||
}`
|
||||
tmpFile, err := os.CreateTemp("", "s3-config-*.json")
|
||||
require.NoError(t, err)
|
||||
defer os.Remove(tmpFile.Name())
|
||||
_, err = tmpFile.Write([]byte(configContent))
|
||||
require.NoError(t, err)
|
||||
require.NoError(t, tmpFile.Close())
|
||||
|
||||
iam := NewIdentityAccessManagementWithStore(&S3ApiServerOption{Config: tmpFile.Name()}, nil, "memory")
|
||||
require.True(t, iam.isEnabled(), "Auth should be enabled")
|
||||
|
||||
for _, tc := range []struct {
|
||||
name string
|
||||
access string
|
||||
secret string
|
||||
object string
|
||||
query string
|
||||
action Action
|
||||
want s3err.ErrorCode
|
||||
}{
|
||||
{"writer put object", "WRITERKEY", "writersecret000000000000000000001", "key", "", s3_constants.ACTION_WRITE, s3err.ErrNone},
|
||||
{"writer put versioning", "WRITERKEY", "writersecret000000000000000000001", "", "versioning", s3_constants.ACTION_WRITE, s3err.ErrAccessDenied},
|
||||
{"writer put versioning with prefix", "WRITERKEY", "writersecret000000000000000000001", "", "versioning&prefix=x", s3_constants.ACTION_WRITE, s3err.ErrAccessDenied},
|
||||
{"writer put lifecycle with prefix", "WRITERKEY", "writersecret000000000000000000001", "", "lifecycle&prefix=x", s3_constants.ACTION_WRITE, s3err.ErrAccessDenied},
|
||||
{"writer list with prefix", "WRITERKEY", "writersecret000000000000000000001", "", "list-type=2&prefix=x", s3_constants.ACTION_LIST, s3err.ErrNone},
|
||||
{"policywriter put object", "POLICYKEY", "policysecret00000000000000000001", "key", "", s3_constants.ACTION_WRITE, s3err.ErrNone},
|
||||
{"policywriter put versioning with prefix", "POLICYKEY", "policysecret00000000000000000001", "", "versioning&prefix=x", s3_constants.ACTION_WRITE, s3err.ErrAccessDenied},
|
||||
{"uploadreader lists uploads under prefix", "UPREADERKEY", "upreadersecret00000000000000001", "", "uploads&prefix=uploads/foo", s3_constants.ACTION_READ, s3err.ErrNone},
|
||||
{"uploadreader cannot list outside prefix", "UPREADERKEY", "upreadersecret00000000000000001", "", "uploads&prefix=other/", s3_constants.ACTION_READ, s3err.ErrAccessDenied},
|
||||
{"uploadlister lists uploads", "UPLISTERKEY", "uplistersecret00000000000000001", "", "uploads&prefix=x", s3_constants.ACTION_READ, s3err.ErrNone},
|
||||
{"getonly cannot list uploads", "GETONLYKEY", "getonlysecret0000000000000000001", "", "uploads&prefix=x", s3_constants.ACTION_READ, s3err.ErrAccessDenied},
|
||||
} {
|
||||
t.Run(tc.name, func(t *testing.T) {
|
||||
url := "http://localhost:8333/cache"
|
||||
if tc.object != "" {
|
||||
url += "/" + tc.object
|
||||
}
|
||||
if tc.query != "" {
|
||||
url += "?" + tc.query
|
||||
}
|
||||
r := httptest.NewRequest(http.MethodPut, url, nil)
|
||||
if tc.action == s3_constants.ACTION_LIST || tc.action == s3_constants.ACTION_READ {
|
||||
r.Method = http.MethodGet
|
||||
}
|
||||
r = mux.SetURLVars(r, map[string]string{"bucket": "cache", "object": tc.object})
|
||||
require.NoError(t, signRawHTTPRequest(context.Background(), r, tc.access, tc.secret, "us-east-1"))
|
||||
|
||||
_, errCode := iam.authRequest(r, tc.action)
|
||||
assert.Equal(t, tc.want, errCode)
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
// The admin explicit-deny path resolves the same action and resource the
|
||||
// policy engine sees, so a promoted prefix must not hide a listing variant:
|
||||
// ?uploads resolves s3:ListBucketMultipartUploads on the bucket ARN, and
|
||||
// ?versions resolves s3:ListBucketVersions, both at bucket level.
|
||||
func TestResolveS3AuthTarget_BucketListings(t *testing.T) {
|
||||
for _, tt := range []struct {
|
||||
name string
|
||||
url string
|
||||
action Action
|
||||
object string
|
||||
wantAction string
|
||||
wantResource string
|
||||
}{
|
||||
{"uploads listing keeps its action", "/cache?uploads&prefix=uploads/", s3_constants.ACTION_READ, "uploads/",
|
||||
s3_constants.S3_ACTION_LIST_MULTIPART_UPLOADS, "arn:aws:s3:::cache"},
|
||||
{"versions listing keeps its action", "/cache?versions&prefix=a/", s3_constants.ACTION_LIST, "a/",
|
||||
s3_constants.S3_ACTION_LIST_BUCKET_VERSIONS, "arn:aws:s3:::cache"},
|
||||
{"plain list keeps its action", "/cache?list-type=2&prefix=a/", s3_constants.ACTION_LIST, "a/",
|
||||
s3_constants.S3_ACTION_LIST_BUCKET, "arn:aws:s3:::cache"},
|
||||
} {
|
||||
t.Run(tt.name, func(t *testing.T) {
|
||||
r := httptest.NewRequest(http.MethodGet, tt.url, nil)
|
||||
r = mux.SetURLVars(r, map[string]string{"bucket": "cache"})
|
||||
action, resource := resolveS3AuthTarget(tt.action, "cache", tt.object, r)
|
||||
assert.Equal(t, tt.wantAction, action)
|
||||
assert.Equal(t, tt.wantResource, resource)
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
@@ -260,7 +260,7 @@ func (s3iam *S3IAMIntegration) AuthorizeAction(ctx context.Context, identity *IA
|
||||
|
||||
// Determine the specific S3 action based on the HTTP request details. The
|
||||
// prefix promoted into objectKey is not part of the URL; resolve against
|
||||
// the bucket-level object so ?versions keeps its own action.
|
||||
// the bucket-level object so ?versions and ?uploads keep their own action.
|
||||
specificAction := ResolveS3Action(r, string(action), bucket, resourceObjectKey)
|
||||
|
||||
// Create action request
|
||||
|
||||
Reference in New Issue
Block a user