Skip to content

fix(backup-store): private file modes, owner-only root, put metadata, S3 suite in CI [backup/restore 2/4] - #393

Open
robinnsc wants to merge 1 commit into
mainfrom
feat/backup-store-hardening
Open

robinnsc wants to merge 1 commit into
mainfrom
feat/backup-store-hardening

Conversation

@robinnsc

@robinnsc robinnsc commented Oct 13, 2026 •

Copy link
Copy Markdown
Collaborator

Backup/restore series (design: #396): #384 restore fidelity (merged first) · #393 store · #394 format · #395 SnapshotEngine · next: PITR surface (#335 rebased), catalog index, backup job, restore job.

What

Hardening of crates/backup-store from #336 before it becomes the backup path's store. One commit, squashed with #336's work (Lee is co-author); supersedes #336.

Filesystem store. Objects are created 0600 and directories 0700 at creation time (OpenOptions::mode / DirBuilder::mode). Before, File::create and create_dir used the process umask, so backup contents could be group- or world-readable.

The module's symlink checks are path-based and can be raced by a process that can write under the root, so the store now requires that only the owner can reach the tree, and enforces it on Unix:

  • open() and validate() refuse a root that grants any permission to group or others (mode & 0o077 != 0) or is owned by another uid. Roots with modes 0750 and 0755, which feat(backup): add backup store crate with filesystem and S3 implementations #336 accepted, are refused: a traversable root lets another user reach a pre-existing permissive subdirectory or a 0644 file under it.
  • They also refuse a root whose ancestor is owned by a third user (neither the server's uid nor root) or is writable by others without the sticky bit; such an ancestor lets another user rename the root away and plant a symlink at its path.
  • Every operation re-checks the root and applies the same owner-only rule to every existing directory it descends into, including the list and delete_prefix walks, so a permissive directory that predates the store cannot be used to reach or replace objects, and a root widened after startup is refused by the next call rather than trusted until the next validate.
  • Every refusal names the path and the fix (chown / chmod 0700).

Consequence for operators, stated in the README and in #396: a backup pickup agent (rsync, restic, borg) cannot read the tree as a member of the server's group; it runs as the server's user or as root. A root under a directory owned by a storage administrator or an NFS mount owned by nobody is refused at startup.

A put whose directory fsync fails after the rename removes the renamed file, so a failed put leaves nothing observable under the key, as the trait promises. The validate probe goes through the same 0600 path, is synced, and is removed on both outcomes.

S3 store. validate() reports a 301 from HeadBucket as a bucket in another region, naming the region from x-amz-bucket-region; a 404 as a missing bucket; a 403 as denied. CreateBucket passes a LocationConstraint outside us-east-1 (the test suite creates its bucket). A missing Content-Length or Size is an error rather than 0, and a missing LastModified is None rather than the epoch.

Trait. put() returns ObjectMeta (bytes written, ETag where the store has one) instead of (); the manifest writer records both per data file without a second head(). ObjectMeta gains etag: Option<String>. S3 fills it from PutObject, CompleteMultipartUpload, HeadObject and ListObjectsV2 with the quotes stripped; the filesystem store reports None. MIN_PART_SIZE_BYTES is exported so tests do not duplicate the floor.

CI. The S3 conformance suite returned Ok when EXTENDDB_TEST_S3_ENDPOINT was unset, so it had never run in CI. All skip paths go through one helper, and EXTENDDB_TEST_S3_REQUIRED turns a missing endpoint into a failure. test.yml starts moto in server mode (pinned there, not in requirements.txt), runs the workspace tests with this crate excluded, then runs this crate with the S3 variables scoped to that step. MinIO was dropped because the project archived in April 2026 and publishes no images; any S3-compatible endpoint still works locally.

Why

#336 was approved with the filesystem store creating world-readable files under a permissive umask and with its S3 tests never executing in CI (design review of #348, findings on store security and test coverage). Two adversarial review rounds on this PR then found that the first fix (refuse group/world write on the root only) still let a traversable root expose permissive subdirectories, that list and delete_prefix skipped the ownership check the other operations applied, and that the root was never re-checked after open. The format PR (#394) needs the ETag and size from put to write manifest-files.json.

Testing done

cargo test -p extenddb-backup-store: 18 unit tests and 51 conformance tests (37 filesystem and shared, 14 S3) pass against a tempdir and, with EXTENDDB_TEST_S3_REQUIRED=1 and EXTENDDB_TEST_S3_ENDPOINT pointed at moto 5.2.4, against moto. With EXTENDDB_TEST_S3_REQUIRED=1 and no endpoint, all 14 S3 tests fail with the intended message.

Tests added or extended in this PR: put metadata agrees with head and list on both stores, including the empty object; created files and directories are 0600/0700; any root mode with group or other bits (0720, 0702, 0777, 1777, 0750, 0755, 0705, 0701) is refused with the full mode in the message, and 0700/0500 are accepted; a world-writable ancestor is refused and a sticky one accepted (the "ancestor owned by a third user" cell needs a second uid and is not exercised; the test says so); a permissive subdirectory is refused by put, get, head, list and delete_prefix and nothing is deleted; a root widened after open is refused by every operation and works again after chmod 0700; a symlinked root is canonicalized; a put over a directory is refused; a failed put leaves no directory skeleton; the validate probe is never left behind; symlink coverage covers a dangling link at the destination, a dangling link in the parent chain, a relative link, and a link chain, each asserting nothing is created outside the root.

cargo fmt --all -- --check
cargo clippy -p extenddb-backup-store --all-targets -- -D warnings -W clippy::pedantic   # 0 warnings
cargo test -p extenddb-backup-store                                                       # 18 + 51
EXTENDDB_TEST_S3_ENDPOINT=http://127.0.0.1:9200 EXTENDDB_TEST_S3_REQUIRED=1 \
  AWS_ACCESS_KEY_ID=testing AWS_SECRET_ACCESS_KEY=testing AWS_REGION=us-east-1 \
  cargo test -p extenddb-backup-store                                                     # 18 + 51, S3 against moto

Not changed here

  • list() still collects the filesystem walk into a Vec before streaming. The trait promises lexicographic order, which a directory walk cannot provide incrementally, and listings under the backup layout are small.
  • The symlink TOCTOU is bounded by the ownership requirement, not closed; closing it needs openat2(RESOLVE_BENEATH), which std does not expose. The ancestor rule is checked at open and validate only; the per-operation re-check covers the root and the directories under it.
  • No consumer of the crate yet; the backup job PR wires it.

Checklist

  • I have read CONTRIBUTING.md
  • All tests pass (cargo test --workspace)
  • Code is formatted (cargo fmt --check)
  • Clippy is clean (cargo clippy -- -W clippy::pedantic)
  • I have added or updated tests for new functionality
  • I have updated documentation if behavior changed
  • Breaking changes are noted below (if any)
  • If this changes the wire protocol, Storage trait, auth model, on-disk format, or public CLI surface, an RFC has been accepted or is linked below. Otherwise, an ADR captures the decision (link below).

ADR / RFC: #396, section 2. The BackupStore trait is internal to this series and has no implementors outside the crate; put's return type and the etag field change its shape.

Breaking changes

None for users; the crate is not yet consumed. Operators who adopt the filesystem store later need a 0700 root under root- or server-owned ancestors; the README and #396 state the pickup-agent consequence.


By submitting this pull request, I confirm that my contribution is made under the terms of the Apache License 2.0 and I agree to the Developer Certificate of Origin (DCO). See CONTRIBUTING.md for details.

@robinnsc robinnsc changed the title fix(backup-store): private file modes, root ownership check, put metadata, S3 suite in CI fix(backup-store): private file modes, root ownership check, put metadata, S3 suite in CI [backup/restore 2/4] Oct 13, 2026
… S3 suite in CI

Filesystem store:
- Objects are created 0600 and directories 0700 at creation time
  (OpenOptions::mode / DirBuilder::mode), not via chmod afterwards. Before,
  File::create and create_dir used the process umask, so backup contents
  could be group- or world-readable.
- The module's symlink checks are path-based and can be raced by a process
  that can write under the root, so the store now requires that only the
  owner can reach the tree. open() and validate() refuse a root that grants
  any permission to group or others (mode & 0o077 != 0) or is owned by
  another uid, and refuse a root whose ancestor is owned by a third user or
  is writable by others without the sticky bit (such an ancestor lets
  another user rename the root away and plant a symlink at its path).
  Every operation re-checks the root and applies the same owner-only rule
  to every existing directory it descends into, including the list and
  delete_prefix walks, so a permissive directory that predates the store
  cannot be used to reach or replace objects. Roots with modes 0750/0755,
  accepted before, are refused: a traversable root lets another user reach
  a pre-existing permissive subdirectory or 0644 file. Errors name the path
  and the fix (chown / chmod 0700).
- A put whose directory sync fails after the rename removes the renamed
  file, so a failed put leaves nothing observable under the key.
- The validate probe is created through the same 0600 path, synced, and
  removed on both outcomes.

S3 store:
- validate() reports a 301 from HeadBucket as a bucket in another region,
  naming the region from x-amz-bucket-region, instead of an unexplained
  error; a 404 is reported as a missing bucket.
- CreateBucket passes a LocationConstraint outside us-east-1.
- A missing Content-Length or Size is an error, not 0; a missing
  LastModified is None, not the epoch.

Trait:
- put() returns ObjectMeta (bytes written, ETag when the store has one)
  instead of (). The backup manifest writer needs both and should not need
  a second head() per data file.
- ObjectMeta gains etag: Option<String>. S3 fills it from PutObject,
  CompleteMultipartUpload, HeadObject and ListObjectsV2 with the quotes
  stripped; the filesystem store reports None.
- MIN_PART_SIZE_BYTES is exported so tests do not duplicate the floor.

Tests:
- Conformance: put metadata agrees with head and list on both stores
  (including the empty object); created files/dirs are 0600/0700; any root
  mode with group or other bits (0720, 0702, 0777, 1777, 0750, 0755, 0705,
  0701) is refused and 0700/0500 accepted; a world-writable ancestor is
  refused and a sticky one accepted; a permissive subdirectory is refused
  by put, get, head, list and delete_prefix; a root widened after open is
  refused by every operation; a symlinked root is canonicalized; a put over
  a directory is refused; a failed put leaves no directory skeleton; the
  validate probe is never left behind. Symlink coverage covers a dangling
  link at the destination, a dangling link in the parent chain, a relative
  link, and a link chain.
- The S3 suite previously returned Ok when EXTENDDB_TEST_S3_ENDPOINT was
  unset, so it never ran in CI. All skip paths go through one helper and
  EXTENDDB_TEST_S3_REQUIRED turns a missing endpoint into a failure.
  test.yml starts moto in server mode (pinned there, not in
  requirements.txt), runs the workspace with the store crate excluded, then
  runs the store crate with the S3 variables scoped to that step. MinIO was
  dropped because the project archived in April 2026 and no longer
  publishes images; any S3-compatible endpoint still works locally.

Not changed: list() still collects the filesystem walk into a Vec before
streaming. The trait promises lexicographic order and a directory walk
cannot provide that incrementally; listings under the backup layout are
small (one backup prefix is a few dozen objects).

Co-authored-by: Lee Hannigan <lhnng@amazon.com>
@robinnsc
robinnsc force-pushed the feat/backup-store-hardening branch from 954b1b3 to 97512f1 Compare October 13, 2026 06:50
robinnsc added a commit that referenced this pull request Oct 13, 2026
…t code

Revision of docs/design/15-backup-restore.md after reviewing the draft
against main 9f43d03 and PRs #384, #335, #336, #337, measuring the read,
compression, and bulk-load paths on PostgreSQL 16 and SQLite with a
1M-item fixture, and two review rounds against the companion PRs #393,
#394 and #395. The revision log at the end of the document lists each
change; the ones that alter behavior:

- SnapshotEngine returns the table definition together with the items
  from one fenced operation (TableSnapshot). The items-only signature in
  the draft allowed a GSI change or DeleteTable between the definition
  read and the data snapshot. Indexes are reported in whatever build
  state they had and only an index being deleted is dropped; an
  ACTIVE-only filter would have left a GSI mid-backfill out of the backup
  for good.
- The snapshot is opened in the CreateBackup request so the backup
  represents the request time, as DynamoDB documents;
  backup_creation_date_time is the instant the snapshot pinned.
- Per-job claims with fencing replace the single-instance lease the
  draft assumed; PostgreSQL and MongoDB have no such lease. The claim is
  an advisory lock on a dedicated catalog connection on PostgreSQL (the
  seed #384's restore lock already uses), the row itself on MongoDB, an
  in-process slot on SQLite; no row ever exists unowned. Heartbeats are a
  timer, staleness is measured on the database's clock, and a failed
  heartbeat aborts the job. The reconciler resolves a dead CREATING row
  at once under a fresh generation conditioned on status = CREATING and
  deletes its prefix only after the stale window.
- DeleteBackup tombstones the row as DELETING, deletes the manifest
  first, and marks DELETED only when the prefix is empty; backup sync is
  scoped to the configured store and never adopts a CREATING, DELETING,
  DELETED or FAILED prefix. Failed creations and deleted backups are
  BackupNotFound, as on the service, and share one retention.
- Restore returns CREATING from the request (table and restore_jobs row
  created atomically; ResourceInUseException on a name race,
  LimitExceededException over the job limit), spools and verifies every
  data file before writing any item, and records progress in a way a
  crash cannot split from the data: a data-database row per file on
  PostgreSQL (data migration 005 reserved), the same transaction per
  10k-item batch on SQLite, idempotent re-application of one file on
  MongoDB followed by a document-count check against the manifest.
  Duplicate detection for unordered backups is the target's primary key,
  not an in-memory key set. The ACTIVE flip is conditional; DeleteTable
  on a restore target fails the job.
- The manifest self-checksum is replaced by a sidecar over the stored
  bytes; the draft's canonical form depended on serde_json formatting.
  backup_arn is the backup's identity and source_table must agree with
  it. N key values are normalized before key text or key order is
  computed; the SQL ORDER BY recipe includes COLLATE "C" on PostgreSQL.
- Index population on restore stays per backend via bulk_load; the
  shared restore-backfill worker the draft described exists only on
  MongoDB.
- Format additions: item_order, per-file key_range, md5 and
  uncompressed_bytes, explicit nulls, three-state vector_indexes of
  creation specs, optional etag, dataFileS3Key as the full object key,
  partition-aware BackupArn as the single ARN implementation.
- Filesystem store: owner-only root re-checked by every operation,
  owner-only rule on every descended directory including list and
  delete_prefix, ancestor ownership rule, objects 0600 and directories
  0700 at creation, put returns object metadata; the consequence for
  backup pickup agents is stated. The S3 conformance suite is mandatory
  in CI against moto (MinIO no longer publishes images).
- ListBackups paging and filtering, the restore override contract,
  sqlite-memory behavior, extenddb destroy leaving the store intact, a
  restore spool outside the store root, max_concurrent_jobs,
  heartbeat_interval_secs and a concrete pool-size rule are specified.
- Phase 1 is rewritten as the PR series with acceptance criteria and
  GitHub numbers. Migration numbers are reserved up front so the PITR PR
  is not on the critical path.

Co-authored-by: Lee Hannigan <lhnng@amazon.com>
@robinnsc robinnsc changed the title fix(backup-store): private file modes, root ownership check, put metadata, S3 suite in CI [backup/restore 2/4] fix(backup-store): private file modes, owner-only root, put metadata, S3 suite in CI [backup/restore 2/4] Oct 13, 2026

This branch has not been deployed

No deployments
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