feat(policy): add the arn:minio:memory ARN for Memory resources - #249
Conversation
Memory resources were authorized against the S3 ARN of the bucket backing the cortex, which forced the storage path into every policy and made a Memory grant indistinguishable from raw S3 access to the same bytes. They get their own namespace instead: arn:minio:memory:::<cortex>[/<collection>[/<name>]] The pattern is a logical path, never a storage key, so the Memory API's on-disk layout can change without invalidating a policy. ValidateMemory now requires the Memory type, so an S3 ARN can no longer carry a Memory action and a Memory ARN can no longer carry an S3 one. Malformed Memory ARNs are rejected rather than accepted as patterns that silently match nothing: a wrong colon count, a foreign partition or service, case variation, embedded whitespace, a backslash, any "." or ".." segment, and a prefix naming no resource at all. Also fixes a bug affecting every ARN type. A bare prefix such as "arn:aws:s3:::" parsed as ResourceARNAll keeping its own prefix as the pattern, so it validated cleanly and then matched nothing -- an Allow that granted zero and, the direction that fails open, a Deny that never fired. AWS rejects the form outright. It now parses as its own type with an empty pattern. S3, S3 Tables and KMS keep loading one so a policy already on disk still works, with ValidateStrict refusing it on the create and update paths; Memory carries no such tolerance and fails at parse. Memory resources had no test coverage at all; the suite passed because nothing exercised them. Adds parsing, validation, matching, cross-type isolation, whole-document validation and end-to-end evaluation, including that an agent-scoped grant denies a neighbour whose id shares a prefix.
|
Warning Review limit reached
Next review available in: 51 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughAdds AIStor Memory ARN support with dedicated parsing, path validation, wildcard matching, strict policy validation, authorization isolation, JSON round trips, and storage-independent serialization tests. ChangesMemory ARN policy support
Estimated code review effort: 4 (Complex) | ~45 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
policy/resource.go (1)
336-366: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftA bare prefix now carries a concrete type, which can break loading of existing non-S3 statements.
Before this change a bare prefix parsed as
ResourceARNAll.ResourceARNAllsatisfiesisS3(),isKMS(),isTable()and nowisMemory(), so a bare prefix loaded inside any statement type. After this change"arn:aws:s3:::"parses asResourceARNS3. A stored KMS or Table statement that carries a bare S3 prefix now failsValidateKMS/ValidateTablewith "type is not KMS" on the load path, not only on the strict path.
TestBareARNPolicyLoadsButCannotBeWrittenonly covers a bare S3 prefix inside an S3 statement, so this case is untested. If the mixed-type combination is judged impossible in practice, add a test that pins that decision. Otherwise keep bare resources loadable across type validators.🛡️ One option: let the type validators tolerate an inert bare resource
func (resourceSet ResourceSet) ValidateKMS() error { for resource := range resourceSet { - if !resource.isKMS() { + if !resource.IsBareARN() && !resource.isKMS() { return Errorf("resource '%v' type is not KMS", resource) }#!/bin/bash # Check whether any stored policy fixture mixes a bare ARN prefix into a non-S3 statement, # and how each type validator reacts to a typed bare resource. rg -n -C 4 'arn:aws:s3:::"|arn:minio:kms:::"|arn:aws:s3tables:::"' --type=go rg -n -C 8 'func \(resourceSet ResourceSet\) Validate(KMS|Table|S3|Memory)' --type=go🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@policy/resource.go` around lines 336 - 366, Preserve load compatibility for bare ARN resources across statement validators: update the relevant ValidateKMS, ValidateTable, ValidateS3, and ValidateMemory logic to tolerate an inert resource with an empty Pattern regardless of its parsed concrete Type, while retaining strict rejection for non-bare mismatched resource types and write-time validation. Add coverage for a bare S3 prefix embedded in non-S3 statements, or explicitly test and document the decision if mixed-type statements are intentionally unsupported.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@policy/memory-resource_test.go`:
- Line 160: Replace the British spellings in all three sites: change “behaviour”
to “behavior” in the inert-resource comment at
policy/memory-resource_test.go:160-160 and the t.Error message at
policy/memory-resource_test.go:249-249, and change “neighbour” to “neighbor” in
the doc comment at policy/memory-resource_test.go:561-561.
- Around line 388-414: Harden the exported Memory matcher, MatchResource, by
normalizing the resource path before wildcard matching so traversal segments
cannot match outside the intended subtree; document the behavior in its GoDoc if
callers must still provide cleaned paths. Update
TestMemoryMatchRequiresCleanResource to remove the skip and assert the
traversal-containing resource does not match, while preserving the existing
invalid-pattern assertion.
- Around line 544-556: Update the policy test loop around Policy validation to
execute each table entry through t.Run using tc.name, keeping unmarshalling,
validation, and assertions inside the subtest. Extend the testCases table with a
valid Memory NotResource case alongside the existing rejecting case, preserving
the current expected-error assertions.
---
Outside diff comments:
In `@policy/resource.go`:
- Around line 336-366: Preserve load compatibility for bare ARN resources across
statement validators: update the relevant ValidateKMS, ValidateTable,
ValidateS3, and ValidateMemory logic to tolerate an inert resource with an empty
Pattern regardless of its parsed concrete Type, while retaining strict rejection
for non-bare mismatched resource types and write-time validation. Add coverage
for a bare S3 prefix embedded in non-S3 statements, or explicitly test and
document the decision if mixed-type statements are intentionally unsupported.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 935b1c71-1c76-499b-8bd1-f31065351020
📒 Files selected for processing (4)
policy/memory-resource_test.gopolicy/resource.gopolicy/resourceset.gopolicy/statement.go
Keep a bare ARN prefix loadable inside a statement of another type. It used to parse as ResourceARNAll, which satisfies every type predicate, so a KMS or Table statement carrying one loaded; giving it a concrete type made those fail on the load path. The type validators now tolerate an inert bare resource. ValidateMemory does not: a Memory statement carrying one cannot already exist. Clean the candidate before matching a Memory resource, so a raw request path cannot escape the subtree its pattern names. Other ARN types match verbatim, as before. Fix three US-locale misspellings, and run the policy tables as subtests.
|
Pushed Bare prefix breaks cross-type loading (outside-diff, Major) — correct, and it defeated the point of the tolerance. Reproduced before fixing:
Traversal not enforced (Major) — agreed, and it is enforceable for Memory alone without touching S3 semantics. S3, Tables and KMS still match verbatim — that is pre-existing and changing it has a much wider blast radius, so it is documented on misspell — fixed all three. Also ran Subtests — both policy tables now use |
|
Correction. This PR's description and commit message state that AWS rejects Checked offline against three validators:
AWS's policy grammar defines What does hold, and what the corrected comment says: AWS documents no S3 resource type with an empty bucket name, and its wildcard-completion rule is scoped to ARNs with fewer than six fields — The fix is unaffected. It rests on MinIO's own behavior, reproduced in the description above: a bare prefix parsed as |
1 participant
What
Memory resources get their own ARN namespace:
They were authorized against the S3 ARN of the bucket backing the cortex, which
forced the storage path into every policy and made a Memory grant
indistinguishable from raw S3 access to the same bytes. The pattern is now a
logical path, so the Memory API's on-disk layout can change without
invalidating a policy.
ValidateMemoryrequires the Memory type, so an S3 ARN cannot carry a Memoryaction and a Memory ARN cannot carry an S3 one.
Bare ARN prefix fix (affects every ARN type)
arn:aws:s3:::parsed asResourceARNAllkeeping its own prefix as thepattern. It validated cleanly and then matched nothing — an
Allowthatgranted zero and, the direction that fails open, a
Denythat never fired:AWS rejects the form outright. It now parses as its own type with an empty
pattern. S3, S3 Tables and KMS keep loading one so a policy already on disk
still works, with
ValidateStrictrefusing it on create/update — the splitParseConfigStrictalready exists to provide. Memory carries no suchtolerance and fails at parse.
Tests
Memory resources had no coverage; the suite passed because nothing exercised
them. Adds parsing, validation, matching, cross-type isolation, whole-document
validation and end-to-end evaluation — including that an agent-scoped grant
denies a neighbour whose id shares a prefix, and that a bare-prefix policy
loads but cannot be written.
Both defences mutation-checked: dropping traversal/charset rejection fails 17
assertions; letting Memory inherit the bare-ARN tolerance fails 3.
Summary by CodeRabbit
New Features
Bug Fixes