From d23a09d4e64150ae4c90cd4edd01efe66a1eb29c Mon Sep 17 00:00:00 2001 From: Lucas Vieira Date: Fri, 25 Sep 2026 17:32:57 -0300 Subject: [PATCH 1/4] fix(s3): authorize a configuring CreateBucket per setting, and populate the ACL condition keys CreateBucket was authorized as s3:CreateBucket alone (plus s3:TagResource for a tag set), even though it applies x-amz-acl / x-amz-grant-*, x-amz-object-ownership and object-lock enablement. On AWS each of those needs its own permission. This got worse when those settings started persisting: a principal holding only s3:CreateBucket could create a `public-read` bucket that used to lose the grant on restart and now keeps it, so a transient gap became durable over-permission. The create now returns one action per setting the request configures, following the permissions section of CreateBucket in the vendored model -- s3:PutBucketAcl, s3:PutBucketOwnershipControls, and s3:PutBucketObjectLockConfiguration plus s3:PutBucketVersioning (object lock turns versioning on, so AWS wants both). Dispatch already requires every returned action to be allowed, so a principal with only s3:CreateBucket can still create a plain bucket. The s3:x-amz-acl and s3:x-amz-grant-* condition keys are also populated now. They are how a policy pins down WHICH ACL a write may ask for, and while they were never emitted a guardrail like `Deny s3:CreateBucket when s3:x-amz-acl != private` matched nothing and silently authorized exactly what it was written to block. They are emitted from the headers on any request that carries them rather than gated on an action list that would drift, and an absent header emits nothing so a condition on it safe-fails to "does not apply". E2E covers the least-privilege principal being refused and then unblocked for both the ACL and the object-lock cases, and a condition-keyed policy admitting `private` while denying `public-read`. The ACL rule follows that documentation precisely: an ACL set to public-read, public-read-write, authenticated-read "or any other custom ACLs" needs s3:PutBucketAcl, while "if you set the ACL to private, or if you don't specify any ACLs, only the s3:CreateBucket permission is required". It is judged by the grants the request resolves to, so a least-privilege caller that always sends `--acl private` -- Terraform's legacy `acl` attribute, SDK wrappers that always set one -- is not refused where real S3 succeeds. A blank `x-amz-grant-*` header does not emit its condition key, for the same reason has_grant_headers treats it as absent: the request asks for no grant, and a present key would make a Null-based guardrail deny it. --- crates/fakecloud-e2e/tests/iam_enforcement.rs | 168 ++++++++++++++++++ crates/fakecloud-s3/src/service/mod.rs | 91 +++++++++- crates/fakecloud-s3/src/service/tests.rs | 134 +++++++++++++- website/content/docs/reference/security.md | 2 + website/content/docs/services/s3.md | 1 + 5 files changed, 383 insertions(+), 13 deletions(-) diff --git a/crates/fakecloud-e2e/tests/iam_enforcement.rs b/crates/fakecloud-e2e/tests/iam_enforcement.rs index 22d6da708..0fbccb0e8 100644 --- a/crates/fakecloud-e2e/tests/iam_enforcement.rs +++ b/crates/fakecloud-e2e/tests/iam_enforcement.rs @@ -415,6 +415,174 @@ async fn sns_publish_allowed_on_specific_topic() { /// bucket carrying `CreateBucketConfiguration.Tags` (issue #2553). A grant of /// `s3:CreateBucket` alone still creates untagged buckets, but a tagged create /// is denied. +/// A create that configures the bucket needs the permission for what it +/// configures. This matters because those settings now PERSIST: a `public-read` +/// bucket created by a principal with no `s3:PutBucketAcl` used to lose the +/// grant on restart and now keeps it. +#[tokio::test] +async fn s3_create_bucket_with_acl_needs_put_bucket_acl() { + let server = start_strict().await; + let (akid, secret) = bootstrap_user(&server, "s3aclcreate").await; + attach_inline_policy( + &server, + "s3aclcreate", + "create-only", + r#"{"Version":"2012-10-17","Statement":[ + {"Effect":"Allow","Action":"s3:CreateBucket","Resource":"*"} + ]}"#, + ) + .await; + + let cfg = sdk_config_with(&server, &akid, &secret).await; + let s3 = aws_sdk_s3::Client::new(&cfg); + + // A plain create is still allowed by s3:CreateBucket alone. + s3.create_bucket() + .bucket("aclperm-plain") + .send() + .await + .unwrap(); + + // So is `--acl private`: the model'"'"'s CreateBucket permissions exempt it + // and a no-ACL create from s3:PutBucketAcl, and clients that always send a + // canned ACL (Terraform'"'"'s legacy `acl` attribute, SDK wrappers) would + // otherwise be refused where real S3 succeeds. + s3.create_bucket() + .bucket("aclperm-private") + .acl(aws_sdk_s3::types::BucketCannedAcl::Private) + .send() + .await + .expect("a private canned ACL needs only s3:CreateBucket"); + + // Asking for an ACL is not. + let err = s3 + .create_bucket() + .bucket("aclperm-denied") + .acl(aws_sdk_s3::types::BucketCannedAcl::PublicRead) + .send() + .await + .expect_err("a canned ACL needs s3:PutBucketAcl"); + assert!( + format!("{err:?}").contains("AccessDenied"), + "unexpected error: {err:?}" + ); + + // Granting it unblocks the create, and the bucket really is public-read. + attach_inline_policy( + &server, + "s3aclcreate", + "create-and-acl", + r#"{"Version":"2012-10-17","Statement":[ + {"Effect":"Allow","Action":["s3:CreateBucket","s3:PutBucketAcl","s3:GetBucketAcl"],"Resource":"*"} + ]}"#, + ) + .await; + s3.create_bucket() + .bucket("aclperm-allowed") + .acl(aws_sdk_s3::types::BucketCannedAcl::PublicRead) + .send() + .await + .unwrap(); + let acl = s3 + .get_bucket_acl() + .bucket("aclperm-allowed") + .send() + .await + .unwrap(); + assert!(acl.grants().iter().any(|g| { + g.grantee() + .and_then(|gr| gr.uri()) + .is_some_and(|u| u.contains("AllUsers")) + })); +} + +/// The `s3:x-amz-acl` condition key is what a policy uses to pin down WHICH ACL +/// a create may ask for. It was never populated, so such a guardrail silently +/// authorized exactly what it was written to block. +#[tokio::test] +async fn s3_create_bucket_acl_condition_key_gates_the_canned_value() { + let server = start_strict().await; + let (akid, secret) = bootstrap_user(&server, "s3aclcond").await; + attach_inline_policy( + &server, + "s3aclcond", + "private-acls-only", + r#"{"Version":"2012-10-17","Statement":[ + {"Effect":"Allow","Action":["s3:CreateBucket","s3:PutBucketAcl"],"Resource":"*", + "Condition":{"StringEquals":{"s3:x-amz-acl":"private"}}} + ]}"#, + ) + .await; + + let cfg = sdk_config_with(&server, &akid, &secret).await; + let s3 = aws_sdk_s3::Client::new(&cfg); + + s3.create_bucket() + .bucket("aclcond-private") + .acl(aws_sdk_s3::types::BucketCannedAcl::Private) + .send() + .await + .expect("the permitted canned value must be allowed"); + + let err = s3 + .create_bucket() + .bucket("aclcond-public") + .acl(aws_sdk_s3::types::BucketCannedAcl::PublicRead) + .send() + .await + .expect_err("public-read must be denied by the condition"); + assert!( + format!("{err:?}").contains("AccessDenied"), + "unexpected error: {err:?}" + ); +} + +/// Object lock enables versioning, so AWS wants both permissions. +#[tokio::test] +async fn s3_create_bucket_with_object_lock_needs_its_permissions() { + let server = start_strict().await; + let (akid, secret) = bootstrap_user(&server, "s3lockcreate").await; + attach_inline_policy( + &server, + "s3lockcreate", + "create-only", + r#"{"Version":"2012-10-17","Statement":[ + {"Effect":"Allow","Action":"s3:CreateBucket","Resource":"*"} + ]}"#, + ) + .await; + + let cfg = sdk_config_with(&server, &akid, &secret).await; + let s3 = aws_sdk_s3::Client::new(&cfg); + let err = s3 + .create_bucket() + .bucket("lockperm-denied") + .object_lock_enabled_for_bucket(true) + .send() + .await + .expect_err("object lock needs its own permissions"); + assert!( + format!("{err:?}").contains("AccessDenied"), + "unexpected error: {err:?}" + ); + + attach_inline_policy( + &server, + "s3lockcreate", + "create-and-lock", + r#"{"Version":"2012-10-17","Statement":[ + {"Effect":"Allow","Action":["s3:CreateBucket","s3:PutBucketObjectLockConfiguration","s3:PutBucketVersioning"],"Resource":"*"} + ]}"#, + ) + .await; + s3.create_bucket() + .bucket("lockperm-allowed") + .object_lock_enabled_for_bucket(true) + .send() + .await + .unwrap(); +} + #[tokio::test] async fn s3_create_bucket_with_tags_needs_tag_resource() { let server = start_strict().await; diff --git a/crates/fakecloud-s3/src/service/mod.rs b/crates/fakecloud-s3/src/service/mod.rs index 3a8ccbb23..a5ffe4bcf 100644 --- a/crates/fakecloud-s3/src/service/mod.rs +++ b/crates/fakecloud-s3/src/service/mod.rs @@ -1531,14 +1531,65 @@ impl AwsService for S3Service { return Vec::new(); }; if action.service == "s3" && action.action == "CreateBucket" { + // A create that also configures the bucket needs the permission for + // each thing it configures, as on AWS: dispatch requires every + // returned action to be allowed, so a principal holding only + // `s3:CreateBucket` can still create a plain bucket. This matters + // more now that these settings PERSIST -- a bucket created + // `public-read` by a caller with no `s3:PutBucketAcl` used to lose + // the grant on restart, and now keeps it. + let mut extra = Vec::new(); let body = std::str::from_utf8(&request.body).unwrap_or(""); if !create_bucket_configuration_tags(body).is_empty() { - let tag_resource = fakecloud_core::auth::IamAction { - service: "s3", - action: "TagResource", - resource: action.resource.clone(), - }; - return vec![action, tag_resource]; + extra.push("TagResource"); + } + // Per the CreateBucket permissions in the vendored model: an ACL set + // to public-read, public-read-write, authenticated-read "or any + // other custom ACLs" needs s3:PutBucketAcl, while "if you set the + // ACL to private, or if you don't specify any ACLs, only the + // s3:CreateBucket permission is required". Judged by the grants the + // request resolves to, so the two spellings of an owner-only ACL + // agree -- and so a least-privilege caller that always sends + // `--acl private` is not refused. + let acl_reaches_past_owner = request + .headers + .get("x-amz-acl") + .and_then(|v| v.to_str().ok()) + .is_some_and(|acl| { + canned_acl_grants(acl, &request.account_id).iter().any(|g| { + g.permission != "FULL_CONTROL" + || g.grantee_type != "CanonicalUser" + || g.grantee_id.as_deref() != Some(request.account_id.as_str()) + }) + }); + if acl_reaches_past_owner || has_grant_headers(&request.headers) { + extra.push("PutBucketAcl"); + } + if request.headers.contains_key("x-amz-object-ownership") { + extra.push("PutBucketOwnershipControls"); + } + if request + .headers + .get("x-amz-bucket-object-lock-enabled") + .and_then(|v| v.to_str().ok()) + .is_some_and(|v| v.eq_ignore_ascii_case("true")) + { + // Object lock forces versioning on, so AWS wants both. + extra.push("PutBucketObjectLockConfiguration"); + extra.push("PutBucketVersioning"); + } + if !extra.is_empty() { + let mut actions = Vec::with_capacity(extra.len() + 1); + let resource = action.resource.clone(); + actions.push(action); + for name in extra { + actions.push(fakecloud_core::auth::IamAction { + service: "s3", + action: name, + resource: resource.clone(), + }); + } + return actions; } } vec![action] @@ -1549,7 +1600,7 @@ impl AwsService for S3Service { request: &AwsRequest, action: &fakecloud_core::auth::IamAction, ) -> std::collections::BTreeMap> { - s3_condition_keys(action.action, &request.query_params) + s3_condition_keys(action.action, &request.query_params, &request.headers) } fn resource_tags_for( @@ -1579,8 +1630,34 @@ impl AwsService for S3Service { fn s3_condition_keys( action: &str, query: &std::collections::HashMap, + headers: &HeaderMap, ) -> std::collections::BTreeMap> { let mut out = std::collections::BTreeMap::new(); + // `s3:x-amz-acl` and the `s3:x-amz-grant-*` family are how a policy pins + // down which ACL a write may ask for -- a guardrail like + // `Deny s3:CreateBucket when s3:x-amz-acl != private` is useless while they + // are never populated. AWS exposes them on every ACL-accepting write, so + // they are emitted whenever the request carries them rather than being + // gated on an action list that would drift. + if let Some(acl) = headers.get("x-amz-acl").and_then(|v| v.to_str().ok()) { + out.insert("s3:x-amz-acl".to_string(), vec![acl.to_string()]); + } + for (header, _) in &GRANT_HEADER_PERMISSIONS { + // Blank values are skipped for the same reason `has_grant_headers` + // treats them as absent: a present-but-empty header asks for no grant, + // and emitting the key would make a `Null`-based guardrail read it as + // present and deny the request. + let values: Vec = headers + .get_all(*header) + .iter() + .filter_map(|v| v.to_str().ok()) + .filter(|v| !v.trim().is_empty()) + .map(|v| v.to_string()) + .collect(); + if !values.is_empty() { + out.insert(format!("s3:{header}"), values); + } + } if matches!(action, "ListObjects" | "ListObjectsV2") { // Both list variants share the same query param shape. if let Some(prefix) = query.get("prefix") { diff --git a/crates/fakecloud-s3/src/service/tests.rs b/crates/fakecloud-s3/src/service/tests.rs index 6b9f5369d..63cd362ad 100644 --- a/crates/fakecloud-s3/src/service/tests.rs +++ b/crates/fakecloud-s3/src/service/tests.rs @@ -6,7 +6,7 @@ fn s3_condition_keys_emits_list_params() { q.insert("prefix".to_string(), "logs/".to_string()); q.insert("delimiter".to_string(), "/".to_string()); q.insert("max-keys".to_string(), "100".to_string()); - let keys = s3_condition_keys("ListObjectsV2", &q); + let keys = s3_condition_keys("ListObjectsV2", &q, &HeaderMap::new()); assert_eq!(keys.get("s3:prefix"), Some(&vec!["logs/".to_string()])); assert_eq!(keys.get("s3:delimiter"), Some(&vec!["/".to_string()])); assert_eq!(keys.get("s3:max-keys"), Some(&vec!["100".to_string()])); @@ -15,7 +15,7 @@ fn s3_condition_keys_emits_list_params() { #[test] fn s3_condition_keys_omits_absent_params() { let q = std::collections::HashMap::new(); - let keys = s3_condition_keys("ListObjectsV2", &q); + let keys = s3_condition_keys("ListObjectsV2", &q, &HeaderMap::new()); assert!(keys.is_empty()); } @@ -23,7 +23,7 @@ fn s3_condition_keys_omits_absent_params() { fn s3_condition_keys_partial_params() { let mut q = std::collections::HashMap::new(); q.insert("prefix".to_string(), "archive/".to_string()); - let keys = s3_condition_keys("ListObjects", &q); + let keys = s3_condition_keys("ListObjects", &q, &HeaderMap::new()); assert_eq!(keys.len(), 1); assert_eq!(keys.get("s3:prefix"), Some(&vec!["archive/".to_string()])); } @@ -32,9 +32,9 @@ fn s3_condition_keys_partial_params() { fn s3_condition_keys_empty_for_non_list_actions() { let mut q = std::collections::HashMap::new(); q.insert("prefix".to_string(), "logs/".to_string()); - assert!(s3_condition_keys("GetObject", &q).is_empty()); - assert!(s3_condition_keys("PutObject", &q).is_empty()); - assert!(s3_condition_keys("ListBuckets", &q).is_empty()); + assert!(s3_condition_keys("GetObject", &q, &HeaderMap::new()).is_empty()); + assert!(s3_condition_keys("PutObject", &q, &HeaderMap::new()).is_empty()); + assert!(s3_condition_keys("ListBuckets", &q, &HeaderMap::new()).is_empty()); } #[test] @@ -4300,6 +4300,128 @@ fn create_bucket_tags_decode_xml_entities() { ); } +#[test] +fn create_bucket_requires_a_permission_per_setting_it_configures() { + use fakecloud_core::service::AwsService as _; + + let svc = make_service(); + let names = |req: &AwsRequest| -> Vec<&'static str> { + svc.iam_actions_for(req).iter().map(|a| a.action).collect() + }; + + // A plain create needs only s3:CreateBucket. + let plain = make_request(Method::PUT, "/perm-plain", &[], b""); + assert_eq!(names(&plain), vec!["CreateBucket"]); + + // An ACL that reaches past the owner needs PutBucketAcl -- the settings + // persist now, so a create that configures them is a configuration call as + // much as a create. + let mut canned = make_request(Method::PUT, "/perm-acl", &[], b""); + canned + .headers + .insert("x-amz-acl", "public-read".parse().unwrap()); + assert_eq!(names(&canned), vec!["CreateBucket", "PutBucketAcl"]); + + // ...but `private` does not. The model'"'"'s CreateBucket permissions are + // explicit: "if you set the ACL to private, or if you don'"'"'t specify any + // ACLs, only the s3:CreateBucket permission is required". A least-privilege + // caller that always sends `--acl private` must not be refused. + let mut private = make_request(Method::PUT, "/perm-private", &[], b""); + private + .headers + .insert("x-amz-acl", "private".parse().unwrap()); + assert_eq!(names(&private), vec!["CreateBucket"]); + + let mut granted = make_request(Method::PUT, "/perm-grant", &[], b""); + granted.headers.insert( + "x-amz-grant-read", + "uri=http://acs.amazonaws.com/groups/global/AllUsers" + .parse() + .unwrap(), + ); + assert_eq!(names(&granted), vec!["CreateBucket", "PutBucketAcl"]); + + let mut owned = make_request(Method::PUT, "/perm-own", &[], b""); + owned.headers.insert( + "x-amz-object-ownership", + "BucketOwnerEnforced".parse().unwrap(), + ); + assert_eq!( + names(&owned), + vec!["CreateBucket", "PutBucketOwnershipControls"] + ); + + // Object lock turns versioning on, so both permissions are required. + let mut locked = make_request(Method::PUT, "/perm-lock", &[], b""); + locked + .headers + .insert("x-amz-bucket-object-lock-enabled", "true".parse().unwrap()); + assert_eq!( + names(&locked), + vec![ + "CreateBucket", + "PutBucketObjectLockConfiguration", + "PutBucketVersioning" + ] + ); + + // A false object-lock header configures nothing. + let mut unlocked = make_request(Method::PUT, "/perm-unlocked", &[], b""); + unlocked + .headers + .insert("x-amz-bucket-object-lock-enabled", "false".parse().unwrap()); + assert_eq!(names(&unlocked), vec!["CreateBucket"]); + + // Everything at once, tags included, in one authorization set. + let mut everything = make_request( + Method::PUT, + "/perm-all", + &[], + b"teama", + ); + everything + .headers + .insert("x-amz-acl", "public-read".parse().unwrap()); + everything + .headers + .insert("x-amz-object-ownership", "ObjectWriter".parse().unwrap()); + assert_eq!( + names(&everything), + vec![ + "CreateBucket", + "TagResource", + "PutBucketAcl", + "PutBucketOwnershipControls" + ] + ); +} + +#[test] +fn acl_condition_keys_are_populated_from_the_request_headers() { + // A guardrail like `Deny s3:CreateBucket when s3:x-amz-acl != private` is + // useless while the key is never emitted, which is what made the + // over-permission above invisible to policy. + let mut headers = HeaderMap::new(); + headers.insert("x-amz-acl", "public-read".parse().unwrap()); + let keys = s3_condition_keys("CreateBucket", &HashMap::new(), &headers); + assert_eq!( + keys.get("s3:x-amz-acl"), + Some(&vec!["public-read".to_string()]) + ); + + let mut grants = HeaderMap::new(); + grants.insert("x-amz-grant-full-control", "id=abc123".parse().unwrap()); + let keys = s3_condition_keys("PutObject", &HashMap::new(), &grants); + assert_eq!( + keys.get("s3:x-amz-grant-full-control"), + Some(&vec!["id=abc123".to_string()]) + ); + + // Absent headers emit nothing, so a policy condition on them safe-fails to + // "does not apply" rather than matching an empty value. + assert!(s3_condition_keys("CreateBucket", &HashMap::new(), &HeaderMap::new()).is_empty()); +} + #[test] fn create_bucket_with_tags_also_requires_tag_resource() { use fakecloud_core::service::AwsService as _; diff --git a/website/content/docs/reference/security.md b/website/content/docs/reference/security.md index cb7284877..16b78fd73 100644 --- a/website/content/docs/reference/security.md +++ b/website/content/docs/reference/security.md @@ -130,6 +130,8 @@ Every operator supports the `...IfExists` suffix (missing key evaluates to `true | `s3:prefix` | `s3:ListObjects`, `s3:ListObjectsV2` | `?prefix=` query param | | `s3:delimiter` | `s3:ListObjects`, `s3:ListObjectsV2` | `?delimiter=` query param | | `s3:max-keys` | `s3:ListObjects`, `s3:ListObjectsV2` | `?max-keys=` query param | +| `s3:x-amz-acl` | any request carrying the header (`CreateBucket`, `PutObject`, `CopyObject`, `CreateMultipartUpload`, `PutBucketAcl`, `PutObjectAcl`) | `x-amz-acl` header | +| `s3:x-amz-grant-read`, `-write`, `-read-acp`, `-write-acp`, `-full-control` | any request carrying the header | matching `x-amz-grant-*` header | | `sns:Protocol` | `sns:Subscribe` | `Protocol` request parameter | | `sns:Endpoint` | `sns:Subscribe` | `Endpoint` request parameter | | `lambda:FunctionArn` | `lambda:AddPermission` | Target function ARN resolved from the path | diff --git a/website/content/docs/services/s3.md b/website/content/docs/services/s3.md index efa446855..5e614024c 100644 --- a/website/content/docs/services/s3.md +++ b/website/content/docs/services/s3.md @@ -16,6 +16,7 @@ fakecloud implements **107 of 107** S3 operations at 100% Smithy conformance. - **Versioning** — enable/suspend, list object versions, delete specific versions. Suspended buckets follow the AWS null-version rules: a write takes over the `null` version (replacing whatever held it), a delete stacks a `null`-id delete marker, earlier versions are kept either way, and an object written before versioning was enabled survives later writes as the `null` version. Suspending is rejected with `InvalidBucketState` on an Object Lock bucket, as on AWS. - **Encryption** — SSE-S3, SSE-KMS (real envelope encryption through the KMS hook with the `aws:s3:arn` encryption context), SSE-C - **Bucket subresources** — policy, CORS, lifecycle, logging, website, public access block, object lock, replication, ownership, inventory, encryption, accelerate, request payment, tagging +- **Permissions a configuring create needs** — `CreateBucket` is authorized as `s3:CreateBucket` plus one action per setting the request configures, as on AWS: `s3:TagResource` for a `CreateBucketConfiguration` tag set, `s3:PutBucketAcl` for an ACL that grants anything beyond the bucket owner (`private`, and a create with no ACL, need only `s3:CreateBucket`), `s3:PutBucketOwnershipControls` for `x-amz-object-ownership`, and `s3:PutBucketObjectLockConfiguration` plus `s3:PutBucketVersioning` for `x-amz-bucket-object-lock-enabled: true`. A principal holding only `s3:CreateBucket` can still create a plain bucket. The `s3:x-amz-acl` and `s3:x-amz-grant-*` condition keys are populated from those headers, so a policy can pin down which ACL a create may ask for. - **Bucket tags at create time** — `CreateBucket` honors the `Tags` tag set inside `CreateBucketConfiguration`, so a bucket comes out of `CreateBucket` already tagged and `GetBucketTagging` returns the set without a follow-up `PutBucketTagging`. This is the path the AWS Terraform provider (6.x) uses for `aws_s3_bucket`'s `tags`. Duplicate keys and `aws:`-prefixed keys are rejected with `InvalidTag` and no bucket is created, and the tags persist across a restart like any other bucket subresource. Under `--iam`, a tagged create is authorized as `s3:CreateBucket` **and** `s3:TagResource`, as on AWS — a grant of `s3:CreateBucket` alone still creates untagged buckets — and the tag set populates `aws:RequestTag/` / `aws:TagKeys` so policies can condition on it. - **CORS** — `PutBucketCors` rules drive real browser CORS: the `OPTIONS` preflight is matched on origin, `Access-Control-Request-Method` and every header in `Access-Control-Request-Headers` before returning `Access-Control-Allow-Origin/-Methods/-Headers` and `Access-Control-Max-Age`, and an actual request carrying `Origin` gets `Access-Control-Allow-Origin` plus `Access-Control-Expose-Headers`. Actual requests are matched on method as well as origin, so a rule allowing only `GET` hands no allow-origin to a `DELETE`. Multipart operations are CORS-evaluated like any other request, so browser multipart upload works. Errors from S3 itself are evaluated too, so a 404 `NoSuchKey` fetched by an allowed origin carries `Access-Control-Allow-Origin` and reaches the caller as a real 404 rather than an opaque CORS failure. Denials raised before the request reaches S3 (SigV4 or IAM, under `--verify-sigv4` / `--iam`) are not CORS-decorated, so under auth enforcement a rejected browser request still surfaces as an opaque failure. `Vary: Origin, Access-Control-Request-Headers, Access-Control-Request-Method` keeps a cache from reusing one origin's response for a different origin. **Every** preflight response carries it, since a preflight's outcome always turns on `Origin`: `200` when the rules allow it, `403 AccessForbidden` when they do not, and `400 BadRequest` ("Insufficient information. Origin request header needed.") when the request omits `Origin` altogether. On the actual-request path it goes on every response evaluated against a bucket's CORS config, successes and errors alike, while a request without `Origin` is not CORS-evaluated and is served normally with neither header, as on S3. A concrete allowed origin also gets `Access-Control-Allow-Credentials: true`, so `credentials: 'include'` works. Note that an actual request declares no headers, so it can match an earlier, broader rule than the preflight did: if a `*` rule precedes the concrete one, the actual response carries `Access-Control-Allow-Origin: *` and no credentials, and the browser blocks a credentialed request that just passed preflight. That is S3's behavior too — order the concrete rule first. `AllowedOrigin` and `AllowedHeader` each take one `*` anywhere in the value (`https://*.example.com`, `x-amz-*`). `ExposeHeader` takes no wildcard at all, as on S3. A rule missing `AllowedMethods` or `AllowedOrigins`, carrying more than one wildcard in a value, or setting a non-numeric `MaxAgeSeconds`, is rejected at write time rather than silently matching nothing. CloudFormation's `CorsConfiguration` goes through the same validation, so a template cannot deploy a bucket whose preflights all fail. - **Bucket and object ACLs** — a canned `x-amz-acl`, the `x-amz-grant-*` headers and an `AccessControlPolicy` body are mutually exclusive, as on S3: naming an ACL two ways is `InvalidRequest` rather than one of them silently winning. Canned values are checked against the set S3 accepts for the target (bucket ACLs take `private`, `public-read`, `public-read-write`, `authenticated-read`, `aws-exec-read` and `log-delivery-write`; `bucket-owner-read` and `bucket-owner-full-control` are object-only), and a grant naming no resolvable grantee is rejected instead of being stored as an entry that matches nobody. `emailAddress=` grantees are kept as `AmazonCustomerByEmail` so they round-trip, but they are not resolved to a canonical user and so convey no access. A bucket whose `ObjectOwnership` is `BucketOwnerEnforced` rejects every ACL-setting call, `CreateMultipartUpload` included. From e06f1c7223c49ef18126639c36f91f43cc33af5f Mon Sep 17 00:00:00 2001 From: Lucas Vieira Date: Sun, 27 Sep 2026 19:59:55 -0300 Subject: [PATCH 2/4] fix(s3): close the gaps review found in the per-setting create authorization - `aws-exec-read` escaped the new `s3:PutBucketAcl` requirement. Its READ grant to the EC2 service's canonical user is not modeled, so it resolves to owner-only grants -- while the BucketOwnerEnforced conflict check has always treated it as an ACL reaching outside the owner, and CloudFormation sends it for `AccessControl: AwsExecRead`. Both sites now share one `acl_reaches_past_owner` helper instead of each deriving the answer, which is how they came to disagree. - The four newly required authorizations were evaluated against an EMPTY `aws:RequestTag` context. Dispatch rebuilds the context per action, so a tag-scoped guardrail that permitted the create then denied the `s3:PutBucketAcl` it implies: a 403 on a request AWS allows, since AWS builds one request context for the whole request. - `s3:x-amz-object-ownership` and `s3:x-amz-bucket-object-lock-enabled` are populated too. Gating a setting on a permission while its condition key stays absent is the same hazard this PR was written to fix: a guardrail that matches nothing authorizes exactly what it was written to block. - A doc comment carrying the `#2553` provenance had been displaced onto the wrong test, leaving `s3_create_bucket_with_tags_needs_tag_resource` undocumented, and two comments this change falsified still described the old behavior. Tests, each verified failing without its fix: `aws-exec-read` requires PutBucketAcl; every implied action sees the create's tags (and carries none on its own operation); the two new condition keys are emitted, and skipped when blank. Plus an e2e proving a tag-scoped `Deny` that permits the create does not then deny its ACL. --- crates/fakecloud-e2e/tests/iam_enforcement.rs | 79 ++++++++++++++++++- crates/fakecloud-s3/src/service/buckets.rs | 17 +--- crates/fakecloud-s3/src/service/mod.rs | 77 +++++++++++++----- crates/fakecloud-s3/src/service/tests.rs | 63 ++++++++++++++- website/content/docs/reference/security.md | 2 + 5 files changed, 201 insertions(+), 37 deletions(-) diff --git a/crates/fakecloud-e2e/tests/iam_enforcement.rs b/crates/fakecloud-e2e/tests/iam_enforcement.rs index 0fbccb0e8..f65e18184 100644 --- a/crates/fakecloud-e2e/tests/iam_enforcement.rs +++ b/crates/fakecloud-e2e/tests/iam_enforcement.rs @@ -411,10 +411,6 @@ async fn sns_publish_allowed_on_specific_topic() { // S3 tests // ====================================================================== -/// Real S3 requires `s3:TagResource` on top of `s3:CreateBucket` to create a -/// bucket carrying `CreateBucketConfiguration.Tags` (issue #2553). A grant of -/// `s3:CreateBucket` alone still creates untagged buckets, but a tagged create -/// is denied. /// A create that configures the bucket needs the permission for what it /// configures. This matters because those settings now PERSIST: a `public-read` /// bucket created by a principal with no `s3:PutBucketAcl` used to lose the @@ -583,6 +579,81 @@ async fn s3_create_bucket_with_object_lock_needs_its_permissions() { .unwrap(); } +/// A create carrying both tags and an ACL is several authorizations, and AWS +/// evaluates them all against ONE request context. A tag-scoped guardrail that +/// permits the create must therefore permit the `s3:PutBucketAcl` it implies: +/// building the context per action left that one seeing no `aws:RequestTag/*` +/// at all, so the condition matched and denied a create AWS allows. +#[tokio::test] +async fn s3_create_bucket_tag_condition_applies_to_the_implied_acl_action() { + let server = start_strict().await; + let (akid, secret) = bootstrap_user(&server, "s3ctxuser").await; + attach_inline_policy( + &server, + "s3ctxuser", + "tag-scoped", + r#"{"Version":"2012-10-17","Statement":[ + {"Effect":"Allow","Action":"s3:*","Resource":"*"}, + {"Effect":"Deny", + "Action":["s3:CreateBucket","s3:TagResource","s3:PutBucketAcl"], + "Resource":"*", + "Condition":{"StringNotEquals":{"aws:RequestTag/CostCenter":"123"}}} + ]}"#, + ) + .await; + + let cfg = sdk_config_with(&server, &akid, &secret).await; + let s3 = aws_sdk_s3::Client::new(&cfg); + + // Tagged with the required value AND carrying an ACL: allowed, because + // every action in the request sees `aws:RequestTag/CostCenter=123`. + s3.create_bucket() + .bucket("ctx-tagged") + .acl(aws_sdk_s3::types::BucketCannedAcl::PublicRead) + .create_bucket_configuration( + aws_sdk_s3::types::CreateBucketConfiguration::builder() + .tags( + aws_sdk_s3::types::Tag::builder() + .key("CostCenter") + .value("123") + .build() + .unwrap(), + ) + .build(), + ) + .send() + .await + .expect("a create whose tags satisfy the guardrail must not be denied by its own ACL"); + + // The guardrail still bites when the tag value is wrong. + let err = s3 + .create_bucket() + .bucket("ctx-wrong-tag") + .acl(aws_sdk_s3::types::BucketCannedAcl::PublicRead) + .create_bucket_configuration( + aws_sdk_s3::types::CreateBucketConfiguration::builder() + .tags( + aws_sdk_s3::types::Tag::builder() + .key("CostCenter") + .value("999") + .build() + .unwrap(), + ) + .build(), + ) + .send() + .await + .expect_err("the wrong tag value must still be denied"); + assert!( + format!("{err:?}").contains("AccessDenied"), + "unexpected error: {err:?}" + ); +} + +/// Real S3 requires `s3:TagResource` on top of `s3:CreateBucket` to create a +/// bucket carrying `CreateBucketConfiguration.Tags` (issue #2553). A grant of +/// `s3:CreateBucket` alone still creates untagged buckets, but a tagged create +/// is denied. #[tokio::test] async fn s3_create_bucket_with_tags_needs_tag_resource() { let server = start_strict().await; diff --git a/crates/fakecloud-s3/src/service/buckets.rs b/crates/fakecloud-s3/src/service/buckets.rs index 8b3101431..cbeb0d18e 100644 --- a/crates/fakecloud-s3/src/service/buckets.rs +++ b/crates/fakecloud-s3/src/service/buckets.rs @@ -298,21 +298,10 @@ impl S3Service { // cased value here would refuse an ACL at create that the very next // PutBucketAcl would then happily set. let ownership_enforced = ownership_header == Some("BucketOwnerEnforced"); - let owner_only = |grants: &[crate::state::AclGrant]| { - grants.iter().all(|g| { - g.permission == "FULL_CONTROL" - && g.grantee_type == "CanonicalUser" - && g.grantee_id.as_deref() == Some(req.account_id.as_str()) - }) - }; + // Shared with the `s3:PutBucketAcl` authorization in `iam_actions_for`, + // which asks the same question and must not answer it differently. let acl_requests_grants = grant_headers_present - || acl_header.is_some_and(|a| { - // `aws-exec-read` grants READ to the EC2 service's canonical - // user. That grantee is not modeled, so the resolved grants - // look owner-only -- but the request still asks for an ACL - // reaching outside the owner, which is what conflicts. - a == "aws-exec-read" || !owner_only(&canned_acl_grants(a, &req.account_id)) - }); + || acl_header.is_some_and(|a| super::acl_reaches_past_owner(a, &req.account_id)); if ownership_enforced && acl_requests_grants { return Err(AwsServiceError::aws_error( StatusCode::BAD_REQUEST, diff --git a/crates/fakecloud-s3/src/service/mod.rs b/crates/fakecloud-s3/src/service/mod.rs index a5ffe4bcf..2ce27918b 100644 --- a/crates/fakecloud-s3/src/service/mod.rs +++ b/crates/fakecloud-s3/src/service/mod.rs @@ -1551,18 +1551,12 @@ impl AwsService for S3Service { // request resolves to, so the two spellings of an owner-only ACL // agree -- and so a least-privilege caller that always sends // `--acl private` is not refused. - let acl_reaches_past_owner = request + let reaches_past_owner = request .headers .get("x-amz-acl") .and_then(|v| v.to_str().ok()) - .is_some_and(|acl| { - canned_acl_grants(acl, &request.account_id).iter().any(|g| { - g.permission != "FULL_CONTROL" - || g.grantee_type != "CanonicalUser" - || g.grantee_id.as_deref() != Some(request.account_id.as_str()) - }) - }); - if acl_reaches_past_owner || has_grant_headers(&request.headers) { + .is_some_and(|acl| acl_reaches_past_owner(acl, &request.account_id)); + if reaches_past_owner || has_grant_headers(&request.headers) { extra.push("PutBucketAcl"); } if request.headers.contains_key("x-amz-object-ownership") { @@ -1623,10 +1617,12 @@ impl AwsService for S3Service { /// Extract service-specific IAM condition keys from an S3 request. /// -/// Today only `ListObjects` / `ListObjectsV2` expose keys (`s3:prefix`, -/// `s3:delimiter`, `s3:max-keys`) via their query params. Other actions -/// return an empty map so the evaluator's safe-fail semantics treat any -/// policy condition referencing an unknown key as "doesn't apply". +/// `ListObjects` / `ListObjectsV2` expose `s3:prefix`, `s3:delimiter` and +/// `s3:max-keys` from their query params, and any request carrying the ACL, +/// object-ownership or object-lock headers exposes the matching `s3:x-amz-*` +/// keys. An action with nothing to expose returns an empty map, so the +/// evaluator's safe-fail semantics treat a policy condition on an unknown key +/// as "doesn't apply". fn s3_condition_keys( action: &str, query: &std::collections::HashMap, @@ -1658,6 +1654,21 @@ fn s3_condition_keys( out.insert(format!("s3:{header}"), values); } } + // The ACL is not the only setting gated by `iam_actions_for`: object + // ownership and object lock are too, and a guardrail like + // `Deny CreateBucket unless s3:x-amz-object-ownership == BucketOwnerEnforced` + // is just as useless while its key is never populated. Blank values are + // skipped for the same reason as the grant headers -- present but empty asks + // for nothing, and emitting the key would make a `Null` check read it as set. + for header in ["x-amz-object-ownership", "x-amz-bucket-object-lock-enabled"] { + if let Some(value) = headers + .get(header) + .and_then(|v| v.to_str().ok()) + .filter(|v| !v.trim().is_empty()) + { + out.insert(format!("s3:{header}"), vec![value.to_string()]); + } + } if matches!(action, "ListObjects" | "ListObjectsV2") { // Both list variants share the same query param shape. if let Some(prefix) = query.get("prefix") { @@ -1740,11 +1751,20 @@ fn s3_request_tags( let tags = parse_tagging_xml(body); Some(tags.into_iter().collect()) } - // CreateBucket carries its tag set inside CreateBucketConfiguration, - // and the `s3:TagResource` authorization it requires alongside - // `s3:CreateBucket` is evaluated against the same tags — so both see - // `aws:RequestTag/*` and `aws:TagKeys`. - "CreateBucket" | "TagResource" => { + // CreateBucket carries its tag set inside CreateBucketConfiguration, and + // every authorization it requires alongside `s3:CreateBucket` is + // evaluated against the same tags -- AWS builds one request context for + // the whole request, so an `aws:RequestTag/*` guardrail that permits the + // create must not then deny the ACL or object-lock action it implies. + // The extra names are harmless on their own operations: a real + // PutBucketAcl body is not a `CreateBucketConfiguration`, so it yields + // the same empty map the catch-all arm would. + "CreateBucket" + | "TagResource" + | "PutBucketAcl" + | "PutBucketOwnershipControls" + | "PutBucketObjectLockConfiguration" + | "PutBucketVersioning" => { let body = std::str::from_utf8(&request.body).unwrap_or(""); let tags = create_bucket_configuration_tags(body); Some(tags.into_iter().collect()) @@ -2501,6 +2521,27 @@ pub(crate) const BUCKET_CANNED_ACLS: [&str; 8] = [ "bucket-owner-full-control", ]; +/// Whether the ACL a request asks for reaches past the bucket owner. +/// +/// Both the `BucketOwnerEnforced` conflict check and the `s3:PutBucketAcl` +/// authorization turn on this one question, so they share the answer rather +/// than each re-deriving it -- they disagreed once already, and the IAM side +/// silently authorized an ACL the conflict check treats as reaching outside the +/// owner. +/// +/// `aws-exec-read` is the case that needs saying out loud: its READ grant to +/// the EC2 service's canonical user is not modeled, so [`canned_acl_grants`] +/// resolves it to owner-only -- but the request does ask for an ACL reaching +/// outside the owner. +pub(crate) fn acl_reaches_past_owner(acl: &str, owner_id: &str) -> bool { + acl == "aws-exec-read" + || !canned_acl_grants(acl, owner_id).iter().all(|g| { + g.permission == "FULL_CONTROL" + && g.grantee_type == "CanonicalUser" + && g.grantee_id.as_deref() == Some(owner_id) + }) +} + pub(crate) fn canned_acl_grants(acl: &str, owner_id: &str) -> Vec { let owner_grant = AclGrant { grantee_type: "CanonicalUser".to_string(), diff --git a/crates/fakecloud-s3/src/service/tests.rs b/crates/fakecloud-s3/src/service/tests.rs index 63cd362ad..5ca1f956c 100644 --- a/crates/fakecloud-s3/src/service/tests.rs +++ b/crates/fakecloud-s3/src/service/tests.rs @@ -4365,6 +4365,17 @@ fn create_bucket_requires_a_permission_per_setting_it_configures() { ] ); + // `aws-exec-read` resolves to owner-only grants (its READ to the EC2 + // service's canonical user is not modeled), so judging by grants alone let + // it slip past PutBucketAcl -- while the BucketOwnerEnforced conflict check + // has always treated it as an ACL reaching outside the owner. CloudFormation + // sends it for `AccessControl: AwsExecRead`, so the gap was reachable. + let mut exec_read = make_request(Method::PUT, "/perm-exec", &[], b""); + exec_read + .headers + .insert("x-amz-acl", "aws-exec-read".parse().unwrap()); + assert_eq!(names(&exec_read), vec!["CreateBucket", "PutBucketAcl"]); + // A false object-lock header configures nothing. let mut unlocked = make_request(Method::PUT, "/perm-unlocked", &[], b""); unlocked @@ -4417,9 +4428,35 @@ fn acl_condition_keys_are_populated_from_the_request_headers() { Some(&vec!["id=abc123".to_string()]) ); + // Object ownership and object lock are gated by `iam_actions_for` too, so + // their keys are emitted as well -- `Deny CreateBucket unless + // s3:x-amz-object-ownership == BucketOwnerEnforced` is a guardrail people + // actually write. + let mut settings = HeaderMap::new(); + settings.insert( + "x-amz-object-ownership", + "BucketOwnerEnforced".parse().unwrap(), + ); + settings.insert("x-amz-bucket-object-lock-enabled", "true".parse().unwrap()); + let keys = s3_condition_keys("CreateBucket", &HashMap::new(), &settings); + assert_eq!( + keys.get("s3:x-amz-object-ownership"), + Some(&vec!["BucketOwnerEnforced".to_string()]) + ); + assert_eq!( + keys.get("s3:x-amz-bucket-object-lock-enabled"), + Some(&vec!["true".to_string()]) + ); + // Absent headers emit nothing, so a policy condition on them safe-fails to // "does not apply" rather than matching an empty value. assert!(s3_condition_keys("CreateBucket", &HashMap::new(), &HeaderMap::new()).is_empty()); + + // A present-but-empty value is skipped for the same reason, so a `Null` + // check does not read it as set. + let mut blank = HeaderMap::new(); + blank.insert("x-amz-object-ownership", "".parse().unwrap()); + assert!(s3_condition_keys("CreateBucket", &HashMap::new(), &blank).is_empty()); } #[test] @@ -4472,11 +4509,35 @@ fn create_bucket_request_tags_feed_condition_keys() { &[], b"teama", ); - for action in ["CreateBucket", "TagResource"] { + // Every action the create implies, not just the create and its TagResource: + // dispatch rebuilds the context per action, and AWS evaluates one context + // for the whole request. An `aws:RequestTag/*` guardrail that permits the + // create must not then deny the ACL or object-lock action it implies. + for action in [ + "CreateBucket", + "TagResource", + "PutBucketAcl", + "PutBucketOwnershipControls", + "PutBucketObjectLockConfiguration", + "PutBucketVersioning", + ] { let tags = s3_request_tags(&req, action).expect("tags extracted"); assert_eq!(tags.get("team").map(String::as_str), Some("a"), "{action}"); } + // On their own operations those actions carry no create-time tag set: the + // body is not a `CreateBucketConfiguration`, so the map is empty rather + // than picking up a foreign body's tags. + let acl_body = make_request( + Method::PUT, + "/req-acl?acl", + &[("acl", "")], + b"o", + ); + assert!(s3_request_tags(&acl_body, "PutBucketAcl") + .expect("tags extracted") + .is_empty()); + // A body with no tag set yields an empty map, not a miss. let plain = make_request(Method::PUT, "/req-plain", &[], b""); assert!(s3_request_tags(&plain, "CreateBucket") diff --git a/website/content/docs/reference/security.md b/website/content/docs/reference/security.md index 16b78fd73..b6a29e7a6 100644 --- a/website/content/docs/reference/security.md +++ b/website/content/docs/reference/security.md @@ -132,6 +132,8 @@ Every operator supports the `...IfExists` suffix (missing key evaluates to `true | `s3:max-keys` | `s3:ListObjects`, `s3:ListObjectsV2` | `?max-keys=` query param | | `s3:x-amz-acl` | any request carrying the header (`CreateBucket`, `PutObject`, `CopyObject`, `CreateMultipartUpload`, `PutBucketAcl`, `PutObjectAcl`) | `x-amz-acl` header | | `s3:x-amz-grant-read`, `-write`, `-read-acp`, `-write-acp`, `-full-control` | any request carrying the header | matching `x-amz-grant-*` header | +| `s3:x-amz-object-ownership` | any request carrying the header (`CreateBucket`, `PutBucketOwnershipControls`) | `x-amz-object-ownership` header | +| `s3:x-amz-bucket-object-lock-enabled` | `CreateBucket` | `x-amz-bucket-object-lock-enabled` header | | `sns:Protocol` | `sns:Subscribe` | `Protocol` request parameter | | `sns:Endpoint` | `sns:Subscribe` | `Endpoint` request parameter | | `lambda:FunctionArn` | `lambda:AddPermission` | Target function ARN resolved from the path | From 3df47472afd8c3548b07d4d7501e7826cd7ea93b Mon Sep 17 00:00:00 2001 From: Lucas Vieira Date: Mon, 28 Sep 2026 05:58:18 -0300 Subject: [PATCH 3/4] fix(s3): make the ABAC test able to fail, and skip blank values everywhere The e2e added last commit passed against the bug it was written for. Its Deny used `StringNotEquals`, and an unpopulated condition key safe-fails EVERY non-`IfExists`, non-`ForAllValues` operator to false, so the Deny did not apply on the broken code either and both assertions held with or without the fix. The claim "verified failing without its fix" was true of the unit test and false of this one. `Null` is the one operator that reads an unpopulated key AS null, so it is the one that can see a per-action context that carries no tags. With the Deny rewritten that way, the test fails on the unfixed code and passes on the fixed one -- verified both directions. Blank values, which the same commit handled in two places out of three: - `s3:x-amz-acl` was emitted for a present-but-empty header while both neighbouring families skipped blanks, and while the write paths treat such a header as absent. A `Null`- or `StringEquals`-gated policy therefore denied a request that asks for no ACL at all. - `x-amz-object-ownership` demanded `s3:PutBucketOwnershipControls` on a blank value, whose condition key is deliberately not emitted -- so a `StringEquals`-gated Allow answered 403 where the handler answers 400. Also: `security.md` claimed `s3:x-amz-object-ownership` covers `PutBucketOwnershipControls`, which carries that value in its XML body rather than a header, so the key is never populated there -- the guardrail the row promises would match nothing, which is the hazard this PR exists to remove. The row now says `CreateBucket` and why. And two comments: the `BUCKET_CANNED_ACLS` doc still said `create_bucket` special-cases `aws-exec-read` (it is `acl_reaches_past_owner` now, driving both callers), and `acl_reaches_past_owner` now says explicitly that it is NOT the same question as the object-write check added by #2565, so the two are not "unified" later: a BucketOwnerEnforced bucket refuses an ACL granting another account anything, while a BucketOwnerEnforced bucket accepts an object PUT only with no ACL or bucket-owner-full-control, which is not even a legal bucket ACL. --- crates/fakecloud-e2e/tests/iam_enforcement.rs | 25 ++++++------- crates/fakecloud-s3/src/service/mod.rs | 36 ++++++++++++++++--- crates/fakecloud-s3/src/service/tests.rs | 20 ++++++++++- website/content/docs/reference/security.md | 2 +- 4 files changed, 61 insertions(+), 22 deletions(-) diff --git a/crates/fakecloud-e2e/tests/iam_enforcement.rs b/crates/fakecloud-e2e/tests/iam_enforcement.rs index f65e18184..511a31216 100644 --- a/crates/fakecloud-e2e/tests/iam_enforcement.rs +++ b/crates/fakecloud-e2e/tests/iam_enforcement.rs @@ -584,6 +584,12 @@ async fn s3_create_bucket_with_object_lock_needs_its_permissions() { /// permits the create must therefore permit the `s3:PutBucketAcl` it implies: /// building the context per action left that one seeing no `aws:RequestTag/*` /// at all, so the condition matched and denied a create AWS allows. +/// +/// The Deny is written with `Null`, deliberately. A `StringNotEquals` Deny cannot +/// catch this: an unpopulated key safe-fails every non-`IfExists` operator to +/// false, so such a Deny would not apply on the broken code either and the test +/// would pass against the bug. `Null` is the one operator that reads an +/// unpopulated key AS null, which is exactly the state being asserted against. #[tokio::test] async fn s3_create_bucket_tag_condition_applies_to_the_implied_acl_action() { let server = start_strict().await; @@ -597,7 +603,7 @@ async fn s3_create_bucket_tag_condition_applies_to_the_implied_acl_action() { {"Effect":"Deny", "Action":["s3:CreateBucket","s3:TagResource","s3:PutBucketAcl"], "Resource":"*", - "Condition":{"StringNotEquals":{"aws:RequestTag/CostCenter":"123"}}} + "Condition":{"Null":{"aws:RequestTag/CostCenter":"true"}}} ]}"#, ) .await; @@ -625,25 +631,14 @@ async fn s3_create_bucket_tag_condition_applies_to_the_implied_acl_action() { .await .expect("a create whose tags satisfy the guardrail must not be denied by its own ACL"); - // The guardrail still bites when the tag value is wrong. + // The guardrail still bites: a create carrying no such tag is denied. let err = s3 .create_bucket() - .bucket("ctx-wrong-tag") + .bucket("ctx-untagged") .acl(aws_sdk_s3::types::BucketCannedAcl::PublicRead) - .create_bucket_configuration( - aws_sdk_s3::types::CreateBucketConfiguration::builder() - .tags( - aws_sdk_s3::types::Tag::builder() - .key("CostCenter") - .value("999") - .build() - .unwrap(), - ) - .build(), - ) .send() .await - .expect_err("the wrong tag value must still be denied"); + .expect_err("a create with no CostCenter tag must be denied"); assert!( format!("{err:?}").contains("AccessDenied"), "unexpected error: {err:?}" diff --git a/crates/fakecloud-s3/src/service/mod.rs b/crates/fakecloud-s3/src/service/mod.rs index b3fe0f00d..8eac35dca 100644 --- a/crates/fakecloud-s3/src/service/mod.rs +++ b/crates/fakecloud-s3/src/service/mod.rs @@ -1693,7 +1693,17 @@ impl AwsService for S3Service { if reaches_past_owner || has_grant_headers(&request.headers) { extra.push("PutBucketAcl"); } - if request.headers.contains_key("x-amz-object-ownership") { + // Presence, but blank counts as absent -- the same rule the ACL + // headers and the condition keys follow. Otherwise a blank value + // demands a permission whose condition key is deliberately not + // emitted, so a `StringEquals`-gated Allow 403s a request the handler + // would have answered with a 400. + if request + .headers + .get("x-amz-object-ownership") + .and_then(|v| v.to_str().ok()) + .is_some_and(|v| !v.trim().is_empty()) + { extra.push("PutBucketOwnershipControls"); } if request @@ -1769,7 +1779,14 @@ fn s3_condition_keys( // are never populated. AWS exposes them on every ACL-accepting write, so // they are emitted whenever the request carries them rather than being // gated on an action list that would drift. - if let Some(acl) = headers.get("x-amz-acl").and_then(|v| v.to_str().ok()) { + if let Some(acl) = headers + .get("x-amz-acl") + .and_then(|v| v.to_str().ok()) + // Blank is skipped like every other header here: the write paths treat a + // present-but-empty canned ACL as absent, so emitting the key would let + // `Null`/`StringEquals` guardrails deny a request that asks for no ACL. + .filter(|v| !v.trim().is_empty()) + { out.insert("s3:x-amz-acl".to_string(), vec![acl.to_string()]); } for (header, _) in &GRANT_HEADER_PERMISSIONS { @@ -2633,9 +2650,10 @@ pub(crate) const OBJECT_OWNERSHIP_VALUES: [&str; 3] = [ /// `AWS::S3::Bucket` `AccessControl: AwsExecRead` maps to it), and AWS answers /// 200, so refusing it would turn a working call into a hard failure. Its grant /// is a READ to the EC2 service's canonical user, which this build cannot -/// reproduce, so it resolves to owner-only -- `create_bucket` special-cases it -/// when checking the BucketOwnerEnforced conflict, since the request does ask -/// for an ACL reaching past the owner. +/// reproduce, so it resolves to owner-only -- [`acl_reaches_past_owner`] special- +/// cases it, since the request does ask for an ACL reaching past the owner. That +/// drives both the BucketOwnerEnforced conflict check and the `s3:PutBucketAcl` +/// authorization a create carrying it requires. /// /// `bucket-owner-read` and `bucket-owner-full-control` are object-scoped, and /// the canned-ACL table says S3 IGNORES them when they are given on a bucket @@ -2667,6 +2685,14 @@ pub(crate) const BUCKET_CANNED_ACLS: [&str; 8] = [ /// the EC2 service's canonical user is not modeled, so [`canned_acl_grants`] /// resolves it to owner-only -- but the request does ask for an ACL reaching /// outside the owner. +/// +/// NOT the same question as the object-write check in +/// [`S3Service::resolve_write_acl_headers`], which asks whether the request names +/// bucket-owner-full-control specifically. The two look alike and must not be +/// unified: a BucketOwnerEnforced BUCKET rejects an ACL that grants another +/// account anything, so `private` is fine there, while a BucketOwnerEnforced +/// bucket accepts an object PUT only when it specifies no ACL at all or +/// bucket-owner-full-control -- which is not even a legal bucket ACL. pub(crate) fn acl_reaches_past_owner(acl: &str, owner_id: &str) -> bool { acl == "aws-exec-read" || !canned_acl_grants(acl, owner_id).iter().all(|g| { diff --git a/crates/fakecloud-s3/src/service/tests.rs b/crates/fakecloud-s3/src/service/tests.rs index cb2b4142e..67fc68d41 100644 --- a/crates/fakecloud-s3/src/service/tests.rs +++ b/crates/fakecloud-s3/src/service/tests.rs @@ -4758,6 +4758,17 @@ fn create_bucket_requires_a_permission_per_setting_it_configures() { .insert("x-amz-acl", "aws-exec-read".parse().unwrap()); assert_eq!(names(&exec_read), vec!["CreateBucket", "PutBucketAcl"]); + // A blank value asks for nothing, so it configures nothing either -- the + // condition keys skip blanks, and demanding a permission whose key is absent + // turns a `StringEquals`-gated Allow into a 403 on a request the handler + // answers with a 400. + let mut blank = make_request(Method::PUT, "/perm-blank", &[], b""); + blank.headers.insert("x-amz-acl", "".parse().unwrap()); + blank + .headers + .insert("x-amz-object-ownership", "".parse().unwrap()); + assert_eq!(names(&blank), vec!["CreateBucket"]); + // A false object-lock header configures nothing. let mut unlocked = make_request(Method::PUT, "/perm-unlocked", &[], b""); unlocked @@ -4838,7 +4849,14 @@ fn acl_condition_keys_are_populated_from_the_request_headers() { // check does not read it as set. let mut blank = HeaderMap::new(); blank.insert("x-amz-object-ownership", "".parse().unwrap()); - assert!(s3_condition_keys("CreateBucket", &HashMap::new(), &blank).is_empty()); + blank.insert("x-amz-bucket-object-lock-enabled", "".parse().unwrap()); + blank.insert("x-amz-acl", "".parse().unwrap()); + blank.insert("x-amz-grant-read", "".parse().unwrap()); + assert!( + s3_condition_keys("CreateBucket", &HashMap::new(), &blank).is_empty(), + "blank values must be skipped on EVERY header, not most of them: {:?}", + s3_condition_keys("CreateBucket", &HashMap::new(), &blank) + ); } #[test] diff --git a/website/content/docs/reference/security.md b/website/content/docs/reference/security.md index b6a29e7a6..01882c976 100644 --- a/website/content/docs/reference/security.md +++ b/website/content/docs/reference/security.md @@ -132,7 +132,7 @@ Every operator supports the `...IfExists` suffix (missing key evaluates to `true | `s3:max-keys` | `s3:ListObjects`, `s3:ListObjectsV2` | `?max-keys=` query param | | `s3:x-amz-acl` | any request carrying the header (`CreateBucket`, `PutObject`, `CopyObject`, `CreateMultipartUpload`, `PutBucketAcl`, `PutObjectAcl`) | `x-amz-acl` header | | `s3:x-amz-grant-read`, `-write`, `-read-acp`, `-write-acp`, `-full-control` | any request carrying the header | matching `x-amz-grant-*` header | -| `s3:x-amz-object-ownership` | any request carrying the header (`CreateBucket`, `PutBucketOwnershipControls`) | `x-amz-object-ownership` header | +| `s3:x-amz-object-ownership` | `CreateBucket` | `x-amz-object-ownership` header (`PutBucketOwnershipControls` carries the value in its XML body, not a header, so the key is not populated there) | | `s3:x-amz-bucket-object-lock-enabled` | `CreateBucket` | `x-amz-bucket-object-lock-enabled` header | | `sns:Protocol` | `sns:Subscribe` | `Protocol` request parameter | | `sns:Endpoint` | `sns:Subscribe` | `Endpoint` request parameter | From b5b5d092c3320e894fe69e4b0d329dc0b18db437 Mon Sep 17 00:00:00 2001 From: Lucas Vieira Date: Tue, 29 Sep 2026 08:16:40 -0300 Subject: [PATCH 4/4] fix(s3): stop emitting a condition key AWS does not define `s3:x-amz-bucket-object-lock-enabled` is not an AWS condition key. S3's object-lock keys are `s3:object-lock-mode`, `s3:object-lock-legal-hold`, `s3:object-lock-remaining-retention-days` and `s3:object-lock-retain-until-date`, all about an object's retention rather than a bucket's creation; there is no key for the `x-amz-bucket-object-lock-enabled` header. The vendored model agrees as far as it can: it carries that string only as the HTTP header, and the one `s3:x-amz-*` condition key it names anywhere is `s3:x-amz-metadata-directive`. Emitting it inverted the point of this PR. A policy author writing `Deny CreateBucket unless s3:x-amz-bucket-object-lock-enabled == true` would see it work against fakecloud and silently do nothing against AWS -- the same "guardrail that matches nothing" this PR exists to remove, pointed the other way. The key, its `security.md` row and its assertion are gone; the test now asserts it is NOT emitted. `s3:x-amz-object-ownership` stays: AWS defines that one, for CreateBucket. The object-lock PERMISSIONS requirement is untouched -- that comes from the model's own Permissions text, not from a condition key. Three comments that were false, one of them load-bearing: - The blank-`x-amz-acl` skip justified itself with "the write paths treat a present-but-empty canned ACL as absent". Only one of the four canned-ACL paths did, and #2581 reverts even that, so the justification would have aged into a lie. It now rests on what is actually invariant -- consistency with the other key families -- and says plainly that three paths answer 400 for a blank value, so nobody reads the skip as "blank is accepted". - `iam_actions_for`'s blank comment claimed the `x-amz-acl` branch skipped blanks. It did not; a blank merely resolved to owner-only because `canned_acl_grants` falls through to the owner for anything unrecognized. Now filtered explicitly, so an arm that ever resolved an unknown value past the owner cannot silently start demanding `s3:PutBucketAcl` for an empty header. - The `BUCKET_CANNED_ACLS` doc said `bucket-owner-full-control` is "not even a legal bucket ACL" while the constant it sits beside accepts it. The accurate statement is that the model's `BucketCannedACL` omits it, so the bucket-side question never has that answer -- the header is still accepted and resolves to the owner's FULL_CONTROL. Also: the `security.md` ownership row claimed a `CreateBucket` gate the emitter does not implement (it is populated from the header on any request, like the rows above it). It now says so, and why AWS scopes the key to CreateBucket anyway. One correction to the previous commit message: it claimed an unpopulated condition key safe-fails every non-`IfExists`, non-`ForAllValues` operator. The `ForAllValues` carve-out applies only to a key the service POPULATED with an empty value list; a key that was never populated safe-fails regardless of qualifier. --- crates/fakecloud-s3/src/service/mod.rs | 57 +++++++++++++++------- crates/fakecloud-s3/src/service/tests.rs | 8 +-- website/content/docs/reference/security.md | 3 +- 3 files changed, 44 insertions(+), 24 deletions(-) diff --git a/crates/fakecloud-s3/src/service/mod.rs b/crates/fakecloud-s3/src/service/mod.rs index 8eac35dca..02c712c12 100644 --- a/crates/fakecloud-s3/src/service/mod.rs +++ b/crates/fakecloud-s3/src/service/mod.rs @@ -1689,6 +1689,12 @@ impl AwsService for S3Service { .headers .get("x-amz-acl") .and_then(|v| v.to_str().ok()) + // Blank counts as absent here too. It resolves to owner-only today + // only because `canned_acl_grants` falls through to the owner for + // any unrecognized value -- so without this, an arm that ever made + // an unknown value resolve past the owner would silently start + // demanding `s3:PutBucketAcl` for an empty header. + .filter(|acl| !acl.trim().is_empty()) .is_some_and(|acl| acl_reaches_past_owner(acl, &request.account_id)); if reaches_past_owner || has_grant_headers(&request.headers) { extra.push("PutBucketAcl"); @@ -1782,9 +1788,12 @@ fn s3_condition_keys( if let Some(acl) = headers .get("x-amz-acl") .and_then(|v| v.to_str().ok()) - // Blank is skipped like every other header here: the write paths treat a - // present-but-empty canned ACL as absent, so emitting the key would let - // `Null`/`StringEquals` guardrails deny a request that asks for no ACL. + // Blank is skipped for consistency with every other key family here (the + // grant headers, object ownership): a present-but-empty header names no + // value, and a key emitted as `""` matches nothing a policy can sensibly + // write. Note this does NOT mean the request succeeds -- three of the four + // canned-ACL paths answer 400 for a blank value; it means the policy + // decision is not made on an empty string. .filter(|v| !v.trim().is_empty()) { out.insert("s3:x-amz-acl".to_string(), vec![acl.to_string()]); @@ -1805,20 +1814,29 @@ fn s3_condition_keys( out.insert(format!("s3:{header}"), values); } } - // The ACL is not the only setting gated by `iam_actions_for`: object - // ownership and object lock are too, and a guardrail like - // `Deny CreateBucket unless s3:x-amz-object-ownership == BucketOwnerEnforced` - // is just as useless while its key is never populated. Blank values are - // skipped for the same reason as the grant headers -- present but empty asks - // for nothing, and emitting the key would make a `Null` check read it as set. - for header in ["x-amz-object-ownership", "x-amz-bucket-object-lock-enabled"] { - if let Some(value) = headers - .get(header) - .and_then(|v| v.to_str().ok()) - .filter(|v| !v.trim().is_empty()) - { - out.insert(format!("s3:{header}"), vec![value.to_string()]); - } + // The ACL is not the only setting `iam_actions_for` gates: object ownership is + // too, and `Deny CreateBucket unless s3:x-amz-object-ownership == + // BucketOwnerEnforced` is just as useless while its key is never populated. + // + // Object lock is deliberately NOT here. AWS defines no condition key for the + // `x-amz-bucket-object-lock-enabled` header -- its object-lock keys are + // `s3:object-lock-mode`, `-legal-hold`, `-remaining-retention-days` and + // `-retain-until-date`, all about an object's retention rather than a bucket's + // creation. Emitting `s3:x-amz-bucket-object-lock-enabled` would invert the + // point of populating keys at all: a guardrail written against it would work + // here and be a permanent no-op against AWS. The permission the lock header + // requires is unaffected -- that comes from the model's Permissions text. + if let Some(value) = headers + .get("x-amz-object-ownership") + .and_then(|v| v.to_str().ok()) + // Blank is skipped like the grant family: present but empty asks for + // nothing, and emitting the key would make a `Null` check read it as set. + .filter(|v| !v.trim().is_empty()) + { + out.insert( + "s3:x-amz-object-ownership".to_string(), + vec![value.to_string()], + ); } if matches!(action, "ListObjects" | "ListObjectsV2") { // Both list variants share the same query param shape. @@ -2692,7 +2710,10 @@ pub(crate) const BUCKET_CANNED_ACLS: [&str; 8] = [ /// unified: a BucketOwnerEnforced BUCKET rejects an ACL that grants another /// account anything, so `private` is fine there, while a BucketOwnerEnforced /// bucket accepts an object PUT only when it specifies no ACL at all or -/// bucket-owner-full-control -- which is not even a legal bucket ACL. +/// bucket-owner-full-control -- a value that is object-scoped in the model +/// (`com.amazonaws.s3#BucketCannedACL` omits it), so the bucket-side question +/// never has this answer. The header is still accepted on a bucket, resolving to +/// the owner's FULL_CONTROL, which is what S3 ignoring it produces. pub(crate) fn acl_reaches_past_owner(acl: &str, owner_id: &str) -> bool { acl == "aws-exec-read" || !canned_acl_grants(acl, owner_id).iter().all(|g| { diff --git a/crates/fakecloud-s3/src/service/tests.rs b/crates/fakecloud-s3/src/service/tests.rs index 67fc68d41..5f98c876c 100644 --- a/crates/fakecloud-s3/src/service/tests.rs +++ b/crates/fakecloud-s3/src/service/tests.rs @@ -4836,10 +4836,10 @@ fn acl_condition_keys_are_populated_from_the_request_headers() { keys.get("s3:x-amz-object-ownership"), Some(&vec!["BucketOwnerEnforced".to_string()]) ); - assert_eq!( - keys.get("s3:x-amz-bucket-object-lock-enabled"), - Some(&vec!["true".to_string()]) - ); + // AWS defines no condition key for the object-lock header, so emitting one + // would give policy authors a guardrail that works here and silently does + // nothing on AWS. + assert_eq!(keys.get("s3:x-amz-bucket-object-lock-enabled"), None); // Absent headers emit nothing, so a policy condition on them safe-fails to // "does not apply" rather than matching an empty value. diff --git a/website/content/docs/reference/security.md b/website/content/docs/reference/security.md index 01882c976..9e0d4967f 100644 --- a/website/content/docs/reference/security.md +++ b/website/content/docs/reference/security.md @@ -132,8 +132,7 @@ Every operator supports the `...IfExists` suffix (missing key evaluates to `true | `s3:max-keys` | `s3:ListObjects`, `s3:ListObjectsV2` | `?max-keys=` query param | | `s3:x-amz-acl` | any request carrying the header (`CreateBucket`, `PutObject`, `CopyObject`, `CreateMultipartUpload`, `PutBucketAcl`, `PutObjectAcl`) | `x-amz-acl` header | | `s3:x-amz-grant-read`, `-write`, `-read-acp`, `-write-acp`, `-full-control` | any request carrying the header | matching `x-amz-grant-*` header | -| `s3:x-amz-object-ownership` | `CreateBucket` | `x-amz-object-ownership` header (`PutBucketOwnershipControls` carries the value in its XML body, not a header, so the key is not populated there) | -| `s3:x-amz-bucket-object-lock-enabled` | `CreateBucket` | `x-amz-bucket-object-lock-enabled` header | +| `s3:x-amz-object-ownership` | any request carrying the header, which in practice is `CreateBucket` | `x-amz-object-ownership` header. AWS defines this key for `CreateBucket` only; `PutBucketOwnershipControls` carries the value in its XML body rather than a header, so no key is populated there | | `sns:Protocol` | `sns:Subscribe` | `Protocol` request parameter | | `sns:Endpoint` | `sns:Subscribe` | `Endpoint` request parameter | | `lambda:FunctionArn` | `lambda:AddPermission` | Target function ARN resolved from the path |