[IAmJSD]

Root cause

aws_iam_role_policy.secrets_access in terraform/modules/compute/aws/lambda/main.tf builds its secretsmanager:GetSecretValue resource list with compact([...]). The admin, credential-encryption, scheduled-task and Archera entries are written as X != "" ? "${X}*" : "", but the database entry was a bare "${var.database_password_secret_arn}*". With an empty ARN that element renders as the literal "*", compact() keeps it, and the Lambda role can read every secret in the account. database_password_secret_arn had no validation, so nothing stopped the empty value.

The only in-tree caller passes module.database.password_secret_arn, so this was a latent fail-open, not a live over-grant (as the issue says).

Fix

1. Validation on database_password_secret_arn (lambda/variables.tf): the value must be a concrete Secrets Manager secret ARN, ^arn:aws(-[a-z]+)*:secretsmanager:[a-z0-9-]+:[0-9]{12}:secret:[A-Za-z0-9/_+=.@-]+$. That rejects an empty value, null, a bare *, and wildcards in the region, account or name, any of which would widen the grant. Partitions aws, aws-us-gov and aws-cn are accepted. The name charset is the one Secrets Manager allows (/_+=.@- plus alphanumerics), so it covers the random -AbCdEf suffix on real ARNs. During plan the caller's value is unknown (the secret is created in the same apply), so the check is deferred and doesn't break the environment root.
2. Guard in lambda/main.tf: the "${arn}*" entry is now arn != "" ? "${arn}*" : "", the same as its siblings. While the validation holds, this branch can't be reached through the variable. It is there for the issue's failure scenario: if a later refactor makes the input optional and drops the validation, the policy still fails closed (see mutation M2 below).

Verification

All evidence comes from unit tests and terraform test with mock_provider "aws" (plan only). Nothing ran against a real AWS account, no plan used a real backend, and nothing was applied.

New test: terraform/modules/compute/aws/lambda/tests/secrets_policy.tftest.hcl (8 runs). It pins the exact rendered GetSecretValue resource list, checks that no statement grants "*", accepts a GovCloud ARN, and rejects empty, *, a wildcard name, a wildcard account and a non-Secrets-Manager ARN.

d=terraform/modules/compute/aws/lambda
terraform -chdir=$d init -backend=false -lockfile=readonly -input=false
terraform -chdir=$d fmt -check -recursive
terraform -chdir=$d test

Fail-before (main's main.tf and variables.tf with the new test file):

run "rejects_empty_db_secret_arn"... fail
Error: Missing expected failure
The checkable object, var.database_password_secret_arn, was expected to
report an error but did not.
Failure! 3 passed, 1 failed, 4 skipped.

I also ran a throwaway probe on main (database_password_secret_arn = "", assert that the resource list does not contain "*"; not committed). It reproduces the issue directly:

aws_iam_role_policy.secrets_access.policy is "{\"Statement\":[{\"Action\":[\"secretsmanager:GetSecretValue\"],\"Effect\":\"Allow\",\"Resource\":[\"*\"]}, ...
GetSecretValue Resource = ["*"]

Pass-after: Terraform 1.10.5 (the CI pin) and 1.15.8 both give Success! 15 passed, 0 failed. for the whole module (new file plus the existing sweeps and timeout tests). On the fixed code, the same probe fails at plan with the new validation message.

Mutation probes:

• M1, validation kept and guard reverted: the suite still passes, 8/8. This is expected, since validation rejects the empty value before the guard matters. The guard is defense-in-depth and no test pins it through the variable.
• M2, guard kept and validation removed: rejects_empty_db_secret_arn fails (caught). The probe with an empty ARN now renders no "*", so the guard alone fails closed.
• M3, regex loosened to ^arn:aws[^:]*:secretsmanager:.+$: rejects_wildcard_secret_name fails (caught).

Other checks run locally:

• terraform fmt -check -recursive terraform/ passes. terraform validate passes on the lambda module and on terraform/environments/aws (init -backend=false) with 1.10.5.
• IAM guard and other Terraform Go tests: AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null go test ./terraform/modules/compute/... ./terraform/environments/aws/... -count=1 all pass. The /dev/null overrides are needed only because this machine's ~/.aws/config default profile is invalid.
• scripts/check-aws-iam-parity.sh passes, and scripts/test-aws-iam-parity.sh gives 12 passed, 0 failed.
• trivy config --severity HIGH,CRITICAL on the lambda module: 0 misconfigurations.
• tflint 0.58.1 with the repo .tflint.hcl on the lambda module: clean.
• git-secrets 1.3.0 with --register-aws, the custom patterns from scripts/setup-git-secrets.sh and .gitallowed, run in a fresh temp repo: clean. scripts/test-git-secrets-allowlist.sh: 42 PASS, 0 FAIL.
• No Go code changed, so go build, vet, golangci-lint and gocyclo don't apply.

Out of scope (noted, not changed)

• Fargate twin (terraform/modules/compute/aws/fargate/main.tf:108-109, :153-154): it writes "${arn}-*" with no guard, and its database_password_secret_arn has no validation. An empty value renders -*, which fails closed (it matches nothing) rather than open, so I left it alone. The same validation could be added there for symmetry if wanted.
• Lambda PutSecretValue statement: if admin_password_secret_arn is empty, it renders "Resource":[] (seen in the probe output above). That is an empty resource list, not a wildcard, but IAM may reject it as malformed. This predates the PR and isn't touched.

Closes #291

Maintainer: please mirror the issue's triage labels onto this PR: priority/p2, severity/medium, urgency/this-quarter, impact/internal, effort/xs, type/security, triaged.

🤖 Generated with Claude Code

[coderabbitai]

Note

Currently processing new changes in this PR. This may take a few minutes, please wait...

⚙️ Run configuration

• Configuration used: Repository: LeanerCloud/cloud-commitments-platform/.coderabbit.yaml
• Review profile: CHILL
• Plan: Advanced
• Run ID: 6d42d586-d9e8-4511-9832-ceef290ef68d

📥 Commits

Reviewing files that changed from the base of the PR and between 8caed39 and 6d951bf.

📒 Files selected for processing: 3

• terraform/modules/compute/aws/lambda/main.tf
• terraform/modules/compute/aws/lambda/tests/secrets_policy.tftest.hcl
• terraform/modules/compute/aws/lambda/variables.tf

+----------------------------------------------------------------------------------------------------------+
| Minimize coupling between modules. Avoid coupling by writing 'shy' code and applying the Law of Demeter. |
+----------------------------------------------------------------------------------------------------------+
  \
   \     /)/)
        ( '')
        /  |
      o(___)

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create a new PR

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

_{Comment @coderabbitai help to get the list of available commands.}

[cristim]

Independent review (claude-cc-platform reviewer, cold brief): APPROVE 6d951bffe0a969ab46f567a896e4b9c9425cca3f

No blocking findings.

Fail-closed check works. Extra expect_failures runs reject null, whitespace, a leading-space ARN, a trailing newline, a ? name and a ! name (6/6). The regex is anchored on both ends, and RE2 $ does not match before a trailing newline.

Callers are safe. The only caller is terraform/environments/aws/compute.tf:47, which passes module.database.password_secret_arn:

• When the secret is created in the same apply, the value is unknown at plan time and validation runs at apply.
• An external ARN with a non-standard name fails loudly at plan, which is the intended behaviour.
• The profiles README example ARN passes.

Runtime: no platform change is needed. internal/database/config.go:94 already refuses to start when both DB_PASSWORD and DB_PASSWORD_SECRET are empty. The IAM over-grant was the only fail-open path, and this PR closes it.

Tests: terraform test passes 15/15 on 1.10.5; fmt -check and validate are clean. Mutation probes:

• widening the name charset to allow * fails rejects_wildcard_secret_name;
• narrowing the partition group to aws fails accepts_govcloud_partition;
• reverting the != "" guard in main.tf:262 leaves everything green. That is expected: the guard is unreachable while the validation holds. Keep it as defence in depth.

Base drift: clean merge onto current main (tree 445fdfc1). Main changed only another hunk of lambda/variables.tf.

Nits:

• The null and whitespace rejections have no test; cheap to add.
• The Fargate twin has no validation. An empty ARN renders -*, which matches nothing, so it fails closed.
• The name charset rejects !, which an RDS-managed master secret (rds!db-...) would need if those are ever adopted.
• The PR lacks sec(iac/aws): lambda secrets policy renders Resource "*" when the DB secret ARN is empty #291's labels.

[cristim]

Independent review (claude-fleet): APPROVE 6d951bf

Findings (none blocking):

• terraform/modules/compute/aws/lambda/tests/secrets_policy.tftest.hcl:117-124 [non-blocking] - rejects_non_secretsmanager_arn does not pin the service segment.
  • Its input (arn:aws:ssm:...:parameter/...) is rejected only because it lacks :secret:. A probe that loosened secretsmanager to [a-z]+ in variables.tf:94 passed 8/8.
  • Failure scenario: a later regex edit drops the service check and nothing catches it.
  • Impact is small: a GetSecretValue grant on a non-Secrets-Manager ARN matches nothing, so it still fails closed.
  • Fix: use arn:aws:ssm:us-east-1:123456789012:secret:cudly-dev-db as the input.
• terraform/modules/compute/aws/lambda/main.tf:262 [non-blocking, no action] - The != "" guard cannot be reached while the validation at variables.tf:93-96 holds. Deletion probe M4 kept every test green, and terraform test cannot bypass a variable validation, so no test can cover the guard. It is acceptable defence in depth rather than dead code:
  • It matches the four sibling entries (main.tf:264-270).
  • It costs nothing.
  • The comment at main.tf:255-259 says why it exists.
  • Keep it.

Attacked:

• Callers: there is one module caller, terraform/environments/aws/compute.tf:29.
  • It passes module.database.password_secret_arn (compute.tf:47).
  • That value comes from module.secrets.database_password_secret_arn (database.tf:15, database/aws/main.tf:64), which is the always-created aws_secretsmanager_secret.database_password.arn (secrets/aws/outputs.tf:3).
  • So no DB-less, dev or bootstrap path passes "". The only tfvars mention, profiles/README.md:249, uses a valid ARN.
  • On a first apply the value is unknown at plan time, so validation runs at apply.
• Regex semantics (variables.tf:94):
  • null makes regex error, can() returns false, and the value is rejected.
  • Leading or trailing whitespace is rejected by the RE2 anchors.
  • The aws, aws-cn, aws-us-gov and aws-iso-b partitions all match.
  • name_prefix secret names match.
  • The error message is clear and names the risk.
• The new test file against the base export: rejects_empty_db_secret_arn FAILS, so the test detects the missing fix.
• Mutation probes, each restored and confirmed identical with cmp:
  • M1, validation dropped: rejects_empty_db_secret_arn FAILS.
  • M2, * allowed in the name: rejects_wildcard_secret_name FAILS.
  • M3, any service and a * account allowed: rejects_wildcard_account FAILS.
  • M3b, only the service segment loosened: survives (finding 1).
  • M4, guard dropped: survives (finding 2).
  • M5, the exact DB ARN entry dropped: both Resource-list assertions FAIL.
• Caveat: after the first failing expect_failures run, later runs are skipped, so each failure proves only the first run it broke.

Verified locally, with Terraform 1.10.5 (the version CI pins), a mock provider, and no cloud credentials. No plan or apply was run.

• terraform fmt -check -recursive: exit 0.
• terraform init -backend=false -input=false: OK.
• terraform validate: Success.
• terraform test: 15 passed, 0 failed.
• The new test file against the base export: 3 passed, 1 failed (rejects_empty_db_secret_arn), 4 skipped.
• CI runs this module's tests in the "Test AWS compute modules" step (.github/workflows/ci.yml:605-614).

Reviewed by an independent agent with no author context. Review only: nothing was pushed, and the GitHub review state is unchanged.

[cristim]

Local verification (macOS, exact head 6d951bffe0a969ab46f567a896e4b9c9425cca3f, export of terraform/): in terraform/modules/compute/aws/lambda, terraform fmt -check -recursive, init -backend=false, validate and test pass (15 passed, 0 failed). No plan or apply, and no credentials.

Gate status: two independent reviews approve this SHA (posted above), local verification passes, and CI is green. Merge waits on @IAmJSD marking the PR ready (see #974).

[cristim]

Local verification at 6d951bffe0a969ab46f567a896e4b9c9425cca3f merged onto current main (b6355a8e; merge-tree 074d396a; macOS):

• Terraform fmt -check, init -backend=false, validate and test pass for: terraform/modules/compute/aws/lambda
• Nothing was applied to any cloud.

[cristim]

Marked ready and merged by the owner's agent after 2 independent reviews and local verification (see the review and verification comments above, all at 6d951bffe0a969ab46f567a896e4b9c9425cca3f).