proposal: multiple policies per VM#12
Conversation
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request proposes a design for supporting multiple policies per VM, allowing for separate authentication, authorization, and audit modules with configurable evaluation targets. Feedback highlights inconsistencies in the proposed function signatures and naming conventions, and suggests refining the evaluation order to ensure that short-circuiting on denial does not bypass audit logging.
| 1. Evaluator gains a fully qualified rule lookup: `evalModule(modules, | ||
| "authz.allow", input)` instead of just rule-by-name within a single | ||
| module. |
There was a problem hiding this comment.
There is a naming and signature inconsistency between this section and the Design sketch (line 72). This section uses evalModule(modules, "authz.allow", input), while the sketch uses evalModules(target_pkg, target_rule, input). It would be better to align the function name and the argument structure (including the modules container) across the document.
| - If decision is deny and `on_deny` is `send_local_response_403`, | ||
| short-circuit immediately. |
There was a problem hiding this comment.
The proposed short-circuiting logic for send_local_response_403 may interfere with the 'audit observes' pattern. If an enforcing target is placed before an audit target in the targets[] list, a denial will prevent the audit target from being evaluated. Consider specifying that all non-enforcing targets should be evaluated before any short-circuiting occurs, or clarifying the expected ordering in the documentation.
|
|
||
| A bare module (current shape, no `modules` wrapper) is wrapped into a | ||
| synthetic single-element list with `package: ""` and a synthetic | ||
| `targets[]` of `[{ package: "", rule: "allow", on_deny: send_403 }]`. |
There was a problem hiding this comment.
For consistency, use send_local_response_403 instead of send_403, matching the example provided in the Goals section (line 43).
ast.zig:
- Module gains optional package field (default "").
- New Modules struct wrapping a slice of Module.
- New buildModulesBundle handles {"type":"modules","modules":[...]}
plus the legacy single-module / bare-expression forms (wrapped as
a 1-element bundle with package="" for backwards compat).
eval.zig:
- New evaluateAddressed(arena, input, ast, target_pkg, target_rule)
parses the AST as a bundle and dispatches to package.rule.
- Existing evaluate() delegates to evaluateAddressed with
("", "allow"), so all existing call sites are untouched.
- New evalBundle OR-combines evalModule across every module whose
package matches; missing package -> deny by default.
Tests:
- src/eval.zig: 4 unit tests (cross-package addressing, missing
package, backwards compat, OR within a package).
- test/run.mjs and test/run_wasmtime.py: 4 cases each verifying
the wire format works through the wasm boundary.
Release build grows from 50K to 54K. ci.yml gains the test-unit job
in line with the other implementation branches.
kanywst
force-pushed
the
feat/multiple-policies
branch
from
0e18049 to
da3e557
Compare
1 participant
Design doc only. Tracks the multiple-policies item from ROADMAP medium term.
See
docs/proposals/multiple-policies.md.Summary:
modules[]+targets[]. Each module can carry an optionalpackage. Targets address rules aspackage.rule.evalModules(target_pkg, target_rule, input). Existing single-module configs keep working via a synthetic wrapper.on_denylets one decision block (403) while another logs only, supporting the "authz enforces, audit observes" pattern in one VM.Status: design only, no implementation.