Skip to content

fix: authorize explicit version deletes with DeleteObjectVersion - #104

Merged
Vonng merged 5 commits into
mainfrom
codex/issue-58-delete-version-authz
Sep 1, 2026
Merged

fix: authorize explicit version deletes with DeleteObjectVersion#104
Vonng merged 5 commits into
mainfrom
codex/issue-58-delete-version-authz

Conversation

@Vonng

@Vonng Vonng commented Sep 1, 2026

Copy link
Copy Markdown
Member

Contribution Licensing (no CLA, inbound=outbound, DCO required)

Every commit in this PR carries a DCO Signed-off-by trailer. The first commit is the deterministic red reproduction requested by issue #58.

Stacked PR: this PR is based on codex/fix-cors-replication-trust / #101 because it reuses the unified replication-trust decision. Retarget to main after #101 merges.

Description

Select delete authorization from the request's effective version ID:

Request Action
no versionId s3:DeleteObject
explicit UUID s3:DeleteObjectVersion
explicit versionId=null s3:DeleteObjectVersion
DeleteObjects evaluated independently per XML entry

The PR:

  • removes the generic ordinary-request DenyOnly DeleteObjectVersion compatibility block;
  • authenticates DeleteObjects once, then authorizes each entry independently;
  • binds s3:versionid conditions to each effective entry, not an outer query or copy-source decoy;
  • preserves authorization-before-UUID-validation error behavior;
  • retains the deployed trusted-replication target contract in a dedicated helper: exact marker + Allow ReplicateDelete + Allow DeleteObject, with an explicit Deny DeleteObjectVersion still blocking a named purge;
  • keeps untrusted internal headers on the ordinary action-mapping path;
  • updates the existing two-site replication test with a target user that has DeleteObject+ReplicateDelete but deliberately lacks DeleteObjectVersion;
  • removes the test script's unrelated AWS CLI dependency in favor of mc rm --version-id.

Refs #58.

Motivation and Context

Previously, SILO authorized every object delete as s3:DeleteObject and used s3:DeleteObjectVersion only as an explicit-deny check. A DeleteObject-only principal could permanently remove historical versions, while a least-privilege DeleteObjectVersion-only purge principal could not perform an explicit version delete.

A direct action remap on trusted replication would break every documented target policy and silently stall permanent-delete replication. This PR therefore separates ordinary S3 authorization from the already-deployed receiver compatibility contract.

How to test this PR?

Passed locally on macOS/arm64 with Go 1.27.0:

  • single and multi-delete tests covering no ID, UUID, null, malformed ID, padded ID, query decoy, per-entry Null/StringEquals conditions, explicit deny, and error ordering;
  • trusted replication tests covering the documented minimal target policy, explicit version deny, and marker-only negative control;
  • existing broad DeleteObject/DeleteMultipleObjects and replication-trust tests;
  • focused suite under -race;
  • go build ./...;
  • go vet ./cmd;
  • go test ./cmd -count=1 (final run: 131.769s);
  • bash -n docs/bucket/replication/delete-replication.sh;
  • real make test-delete-replication with race binaries and two local sites. A target repluser policy contained DeleteObject+ReplicateDelete but no DeleteObjectVersion; permanent version deletion and delete-marker replication both converged.

golangci-lint reports only the four known findings inherited from #101 (two replication-trust.go formatting/revive findings and two CORS test misspellings); this delta adds no lint finding.

Claude Code Opus 5 performed an adversarial implementation review and a targeted rereview. Final verdict: GO, no P0/P1/P2.

Compatibility impact

  • Intentional authorization change: ordinary principals with only s3:DeleteObject lose the ability to delete an explicitly named UUID or null version. Grant s3:DeleteObjectVersion where that operation is intended.
  • A DeleteObjectVersion-only principal can purge a named version but cannot create a delete marker.
  • Explicit denies and s3:versionid conditions retain normal precedence; DeleteObjects may return mixed per-entry results.
  • Replication target policies remain unchanged: DeleteObject+ReplicateDelete is sufficient; an explicit Deny DeleteObjectVersion still blocks a purge.
  • External authorization plugins see one DeleteObjectVersion action for an ordinary named-version delete instead of the former deny-only check followed by DeleteObject.
  • X-Minio-Force-Delete prefix cleanup remains DeleteObject-gated and is not remapped by this PR.
  • Directory-object implicit-null cleanup is inherited and deliberately out of scope.
  • No wire, object, or metadata format changes. Rollback restores the old authorization mapping without data migration.

Types of changes

  • Bug fix
  • New feature
  • Optimization
  • Breaking change (ordinary explicit-version authorization becomes stricter)

Checklist:

  • All commits are signed off (git commit -s) per the DCO
  • Unit tests added/updated
  • make verifiers passes
  • Relevant package tests and build pass
  • Compatibility and rollback impact documented
  • Internal documentation updated
  • Public EN/ZH documentation prepared in pgsty/silo.pgsty.com

@Vonng

Vonng commented Sep 1, 2026

Copy link
Copy Markdown
Member Author

Final stacked validation update:

The PR remains based on #101 and should be retargeted to main after #101 merges; no merge/tag/release/deployment has been performed.

Prove that DeleteObjectVersion-only principals cannot delete named UUID or null versions while DeleteObject-only principals can, contrary to the S3 action mapping in issue #58.

Signed-off-by: Feng Ruohang <rh@vonng.com>
Select DeleteObject or DeleteObjectVersion from each request's effective version ID for single and multi-delete. Authenticate multi-delete once, bind version conditions to each XML entry, and keep the established DeleteObject plus ReplicateDelete target contract with explicit version denies honored only on trusted replication.\n\nRefs: #58

Signed-off-by: Feng Ruohang <rh@vonng.com>
Document that replication targets retain the DeleteObject plus ReplicateDelete contract and extend the existing two-site test with a target user that lacks DeleteObjectVersion. Use mc for explicit version deletion so the gate no longer depends on AWS CLI.

Signed-off-by: Feng Ruohang <rh@vonng.com>
Authenticate DeleteObjects before validating entry count, remove the obsolete per-version auth helper, and pin the marker-only request to the ordinary authorization path.\n\nRefs: #58

Signed-off-by: Feng Ruohang <rh@vonng.com>
@Vonng
Vonng force-pushed the codex/issue-58-delete-version-authz branch from 770ccfc to 6d65517 Compare September 1, 2026 23:08
@Vonng
Vonng changed the base branch from codex/fix-cors-replication-trust to main September 1, 2026 23:08
Accept the additional DeleteObjects route literal introduced by the issue #58 regression matrix.

Signed-off-by: Feng Ruohang <rh@vonng.com>
@Vonng
Vonng force-pushed the codex/issue-58-delete-version-authz branch from 6d65517 to d2d47a4 Compare September 1, 2026 23:08
@Vonng
Vonng merged commit 0a9c777 into main Sep 1, 2026
9 checks passed
@Vonng
Vonng deleted the codex/issue-58-delete-version-authz branch September 2, 2026 15:06
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