Skip to content

Fix/az net 003 az db 002 scanner correctness - #291

Open
safidnadaf wants to merge 8 commits into
openshield-org:devfrom
safidnadaf:fix/az-net-003-az-db-002-scanner-correctness
Open

Fix/az net 003 az db 002 scanner correctness#291
safidnadaf wants to merge 8 commits into
openshield-org:devfrom
safidnadaf:fix/az-net-003-az-db-002-scanner-correctness

Conversation

@safidnadaf

@safidnadaf safidnadaf commented Aug 19, 2026

Copy link
Copy Markdown
Collaborator

What does this PR do?

Fixes scanner correctness issues in AZ-NET-003 and AZ-DB-002 and adds regression test coverage.

The changes improve handling of Azure SDK enum values, Azure resource IDs, SQL auditing policy states, and NSG source address prefixes. Additional regression tests were added for the affected database, network, and identity rules.

Type of change

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

Rule details (if applicable)

  • Rule ID: AZ-NET-003, AZ-DB-002
  • Severity: HIGH / MEDIUM
  • Category: Network / Database
  • Frameworks mapped: Existing mappings unchanged

Testing

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

Regression tests

Identity rule test suite:

19 passed in 0.42s

The tests cover AZ-IDN-001 through AZ-IDN-009, including both compliant and non-compliant scenarios.

Additional regression coverage was added for AZ-DB-002 and AZ-NET-003, including Azure SDK enum handling and edge cases.

Related issue

Closes #

Checklist

  • Every commit includes a DCO Signed-off-by trailer (git commit -s; see docs/dco.md)
  • 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

@TFT444 TFT444 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.

@safidnadaf the core fixes are good: enum_str() with real-SDK-model regression tests is exactly the right approach, and the parse_resource_id KeyError fix is solid. Three things before merge:

  1. Same bug, next attribute: AZ-NET-003 now handles plural source_address_prefixes, but the port check still reads only destination_port_range. A rule with destination_port_ranges=["443"] (plural, singular unset) is silently missed, which is the exact false-negative class this PR fixes. Please handle the plural port field the same way, with a test.

  2. AZ-DB-002 silent skip: when the policy lookup fails you now log and skip. The repo's convention for "cannot verify" (see AZ-CMP-001/002/003) is an indeterminate LOW finding with metadata.determination = "indeterminate" so the gap stays visible in scan output. Please align, or state why this rule should differ.

  3. Unrelated churn: tests/test_rules_identity.py has ~750 lines of pure reformatting with one real new test buried inside, and the file lost its trailing newline. Please revert the cosmetic reformat and keep only the new test_idn_007_disabled_user test; it doesn't belong in a scanner-correctness PR and wrecks blame for the whole file.

Fix these and this is good to go from my side.

@TFT444

TFT444 commented Aug 23, 2026

Copy link
Copy Markdown
Collaborator

@safidnadaf ci still failing do a double cheke please

@safidnadaf
safidnadaf force-pushed the fix/az-net-003-az-db-002-scanner-correctness branch from 9231bca to fcc4b0c Compare August 24, 2026 08:50

@parthrohit22 parthrohit22 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The actual AZ-NET-003 fix is correct and I traced it by hand: previously only the singular destination_port_range was checked against {"443","*"}, but Azure's SecurityRule model uses destination_port_range XOR destination_port_ranges - when a rule specifies multiple discontiguous ports, Azure populates the plural array and leaves the singular field empty, so a rule allowing 443 via destination_port_ranges: ["80","443"] was never flagged. The fix adds the plural check as an additional match condition, which only ever broadens detection (can't turn a prior true positive into a false negative) and uses exact string equality (can't introduce a false positive from this alone). Good, narrow, correctly-reasoned fix with a targeted regression test.

But as submitted this PR will silently delete regression coverage for two unrelated rules on merge. Verified independently, not just from the diff: dev currently has 44 test functions and 10 references to az_net_016/017 in tests/test_rules_network.py; this PR's head has 39 and 0. I ran the actual merge (git merge-tree upstream/dev HEAD) rather than reasoning about it in the abstract - it completes cleanly with zero conflicts (matching GitHub's own mergeable: MERGEABLE), and the resulting file still has 0 references to az_net_016/017. Root cause: az_net_016.py/017.py and their tests landed on dev after this branch point, and this PR reformats essentially the entire test file (line-wrapping unrelated to the actual fix, touching everything through test_net_015), which makes git's line-based merge treat the tail of the file as fully owned by this branch and silently drop dev's later addition instead of flagging a conflict. Post-merge, AZ-NET-016 and AZ-NET-017 (both MEDIUM severity) would have zero test coverage anywhere in the repo, with no CI signal that anything changed.

Separately, the PR title/description claims to fix AZ-DB-002 alongside AZ-NET-003, but git diff upstream/dev...HEAD -- scanner/rules/az_db_002.py is empty - confirmed no net change to that file, azure_client.py, tests/helpers/mock_azure.py, or tests/test_rules_database.py. History shows that AZ-DB-002 fix already landed on dev separately (commit 68c1908, PR #163). The individual commits on this branch still carry those changes, but they're no-ops against current dev, so the description should drop the AZ-DB-002 claim once this is rebased (or if the rebase surfaces a real remaining AZ-DB-002 delta, keep it and describe what's actually left).

Requesting changes for:

  1. Rebase onto current dev and re-apply only the actual AZ-NET-003 change/tests rather than reformatting the whole file, so AZ-NET-016/017 coverage survives the merge.
  2. Drop the ~600 lines of unrelated whitespace/line-wrap reformatting across test_rules_identity.py and the untouched parts of test_rules_network.py - none of it is required by this repo's ruff line-length config, and it's what's masking the merge-deletion risk above. The one genuine new test in there (test_idn_007_disabled_user) is fine on its own merits but belongs in its own PR, not mixed into an unrelated correctness fix.
  3. Update the PR description to match what's actually in the diff (drop the AZ-DB-002 claim, or fix it for real).

Non-blocking: test_net_003_detects_plural_destination_port_ranges only covers the noncompliant case for the new plural-port logic; a compliant-case counterpart (destination_port_ranges without 443/*, open source, asserting zero findings) would pin down that the fix doesn't over-match, which is exactly the failure mode worth guarding against when fixing a false negative.

Full suite: 706 passed, 3 skipped, 2 failed - the 2 failures are test_observability.py hitting an unreachable postgres host in this environment (gated by DATABASE_URL, unrelated to this diff, that file isn't touched here). Targeted -k "net_003 or db_002 or idn_007" run: 19/19 passed.

safidnadaf and others added 7 commits August 29, 2026 10:47
…-002

Addresses review feedback on PR openshield-org#163:

- Add a shared enum_str() helper in azure_client.py that safely unwraps
  Azure SDK enum fields via .value, since str(enum_member) yields e.g.
  'SecurityRuleDirection.INBOUND' rather than 'Inbound' and silently
  breaks naive string comparisons against real SDK objects.
- AZ-NET-003: normalise direction, access, and source_address_prefix
  through enum_str() so real SecurityRuleDirection/SecurityRuleAccess
  enum values are detected correctly, not just plain-string mocks.
- AZ-DB-002: normalise the auditing policy state through enum_str() so
  a real BlobAuditingPolicyState.ENABLED value is not mistaken for
  disabled (false positive) or vice versa.
- AZ-DB-002: malformed ARM IDs are now logged explicitly instead of
  silently skipped.
- AZ-NET-003: the matched plural source_address_prefixes entry is now
  included in finding metadata.
- Add regression tests using real azure-mgmt-network / azure-mgmt-sql
  SDK model classes (SecurityRule, SecurityRuleDirection,
  SecurityRuleAccess, ServerBlobAuditingPolicy, BlobAuditingPolicyState)
  rather than only SimpleNamespace/string-backed mocks, per
  SHAURYAKSHARMA24's review.
- Sync branch with upstream dev (v0.3.0) and apply current ruff format
  gate, per ritiksah141's review.
Signed-off-by: safidnadaf <safidnadaf25@gmail.com>
Signed-off-by: safidnadaf <safidnadaf25@gmail.com>
Signed-off-by: safidnadaf <safidnadaf25@gmail.com>
Signed-off-by: safidnadaf <safidnadaf25@gmail.com>
@safidnadaf
safidnadaf force-pushed the fix/az-net-003-az-db-002-scanner-correctness branch from da752e6 to 61a7b0a Compare August 30, 2026 13:58
Signed-off-by: safidnadaf <safidnadaf25@gmail.com>
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.

3 participants