Repository navigation
fix: an exception with no Resources matches everywhere, not nowhere - #189
Conversation
getResourceExceptions iterated ruleException.Resources to find a match, so an exception with an empty Resources list was matched against zero designators and therefore matched nothing - even though kubescape's own hasExplicitControlException already documents and relies on "no resources = no scope constraint" for manual controls. The same shape of exception behaved oppositely depending only on whether the target control happened to be manual-review or resource-backed. Added an explicit "Resources is empty = matches everywhere" branch in getResourceExceptions. It reuses hasException's existing matching logic (cluster check, RegoResponseVector handling, ObjectSelector) via a new hasExceptionForAttributes helper - hasException split into the leading guard (which rejects a populated-but-all-empty-fielded PortalDesignator, a different footgun from an empty Resources list) and the rest, so the new branch can go straight to the rest with a digested all-empty PortalDesignator instead of hand-rolling a simplified version of RegoResponseVector/ObjectSelector handling that could drift from the real logic. An ObjectSelector, if present on a scope-less exception, still constrains which workloads it applies to - only the designator axis is left unconstrained. Verified with tests covering: the basic matches-everywhere case, ObjectSelector still gating a scope-less exception, PosturePolicies/control-name gating being unaffected, and the fix reachable through SetRuleResponsExceptions (not just GetResourceExceptions directly). This is a real, semver-relevant behavior change: any consumer of GetResourceExceptions or SetRuleResponsExceptions that (deliberately or not) relied on an empty-Resources exception matching nothing will now see it match everything the exception's ObjectSelector and PosturePolicies scope it to. Checked for other consumers via GitHub code search across the kubescape org and beyond (GetResourceExceptions, SetRuleResponsExceptions, exceptions.NewProcessor, resourcesresults.SetExceptions): kubescape/kubescape is the only repo, aside from opa-utils itself, that calls into this exception- matching path at all. kubevuln, operator, backend, regolibrary and synchronizer all depend on opa-utils for its data types but do not call these functions. Signed-off-by: Lalit Kishore <lr_be24@thapar.edu>
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 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 |
matthyx
left a comment
There was a problem hiding this comment.
Verified locally (checked out pr-189, go build ./..., go vet ./..., go test ./..., gofmt -l) — all clean, no regressions across the module.
Also confirmed the 4 new tests are real regression tests, not tautologies: reverted just exceptionprocessor.go to main and re-ran them — all 4 fail against pre-fix code (getResourceExceptions returns 0 matches for a scope-less exception) and pass with the fix applied.
The approach (split hasException into a guard + hasExceptionForAttributes, add an explicit len(Resources) == 0 branch that calls the split-out helper with a digested empty designator) is exactly what was discussed and agreed on in kubescape/kubescape#3368 — option 1, empty Resources = matches everywhere, with ObjectSelector still constraining scope-less exceptions. The guard-bypass rationale is correctly reasoned: the guard in hasException protects against a populated-but-all-empty-fielded PortalDesignator (a different footgun), not against Resources itself being an empty list.
Consumer check looks solid — only kubescape/kubescape calls into this matching path per the described search; other opa-utils consumers only use the data types.
LGTM, approving.
Summary
Fixes kubescape/kubescape#3368. Implements option 1 from the discussion there, per @matthyx's call: opa-utils treats an empty
Resourceslist as "matches everywhere."getResourceExceptionsfound a match by iteratingruleException.Resources:An exception with an empty
Resourceslist has zero iterations here, so it matched nothing — even though kubescape's ownhasExplicitControlException(core/pkg/opaprocessor/processorhandlerutils.go) already documents and relies on "no resources = no scope constraint" for manual controls:Same exception shape, opposite outcome, depending only on whether the target control happened to be manual-review (kubescape's own code) or resource-backed (this function).
Fix
hasExceptionis split into its leading guard (rejects a populatedPortalDesignatorwhose fields are all empty — a different footgun: a misconfigured, accidentally-wildcard designator) and a newhasExceptionForAttributes, which is the rest of the original logic (cluster check, RegoResponseVector handling, ObjectSelector,metadataHasException).getResourceExceptionsgets an explicitlen(ruleException.Resources) == 0branch that callshasExceptionForAttributesdirectly with a digested all-emptyPortalDesignator, instead of hand-rolling a simplified match that could drift from the real RegoResponseVector/ObjectSelector logic.ObjectSelector, if present on a scope-less exception, still constrains which workloads it applies to — only the designator axis (cluster/namespace/name/kind/labels/path) is left unconstrained. This is deliberate and matches @matthyx's point in the issue thread: the guard being bypassed protects against a different footgun than the one this fix addresses.Semver / consumer impact (per @matthyx's request in the issue thread)
This is a real, semver-relevant behavior change for
GetResourceExceptionsandSetRuleResponsExceptions: any caller that (deliberately or not) relied on an empty-Resourcesexception matching nothing will now see it match everything the exception'sObjectSelector/PosturePoliciesscope it to.Checked for other consumers before opening this:
kubescapeand unrestricted) for direct calls toGetResourceExceptions/SetRuleResponsExceptions: only hits insideopa-utilsitself (exceptionprocessor.go,resourcesresults/exceptions.go, and their tests).exceptions.NewProcessorandresourcesresults.SetExceptions(the higher-level entry points that reach this code): onlykubescape/kubescapeandopa-utilsitself.kubevuln,operator,backend,regolibrary, andsynchronizerall depend onopa-utils(per go.mod) but none of them call into this exception-matching path — they use it for its report/data types only.So
kubescape/kubescapeis the only known real consumer of this behavior change, and that's exactly the repo the issue was filed against.How this was verified
Added 4 tests to
exceptions/exceptionprocessor_test.go:TestGetResourceExceptions_EmptyResourcesMatchesEverywhere— a scope-less exception matches an arbitrary workload on an arbitrary cluster.TestGetResourceExceptions_EmptyResourcesWithObjectSelectorStillConstrains— a scope-less exception with anObjectSelectormatches a labeled workload but not an unlabeled one (confirms the fix is additive, not a blanket bypass).TestGetResourceExceptions_EmptyResourcesDifferentControlDoesNotMatch— the scope-less match is still gated byPosturePolicies/control-name through the normalListRuleExceptionspipeline.TestSetRuleResponsExceptions_EmptyResourcesMatchesEverywhere— the fix is reachable throughSetRuleResponsExceptions(the entry pointSetRuleExceptions/SetControlExceptions/SetFrameworkExceptionsactually use), not justGetResourceExceptionsdirectly.Confirmed all 4 are real regression tests by running them against the pre-fix code (
git stashon justexceptionprocessor.go):Real test output (this run, on this branch, with the fix applied)
That one finding is in
designators_cache_test.go, a file this PR does not touch — confirmed pre-existing onmainand unrelated (git diff --name-onlyshows onlyexceptionprocessor.goandexceptionprocessor_test.go).Test plan
go build ./...go vet ./...golangci-lint run— 0 issues in changed files (one pre-existing, unrelated finding elsewhere)gofmt -l— no unformatted filescc @matthyx