Fix/az net 003 az db 002 scanner correctness - #291
Conversation
TFT444
left a comment
There was a problem hiding this comment.
@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:
-
Same bug, next attribute: AZ-NET-003 now handles plural
source_address_prefixes, but the port check still reads onlydestination_port_range. A rule withdestination_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. -
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. -
Unrelated churn:
tests/test_rules_identity.pyhas ~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 newtest_idn_007_disabled_usertest; 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.
|
@safidnadaf ci still failing do a double cheke please |
9231bca to
fcc4b0c
Compare
parthrohit22
left a comment
There was a problem hiding this comment.
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:
- 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.
- 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.
- 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.
…-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>
da752e6 to
61a7b0a
Compare
Signed-off-by: safidnadaf <safidnadaf25@gmail.com>
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
Rule details (if applicable)
Testing
Regression tests
Identity rule test suite:
19 passed in 0.42sThe 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
Signed-off-bytrailer (git commit -s; seedocs/dco.md)feat/description