diff --git a/crates/fakecloud-s3/src/service/config/mod.rs b/crates/fakecloud-s3/src/service/config/mod.rs index 368b96e35..7a3b7b36a 100644 --- a/crates/fakecloud-s3/src/service/config/mod.rs +++ b/crates/fakecloud-s3/src/service/config/mod.rs @@ -386,6 +386,24 @@ impl S3Service { } } + /// 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. 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)?; + let b = state.buckets.get(bucket)?; + Some(b.acl_owner_id.clone()) + } + /// Returns true when the bucket has /// `ObjectOwnership=BucketOwnerEnforced` set in its /// `OwnershipControls` configuration. Under that mode AWS diff --git a/crates/fakecloud-s3/src/service/mod.rs b/crates/fakecloud-s3/src/service/mod.rs index 749cd1f03..8017a6e70 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)?; @@ -274,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, }; @@ -297,15 +308,14 @@ impl S3Service { // "bucket owner full control". 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) + }), (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 58d067b37..d64cc1f8d 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..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 entirely (all ACL writes rejected, reads return owner-only). +- **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