policy: refuse condition variable suffixes only when saving

[harshavardhana]

#276 put the suffix check in isValid, which also runs when a policy is
loaded, not just when it is saved:

• BucketPolicy.UnmarshalJSON and ParseBucketPolicyConfig run it, so a
  stored bucket policy with aws:SourceIp/x failed to load, and with it the
  bucket's metadata.
• Servers parse session policies with ParseConfig on every request, so a
  service account or STS credential whose session policy had such a key was
  denied everything.
• parseKey rejected a trailing slash (aws:username/) during decode, so
  such an IAM policy failed to load too.

What changes

• Statement.isValid and BPStatement.isValid no longer check suffixes.
  isValidStrict still does.
• New Policy.CheckVariables and BucketPolicy.CheckVariables for servers
  to call before saving, whatever the strictness setting.
• A trailing-slash key now parses, is marked as naming no variable, and
  marshals back with its slash, so a re-save or replication does not rewrite
  it. CheckVariables refuses it (ends in a slash that names no variable).
• Unchanged from policy: allow a /variable suffix only on keys that take one #276: a suffix the key does not take evaluates with no
  value, so the condition fails closed and a caller cannot satisfy it.

Tests

• The policy: allow a /variable suffix only on keys that take one #276 refusal tests now assert that the policy loads and that
  CheckVariables refuses it.
• New: a trailing slash round-trips through marshal; a stored bucket policy
  decodes, and its suffixed Allow is not satisfied by a caller-supplied value;
  aws:username/ fails closed like aws:username/x.
• Mutation-checked: making either CheckVariables a no-op, dropping the
  slash on marshal, or letting a trailing slash read a value each fail a test.

Servers need a follow-up to call CheckVariables on their save paths; the
AIStor change follows once this merges.

🤖 Generated with Claude Code

Summary by CodeRabbit

• Bug Fixes
  • Policies containing condition keys with variable suffixes can now be loaded without being rejected during general validation.
  • Unsupported variables are still rejected by variable checks before a policy is saved, and invalid stored conditions fail closed when evaluated.
  • Empty variable suffixes are preserved when policies are serialized and are rejected by variable checks.
  • Condition variables supported for S3 keys continue to pass validation.

[coderabbitai]

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Advanced

Run ID: f91d25a9-0775-4f34-9bc6-7075e377d5e3

📥 Commits

Reviewing files that changed from the base of the PR and between bd7a07f and fdf132a.

📒 Files selected for processing (8)

• policy/admin_policy_name_test.go
• policy/bucket-policy-statement.go
• policy/bucket-policy.go
• policy/condition/func.go
• policy/condition/key.go
• policy/condition_variable_test.go
• policy/policy.go
• policy/statement.go

💤 Files with no reviewable changes (2)

• policy/bucket-policy-statement.go
• policy/statement.go

Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.

---

📝 Walkthrough

Walkthrough

Condition keys with disallowed or empty variable suffixes can now be parsed and retained when policies load. Statement validation no longer performs variable checks. Policy.CheckVariables and BucketPolicy.CheckVariables provide explicit checks. Tests cover allowed suffixes and fail-closed evaluation of stored policies.

Changes

Condition Variable Validation

Layer / File(s)	Summary
Condition key parsing and variable rules policy/condition/key.go, policy/condition/func.go	Condition keys preserve trailing slashes with empty suffixes. Variable checks distinguish empty suffixes and reject keys that do not allow variables.
Explicit policy validation policy/statement.go, policy/bucket-policy-statement.go, policy/policy.go, policy/bucket-policy.go	Statement validation no longer checks condition variables. Policy.CheckVariables and BucketPolicy.CheckVariables check statement conditions and return the first error.
Load, save, and evaluation coverage policy/condition_variable_test.go, policy/admin_policy_name_test.go	Tests cover parsing and explicit validation for allowed, disallowed, and empty suffixes. Tests also check fail-closed evaluation of stored policies with matching variable conditions.

Priority: ⬇️ Low

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Bug fix

Merge Risk: ⚪ Minimal · up to fdf13

No confirmed issue blocks merging this library change. Servers must call the new variable checks before saving policies.

Security Architecture Review

Security architecture risk: 🟡 Moderate · up to fdf13

Stored policies should become readable again without letting invalid condition suffixes grant access. The remaining risk is that policy-saving services must adopt the new explicit check; this change does not establish that they do so.

Retained concerns

• Medium · security · inferred: Pre-save rejection now depends on callers explicitly invoking CheckVariables. A writer relying on ordinary parsing or validation can accept a policy with a disallowed suffix; whether deployed writers do so is not established.

Security review details

Security Blast Radius

• inferred — The changed contract applies to consumers of IAM and bucket-policy parsing and saving. Actual deployed writer reachability, tenant scope, and persistence exposure cannot be determined from this package.

Security Findings and Attack Paths

• inferred — An authorized policy writer using only ordinary parsing or validation could accept an invalid-suffix policy that the previous validation rejected. No deployed writer or unauthorized-access path is demonstrated, and invalid-suffix Allows fail closed during evaluation.

Trust Boundaries and Controls

• observed — Parsing no longer serves as the condition-suffix save control. Explicit CheckVariables calls provide that control, while VariablesAllowed guards the authorization evaluator independently.

Resilience and Maintainability Implications

• inferred — CheckVariables inspects policy conditions without recording a validated state. Whether the same policy snapshot is checked and saved across concurrent updates, interruption, or retry depends on persistence callers not established here.

Hardening Proposals

• proposed — Before deploying the load-behavior change to policy writers, verify every IAM and bucket-policy save path checks variables on the policy snapshot it persists, including update and retry paths.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and concisely describes the main change: condition variable suffixes are rejected when saving rather than when loading.
Docstring Coverage	✅ Passed	Docstring coverage is 90.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 10 functions across 6 files.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{A rabbit checked each policy key,
And found the empty slash to see.
“Load it first, then check before save,”
It told the carrots in its cave.
The tags passed; unsafe matches stayed shut,
Then bunny hopped through clover-cut.}

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