Add enterprise network and perimeter security controls - #277
Conversation
|
Updated the branch with the latest upstream dev changes and resolved the compliance mapping and rules reference conflicts. Verification after installing the merged requirements:
The branch is now up to date with dev and all tests pass. |
There was a problem hiding this comment.
Thanks for the substantial work on the network and perimeter rules. I checked the collectors against the Azure behavior they are meant to represent. Two issues need to be corrected before we merge:
-
AZ-NET-021 is not testing effective DNS resolution.
custom_dns_configscontains the FQDN and private IP records Azure expects to be configured for the endpoint. It does not tell us what a workload in the VNet, or an on-premises resolver, actually resolves. As written, the rule can see a private endpoint IP even when the zone is unlinked, an A record is missing or stale, or forwarding is broken. Please add an explicit opt-in resolution probe from the relevant network context and compare the result with the endpoint IP. If that is outside the intended scope, rename and narrow the rule so it only claims to validate configuration evidence. Please include a regression case for broken effective DNS with a valid privatecustomDnsConfig. -
AZ-NET-025 requires a log category that WAF_v2 cannot provide.
get_waf_diagnostic_logging()always requiresApplicationGatewayPerformanceLog, but Microsoft supports that category only on Application Gateway v1. For v2, performance data comes from metrics. This would flag a correctly configured WAF_v2 gateway as noncompliant forever. Please make the required categories SKU-aware and add coverage for both v1 and v2.
GitHub CI is green. My focused local test run could not collect because this checkout is missing azure.mgmt.authorization; that is an environment limitation, not the reason for this review. These two findings come from comparing the production logic with the Azure contracts.
Please tag me when the changes are ready and I will review them again.
|
@m-khan-97, addressed both requested changes in
Verification after merging the current
The branch is current with |
TFT444
left a comment
There was a problem hiding this comment.
Reviewed the fixes for AZ-NET-021 and AZ-NET-022. Scoping 021 to ARM configuration evidence only is the right call, and the dedicated test confirms it makes no resolver-path claims. AZ-NET-022 correctly treats collection failures and unknown states as indeterminate. Approving once CI passes.
parthrohit22
left a comment
There was a problem hiding this comment.
Deep-reviewed AZ-NET-018 through AZ-NET-027 plus the new azure_client.py collectors. Most of this is solid - the indeterminate-vs-compliant-vs-noncompliant discipline across 018-022 is exactly right and consistent with the codebase's established convention (az_net_016.py / get_all_azure_firewalls: API failure or missing evidence skips rather than flags), per-service failure isolation in get_critical_paas_inventory is a genuine robustness improvement, and evidence metadata via the shared _perimeter_common/_private_link_common helpers is consistently attached. Test coverage for 018-022 and 024-026 targets empty-inventory/API-failure/malformed-evidence paths, not just the happy path. RULE_IDs don't collide with anything existing, playbooks pass bash -n, framework mappings are complete across all four compliance JSONs.
Two real bugs though, one of them severe enough to block on its own. Left inline comments on both.
Also worth a look before merge, not blocking: playbooks/cli/fix_az_net_018.sh, _020.sh, _022.sh, _026.sh, _027.sh are echo-only guidance with no executable remediation, unlike the established convention elsewhere (fix_az_cmp_001.sh, fix_az_net_016.sh) which run a confirmed az CLI command. If that's deliberate for higher-risk changes it's a reasonable call, but it's worth stating explicitly somewhere (PR description or a code comment) so it doesn't read as an oversight - especially now that the one playbook in this batch that does execute a command (023) turned out to have an invalid one.
Full suite: 755 passed, 3 skipped, 2 failed - the 2 failures are test_observability.py hitting an unreachable postgres host in this environment, unrelated to this diff (confirmed that file isn't touched here).
| ) | ||
| continue | ||
| normalized = str(getattr(mode, "value", mode)).replace("_", "").lower() | ||
| if normalized == "alertanddeny": |
There was a problem hiding this comment.
This rule can never pass against a real subscription. I checked the actual Azure SDK enum (azure.mgmt.network.models.AzureFirewallThreatIntelMode) and it has exactly three values: Alert, Deny, Off. There is no AlertAndDeny mode anywhere in the Azure Firewall API. I ran this scan() against all three real enum values directly and every one is flagged HIGH - including Deny, which is the strictest, most secure setting a firewall can actually have. There is no configuration that satisfies this check today.
The test (test_net_023_requires_alert_and_deny) doesn't catch this because it feeds in a fabricated threat_intel_mode="AlertAndDeny" as the compliant case instead of testing the real Deny value - so the test and the rule share the same wrong assumption rather than one catching the other.
The playbook compounds this: fix_az_net_023.sh runs az network firewall update ... --threat-intel-mode AlertAndDeny, which isn't a valid value for that CLI flag either, so running the suggested remediation against a real subscription fails outright.
Suggest: compliant should be mode == "Deny" (optionally treat Alert as a lesser MEDIUM rather than folding both into one broken HIGH check), and the playbook needs --threat-intel-mode Deny.
There was a problem hiding this comment.
Fixed in 70f4be6. AZ-NET-023 now treats the real Azure Firewall Deny enum value as compliant, flags Alert and Off, reports Deny as the expected value, and uses --threat-intel-mode Deny in the remediation playbook. Regression coverage now uses the actual SDK enum values.
|
@parthrohit22, both blocking findings are addressed in
Verification after merging current
Could you please take another look? |
parthrohit22
left a comment
There was a problem hiding this comment.
Both blockers from my last review are fixed in 70f4be6, confirmed against the actual diff rather than taking the reply on faith.
AZ-NET-023: re-checked the real Azure SDK enum (AzureFirewallThreatIntelMode) - it now correctly checks normalized == "deny" instead of the fabricated "alertanddeny", which never existed. The playbook now runs --threat-intel-mode Deny, a valid CLI value, instead of the invalid AlertAndDeny it had before. The old test that fed in the fabricated value is gone; test_net_023_flags_real_non_deny_modes is parametrized on the two real non-compliant values (Alert, Off) and test_net_023_accepts_real_deny_mode_and_preserves_unknown covers the real compliant case plus missing/unavailable evidence.
AZ-NET-027: now gates on public_gateway(gateway) and waf_enabled(gateway), matching 024-026. test_net_027_ignores_public_gateway_without_waf covers exactly the case I flagged as untested - a public gateway with no WAF policy at all - and asserts zero findings instead of the old misleading "missing rate-limit rule in a nonexistent policy" result.
Pulled the PR head into a clean clone and ran it myself rather than trusting the reported numbers: tests/test_enterprise_perimeter_rules.py - 24/24 pass, including both new regression tests.
Approving.
parthrohit22
left a comment
There was a problem hiding this comment.
Both blockers from my last review are fixed in 70f4be6, confirmed against the actual diff rather than taking the reply on faith.
AZ-NET-023: re-checked the real Azure SDK enum (AzureFirewallThreatIntelMode) - it now correctly checks normalized == "deny" instead of the fabricated "alertanddeny", which never existed. The playbook now runs --threat-intel-mode Deny, a valid CLI value, instead of the invalid AlertAndDeny it had before. The old test that fed in the fabricated value is gone; test_net_023_flags_real_non_deny_modes is parametrized on the two real non-compliant values (Alert, Off) and test_net_023_accepts_real_deny_mode_and_preserves_unknown covers the real compliant case plus missing/unavailable evidence.
AZ-NET-027: now gates on public_gateway(gateway) and waf_enabled(gateway), matching 024-026. test_net_027_ignores_public_gateway_without_waf covers exactly the case I flagged as untested - a public gateway with no WAF policy at all - and asserts zero findings instead of the old misleading "missing rate-limit rule in a nonexistent policy" result.
Pulled the PR head into a clean clone and ran it myself rather than trusting the reported numbers: tests/test_enterprise_perimeter_rules.py - 24/24 pass, including both new regression tests.
Approving. nice work @ritiksah141
|
@TFT444 Have a look , re-requesting review from your side, I flagged it green , if you have same opinion, it's mergeable now. |
|
@m-khan-97 , could be please review it as I have addressed you request. |
m-khan-97
left a comment
There was a problem hiding this comment.
Thanks, Ritik. I verified the latest commit directly. The four previously reported fixes are correctly implemented, the focused suite passes 47/47 locally, and the authoritative CI run on 040284f is green. The cancelled jobs shown by GitHub belong to a duplicate run and are not a source failure.\n\nI found two remaining evidence-state gaps before I can approve:\n\n1. AZ-NET-023 treats every non-Deny value as a confirmed HIGH. Alert and Off are positive noncompliant evidence, but an unrecognized/future SDK value is not. Please flag only the known unsafe enum values and preserve any unexpected value as UNKNOWN. Add a regression case such as FutureMode that produces no finding.\n\n2. AZ-NET-027 uses getattr(rule, "state", "Enabled"), so a malformed or partially collected RateLimitRule with no state is silently assumed enabled and can make the gateway appear compliant. Please require explicit Enabled evidence. If a rate-limit rule exists but its state is unavailable and there is no explicitly enabled rule, preserve the result as UNKNOWN rather than inferring either compliance or a confirmed violation. Add missing-state and unknown-state regression cases.\n\nThese are small, shared trust-contract corrections rather than a redesign. Once they are covered and CI is green, tag me and I will re-review promptly.
|
@m-khan-97, addressed both evidence-state gaps in
Verification on the current
Could you please review again as CI is green as well? |
|
@m-khan-97, i have addressed the changes requested in the last review so could you please re-review it and see if its fine now cause with out your approval it cant not be merged. |
4de1df8 to
d8762bc
Compare
|
@m-khan-97 @parthrohit22 @TFT444, coordination update for #277:
The only failing check is I have not suppressed these new HIGH and CRITICAL advisories inside #277 because that is a project-wide AI dependency decision and overlaps the existing Docker/AI dependency blocker. The #277 rebase and PR-specific validation are complete. |
TFT444
left a comment
There was a problem hiding this comment.
Checked m-khan-97's two remaining items against the current HEAD.
AZ-NET-023: Already guarded. After normalizing the enum, the code does if normalized not in {"alert", "off"}: ... continue, so any unrecognized or future value logs a warning and skips without creating a finding. test_net_023_accepts_real_deny_mode_and_preserves_unknown feeds in "FutureMode" and asserts == [] — the regression case m-khan-97 asked for is present.
AZ-NET-027: No silent Enabled default. The code uses getattr(rule, "state", None), filters out None before normalizing, then checks len(normalized_states) != len(states) to detect missing states and route them to UNKNOWN. test_net_027_preserves_unknown_rate_limit_state is parametrized on both None and "FutureState" and asserts zero findings in each case.
Both fixes were in the code when m-khan-97 reviewed 040284f. CI is green, DCO signed, parthrohit22 approved. Approved.
|
@ritiksah141 ci failing have a look please after its good to go |
d8762bc to
566e05a
Compare
|
@m-khan-97 @Vishnu2707, PR #277 has now been rebased onto the latest Verification completed on the rebased head
The requested evidence-state fixes remain present after the rebase. Please take another look when available. This should now be ready for final approval and merge. |
|
@m-khan-97, @TFT444 and @parthrohit22, could you please re approve it as all has been addressed and then re ran the CI and I used --force-with-lease, not a plain force push. It verifies that the remote branch has not changed unexpectedly since it was fetched, which helps avoid overwriting someone else’s newer work. So, nothing concering with the force push if you guys feel something up please lemme know |
|
@m-khan-97 Wanted to close the loop on your last review before pinging you again for a re-look. Pulled the current head (
Re-ran the specific regression suite myself ( Both blockers you raised look correctly closed out from where I'm sitting; if you get a chance to take another look and either re-approve or let us know if something's still off, that'd get this moving toward merge. Formally re-requesting your review as well so it shows up properly in your queue. |
|
@ritiksah141, thanks — the requested evidence-state fixes are preserved, Parth has independently verified them, and the current checks are green. Since #308 merged after this head was prepared, the branch is now one commit behind |
Signed-off-by: ritiksah141 <ritiksah141@gmail.com>
Signed-off-by: ritiksah141 <ritiksah141@gmail.com>
Signed-off-by: ritiksah141 <ritiksah141@gmail.com>
Signed-off-by: ritiksah141 <ritiksah141@gmail.com>
566e05a to
6f127d4
Compare
|
@m-khan-97, the final rebase is complete. PR #277 is now based directly on current The requested evidence-state fixes are preserved. All four PR commits retain Verification after the rebase:
Please complete the final review when available. |
m-khan-97
left a comment
There was a problem hiding this comment.
Thanks, Ritik. I verified the final rebased head against current dev. Both remaining evidence-state fixes are preserved exactly as requested: AZ-NET-023 only treats the known unsafe Alert/Off values as confirmed findings, and AZ-NET-027 no longer assumes a missing or future rule state is enabled. The regression coverage remains in place, the branch is based on the merged severity contract, DCO is valid, and all 20 required checks are green. Approving from my side.
|
@ritiksah141, a timing note: I completed the promised approval after verifying |
…Z-SECOPS-010 The dev merge into this branch pulled in openshield-org#277 (enterprise network and perimeter controls) and openshield-org#320, both merged since this branch was last updated. openshield-org#277's ten new AZ-NET-018..027 rules and the previously-merged AZ-SECOPS-010 carry compliance mappings across all four framework files with no mapping_type/evidence_type/primary_source/rationale/ review_status - this PR's own validate_mapping_pack.py correctly rejects that, since neither rule existed yet when this PR's evidence- schema requirement was written. Same root cause as the AZ-CMP-007 gap from openshield-org#307 fixed earlier in this branch's history. Filled in following the exact conventions of each entry's nearest sibling in the same file: - cis_azure_benchmark.json: mapping_type "not_applicable" for the N/A-* control_ids (all ten AZ-NET-0xx entries), "direct" for AZ-SECOPS-010's real numbered control (2.1.20) - matching this PR's existing framework-level-default convention for non-N/A CIS entries. - nist_csf.json / iso27001.json / soc2.json: mapping_type "supporting", evidence_type "automated_configuration_scan", with primary_source/ rationale text following the exact template of each file's sibling entries (e.g. AZ-NET-016, AZ-SECOPS-009). - owner: null, review_status: "pending_review", review_date: null, matching every other unreviewed entry in these files. Also cleans up a merge artifact in CHANGELOG.md: openshield-org#277 had added a second, malformed top-level "## Unreleased" section (missing brackets, no ### subsections) above the file's existing well-formed "## [Unreleased]" - folded its one entry into the correct section. Verified: mapping-pack validator clean (0 errors, was 55). Full backend suite (892 passed, 5 skipped - pre-existing/environment-only). alembic heads still resolves to exactly one head (3a76ff935bf6). ruff check and format --check clean. Diff confirmed scoped to exactly the 11 affected rule_ids across the 4 framework files plus the CHANGELOG cleanup - no other entries touched. Signed-off-by: Parth J Rohit <parthrohit60@gmail.com> Signed-off-by: parthrohit22 <parthrohit60@gmail.com>
Summary
Implements the complete enterprise network and perimeter security backlog from issue #253 with ten evidence-rich Azure controls covering Private Link, critical PaaS exposure, Azure Firewall threat intelligence, Application Gateway WAF enforcement, diagnostics, managed rules, bot protection, and rate limiting.
What changed
AZ-NET-018to detect supported Private Endpoint targets that retain public network access.AZ-NET-019for pending, rejected, or disconnected Private Endpoint connections.AZ-NET-020for approved Private Endpoints without a Private DNS zone group.AZ-NET-021for Private Endpoint FQDN evidence that resolves only to public addresses.AZ-NET-022for public Storage, Key Vault, Azure SQL, PostgreSQL, and App Service resources without an exact approved exception.AZ-NET-023to require Azure Firewall threat intelligenceAlertAndDenyenforcement.AZ-NET-024to require Application Gateway WAF Prevention mode.AZ-NET-025to require Application Gateway access, performance, and firewall diagnostic logs.AZ-NET-026to require a current OWASP 3.2 or DRS 2.1+ base rule set plus Bot Manager 1.0+.AZ-NET-027to require an enabled WAFRateLimitRuleon public Application Gateways.N/A-*CIS decisions instead ofTBD-*placeholders.False-finding safeguards
OPENSHIELD_PUBLIC_PAAS_EXCEPTIONS; partial matches are not accepted.Verification
Type of change
Testing
Checklist
Signed-off-bytrailerAZ-NET-018throughAZ-NET-027TBD-*framework mappings were introduceddevRelated issue
Closes #253
Files to review
scanner/azure_client.py- failure-aware Private Link, PaaS, Application Gateway, WAF policy, and diagnostic collectors.scanner/rules/az_net_018.pythroughscanner/rules/az_net_027.py- the ten enterprise perimeter controls.tests/test_private_link_rules.pyandtests/test_enterprise_perimeter_rules.py- positive, negative, empty, malformed, exception, and failure coverage.playbooks/cli/fix_az_net_018.shthroughplaybooks/cli/fix_az_net_027.sh- review-gated remediation guidance.compliance/frameworks/*.jsonandcompliance/assurance/network_layer.json- verified mappings and OSI classifications.docs/private-link-controls.md,docs/rules-reference.md, andCHANGELOG.md- behavior, limitations, permissions, and release documentation.