From 084b9be55ad333374c731802469a793ed01e93c3 Mon Sep 17 00:00:00 2001 From: Sung-Kyu Yoo Date: Sat, 5 Sep 2026 20:30:47 +0900 Subject: [PATCH 1/4] test: add reproducer for s3-control routing into the s3 provider MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit RED: TestDetectProtocol_S3Control fails 5/10 subtests with expected "s3control", actual "s3". S3 Control signs with S3's own signing name, so DetectProtocol branch 3's `svc != "s3"` guard sends it to the REST-XML default — the one branch that never consults SigningSiblings — and the S3 provider parses /v20180820/accesspoint/ap as bucket + key. The 5 S3 guard subtests pass, so the failure is the defect and not the test setup. --- internal/gateway/protocol_test.go | 35 +++++++++++++++++++++++++++++++ 1 file changed, 35 insertions(+) diff --git a/internal/gateway/protocol_test.go b/internal/gateway/protocol_test.go index fd905f43..108b94c7 100644 --- a/internal/gateway/protocol_test.go +++ b/internal/gateway/protocol_test.go @@ -18,6 +18,41 @@ func TestDetectProtocol_RESTXML(t *testing.T) { assert.Equal(t, "s3", service) } +// TestDetectProtocol_S3Control covers the one service that shares S3's signing +// name. S3 Control signs as "s3", so branch 3's `svc != "s3"` guard sends it to +// the REST-XML default — the only branch that never consults SigningSiblings. +// Without a split it lands on the S3 provider, which parses +// /v20180820/accesspoint/ap as bucket "v20180820", key "accesspoint/ap" and +// stores it: a 200 for a service that served nothing. +func TestDetectProtocol_S3Control(t *testing.T) { + cases := []struct{ name, method, path, want string }{ + // Every S3 Control operation is under /v20180820/ and no S3 operation is. + {"create_access_point", "PUT", "/v20180820/accesspoint/ap", "s3control"}, + {"list_access_points", "GET", "/v20180820/accesspoint", "s3control"}, + {"delete_access_point", "DELETE", "/v20180820/accesspoint/ap", "s3control"}, + {"get_public_access_block", "GET", "/v20180820/configuration/publicAccessBlock", "s3control"}, + {"create_job", "POST", "/v20180820/jobs", "s3control"}, + // S3 itself must be untouched, including the shapes most likely to be + // caught by a sloppier prefix test. + {"s3_create_bucket", "PUT", "/my-bucket", "s3"}, + {"s3_put_object", "PUT", "/my-bucket/some/key", "s3"}, + {"s3_list_buckets", "GET", "/", "s3"}, + {"s3_bucket_named_like_the_prefix", "PUT", "/v20180820", "s3"}, + {"s3_key_containing_the_prefix", "PUT", "/my-bucket/v20180820/x", "s3"}, + } + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + req := httptest.NewRequest(c.method, c.path, nil) + // S3 Control signs with S3's own name; that is the whole problem. + req.Header.Set("Authorization", + "AWS4-HMAC-SHA256 Credential=AKIA/20130524/us-east-1/s3/aws4_request, Signature=abc") + proto, service := DetectProtocol(req) + assert.Equal(t, "rest-xml", proto) + assert.Equal(t, c.want, service) + }) + } +} + func TestDetectProtocol_JSON10_DynamoDB(t *testing.T) { req := httptest.NewRequest("POST", "/", strings.NewReader(`{}`)) req.Header.Set("X-Amz-Target", "DynamoDB_20120810.PutItem") From 5aaf6235f25929745d55c6df3d25efcb6b84c401 Mon Sep 17 00:00:00 2001 From: Sung-Kyu Yoo Date: Sat, 5 Sep 2026 20:31:57 +0900 Subject: [PATCH 2/4] fix: route S3 Control away from the S3 provider MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit GREEN: TestDetectProtocol_S3Control 10/10 subtests pass; full suite 111 packages ok, 0 failures. DetectProtocol branch 4 splits s3-control off s3 by its /v20180820/ path prefix — the third instance of the URL-path split already used for opensearch and apigateway. A route-table split would be wrong rather than merely larger: S3's own PutObject PUT /{Bucket}/{Key+} matches an S3 Control path, so asking which service models the path answers "s3". --- changes/unreleased/Fixed-20260905-200000.yaml | 5 +++++ internal/gateway/protocol.go | 19 ++++++++++++++++++- 2 files changed, 23 insertions(+), 1 deletion(-) create mode 100644 changes/unreleased/Fixed-20260905-200000.yaml diff --git a/changes/unreleased/Fixed-20260905-200000.yaml b/changes/unreleased/Fixed-20260905-200000.yaml new file mode 100644 index 00000000..e0fc7dbb --- /dev/null +++ b/changes/unreleased/Fixed-20260905-200000.yaml @@ -0,0 +1,5 @@ +kind: Fixed +body: 'S3 Control requests were served by S3. `s3control` signs with S3''s own signing name, so every call fell through to the REST-XML default and the S3 provider parsed it as a bucket and key — `CreateAccessPoint` returned 200 and left an object in a bucket named `v20180820`. S3 Control is now split off by its `/v20180820/` path prefix, and its unserved operations return a clean AWS error instead of a fabricated success' +time: 2026-09-05T20:00:00.000000+09:00 +custom: + Issue: "141" diff --git a/internal/gateway/protocol.go b/internal/gateway/protocol.go index 540429a2..3336f788 100644 --- a/internal/gateway/protocol.go +++ b/internal/gateway/protocol.go @@ -78,7 +78,24 @@ func DetectProtocol(r *http.Request) (protocol string, serviceID string) { }) } - // 4. Default: REST-XML / S3 + // 4. Default: REST-XML / S3. + // + // S3 Control signs with S3's own name ("s3"), so branch 3's `svc != "s3"` + // guard sends it here — and this is the one branch that never consults + // SigningSiblings, even though aliases.SigningSiblings["s3"] already lists + // both. Every S3 Control operation is under /v20180820/ and no S3 operation + // is, so the path separates them, exactly as it does for opensearch and + // apigateway above. + // + // A route-table split would be wrong here rather than merely bigger: S3's + // own `PutObject PUT /{Bucket}/{Key+}` matches /v20180820/accesspoint/ap, + // so asking which service models the path answers "s3". That is also what + // the bug was — the S3 provider stored the request as bucket "v20180820", + // key "accesspoint/ap" and returned 200, fabricating a success for a + // service that served nothing. + if strings.HasPrefix(r.URL.Path, "/v20180820/") { + return "rest-xml", "s3control" + } return "rest-xml", "s3" } From 5d6ad6f8c6e4ac3bec4d3002ea0463c419b8dff4 Mon Sep 17 00:00:00 2001 From: Sung-Kyu Yoo Date: Sat, 5 Sep 2026 20:37:49 +0900 Subject: [PATCH 3/4] test: pin S3 Control routing end to end, and the shadowing it costs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Compat RED (fix reverted): create_access_point DID NOT RAISE — it returned 200 and stored an object. GREEN with the fix: 4 passed. Full compat suite 846 passed, 3 skipped; go test ./... all green. The guard test caught a real gap in the fix's blast radius: a bucket named exactly "v20180820" produces object paths under /v20180820/, which the prefix claims. That is now asserted rather than left to be found, in both the Go table and the compat suite, with the reason the alternative is worse: x-amz-account-id is bound by 96 of S3 Control's 97 operations and none of S3's 107, but gating on it sends Outposts CreateBucket to S3 as a stored object and a 200. A wrong-but-honest error beats a fabricated success. --- internal/gateway/protocol_test.go | 12 +++ tests/compatibility/test_s3control.py | 113 ++++++++++++++++++++++++++ 2 files changed, 125 insertions(+) create mode 100644 tests/compatibility/test_s3control.py diff --git a/internal/gateway/protocol_test.go b/internal/gateway/protocol_test.go index 108b94c7..f74b8255 100644 --- a/internal/gateway/protocol_test.go +++ b/internal/gateway/protocol_test.go @@ -39,6 +39,18 @@ func TestDetectProtocol_S3Control(t *testing.T) { {"s3_list_buckets", "GET", "/", "s3"}, {"s3_bucket_named_like_the_prefix", "PUT", "/v20180820", "s3"}, {"s3_key_containing_the_prefix", "PUT", "/my-bucket/v20180820/x", "s3"}, + // The one case the split gets wrong, asserted rather than left to be + // found. A bucket named exactly "v20180820" makes object paths that are + // indistinguishable from S3 Control's, and the prefix wins. + // + // The alternative is worse. x-amz-account-id is bound by 96 of S3 + // Control's 97 operations and by none of S3's 107, so gating on it would + // separate these — but it would send Outposts + // `CreateBucket PUT /v20180820/bucket/{Bucket}`, the one operation that + // does not bind it, to S3, where it becomes a stored object and a 200. + // Shadowing costs a clean error; header-gating costs a fabricated + // success, and only one of those is a guarantee this project makes. + {"object_in_a_bucket_named_like_the_prefix_is_shadowed", "PUT", "/v20180820/some-key", "s3control"}, } for _, c := range cases { t.Run(c.name, func(t *testing.T) { diff --git a/tests/compatibility/test_s3control.py b/tests/compatibility/test_s3control.py new file mode 100644 index 00000000..026ee459 --- /dev/null +++ b/tests/compatibility/test_s3control.py @@ -0,0 +1,113 @@ +"""S3 Control routing. + +S3 Control signs with S3's own signing name, so before the /v20180820/ split it +fell through DetectProtocol to the REST-XML default and the S3 provider parsed +every request as a bucket and a key. CreateAccessPoint returned 200 and left an +object behind — a fabricated success for a service that served nothing, which is +the one guarantee docs/coverage.md calls absolute. + +These tests pin the routing, not the depth. They are written to survive the +engine learning rest-xml: none of them asserts that s3control fails, only that +whatever answers is s3control rather than S3. +""" + +import pytest +from botocore.exceptions import ClientError + +from conftest import _make_client + +ACCOUNT_ID = "000000000000" + +# Error codes only the S3 provider produces, by parsing a path as a bucket and +# a key. Seeing one of these from an S3 Control call means the request was +# misrouted, whatever the status code says. +S3_ERROR_CODES = {"NoSuchKey", "NoSuchBucket", "MethodNotAllowed", "InvalidRequest"} + + +@pytest.fixture +def s3control_client(devcloud_server): + return _make_client("s3control") + + +def _error_code(call, **kwargs): + """Run an operation and return its AWS error code, or None if it succeeded.""" + try: + call(**kwargs) + return None + except ClientError as exc: + return exc.response["Error"]["Code"] + + +def test_s3control_call_does_not_become_an_s3_object(s3control_client, s3_client): + """The defect itself: a CreateAccessPoint must not create S3 state. + + Whether it succeeds or declines is a coverage question that Milestone 5's + engine work answers. Whether it silently becomes an object is a correctness + question, and the answer must always be no. + """ + before = {b["Name"] for b in s3_client.list_buckets()["Buckets"]} + + _error_code( + s3control_client.create_access_point, + AccountId=ACCOUNT_ID, + Name="my-access-point", + Bucket="some-bucket", + ) + + after = {b["Name"] for b in s3_client.list_buckets()["Buckets"]} + assert after == before, ( + "an S3 Control request created S3 state; it was routed to the S3 provider" + ) + assert "v20180820" not in after, "the S3 Control URI prefix was stored as a bucket" + + +def test_s3control_answers_as_itself_not_as_s3(s3control_client): + """Whatever s3control returns, it must not be S3's answer about a path S3 + invented. Before the split this was NoSuchKey for a bucket "v20180820".""" + code = _error_code(s3control_client.list_access_points, AccountId=ACCOUNT_ID) + assert code not in S3_ERROR_CODES, ( + f"got S3's {code} — the request reached the S3 provider, not s3control" + ) + + +def test_s3_is_unaffected_by_the_split(s3_client): + """The split must cost S3 nothing for any normal bucket or key.""" + s3_client.create_bucket(Bucket="split-guard-bucket") + names = {b["Name"] for b in s3_client.list_buckets()["Buckets"]} + assert "split-guard-bucket" in names + + # A key that merely contains the prefix is still S3's: only a path that + # *starts* with /v20180820/ is claimed. + s3_client.put_object(Bucket="split-guard-bucket", Key="v20180820/nested", Body=b"x") + got = s3_client.get_object(Bucket="split-guard-bucket", Key="v20180820/nested") + assert got["Body"].read() == b"x" + + +def test_bucket_named_like_the_prefix_is_shadowed_cleanly(s3_client): + """The one case the split gets wrong, asserted rather than left to be found. + + A bucket named exactly "v20180820" produces object paths under + /v20180820/..., which is indistinguishable from an S3 Control request. The + prefix wins, so those objects are unreachable. + + This is the deliberate trade. The alternative — gating on the + x-amz-account-id header, which 96 of S3 Control's 97 operations send and no + S3 operation does — would route Outposts `CreateBucket PUT + /v20180820/bucket/{Bucket}` to S3, where it becomes a stored object and a + 200. A wrong-but-honest error beats a fabricated success, so the shadowing + stands and is tested. + """ + # Creating it is fine: PUT /v20180820 has no trailing slash, so it is S3's. + s3_client.create_bucket(Bucket="v20180820") + assert "v20180820" in {b["Name"] for b in s3_client.list_buckets()["Buckets"]} + + # Putting an object into it is not: the path becomes /v20180820/. + code = _error_code( + s3_client.put_object, Bucket="v20180820", Key="shadowed", Body=b"x" + ) + assert code is not None, ( + "an object write into the shadowed bucket must not silently succeed" + ) + assert code not in S3_ERROR_CODES or code == "InvalidRequest", ( + f"expected s3control's clean refusal, got {code}" + ) From 61822c32e92e16f21acf893d739c2c57bf35df53 Mon Sep 17 00:00:00 2001 From: Sung-Kyu Yoo Date: Sun, 6 Sep 2026 02:00:46 +0900 Subject: [PATCH 4/4] chore: point the changelog fragment at the real PR number --- changes/unreleased/Fixed-20260905-200000.yaml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/changes/unreleased/Fixed-20260905-200000.yaml b/changes/unreleased/Fixed-20260905-200000.yaml index e0fc7dbb..6dadc389 100644 --- a/changes/unreleased/Fixed-20260905-200000.yaml +++ b/changes/unreleased/Fixed-20260905-200000.yaml @@ -2,4 +2,4 @@ kind: Fixed body: 'S3 Control requests were served by S3. `s3control` signs with S3''s own signing name, so every call fell through to the REST-XML default and the S3 provider parsed it as a bucket and key — `CreateAccessPoint` returned 200 and left an object in a bucket named `v20180820`. S3 Control is now split off by its `/v20180820/` path prefix, and its unserved operations return a clean AWS error instead of a fabricated success' time: 2026-09-05T20:00:00.000000+09:00 custom: - Issue: "141" + Issue: "142"