diff --git a/crates/fakecloud-core/src/dispatch.rs b/crates/fakecloud-core/src/dispatch.rs index d677aff97..f8e4f2ce6 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,39 @@ 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. +/// +/// 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(); + }; + 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,16 +1148,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(); - if after.is_empty() || (!virtual_hosted_s3 && !after.contains('/')) { + // 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 @@ -1997,6 +2028,53 @@ 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_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(); @@ -2016,6 +2094,39 @@ 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 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, + "/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 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-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..4ffff317d 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,86 @@ 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:?}" + ); +} + +#[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 7d574ec58..f6523fce5 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, + 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 { @@ -214,6 +215,13 @@ 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 = create_bucket_configuration_tags(body_str); + validate_tags(&create_tags)?; + // Parse ACL from header let acl = req .headers @@ -238,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()); @@ -290,11 +305,47 @@ 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); + // 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. + 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) .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/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 dcfe917ae..4cae69419 100644 --- a/crates/fakecloud-s3/src/service/tests.rs +++ b/crates/fakecloud-s3/src/service/tests.rs @@ -3473,6 +3473,199 @@ 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_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_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(); 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 75546accf..10b3a2189 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. 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