From f72983c1fd191c8db70c4cf604c7e484043ac080 Mon Sep 17 00:00:00 2001 From: Chris Lu Date: Tue, 19 May 2026 14:24:25 -0700 Subject: [PATCH] fix(s3): stop S3 Tables routes from swallowing buckets named "buckets" or "get-table" (#9566) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(s3): stop S3 Tables routes from swallowing buckets named "buckets" or "get-table" The S3 Tables REST endpoints share top-level paths with the regular S3 API (/buckets for ListTableBuckets/CreateTableBucket, /get-table for GetTable). They are registered first on the same router as the bucket subrouter, so a path-style request such as GET /buckets?list-type=2 on a bucket actually named "buckets" matched ListTableBuckets and returned JSON. AWS SDK V2 (and Hadoop s3a / Spark) then failed XML parsing with "Unexpected character '{' (code 123) in prolog". Disambiguate by requiring the AWS V4 credential scope to name the s3tables service on the colliding routes. Regular S3 SDKs sign with service=s3, S3 Tables SDKs sign with service=s3tables, and the scope is present in both the Authorization header and the X-Amz-Credential query parameter for presigned URLs, so the matcher works for both flavors. ARN-bearing S3 Tables routes (/buckets/, /namespaces/, etc.) already cannot collide because colons are not valid in bucket names, so they are left untouched. * fix(s3): accept AWS JSON RPC content type as S3 Tables intent signal The Iceberg catalog integration tests send unsigned PUT /buckets with Content-Type: application/x-amz-json-1.1 to create table buckets. With only the credential-scope check, those requests fell through to the regular S3 CreateBucket handler and the suite went red on this branch. Extend the matcher so a request is recognized as S3 Tables when either: - its AWS V4 credential scope names SERVICE=s3tables; or - it carries the canonical AWS JSON RPC 1.1 content type and is unsigned (a request explicitly signed for SERVICE=s3 still wins). The regular S3 SDKs do not send application/x-amz-json-1.1, so the signal is safe for the colliding paths (/buckets, /get-table). Also add an AWS SDK V2 for Go integration test under test/s3/sdk_v2_routing/ that drives the SDK's own XML deserializer against a bucket literally named "buckets" and "get-table" — the SDK errors before the test asserts if the server returns the wrong body shape. Wired up via .github/workflows/s3-sdk-v2-routing-tests.yml, mirroring the etag/acl workflow. * s3api: extend service matcher to all S3 Tables routes; simplify scope check - Apply serviceMatcher to every S3 Tables route, not just the bare-path ones. ARN-bearing paths could otherwise be hit by an S3 object key that starts with arn:aws:s3tables:..., inside a bucket named "buckets", "namespaces", "tables", or "tag". One matcher everywhere closes both collision classes. - Replace strings.Split + index lookup with strings.Contains for the credential-scope check. The scope shape is fixed at AK/DATE/REGION/SERVICE/aws4_request, slashes only delimit components, and access keys are alphanumeric — so /s3tables/ matches iff SERVICE is exactly s3tables. Existing unit cases (including the access-key-substring case) still pass. - Read the GetObject body in the SDK v2 routing test with io.ReadAll; the single Read could return short and make the equality check flaky. * s3api: drop content-type fallback; sign s3 tables harness traffic instead The content-type fallback in isS3TablesSignedRequest let an anonymous regular-S3 request whose body type is application/x-amz-json-1.1 hit an S3 Tables route when the path-style object key happened to be shaped like an S3 Tables ARN (e.g. PutObject on bucket "buckets" with key arn:aws:s3tables:.../bucket/foo/policy). Narrow the matcher back to the AWS V4 credential scope so only requests signed for SERVICE=s3tables match the S3 Tables routes. Update the Iceberg catalog test harness — the only caller still sending unsigned PUT /buckets — to sign with SERVICE=s3tables. The mini instance runs in default-allow mode, so the signature itself is not verified; only the credential scope matters for the route match. Drop the stale unit cases for the JSON-RPC content-type signal and the routing test that exercised unsigned harness traffic. --- .github/workflows/s3-sdk-v2-routing-tests.yml | 110 +++++++++ test/s3/sdk_v2_routing/Makefile | 41 ++++ .../sdk_v2_routing/s3_sdk_v2_routing_test.go | 210 ++++++++++++++++++ test/s3tables/catalog/iceberg_catalog_test.go | 26 ++- weed/s3api/s3api_tables.go | 95 ++++++-- weed/s3api/s3api_tables_routing_test.go | 158 +++++++++++++ 6 files changed, 616 insertions(+), 24 deletions(-) create mode 100644 .github/workflows/s3-sdk-v2-routing-tests.yml create mode 100644 test/s3/sdk_v2_routing/Makefile create mode 100644 test/s3/sdk_v2_routing/s3_sdk_v2_routing_test.go create mode 100644 weed/s3api/s3api_tables_routing_test.go diff --git a/.github/workflows/s3-sdk-v2-routing-tests.yml b/.github/workflows/s3-sdk-v2-routing-tests.yml new file mode 100644 index 000000000..ec73723d6 --- /dev/null +++ b/.github/workflows/s3-sdk-v2-routing-tests.yml @@ -0,0 +1,110 @@ +name: "S3 SDK V2 Route Disambiguation Tests" + +on: + push: + branches: [ master ] + paths: + - 'weed/s3api/**' + - 'test/s3/sdk_v2_routing/**' + - '.github/workflows/s3-sdk-v2-routing-tests.yml' + pull_request: + branches: [ master ] + paths: + - 'weed/s3api/**' + - 'test/s3/sdk_v2_routing/**' + - '.github/workflows/s3-sdk-v2-routing-tests.yml' + +concurrency: + group: ${{ github.head_ref || github.ref }}/s3-sdk-v2-routing-tests + cancel-in-progress: true + +permissions: + contents: read + +jobs: + s3-sdk-v2-routing-tests: + name: S3 SDK V2 Routing Tests + runs-on: ubuntu-22.04 + timeout-minutes: 10 + steps: + - name: Check out code + uses: actions/checkout@v6 + + - name: Set up Go + uses: actions/setup-go@v6 + with: + go-version-file: 'go.mod' + + - name: Install SeaweedFS + run: | + cd weed && go install -buildvcs=false + + - name: Start weed mini (S3 on :8333) + # Pins the regression for issue #9559: AWS SDK V2 / Hadoop s3a + # listing a bucket literally named "buckets" must get an XML + # ListObjectsV2 response, not the JSON ListTableBuckets body + # served by the S3 Tables REST endpoint on the same path. + run: | + mkdir -p /tmp/seaweedfs-sdk-v2-routing + cat > /tmp/seaweedfs-sdk-v2-routing-s3.json <<'JSON' + { + "identities": [ + { + "name": "admin", + "credentials": [ + {"accessKey": "some_access_key1", "secretKey": "some_secret_key1"} + ], + "actions": ["Admin", "Read", "Write"] + } + ] + } + JSON + AWS_ACCESS_KEY_ID=some_access_key1 \ + AWS_SECRET_ACCESS_KEY=some_secret_key1 \ + weed mini \ + -dir=/tmp/seaweedfs-sdk-v2-routing \ + -s3.port=8333 \ + -s3.config=/tmp/seaweedfs-sdk-v2-routing-s3.json \ + -ip=127.0.0.1 \ + > /tmp/weed-mini.log 2>&1 & + echo $! > /tmp/weed-mini.pid + + for i in $(seq 1 30); do + if curl -s -o /dev/null -w "%{http_code}" http://127.0.0.1:8333/ | grep -qE "^(200|403)$"; then + echo "weed mini is ready" + exit 0 + fi + sleep 1 + done + echo "weed mini failed to start within 30s" + tail -50 /tmp/weed-mini.log + exit 1 + + - name: Run SDK V2 routing tests + env: + S3_ENDPOINT: http://127.0.0.1:8333 + AWS_ACCESS_KEY_ID: some_access_key1 + AWS_SECRET_ACCESS_KEY: some_secret_key1 + AWS_REGION: us-east-1 + run: go test -v -timeout=5m ./test/s3/sdk_v2_routing/... + + - name: Stop weed mini + if: always() + run: | + if [ -f /tmp/weed-mini.pid ]; then + kill "$(cat /tmp/weed-mini.pid)" 2>/dev/null || true + fi + + - name: Show server log on failure + if: failure() + run: | + echo "=== weed mini log (last 200 lines) ===" + tail -n 200 /tmp/weed-mini.log 2>/dev/null || echo "no log available" + + - name: Archive log + if: failure() + uses: actions/upload-artifact@v7 + with: + name: s3-sdk-v2-routing-server-log + path: /tmp/weed-mini.log + retention-days: 3 diff --git a/test/s3/sdk_v2_routing/Makefile b/test/s3/sdk_v2_routing/Makefile new file mode 100644 index 000000000..67095e0a5 --- /dev/null +++ b/test/s3/sdk_v2_routing/Makefile @@ -0,0 +1,41 @@ +# AWS SDK V2 Route Disambiguation Integration Tests +# +# Pins the regression for the route collision between the regular S3 API +# and the S3 Tables REST API on shared top-level paths (/buckets, +# /get-table). The tests use the real AWS SDK V2 for Go so the SDK's own +# XML deserializer is the assertion — a JSON body produces an SDK error +# before any test code runs. +# +# Prerequisites: +# - SeaweedFS running with S3 API enabled on port 8333 +# - Go 1.21+ +# +# Usage: +# make test - Run the SDK V2 routing tests +# make test-verbose - Run with verbose output +# make clean - Clean test cache + +.PHONY: all test test-verbose clean help + +S3_ENDPOINT ?= http://127.0.0.1:8333 + +all: test + +test: + @echo "Running SDK V2 routing tests against $(S3_ENDPOINT)..." + S3_ENDPOINT=$(S3_ENDPOINT) go test -v -timeout 5m ./... + +test-verbose: + S3_ENDPOINT=$(S3_ENDPOINT) go test -v -timeout 5m -count=1 ./... + +clean: + go clean -testcache + +help: + @echo "AWS SDK V2 Route Disambiguation Tests" + @echo "Targets:" + @echo " test Run the routing tests" + @echo " test-verbose Run with verbose output" + @echo " clean Clean test cache" + @echo "Environment Variables:" + @echo " S3_ENDPOINT S3 endpoint URL (default: http://127.0.0.1:8333)" diff --git a/test/s3/sdk_v2_routing/s3_sdk_v2_routing_test.go b/test/s3/sdk_v2_routing/s3_sdk_v2_routing_test.go new file mode 100644 index 000000000..2345fdf57 --- /dev/null +++ b/test/s3/sdk_v2_routing/s3_sdk_v2_routing_test.go @@ -0,0 +1,210 @@ +// Package sdkv2routing_test exercises route disambiguation between the +// regular S3 API and the S3 Tables REST API on top-level paths the two +// share (/buckets, /get-table). The bug it pins: +// +// When a user has an S3 bucket named "buckets" (or "get-table"), a +// path-style ListObjectsV2 request sent by AWS SDK V2 / Hadoop s3a / +// Spark would be routed to the S3 Tables ListTableBuckets handler and +// receive a JSON body. AWS SDK V2 then fails XML parsing with +// "Unexpected character '{' (code 123) in prolog". +// +// These tests use the real AWS SDK V2 for Go, so the SDK's own +// deserializer is the assertion: if the server returns the wrong +// content type, the SDK errors out before any test assertion runs. +package sdkv2routing_test + +import ( + "context" + "io" + "os" + "strings" + "testing" + "time" + + "github.com/aws/aws-sdk-go-v2/aws" + "github.com/aws/aws-sdk-go-v2/config" + "github.com/aws/aws-sdk-go-v2/credentials" + "github.com/aws/aws-sdk-go-v2/service/s3" + "github.com/stretchr/testify/require" +) + +const ( + defaultEndpoint = "http://127.0.0.1:8333" + defaultAccessKey = "some_access_key1" + defaultSecretKey = "some_secret_key1" + defaultRegion = "us-east-1" +) + +func getS3Client(t *testing.T) *s3.Client { + t.Helper() + + endpoint := os.Getenv("S3_ENDPOINT") + if endpoint == "" { + endpoint = defaultEndpoint + } + accessKey := os.Getenv("AWS_ACCESS_KEY_ID") + if accessKey == "" { + accessKey = defaultAccessKey + } + secretKey := os.Getenv("AWS_SECRET_ACCESS_KEY") + if secretKey == "" { + secretKey = defaultSecretKey + } + region := os.Getenv("AWS_REGION") + if region == "" { + region = defaultRegion + } + + cfg, err := config.LoadDefaultConfig(context.TODO(), + config.WithRegion(region), + config.WithCredentialsProvider(credentials.NewStaticCredentialsProvider(accessKey, secretKey, "")), + config.WithEndpointResolverWithOptions(aws.EndpointResolverWithOptionsFunc( + func(service, region string, options ...interface{}) (aws.Endpoint, error) { + return aws.Endpoint{ + URL: endpoint, + SigningRegion: defaultRegion, + HostnameImmutable: true, + }, nil + })), + ) + require.NoError(t, err) + + return s3.NewFromConfig(cfg, func(o *s3.Options) { + o.UsePathStyle = true + }) +} + +// ensureBucket creates bucket if it doesn't already exist. It tolerates +// BucketAlreadyOwnedByYou / BucketAlreadyExists so the tests are +// idempotent across local re-runs. +func ensureBucket(t *testing.T, ctx context.Context, client *s3.Client, bucket string) { + t.Helper() + _, err := client.CreateBucket(ctx, &s3.CreateBucketInput{Bucket: aws.String(bucket)}) + if err == nil { + return + } + msg := err.Error() + if strings.Contains(msg, "BucketAlreadyOwnedByYou") || strings.Contains(msg, "BucketAlreadyExists") { + return + } + t.Fatalf("CreateBucket(%q) failed: %v", bucket, err) +} + +// deleteBucket best-effort cleans up a bucket and any objects in it. +// Test does not fail if cleanup fails — the next run is idempotent. +func deleteBucket(ctx context.Context, client *s3.Client, bucket string) { + paginator := s3.NewListObjectsV2Paginator(client, &s3.ListObjectsV2Input{Bucket: aws.String(bucket)}) + for paginator.HasMorePages() { + page, err := paginator.NextPage(ctx) + if err != nil { + break + } + for _, obj := range page.Contents { + client.DeleteObject(ctx, &s3.DeleteObjectInput{Bucket: aws.String(bucket), Key: obj.Key}) + } + } + client.DeleteBucket(ctx, &s3.DeleteBucketInput{Bucket: aws.String(bucket)}) +} + +// TestListObjectsV2_OnBucketNamedBuckets is the direct reproducer for +// issue #9559: Spark / Hadoop s3a does a ListObjectsV2 against bucket +// "buckets" via AWS SDK V2, which fails with +// "Could not parse XML response. ... Unexpected character '{' (code 123) +// in prolog" when SeaweedFS routes the request to the JSON-returning +// ListTableBuckets handler. The SDK's response deserializer is the +// real assertion here — a JSON body produces an SDK error before we +// reach require.NoError. +func TestListObjectsV2_OnBucketNamedBuckets(t *testing.T) { + ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) + defer cancel() + client := getS3Client(t) + + const bucket = "buckets" + ensureBucket(t, ctx, client, bucket) + t.Cleanup(func() { deleteBucket(ctx, client, bucket) }) + + out, err := client.ListObjectsV2(ctx, &s3.ListObjectsV2Input{ + Bucket: aws.String(bucket), + Prefix: aws.String("logs/"), + }) + require.NoError(t, err, "AWS SDK V2 must parse the response as XML") + require.NotNil(t, out) +} + +// TestPutGetObject_OnBucketNamedBuckets exercises the full read/write +// round-trip on the colliding bucket name. PutObject and GetObject go +// through different routes than ListObjectsV2, and verifying them +// guards against future regressions that re-route only some verbs. +func TestPutGetObject_OnBucketNamedBuckets(t *testing.T) { + ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) + defer cancel() + client := getS3Client(t) + + const bucket = "buckets" + const key = "logs/hello.txt" + const body = "hello from issue 9559" + + ensureBucket(t, ctx, client, bucket) + t.Cleanup(func() { deleteBucket(ctx, client, bucket) }) + + _, err := client.PutObject(ctx, &s3.PutObjectInput{ + Bucket: aws.String(bucket), + Key: aws.String(key), + Body: strings.NewReader(body), + }) + require.NoError(t, err) + + out, err := client.GetObject(ctx, &s3.GetObjectInput{Bucket: aws.String(bucket), Key: aws.String(key)}) + require.NoError(t, err) + defer out.Body.Close() + + got, err := io.ReadAll(out.Body) + require.NoError(t, err) + require.Equal(t, body, string(got)) +} + +// TestListObjectsV2_OnBucketNamedGetTable covers the second colliding +// path. The S3 Tables GET /get-table endpoint shares its path with a +// bucket literally named "get-table", which is a legal S3 bucket name. +func TestListObjectsV2_OnBucketNamedGetTable(t *testing.T) { + ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) + defer cancel() + client := getS3Client(t) + + const bucket = "get-table" + ensureBucket(t, ctx, client, bucket) + t.Cleanup(func() { deleteBucket(ctx, client, bucket) }) + + out, err := client.ListObjectsV2(ctx, &s3.ListObjectsV2Input{Bucket: aws.String(bucket)}) + require.NoError(t, err) + require.NotNil(t, out) +} + +// TestCreateAndListBuckets_ServiceLevel verifies the SDK's service-level +// ListBuckets still parses as XML when a bucket named "buckets" exists. +// ListBuckets goes through GET / (root) which is unaffected by the +// /buckets route collision — this is a guard against the matcher +// accidentally widening to top-level paths. +func TestCreateAndListBuckets_ServiceLevel(t *testing.T) { + ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) + defer cancel() + client := getS3Client(t) + + const bucket = "buckets" + ensureBucket(t, ctx, client, bucket) + t.Cleanup(func() { deleteBucket(ctx, client, bucket) }) + + out, err := client.ListBuckets(ctx, &s3.ListBucketsInput{}) + require.NoError(t, err) + require.NotNil(t, out) + + found := false + for _, b := range out.Buckets { + if aws.ToString(b.Name) == bucket { + found = true + break + } + } + require.True(t, found, "bucket %q must appear in service-level ListBuckets", bucket) +} + diff --git a/test/s3tables/catalog/iceberg_catalog_test.go b/test/s3tables/catalog/iceberg_catalog_test.go index 3930d292c..50d187de7 100644 --- a/test/s3tables/catalog/iceberg_catalog_test.go +++ b/test/s3tables/catalog/iceberg_catalog_test.go @@ -17,6 +17,9 @@ import ( "testing" "time" + "github.com/aws/aws-sdk-go/aws/credentials" + v4 "github.com/aws/aws-sdk-go/aws/signer/v4" + "github.com/seaweedfs/seaweedfs/test/testutil" ) @@ -451,19 +454,22 @@ func icebergPath(prefix, path string) string { return withPrefix } -// createTableBucket creates a table bucket via the S3Tables REST API +// createTableBucket creates a table bucket via the S3Tables REST API. +// The request is AWS V4 signed for SERVICE=s3tables so the S3 Tables +// route matcher accepts it; signing with regular SERVICE=s3 would let +// the request fall through to the S3 CreateBucket handler. func createTableBucket(t *testing.T, env *TestEnvironment, bucketName string) { t.Helper() - // Use S3Tables REST API to create the bucket endpoint := fmt.Sprintf("http://localhost:%d/buckets", env.s3Port) - reqBody := fmt.Sprintf(`{"name":"%s"}`, bucketName) + req, err := http.NewRequest(http.MethodPut, endpoint, strings.NewReader(reqBody)) if err != nil { t.Fatalf("Failed to create request: %v", err) } req.Header.Set("Content-Type", "application/x-amz-json-1.1") + signS3TablesRequest(t, req, reqBody) resp, err := http.DefaultClient.Do(req) if err != nil { @@ -480,6 +486,20 @@ func createTableBucket(t *testing.T, env *TestEnvironment, bucketName string) { t.Logf("Created table bucket %s", bucketName) } +// signS3TablesRequest signs req with AWS V4 for SERVICE=s3tables. The +// underlying weed mini instance runs in default-allow mode so the +// signature itself is not verified; only the credential scope matters, +// because the S3 Tables route matcher requires SERVICE=s3tables to +// distinguish S3 Tables traffic from regular S3 calls on the same paths. +func signS3TablesRequest(t *testing.T, req *http.Request, body string) { + t.Helper() + creds := credentials.NewStaticCredentials("test-ak", "test-sk", "") + signer := v4.NewSigner(creds) + if _, err := signer.Sign(req, strings.NewReader(body), "s3tables", "us-east-1", time.Now()); err != nil { + t.Fatalf("Failed to sign S3 Tables request: %v", err) + } +} + // randomSuffix returns a short random hex suffix for unique resource naming. func randomSuffix() string { return fmt.Sprintf("%x", time.Now().UnixNano()&0xffffffff) diff --git a/weed/s3api/s3api_tables.go b/weed/s3api/s3api_tables.go index b27943334..6ea2732f2 100644 --- a/weed/s3api/s3api_tables.go +++ b/weed/s3api/s3api_tables.go @@ -84,54 +84,64 @@ func (s3a *S3ApiServer) registerS3TablesRoutes(router *mux.Router) { targetMatcher := func(r *http.Request, rm *mux.RouteMatch) bool { return strings.HasPrefix(r.Header.Get("X-Amz-Target"), "S3Tables.") } + // serviceMatcher gates every S3 Tables route so it only matches when the + // request is genuinely targeting the s3tables service. The bare paths + // (/buckets, /get-table) collide with regular S3 buckets of the same + // name; the ARN-bearing paths (/buckets/, /namespaces/, ...) + // could collide with object keys that look like S3 Tables ARNs inside a + // bucket named "buckets", "namespaces", "tables", or "tag". A single + // matcher applied to every route closes both classes of collision. + serviceMatcher := func(r *http.Request, rm *mux.RouteMatch) bool { + return isS3TablesSignedRequest(r) + } router.Methods(http.MethodPost).Path("/").MatcherFunc(targetMatcher). HandlerFunc(track(s3a.authenticateS3Tables(s3TablesApi.S3TablesHandler), "S3Tables-Target")) - router.Methods(http.MethodPut).Path("/buckets"). + router.Methods(http.MethodPut).Path("/buckets").MatcherFunc(serviceMatcher). HandlerFunc(track(s3a.authenticateS3Tables(s3TablesApi.handleRestOperation("CreateTableBucket", buildCreateTableBucketRequest)), "S3Tables-CreateTableBucket")) - router.Methods(http.MethodGet).Path("/buckets"). + router.Methods(http.MethodGet).Path("/buckets").MatcherFunc(serviceMatcher). HandlerFunc(track(s3a.authenticateS3Tables(s3TablesApi.handleRestOperation("ListTableBuckets", buildListTableBucketsRequest)), "S3Tables-ListTableBuckets")) - router.Methods(http.MethodGet).Path("/buckets/{tableBucketARN:" + tableBucketARNRegex + "}"). + router.Methods(http.MethodGet).Path("/buckets/{tableBucketARN:" + tableBucketARNRegex + "}").MatcherFunc(serviceMatcher). HandlerFunc(track(s3a.authenticateS3Tables(s3TablesApi.handleRestOperation("GetTableBucket", buildTableBucketArnRequest)), "S3Tables-GetTableBucket")) - router.Methods(http.MethodDelete).Path("/buckets/{tableBucketARN:" + tableBucketARNRegex + "}"). + router.Methods(http.MethodDelete).Path("/buckets/{tableBucketARN:" + tableBucketARNRegex + "}").MatcherFunc(serviceMatcher). HandlerFunc(track(s3a.authenticateS3Tables(s3TablesApi.handleRestOperation("DeleteTableBucket", buildDeleteTableBucketRequest)), "S3Tables-DeleteTableBucket")) - router.Methods(http.MethodPut).Path("/buckets/{tableBucketARN:" + tableBucketARNRegex + "}/policy"). + router.Methods(http.MethodPut).Path("/buckets/{tableBucketARN:" + tableBucketARNRegex + "}/policy").MatcherFunc(serviceMatcher). HandlerFunc(track(s3a.authenticateS3Tables(s3TablesApi.handleRestOperation("PutTableBucketPolicy", buildPutTableBucketPolicyRequest)), "S3Tables-PutTableBucketPolicy")) - router.Methods(http.MethodGet).Path("/buckets/{tableBucketARN:" + tableBucketARNRegex + "}/policy"). + router.Methods(http.MethodGet).Path("/buckets/{tableBucketARN:" + tableBucketARNRegex + "}/policy").MatcherFunc(serviceMatcher). HandlerFunc(track(s3a.authenticateS3Tables(s3TablesApi.handleRestOperation("GetTableBucketPolicy", buildGetTableBucketPolicyRequest)), "S3Tables-GetTableBucketPolicy")) - router.Methods(http.MethodDelete).Path("/buckets/{tableBucketARN:" + tableBucketARNRegex + "}/policy"). + router.Methods(http.MethodDelete).Path("/buckets/{tableBucketARN:" + tableBucketARNRegex + "}/policy").MatcherFunc(serviceMatcher). HandlerFunc(track(s3a.authenticateS3Tables(s3TablesApi.handleRestOperation("DeleteTableBucketPolicy", buildDeleteTableBucketPolicyRequest)), "S3Tables-DeleteTableBucketPolicy")) - router.Methods(http.MethodPut).Path("/namespaces/{tableBucketARN:" + tableBucketARNRegex + "}"). + router.Methods(http.MethodPut).Path("/namespaces/{tableBucketARN:" + tableBucketARNRegex + "}").MatcherFunc(serviceMatcher). HandlerFunc(track(s3a.authenticateS3Tables(s3TablesApi.handleRestOperation("CreateNamespace", buildCreateNamespaceRequest)), "S3Tables-CreateNamespace")) - router.Methods(http.MethodGet).Path("/namespaces/{tableBucketARN:" + tableBucketARNRegex + "}"). + router.Methods(http.MethodGet).Path("/namespaces/{tableBucketARN:" + tableBucketARNRegex + "}").MatcherFunc(serviceMatcher). HandlerFunc(track(s3a.authenticateS3Tables(s3TablesApi.handleRestOperation("ListNamespaces", buildListNamespacesRequest)), "S3Tables-ListNamespaces")) - router.Methods(http.MethodGet).Path("/namespaces/{tableBucketARN:" + tableBucketARNRegex + "}/{namespace}"). + router.Methods(http.MethodGet).Path("/namespaces/{tableBucketARN:" + tableBucketARNRegex + "}/{namespace}").MatcherFunc(serviceMatcher). HandlerFunc(track(s3a.authenticateS3Tables(s3TablesApi.handleRestOperation("GetNamespace", buildGetNamespaceRequest)), "S3Tables-GetNamespace")) - router.Methods(http.MethodDelete).Path("/namespaces/{tableBucketARN:" + tableBucketARNRegex + "}/{namespace}"). + router.Methods(http.MethodDelete).Path("/namespaces/{tableBucketARN:" + tableBucketARNRegex + "}/{namespace}").MatcherFunc(serviceMatcher). HandlerFunc(track(s3a.authenticateS3Tables(s3TablesApi.handleRestOperation("DeleteNamespace", buildDeleteNamespaceRequest)), "S3Tables-DeleteNamespace")) - router.Methods(http.MethodPut).Path("/tables/{tableBucketARN:" + tableBucketARNRegex + "}/{namespace}"). + router.Methods(http.MethodPut).Path("/tables/{tableBucketARN:" + tableBucketARNRegex + "}/{namespace}").MatcherFunc(serviceMatcher). HandlerFunc(track(s3a.authenticateS3Tables(s3TablesApi.handleRestOperation("CreateTable", buildCreateTableRequest)), "S3Tables-CreateTable")) - router.Methods(http.MethodGet).Path("/tables/{tableBucketARN:" + tableBucketARNRegex + "}"). + router.Methods(http.MethodGet).Path("/tables/{tableBucketARN:" + tableBucketARNRegex + "}").MatcherFunc(serviceMatcher). HandlerFunc(track(s3a.authenticateS3Tables(s3TablesApi.handleRestOperation("ListTables", buildListTablesRequest)), "S3Tables-ListTables")) - router.Methods(http.MethodDelete).Path("/tables/{tableBucketARN:" + tableBucketARNRegex + "}/{namespace}/{name}"). + router.Methods(http.MethodDelete).Path("/tables/{tableBucketARN:" + tableBucketARNRegex + "}/{namespace}/{name}").MatcherFunc(serviceMatcher). HandlerFunc(track(s3a.authenticateS3Tables(s3TablesApi.handleRestOperation("DeleteTable", buildDeleteTableRequest)), "S3Tables-DeleteTable")) - router.Methods(http.MethodPut).Path("/tables/{tableBucketARN:" + tableBucketARNRegex + "}/{namespace}/{name}/policy"). + router.Methods(http.MethodPut).Path("/tables/{tableBucketARN:" + tableBucketARNRegex + "}/{namespace}/{name}/policy").MatcherFunc(serviceMatcher). HandlerFunc(track(s3a.authenticateS3Tables(s3TablesApi.handleRestOperation("PutTablePolicy", buildPutTablePolicyRequest)), "S3Tables-PutTablePolicy")) - router.Methods(http.MethodGet).Path("/tables/{tableBucketARN:" + tableBucketARNRegex + "}/{namespace}/{name}/policy"). + router.Methods(http.MethodGet).Path("/tables/{tableBucketARN:" + tableBucketARNRegex + "}/{namespace}/{name}/policy").MatcherFunc(serviceMatcher). HandlerFunc(track(s3a.authenticateS3Tables(s3TablesApi.handleRestOperation("GetTablePolicy", buildGetTablePolicyRequest)), "S3Tables-GetTablePolicy")) - router.Methods(http.MethodDelete).Path("/tables/{tableBucketARN:" + tableBucketARNRegex + "}/{namespace}/{name}/policy"). + router.Methods(http.MethodDelete).Path("/tables/{tableBucketARN:" + tableBucketARNRegex + "}/{namespace}/{name}/policy").MatcherFunc(serviceMatcher). HandlerFunc(track(s3a.authenticateS3Tables(s3TablesApi.handleRestOperation("DeleteTablePolicy", buildDeleteTablePolicyRequest)), "S3Tables-DeleteTablePolicy")) - router.Methods(http.MethodPost).Path("/tag/{resourceArn:arn:aws:s3tables:.*}"). + router.Methods(http.MethodPost).Path("/tag/{resourceArn:arn:aws:s3tables:.*}").MatcherFunc(serviceMatcher). HandlerFunc(track(s3a.authenticateS3Tables(s3TablesApi.handleRestOperation("TagResource", buildTagResourceRequest)), "S3Tables-TagResource")) - router.Methods(http.MethodGet).Path("/tag/{resourceArn:arn:aws:s3tables:.*}"). + router.Methods(http.MethodGet).Path("/tag/{resourceArn:arn:aws:s3tables:.*}").MatcherFunc(serviceMatcher). HandlerFunc(track(s3a.authenticateS3Tables(s3TablesApi.handleRestOperation("ListTagsForResource", buildListTagsForResourceRequest)), "S3Tables-ListTagsForResource")) - router.Methods(http.MethodDelete).Path("/tag/{resourceArn:arn:aws:s3tables:.*}"). + router.Methods(http.MethodDelete).Path("/tag/{resourceArn:arn:aws:s3tables:.*}").MatcherFunc(serviceMatcher). HandlerFunc(track(s3a.authenticateS3Tables(s3TablesApi.handleRestOperation("UntagResource", buildUntagResourceRequest)), "S3Tables-UntagResource")) - router.Methods(http.MethodGet).Path("/get-table"). + router.Methods(http.MethodGet).Path("/get-table").MatcherFunc(serviceMatcher). HandlerFunc(track(s3a.authenticateS3Tables(s3TablesApi.handleRestOperation("GetTable", buildGetTableRequest)), "S3Tables-GetTable")) glog.V(1).Infof("S3 Tables API enabled") @@ -623,6 +633,49 @@ func buildUntagResourceRequest(r *http.Request) (interface{}, error) { }, nil } +// isS3TablesSignedRequest reports whether the request is targeting the +// S3 Tables service. The signal is the AWS V4 credential scope, which +// names SERVICE=s3tables for S3 Tables SDKs and SERVICE=s3 for regular +// S3 SDKs. The credential scope appears in the Authorization header +// (Credential=AK/DATE/REGION/SERVICE/aws4_request) for signed requests +// and in the X-Amz-Credential query parameter for presigned requests. +// +// The credential scope is the only acceptable signal: a content-type- +// based fallback would let an anonymous regular-S3 request (e.g. a +// PutObject with body type application/x-amz-json-1.1) sneak through +// to an S3 Tables route whenever the object key is shaped like an +// S3 Tables ARN. Clients that genuinely target S3 Tables — including +// internal test harnesses running against a default-allow server — +// must sign with SERVICE=s3tables. +func isS3TablesSignedRequest(r *http.Request) bool { + scope := extractCredentialScope(r) + // Credential scope is AK/DATE/REGION/SERVICE/aws4_request. Slashes + // do not appear inside any other component (access keys are + // alphanumeric), so /s3tables/ matches iff SERVICE is exactly + // s3tables. + return scope != "" && strings.Contains(scope, "/s3tables/") +} + +// extractCredentialScope returns the raw credential value from either the +// Authorization header or the X-Amz-Credential query parameter, without the +// "Credential=" prefix. Returns the empty string when neither is present. +func extractCredentialScope(r *http.Request) string { + if auth := r.Header.Get("Authorization"); auth != "" { + idx := strings.Index(auth, "Credential=") + if idx >= 0 { + tail := auth[idx+len("Credential="):] + if comma := strings.IndexByte(tail, ','); comma >= 0 { + tail = tail[:comma] + } + return strings.TrimSpace(tail) + } + } + if cred := r.URL.Query().Get("X-Amz-Credential"); cred != "" { + return cred + } + return "" +} + // authenticateS3Tables wraps the handler with IAM authentication using AuthSignatureOnly // This authenticates the request but delegates authorization to the S3 Tables handler // which performs granular permission checks based on the specific operation. diff --git a/weed/s3api/s3api_tables_routing_test.go b/weed/s3api/s3api_tables_routing_test.go new file mode 100644 index 000000000..349ccbe59 --- /dev/null +++ b/weed/s3api/s3api_tables_routing_test.go @@ -0,0 +1,158 @@ +package s3api + +import ( + "net/http" + "net/http/httptest" + "testing" + + "github.com/gorilla/mux" +) + +// TestIsS3TablesSignedRequest covers the credential-scope parser used to +// disambiguate S3 Tables REST requests from regular S3 requests on paths +// the two APIs share (e.g. /buckets, /get-table). +func TestIsS3TablesSignedRequest(t *testing.T) { + cases := []struct { + name string + auth string + cred string + want bool + }{ + { + name: "regular S3 auth header", + auth: "AWS4-HMAC-SHA256 Credential=AKIA/20260101/us-east-1/s3/aws4_request, SignedHeaders=host;x-amz-date, Signature=deadbeef", + want: false, + }, + { + name: "S3 Tables auth header", + auth: "AWS4-HMAC-SHA256 Credential=AKIA/20260101/us-east-1/s3tables/aws4_request, SignedHeaders=host;x-amz-date, Signature=deadbeef", + want: true, + }, + { + name: "S3 Tables presigned query", + cred: "AKIA/20260101/us-east-1/s3tables/aws4_request", + want: true, + }, + { + name: "regular S3 presigned query", + cred: "AKIA/20260101/us-east-1/s3/aws4_request", + want: false, + }, + { + name: "unsigned request", + want: false, + }, + { + name: "malformed credential", + auth: "AWS4-HMAC-SHA256 Credential=AKIA, Signature=zzz", + want: false, + }, + { + name: "service substring in access key must not match", + auth: "AWS4-HMAC-SHA256 Credential=s3tablesAKIA/20260101/us-east-1/s3/aws4_request, Signature=zzz", + want: false, + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + req := httptest.NewRequest(http.MethodGet, "/buckets", nil) + if tc.auth != "" { + req.Header.Set("Authorization", tc.auth) + } + if tc.cred != "" { + q := req.URL.Query() + q.Set("X-Amz-Credential", tc.cred) + req.URL.RawQuery = q.Encode() + } + if got := isS3TablesSignedRequest(req); got != tc.want { + t.Fatalf("isS3TablesSignedRequest=%v, want %v", got, tc.want) + } + }) + } +} + +// TestRouting_ListObjectsV2OnBucketNamedBuckets covers the route collision +// between regular S3 listObjectsV2 on a bucket named "buckets" and the +// S3 Tables ListTableBuckets endpoint, both of which use the path /buckets. +// The credential scope on a regular S3 request is /s3/aws4_request, so the +// S3 Tables route must reject it and let the bucket subrouter take it, +// otherwise AWS SDK V2 / s3a clients receive a JSON body in place of the +// expected XML and fail to parse the response. +func TestRouting_ListObjectsV2OnBucketNamedBuckets(t *testing.T) { + router := mux.NewRouter() + s3a := setupRoutingTestServer(t) + s3a.registerRouter(router) + + req, _ := http.NewRequest(http.MethodGet, "/buckets?list-type=2&prefix=logs%2F", nil) + signRoutingTestRequest(t, req, "", "s3") + + var match mux.RouteMatch + if !router.Match(req, &match) { + t.Fatalf("expected GET /buckets?list-type=2 (signed for s3) to match a route; got no match: %v", match.MatchErr) + } + if tmpl, _ := match.Route.GetPathTemplate(); tmpl == "/buckets" { + t.Fatalf("GET /buckets?list-type=2 signed for service=s3 matched the S3 Tables ListTableBuckets route (Path=%q); it must fall through to the bucket subrouter", tmpl) + } +} + +// TestRouting_S3TablesListTableBucketsStillReachable verifies the matcher +// does not break legitimate S3 Tables traffic: a request signed for the +// s3tables service on GET /buckets must still reach ListTableBuckets. +func TestRouting_S3TablesListTableBucketsStillReachable(t *testing.T) { + router := mux.NewRouter() + s3a := setupRoutingTestServer(t) + s3a.registerRouter(router) + + req, _ := http.NewRequest(http.MethodGet, "/buckets", nil) + signRoutingTestRequest(t, req, "", "s3tables") + + var match mux.RouteMatch + if !router.Match(req, &match) { + t.Fatalf("expected GET /buckets (signed for s3tables) to match a route; got no match: %v", match.MatchErr) + } + tmpl, _ := match.Route.GetPathTemplate() + if tmpl != "/buckets" { + t.Fatalf("GET /buckets signed for service=s3tables matched %q; want the S3 Tables route /buckets", tmpl) + } +} + +// TestRouting_CreateBucketNamedBuckets covers the PUT collision: a PUT +// /buckets signed for s3 is CreateBucket on the regular S3 API, not +// CreateTableBucket. +func TestRouting_CreateBucketNamedBuckets(t *testing.T) { + router := mux.NewRouter() + s3a := setupRoutingTestServer(t) + s3a.registerRouter(router) + + req, _ := http.NewRequest(http.MethodPut, "/buckets", nil) + signRoutingTestRequest(t, req, "", "s3") + + var match mux.RouteMatch + if !router.Match(req, &match) { + t.Fatalf("expected PUT /buckets (signed for s3) to match a route; got no match: %v", match.MatchErr) + } + if tmpl, _ := match.Route.GetPathTemplate(); tmpl == "/buckets" { + t.Fatalf("PUT /buckets signed for service=s3 matched the S3 Tables CreateTableBucket route (Path=%q); it must fall through to the bucket subrouter", tmpl) + } +} + +// TestRouting_GetObjectOnBucketNamedGetTable covers the GET /get-table +// collision: a regular S3 request on a bucket named "get-table" must +// reach the bucket subrouter, not the S3 Tables GetTable handler. +func TestRouting_GetObjectOnBucketNamedGetTable(t *testing.T) { + router := mux.NewRouter() + s3a := setupRoutingTestServer(t) + s3a.registerRouter(router) + + req, _ := http.NewRequest(http.MethodGet, "/get-table?list-type=2", nil) + signRoutingTestRequest(t, req, "", "s3") + + var match mux.RouteMatch + if !router.Match(req, &match) { + t.Fatalf("expected GET /get-table?list-type=2 (signed for s3) to match a route; got no match: %v", match.MatchErr) + } + if tmpl, _ := match.Route.GetPathTemplate(); tmpl == "/get-table" { + t.Fatalf("GET /get-table signed for service=s3 matched the S3 Tables GetTable route (Path=%q); it must fall through to the bucket subrouter", tmpl) + } +}