mirror of
https://github.com/seaweedfs/seaweedfs.git
synced 2026-09-14 02:20:41 +02:00
s3: close list-type / ownership-controls routing mismatch (#11280)
* s3: reject list-type paired with another operation subresource ?list-type=2&ownershipControls= routes to ListObjectsV2 (the list-type route is registered first) while the IAM action resolver resolves the ownershipControls selector to s3:GetBucketOwnershipControls. A principal denied s3:ListBucket but allowed s3:GetBucketOwnershipControls would therefore list the bucket. list-type selects an operation just like the other keys in operationSubresources, so add it there and reject the combination before routing, matching the fix for policy&tagging (#10987). * s3: resolve list-type to s3:ListBucket ahead of bucket subresources The router registers the ListObjectsV2 route ahead of the bucket subresource routes, so the action resolver should resolve list-type the same way. Without this, a request carrying list-type and another operation selector resolves to the subresource action (e.g. s3:GetBucketOwnershipControls) while being served by ListObjectsV2. The ambiguity guard rejects such combinations before routing, but resolving list-type to s3:ListBucket keeps the resolver aligned with the router, mirroring how versions is handled. * s3: match list-type=2 exactly in action resolver The router selects ListObjectsV2 only for list-type=2; other values fall through to the subresource routes. Resolve the same way so the action matches the handler for every list-type value, not just 2.
This commit is contained in:
@@ -217,6 +217,12 @@ func resolveFromQueryParameters(query url.Values, method string, hasObject bool)
|
||||
}
|
||||
}
|
||||
|
||||
if query.Get("list-type") == "2" {
|
||||
if method == http.MethodGet && !hasObject {
|
||||
return s3_constants.S3_ACTION_LIST_BUCKET
|
||||
}
|
||||
}
|
||||
|
||||
// Check bucket-level query parameters using data-driven approach
|
||||
// These are strictly bucket-level operations, so only apply when !hasObject
|
||||
if !hasObject {
|
||||
|
||||
@@ -158,6 +158,33 @@ func TestResolveS3Action_CreateBucket(t *testing.T) {
|
||||
}
|
||||
}
|
||||
|
||||
// list-type selects ListObjectsV2, which the router registers ahead of the
|
||||
// bucket subresource routes. The resolver must align with routing and resolve
|
||||
// it to s3:ListBucket even when an operation subresource like ownershipControls
|
||||
// is also present, so authorization checks the listing action the handler runs.
|
||||
func TestResolveS3Action_ListType(t *testing.T) {
|
||||
tests := []struct {
|
||||
name string
|
||||
query string
|
||||
want string
|
||||
}{
|
||||
{"list-type alone", "list-type=2", s3_constants.S3_ACTION_LIST_BUCKET},
|
||||
{"list-type with listing params", "list-type=2&prefix=a&continuation-token=x", s3_constants.S3_ACTION_LIST_BUCKET},
|
||||
{"list-type with ownershipControls", "list-type=2&ownershipControls=", s3_constants.S3_ACTION_LIST_BUCKET},
|
||||
// The router only selects ListObjectsV2 for list-type=2; another value
|
||||
// falls through to the subresource route, so ownershipControls wins.
|
||||
{"list-type=1 with ownershipControls", "list-type=1&ownershipControls=", s3_constants.S3_ACTION_GET_BUCKET_OWNERSHIP_CONTROLS},
|
||||
}
|
||||
for _, tt := range tests {
|
||||
t.Run(tt.name, func(t *testing.T) {
|
||||
r, _ := http.NewRequest(http.MethodGet, "http://localhost/bucket?"+tt.query, nil)
|
||||
if got := ResolveS3Action(r, s3_constants.ACTION_LIST, "bucket", ""); got != tt.want {
|
||||
t.Errorf("ResolveS3Action() = %q, want %q", got, tt.want)
|
||||
}
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
// A base action naming another service carries no S3 request shape, so a query
|
||||
// parameter on the request must not redirect it to an S3 action.
|
||||
func TestResolveS3ActionKeepsNonS3Service(t *testing.T) {
|
||||
|
||||
@@ -45,6 +45,10 @@ func TestAmbiguousSubresource(t *testing.T) {
|
||||
"delete=&policy=",
|
||||
"uploads=&uploadId=xyz",
|
||||
"policy=&tagging=&cors=",
|
||||
"list-type=2&ownershipControls=",
|
||||
"list-type=2&tagging=",
|
||||
"ownershipControls=&list-type=2",
|
||||
"list-type=2&versions=",
|
||||
} {
|
||||
req, _ := http.NewRequest("PUT", "http://localhost/bucket?"+query, nil)
|
||||
assert.True(t, hasAmbiguousSubresource(req.URL.Query()), "%q names two operations", query)
|
||||
@@ -73,3 +77,27 @@ func TestAmbiguousSubresourceRejectedBeforeHandler(t *testing.T) {
|
||||
handler.ServeHTTP(rec, req)
|
||||
assert.True(t, served, "an unambiguous request must still be served")
|
||||
}
|
||||
|
||||
// The listing disclosure: GET /bucket?list-type=2&ownershipControls= routes to
|
||||
// ListObjectsV2 while resolving as s3:GetBucketOwnershipControls, so a principal
|
||||
// denied s3:ListBucket but allowed the ownership-controls read would list the
|
||||
// bucket. The guard has to reject the combined request before the listing handler.
|
||||
func TestListTypeOwnershipControlsRejectedBeforeHandler(t *testing.T) {
|
||||
served := false
|
||||
handler := validateRequestPath(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
|
||||
served = true
|
||||
}))
|
||||
|
||||
req, _ := http.NewRequest("GET", "http://localhost/bucket?list-type=2&ownershipControls=", nil)
|
||||
rec := httptest.NewRecorder()
|
||||
handler.ServeHTTP(rec, req)
|
||||
|
||||
require.False(t, served, "a list-type+ownershipControls request must not reach a handler")
|
||||
assert.Equal(t, http.StatusBadRequest, rec.Code)
|
||||
|
||||
served = false
|
||||
req, _ = http.NewRequest("GET", "http://localhost/bucket?list-type=2&prefix=a", nil)
|
||||
rec = httptest.NewRecorder()
|
||||
handler.ServeHTTP(rec, req)
|
||||
assert.True(t, served, "a plain list-type request must still be served")
|
||||
}
|
||||
|
||||
@@ -49,9 +49,9 @@ func hasPathSegmentQuery(rawQuery string) bool {
|
||||
var operationSubresources = map[string]bool{
|
||||
"accelerate": true, "acl": true, "analytics": true, "attributes": true,
|
||||
"cors": true, "delete": true, "encryption": true, "intelligent-tiering": true,
|
||||
"inventory": true, "legal-hold": true, "lifecycle": true, "location": true,
|
||||
"logging": true, "metrics": true, "notification": true, "object-lock": true,
|
||||
"ownershipControls": true, "policy": true, "policyStatus": true,
|
||||
"inventory": true, "legal-hold": true, "lifecycle": true, "list-type": true,
|
||||
"location": true, "logging": true, "metrics": true, "notification": true,
|
||||
"object-lock": true, "ownershipControls": true, "policy": true, "policyStatus": true,
|
||||
"publicAccessBlock": true, "renameObject": true, "replication": true,
|
||||
"requestPayment": true, "retention": true, "tagging": true, "uploadId": true,
|
||||
"uploads": true, "versioning": true, "versions": true, "website": true,
|
||||
|
||||
Reference in New Issue
Block a user