fix(agt-policies): fail closed on non-object input.snapshot in migrated Rego - #3659
Conversation
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
PR Review Summary
Verdict: AI review comments are untrusted advisory output. The summary reports workflow-generated completion status only, not model-authored pass/fail claims. |
|
🔴 Contributor Check: HIGH
Automated check by AGT Contributor Check. |
github-actions
Bot
added
the
needs-review:HIGH
29ab7b7 to
4796af1
Compare
Imran Siddique (imran-siddique)
left a comment
Imran Siddique (imran-siddique)
left a comment
There was a problem hiding this comment.
Read the diff rather than the description, because the interesting part of this fix is not the guard itself but why the obvious version of it would not work.
The failure is real and it is the worst kind. Migrated Rego renders default verdict := {"decision": "allow"} and reaches every field through object.get(input.snapshot, ...). With input.snapshot absent, null, or a non-object, every accessor is undefined, no _match_i fires, and evaluation falls through to default-allow. A caller who omits or mistypes the snapshot root is therefore allowed by a deny-side policy, and nothing in the output says the policy was never evaluated. Silent allow on malformed input is exactly the direction a policy engine must not fail in.
Two things in here are why this is right rather than merely plausible.
The defaulted helper. default _snapshot_valid := false instead of an inline not is_object(input.snapshot). An inline negation over a builtin with an undefined operand is itself undefined, so the naive form would leak the absent-snapshot case straight back to default-allow: the guard would appear to be there and would not fire in the case it was written for. Your comment says this at the site, which is the right place for it.
Gating every rule branch on _snapshot_valid. Without it the guard and an always-matching branch, including the fail-closed deny for an unsupported operator, could both hold and OPA would raise a complete-rule conflict on verdict. Turning a silent allow into a hard evaluation error would be a different bug. test_invalid_rule_and_bad_snapshot_do_not_conflict covers precisely that interaction.
The tests evaluate the rendered policy rather than asserting on the rendered text, which is the difference between testing the fix and testing the string that describes it. test_valid_object_snapshot_without_match_still_allows is the one that matters for regression: the guard must not turn a legitimate no-match into a deny.
MohammadHaroonAbuomar this closes #3517, which you filed. It is green and has been sitting unreviewed for two days.
I do not have the approve bit here, so this is a comment rather than an approval, but I would merge it as it stands.
…ed Rego
The migration tool renders Rego with `default verdict := {"decision": "allow"}` and inlines each rule condition as chained `object.get(input.snapshot, ...)` accessors. When `input.snapshot` is absent, null, or a non-object, every accessor is undefined, no `_match_i` rule fires, and evaluation falls through to the default-allow verdict, so a caller that omits or mistypes the snapshot root is allowed on a deny-side policy (issue microsoft#3517).
Render a highest-precedence fail-closed guard: `default _snapshot_valid := false` plus `_snapshot_valid if { is_object(input.snapshot) }`, and a `verdict := deny if { not _snapshot_valid }` branch. A defaulted helper is required because an inline `not is_object(input.snapshot)` is itself undefined when the reference is undefined, so the absent-snapshot case would otherwise leak through to default-allow. Every rule branch, including the always-matching fail-closed deny rendered for an unsupported operator, is gated on `_snapshot_valid`, so a rule branch and the guard can never both hold and OPA never raises a complete-rule conflict on `verdict`.
Valid object snapshots are unaffected: a well-formed snapshot that matches no rule still falls through to default-allow, and a matching rule still applies. Add opa-backed tests covering absent, null, string, number, and array snapshots (all deny), a valid snapshot without a match (allow), a matching rule (deny), and an unsupported-operator rule combined with a bad snapshot (single deny, no conflict).
Reported-by: @MohammadHaroonAbuomar
Signed-off-by: AlgoVoi <chopmob@gmail.com>
The new regression test for the snapshot-guard fix shipped without the standard license header, which the changed-files header gate rejects. Add the canonical Microsoft MIT header to match every sibling test file. Signed-off-by: AlgoVoi <chopmob@gmail.com>
4796af1 to
f320c9a
Compare
2 participants
Fixes #3517.
The migration tool renders Rego with
default verdict := {"decision": "allow"}and inlines each rule condition as chainedobject.get(input.snapshot, ...)accessors. Wheninput.snapshotis absent, null, or a non-object, every accessor is undefined, no_match_irule fires, and evaluation falls through to the default-allow verdict, so a caller that omits or mistypes the snapshot root is allowed on a deny-side policy.Fix
Render a highest-precedence fail-closed guard in
_render_rego:default _snapshot_valid := falseplus_snapshot_valid if { is_object(input.snapshot) }verdict := {"decision": "deny", "reason": "runtime_error:snapshot_invalid", ...} if { not _snapshot_valid }branch_snapshot_validA defaulted helper is required because an inline
not is_object(input.snapshot)is itself undefined when the reference is undefined, so the absent-snapshot case would otherwise leak through to default-allow. Gating every rule branch (including the always-matching fail-closed deny rendered for an unsupported operator) on_snapshot_validkeeps the guard mutually exclusive with them, so a rule branch and the guard can never both hold and OPA never raises a complete-rule conflict onverdict.Valid object snapshots are unaffected: a well-formed snapshot that matches no rule still falls through to default-allow, and a matching rule still applies.
Testing
agt-policiessuite: 266 passed, 2 skipped (native ACS SDK built via maturin, plus opa, matching the policy-engine CI runtime).tests/test_migrate_snapshot_guard.pyevaluates the generated Rego through opa: absent, null, string, number, and array snapshots all deny; a valid object snapshot without a match allows; a matching rule denies; an unsupported-operator rule combined with a bad snapshot resolves to a single deny with no conflict; plus a render-level assertion.ruff check src/ --select E,F,W --ignore E501is clean.