Skip to content

feat: add enterprise governance and tenant controls - #292

Open
TFT444 wants to merge 4 commits into
devfrom
feat/257-enterprise-governance-controls
Open

TFT444 wants to merge 4 commits into
devfrom
feat/257-enterprise-governance-controls

Conversation

@TFT444

@TFT444 TFT444 commented Aug 20, 2026

Copy link
Copy Markdown
Collaborator

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

  • New scan rule
  • Remediation playbook
  • Bug fix
  • Dashboard/front-end work
  • API endpoint
  • Documentation
  • Compliance mapping

Rule details

  • Rule IDs: AZ-GOV-001 through AZ-GOV-010
  • Severity: HIGH and MEDIUM
  • Category: Governance
  • Frameworks mapped: CIS, NIST CSF, ISO 27001, and SOC 2

The 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, while FAIL requires confirmed unsafe evidence.

Testing

  • Tested against a real Azure subscription
  • Returns correct JSON output in focused tests
  • All seven CI checks pass
  • No hardcoded credentials or secrets

Local validation:

  • 25 passed across governance foundation, governance rules, and CIS mapping tests
  • ruff check . passed
  • ruff format --check . passed for 301 files
  • All 105 rules, 105 reference rows, playbooks, and framework mappings cross-reference successfully
  • All governance playbooks passed bash -n
  • Full suite result: 747 passed, 3 skipped, 1 unrelated local Chroma dependency failure in tests/test_rag_dependencies.py

Live Azure validation was not performed. The PR should remain draft until CI and maintainer review confirm the implementation.

Related issue

Closes #257

Checklist

  • Every commit includes a DCO Signed-off-by trailer
  • My code follows the rule template in CONTRIBUTING.md
  • I added or updated the matching CLI playbook
  • I added or updated all four compliance framework mappings
  • I have not committed any real Azure credentials
  • My branch name follows the convention: feat/description

@github-actions

Copy link
Copy Markdown

Dependency Review

✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.

Scanned Files

None

@TFT444
TFT444 marked this pull request as ready for review August 20, 2026 14:07
@TFT444 TFT444 self-assigned this Aug 20, 2026

@ritiksah141 ritiksah141 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All good on my side.

Approving it.

@TFT444
TFT444 requested a review from m-khan-97 August 24, 2026 22:07
@TFT444

TFT444 commented Aug 24, 2026

Copy link
Copy Markdown
Collaborator Author

@m-khan-97 can you please review that pr. Thank you

@TFT444
TFT444 force-pushed the feat/257-enterprise-governance-controls branch from 2a1551d to 05378bd Compare August 27, 2026 22:19
@TFT444

TFT444 commented Aug 27, 2026

Copy link
Copy Markdown
Collaborator Author

@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 parthrohit22 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread scanner/rules/_governance_common.py Outdated
item
for item in assignments
if normal(properties(item).get("roleDefinitionId")).endswith(OWNER_ROLE_ID)
and _assignment_scope(item) == normal(subscription_scope)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@m-khan-97

Copy link
Copy Markdown
Collaborator

@TFT444, please address Parth’s substantive governance/API review findings first, then rebase onto current dev and remove the now-obsolete temporary ChromaDB audit exclusions introduced before #317 merged. Request rereview only after the corrected head passes the full suite.

@TFT444
TFT444 force-pushed the feat/257-enterprise-governance-controls branch from 05378bd to 14f9d83 Compare August 29, 2026 00:53
@TFT444

TFT444 commented Aug 29, 2026

Copy link
Copy Markdown
Collaborator Author

@parthrohit22 the one bug you confirmed is fixed: AZ-GOV-006 now counts all Owner assignments returned by atScope(), including those inherited from parent management groups, not just subscription-scoped ones. Added a regression test with an MG-scoped fixture to pin the behavior. Also removed the now-obsolete chromadb pip-audit exclusions (chromadb was removed in PR #317) and rebased onto current dev. All 22 governance tests pass.

@TFT444
TFT444 force-pushed the feat/257-enterprise-governance-controls branch from 14f9d83 to c645347 Compare August 31, 2026 23:15
@TFT444
TFT444 requested a review from parthrohit22 August 31, 2026 23:22
@TFT444

TFT444 commented Aug 31, 2026

Copy link
Copy Markdown
Collaborator Author

@m-khan-97 @ritiksah141 @parthrohit22 rebase and the requrent fixes are done. Have a Look Please.

thank you

ritiksah141
ritiksah141 previously approved these changes Sep 1, 2026
parthrohit22
parthrohit22 previously approved these changes Sep 1, 2026

@parthrohit22 parthrohit22 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirmed the AZ-GOV-006 fix — _governance_common.py now counts every Owner assignment atScope() returns instead of filtering to subscription-exact scope, which is the right call since that filter is exactly what was discarding management-group-inherited grants. Ran the new regression test myself: test_gov_006_owner_threshold and the parametrized test_missing_evidence_never_becomes_failure case both pass.

Approving.

@TFT444

TFT444 commented Sep 1, 2026

Copy link
Copy Markdown
Collaborator Author

Thank you for approving merging now as every requirement met.

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>
@TFT444
TFT444 dismissed stale reviews from parthrohit22 and ritiksah141 via ab79b6b September 1, 2026 15:22
@TFT444
TFT444 force-pushed the feat/257-enterprise-governance-controls branch from c645347 to ab79b6b Compare September 1, 2026 15:22
@TFT444

TFT444 commented Sep 7, 2026

Copy link
Copy Markdown
Collaborator Author

@parthrohit22 @m-khan-97 can you please double check

@m-khan-97 m-khan-97 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tanvir, I rechecked ab79b6b and ran the governance foundation/rule tests locally: 26 passed, including the inherited-Owner regression. That earlier fix is in place.

I found a separate credential-boundary issue in scanner/governance.py, so I cannot approve yet. Both _get_all() and _post_values() accept pagination URLs with next_link.startswith(ARM_ENDPOINT). That also accepts a different hostname beginning with the same text. I reproduced both paths using a fake session and fake token: the second request went to https://management.azure.com.attacker.invalid/next with the Authorization header attached. Both safety assertions failed. No real credentials or network requests were used; this demonstrates unsafe handling of a supplied pagination URL, not that Azure normally returns such a URL.

Please parse and validate the URL against the exact approved HTTPS origin before attaching credentials, including rejecting userinfo and unexpected ports. Apply the same boundary to both pagination paths, and do not silently return partial evidence as complete when a continuation is rejected. Add regression tests for valid ARM pagination, lookalike hosts, userinfo URLs and rejected continuations. Please also review redirect handling so it cannot silently invalidate that boundary.

Once that is fixed, I can re-run the focused tests and finish the review. The existing green tests do not exercise this case.

@parthrohit22 parthrohit22 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-reviewed ab79b6b at @TFT444's request.

The AZ-GOV-006 inherited-management-group Owner fix from my earlier review is in place (_governance_common.py counts every atScope() assignment, regression test with an MG-scoped fixture present).

I can confirm @m-khan-97's credential-boundary finding independently — it is real, not theoretical:

scanner/governance.py lines 126 and 168:

url = next_link if isinstance(next_link, str) and next_link.startswith(ARM_ENDPOINT) else ""

ARM_ENDPOINT = "https://management.azure.com" with no trailing /, so startswith accepts:

  • https://management.azure.com.attacker.example/next (suffix host)
  • https://management.azure.com@attacker.example/next (userinfo — the real host is attacker.example)
  • https://management.azure.com:8443/... (unexpected port)

and session.get/post then sends Authorization: Bearer <ARM token> (from _headers()) to that host. Same loose check on the first hop in _get_all (path.startswith("https://")), though there the input is always internally constructed.

Suggested fix: compare urlsplit(next_link) against the approved origin exactly — scheme == "https", hostname == "management.azure.com" (or the configured sovereign-cloud host), no username/password, and port in (None, 443) — before assigning url. Apply it to both pagination paths, and when a continuation URL is rejected, fail the call (return None) rather than returning the partial page as if it were complete. Worth confirming session has redirects disabled or same-origin-checked on these calls too, so a 3xx can't move the credentialed request off-origin after the check.

Not adding a separate block since m-khan's covers it; flagging that I verified it and agree it needs to land before merge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Implement enterprise governance and tenant control rules

4 participants