feat: add enterprise governance and tenant controls - #292
Conversation
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
ritiksah141
left a comment
There was a problem hiding this comment.
All good on my side.
Approving it.
|
@m-khan-97 can you please review that pr. Thank you |
2a1551d to
05378bd
Compare
|
@ritiksah141 looks like your approval came through as a comment rather than a formal review approval. Could you re-submit it as an approval so the PR can move forward? No code changes since you reviewed it. |
parthrohit22
left a comment
There was a problem hiding this comment.
Read the collector and the shared evaluator in full before touching individual rules, since all 10 route through them. Genuinely solid work in both: GovernanceCollector handles all three pagination shapes correctly (ARM nextLink, Resource Graph $skipToken, OData @odata.nextLink), and it defensively checks that a nextLink actually starts with the real ARM endpoint before following it - a real guard against a malformed or redirected pagination link, not something I see everywhere. Every evidence set (hierarchy, policy_assignments, locks, etc.) is independently nullable so one API failure can't corrupt or silently zero out an unrelated evidence set, load_governance_policy() fails loud on any missing/malformed field instead of defaulting, and test_missing_evidence_never_becomes_failure proving the indeterminate-handling discipline across all 10 rules in one parametrized test is good test design. No RULE_ID collisions, all 10 playbooks exist and pass bash -n, all four compliance framework JSONs have complete AZ-GOV-001..010 mappings. Ran the full suite myself in a clean clone: 737 passed, 3 skipped, 0 failed, ruff clean, CI 20/20 green.
One real bug though, confirmed with a repro rather than just reasoning about it - left inline on the specific line.
AZ-GOV-006 (excessive subscription Owners) undercounts real Owner exposure. The collector queries roleAssignments?...&$filter=atScope(), and Azure's atScope() filter returns assignments made at the queried scope and any ancestor scope that applies down to it - a management group grant included, not just direct subscription-level grants. But the AZ-GOV-006 branch only counts an Owner assignment toward the total when it's scoped exactly at the subscription. I fed evaluate() two real Owner assignments - one at the subscription, one inherited from a parent management group - against a policy capping Owners at 1, and got zero findings back. Two effective Owners, threshold of one, nothing flagged. test_gov_006_owner_threshold only ever constructs subscription-scoped fixtures, so this gap is genuinely untested. For a rule whose whole purpose is bounding effective Owner exposure, silently missing the most common enterprise pattern (Owner granted at a parent management group) is a real false negative, not a style nit.
| item | ||
| for item in assignments | ||
| if normal(properties(item).get("roleDefinitionId")).endswith(OWNER_ROLE_ID) | ||
| and _assignment_scope(item) == normal(subscription_scope) |
There was a problem hiding this comment.
This only counts an Owner assignment when it's scoped exactly at the subscription (_assignment_scope(item) == normal(subscription_scope)). But the collector's roleAssignments query uses $filter=atScope(), which Azure documents as returning assignments at the given scope and any scope above it that applies down - so a management-group-level Owner grant is included in assignments here, just with a scope that will never equal subscription_scope.
Verified with a direct repro: fed evaluate() one subscription-scoped Owner assignment and one Owner assignment whose id is rooted at /providers/Microsoft.Management/managementGroups/corp/..., against a policy with maximum_subscription_owners=1. Result was [] - two real effective Owners, zero findings, when it should have flagged with owner_count: 2.
Suggest counting any assignment in assignments that resolves to Owner and applies to this subscription (i.e. don't filter on exact scope equality here) rather than only ones scoped directly at the subscription - _assignment_scope(item) is still useful to report where the grant originates in the finding metadata, just not as a filter that excludes inherited grants from the count.
Signed-off-by: Tanvir Farhad <tamimtarafder12@gmail.com>
Add tests for GOV-003 policy_definitions None branch, GOV-005 locks None branch, and extend the parametrize table to include GOV-003, GOV-007, and GOV-009 so every rule None-evidence guard is explicitly verified. Signed-off-by: Tanvir Farhad <tamimtarafder12@gmail.com>
No patched chromadb version exists. PR #317 removes chromadb from core requirements entirely; this ignore is a short-term unblock until that lands and the branch rebases. Signed-off-by: Tanvir Farhad <tamimtarafder12@gmail.com>
The atScope() ARM filter returns both direct subscription-level and inherited management-group Owner assignments. The previous filter `_assignment_scope(item) == subscription_scope` discarded all MG-inherited grants, making it possible to exceed the Owner threshold with zero findings. - Remove the subscription-scope filter so all effective Owner assignments are counted toward the threshold - Add a regression test using an MG-scoped fixture to pin this behaviour - Remove now-obsolete chromadb CVE-2026-45830/45833 pip-audit exclusions (chromadb was removed from requirements in PR #317) Signed-off-by: Tanvir Farhad <tamimtarafder12@gmail.com>
05378bd to
14f9d83
Compare
|
@parthrohit22 the one bug you confirmed is fixed: AZ-GOV-006 now counts all Owner assignments returned by |
What does this PR do?
Adds ten policy-driven enterprise governance rules covering management hierarchy, Azure Policy, exemptions, RBAC, resource locks, provider registration, ownership metadata, and unresolved configuration drift.
Type of change
Rule details
AZ-GOV-001throughAZ-GOV-010The collector uses read-only ARM and Resource Graph requests with pagination. Organisation-specific expectations are loaded from a strict policy file. Missing or inaccessible evidence is treated as
UNKNOWN, whileFAILrequires confirmed unsafe evidence.Testing
Local validation:
25 passedacross governance foundation, governance rules, and CIS mapping testsruff check .passedruff format --check .passed for 301 filesbash -ntests/test_rag_dependencies.pyLive Azure validation was not performed. The PR should remain draft until CI and maintainer review confirm the implementation.
Related issue
Closes #257
Checklist
Signed-off-bytrailer