fix(scanner): replace broken AZ-NET-012 NSG flow-log logic with VNet flow-log evidence - #316
Conversation
…flow-log evidence AZ-NET-012 called AzureClient.get_nsg_flow_logs(resource_group), which does not exist. Its bare except turned that AttributeError into a finding for every NSG in production, and the test suite mocked the invented method instead of catching the bug. The remediation script also only ever created NSG flow logs. Microsoft stopped new NSG flow log creation on 2025-06-30 and retires the feature entirely on 2027-09-30, so recommending new NSG flow log creation is itself broken guidance. - AzureClient.get_flow_logs() lists Network Watcher flow logs per-region (there is no flat subscription-wide endpoint), returning a dict keyed by normalized region. A region maps to its list of FlowLog resources, None when that region's watcher/flow-log listing failed, or is simply absent when no watcher exists there - callers must treat all three as indeterminate, never as "no flow log configured". A failure on one region's watcher does not discard results already collected from another region. - az_net_012.scan() now evaluates VNet-scoped flow logs at the correct Azure scope. An existing (never newly created) legacy NSG flow log on one of the VNet's own subnet NSGs is accepted as coverage rather than driving a migration finding, since NSG flow logs remain functional until 2027 and creating a new one is blocked. A region with no reachable Network Watcher evidence is skipped, not flagged - a missing watcher is AZ-NET-011's finding, not this rule's. - playbooks/cli/fix_az_net_012.sh now creates a VNet flow log instead of an NSG flow log, and is idempotent (checks for an already-enabled flow log first), preview-first (prints the exact command before requiring APPLY confirmation), and target-verified (re-reads state after creation, prints the rollback command on verification failure). - Framework mapping descriptions (all four compliance JSONs), docs/rules-reference.md, and website/content.js updated to describe the VNet flow-log evidence semantics instead of NSG-only. Tests: replaced the two tests that validated the rule against a mocked method the real client never implemented with nine tests covering no VNets, VNet flow log compliant/noncompliant, legacy NSG flow log compliant/noncompliant, an unwatched region, a failed region, and partial collection (one region fails without suppressing a real finding in another). Added three unit tests for AzureClient.get_flow_logs() directly covering the multi-region merge, total failure, and per-region partial failure. Signed-off-by: parthrohit22 <parthrohit60@gmail.com>
ritiksah141
left a comment
There was a problem hiding this comment.
Approving. Reviewed end to end and verified locally: the core fix is correct, the full test suite passes (724 passed, 2 skipped), ruff is clean, the playbook syntax is valid, all framework JSON files parse, and every CI check is green.
A few minor nits, none blocking:
docs/validation/SCANNER_VALIDATION.md(around lines 131-134) still notes that AZ-NET-012 references a nonexistentget_nsg_flow_logsmethod and that real results should be treated as "Pending investigation". This doc is based on the current rule contents, so that note is stale after this PR and can be removed or updated.compliance/assurance/network_layer.json(around line 883) still names the AZ-NET-012 rule classification "NSG flow logs disabled". Cosmetic inconsistency with the new "VNet Flow Logs Not Enabled" naming; tests validate IDs only, so it slipped through.- In
AzureClient.get_flow_logs(),result[location] = ...overwrites the previous entry if a subscription ever had two watchers in the same region. Theoretical, since Azure enforces one watcher per region, but merging the lists would be free insurance. - The playbook idempotency check only detects an existing flow log named
<vnet-name>-vnet-flowlog, not an arbitrary-named flow log that already covers the VNet. The derived name can also exceed flow-log name limits when the VNet name is long (up to 64 chars). Both are edge cases; theazerror would surface safely.
One note for the merge record: findings for this rule now report resource_type: Microsoft.Network/virtualNetworks instead of NSGs. Intentional and correct, but dashboards filtered on the old type will see the shift.
TFT444
left a comment
There was a problem hiding this comment.
Verified the fix end to end. A few observations, none blocking.
The core bug fix is correct. Calling a nonexistent get_nsg_flow_logs and swallowing the AttributeError in a bare except really did false-positive every NSG in production. The new get_flow_logs() collector exists on the real client, the per-region isolation logic (a failure in one region doesn't fabricate absence in another) is sound, and the VNet-level scope is the right target now that NSG flow log creation is blocked upstream.
Legacy NSG flow log fallback is the right call. Flagging an environment that already has NSG flow logs and telling it to migrate would recommend an action Microsoft currently blocks. Accepting existing coverage and only flagging genuinely uncovered VNets is correct.
Test coverage is thorough. The 9 rule tests and 3 client tests cover the meaningful edge cases: partial region failure, indeterminate vs confirmed absence, disabled flow logs not counting as coverage, and legacy NSG logs being accepted.
Two minor observations worth a follow-up (not blocking this PR):
-
get_flow_logs()constructs a newNetworkManagementClienton every call. Other methods in the class do the same, so it's consistent with the existing pattern, but a cachedself._network_clientwould reduce token churn on subscriptions with many regions. -
The
nsg_idsset comprehension can admit an empty string if a subnet's NSG object has noidattribute._covered_by_enabled_flow_logguards this correctly (if not target_id: return False), so it is not a bug, but addingif nsg_idto the comprehension filter would make the intent explicit.
CI fully green, DCO signed. Approved.
What does this PR do?
Replaces AZ-NET-012's broken NSG-flow-log check (a call to a nonexistent SDK method that silently false-positived every NSG) with correct VNet-flow-log evidence, since NSG flow log creation is itself being retired by Microsoft.
Type of change
Rule details (if applicable)
What was wrong
AZ-NET-012calledAzureClient.get_nsg_flow_logs(resource_group), which does not exist anywhere onAzureClient. The rule's bareexcept Exception: flow_log_enabled = Falseswallowed the resultingAttributeErroron every call, so every NSG in a real subscription was flagged, regardless of actual flow-log configuration. The existing test suite mocked the invented method rather than exercising the real client, so this false positive shipped undetected (documented indocs/validation/COMPREHENSIVE_VALIDATION_REPORT.md's H-2 finding).Separately, the remediation script only ever created NSG flow logs. Microsoft stopped new NSG flow log creation on 2025-06-30 (full retirement 2027-09-30) — the old remediation was already broken guidance independent of the detection bug.
What changed
scanner/azure_client.py: addedget_flow_logs(), which lists Network Watcher flow logs per-region (there's no flat subscription-wide endpoint — each region's Network Watcher must be queried individually) and returns a dict keyed by normalized region. A region maps to its list ofFlowLogresources,Nonewhen that specific region's watcher/flow-log listing failed, or is simply absent when no watcher exists there. A failure on one region never discards results already collected from another.scanner/rules/az_net_012.py: now evaluates VNet-scoped flow logs at the correct Azure scope. An existing (never newly created) legacy NSG flow log on one of the VNet's own subnet NSGs is accepted as coverage rather than driving a migration finding — NSG flow logs stay functional until 2027, and creating a new one is blocked, so flagging for "migration" would recommend an impossible action. A VNet in a region with no reachable Network Watcher evidence is skipped (indeterminate), never flagged — a missing watcher is AZ-NET-011's finding, not this rule's.playbooks/cli/fix_az_net_012.sh: now creates a VNet flow log instead of an NSG flow log, and is idempotent (checks for an already-enabled flow log first and exits early), preview-first (prints the exactazcommand and requires typedAPPLYconfirmation), and target-verified (re-reads the flow log's state after creation and prints the rollback command if verification fails).compliance/frameworks/*.json),docs/rules-reference.md, andwebsite/content.js: description/name updated to reflect VNet flow-log evidence instead of NSG-only.control_id/control_name(the frameworks' own official control text) left untouched.Testing
tests/test_rules_network.py— the two AZ-NET-012 tests that validated the rule against a mocked method the real client never implemented are replaced with 9 tests: no VNets, VNet-flow-log compliant/noncompliant (enabled and disabled), legacy-NSG-flow-log compliant/noncompliant (enabled and disabled), an unwatched region, a failed-collection region (403/429-style), and partial collection (one region fails without suppressing a real finding in a different, healthy region).tests/test_azure_client_management.py— 3 new unit tests forAzureClient.get_flow_logs()directly: multi-region merge, top-level failure (watcher enumeration itself fails), and per-region partial failure.Related issue
Closes #300
Checklist
Signed-off-bytrailer (git commit -s; seedocs/dco.md)