From 991a09f586835199d4ada20c45271d477ae30645 Mon Sep 17 00:00:00 2001 From: Lucas Vieira Date: Mon, 28 Sep 2026 06:13:40 -0300 Subject: [PATCH 1/2] fix(s3): judge the BucketOwnerEnforced exception against the bucket owner Follow-up to #2565, found by reviewing that PR's own follow-up commit after it had merged. A BucketOwnerEnforced bucket accepts an object write that specifies no ACL, or one that specifies bucket owner full control -- "an equivalent form of this ACL" included, per the model. The check compared the grantee against the CALLER instead of the bucket owner. Those coincide for a bucket the caller created, so the common path was right, and they diverge for a bucket persisted by another account: the loader hydrates every persisted bucket into the configured default account while each keeps its own stored `acl_owner_id`. There: - `x-amz-grant-full-control: id=` was accepted, storing an object ACL that gives the bucket owner NOTHING on a bucket whose whole purpose is that the owner controls every object; - `x-amz-grant-full-control: id=`, which AWS accepts, was refused. `S3Store`-side nothing changes; the resolver now asks the bucket for its `acl_owner_id` and falls back to the caller when the bucket is not in memory, which is the case where the ACL headers are validated before the operation reports the missing bucket. The test asserted the bug. It granted full control to the caller while `seed_bucket` owns the bucket as "owner", so it passed only because of the wrong comparison -- fixing the code would have failed it. It now asserts both directions against the real owner, and a grant to anyone else (the caller included) is `AccessControlListNotSupported`. Also reverts the blank `x-amz-acl` leniency from that same commit. `""` is not a member of `ObjectCannedACL`; `main` answered 400 for it on PutObject and CreateMultipartUpload before #2565, and PutObjectAcl, PutBucketAcl and CreateBucket all still do. Treating it as "no ACL asked for" on the object-write paths alone made one operation disagree with four rather than fixing anything, and the `x-amz-grant-*` precedent cited for it is a different case: an empty value there names no grantee, not an invalid ACL. Its test now pins the agreement instead of the exception. - drops a `!g.is_empty()` guard that could never be false (`resolved_grant_headers` errors on an empty clause set rather than returning one) and read as though it were load-bearing - docs: the "ACL ownership modes" bullet still said BucketOwnerEnforced disables ACLs entirely, which the exception falsified --- crates/fakecloud-s3/src/service/config/mod.rs | 16 +++++ crates/fakecloud-s3/src/service/mod.rs | 32 +++++----- crates/fakecloud-s3/src/service/tests.rs | 62 +++++++++++++------ website/content/docs/services/s3.md | 2 +- 4 files changed, 76 insertions(+), 36 deletions(-) diff --git a/crates/fakecloud-s3/src/service/config/mod.rs b/crates/fakecloud-s3/src/service/config/mod.rs index 368b96e35..10e8614f9 100644 --- a/crates/fakecloud-s3/src/service/config/mod.rs +++ b/crates/fakecloud-s3/src/service/config/mod.rs @@ -392,6 +392,22 @@ impl S3Service { /// disables ACLs entirely — every ACL-mutating call must reject /// with `AccessControlListNotSupported` so callers don't /// silently no-op against a bucket that ignores their grants. + /// The canonical user the bucket's ACL is owned by, which is who "bucket + /// owner full control" means. Usually the caller, but not always: a bucket + /// persisted by one account is hydrated into the configured default account + /// on restart while keeping its own stored `acl_owner_id`. + /// + /// `None` when the bucket is not in memory, which is not an error here -- + /// the ACL headers are validated before the operation looks the bucket up, + /// so the caller falls back to its own id and the missing bucket is reported + /// by the operation itself. + pub(super) fn bucket_acl_owner_id(&self, account_id: &str, bucket: &str) -> Option { + let accts = self.state.read(); + let state = accts.get(account_id)?; + let b = state.buckets.get(bucket)?; + Some(b.acl_owner_id.clone()) + } + pub(super) fn bucket_owner_enforced(&self, account_id: &str, bucket: &str) -> bool { let accts = self.state.read(); let Some(state) = accts.get(account_id) else { diff --git a/crates/fakecloud-s3/src/service/mod.rs b/crates/fakecloud-s3/src/service/mod.rs index 79403b10b..2f81c38e4 100644 --- a/crates/fakecloud-s3/src/service/mod.rs +++ b/crates/fakecloud-s3/src/service/mod.rs @@ -249,15 +249,9 @@ impl S3Service { bucket: &str, headers: &HeaderMap, ) -> Result { - // A present-but-blank value is treated as absent, the same rule - // `has_grant_headers` applies to the `x-amz-grant-*` family and for the - // same reason: it is what a client sends for an unset config field, and - // testing presence alone turned that into a hard 400 on every - // ACL-accepting write. let canned = headers .get("x-amz-acl") .and_then(|v| v.to_str().ok()) - .filter(|s| !s.trim().is_empty()) .map(|s| s.to_string()); if let Some(acl) = canned.as_deref() { validate_object_canned_acl(acl)?; @@ -295,17 +289,25 @@ impl S3Service { // collapse onto an owner-only grant only because their real grantees // are not modeled. `private` in particular is neither "no ACL" nor // "bucket owner full control". + // The BUCKET OWNER, which is who the exception is about -- not the + // caller. They coincide for a bucket this account created, and diverge + // for one persisted by another account and hydrated into the default + // account on restart, where comparing against the caller accepted a + // grant to somebody who is not the owner and refused the grant to the + // owner that AWS actually accepts. + let owner_id = self + .bucket_acl_owner_id(account_id, bucket) + .unwrap_or_else(|| account_id.to_string()); let asks_for_owner_full_control = match (&grants, canned.as_deref()) { // "an equivalent form of this ACL": explicit grants that give the - // owner full control and nobody anything. - (Some(g), _) => { - !g.is_empty() - && g.iter().all(|grant| { - grant.permission == "FULL_CONTROL" - && grant.grantee_type == "CanonicalUser" - && grant.grantee_id.as_deref() == Some(account_id) - }) - } + // owner full control and nobody anything. `resolved_grant_headers` + // never yields an empty vector (it errors instead), so every grant + // here is a real clause from the request. + (Some(g), _) => g.iter().all(|grant| { + grant.permission == "FULL_CONTROL" + && grant.grantee_type == "CanonicalUser" + && grant.grantee_id.as_deref() == Some(owner_id.as_str()) + }), (None, Some(acl)) => acl == "bucket-owner-full-control", (None, None) => false, }; diff --git a/crates/fakecloud-s3/src/service/tests.rs b/crates/fakecloud-s3/src/service/tests.rs index d42b11948..643741de2 100644 --- a/crates/fakecloud-s3/src/service/tests.rs +++ b/crates/fakecloud-s3/src/service/tests.rs @@ -3885,26 +3885,45 @@ fn bucket_owner_full_control_is_accepted_when_ownership_disables_acls() { // Assert what it resolves to, not that the function echoed back the header // it was handed: the whole point is that the owner ends up with full // control and nobody else appears. + // Resolved against the bucket's own ACL owner, which is what the write paths + // pass and therefore what ends up stored. let grants = allowed - .grants_for("123456789012") + .grants_for("owner") .expect("the canned value resolves to grants"); assert_eq!(grants.len(), 1, "{grants:?}"); assert_eq!(grants[0].permission, "FULL_CONTROL"); assert_eq!(grants[0].grantee_type, "CanonicalUser"); - assert_eq!(grants[0].grantee_id.as_deref(), Some("123456789012")); + assert_eq!(grants[0].grantee_id.as_deref(), Some("owner")); // The exception is about what the ACL GRANTS, not how it is spelled: AWS - // accepts "an equivalent form of this ACL" too, so an explicit - // full-control grant to the owner is accepted just like the canned value. + // accepts "an equivalent form of this ACL" too, so an explicit full-control + // grant to the BUCKET OWNER is accepted just like the canned value. + // + // `seed_bucket` owns this bucket as "owner" while the caller is + // 123456789012, which is the case that distinguishes the two. They coincide + // for a bucket the caller created; they diverge for one persisted by another + // account and hydrated into the default account on restart. svc.resolve_write_acl_headers("123456789012", "bofc", &{ let mut h = HeaderMap::new(); - h.insert( - "x-amz-grant-full-control", - "id=123456789012".parse().unwrap(), - ); + h.insert("x-amz-grant-full-control", "id=owner".parse().unwrap()); h }) - .expect("an owner-only grant is the equivalent form and must be accepted"); + .expect("full control to the bucket owner is the equivalent form"); + + // ...and a grant to anyone else is not, even the caller. Such a request + // would leave an object on a BucketOwnerEnforced bucket whose ACL gives the + // bucket owner nothing, which is the state that setting exists to prevent. + let err = svc + .resolve_write_acl_headers("123456789012", "bofc", &{ + let mut h = HeaderMap::new(); + h.insert( + "x-amz-grant-full-control", + "id=123456789012".parse().unwrap(), + ); + h + }) + .expect_err("full control to a non-owner is not the equivalent form"); + assert_eq!(err.code(), "AccessControlListNotSupported", "{err:?}"); // Every other ACL is refused. The model's wording is narrow -- such a // bucket "only accept[s] PUT requests that don't specify an ACL or PUT @@ -3988,28 +4007,31 @@ async fn put_object_honors_the_bucket_owner_enforced_rules_end_to_end() { } #[tokio::test] -async fn a_blank_acl_header_is_treated_as_no_acl() { - // A present-but-blank header is what a client sends for an unset config - // field; the `x-amz-grant-*` family already treats it as absent, and - // 400-ing on the canned one made that inconsistent. +async fn a_blank_acl_header_is_rejected_on_every_acl_setting_path() { + // `""` is not a member of `ObjectCannedACL`, and every path that accepts a + // canned ACL answers 400 for it. Treating it as "no ACL asked for" on the + // object-write paths alone made one operation disagree with four; the + // `x-amz-grant-*` family's blank rule is a different case, since an empty + // value there names no grantee rather than naming an invalid ACL. let svc = make_service(); seed_bucket(&svc, "blank-acl"); - let resolved = svc + let err = svc .resolve_write_acl_headers("123456789012", "blank-acl", &{ let mut h = HeaderMap::new(); h.insert("x-amz-acl", "".parse().unwrap()); h }) - .expect("a blank canned ACL asks for nothing, so it is not an error"); - assert_eq!(resolved.canned, None); - assert!(resolved.grants_for("123456789012").is_none()); + .expect_err("a blank canned ACL is not a canned ACL"); + assert_eq!(err.code(), "InvalidArgument", "{err:?}"); let mut req = make_request(Method::PUT, "/blank-acl/k", &[], b"body"); req.headers.insert("x-amz-acl", "".parse().unwrap()); - svc.put_object("123456789012", &req, "blank-acl", "k") - .await - .expect("a blank canned ACL must not fail the write"); + let err = match svc.put_object("123456789012", &req, "blank-acl", "k").await { + Ok(_) => panic!("a blank canned ACL must not be accepted"), + Err(e) => e, + }; + assert_eq!(err.code(), "InvalidArgument", "{err:?}"); } #[test] diff --git a/website/content/docs/services/s3.md b/website/content/docs/services/s3.md index 8644b8211..5a22359b6 100644 --- a/website/content/docs/services/s3.md +++ b/website/content/docs/services/s3.md @@ -26,7 +26,7 @@ fakecloud implements **107 of 107** S3 operations at 100% Smithy conformance. - **Object Lambda** — `WriteGetObjectResponse` actually stores the transformed body + metadata against the original request token; the next `GetObject` on the access point returns the transformed payload. - **Public Access Block** — `IgnorePublicAcls` is enforced on `GetObject`; public-read ACL grants are ignored when the bucket-level block is set. `BlockPublicAcls` is enforced when the ACL is written, on the object-write paths as well as `PutObjectAcl` / `PutBucketAcl`: `put-object --acl public-read` against a bucket with `BlockPublicAcls` is `AccessDenied`, as on AWS, rather than quietly storing the `AllUsers` grant the block exists to prevent. - **Anonymous access** — unsigned `GET`/`HEAD` requests (`GET /bucket/key` with no SigV4) reach S3 directly, so you can serve static assets / CDN origins straight from a bucket. In the default (no-IAM) mode they're served permissively; under `--iam soft|strict` an anonymous request is authorized only when a bucket policy grants the action to `Principal:"*"` or a public-read ACL (`AllUsers`) is set, denied otherwise. -- **ACL ownership modes** — `BucketOwnerEnforced` disables ACLs entirely (all ACL writes rejected, reads return owner-only). +- **ACL ownership modes** — `BucketOwnerEnforced` disables ACLs: `PutBucketAcl` and `PutObjectAcl` are rejected outright and reads return owner-only. The object-write paths keep the one exception AWS documents, described under Bucket and object ACLs above: a request specifying no ACL, or bucket-owner-full-control (canned or as the equivalent explicit grant to the bucket owner), is accepted. ## Protocol From 0b209fa01f0913d66a674ccdd77c72e903008bff Mon Sep 17 00:00:00 2001 From: Lucas Vieira Date: Tue, 29 Sep 2026 08:08:43 -0300 Subject: [PATCH 2/2] fix(s3): resolve the bucket owner once, and only when an ACL is named Review of the commit before this one: - The new doc block landed INSIDE `bucket_owner_enforced`'s rustdoc, so `bucket_acl_owner_id` was documented as "Returns true when the bucket has ObjectOwnership=BucketOwnerEnforced" -- for a function returning `Option` -- and `bucket_owner_enforced` was left undocumented. Both now carry their own docs, and `bucket_acl_owner_id`'s says why the `None` fallback is inert rather than merely tolerable: every consumer pairs it with `bucket_owner_enforced`, which returns false on exactly the same two misses, so the fallback value is never what an answer turns on. - The owner was looked up on EVERY object write and discarded whenever the request carried no ACL, which is the overwhelming majority -- an extra acquisition of the global read lock on the hot path. It is now looked up only when the request actually names an ACL. - That lookup also now feeds the resolved grants, not just the exception check. `requested` was still built with the CALLER's id while the write paths store grants under the bucket's owner. Nothing turned on it (`grants_are_public` reads only `grantee_uri`), so it was a latent trap rather than a bug -- but deriving the owner two ways in one function is exactly how the original defect happened. - The docs line rewritten last commit kept a claim nothing implements: "reads return owner-only". No read path consults the ownership setting -- `GetBucketAcl` and `GetObjectAcl` return the stored grants verbatim -- so an ACL set before the mode was turned on is still reported. The bullet now says writes are rejected, reads are not reinterpreted, and that AWS differs there. Re-verified after the restructure: forcing the comparison back to the caller fails `bucket_owner_full_control_is_accepted_when_ownership_disables_acls`; with it in place, 488 unit / 126 e2e / 51 conformance pass and the probe holds at 4040/4068. --- crates/fakecloud-s3/src/service/config/mod.rs | 22 +++++++------ crates/fakecloud-s3/src/service/mod.rs | 32 ++++++++++++------- website/content/docs/services/s3.md | 2 +- 3 files changed, 33 insertions(+), 23 deletions(-) diff --git a/crates/fakecloud-s3/src/service/config/mod.rs b/crates/fakecloud-s3/src/service/config/mod.rs index 10e8614f9..7a3b7b36a 100644 --- a/crates/fakecloud-s3/src/service/config/mod.rs +++ b/crates/fakecloud-s3/src/service/config/mod.rs @@ -386,21 +386,17 @@ impl S3Service { } } - /// Returns true when the bucket has - /// `ObjectOwnership=BucketOwnerEnforced` set in its - /// `OwnershipControls` configuration. Under that mode AWS - /// disables ACLs entirely — every ACL-mutating call must reject - /// with `AccessControlListNotSupported` so callers don't - /// silently no-op against a bucket that ignores their grants. /// The canonical user the bucket's ACL is owned by, which is who "bucket /// owner full control" means. Usually the caller, but not always: a bucket /// persisted by one account is hydrated into the configured default account /// on restart while keeping its own stored `acl_owner_id`. /// - /// `None` when the bucket is not in memory, which is not an error here -- - /// the ACL headers are validated before the operation looks the bucket up, - /// so the caller falls back to its own id and the missing bucket is reported - /// by the operation itself. + /// `None` when the bucket is not in memory. That is not an error here, and + /// callers may fall back to the caller's own id: every consumer of this pairs + /// it with [`S3Service::bucket_owner_enforced`], which returns `false` for + /// exactly the same two misses, so the fallback value is never the one an + /// answer turns on -- and the missing bucket is reported by the operation + /// itself. pub(super) fn bucket_acl_owner_id(&self, account_id: &str, bucket: &str) -> Option { let accts = self.state.read(); let state = accts.get(account_id)?; @@ -408,6 +404,12 @@ impl S3Service { Some(b.acl_owner_id.clone()) } + /// Returns true when the bucket has + /// `ObjectOwnership=BucketOwnerEnforced` set in its + /// `OwnershipControls` configuration. Under that mode AWS + /// disables ACLs entirely — every ACL-mutating call must reject + /// with `AccessControlListNotSupported` so callers don't + /// silently no-op against a bucket that ignores their grants. pub(super) fn bucket_owner_enforced(&self, account_id: &str, bucket: &str) -> bool { let accts = self.state.read(); let Some(state) = accts.get(account_id) else { diff --git a/crates/fakecloud-s3/src/service/mod.rs b/crates/fakecloud-s3/src/service/mod.rs index 2f81c38e4..6277de17b 100644 --- a/crates/fakecloud-s3/src/service/mod.rs +++ b/crates/fakecloud-s3/src/service/mod.rs @@ -268,11 +268,28 @@ impl S3Service { "Specifying both Canned ACLs and Header Grants is not allowed", )); } + // The bucket's ACL owner, which is who "bucket owner full control" means + // -- not the caller. They coincide for a bucket this account created, and + // diverge for one persisted by another account and hydrated into the + // default account on restart, where comparing against the caller accepted + // a grant to somebody who is not the owner and refused the grant to the + // owner that AWS accepts. + // + // Looked up only when the request actually names an ACL: the overwhelming + // majority of object writes carry none, and this takes the global read + // lock. Resolved once and used for both the grants below and the exception + // check, so the two cannot derive the owner differently. + let bucket_owner = (canned.is_some() || grants.is_some()) + .then(|| self.bucket_acl_owner_id(account_id, bucket)) + .flatten(); + let owner_id = bucket_owner.as_deref().unwrap_or(account_id); + // The grants the request resolves to, which is what the public-ACL check - // below has to judge. + // below has to judge. Resolved against the bucket's owner, which is what + // the write paths store them under. let requested = match (&grants, canned.as_deref()) { (Some(g), _) => Some(g.clone()), - (None, Some(acl)) => Some(canned_acl_grants_for_object(acl, account_id)), + (None, Some(acl)) => Some(canned_acl_grants_for_object(acl, owner_id)), (None, None) => None, }; @@ -289,15 +306,6 @@ impl S3Service { // collapse onto an owner-only grant only because their real grantees // are not modeled. `private` in particular is neither "no ACL" nor // "bucket owner full control". - // The BUCKET OWNER, which is who the exception is about -- not the - // caller. They coincide for a bucket this account created, and diverge - // for one persisted by another account and hydrated into the default - // account on restart, where comparing against the caller accepted a - // grant to somebody who is not the owner and refused the grant to the - // owner that AWS actually accepts. - let owner_id = self - .bucket_acl_owner_id(account_id, bucket) - .unwrap_or_else(|| account_id.to_string()); let asks_for_owner_full_control = match (&grants, canned.as_deref()) { // "an equivalent form of this ACL": explicit grants that give the // owner full control and nobody anything. `resolved_grant_headers` @@ -306,7 +314,7 @@ impl S3Service { (Some(g), _) => g.iter().all(|grant| { grant.permission == "FULL_CONTROL" && grant.grantee_type == "CanonicalUser" - && grant.grantee_id.as_deref() == Some(owner_id.as_str()) + && grant.grantee_id.as_deref() == Some(owner_id) }), (None, Some(acl)) => acl == "bucket-owner-full-control", (None, None) => false, diff --git a/website/content/docs/services/s3.md b/website/content/docs/services/s3.md index 5a22359b6..ecde3eae9 100644 --- a/website/content/docs/services/s3.md +++ b/website/content/docs/services/s3.md @@ -26,7 +26,7 @@ fakecloud implements **107 of 107** S3 operations at 100% Smithy conformance. - **Object Lambda** — `WriteGetObjectResponse` actually stores the transformed body + metadata against the original request token; the next `GetObject` on the access point returns the transformed payload. - **Public Access Block** — `IgnorePublicAcls` is enforced on `GetObject`; public-read ACL grants are ignored when the bucket-level block is set. `BlockPublicAcls` is enforced when the ACL is written, on the object-write paths as well as `PutObjectAcl` / `PutBucketAcl`: `put-object --acl public-read` against a bucket with `BlockPublicAcls` is `AccessDenied`, as on AWS, rather than quietly storing the `AllUsers` grant the block exists to prevent. - **Anonymous access** — unsigned `GET`/`HEAD` requests (`GET /bucket/key` with no SigV4) reach S3 directly, so you can serve static assets / CDN origins straight from a bucket. In the default (no-IAM) mode they're served permissively; under `--iam soft|strict` an anonymous request is authorized only when a bucket policy grants the action to `Principal:"*"` or a public-read ACL (`AllUsers`) is set, denied otherwise. -- **ACL ownership modes** — `BucketOwnerEnforced` disables ACLs: `PutBucketAcl` and `PutObjectAcl` are rejected outright and reads return owner-only. The object-write paths keep the one exception AWS documents, described under Bucket and object ACLs above: a request specifying no ACL, or bucket-owner-full-control (canned or as the equivalent explicit grant to the bucket owner), is accepted. +- **ACL ownership modes** — `BucketOwnerEnforced` disables ACL WRITES: `PutBucketAcl` and `PutObjectAcl` are rejected outright. Reads are not reinterpreted: `GetBucketAcl` / `GetObjectAcl` return whatever grants are stored, so an ACL set before the mode was turned on is still reported (AWS reports owner-only there). The object-write paths keep the one exception AWS documents, described under Bucket and object ACLs above: a request specifying no ACL, or bucket-owner-full-control (canned or as the equivalent explicit grant to the bucket owner), is accepted. ## Protocol