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".

[parthrohit22]

@TFT444 both fixed:

• AZ-CMP-003 no longer breaks out on the first Succeeded extension — all matched extensions are checked, grouped by name so duplicate records of the same extension still resolve on "any succeeded", but a genuinely different failed extension (e.g. MDE.Linux) alongside a healthy one now surfaces as indeterminate instead of being silently swallowed.
• Tightened the EDR marker to the full "edr solution should be installed" title instead of the bare substring.

Added regression tests for both. Re-requesting your review.

[SHAURYAKSHARMA24]

Most of the earlier blockers are fixed: the subnet/multi-NIC paths, Defender precedence and extension ordering all behave correctly, and the current head merges cleanly with dev with the focused and full backend suites passing.

One AZ-CMP-004 false-positive path remains. Freshness is only checked after a zero patch count, so a conclusive summary with pending patches becomes HIGH before last_modified_time is validated. I reproduced both a 45-day-old summary and one with no timestamp returning patch_assessment_override/HIGH; either can describe patches already remediated and should be indeterminate, not a confirmed current violation. Please require a usable, current timestamp before trusting nonzero counts too, and cover the stale/missing-timestamp pending cases (the freshness helper should also reject materially future timestamps).

Non-blocking integration cleanup: merging current dev leaves azure-mgmt-security==7.0.0 duplicated in requirements.txt.

[parthrohit22]

Rebased onto current dev (was 4 commits behind #279/#277/#320/#230). The internal dev merge was re-done; the only conflicts were additive (tests/helpers/mock_azure.py JIT vs patch-status helpers, both kept) and the now-redundant _subnet_id collision fix dropped since that landed upstream.

####@SHAURYAKSHARMA24 - AZ-CMP-004 freshness-ordering false positive (blocker)

Fixed in scanner/rules/az_cmp_004.py. The _is_fresh(last_modified_time) check now runs as a gate before the zero-vs-nonzero patch-count branch, not only on the zero path:

• Conclusive status + usable count → _is_fresh() → if not fresh (stale > 30d, missing, or unparseable timestamp), emit the indeterminate LOW finding (reason: "assessment_stale") and stop — on the pending-patch path as well as the clean path.
• Only a fresh, current timestamp lets the scan act on the count: nonzero → patch_assessment_override HIGH, zero → genuine clean pass.

So a 45-day-old summary or one with no last_modified_time reporting pending patches is now indeterminate, not a confirmed current violation.

_is_fresh() additionally rejects materially-future timestamps (more than a 5-minute clock-skew tolerance ahead of now) as bad data, instead of the previous behaviour where a negative age slipped through the <= 30 days check as "fresh".

New regression tests in tests/test_rules_compute.py:

test	scenario	expected
..._stale_pending_patch_assessment_is_indeterminate_not_high	nonzero count, timestamp > 30d old	indeterminate LOW, reason=assessment_stale, not patch_assessment_override
..._pending_patch_assessment_with_no_timestamp_is_indeterminate_not_high	nonzero count, last_modified_time=None	indeterminate LOW
..._fresh_pending_patch_assessment_still_returns_high	nonzero count, timestamp 2d old	HIGH patch_assessment_override (true-positive not regressed)
..._materially_future_assessment_timestamp_is_indeterminate	nonzero count, timestamp +3d	indeterminate LOW
..._is_fresh_tolerates_benign_clock_skew	zero count, timestamp +1min	clean pass (skew tolerance not too tight)

Non-blocking cleanup

• requirements.txt: removed the duplicate azure-mgmt-security==7.0.0 the dev merge introduced (build(deps) commit).

@TFT444 — AZ-CMP-003 (from the 07:35 review)

Both points were addressed in 62e25d3 before this rebase: the extension loop now iterates every matched extension before marking healthy (test_cmp_003_one_succeeded_and_one_failed_extension_is_indeterminate_not_a_pass), and the EDR marker is tightened to the full "edr solution should be installed" title (test_cmp_003_edr_solution_substring_alone_does_not_match_unrelated_recommendation).

Verification

• pytest tests/test_rules_compute.py tests/test_azure_client_management.py -q → 88 passed
• Full backend suite → 972 passed, 3 skipped (2 unrelated test_observability.py cases need the CI Postgres service)
• ruff check . / ruff format --check . → clean

@SHAURYAKSHARMA24 @TFT444 — re-review please.

[TFT444]

Re-review of current head (92af031) against SHAURYAKSHARMA24's active blocker (2026-09-02).

The one active blocker is resolved.

SHAURYAKSHARMA24 reported: "Freshness is only checked after a zero patch count, so a conclusive summary with pending patches becomes HIGH before last_modified_time is validated."

This does not match the code at the current head. The _is_fresh gate fires unconditionally after critical_count is extracted and before the critical_count > 0 branch:

if not _is_fresh(getattr(patch_summary, "last_modified_time", None)):
    findings.append(_indeterminate_finding("assessment_stale", patch_summary))
    continue

if critical_count > 0:
    findings.append(...)   # HIGH override

A 45-day-old summary with 3 pending patches hits assessment_stale and returns LOW. A summary with no last_modified_time also hits assessment_stale because _is_fresh(None) returns False immediately. Both scenarios have dedicated regression tests: test_cmp_004_config_ok_but_stale_pending_patch_assessment_is_indeterminate_not_high and test_cmp_004_config_ok_but_pending_patch_assessment_with_no_timestamp_is_indeterminate_not_high. Future timestamps rejected by _is_fresh are also addressed, with a tolerance test for benign clock skew.

The azure-mgmt-security==7.0.0 duplication SHAURYAKSHARMA24 flagged in requirements.txt is not present on the current head; exactly one entry exists.

TFT444's two prior blockers are also still resolved.

1. AZ-CMP-003 break on first succeeded extension (multi-extension false-negative): Fixed. The new code groups by extension name into states_by_name, then derives unconfirmed_names by checking whether "succeeded" appears in each name's state list. A VM with IaaSAntimalware (Succeeded) and MDE.Linux (Failed) correctly lands in unconfirmed_names. test_cmp_003_one_succeeded_and_one_failed_extension_is_indeterminate_not_a_pass covers both NIC orderings. The break is gone.

2. AZ-CMP-003 overly broad "edr solution" substring: Fixed. _ENDPOINT_PROTECTION_DISPLAY_NAME_MARKERS now contains the full string "edr solution should be installed". test_cmp_003_edr_solution_substring_alone_does_not_match_unrelated_recommendation verifies that "Review edr solution licensing costs" falls through to the extension check rather than being picked up as a Defender signal.

Non-blocking observations (no action required):

• _is_fresh uses timedelta.days which floors to integer days, so the effective freshness window is up to ~24 hours wider than the stated 30-day threshold. Noted in the prior round; acceptable given the threshold is already a conservative buffer.
• get_vm_patch_status re-instantiates the SDK client per call with no cache. Consistent with the pattern used elsewhere for infrequent non-list fetches; does not block merge.
• _indeterminate_finding is a closure inside scan in az_cmp_004, slightly inconsistent with codebase style. No correctness impact.

Approving.

[parthrohit22]

Pushed 8fad850 to get CI green.

Root cause: the dev rebase left a duplicated copy of the AZ-CMP-007 helper block + tests sitting before the AZ-CMP-004 assessment tests. That stale copy defined a local _subnet_id(name) which shadowed the module-level _subnet_id(vnet_name, subnet_name), and it carried two stray f[...] asserts.

Effect:

• Backend Tests: 7 AZ-CMP-001 tests failed with TypeError: _subnet_id() takes 1 positional argument but 2 were given.
• Lint (ruff): 11 errors — 9x F811 (redefined AZ-CMP-007 tests) + 2x F821 (undefined f).

Fix: removed the 206-line duplicated block. The canonical AZ-CMP-007 block at end of file is kept (it uses the renamed _jit_subnet_id helper and has no stray asserts). No production code touched.

Local: ruff check . clean, pytest tests/test_rules_compute.py → 56 passed.

[TFT444]

Re-reviewed the latest head, including the compute-rule false positive/negative fixes and the style commit.

The indeterminate/confirmed split across AZ-CMP-001, 003, and 004 is consistent and correct. The confirmed/indeterminate upgrade logic in AZ-CMP-001 (an indeterminate NIC finding can be replaced by a later confirmed HIGH, but an existing indeterminate cannot be overwritten by another indeterminate) is right. The _subnet_nsg_status three-value return avoids conflating 'unresolvable' with 'no NSG', which was the root of the false positive. AZ-CMP-003 correctly builds the Defender assessment index once per scan at O(n) and then does O(1) per-VM lookups, and the 'unhealthy wins across all matching assessments' logic handles iteration-order sensitivity. The _is_fresh helper in AZ-CMP-004 is conservative (missing timezone, future timestamps, and unparseable strings all return False) and the stale-assessment indeterminate result prevents a stale clean assessment from masking newly available patches.

The mock client updates and the set_subnet/get_security_assessments/get_vm_patch_status additions match the production interface. No issues found.

Approving.

[parthrohit22]

@SHAURYAKSHARMA24 please have a look, it looks clean and mergeable now.

[parthrohit22]

@SHAURYAKSHARMA24 rebased onto current dev (picks up #278; no conflicts — it doesn't touch the AZ-CMP-001/003/004 rules, their mock helpers, or tests/test_rules_compute.py). All 20 checks green on the new head.

The AZ-CMP-004 freshness-ordering blocker from your Sep 2 review has been fixed since Sep 2: _is_fresh(last_modified_time) now gates before the zero-vs-nonzero patch-count branch, so a stale or timestamp-less summary reporting pending patches returns an indeterminate LOW (reason: assessment_stale), never a confirmed HIGH patch_assessment_override. _is_fresh() also rejects materially-future timestamps. Regression tests: ..._stale_pending_patch_assessment_is_indeterminate_not_high, ..._pending_patch_assessment_with_no_timestamp_is_indeterminate_not_high, ..._fresh_pending_patch_assessment_still_returns_high, ..._materially_future_assessment_timestamp_is_indeterminate. The azure-mgmt-security duplicate in requirements.txt you also flagged is gone.

@TFT444 re-verified this exact path and approved on Sep 6. Re-requesting your review since it's the only outstanding change request.