fix: deny public read on MinIO signatures prefix #209

Merged

forgejo_admin merged 1 commit from 186-minio-signature-policy-fix into main

2026-03-28 00:07:47 +00:00

forgejo_admin commented

2026-03-27 22:12:51 +00:00

Owner

Summary

Contract signature images in assets/westside/signatures/ were publicly accessible via the MinIO CDN because the bucket policy granted s3:GetObject on arn:aws:s3:::assets/* with no exclusions. This adds an explicit Deny statement for the signatures prefix so they return 403, while all other CDN assets (branding, coaches, sponsors, email images) remain publicly readable.

Changes

terraform/modules/storage/main.tf:75-96 -- Replaced single Allow-all statement with a two-statement policy: an explicit DenyPublicSignatures on assets/westside/signatures/* followed by AllowPublicAssets on assets/*. In S3 policy evaluation, explicit Deny always overrides Allow.

tofu plan Output

# module.storage.minio_s3_bucket_policy.assets_public_read will be updated in-place
~ resource "minio_s3_bucket_policy" "assets_public_read" {
      id     = "assets"
    ~ policy = jsonencode(
        ~ {
            ~ Statement = [
                ~ {
                    ~ Effect    = "Allow" -> "Deny"
                    ~ Resource  = [
                        - "arn:aws:s3:::assets/*",
                        + "arn:aws:s3:::assets/westside/signatures/*",
                      ]
                    + Sid       = "DenyPublicSignatures"
                  },
                + {
                    + Action    = ["s3:GetObject"]
                    + Effect    = "Allow"
                    + Principal = { AWS = ["*"] }
                    + Resource  = ["arn:aws:s3:::assets/*"]
                    + Sid       = "AllowPublicAssets"
                  },
              ]
          }
      )
  }

Plan: 0 to add, 1 to change, 0 to destroy.

Test Plan

tofu apply the change
curl -sI "https://minio-api.tail5b443a.ts.net/assets/westside/signatures/108_1774343239303.png" returns 403
curl -sI "https://minio-api.tail5b443a.ts.net/assets/westside/branding/logo.png" returns 200 (public assets unaffected)
Verify westside-contracts can still upload signatures via service account credentials (write path uses IAM user, not anonymous policy)
Verify email image CDN URLs still resolve (e.g. sponsor/coach images in pal-e-mail templates)

Review Checklist

Passed tofu fmt and tofu validate
No secrets committed
No unnecessary file changes (1 file, 1 resource)
Commit messages are descriptive

Closes #186
project-westside-basketball -- the project this work belongs to
arch-deployment-westside-basketball -- related architecture
story:WS-S4 (static assets via public CDN)

## Summary Contract signature images in `assets/westside/signatures/` were publicly accessible via the MinIO CDN because the bucket policy granted `s3:GetObject` on `arn:aws:s3:::assets/*` with no exclusions. This adds an explicit Deny statement for the signatures prefix so they return 403, while all other CDN assets (branding, coaches, sponsors, email images) remain publicly readable. ## Changes - `terraform/modules/storage/main.tf:75-96` -- Replaced single Allow-all statement with a two-statement policy: an explicit `DenyPublicSignatures` on `assets/westside/signatures/*` followed by `AllowPublicAssets` on `assets/*`. In S3 policy evaluation, explicit Deny always overrides Allow. ## tofu plan Output ``` # module.storage.minio_s3_bucket_policy.assets_public_read will be updated in-place ~ resource "minio_s3_bucket_policy" "assets_public_read" { id = "assets" ~ policy = jsonencode( ~ { ~ Statement = [ ~ { ~ Effect = "Allow" -> "Deny" ~ Resource = [ - "arn:aws:s3:::assets/*", + "arn:aws:s3:::assets/westside/signatures/*", ] + Sid = "DenyPublicSignatures" }, + { + Action = ["s3:GetObject"] + Effect = "Allow" + Principal = { AWS = ["*"] } + Resource = ["arn:aws:s3:::assets/*"] + Sid = "AllowPublicAssets" }, ] } ) } Plan: 0 to add, 1 to change, 0 to destroy. ``` ## Test Plan - [ ] `tofu apply` the change - [ ] `curl -sI "https://minio-api.tail5b443a.ts.net/assets/westside/signatures/108_1774343239303.png"` returns 403 - [ ] `curl -sI "https://minio-api.tail5b443a.ts.net/assets/westside/branding/logo.png"` returns 200 (public assets unaffected) - [ ] Verify westside-contracts can still upload signatures via service account credentials (write path uses IAM user, not anonymous policy) - [ ] Verify email image CDN URLs still resolve (e.g. sponsor/coach images in pal-e-mail templates) ## Review Checklist - [x] Passed `tofu fmt` and `tofu validate` - [x] No secrets committed - [x] No unnecessary file changes (1 file, 1 resource) - [x] Commit messages are descriptive ## Related Notes - Closes #186 - `project-westside-basketball` -- the project this work belongs to - `arch-deployment-westside-basketball` -- related architecture - story:WS-S4 (static assets via public CDN)

forgejo_admin added 1 commit

2026-03-27 22:12:51 +00:00

fix: deny public read on assets/westside/signatures/ prefix

ci/woodpecker/push/woodpecker Pipeline was successful

Details

ci/woodpecker/pr/woodpecker Pipeline was successful

Details

ci/woodpecker/pull_request_closed/woodpecker Pipeline was successful

Details

3fa13f023d

The MinIO assets bucket policy granted s3:GetObject on assets/* which
exposed contract signature images. Add an explicit Deny statement for
the westside/signatures/ prefix so signatures return 403 while all
other CDN assets (branding, coaches, sponsors) remain publicly readable.

Closes #186

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

forgejo_admin commented

2026-03-27 22:13:35 +00:00

Author

Owner

QA Review -- PR #209

Diff Analysis

Single file change: terraform/modules/storage/main.tf (+16, -6). Replaces a single Allow-all bucket policy statement with a two-statement policy adding an explicit Deny on assets/westside/signatures/*.

Findings

Policy correctness: The S3/MinIO policy evaluation model grants explicit Deny absolute precedence over Allow, regardless of statement ordering. This is the correct and idiomatic approach -- no edge cases where Allow could override.

Authenticated access unaffected: The Deny targets Principal: { AWS: ["*"] } (anonymous public access only). Service accounts authenticating via IAM credentials (e.g., the westside-contracts upload user) are governed by their own IAM policy, not this anonymous bucket policy. Write path is safe.

Formatting and validation: tofu fmt produced no changes, tofu validate passed. Plan output included and shows exactly 1 in-place update.

Scope: Tight -- 1 resource, 1 file, no blast radius. No changes to other buckets, IAM users, or policies.

Nits

None.

VERDICT: APPROVE

Clean security fix. Explicit Deny is the correct S3 policy pattern. No regressions possible on the public CDN paths or the authenticated upload path.

## QA Review -- PR #209 ### Diff Analysis Single file change: `terraform/modules/storage/main.tf` (+16, -6). Replaces a single Allow-all bucket policy statement with a two-statement policy adding an explicit Deny on `assets/westside/signatures/*`. ### Findings **Policy correctness:** The S3/MinIO policy evaluation model grants explicit Deny absolute precedence over Allow, regardless of statement ordering. This is the correct and idiomatic approach -- no edge cases where Allow could override. **Authenticated access unaffected:** The Deny targets `Principal: { AWS: ["*"] }` (anonymous public access only). Service accounts authenticating via IAM credentials (e.g., the westside-contracts upload user) are governed by their own IAM policy, not this anonymous bucket policy. Write path is safe. **Formatting and validation:** `tofu fmt` produced no changes, `tofu validate` passed. Plan output included and shows exactly 1 in-place update. **Scope:** Tight -- 1 resource, 1 file, no blast radius. No changes to other buckets, IAM users, or policies. ### Nits None. ### VERDICT: APPROVE Clean security fix. Explicit Deny is the correct S3 policy pattern. No regressions possible on the public CDN paths or the authenticated upload path.