WIP: Match risk rules against wildcard verbs and resources (#529) - #531
Draft
marcinc wants to merge 1 commit into
Draft
WIP: Match risk rules against wildcard verbs and resources (#529)#531marcinc wants to merge 1 commit into
marcinc wants to merge 1 commit into
Conversation
A risk rule's match rules named a resource and a verb, and the generated query selected a Rule node by equality on both, so a role rule granting the RBAC wildcard in their place was not matched even though it grants what the rule asks for. This is the mirror image of #80: that issue was about a match rule being too broad, this one about it being too narrow. Reported in issue #529. A ClusterRole granting `verbs: ['*']` on secrets was reported by risky-any-verb-secrets but not by risky-get-secrets or risky-list-secrets, costing precision. Worse, a ClusterRole granting `get, list, delete` on `resources: ['*']` within the core group was reported by nothing at all, since the fix for #80 scopes the `*` resource rules to the `*` API group. Two escalation paths were invisible entirely: a role granting every verb on rolebindings plus `bind` on roles satisfied neither risky-create-rolebinding-role nor risky-add-rolebinding, and a role granting every verb on pods/exec was missed by risky-exec-pods. Accept the wildcard alongside whatever a match rule names, the way the API groups already are. A resource and a verb stay one per selector, since their values are matched together and a role only matches when it grants all of them - only the wildcard joins each of them. Selector values are now uniformly either a single value the Rule node property must equal or a list of alternatives, so the builder splits them by shape rather than by name, and the `:api_groups` key is `:api_group` to match the property it tests. `nonResourceURLs` are left as written: RBAC matches a URL by prefix, so `*` is only one of the patterns that can cover it, and singling it out would be arbitrary. One grant is excluded: every verb on every resource of the `*` API group. That role can do anything, which is not a specific risk to report against a named resource and verb, and unrestricted-cluster-wide-subjects and unrestricted-ns-level-subjects already report it in those words for every subject it reaches. Without the exclusion a cluster-admin-equivalent role appears under every risky-role rule there is - 46 findings on the test cluster, per role - burying the findings a reader can act on under one they cannot. A role rule naming an API group is NOT excluded, `core` included. Those two templates additionally treat `core` as unrestricted, which is a fair reading of what a subject can reach through it, but it does not follow that every resource in the group goes unnamed: such a role grants nothing on apps, batch, RBAC or any CRD, so the core resources it does cover are worth naming. Excluding `core` as well was measured and rejected - it suppressed 12 accurate findings and left `core: */*` reporting once while `apps: */*` reported twelve times, an asymmetry justified only by which groups the subject templates happen to cover. Measured against a kind cluster carrying operator, team and CI RBAC plus the wildcard shapes from the issue: 30 findings before, 73 after, none lost. The risk rule queries cost 89ms in total there against 43ms before, both negligible beside ingest.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #529.
What is wrong
A risk rule's match rules name a resource and a verb. The generated query selected a Rule node by exact equality on both, so a role rule granting the RBAC wildcard in their place was not matched, even though it grants what the rule asks for.
This is the opposite of #80. That issue was about a match rule being too broad; this one is about it being too narrow.
On a test cluster:
verbs: ['*']on secrets was reported byrisky-any-verb-secrets, but not byrisky-get-secretsorrisky-list-secretsget, list, deleteonresources: ['*']in the core group was reported by nothing at all, because the fix for apiGroup is not being taken into account #80 scopes the*resource rules to the*API groupbindon roles satisfied neitherrisky-create-rolebinding-rolenorrisky-add-rolebinding, and a role granting every verb onpods/execwas missed byrisky-exec-podsWhat changed
The wildcard is now accepted alongside whatever a match rule names, in the same way the API groups already are.
A resource and a verb stay one per selector, because their values are matched together and a role only matches when it grants all of them. Only the wildcard joins each of them.
A selector value is now either a single value the Rule property must equal, or a list of alternatives. The builder tells them apart by shape rather than by name, and the
:api_groupskey becomes:api_groupso that it matches the property it tests.nonResourceURLsare left as written. RBAC matches a URL by prefix, so*is only one of the patterns that can cover a URL, and singling it out would be arbitrary.The grant left out
A role rule granting every verb on every resource of the
*API group is excluded.Such a role can do anything. That is not a specific risk to report against a named resource and verb, and
unrestricted-cluster-wide-subjectsandunrestricted-ns-level-subjectsalready report it in those words, for every subject it reaches. Without the exclusion a role of that shape turns up under every risky-role rule there is, 46 findings per role on the test cluster, which buries the findings a reader can act on.A role rule that names an API group is not excluded,
coreincluded. It grants nothing outside that group, so the resources it does cover are worth naming. Excludingcoreas well was measured and rejected: it hid 12 accurate findings, and leftcore: */*reporting once whileapps: */*reported 12 times. Only the choice of groups the subject templates happen to cover would justify that difference.Testing
Against a kind cluster carrying operator, team and CI RBAC, plus the wildcard shapes from the issue:
apps: */*still reports 12 findings;*/*/*is kept out of the risky-role rules but still reported for its subjectsRISK_RULE_QUERY_EXCLUDE_DEFAULT_ROLES=false,cluster-adminis left out andadminandeditreport as beforematch_rulesincustom-rules.yamlstill works, including a rule matching onnonResourceURLs286 examples pass. The new guards were checked by breaking the code on purpose: removing the exclusion fails 3 of them, dropping the verb widening fails 9, dropping the resource widening fails 10.
Worth knowing
The report gets longer. Most of that is real coverage, but a
core: */*role is now reported both for its subjects and under 13 risky-role titles.The risk rule queries take a bit longer.