Skip to content

fix(terraform): drop the unencrypted-upload opt-out and reject collector + managed scan together - #91

Closed
tim-thacker-nullify wants to merge 1 commit into
fix/terraform-managed-scan-review-r2from
fix/terraform-managed-scan-review-r3
Closed

tim-thacker-nullify wants to merge 1 commit into
fix/terraform-managed-scan-review-r2from
fix/terraform-managed-scan-review-r3

Conversation

@tim-thacker-nullify

@tim-thacker-nullify tim-thacker-nullify commented Sep 15, 2026 •

Copy link
Copy Markdown
Member

Claude

Round-3 review fixes for #88 and #89, stacked on #89's head branch.

R3-1, R3-6: allow_unencrypted_upload is removed, not plumbed.

  • An empty kms_key_arn also sets enable_kms_access = false, so the role gets no KMS statement.
  • The destination bucket sets SSE-KMS default encryption (monorepo context/iac/s3.tf:45-51, dast/iac/s3.tf:41-47), so a headerless upload is still encrypted with Nullify's key and needs kms:GenerateDataKey on it.
  • That key is Nullify's, reachable only through kms_key_arn, so the opt-out cannot be made to work.
  • The precondition is now kms_key_arn != "" alone; variable description, README KMS section and upgrade notes match.

Other findings:

Removing a declared variable breaks a direct module caller that sets it; no shipped example or root does. Nothing run locally — no terraform, helm, kubectl or AWS calls.

🤖 Generated with Claude Code


View with [code]smith Autofix with [code]smith
Need help on this PR? Tag @codesmith-bot with what you need. Autofix is disabled.

…tor + managed scan together

allow_unencrypted_upload could not work against Nullify's bucket: an empty
kms_key_arn also sets enable_kms_access = false, so the role gets no KMS
statement, and the bucket's SSE-KMS default encryption still requires
kms:GenerateDataKey on Nullify's key. The variable traded the collector's
self-explaining error for an opaque S3 AccessDenied, so it is removed and
kms_key_arn is unconditionally required when enable_collector is true.

- k8s-resources: remove allow_unencrypted_upload and the ALLOW_UNENCRYPTED_UPLOAD
  env block; the KMS precondition is now kms_key_arn != "" alone
- k8s-resources: precondition rejecting enable_collector together with
  enable_managed_scan_rbac, matching the example's scan_mode = "both" rejection
- nullify-aws-integration: kms_key_arn description now states the cross-account
  rule the locals actually implement
- terraform/README.md: fold #88's bare-key-form note into the rewritten KMS
  section so merging both PRs cannot leave two near-duplicate paragraphs
- multi-cluster-complete: comment out the placeholder KMS ARN and shape it so
  the variable regex rejects it if pasted as-is
- CI: validate rows for the four remaining modules/ directories

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@tim-thacker-nullify

Copy link
Copy Markdown
Member Author

Claude

Collapsed into #78 at fa505f0 after passing adversarial review; all of this PR's commits are now in #78.

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