fix(scanner): remove compute-rule false positives/negatives (#268)

[parthrohit22]

Summary

Issue #268 claimed three compute rules relied on weak signals that produce false positives or
false negatives. Read the current code first (per the issue's own instruction to verify rather
than assume), confirmed all three claims across two review rounds, and fixed each.

Root cause per rule

• AZ-CMP-001 (scanner/rules/az_cmp_001.py): only checked nic.network_security_group.
  A VM whose NIC had no NSG of its own but whose subnet carried one -- a common, valid Azure
  pattern -- was flagged as a false-positive HIGH finding.
• AZ-CMP-003 (scanner/rules/az_cmp_003.py): only checked whether a recognised
  endpoint-protection extension name was present (KNOWN_EP_EXTENSIONS), with no health check.
  An installed-but-failed EP extension read as a silent compliant pass -- a false negative.
• AZ-CMP-004 (scanner/rules/az_cmp_004.py): relied only on config flags
  (patch_mode/enable_automatic_updates). A VM configured for automatic patching can still be
  months behind if the platform hasn't actually applied anything -- config alone doesn't prove
  patches landed, a false negative.

What changed

• AzureClient.get_subnet(subnet_id): resolves a NIC's subnet to check for a subnet-level NSG,
  cached for the client's lifetime.
• az_cmp_001.scan(): a VM is non-compliant only when it has a public IP and no NIC-level NSG
  and no subnet-level NSG. An unresolvable subnet is now treated as indeterminate (LOW,
  subnet_nsg_attached: None), never as confirmed unprotected -- an unreadable subnet must not
  produce a confirmed HIGH finding.
• AzureClient.get_security_assessments(): lists Microsoft Defender for Cloud security
  assessments for the subscription (new azure-mgmt-security==7.0.0 dependency), cached for the
  client's lifetime.
• az_cmp_003.scan(): Defender's "Endpoint protection" assessment is now the primary signal --
  Defender confirming Healthy/Unhealthy is real agent telemetry and overrides extension-name
  presence. Falls back to the extension check (now also validating provisioning_state, with a
  non-"Succeeded" state surfaced as indeterminate LOW rather than a silent pass) only when
  Defender's signal is unavailable. The per-VM lookup is now built from a single up-front index of
  the subscription-wide assessments list instead of rescanning it per VM, and when a resource has
  more than one "endpoint protection" assessment, an Unhealthy code always wins regardless of API
  response order.
• AzureClient.get_vm_patch_status(): fetches a VM's live patch assessment (Azure Update
  Manager / Microsoft.Maintenance) from its runtime instance view.
• az_cmp_004.scan(): a config-based compliant pass is now overridden into a confirmed finding
  when a conclusive real assessment (status succeeded/completedWithWarnings) shows critical or
  security patches still pending. Config with auto-patching disabled remains a finding
  regardless of assessment data, since that's unmanaged-drift risk on its own.
• Tests: extensive coverage added across tests/test_rules_compute.py,
  tests/test_azure_client_management.py, and tests/helpers/mock_azure.py for all of the above,
  including duplicate-extension handling, subnet caching, Defender health precedence, an
  order-independence regression test for multiple endpoint-protection assessments per resource,
  and an SDK-shape guard for the patch-assessment response.

Tested

• python -m pytest tests/test_rules_compute.py tests/test_azure_client_management.py -q --
  all passing
• ruff check / ruff format --check -- all checks passed
• GitHub Actions CI green, including CI Summary
• No changes to identity/enterprise rules or the frontend.

Fixes #268

[TFT444]

@parthrohit22 Good improvements, but this does not yet complete issue #268. Defender health and real patch-assessment evidence are still missing, and an inaccessible subnet can currently be treated as confirmed unprotected and produce a HIGH finding. Please either implement the remaining scope and return an unknown result for unreadable subnet evidence, or narrow the PR and linked issue to the work actually completed.

[Vishnu2707]

@TFT444 - I can see a new commit, please do review the PR again. @parthrohit22 - as a best practise it would be great if u could put a comment on what was addressed.

[TFT444]

@TFT444 - I can see a new commit, please do review the PR again. @parthrohit22 - as a best practise it would be great if u could put a comment on what was addressed.

@parthrohit22 has not pushed any updates since my review. It needs either the remaining scope implemented plus the subnet unknown result fix or the PR and linked issue narrowed to match what's actually done, plus conflict resolution, before this can merge.

What's blocking it:

Your review said the PR doesn't fully complete issue #268. Defender health and real patch-assessment evidence are still missing.
An inaccessible or unreadable subnet is currently treated as confirmed unprotected, producing a HIGH finding, when it should return an unknown result instead.
Needs to be rebased against dev to resolve the merge conflict.

@parthrohit22 please have a look into the conflict and the blocking i mentioned please

[parthrohit22]

Ready for review

PR #272 has been updated with the review follow-up:

• AZ-CMP-001 now treats missing or empty subnet IDs as indeterminate instead of confirmed “no NSG”; indeterminate findings use LOW severity and subnet_nsg_attached: None.
• AZ-CMP-003 preserves duplicate extension records and evaluates all recognized endpoint-protection extensions deterministically instead of overwriting by lowercased type.
• AzureClient.get_subnet() now caches subnet lookups, including failed resolutions, using the same pattern as disk caching.
• Added regression coverage for missing subnet IDs, indeterminate metadata, duplicate extensions, and subnet-cache behavior.
• Applied Ruff formatting required by CI.

Validation completed:

• 61 relevant compute/Azure-client tests pass.
• Ruff check and format check pass.
• GitHub Actions CI is fully green, including CI Summary.

Review commits: 5c5e63b, fc96b45.

[TFT444]

@parthrohit22 re-reviewed the current head. All three blockers from my last review are genuinely fixed: Defender health is now the primary signal in AZ-CMP-003, AZ-CMP-004 checks real patch-assessment evidence, and unresolvable subnets in AZ-CMP-001 surface as indeterminate LOW instead of confirmed HIGH. Test coverage is strong, including the SDK-shape guard.

One thing before merge: the PR description is stale. It still says az_cmp_004.py is unchanged (the diff modifies it substantially) and never mentions the Defender integration or the new azure-mgmt-security==7.0.0 dependency. Please rewrite it to match the head, since a new third-party dependency should not be invisible in the description.

Non-blocking follow-ups: _defender_endpoint_protection_status scans the full subscription assessment list per VM (consider indexing by resource ID once), and the loose "endpoint protection" in display_name match can hit multiple assessment types order-dependently.

Update the description and I'm happy to dissolve my block.

[parthrohit22]

@TFT444 Updated:

• Rewrote the PR description to match the current head -- it now covers the AZ-CMP-004 patch-assessment override, the Defender integration in AZ-CMP-003, and calls out the new azure-mgmt-security==7.0.0 dependency.
• Fixed both non-blocking follow-ups: _defender_endpoint_protection_status now looks up a resource-ID index built once per scan instead of rescanning the subscription-wide assessments list per VM, and when a resource has more than one "endpoint protection" assessment, an Unhealthy code always wins so the result no longer depends on API response order. Added a regression test (test_cmp_003_defender_unhealthy_wins_over_healthy_regardless_of_assessment_order) covering both orderings.

tests/test_rules_compute.py and tests/test_azure_client_management.py pass, ruff check/format are clean.

Latest commit: b9d0b6a.

[SHAURYAKSHARMA24]

Requesting changes.

Thanks for addressing the earlier review points. I re-reviewed the current head and found four remaining correctness issues:

1. AZ-CMP-001: an indeterminate public NIC causes an immediate break, so a later NIC with a confirmed public-IP/no-NSG exposure is never evaluated. A confirmed HIGH finding can therefore be downgraded to LOW.

2. AZ-CMP-003: Defender matching only accepts display names containing endpoint protection. This misses the current Defender recommendation, EDR solution should be installed on virtual machines, causing Healthy Defender evidence to be ignored and potentially producing false HIGH findings through extension fallback.

3. AZ-CMP-003: an extension with missing provisioning_state is treated as healthy and silently passes. Missing state is unknown evidence and should be surfaced as indeterminate rather than compliant.

4. AZ-CMP-004: unavailable, failed, incomplete, and stale patch assessments are treated as clean whenever automatic patching is configured. The rule needs an indeterminate result for unavailable/non-conclusive/stale evidence, and should use last_modified_time to enforce a freshness threshold.

Please add regression coverage for each scenario and request re-review after updating the PR.

[parthrohit22]

@SHAURYAKSHARMA24 All four addressed in c36673f:

1. AZ-CMP-001: the immediate break on the first flagged NIC could downgrade a confirmed HIGH on a later NIC to LOW. scan() now keeps the worst evaluated result across all of a VM's NICs, only stopping early once a confirmed violation is found (nothing can outrank it). Added tests for both NIC orderings (indeterminate-then-confirmed and confirmed-then-indeterminate).

2. AZ-CMP-003 EDR naming: confirmed against Microsoft's own docs - the recommendation was renamed from "Endpoint protection should be installed..." to "EDR solution should be installed on virtual machines" when Defender moved off the deprecated Log Analytics agent to agentless EDR scanning. The index now matches either display name, so Healthy Defender evidence on a current subscription is found instead of silently falling through to the extension check. Added a test using the real current name.

3. AZ-CMP-003 missing provisioning_state: no longer treated as confirmed healthy. It's folded into the same indeterminate path as Failed/Canceled - missing data proves nothing either way. Updated the existing pass-case fixtures that were relying on the old default (they now set provisioning_state="Succeeded" explicitly, since that's what they're actually testing) and added a dedicated test for the missing-state case.

4. AZ-CMP-004: unavailable, non-conclusive (InProgress/Failed/Unknown), and stale assessments no longer silently pass just because config is correct. The check now requires a conclusive status AND last_modified_time within a 30-day freshness window to count as real evidence; anything short of that surfaces as an indeterminate LOW finding, matching the LOW/indeterminate split AZ-CMP-001/003 already use. Added tests for unavailable, non-conclusive, stale, fresh, and missing-timestamp cases.

Regression coverage: tests/test_rules_compute.py is up to 42 tests (from 32), all passing. Full suite: 759 passed, 3 skipped (2 pre-existing unrelated test_observability.py failures in this sandbox from an unreachable postgres host). Ruff clean. Rebased onto latest dev - the current SCA (pip-audit) failure on this PR is dev still carrying chromadb==0.4.24 pending #317's merge, unrelated to this diff.

Please take another look when you get a chance.

[parthrohit22]

Rebased onto current dev (picking up everything merged since — #274, #277, #307, #308, #316, #317, #320, and more).

Two real files conflicted (tests/helpers/mock_azure.py, tests/test_rules_compute.py) — both were purely additive on both sides (dev added set_jit_policies/get_jit_network_access_policies and an AZ-CMP-007 test section; this branch's own AZ-CMP-004 patch-assessment tests), so I kept both. One thing git's line-based diff couldn't catch on its own: both sides independently defined a helper named _subnet_id in tests/test_rules_compute.py with different signatures (this branch's _subnet_id(vnet_name, subnet_name), 10 call sites; dev's newly-added _subnet_id(name), 1 call site) — no textual overlap, so no conflict marker, but Python resolves both to whichever definition comes last in the file, silently shadowing this branch's version for all 10 of its call sites. Caught by actually running the test suite after the merge (7 failures), not by trusting a clean git merge. Renamed dev's newer, single-use helper to _jit_subnet_id rather than touching the 10 existing call sites.

Verified: full backend suite (812 passed, 5 skipped — pre-existing/environment-only), ruff and format clean. All 20 CI checks are green on the current head.

@SHAURYAKSHARMA24 @TFT444 all four items from your reviews were addressed in c36673f (recapped in my comment on the 27th) and remain intact after this rebase — re-requesting review since it's been a few days since the current head was last looked at.

Dismissing to allow deeper review before any approval decision

[TFT444]

AZ-CMP-003: Silent false-negative on multi-extension VMs break on first succeeded extension skips all remaining extensions, leaving failed ones undetected and unconfirmed_names incomplete. A VM with IaaSAntimalware (succeeded) + MDE.Linux (failed) is stamped compliant; the failed extension never appears in finding metadata — directly contradicting the Defender 'unhealthy wins' contract already enforced in this same rule.

AZ-CMP-003: Overly broad "edr solution" substring match creates future false-negative surface. Any new Microsoft Defender recommendation containing 'edr solution' as a substring will be incorrectly associated with the endpoint-protection check; tightening to "edr solution should be installed" eliminates unintended future matches without breaking current behaviour.

Minor (no block): get_subnet cache key not lowercased (case-insensitive Azure IDs can miss cache), _indeterminate_finding closure in AZ-CMP-004 is a late-binding maintenance trap, _is_fresh uses age.days floor making the 30-day boundary off by up to 24 h, and get_subnet/get_vm_patch_status re-instantiate SDK clients per call instead of using the shared client pattern used elsewhere.

Dismissing — posted before user review

[TFT444]

AZ-CMP-003 silent false-negative: break on the first succeeded extension stamps the VM compliant without checking remaining extensions. A VM with IaaSAntimalware (succeeded) + MDE.Linux (failed) passes, directly contradicting the "unhealthy wins" Defender contract in the same rule. Fix: iterate all matched extensions before marking healthy.

AZ-CMP-003 overly broad substring: "edr solution" matches any future Defender recommendation containing those words, risking false-negative coverage gaps. Fix: tighten to "edr solution should be installed".