Skip to content

feat: add VM Scale Set inventory collector and rule AZ-CMP-005 - #275

Open
dipeshrayg wants to merge 2 commits into
openshield-org:devfrom
dipeshrayg:feat/az-cmp-005-vmss-inventory
Open

feat: add VM Scale Set inventory collector and rule AZ-CMP-005#275
dipeshrayg wants to merge 2 commits into
openshield-org:devfrom
dipeshrayg:feat/az-cmp-005-vmss-inventory

Conversation

@dipeshrayg

Copy link
Copy Markdown
Contributor

What does this PR do?

Adds the first VM Scale Set coverage to OpenShield: a new AzureClient.get_virtual_machine_scale_sets() inventory collector, plus AZ-CMP-005, which flags VMSS network interface configurations with a public IP and no NSG.

Type of change

  • New scan rule
  • Remediation playbook
  • Compliance mapping

Rule details

  • Rule ID: AZ-CMP-005
  • Severity: HIGH
  • Category: Compute
  • Frameworks mapped: CIS (see note below) / NIST / ISO 27001 / SOC 2

Design notes

Per the issue's guidance, this reuses AZ-CMP-001's detection logic (public IP + no NSG) rather than inventing new logic, applied to the VMSS network profile template instead of a standalone VM's NIC. A VMSS's virtual_machine_profile.network_profile.network_interface_configurations[] embeds ip_configurations[].public_ip_address_configuration and .network_security_group directly (confirmed against the installed azure-mgmt-compute SDK models), so no second collector call (e.g. resolving a NIC by ID) is needed the way AZ-CMP-001 does.

Testing

  • Tested against a real Azure free trial subscription — not done, no live subscription available
  • Returns correct JSON output — tests/test_rules_compute.py covers compliant (NSG present), compliant (no public IP), non-compliant, missing-profile, and multi-config cases; tests/test_azure_client_management.py covers the new collector's success/failure paths
  • All seven CI-equivalent checks reproduced locally and passing: rule syntax, rule structure/duplicate-ID validation, hardcoded-credential scan, playbook existence + bash syntax, compliance JSON validity, API syntax, and compliance↔rule cross-reference
  • No hardcoded credentials or secrets
  • Full test suite (253 relevant tests) + ruff check pass locally

Related issue

Closes #271

Note on the CIS mapping

CIS 7.1 ("Ensure that Network Security Groups are attached to network interfaces with public IP addresses") is the same real control, already assigned to AZ-CMP-001 under this repo's one-CIS-ID-per-rule convention. Rather than reuse 7.1 or invent a second number CIS doesn't define, AZ-CMP-005 is mapped to N/A-CMP-005, mirroring the pattern already established for AZ-KV-001 (N/A-KV-001, referencing AZ-KV-004's 8.5). NIST/ISO27001/SOC2 do reuse AZ-CMP-001's control IDs directly, consistent with test_cis_benchmark_mapping.py's own stated policy that only CIS enforces one-rule-per-control.

Checklist

  • Every commit includes a DCO Signed-off-by trailer (git commit -s)
  • 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

Adds AzureClient.get_virtual_machine_scale_sets() (list_all across the
subscription, following the get_virtual_machines() pattern) and its
MockAzureClient test double.

Ships AZ-CMP-005 as the first rule to use it: flags VMSS network
interface configurations that provision a public IP with no NSG
attached, the VMSS-template equivalent of AZ-CMP-001's per-VM NIC
check. Detection reads the network interface configuration template
directly (network_interface_configurations[].ip_configurations[] /
.network_security_group) rather than resolving separate NIC resources,
since a VMSS profile embeds these settings inline.

Includes a remediation playbook (az vmss update --set on the network
profile, with an explicit warning about the required instance
upgrade), collector and rule tests, and compliance framework mappings.
CIS is mapped to N/A-CMP-005 following the repository's established
convention for the same real control (7.1, owned by AZ-CMP-001)
applied to a second resource type, since the one-CIS-ID-per-rule
convention doesn't allow reusing 7.1 directly.

Closes openshield-org#271

Signed-off-by: Dipesh Ray <dipesh.ray.g@gmail.com>

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

@dipeshrayg The VMSS work is useful, but it conflicts with #273 because both PRs use AZ-CMP-005. Please rename this rule to AZ-CMP-006 and update the rule, playbook, tests and all framework keys. Also add the missing SOC 2 entry to the rule's FRAMEWORKS mapping and account for subnet-level NSGs so a protected VMSS is not falsely flagged.

@m-khan-97

Copy link
Copy Markdown
Collaborator

Heads-up: AZ-CMP-005 is already claimed by #273 (@shariqueahmad108-ship-it, opened a day before this one) — Trusted Launch (Secure Boot + vTPM) on Gen2 VMs. Both PRs create scanner/rules/az_cmp_005.py at the same path with the same RULE_ID, and both add an AZ-CMP-005 entry to all four compliance framework JSONs.

Whichever merges first will claim the ID cleanly; this one will then hit a git conflict on rebase (new file at an already-existing path) and, if force-resolved, fail CI's duplicate-RULE_ID structure check.

Since this PR's rule is materially different (VMSS network interface exposure, not Trusted Launch), the fix is just a renumber — could you rebase this onto whichever of #273 or dev merges first and bump this rule to AZ-CMP-006 (rule file, playbook, and all four compliance JSON entries)? Happy to take another look once that's done.

AZ-CMP-005 collided with openshield-org#273 (Trusted Launch check), opened a day
before this PR and already claiming that rule ID. Renumbered the rule
file, playbook, tests, and all four compliance framework entries to
AZ-CMP-006.

Also fixes a false-positive gap flagged in review: the rule only
checked for an NSG on the VMSS network interface configuration itself,
missing the case where the NSG is attached at the subnet level
instead. A VMSS whose NIC has no NSG but deploys into a
subnet that does have one was being incorrectly flagged. Now resolves
each network interface configuration's subnet (via the existing
get_virtual_networks() collector, no new collector needed) and treats
either a NIC-level or subnet-level NSG as compliant, matching how
AZ-NET-010 already reads subnet.network_security_group.

Added SOC2 to the rule's own FRAMEWORKS dict (was previously only in
soc2.json, inconsistent with how several other rules, e.g. AZ-KV-006,
already include it directly).

Addresses review feedback from TFT444 and m-khan-97 on openshield-org#275.

Signed-off-by: Dipesh Ray <dipesh.ray.g@gmail.com>
@dipeshrayg

Copy link
Copy Markdown
Contributor Author

Thanks both for catching this.

  • Renumbered to AZ-CMP-006 — rule file, playbook, tests, and all four compliance framework entries (rule filename/path, RULE_ID/RULE_NAME references, playbook header, and JSON keys in cis_azure_benchmark/nist_csf/iso27001/soc2).
  • Added the missing SOC2 entry to the rule's own FRAMEWORKS dict (it was only in soc2.json before).
  • Fixed the subnet-level NSG gap: the rule now resolves each network interface configuration's subnet via the existing get_virtual_networks() collector and treats either a NIC-level or subnet-level NSG as compliant — no new collector needed, mirrors how AZ-NET-010 already reads subnet.network_security_group. Added two regression tests for this (subnet-protected VMSS returns no findings, VMSS with neither NIC nor subnet NSG still flags).

Verified locally: full test suite (46 compute/client/mapping tests + the two new subnet cases), ruff clean, and all 7 CI-equivalent checks (syntax, structure/dup-ID, credential scan, playbook existence+syntax, compliance JSON validity, API syntax, cross-reference) reproduced and passing — no more AZ-CMP-005 references anywhere in the tree. Rebased is unnecessary since #273 hasn't merged yet and this PR no longer touches AZ-CMP-005 at all, so there's nothing left to conflict on.

@dipeshrayg

Copy link
Copy Markdown
Contributor Author

Hey @TFT444 @m-khan-97 — I've pushed the fixes for both of your comments: renumbered to AZ-CMP-006, added the SOC2 mapping, and fixed the subnet-level NSG false positive with new regression tests for it. All CI checks are green. Let me know if there's anything else you'd like changed, happy to take another pass whenever you get a chance to look.

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.

feat: add VM Scale Set inventory, collectors, and first rule

3 participants