Skip to content

fix(s3): persist the ACL, object lock and ownership a bucket is created with - #2558

Merged
vieiralucas merged 1 commit into
mainfrom
worktree-s3-create-subresource-persistence
Sep 25, 2026
Merged

vieiralucas merged 1 commit into
mainfrom
worktree-s3-create-subresource-persistence

Conversation

@vieiralucas

@vieiralucas vieiralucas commented Sep 25, 2026 •

Copy link
Copy Markdown
Member

Summary

CreateBucket set acl_grants, object_lock_config and ownership_controls in memory and persisted none of them. None live in BucketMeta, and the loader restores the latter two as None and falls back to the default owner grant when acl.toml is absent — so after a restart:

  • a bucket created with x-amz-acl: public-read came back owner-only, and
  • a bucket created with x-amz-bucket-object-lock-enabled: true came back with no lock configuration at all, which silently stops retention being enforced.

Versioning survived, because that one is in meta.toml — which is what made the gap easy to miss.

Found while reviewing #2557 (the CreateBucketConfiguration.Tags fix); this is the create-time half of the same problem.

What it does

Each of the three is written when the create sets it, and the bucket's stored subresources are cleared first so a create cannot inherit a sidecar left behind by a create or delete that stopped partway, or by /_fakecloud/reset (which clears memory and deliberately leaves the store alone).

Two ordering decisions worth calling out, both learned the hard way in review:

  • The meta is written before the clear, so a create that fails has destroyed nothing. Clearing first threw away the configuration of whatever bucket the name belonged to, for a create that never happened.
  • objects/ is never touched. The loader skips a bucket whose objects it cannot read, so that bucket is absent from memory while its data sits intact on disk — re-creating the name must not be what destroys it. Whether a create should adopt or discard a stale object tree is a separate question this PR does not answer.

ACL fixes that come with it

Persisting the ACL turns a set of latent bugs durable, so they are fixed here:

  • Canned x-amz-acl, x-amz-grant-* and an AccessControlPolicy body are mutually exclusive, as on S3 — letting one win silently discarded what the others asked for.
  • Canned values are validated against the set the target accepts; an unrecognized one fell through to owner-only, so a typo wiped every public grant. The object-scoped bucket-owner-* values are accepted on a bucket and resolve to owner-only, which is what S3 ignoring them produces.
  • A grant naming no resolvable grantee is refused rather than stored as an entry matching nobody, and grantee values are trimmed so a pretty-printed <ID> does not bake whitespace into a canonical id.
  • emailAddress= grantees are kept as AmazonCustomerByEmail instead of dropped, and logged as unenforced — nothing resolves them to a canonical user, so the grant conveys no access.
  • PutBucketAcl honors the grant headers; without that arm put-bucket-acl --grant-read fell through to an empty body and wiped the ACL.
  • PutObjectAcl updates the versioned copy too, so a versionId read no longer answers a different ACL.
  • An ACL on a bucket whose ownership disables them is refused by every path that can set one, CreateMultipartUpload included.
  • PutBucketOwnershipControls validates every ObjectOwnership the document carries (bucket_owner_enforced matches the stored text, so a second rule could turn ACLs off while the API reported otherwise) and persists before mutating memory.

Storage-integrity fixes in the same paths

  • Every writer of a persisted snapshot propagates a serialization failure instead of writing an empty document. An empty acl.toml is worse than none: the loader takes its presence to mean an explicit ACL and drops the owner grant. A sidecar whose grants are all unusable falls back to that owner grant too.
  • delete_object removes the sidecar before the body. load iterates *.toml and treats a sidecar with no body as fatal for the bucket, while an orphan body is never read — so the body is the residue an interrupted delete should leave. A body that cannot be unlinked warns rather than failing a delete that did happen.
  • The analytics, intelligent-tiering and metrics sidecars neither default silently (losing the configurations, which the next Put then makes permanent) nor fail the whole bucket over a reporting config: they are skipped with a warning.

Test plan

  • Unit (fakecloud-s3, 479 passing): the ACL mutual-exclusion rules, canned validation on buckets and objects, unresolvable and blank grantees, the email grantee round trip asserted against stored state, BucketOwnerEnforced conflicts including multipart, ownership-value validation, and the versioned ACL sync — the last asserted with real version ids, since an earlier version of that fixture used None on both sides and could not fail.
  • Unit (fakecloud-persistence, 71 passing): the sidecar-before-body delete ordering, forced by making the sidecar removal fail so only a sidecar-first delete leaves the body intact.
  • E2E: create-time ACL/object-lock/ownership across a restart; object ACL across a restart; grant headers across a restart; an untagged create clearing an orphan tags.toml; create-after-reset reusing the name without adopting the old configuration; and re-creating the name of a load-skipped bucket without destroying its objects.
  • Each behavioral fix was verified by reverting it and confirming the matching test fails — including the two that a review showed could not fail as first written.
  • Suites: fakecloud-s3 + fakecloud-persistence 550 passed; e2e s3, s3_persistence, s3_anonymous_access, s3_access_points, s3_post_object, iam_enforcement, cloudformation_provisioner_properties all green; fakecloud-conformance --test s3 51 passed; the s3 probe reports 4040/4068 variants, unchanged from before the branch. cargo clippy --workspace --all-targets -- -D warnings and cargo fmt --check clean.

Surface sync

  • Reference docs — website/content/docs/services/s3.md: the Object Lock bullet now states that a bucket keeps its lock configuration, canned ACL and ownership rule across a restart, and a new bullet documents the ACL rules above.
  • SDKs / introspection — no change: this is AWS API behavior, not a /_fakecloud/* surface.
  • Counts / metadata — unchanged: no new operation, service or conformance variant, so conformance-baseline.json, the ops index, the README headline and the GitHub repo description all stay as they are.

Known gaps, deliberately not in scope

  • A create still adopts a stale objects/ tree for a reused name (pre-existing). Discarding it is what destroys a load-skipped bucket's recoverable data, so the trade needs its own change.
  • CopyObject ignores x-amz-acl / x-amz-grant-* entirely (pre-existing) and so is unaffected by the validation added here.
  • Under --iam, a create carrying an ACL is still authorized as s3:CreateBucket alone; AWS also requires s3:PutBucketAcl, and the s3:x-amz-acl condition key is not populated.

Summary by cubic

Persists the ACL, object lock, and ownership rules a bucket is created with, so they survive a restart instead of being lost. The old behavior dropped all three; losing object lock silently disables retention.

Behavior changes

  • CreateBucket now writes these subresources and clears stale ones first; re-creating a name never touches an existing objects/ tree.
  • Canned ACL, grant headers, and an ACL policy body are now mutually exclusive and validated; unknown canned values and unresolvable grantees are rejected.
  • PutBucketAcl honors grant headers, PutObjectAcl updates the versioned copy, and ACLs on bucket_owner_enforced buckets are refused on every path.
  • PutBucketOwnershipControls validates every ownership value and persists before mutating memory.

Storage integrity

  • Snapshot serialization failures now return 500 instead of writing empty documents.
  • Object delete removes the sidecar before the body, so an interrupted delete leaves the body as recoverable residue.
  • Analytics, metrics, and intelligent-tiering sidecars are skipped with a warning instead of silently defaulting or failing the bucket.

Written for commit 9979ab1. Summary will update on new commits.

Review in cubic

…ed with

CreateBucket set acl_grants, object_lock_config and ownership_controls in memory
and wrote none of them. None live in BucketMeta, and the loader restores the
latter two as None and falls back to the default owner grant when acl.toml is
absent, so after a restart a bucket created with `x-amz-acl: public-read` came
back owner-only and one created with `x-amz-bucket-object-lock-enabled: true`
came back with no lock configuration at all -- which silently stops retention
being enforced. Versioning survived, since that one IS in meta, which is what
made the gap easy to miss.

Each of the three is now written when the create sets it, and the bucket's
stored subresources are cleared first so a create cannot inherit a sidecar left
by a create or delete that stopped partway, or by a `/_fakecloud/reset` (which
clears memory and deliberately leaves the store alone). The meta is written
before that clear, so a create that fails has destroyed nothing; `objects/` is
never touched, because the loader skips a bucket whose objects it cannot read
and re-creating the name must not be what destroys data that is still
recoverable.

Persisting the ACL made a set of latent ACL bugs durable, so they are fixed
here too:

- A canned `x-amz-acl`, the `x-amz-grant-*` headers and an AccessControlPolicy
  body are mutually exclusive, as on S3. Letting one win silently discarded what
  the others asked for.
- Canned values are validated against the set the target accepts. An
  unrecognized one fell through to owner-only, so a typo wiped every public
  grant. The object-scoped `bucket-owner-*` values are accepted on a bucket and
  resolve to owner-only, which is what S3 ignoring them produces.
- A grant naming no resolvable grantee is refused rather than stored as an entry
  that can never match anyone, and grantee values are trimmed so a pretty-printed
  <ID> does not bake whitespace into a canonical id.
- `emailAddress=` grantees are kept as AmazonCustomerByEmail instead of being
  dropped, and logged as unenforced: nothing resolves them to a canonical user,
  so the grant conveys no access.
- PutBucketAcl honors the grant headers; without that arm a
  `put-bucket-acl --grant-read` fell through to an empty body and wiped the ACL.
- PutObjectAcl updates the versioned copy of the object too, so a versionId read
  no longer answers a different ACL than the unversioned one.
- An ACL on a bucket whose ownership disables them is refused by every path that
  can set one, CreateMultipartUpload included.
- PutBucketOwnershipControls validates every ObjectOwnership the document
  carries -- `bucket_owner_enforced` matches the stored text, so a second rule
  could turn ACLs off while the API reported otherwise -- and persists before
  mutating memory.

And three storage-integrity fixes in the same paths:

- Every writer of a persisted snapshot propagates a serialization failure rather
  than writing an empty document. An empty acl.toml is worse than none: the
  loader takes its presence to mean an explicit ACL and drops the owner grant.
  A sidecar whose grants are all unusable now falls back to that owner grant too.
- delete_object removes the sidecar before the body. `load` iterates `*.toml`
  and treats a sidecar with no body as fatal for the bucket, while an orphan
  body is never read, so the body is the residue an interrupted delete should
  leave. A body that cannot be unlinked warns rather than failing a delete that
  did happen.
- The analytics, intelligent-tiering and metrics sidecars neither default
  silently (losing the configurations, which the next Put then makes permanent)
  nor fail the bucket over a reporting config: they are skipped with a warning.

Fixes the create-time half of the gap found while reviewing #2557.
@vieiralucas
vieiralucas merged commit f57485c into main Sep 25, 2026
157 checks passed
@vieiralucas
vieiralucas deleted the worktree-s3-create-subresource-persistence branch September 25, 2026 08:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant