diff --git a/changes/unreleased/Fixed-20260905-200000.yaml b/changes/unreleased/Fixed-20260905-200000.yaml new file mode 100644 index 00000000..6dadc389 --- /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: "142" 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" } diff --git a/internal/gateway/protocol_test.go b/internal/gateway/protocol_test.go index fd905f43..f74b8255 100644 --- a/internal/gateway/protocol_test.go +++ b/internal/gateway/protocol_test.go @@ -18,6 +18,53 @@ 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"}, + // 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) { + 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") 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}" + )