fix(s3): persist the ACL, object lock and ownership a bucket is created with - #2558
Merged
Merged
Conversation
…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.
This was referenced Sep 25, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
CreateBucketsetacl_grants,object_lock_configandownership_controlsin memory and persisted none of them. None live inBucketMeta, and the loader restores the latter two asNoneand falls back to the default owner grant whenacl.tomlis absent — so after a restart:x-amz-acl: public-readcame back owner-only, andx-amz-bucket-object-lock-enabled: truecame 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.Tagsfix); 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:
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:
x-amz-acl,x-amz-grant-*and anAccessControlPolicybody are mutually exclusive, as on S3 — letting one win silently discarded what the others asked for.bucket-owner-*values are accepted on a bucket and resolve to owner-only, which is what S3 ignoring them produces.<ID>does not bake whitespace into a canonical id.emailAddress=grantees are kept asAmazonCustomerByEmailinstead of dropped, and logged as unenforced — nothing resolves them to a canonical user, so the grant conveys no access.PutBucketAclhonors the grant headers; without that armput-bucket-acl --grant-readfell through to an empty body and wiped the ACL.PutObjectAclupdates the versioned copy too, so aversionIdread no longer answers a different ACL.CreateMultipartUploadincluded.PutBucketOwnershipControlsvalidates everyObjectOwnershipthe document carries (bucket_owner_enforcedmatches 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
acl.tomlis 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_objectremoves the sidecar before the body.loaditerates*.tomland 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.Test plan
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,BucketOwnerEnforcedconflicts including multipart, ownership-value validation, and the versioned ACL sync — the last asserted with real version ids, since an earlier version of that fixture usedNoneon both sides and could not fail.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.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.fakecloud-s3+fakecloud-persistence550 passed; e2es3,s3_persistence,s3_anonymous_access,s3_access_points,s3_post_object,iam_enforcement,cloudformation_provisioner_propertiesall green;fakecloud-conformance --test s351 passed; the s3 probe reports 4040/4068 variants, unchanged from before the branch.cargo clippy --workspace --all-targets -- -D warningsandcargo fmt --checkclean.Surface sync
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./_fakecloud/*surface.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
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.CopyObjectignoresx-amz-acl/x-amz-grant-*entirely (pre-existing) and so is unaffected by the validation added here.--iam, a create carrying an ACL is still authorized ass3:CreateBucketalone; AWS also requiress3:PutBucketAcl, and thes3:x-amz-aclcondition 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
CreateBucketnow writes these subresources and clears stale ones first; re-creating a name never touches an existingobjects/tree.PutBucketAclhonors grant headers,PutObjectAclupdates the versioned copy, and ACLs onbucket_owner_enforcedbuckets are refused on every path.PutBucketOwnershipControlsvalidates every ownership value and persists before mutating memory.Storage integrity
Written for commit 9979ab1. Summary will update on new commits.