Repository navigation
fix: add matchAnyDomain policy condition function - #33385
zhang-arvin wants to merge 7 commits into
Conversation
❌ PR checklist incompleteThis PR cannot be merged until the following are addressed on its linked issue:
The fields live on the linked issue in the Shipping project (open the issue → right sidebar → Projects). After you set them, re-run this check (or push a commit) — issue/project changes do not re-trigger it automatically. Maintainers can bypass this check by adding the |
|
Hi there 👋 Thanks for your contribution! The OpenMetadata team will review the PR shortly! Once it has been labeled as Let us know if you need any help! |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| OMQueryBuilder domainQueryCombined; | ||
| if (domainQueries.size() == 1) { | ||
| domainQueryCombined = domainQueries.get(0); | ||
| } else { | ||
| domainQueryCombined = queryBuilderFactory.boolQuery().should(domainQueries); | ||
| } | ||
| collector.addMust(domainQueryCombined); |
There was a problem hiding this comment.
An empty matchAnyDomain() call reaches this branch with no domain queries. The resulting empty bool query is treated as match-all by Elasticsearch and OpenSearch, so an ALLOW policy can expose every searchable resource even though the direct RuleEvaluator returns false for the same condition. Reject zero arguments or emit a match-none query to keep search authorization fail-closed.
How this was verified: A zero-argument expression produces an empty bool query that is retained as the ALLOW must-clause with match-all semantics.
Knowledge Base Used: Platform security and governance
There was a problem hiding this comment.
Fixed in e661598. matchAnyDomain/matchAnyCertification now switch on argument count and call collector.setMatchNothing(true) for the zero-argument case (same fail-closed primitive hasAnyRole/inAnyTeam use), so an ALLOW rule with an empty condition translates to match_none rather than an empty (match-all) bool query.
|
Hi there 👋 Thanks for your contribution! The OpenMetadata team will review the PR shortly! Once it has been labeled as Let us know if you need any help! |
2c69e86 to
f027fb9
Compare
|
Addressed the greptile P1 "Empty Condition Grants Access".
AI agent generated. |
|
Hi there 👋 Thanks for your contribution! The OpenMetadata team will review the PR shortly! Once it has been labeled as Let us know if you need any help! |
![gitar-bot[bot]](https://avatars.githubusercontent.com/in/827041?s=60&v=4){kind=small}
…guments matchAnyDomain() with an empty argument list produced an empty bool query (should: []), which Elasticsearch and OpenSearch evaluate as match_all. An ALLOW rule carrying such a condition therefore granted access to every searchable resource, diverging from the direct RuleEvaluator which returns false for the same condition. Emit no clause and mark the rule match_nothing instead, so an empty condition degrades to deny-all. Applied to matchAnyCertification as well (same pattern, currently unreachable via the SpEL signature but identical bug class). Adds regression tests on Elastic and OpenSearch asserting the generated query is match_none and contains no domain clause and no empty bool clause.
f027fb9 to
e661598
Compare
|
Hi there 👋 Thanks for your contribution! The OpenMetadata team will review the PR shortly! Once it has been labeled as Let us know if you need any help! |
…tionEvaluator matchAnyDomain was inserted before the closing brace of matchAnyServiceType, so it became a nested method declaration. That is an illegal start of expression and broke compilation of openmetadata-service (CI: illegal start of expression at RBACConditionEvaluator.java:412). Close matchAnyServiceType first so matchAnyDomain is a proper class member.
|
Hi there 👋 Thanks for your contribution! The OpenMetadata team will review the PR shortly! Once it has been labeled as Let us know if you need any help! |
Code Review ✅ Approved 2 closed / 2 findings🔴 High risk · Adding matchAnyDomain to policy evaluation and search filtering changes authorization; incorrect domain matching could expose resources or deny legitimate access. Adds ✅ 2 closed✅ Bug:
|
| Compact |
|
Was this helpful? React with 👍 / 👎 | Powered by Gitar — free for open source
This branch has not been deployed
2 participants
Fixes #32970
Problem
The official docs ("Roles and Policies — Use Cases", Use Case 8) reference
matchAnyDomain('Finance')as a policy condition function, but:GET /policies/functionsdoes not return it, becausePolicyResource.listPolicyFunctions()only collects@Functionmethods fromRuleEvaluator, andmatchAnyDomainonly exists inAlertsRuleEvaluator(the notification/alert chain).matchAnyDomainare recognized byExpressionValidatorbut rejected at evaluation time becauseRuleEvaluatorhas no such function.Change
@Function(name = "matchAnyDomain")toRuleEvaluator: returns true when the accessed entity belongs to at least one of the given domain FQNs. During expression validation it resolves each referenced domain (likematchAnyTag) and returns false.RBACConditionEvaluator(case "matchAnyDomain"): builds an OR (should) ofdomains.fullyQualifiedNameterm queries so domain filtering applies on search/list paths.RuleEvaluatorTest(match/no-match/empty/null domains, null resource context, expression validation) and ES/OSRBACConditionEvaluatorTestsearch-query assertions.Notes
ExpressionValidatoralready listedmatchAnyDomainin its fallback whitelist.domains.fullyQualifiedName— the same fieldhasDomainpost-filtering relies on; domains inherited via parent entities may not appear on the index doc (same limitation as existing domain condition search translation).Greptile Summary
This PR adds
matchAnyDomainto direct policy evaluation and search-time RBAC filtering, with Elasticsearch, OpenSearch, and policy-evaluator tests.Confidence Score: 3/5
This PR is not safe to merge until an empty
matchAnyDomain()condition is prevented from granting unrestricted search visibility.The ordinary single- and multi-domain paths are aligned, but the reachable zero-argument path makes search authorization match all resources while direct authorization returns false.
Files Needing Attention: openmetadata-service/src/main/java/org/openmetadata/service/search/security/RBACConditionEvaluator.java
Security Review
The search evaluator turns an empty
matchAnyDomain()ALLOW condition into a match-all query, potentially exposing resources that direct policy evaluation would deny.Important Files Changed
matchAnyDomain, but zero arguments produce a fail-open match-all ALLOW query.matchAnyDomainevaluation and validation using resource domain FQNs.Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart LR P[Policy condition] --> V[Direct RuleEvaluator] P --> S[Search RBAC evaluator] V -->|Non-empty arguments| D[Compare resource domain FQNs] S -->|Non-empty arguments| Q[Domain term query] V -->|Empty arguments| F[False] S -->|Empty arguments| B[Empty bool query] B --> M[Match all search results]Reviews (1): Last reviewed commit: "style: apply google-java-format to match..." | Re-trigger Greptile
Context used: