From 477a37c7851e30aa1d9762425fb76515d4a8c72c Mon Sep 17 00:00:00 2001 From: Lucas Vieira Date: Thu, 24 Sep 2026 17:31:22 -0300 Subject: [PATCH 1/6] fix(s3): honor Tags in CreateBucketConfiguration CreateBucket accepted the `Tags` tag set that was added to CreateBucketConfiguration in the 2025 S3 API, but dropped it: only LocationConstraint was parsed out of the body, so the bucket was created untagged and GetBucketTagging answered NoSuchTagSet. That path matters for Terraform. The AWS provider (6.x) tags a bucket this way on create and skips PutBucketTagging when the create succeeds, so an `aws_s3_bucket` with `tags` applied cleanly and then read back `tags = {}`. - parse the `` tag set out of CreateBucketConfiguration with the same parse_tagging_xml the PutBucketTagging path uses, and store it on the new bucket - run validate_tags first, so duplicate keys and `aws:`-prefixed keys fail with InvalidTag before any bucket is created - write the Tags subresource alongside the bucket meta, since bucket tags are not part of BucketMeta and would otherwise be lost on restart - unit tests for the stored tag set, the no-tags case, InvalidTag rejection leaving no bucket behind, and XML-entity round-tripping - e2e coverage for create-with-tags (plus PutBucketTagging still replacing the create-time set), the duplicate-key rejection, and tags surviving a restart in persistent mode - document the behavior on the S3 service page Fixes #2553 Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_015paJopoDarw6WeFFoZMDMf --- crates/fakecloud-e2e/tests/s3.rs | 83 +++++++++++++++++++ crates/fakecloud-e2e/tests/s3_persistence.rs | 43 +++++++++- crates/fakecloud-s3/src/service/buckets.rs | 31 ++++++- crates/fakecloud-s3/src/service/tests.rs | 85 ++++++++++++++++++++ website/content/docs/services/s3.md | 1 + 5 files changed, 239 insertions(+), 4 deletions(-) diff --git a/crates/fakecloud-e2e/tests/s3.rs b/crates/fakecloud-e2e/tests/s3.rs index a5e1bd303..d152f782f 100644 --- a/crates/fakecloud-e2e/tests/s3.rs +++ b/crates/fakecloud-e2e/tests/s3.rs @@ -2851,6 +2851,89 @@ async fn s3_bucket_tagging() { .unwrap(); } +#[tokio::test] +async fn s3_create_bucket_with_tags_in_configuration() { + // CreateBucketConfiguration.Tags — the AWS Terraform provider tags a bucket + // this way on create and then skips PutBucketTagging, so dropping the tag + // set leaves `aws_s3_bucket` reading back `tags = {}` (issue #2553). + let server = TestServer::start().await; + let client = server.s3_client().await; + + use aws_sdk_s3::types::{CreateBucketConfiguration, Tag}; + client + .create_bucket() + .bucket("btag-on-create") + .create_bucket_configuration( + CreateBucketConfiguration::builder() + .tags(Tag::builder().key("team").value("a").build().unwrap()) + .tags(Tag::builder().key("env").value("prod").build().unwrap()) + .build(), + ) + .send() + .await + .unwrap(); + + let resp = client + .get_bucket_tagging() + .bucket("btag-on-create") + .send() + .await + .unwrap(); + let ts = resp.tag_set(); + assert_eq!(ts.len(), 2, "tag set: {ts:?}"); + assert!(ts.iter().any(|t| t.key() == "team" && t.value() == "a")); + assert!(ts.iter().any(|t| t.key() == "env" && t.value() == "prod")); + + // PutBucketTagging still replaces the create-time set wholesale. + use aws_sdk_s3::types::Tagging; + client + .put_bucket_tagging() + .bucket("btag-on-create") + .tagging( + Tagging::builder() + .tag_set(Tag::builder().key("team").value("b").build().unwrap()) + .build() + .unwrap(), + ) + .send() + .await + .unwrap(); + let resp = client + .get_bucket_tagging() + .bucket("btag-on-create") + .send() + .await + .unwrap(); + assert_eq!(resp.tag_set().len(), 1); + assert_eq!(resp.tag_set()[0].value(), "b"); +} + +#[tokio::test] +async fn s3_create_bucket_with_duplicate_tag_keys_is_rejected() { + let server = TestServer::start().await; + let client = server.s3_client().await; + + use aws_sdk_s3::types::{CreateBucketConfiguration, Tag}; + let err = client + .create_bucket() + .bucket("btag-dup-keys") + .create_bucket_configuration( + CreateBucketConfiguration::builder() + .tags(Tag::builder().key("team").value("a").build().unwrap()) + .tags(Tag::builder().key("team").value("b").build().unwrap()) + .build(), + ) + .send() + .await + .expect_err("duplicate tag keys must be rejected"); + let msg = format!("{err:?}"); + assert!(msg.contains("InvalidTag"), "unexpected error: {msg}"); + + // The rejected create must not have left a bucket behind. + let head = client.head_bucket().bucket("btag-dup-keys").send().await; + assert!(head.is_err(), "bucket created despite InvalidTag"); +} + // ---- S3 ACL Tests ---- #[tokio::test] diff --git a/crates/fakecloud-e2e/tests/s3_persistence.rs b/crates/fakecloud-e2e/tests/s3_persistence.rs index 97d4ca75a..89d259ca6 100644 --- a/crates/fakecloud-e2e/tests/s3_persistence.rs +++ b/crates/fakecloud-e2e/tests/s3_persistence.rs @@ -5,9 +5,10 @@ use std::time::Duration; use aws_sdk_s3::primitives::ByteStream; use aws_sdk_s3::types::{ BucketVersioningStatus, CompletedMultipartUpload, CompletedPart, CorsConfiguration, CorsRule, - ObjectLockConfiguration, ObjectLockEnabled, ObjectLockLegalHold, ObjectLockLegalHoldStatus, - ServerSideEncryption, ServerSideEncryptionByDefault, ServerSideEncryptionConfiguration, - ServerSideEncryptionRule, StorageClass, Tag, Tagging, VersioningConfiguration, + CreateBucketConfiguration, ObjectLockConfiguration, ObjectLockEnabled, ObjectLockLegalHold, + ObjectLockLegalHoldStatus, ServerSideEncryption, ServerSideEncryptionByDefault, + ServerSideEncryptionConfiguration, ServerSideEncryptionRule, StorageClass, Tag, Tagging, + VersioningConfiguration, }; use helpers::{run_until_exit, TestServer}; use sha2::{Digest, Sha256}; @@ -1263,3 +1264,39 @@ async fn persistence_body_cache_small_and_large_objects() { assert_eq!(sha256(&got), sha256(expected), "body mismatch for {}", key); } } + +#[tokio::test] +async fn persistence_create_bucket_tags_survive_restart() { + // Tags supplied through CreateBucketConfiguration go to the same Tags + // subresource PutBucketTagging writes, so they must reload after a restart. + let tmp = tempfile::tempdir().unwrap(); + let mut server = TestServer::start_persistent(tmp.path()).await; + let client = server.s3_client().await; + + client + .create_bucket() + .bucket("create-tag-bucket") + .create_bucket_configuration( + CreateBucketConfiguration::builder() + .tags(Tag::builder().key("team").value("a").build().unwrap()) + .build(), + ) + .send() + .await + .unwrap(); + + server.restart().await; + let client = server.s3_client().await; + + let tags = client + .get_bucket_tagging() + .bucket("create-tag-bucket") + .send() + .await + .unwrap(); + let ts = tags.tag_set(); + assert!( + ts.iter().any(|t| t.key() == "team" && t.value() == "a"), + "create-time tags lost across restart: {ts:?}" + ); +} diff --git a/crates/fakecloud-s3/src/service/buckets.rs b/crates/fakecloud-s3/src/service/buckets.rs index 7d574ec58..dce64109c 100644 --- a/crates/fakecloud-s3/src/service/buckets.rs +++ b/crates/fakecloud-s3/src/service/buckets.rs @@ -5,13 +5,14 @@ use http::{HeaderMap, StatusCode}; use bytes::Bytes; use fakecloud_aws::arn::Arn; use fakecloud_core::service::{AwsRequest, AwsResponse, AwsServiceError}; +use fakecloud_persistence::{BucketSubresource, TagsSnapshot}; use crate::persistence::bucket_meta_snapshot; use crate::state::S3Bucket; use super::{ canned_acl_grants, extract_xml_value, is_valid_bucket_name, is_valid_region, no_such_bucket, - s3_xml, xml_escape, S3Service, + parse_tagging_xml, s3_xml, validate_tags, xml_escape, S3Service, }; impl S3Service { @@ -214,6 +215,17 @@ impl S3Service { _ => req.region.clone(), }; + // CreateBucketConfiguration carries an optional tag set (added to + // the S3 API in 2025). The AWS Terraform provider tags a bucket this way + // on create and then skips PutBucketTagging, so dropping these leaves the + // bucket untagged (issue #2553). + let create_tags = if has_config_body && body_str.contains("") { + parse_tagging_xml(body_str) + } else { + Vec::new() + }; + validate_tags(&create_tags)?; + // Parse ACL from header let acl = req .headers @@ -290,11 +302,28 @@ impl S3Service { )); } + let tags_snapshot = if create_tags.is_empty() { + None + } else { + b.tags = create_tags.into_iter().collect(); + Some(TagsSnapshot { + tags: b.tags.clone(), + }) + }; + let meta = bucket_meta_snapshot(&b); state.buckets.insert(bucket.to_string(), b); self.store .put_bucket_meta(bucket, &meta) .map_err(super::persistence_error)?; + // Bucket tags live in their own subresource, not in the bucket meta, so + // they need an explicit write to survive a restart. + if let Some(snap) = tags_snapshot { + let payload = toml::to_string(&snap).unwrap_or_default(); + self.store + .put_bucket_subresource(bucket, BucketSubresource::Tags, &payload) + .map_err(super::persistence_error)?; + } let mut headers = HeaderMap::new(); headers.insert("location", format!("/{bucket}").parse().unwrap()); diff --git a/crates/fakecloud-s3/src/service/tests.rs b/crates/fakecloud-s3/src/service/tests.rs index dcfe917ae..96cc896e8 100644 --- a/crates/fakecloud-s3/src/service/tests.rs +++ b/crates/fakecloud-s3/src/service/tests.rs @@ -3473,6 +3473,91 @@ fn create_bucket_idempotent_same_region_us_east_1() { assert_eq!(resp.status, StatusCode::OK); } +#[test] +fn create_bucket_stores_tags_from_create_bucket_configuration() { + let svc = make_service(); + let req = make_request( + Method::PUT, + "/tagged", + &[], + b"teama\ + envprod", + ); + svc.create_bucket("123456789012", &req, "tagged").unwrap(); + + let get = make_request(Method::GET, "/tagged", &[("tagging", "")], b""); + let resp = svc + .get_bucket_tagging("123456789012", &get, "tagged") + .unwrap(); + let body = std::str::from_utf8(resp.body.expect_bytes()).unwrap(); + assert!( + body.contains("envprod"), + "tag set missing env: {body}" + ); + assert!( + body.contains("teama"), + "tag set missing team: {body}" + ); +} + +#[test] +fn create_bucket_without_tags_has_no_tag_set() { + let svc = make_service(); + let req = make_request( + Method::PUT, + "/untagged", + &[], + b"", + ); + svc.create_bucket("123456789012", &req, "untagged").unwrap(); + + let get = make_request(Method::GET, "/untagged", &[("tagging", "")], b""); + assert_aws_err( + svc.get_bucket_tagging("123456789012", &get, "untagged"), + "NoSuchTagSet", + ); +} + +#[test] +fn create_bucket_rejects_duplicate_tag_keys() { + let svc = make_service(); + let req = make_request( + Method::PUT, + "/dup-tags", + &[], + b"teama\ + teamb", + ); + assert_aws_err( + svc.create_bucket("123456789012", &req, "dup-tags"), + "InvalidTag", + ); + // The rejected request must not have left a bucket behind. + assert_aws_err(svc.head_bucket("123456789012", "dup-tags"), "NotFound"); +} + +#[test] +fn create_bucket_tags_decode_xml_entities() { + let svc = make_service(); + let req = make_request( + Method::PUT, + "/amp-tags", + &[], + b"a&bc&d", + ); + svc.create_bucket("123456789012", &req, "amp-tags").unwrap(); + + let get = make_request(Method::GET, "/amp-tags", &[("tagging", "")], b""); + let resp = svc + .get_bucket_tagging("123456789012", &get, "amp-tags") + .unwrap(); + let body = std::str::from_utf8(resp.body.expect_bytes()).unwrap(); + assert!( + body.contains("a&bc&d"), + "entity round-trip wrong: {body}" + ); +} + #[test] fn create_bucket_already_owned_other_region() { let svc = make_service(); diff --git a/website/content/docs/services/s3.md b/website/content/docs/services/s3.md index 75546accf..819a0dbba 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 +- **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. - **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. - **Object Lock** — legal hold, retention modes - **Website hosting** — index/error documents, redirect rules From a769f6902799e68e7ac2155b667a16214da01c85 Mon Sep 17 00:00:00 2001 From: Lucas Vieira Date: Thu, 24 Sep 2026 17:53:13 -0300 Subject: [PATCH 2/6] fix(s3): authorize a tagged CreateBucket as s3:TagResource too Tagging a bucket at create time was authorized as nothing but s3:CreateBucket. The S3 model is explicit on CreateBucketConfiguration$Tags: "You must have the s3:TagResource permission to create a general purpose bucket with tags", so a principal granted s3:CreateBucket and denied s3:TagResource could tag a bucket that real S3 would refuse to create. Fixing that surfaced a second bug one layer down. The AWS SDKs send CreateBucket as `PUT //`, and dispatch's streaming heuristic read the trailing slash as an object upload, so the body was handed to the handler unbuffered. Everything that runs before the handler therefore saw an empty body: IAM enforcement could not read CreateBucketConfiguration at all, which is why a policy conditioned on the create-time tag set never matched. The handler itself was unaffected -- it drains the stream into `req.body` for non-object PUTs, which is why the tags applied even though the authorization layer could not see them. - streaming_route now requires a non-empty key after the bucket on the path-style route, so `PUT //` is buffered like `PUT /` - S3 overrides iam_actions_for to append s3:TagResource when the create body carries a tag set; dispatch already requires every returned action to be allowed, so an untagged create still needs only s3:CreateBucket - s3_request_tags covers CreateBucket and TagResource, so the create-time tag set populates aws:RequestTag/ and aws:TagKeys - create_bucket_configuration_tags replaces the handler's ad-hoc `contains("")` gate, which would have silently dropped the tag set from a client that wrote `` or any attribute on the element - unit tests for the action pair, the condition-key extraction, and the helper's gating; e2e tests for the TagResource requirement and for an aws:RequestTag-conditioned create; a dispatch regression test for the trailing-slash route Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_015paJopoDarw6WeFFoZMDMf --- crates/fakecloud-core/src/dispatch.rs | 23 ++- crates/fakecloud-e2e/tests/iam_enforcement.rs | 140 ++++++++++++++++++ crates/fakecloud-s3/src/service/buckets.rs | 10 +- crates/fakecloud-s3/src/service/mod.rs | 50 +++++++ crates/fakecloud-s3/src/service/tests.rs | 80 ++++++++++ website/content/docs/reference/security.md | 2 +- website/content/docs/services/s3.md | 2 +- 7 files changed, 297 insertions(+), 10 deletions(-) diff --git a/crates/fakecloud-core/src/dispatch.rs b/crates/fakecloud-core/src/dispatch.rs index d677aff97..7e8ef9cc4 100644 --- a/crates/fakecloud-core/src/dispatch.rs +++ b/crates/fakecloud-core/src/dispatch.rs @@ -1133,7 +1133,15 @@ fn streaming_route( let virtual_hosted_s3 = protocol::parse_routing_host_from_headers(headers) .filter(|h| h.service == "s3" && h.bucket.is_some()) .is_some(); - if after.is_empty() || (!virtual_hosted_s3 && !after.contains('/')) { + // Path-style needs a non-empty key after the bucket: `PUT /` + // and `PUT //` are both CreateBucket, and the trailing-slash + // form is what the AWS SDKs actually send. Treating it as an object + // upload left the body unbuffered, so everything that runs before the + // handler -- IAM enforcement above all -- saw an empty body and could + // not read `CreateBucketConfiguration` (no `aws:RequestTag` from a + // create-time tag set, for one). + let path_style_key = after.split_once('/').map(|(_, key)| key).unwrap_or(""); + if after.is_empty() || (!virtual_hosted_s3 && path_style_key.is_empty()) { return None; } let header_s3 = headers @@ -1997,6 +2005,19 @@ mod tests { ); } + #[test] + fn streaming_route_path_style_create_bucket_with_trailing_slash_skipped() { + // The AWS SDKs send CreateBucket as `PUT //`. The trailing + // slash must not make it look like an object upload: the body carries + // `CreateBucketConfiguration` (location constraint, tag set) and has + // to be buffered before IAM enforcement reads it. + let headers = s3_sigv4_headers(); + assert_eq!( + streaming_route(&http::Method::PUT, "/my-bucket/", &headers, &HashMap::new(),), + None, + ); + } + #[test] fn streaming_route_virtual_hosted_s3_put_object() { let mut headers = s3_sigv4_headers(); diff --git a/crates/fakecloud-e2e/tests/iam_enforcement.rs b/crates/fakecloud-e2e/tests/iam_enforcement.rs index 8a97dfff5..22d6da708 100644 --- a/crates/fakecloud-e2e/tests/iam_enforcement.rs +++ b/crates/fakecloud-e2e/tests/iam_enforcement.rs @@ -411,6 +411,146 @@ 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. +#[tokio::test] +async fn s3_create_bucket_with_tags_needs_tag_resource() { + let server = start_strict().await; + let (akid, secret) = bootstrap_user(&server, "s3taguser").await; + attach_inline_policy( + &server, + "s3taguser", + "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); + + // Untagged create is allowed by s3:CreateBucket alone. + s3.create_bucket() + .bucket("tagperm-untagged") + .send() + .await + .unwrap(); + + // The same grant must not authorize a tagged create. + let err = s3 + .create_bucket() + .bucket("tagperm-denied") + .create_bucket_configuration( + aws_sdk_s3::types::CreateBucketConfiguration::builder() + .tags( + aws_sdk_s3::types::Tag::builder() + .key("team") + .value("a") + .build() + .unwrap(), + ) + .build(), + ) + .send() + .await + .unwrap_err(); + let msg = format!("{err:?}"); + assert!( + msg.contains("AccessDenied"), + "expected AccessDenied for tagged create, got {msg}" + ); + + // Adding s3:TagResource unblocks it, and the bucket comes out tagged. + attach_inline_policy( + &server, + "s3taguser", + "create-and-tag", + r#"{"Version":"2012-10-17","Statement":[ + {"Effect":"Allow","Action":["s3:CreateBucket","s3:TagResource","s3:GetBucketTagging"],"Resource":"*"} + ]}"#, + ) + .await; + s3.create_bucket() + .bucket("tagperm-allowed") + .create_bucket_configuration( + aws_sdk_s3::types::CreateBucketConfiguration::builder() + .tags( + aws_sdk_s3::types::Tag::builder() + .key("team") + .value("a") + .build() + .unwrap(), + ) + .build(), + ) + .send() + .await + .unwrap(); + let tags = s3 + .get_bucket_tagging() + .bucket("tagperm-allowed") + .send() + .await + .unwrap(); + assert!(tags + .tag_set() + .iter() + .any(|t| t.key() == "team" && t.value() == "a")); +} + +/// The create-time tag set feeds `aws:RequestTag/*`, so a policy conditioned +/// on the tag value actually decides the create. +#[tokio::test] +async fn s3_create_bucket_tags_drive_request_tag_condition() { + let server = start_strict().await; + let (akid, secret) = bootstrap_user(&server, "s3reqtaguser").await; + attach_inline_policy( + &server, + "s3reqtaguser", + "team-a-only", + r#"{"Version":"2012-10-17","Statement":[ + {"Effect":"Allow","Action":["s3:CreateBucket","s3:TagResource"],"Resource":"*", + "Condition":{"StringEquals":{"aws:RequestTag/team":"a"}}} + ]}"#, + ) + .await; + + let cfg = sdk_config_with(&server, &akid, &secret).await; + let s3 = aws_sdk_s3::Client::new(&cfg); + let create_with_team = |bucket: &'static str, team: &'static str| { + let s3 = s3.clone(); + async move { + s3.create_bucket() + .bucket(bucket) + .create_bucket_configuration( + aws_sdk_s3::types::CreateBucketConfiguration::builder() + .tags( + aws_sdk_s3::types::Tag::builder() + .key("team") + .value(team) + .build() + .unwrap(), + ) + .build(), + ) + .send() + .await + } + }; + + create_with_team("reqtag-allowed", "a").await.unwrap(); + + let err = create_with_team("reqtag-denied", "b").await.unwrap_err(); + let msg = format!("{err:?}"); + assert!( + msg.contains("AccessDenied"), + "expected AccessDenied for the non-matching tag value, got {msg}" + ); +} + #[tokio::test] async fn s3_get_object_resource_scoped() { let server = start_strict().await; diff --git a/crates/fakecloud-s3/src/service/buckets.rs b/crates/fakecloud-s3/src/service/buckets.rs index dce64109c..73ca15452 100644 --- a/crates/fakecloud-s3/src/service/buckets.rs +++ b/crates/fakecloud-s3/src/service/buckets.rs @@ -11,8 +11,8 @@ use crate::persistence::bucket_meta_snapshot; use crate::state::S3Bucket; use super::{ - canned_acl_grants, extract_xml_value, is_valid_bucket_name, is_valid_region, no_such_bucket, - parse_tagging_xml, s3_xml, validate_tags, xml_escape, S3Service, + canned_acl_grants, create_bucket_configuration_tags, extract_xml_value, is_valid_bucket_name, + is_valid_region, no_such_bucket, s3_xml, validate_tags, xml_escape, S3Service, }; impl S3Service { @@ -219,11 +219,7 @@ impl S3Service { // the S3 API in 2025). The AWS Terraform provider tags a bucket this way // on create and then skips PutBucketTagging, so dropping these leaves the // bucket untagged (issue #2553). - let create_tags = if has_config_body && body_str.contains("") { - parse_tagging_xml(body_str) - } else { - Vec::new() - }; + let create_tags = create_bucket_configuration_tags(body_str); validate_tags(&create_tags)?; // Parse ACL from header diff --git a/crates/fakecloud-s3/src/service/mod.rs b/crates/fakecloud-s3/src/service/mod.rs index 6646b91bd..850c2426e 100644 --- a/crates/fakecloud-s3/src/service/mod.rs +++ b/crates/fakecloud-s3/src/service/mod.rs @@ -1453,6 +1453,31 @@ impl AwsService for S3Service { }) } + /// Tagging a bucket at create time needs `s3:TagResource` as well as + /// `s3:CreateBucket` — the S3 model says so on + /// `CreateBucketConfiguration$Tags` ("You must have the + /// `s3:TagResource` permission to create a general purpose bucket with + /// tags"). Dispatch requires every returned action to be allowed, so a + /// principal granted only `s3:CreateBucket` can still create an untagged + /// bucket but not a tagged one, as on AWS. + fn iam_actions_for(&self, request: &AwsRequest) -> Vec { + let Some(action) = self.iam_action_for(request) else { + return Vec::new(); + }; + if action.service == "s3" && action.action == "CreateBucket" { + 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]; + } + } + vec![action] + } + fn iam_condition_keys_for( &self, request: &AwsRequest, @@ -1572,6 +1597,15 @@ 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" => { + let body = std::str::from_utf8(&request.body).unwrap_or(""); + let tags = create_bucket_configuration_tags(body); + Some(tags.into_iter().collect()) + } _ => Some(std::collections::BTreeMap::new()), } } @@ -2940,6 +2974,22 @@ pub(crate) fn parse_delete_objects_quiet(xml: &str) -> bool { .unwrap_or(false) } +/// Extract the `` tag set from a `CreateBucketConfiguration` body. +/// +/// The tag set was added to `CreateBucketConfiguration` in the 2025 S3 API; +/// it is serialized as ``, the same +/// `Tag` shape `PutBucketTagging` uses. Returns an empty vec for a body that +/// is not a `CreateBucketConfiguration` or carries no tags — no `` +/// element means no `` elements to find, so the scan is its own gate +/// and an element written with a namespace or attribute +/// (``) is not silently skipped. +pub(crate) fn create_bucket_configuration_tags(body: &str) -> Vec<(String, String)> { + if body.is_empty() || !body.contains("CreateBucketConfiguration") { + return Vec::new(); + } + parse_tagging_xml(body) +} + /// Minimal XML parser for `kv...`. /// Returns a Vec to preserve insertion order and detect duplicates. pub(crate) fn parse_tagging_xml(xml: &str) -> Vec<(String, String)> { diff --git a/crates/fakecloud-s3/src/service/tests.rs b/crates/fakecloud-s3/src/service/tests.rs index 96cc896e8..9e4aded32 100644 --- a/crates/fakecloud-s3/src/service/tests.rs +++ b/crates/fakecloud-s3/src/service/tests.rs @@ -3558,6 +3558,86 @@ fn create_bucket_tags_decode_xml_entities() { ); } +#[test] +fn create_bucket_with_tags_also_requires_tag_resource() { + use fakecloud_core::service::AwsService as _; + + let svc = make_service(); + let tagged = make_request( + Method::PUT, + "/tagged-perm", + &[], + b"teama", + ); + let actions = svc.iam_actions_for(&tagged); + let names: Vec<&str> = actions.iter().map(|a| a.action).collect(); + assert_eq!(names, vec!["CreateBucket", "TagResource"]); + // Both are authorized against the same bucket ARN. + assert_eq!(actions[0].resource, actions[1].resource); + assert_eq!(actions[1].service, "s3"); + + // An untagged create still needs only s3:CreateBucket. + let plain = make_request(Method::PUT, "/plain-perm", &[], b""); + let names: Vec<&str> = svc + .iam_actions_for(&plain) + .iter() + .map(|a| a.action) + .collect(); + assert_eq!(names, vec!["CreateBucket"]); + + // ...as does a CreateBucketConfiguration that carries no tag set. + let loc_only = make_request( + Method::PUT, + "/loc-perm", + &[], + b"eu-west-1", + ); + let names: Vec<&str> = svc + .iam_actions_for(&loc_only) + .iter() + .map(|a| a.action) + .collect(); + assert_eq!(names, vec!["CreateBucket"]); +} + +#[test] +fn create_bucket_request_tags_feed_condition_keys() { + let req = make_request( + Method::PUT, + "/req-tags", + &[], + b"teama", + ); + for action in ["CreateBucket", "TagResource"] { + let tags = s3_request_tags(&req, action).expect("tags extracted"); + assert_eq!(tags.get("team").map(String::as_str), Some("a"), "{action}"); + } + + // 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") + .expect("tags extracted") + .is_empty()); +} + +#[test] +fn create_bucket_configuration_tags_ignores_foreign_bodies() { + // A PutBucketTagging body must not be mistaken for a create-time tag set: + // the helper is gated on the CreateBucketConfiguration root element. + let tagging = + r#"envprod"#; + assert!(create_bucket_configuration_tags(tagging).is_empty()); + assert!(create_bucket_configuration_tags("").is_empty()); + + // A namespaced or attributed element is still parsed — the scan + // matches the children, not the container. + let ns = r#"teama"#; + assert_eq!( + create_bucket_configuration_tags(ns), + vec![("team".to_string(), "a".to_string())] + ); +} + #[test] fn create_bucket_already_owned_other_region() { let svc = make_service(); diff --git a/website/content/docs/reference/security.md b/website/content/docs/reference/security.md index 6183e00d7..cb7284877 100644 --- a/website/content/docs/reference/security.md +++ b/website/content/docs/reference/security.md @@ -213,7 +213,7 @@ Tag-based access control via four condition key families: | Condition key | Description | Enforced services | |---|---|---| | `aws:ResourceTag/` | Tags on the target resource | S3, SQS, SNS, IAM, KMS, DynamoDB (a table's tags, also for its indexes and streams) | -| `aws:RequestTag/` | Tags sent in the request (e.g. on CreateQueue, PutObject) | S3, SQS, SNS, IAM, KMS, DynamoDB (`CreateTable`, `TagResource`) | +| `aws:RequestTag/` | Tags sent in the request (e.g. on CreateQueue, PutObject, CreateBucket) | S3 (including a `CreateBucket` tag set, which also requires `s3:TagResource`), SQS, SNS, IAM, KMS, DynamoDB (`CreateTable`, `TagResource`) | | `aws:TagKeys` | List of tag keys in the request (for `ForAllValues`/`ForAnyValue`) | S3, SQS, SNS, IAM, KMS, DynamoDB (`CreateTable`, `TagResource`, `UntagResource`) | | `aws:PrincipalTag/` | Tags on the calling IAM user or assumed role | All enforced services | diff --git a/website/content/docs/services/s3.md b/website/content/docs/services/s3.md index 819a0dbba..10b3a2189 100644 --- a/website/content/docs/services/s3.md +++ b/website/content/docs/services/s3.md @@ -16,7 +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 -- **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. +- **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. - **Object Lock** — legal hold, retention modes - **Website hosting** — index/error documents, redirect rules From b5909e34e547c5d8e9bbf8d5e94f61ba745067df Mon Sep 17 00:00:00 2001 From: Lucas Vieira Date: Thu, 24 Sep 2026 18:02:14 -0300 Subject: [PATCH 3/6] fix(s3): persist bucket tags before committing the create Write the Tags subresource ahead of meta.toml and the in-memory insert, so a store error during CreateBucket leaves nothing usable behind: the loader skips a bucket directory that has no meta.toml, so the failure mode is now "no bucket" rather than "bucket exists, permanently untagged" -- the same guarantee the InvalidTag path already gives. Also document why the us-east-1 idempotent re-create does not re-apply the create-time tag set (it re-applies no create-time setting -- not the canned ACL, object lock, or object ownership either), with a test locking that in. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_015paJopoDarw6WeFFoZMDMf --- crates/fakecloud-s3/src/service/buckets.rs | 27 +++++++++++++++------ crates/fakecloud-s3/src/service/tests.rs | 28 ++++++++++++++++++++++ 2 files changed, 48 insertions(+), 7 deletions(-) diff --git a/crates/fakecloud-s3/src/service/buckets.rs b/crates/fakecloud-s3/src/service/buckets.rs index 73ca15452..8bfa6b358 100644 --- a/crates/fakecloud-s3/src/service/buckets.rs +++ b/crates/fakecloud-s3/src/service/buckets.rs @@ -246,7 +246,14 @@ impl S3Service { } let state = accts.get_or_create(account_id); if let Some(existing) = state.buckets.get(bucket) { - // In us-east-1, re-creating same bucket in same region is idempotent (returns 200) + // In us-east-1, re-creating same bucket in same region is idempotent + // (returns 200). The re-create is a no-op on the existing bucket: it + // re-applies none of the create-time settings — not the canned ACL, + // not object lock, not object ownership, and not the + // `CreateBucketConfiguration` tag set. Applying only the tags here + // would make them the one create-time setting that mutates a bucket + // that already exists; `PutBucketTagging` is the operation that + // changes the tags of an existing bucket. if existing.region == requested_region && requested_region == "us-east-1" { let mut headers = HeaderMap::new(); headers.insert("location", format!("/{bucket}").parse().unwrap()); @@ -308,18 +315,24 @@ impl S3Service { }; let meta = bucket_meta_snapshot(&b); - state.buckets.insert(bucket.to_string(), b); - self.store - .put_bucket_meta(bucket, &meta) - .map_err(super::persistence_error)?; - // Bucket tags live in their own subresource, not in the bucket meta, so - // they need an explicit write to survive a restart. + // Persist before committing the bucket to memory, tags first: bucket + // tags live in their own subresource rather than in the bucket meta, so + // they need an explicit write to survive a restart, and writing them + // ahead of `meta.toml` keeps a failed create from leaving anything + // usable behind. The loader skips a bucket directory with no + // `meta.toml`, and the in-memory insert is last, so a store error at + // either step surfaces as an error with no half-created bucket — the + // same guarantee the `InvalidTag` path gives. if let Some(snap) = tags_snapshot { let payload = toml::to_string(&snap).unwrap_or_default(); self.store .put_bucket_subresource(bucket, BucketSubresource::Tags, &payload) .map_err(super::persistence_error)?; } + self.store + .put_bucket_meta(bucket, &meta) + .map_err(super::persistence_error)?; + state.buckets.insert(bucket.to_string(), b); let mut headers = HeaderMap::new(); headers.insert("location", format!("/{bucket}").parse().unwrap()); diff --git a/crates/fakecloud-s3/src/service/tests.rs b/crates/fakecloud-s3/src/service/tests.rs index 9e4aded32..4cae69419 100644 --- a/crates/fakecloud-s3/src/service/tests.rs +++ b/crates/fakecloud-s3/src/service/tests.rs @@ -3638,6 +3638,34 @@ fn create_bucket_configuration_tags_ignores_foreign_bodies() { ); } +#[test] +fn create_bucket_idempotent_recreate_does_not_reapply_create_time_tags() { + // The us-east-1 idempotent re-create is a no-op on the existing bucket. It + // re-applies no create-time setting, tags included — PutBucketTagging is + // what changes an existing bucket's tags. + let svc = make_service(); + let plain = make_request(Method::PUT, "/idem-tags", &[], b""); + svc.create_bucket("123456789012", &plain, "idem-tags") + .unwrap(); + + let tagged = make_request( + Method::PUT, + "/idem-tags", + &[], + b"teama", + ); + let resp = svc + .create_bucket("123456789012", &tagged, "idem-tags") + .unwrap(); + assert_eq!(resp.status, StatusCode::OK); + + let get = make_request(Method::GET, "/idem-tags", &[("tagging", "")], b""); + assert_aws_err( + svc.get_bucket_tagging("123456789012", &get, "idem-tags"), + "NoSuchTagSet", + ); +} + #[test] fn create_bucket_already_owned_other_region() { let svc = make_service(); From a016e1fdc1b01a3cce3be5f0b20dd11ef5b36ef0 Mon Sep 17 00:00:00 2001 From: Lucas Vieira Date: Thu, 24 Sep 2026 18:12:24 -0300 Subject: [PATCH 4/6] fix(s3): keep the tag file in step with the create, and the stream gate with routing Two holes the previous commits left: - An untagged CreateBucket now deletes the Tags subresource instead of leaving it alone. Writing tags.toml before meta.toml means a create that fails partway leaves an orphan tag file in a directory the loader skips (no meta.toml) -- but a later untagged create of that name would adopt the never-committed tag set on the next restart. Deleting is a no-op in the normal case, where no such file exists. - streaming_route derived the path-style key with split_once, which calls the key of `PUT ///` "/" and streams it, while routing filters empty segments and reads the same path as a bucket-level operation -- the exact empty-body mismatch the trailing-slash fix was for. The key is now derived with the same split-and-drop-empty rule the request builder uses, so the two cannot disagree. Adds a persistence e2e for the orphan tag file (verified failing without the delete), plus dispatch tests for the doubled-slash path and for a real object key that ends in '/', which must still stream. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_015paJopoDarw6WeFFoZMDMf --- crates/fakecloud-core/src/dispatch.rs | 46 ++++++++++++++++++- crates/fakecloud-e2e/tests/s3_persistence.rs | 47 ++++++++++++++++++++ crates/fakecloud-s3/src/service/buckets.rs | 23 +++++++--- 3 files changed, 110 insertions(+), 6 deletions(-) diff --git a/crates/fakecloud-core/src/dispatch.rs b/crates/fakecloud-core/src/dispatch.rs index 7e8ef9cc4..2a7769d59 100644 --- a/crates/fakecloud-core/src/dispatch.rs +++ b/crates/fakecloud-core/src/dispatch.rs @@ -1140,7 +1140,17 @@ fn streaming_route( // handler -- IAM enforcement above all -- saw an empty body and could // not read `CreateBucketConfiguration` (no `aws:RequestTag` from a // create-time tag set, for one). - let path_style_key = after.split_once('/').map(|(_, key)| key).unwrap_or(""); + // + // The key is derived exactly the way the request builder derives + // `path_segments` -- split on '/', drop empty segments -- so the two + // cannot disagree. A naive `split_once` would call the key of + // `PUT ///` "/" and stream it, while routing collapses that + // path to a bucket-level operation and would again see an empty body. + let path_style_key = after + .split('/') + .skip(1) + .find(|seg| !seg.is_empty()) + .unwrap_or(""); if after.is_empty() || (!virtual_hosted_s3 && path_style_key.is_empty()) { return None; } @@ -2018,6 +2028,40 @@ mod tests { ); } + #[test] + fn streaming_route_path_style_doubled_slash_skipped() { + // Routing filters empty path segments, so `PUT ///` is a + // bucket-level operation, not an object upload with a "/" key. The + // streaming gate has to agree, or the body goes unbuffered for a + // request the handler reads as CreateBucket / PutBucketTagging. + let headers = s3_sigv4_headers(); + assert_eq!( + streaming_route( + &http::Method::PUT, + "/my-bucket//", + &headers, + &HashMap::new() + ), + None, + ); + } + + #[test] + fn streaming_route_path_style_key_with_trailing_slash_streams() { + // A real object key that ends in '/' (a directory marker) still + // streams -- the gate rejects an absent key, not a trailing slash. + let headers = s3_sigv4_headers(); + assert_eq!( + streaming_route( + &http::Method::PUT, + "/my-bucket/folder/", + &headers, + &HashMap::new(), + ), + Some(("s3", "")), + ); + } + #[test] fn streaming_route_virtual_hosted_s3_put_object() { let mut headers = s3_sigv4_headers(); diff --git a/crates/fakecloud-e2e/tests/s3_persistence.rs b/crates/fakecloud-e2e/tests/s3_persistence.rs index 89d259ca6..4ffff317d 100644 --- a/crates/fakecloud-e2e/tests/s3_persistence.rs +++ b/crates/fakecloud-e2e/tests/s3_persistence.rs @@ -1300,3 +1300,50 @@ async fn persistence_create_bucket_tags_survive_restart() { "create-time tags lost across restart: {ts:?}" ); } + +#[tokio::test] +async fn persistence_untagged_create_clears_an_orphan_tag_file() { + // A create that fails after writing its tag set (or dies between the tag + // write and meta.toml) leaves a tags.toml in a directory with no meta.toml. + // The loader skips such a directory, but a later untagged CreateBucket of + // the same name must not adopt that never-committed tag set on the next + // restart. + let tmp = tempfile::tempdir().unwrap(); + let mut server = TestServer::start_persistent(tmp.path()).await; + let client = server.s3_client().await; + + // Simulate the half-written create: a bucket directory holding only tags. + let orphan_dir = tmp.path().join("s3").join("buckets").join("orphan-tags"); + std::fs::create_dir_all(&orphan_dir).unwrap(); + std::fs::write(orphan_dir.join("tags.toml"), "[tags]\nteam = \"ghost\"\n").unwrap(); + + server.restart().await; + let client2 = server.s3_client().await; + // No meta.toml, so the loader skipped it and no bucket exists. + client2 + .head_bucket() + .bucket("orphan-tags") + .send() + .await + .expect_err("orphan directory must not resurrect the bucket"); + drop(client); + + client2 + .create_bucket() + .bucket("orphan-tags") + .send() + .await + .unwrap(); + + server.restart().await; + let client3 = server.s3_client().await; + + let err = client3 + .get_bucket_tagging() + .bucket("orphan-tags") + .send() + .await + .expect_err("untagged create must not inherit the orphan tag set"); + let msg = format!("{err:?}"); + assert!(msg.contains("NoSuchTagSet"), "unexpected error: {msg}"); +} diff --git a/crates/fakecloud-s3/src/service/buckets.rs b/crates/fakecloud-s3/src/service/buckets.rs index 8bfa6b358..f6523fce5 100644 --- a/crates/fakecloud-s3/src/service/buckets.rs +++ b/crates/fakecloud-s3/src/service/buckets.rs @@ -323,11 +323,24 @@ impl S3Service { // `meta.toml`, and the in-memory insert is last, so a store error at // either step surfaces as an error with no half-created bucket — the // same guarantee the `InvalidTag` path gives. - if let Some(snap) = tags_snapshot { - let payload = toml::to_string(&snap).unwrap_or_default(); - self.store - .put_bucket_subresource(bucket, BucketSubresource::Tags, &payload) - .map_err(super::persistence_error)?; + match tags_snapshot { + Some(snap) => { + let payload = toml::to_string(&snap).unwrap_or_default(); + self.store + .put_bucket_subresource(bucket, BucketSubresource::Tags, &payload) + .map_err(super::persistence_error)?; + } + // An untagged create clears the file rather than leaving it alone: + // a create that failed after its tag write (or died between the two + // writes) leaves a `tags.toml` in a directory with no `meta.toml`, + // which the loader skips — but a later untagged create of the same + // name would adopt that never-committed tag set on the next restart. + // Deleting is a no-op when the file is absent, which is the normal + // case. + None => self + .store + .delete_bucket_subresource(bucket, BucketSubresource::Tags) + .map_err(super::persistence_error)?, } self.store .put_bucket_meta(bucket, &meta) From 9dcb9238335e91462aba5c33dc43035af99241a2 Mon Sep 17 00:00:00 2001 From: Lucas Vieira Date: Thu, 24 Sep 2026 18:28:52 -0300 Subject: [PATCH 5/6] fix(s3): derive the streaming gate's key from the routed path The gate was tightened for path-style addressing only. A virtual-hosted request that also carries the bucket in the path (`Host: b.s3...` with `PUT /b` or `PUT /b/`) still streamed, even though routing has an explicit bucket-root branch that collapses exactly that shape to CreateBucket -- so the handler and IAM saw an empty body, the CreateBucketConfiguration tag set was dropped, and iam_actions_for omitted the s3:TagResource requirement. Same failure mode as the path-style one, left open for the one shape the router already special-cases. Rather than tighten a second predicate, the router's path derivation moves into s3_routing_path and the gate calls it: streaming now requires that the routed path split into a bucket AND a non-empty key, by the same split-and-drop-empty rule the request builder uses for path_segments. The two cannot drift again -- they are the same code -- which is what produced both of these bugs. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_015paJopoDarw6WeFFoZMDMf --- crates/fakecloud-core/src/dispatch.rs | 117 +++++++++++++++++--------- 1 file changed, 76 insertions(+), 41 deletions(-) diff --git a/crates/fakecloud-core/src/dispatch.rs b/crates/fakecloud-core/src/dispatch.rs index 2a7769d59..1417845db 100644 --- a/crates/fakecloud-core/src/dispatch.rs +++ b/crates/fakecloud-core/src/dispatch.rs @@ -486,19 +486,10 @@ pub async fn dispatch( // ran against the wire path, so this rewrite is signature-safe. let wire_path = parts.uri.path(); let path = if detected.service == "s3" { - if let Some(bucket) = host_info.as_ref().and_then(|h| h.bucket.as_deref()) { - let prefix_with_slash = format!("/{bucket}/"); - let is_bucket_root = wire_path.trim_end_matches('/') == format!("/{bucket}"); - if wire_path.starts_with(&prefix_with_slash) || is_bucket_root { - wire_path.to_string() - } else if wire_path == "/" || wire_path.is_empty() { - format!("/{bucket}") - } else { - format!("/{bucket}{wire_path}") - } - } else { - wire_path.to_string() - } + s3_routing_path( + wire_path, + host_info.as_ref().and_then(|h| h.bucket.as_deref()), + ) } else { wire_path.to_string() }; @@ -1086,6 +1077,31 @@ impl DispatchConfig { } } +/// The path an S3 request is routed on: for virtual-hosted-style the bucket +/// comes from the Host header and is prepended, unless the client already put +/// it in the path (`PUT /` / `PUT //key` against a +/// virtual-hosted host). Path-style requests pass through unchanged. +/// +/// Shared with [`streaming_route`] on purpose. The streaming gate has to agree +/// with routing about where the bucket ends and the key begins; when the two +/// derived it separately they drifted, and a request the router read as a +/// bucket-level operation was dispatched unbuffered, so the handler and IAM +/// enforcement saw an empty body. +fn s3_routing_path(wire_path: &str, host_bucket: Option<&str>) -> String { + let Some(bucket) = host_bucket else { + return wire_path.to_string(); + }; + let prefix_with_slash = format!("/{bucket}/"); + let is_bucket_root = wire_path.trim_end_matches('/') == format!("/{bucket}"); + if wire_path.starts_with(&prefix_with_slash) || is_bucket_root { + wire_path.to_string() + } else if wire_path == "/" || wire_path.is_empty() { + format!("/{bucket}") + } else { + format!("/{bucket}{wire_path}") + } +} + /// Extract the 12-digit account ID segment from an AWS ARN. /// /// ARNs follow `arn:::::`. @@ -1124,34 +1140,23 @@ fn streaming_route( // presigned URL (X-Amz-Credential .../s3/...) OR a SigV2 presigned // URL (AWSAccessKeyId + Signature + Expires query parameters). if method == http::Method::PUT { - let after = path.trim_start_matches('/'); - // Path-style PutObject is `PUT //` (path contains a - // slash); virtual-hosted-style is `PUT /` with the bucket - // in the Host header. For virtual-hosted, accept any non-empty - // path so the key flows through the streaming dispatch — the - // Host parser already routed this request to S3. - let virtual_hosted_s3 = protocol::parse_routing_host_from_headers(headers) - .filter(|h| h.service == "s3" && h.bucket.is_some()) - .is_some(); - // Path-style needs a non-empty key after the bucket: `PUT /` - // and `PUT //` are both CreateBucket, and the trailing-slash - // form is what the AWS SDKs actually send. Treating it as an object - // upload left the body unbuffered, so everything that runs before the - // handler -- IAM enforcement above all -- saw an empty body and could - // not read `CreateBucketConfiguration` (no `aws:RequestTag` from a - // create-time tag set, for one). - // - // The key is derived exactly the way the request builder derives - // `path_segments` -- split on '/', drop empty segments -- so the two - // cannot disagree. A naive `split_once` would call the key of - // `PUT ///` "/" and stream it, while routing collapses that - // path to a bucket-level operation and would again see an empty body. - let path_style_key = after - .split('/') - .skip(1) - .find(|seg| !seg.is_empty()) - .unwrap_or(""); - if after.is_empty() || (!virtual_hosted_s3 && path_style_key.is_empty()) { + // An object upload needs a bucket AND a key. Resolve the request the + // way routing will (bucket from the Host header for virtual-hosted, + // straight from the path otherwise), then split it the way the request + // builder builds `path_segments` -- on '/', dropping empty segments. + // Anything that leaves no key is a bucket-level operation: + // `PUT /`, `PUT //` (the form the AWS SDKs actually + // send for CreateBucket), `PUT ///`, and the virtual-hosted + // equivalents. Streaming those left the body unbuffered, so everything + // that runs before the handler -- IAM enforcement above all -- saw an + // empty body and could not read `CreateBucketConfiguration` (no + // `aws:RequestTag` from a create-time tag set, for one). + let host_bucket = protocol::parse_routing_host_from_headers(headers) + .filter(|h| h.service == "s3") + .and_then(|h| h.bucket); + let routed = s3_routing_path(path, host_bucket.as_deref()); + let has_key = routed.split('/').filter(|seg| !seg.is_empty()).count() >= 2; + if !has_key { return None; } let header_s3 = headers @@ -2081,6 +2086,36 @@ mod tests { ); } + #[test] + fn streaming_route_virtual_hosted_bucket_in_path_skipped() { + // `Host: my-bucket.s3...` with the bucket ALSO in the path is the + // router's bucket-root shape (dispatch collapses it to CreateBucket), + // so it must not stream -- with or without the trailing slash. + let mut headers = s3_sigv4_headers(); + headers.insert( + "host", + "my-bucket.s3.us-east-1.amazonaws.com".parse().unwrap(), + ); + assert_eq!( + streaming_route(&http::Method::PUT, "/my-bucket", &headers, &HashMap::new()), + None, + ); + assert_eq!( + streaming_route(&http::Method::PUT, "/my-bucket/", &headers, &HashMap::new()), + None, + ); + // ...but a real key under that same host still streams. + assert_eq!( + streaming_route( + &http::Method::PUT, + "/my-bucket/key.txt", + &headers, + &HashMap::new(), + ), + Some(("s3", "")), + ); + } + #[test] fn streaming_route_virtual_hosted_s3_root_skipped() { // `PUT /` against a virtual-hosted Host = CreateBucket, which From c9c530873d16f24e8df37b6bf3fc33f5cf3275fd Mon Sep 17 00:00:00 2001 From: Lucas Vieira Date: Thu, 24 Sep 2026 18:36:22 -0300 Subject: [PATCH 6/6] docs(s3): note the virtual-hosted key-shadowing limit in s3_routing_path Under virtual-hosted addressing real S3 treats the whole path as the key, so a key beginning with `/` (or a key that is the bucket name) is not representable here: the passthrough reads it as a client that put the bucket in the path and drops the prefix. Nothing in a single request tells the two apart, so the behavior resolves the ambiguity in favor of the mixed-addressing client -- pre-existing, moved verbatim into the helper, now stated rather than implied, including in the test that exercises the shape. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_015paJopoDarw6WeFFoZMDMf --- crates/fakecloud-core/src/dispatch.rs | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/crates/fakecloud-core/src/dispatch.rs b/crates/fakecloud-core/src/dispatch.rs index 1417845db..f8e4f2ce6 100644 --- a/crates/fakecloud-core/src/dispatch.rs +++ b/crates/fakecloud-core/src/dispatch.rs @@ -1087,6 +1087,14 @@ impl DispatchConfig { /// derived it separately they drifted, and a request the router read as a /// bucket-level operation was dispatched unbuffered, so the handler and IAM /// enforcement saw an empty body. +/// +/// Known limitation: under virtual-hosted addressing the whole path is the +/// key on real S3, so an object key that begins with `/` (or a key +/// that IS the bucket name) is not representable here — the passthrough reads +/// it as a client that put the bucket in the path, and the prefix is dropped. +/// Nothing in a single request distinguishes the two, so this resolves the +/// ambiguity in favor of the mixed-addressing client. Keys shaped like their +/// own bucket name are the cost. fn s3_routing_path(wire_path: &str, host_bucket: Option<&str>) -> String { let Some(bucket) = host_bucket else { return wire_path.to_string(); @@ -2104,7 +2112,10 @@ mod tests { streaming_route(&http::Method::PUT, "/my-bucket/", &headers, &HashMap::new()), None, ); - // ...but a real key under that same host still streams. + // ...but a path the router reads as a key under that bucket still + // streams. (Per the `s3_routing_path` limitation, a virtual-hosted key + // that genuinely begins with `my-bucket/` is indistinguishable from + // this and is routed the same way.) assert_eq!( streaming_route( &http::Method::PUT,