amadan

carlos / platform Public

objstore: multipart and copy were open too, and now something says so

"By either route" was two routes. The review asked what the third one
is, and there were two more, both authorised as plain s3:PutObject with
the tags arriving somewhere other than a simple PUT's x-amz-tagging
header: CreateMultipartUpload with x-amz-tagging, and CopyObject with a
REPLACE tagging directive. Multipart is the one an app does not even
have to choose — any upload over the SDK's threshold takes that path.

Against MinIO with the Deny removed, BOTH set carlos-scan=clean. With
the Deny in place, both refuse. So the s3:RequestObjectTagKeys condition
was already covering them — the gateway populates that key in those
handlers too — and nothing anywhere tested it. The fix from 6fba2b3 was
broader than the evidence for it; now the evidence matches.

AND THE CONTROLS, WITHOUT WHICH THAT PARAGRAPH IS WORTHLESS. Both new
subtests assert AccessDenied, so if multipart and copy were denied
ANYWAY — never permitted by the app policy in the first place — they
would pass while saying nothing about tagging, and a policy that later
lost the tag-key Deny would still look green. Each route is therefore
exercised again without our tag keys and must SUCCEED. Plus a third
control: an ordinary upload carrying a tag of the app's own
(team=payments) must still work, or the Deny is firing on every write
and the store is merely broken.

Verified by mutation rather than by reading: with the fixture's Deny
removed, all three tagged routes succeed and the three subtests fail;
with it, all three refuse and all three controls pass.

Also free, and taken: s3:PutObjectVersionTagging and
s3:DeleteObjectVersionTagging join the first Deny. Nothing here ever
calls PutBucketVersioning so they are unreachable today, which is
exactly when a shape is cheapest to close.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PEkJh1ESqiA84quAHJV4K6
Paul Campbell pushed by paul@keymail.dev cab8ea26e9a928d196f1e91fb98cd92ec2558d46 parent 196f03f
3 files changed, +139 −3
  • .amadan/ci.d/30-minio-contract +1 −1
  • internal/objstore/aws.go +9 −2
  • internal/objstore/contract_test.go +129 −0
diff --git a/.amadan/ci.d/30-minio-contract b/.amadan/ci.d/30-minio-contract
index 7c2ab4d5b..7cf3f8bb4 100755
--- a/.amadan/ci.d/30-minio-contract
+++ b/.amadan/ci.d/30-minio-contract
@@ -275,7 +275,7 @@ cat >"$rundir/policy.json" <<'EOF'
{"Version": "2012-10-17", "Statement": [
{"Effect": "Allow", "Action": ["s3:ListBucket"], "Resource": ["arn:aws:s3:::contract-bucket"]},
{"Effect": "Allow", "Action": ["s3:GetObject", "s3:PutObject", "s3:DeleteObject", "s3:GetObjectTagging"], "Resource": ["arn:aws:s3:::contract-bucket/*"]},
- {"Effect": "Deny", "Action": ["s3:PutObjectTagging", "s3:DeleteObjectTagging"], "Resource": ["arn:aws:s3:::contract-bucket/*"]},
+ {"Effect": "Deny", "Action": ["s3:PutObjectTagging", "s3:DeleteObjectTagging", "s3:PutObjectVersionTagging", "s3:DeleteObjectVersionTagging"], "Resource": ["arn:aws:s3:::contract-bucket/*"]},
{"Effect": "Deny", "Action": ["s3:PutObject"], "Resource": ["arn:aws:s3:::contract-bucket/*"],
"Condition": {"ForAnyValue:StringEquals": {"s3:RequestObjectTagKeys": ["carlos-scan", "carlos-scan-version", "carlos-scan-at"]}}}
]}
diff --git a/internal/objstore/aws.go b/internal/objstore/aws.go
index 438821bde..a963de3c2 100644
--- a/internal/objstore/aws.go
+++ b/internal/objstore/aws.go
@@ -186,8 +186,15 @@ func storePolicyJSON(bucketARN string, deny *DenyTag) string {
// exists and why the first is not sufficient on its own.
doc.Statement = append(doc.Statement,
policyStatement{
- Effect: "Deny",
- Action: []string{"s3:PutObjectTagging", "s3:DeleteObjectTagging"},
+ Effect: "Deny",
+ Action: []string{
+ "s3:PutObjectTagging", "s3:DeleteObjectTagging",
+ // Nothing here ever calls PutBucketVersioning, so these two
+ // are unreachable today. They cost nothing and close the shape
+ // before a bucket is ever versioned — which is the cheap half
+ // of the lesson this policy has already learned once.
+ "s3:PutObjectVersionTagging", "s3:DeleteObjectVersionTagging",
+ },
Resource: bucketARN + "/*",
},
policyStatement{
diff --git a/internal/objstore/contract_test.go b/internal/objstore/contract_test.go
index 495f1c483..8e6e84e71 100644
--- a/internal/objstore/contract_test.go
+++ b/internal/objstore/contract_test.go
@@ -218,6 +218,135 @@ func TestDeliveredContractMinio(t *testing.T) {
requireDenied(t, err, "PutObject with x-amz-tagging")
})
})
+
+ // THE OTHER ROUTES. "By either route" was two routes, and the property has
+ // more than two.
+ //
+ // Each of these is authorised as plain `s3:PutObject` and carries tags
+ // somewhere other than the `x-amz-tagging` header of a simple PUT, so the
+ // Deny added for that header is not obviously in the path. Whether a gateway
+ // populates `s3:RequestObjectTagKeys` in these handlers is a gateway-specific
+ // fact of exactly the kind that already bit this design once — which is why
+ // they are asserted against a real server rather than reasoned about.
+ t.Run("the app cannot tag by any other route", func(t *testing.T) {
+ // Multipart: any upload over the SDK's threshold takes this path, so
+ // an app does not even have to choose it deliberately.
+ t.Run("not by CreateMultipartUpload", func(t *testing.T) {
+ const key = "contract-test/multipart-tagged.bin"
+ out, err := client.CreateMultipartUpload(ctx, &awss3.CreateMultipartUploadInput{
+ Bucket: aws.String(bucket),
+ Key: aws.String(key),
+ Tagging: aws.String("carlos-scan=clean"),
+ })
+ if err == nil {
+ // Nothing was completed, but the upload id exists; clean it up so
+ // the bucket is not left holding a live multipart session.
+ _, _ = client.AbortMultipartUpload(ctx, &awss3.AbortMultipartUploadInput{
+ Bucket: aws.String(bucket), Key: aws.String(key), UploadId: out.UploadId,
+ })
+ }
+ requireDenied(t, err, "CreateMultipartUpload with x-amz-tagging")
+ })
+
+ // Copy with REPLACE: the app already has GetObject and PutObject, so it
+ // can copy its own object over itself and name the tags on the way.
+ t.Run("not by CopyObject with a REPLACE tagging directive", func(t *testing.T) {
+ const src = "contract-test/copy-src.txt"
+ if _, err := client.PutObject(ctx, &awss3.PutObjectInput{
+ Bucket: aws.String(bucket),
+ Key: aws.String(src),
+ Body: bytes.NewReader([]byte("payload")),
+ }); err != nil {
+ t.Fatalf("seed the copy source: %v", err)
+ }
+ defer client.DeleteObject(ctx, &awss3.DeleteObjectInput{
+ Bucket: aws.String(bucket), Key: aws.String(src),
+ })
+
+ const dst = "contract-test/copy-tagged.txt"
+ _, err := client.CopyObject(ctx, &awss3.CopyObjectInput{
+ Bucket: aws.String(bucket),
+ Key: aws.String(dst),
+ CopySource: aws.String(bucket + "/" + src),
+ TaggingDirective: s3types.TaggingDirectiveReplace,
+ Tagging: aws.String("carlos-scan=clean"),
+ })
+ if err == nil {
+ _, _ = client.DeleteObject(ctx, &awss3.DeleteObjectInput{
+ Bucket: aws.String(bucket), Key: aws.String(dst),
+ })
+ }
+ requireDenied(t, err, "CopyObject with a REPLACE tagging directive")
+ })
+
+ // THE CONTROLS, WITHOUT WHICH THE TWO ABOVE PROVE NOTHING.
+ //
+ // Both assert AccessDenied. If these operations were denied anyway —
+ // because the app policy never permitted multipart or copy in the
+ // first place — the assertions would pass while saying nothing at all
+ // about tagging, and a later policy that lost the tag-key Deny would
+ // still look green. So each route is exercised again WITHOUT our tag
+ // keys and must succeed: that is what makes the refusals above
+ // attributable to the tag and not to the verb.
+ t.Run("but multipart itself is allowed", func(t *testing.T) {
+ const key = "contract-test/multipart-plain.bin"
+ out, err := client.CreateMultipartUpload(ctx, &awss3.CreateMultipartUploadInput{
+ Bucket: aws.String(bucket),
+ Key: aws.String(key),
+ })
+ if err != nil {
+ t.Fatalf("CreateMultipartUpload without tags = %v — the refusal above was about the VERB, not the tag, and proves nothing", err)
+ }
+ _, _ = client.AbortMultipartUpload(ctx, &awss3.AbortMultipartUploadInput{
+ Bucket: aws.String(bucket), Key: aws.String(key), UploadId: out.UploadId,
+ })
+ })
+
+ t.Run("and copy itself is allowed", func(t *testing.T) {
+ const src = "contract-test/copy-control-src.txt"
+ const dst = "contract-test/copy-control-dst.txt"
+ if _, err := client.PutObject(ctx, &awss3.PutObjectInput{
+ Bucket: aws.String(bucket), Key: aws.String(src),
+ Body: bytes.NewReader([]byte("payload")),
+ }); err != nil {
+ t.Fatalf("seed: %v", err)
+ }
+ defer client.DeleteObject(ctx, &awss3.DeleteObjectInput{
+ Bucket: aws.String(bucket), Key: aws.String(src),
+ })
+ _, err := client.CopyObject(ctx, &awss3.CopyObjectInput{
+ Bucket: aws.String(bucket),
+ Key: aws.String(dst),
+ CopySource: aws.String(bucket + "/" + src),
+ })
+ if err != nil {
+ t.Fatalf("CopyObject without tags = %v — the refusal above was about the VERB, not the tag, and proves nothing", err)
+ }
+ _, _ = client.DeleteObject(ctx, &awss3.DeleteObjectInput{
+ Bucket: aws.String(bucket), Key: aws.String(dst),
+ })
+ })
+
+ // And the same control for the plain PUT route, which has the same
+ // hazard: an ordinary untagged upload must still work, or the Deny is
+ // firing on every write and the store is simply broken.
+ t.Run("and an ordinary tagged-with-something-else upload is allowed", func(t *testing.T) {
+ const key = "contract-test/foreign-tag.txt"
+ _, err := client.PutObject(ctx, &awss3.PutObjectInput{
+ Bucket: aws.String(bucket),
+ Key: aws.String(key),
+ Body: bytes.NewReader([]byte("payload")),
+ Tagging: aws.String("team=payments"),
+ })
+ if err != nil {
+ t.Fatalf("PutObject with a tag of the app's own = %v — the Deny is keyed on OUR tag names and must not touch a tenant's", err)
+ }
+ _, _ = client.DeleteObject(ctx, &awss3.DeleteObjectInput{
+ Bucket: aws.String(bucket), Key: aws.String(key),
+ })
+ })
+ })
+
t.Run("delete removes it", func(t *testing.T) {
_, err := client.DeleteObject(ctx, &awss3.DeleteObjectInput{
Bucket: aws.String(bucket),