diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml new file mode 100644 index 00000000..95f55108 --- /dev/null +++ b/.github/workflows/ci.yml @@ -0,0 +1,370 @@ +name: OpenShield CI + +on: + pull_request: + branches: + - dev + - main + +jobs: + ci-checks: + name: Run All CI Checks + runs-on: ubuntu-latest + + steps: + # ── 1. Checkout the code ───────────────────────────────────────── + - name: Checkout repository + uses: actions/checkout@v4 + + # ── 2. Set up Python ───────────────────────────────────────────── + - name: Set up Python 3.11 + uses: actions/setup-python@v5 + with: + python-version: "3.11" + + # ── 3. Install dependencies ─────────────────────────────────────── + - name: Install dependencies + run: | + python -m pip install --upgrade pip + pip install -r requirements.txt + + # ── CHECK 1: Python syntax on all rule files ─────────────────────── + - name: Python syntax check (rule files) + id: syntax_check + run: | + echo "=== Checking Python syntax on scanner/rules/ ===" + FAIL=0 + for f in scanner/rules/az_*.py; do + if ! python -m py_compile "$f" 2>&1; then + echo "SYNTAX ERROR: $f" + FAIL=1 + else + echo "OK: $f" + fi + done + if [ "$FAIL" -eq 1 ]; then + echo "One or more rule files have syntax errors." + exit 1 + fi + + # ── CHECK 2: Rule structure validation + RULE_ID uniqueness ────── + - name: Rule structure validation + id: structure_check + run: | + echo "=== Validating rule file structure ===" + python - <<'PYEOF' + import os + import importlib.util + import sys + from collections import defaultdict + + rules_dir = "scanner/rules" + required_fields = ["RULE_ID", "SEVERITY", "FRAMEWORKS"] + valid_severities = {"CRITICAL", "HIGH", "MEDIUM", "LOW", "INFO"} + failures = [] + seen_ids = defaultdict(list) + + for filename in sorted(os.listdir(rules_dir)): + if not filename.startswith("az_") or not filename.endswith(".py"): + continue + + filepath = os.path.join(rules_dir, filename) + spec = importlib.util.spec_from_file_location("rule", filepath) + mod = importlib.util.module_from_spec(spec) + + try: + spec.loader.exec_module(mod) + except Exception as e: + failures.append(f"{filename}: import error — {e}") + continue + + for field in required_fields: + if not hasattr(mod, field): + failures.append(f"{filename}: missing field '{field}'") + + if hasattr(mod, "SEVERITY"): + if mod.SEVERITY not in valid_severities: + failures.append( + f"{filename}: SEVERITY '{mod.SEVERITY}' not in {valid_severities}" + ) + + if hasattr(mod, "FRAMEWORKS"): + if not isinstance(mod.FRAMEWORKS, dict) or len(mod.FRAMEWORKS) == 0: + failures.append(f"{filename}: FRAMEWORKS must be a non-empty dict") + + if hasattr(mod, "RULE_ID"): + seen_ids[mod.RULE_ID].append(filename) + + # Two files sharing a RULE_ID silently corrupt scan reports + for rule_id, files in seen_ids.items(): + if len(files) > 1: + failures.append( + f"DUPLICATE RULE_ID '{rule_id}' in: {', '.join(files)}" + ) + + if failures: + print("RULE STRUCTURE FAILURES:") + for f in failures: + print(f" - {f}") + sys.exit(1) + else: + print(f"All {len(seen_ids)} rule files passed structure validation.") + PYEOF + + # ── CHECK 3: Hardcoded credential scan ──────────────────────────── + - name: Hardcoded credential scan + id: cred_scan + run: | + echo "=== Scanning for hardcoded credentials ===" + PATTERNS=( + "password\s*=" + "secret\s*=" + "api_key\s*=" + "client_secret\s*=" + "AZURE_CLIENT_SECRET\s*=\s*['\"][^'\"]\+" + "-----BEGIN.*PRIVATE KEY-----" + "AccountKey=" + ) + + FAIL=0 + for pattern in "${PATTERNS[@]}"; do + matches=$(grep -rniE "$pattern" \ + --include="*.py" --include="*.sh" --include="*.json" --include="*.yml" \ + --exclude-dir=".git" \ + --exclude-dir="venv" \ + --exclude="ci.yml" \ + . 2>/dev/null | \ + grep -v "\.env" | \ + grep -v "os\.environ" | \ + grep -v "os\.getenv" | \ + grep -v "#" | \ + grep -v "example" | \ + grep -v "placeholder" || true) + + if [ -n "$matches" ]; then + echo "POTENTIAL CREDENTIAL LEAK — pattern '$pattern':" + echo "$matches" + FAIL=1 + fi + done + + if [ "$FAIL" -eq 1 ]; then + echo "Hardcoded credentials detected. Remove them and use environment variables." + exit 1 + else + echo "No hardcoded credentials found." + fi + + # ── CHECK 4: Playbook existence + bash syntax ───────────────────── + - name: Playbook existence and syntax check + id: playbook_check + run: | + echo "=== Checking playbooks exist and are valid bash ===" + FAIL=0 + for rule_file in scanner/rules/az_*.py; do + filename=$(basename "$rule_file" .py) + playbook="playbooks/cli/fix_${filename}.sh" + + if [ ! -f "$playbook" ]; then + echo "MISSING PLAYBOOK: $playbook (required for $rule_file)" + FAIL=1 + elif ! bash -n "$playbook" 2>&1; then + echo "BASH SYNTAX ERROR: $playbook" + FAIL=1 + else + echo "OK: $playbook" + fi + done + + if [ "$FAIL" -eq 1 ]; then + echo "Fix the missing or broken playbook(s) before this PR can merge." + exit 1 + fi + + # ── CHECK 5: Compliance JSON validation ─────────────────────────── + - name: Compliance JSON validation + id: json_check + run: | + echo "=== Validating compliance framework JSON files ===" + python - <<'PYEOF' + import json + import sys + import os + + framework_dir = "compliance/frameworks" + expected_files = [ + "cis_azure_benchmark.json", + "nist_csf.json", + "iso27001.json", + "soc2.json", + ] + failures = [] + + for fname in expected_files: + fpath = os.path.join(framework_dir, fname) + + if not os.path.exists(fpath): + failures.append(f"MISSING FILE: {fpath}") + continue + + try: + with open(fpath) as f: + data = json.load(f) + + if not isinstance(data, dict) or len(data) == 0: + failures.append(f"{fname}: must be a non-empty JSON object") + continue + + n_controls = len(data.get("controls", {})) + print(f"OK: {fname} ({n_controls} controls)") + + except json.JSONDecodeError as e: + failures.append(f"{fname}: invalid JSON — {e}") + + if failures: + print("COMPLIANCE JSON FAILURES:") + for f in failures: + print(f" - {f}") + sys.exit(1) + PYEOF + + # ── CHECK 6: API syntax check ────────────────────────────────────── + - name: API syntax check + id: api_check + run: | + echo "=== Checking Python syntax on API files ===" + FAIL=0 + if [ -d "api" ]; then + while IFS= read -r -d '' f; do + if ! python -m py_compile "$f" 2>&1; then + echo "SYNTAX ERROR: $f" + FAIL=1 + else + echo "OK: $f" + fi + done < <(find api/ -name "*.py" -print0) + else + echo "No api/ directory found — skipping" + fi + + if [ "$FAIL" -eq 1 ]; then + echo "One or more API files have syntax errors." + exit 1 + fi + + # ── CHECK 7: Compliance JSON ↔ rule file cross-reference ────────── + - name: Compliance rule cross-reference + id: xref_check + run: | + echo "=== Cross-referencing compliance controls against rule files ===" + python - <<'PYEOF' + import json + import os + import importlib.util + import sys + + rules_dir = "scanner/rules" + framework_dir = "compliance/frameworks" + + existing_ids = set() + for filename in os.listdir(rules_dir): + if not filename.startswith("az_") or not filename.endswith(".py"): + continue + filepath = os.path.join(rules_dir, filename) + spec = importlib.util.spec_from_file_location("rule", filepath) + mod = importlib.util.module_from_spec(spec) + try: + spec.loader.exec_module(mod) + if hasattr(mod, "RULE_ID"): + existing_ids.add(mod.RULE_ID) + except Exception: + pass + + failures = [] + + for fname in os.listdir(framework_dir): + if not fname.endswith(".json"): + continue + fpath = os.path.join(framework_dir, fname) + try: + data = json.load(open(fpath)) + except (json.JSONDecodeError, OSError): + continue + + for rule_id in data.get("controls", {}): + if rule_id not in existing_ids: + failures.append( + f"{fname}: references '{rule_id}' but no matching rule file found" + ) + + if failures: + print("COMPLIANCE CROSS-REFERENCE FAILURES:") + for f in failures: + print(f" - {f}") + print() + print("Either add the missing rule file or remove the stale control mapping.") + sys.exit(1) + else: + print(f"All compliance controls map to existing rule files. ({len(existing_ids)} rules checked)") + PYEOF + + # ── Final summary — always runs, shows per-check pass/fail ──────── + - name: CI Summary + if: always() + env: + SYNTAX: ${{ steps.syntax_check.outcome }} + STRUCTURE: ${{ steps.structure_check.outcome }} + CREDS: ${{ steps.cred_scan.outcome }} + PLAYBOOK: ${{ steps.playbook_check.outcome }} + JSON: ${{ steps.json_check.outcome }} + API: ${{ steps.api_check.outcome }} + XREF: ${{ steps.xref_check.outcome }} + run: | + python - <<'PYEOF' + import os + + checks = [ + ("Python syntax (rule files)", os.environ["SYNTAX"]), + ("Rule structure + RULE_ID uniqueness", os.environ["STRUCTURE"]), + ("Hardcoded credential scan", os.environ["CREDS"]), + ("Playbook existence + bash syntax", os.environ["PLAYBOOK"]), + ("Compliance JSON validation", os.environ["JSON"]), + ("API syntax check", os.environ["API"]), + ("Compliance vs rule cross-reference", os.environ["XREF"]), + ] + + labels = { + "success": "PASS", + "failure": "FAIL", + "skipped": "SKIP", + "cancelled": "CANCELLED", + } + + lines = [ + "## OpenShield CI Results", + "", + "| Check | Result |", + "|---|---|", + ] + + all_passed = True + for name, outcome in checks: + label = labels.get(outcome, outcome.upper()) + lines.append(f"| {name} | {label} |") + if outcome != "success": + all_passed = False + + lines.append("") + if all_passed: + lines.append("**Result: All checks passed.**") + else: + lines.append("**Result: One or more checks failed. See the step logs above for details.**") + + summary = "\n".join(lines) + print(summary) + + summary_path = os.environ.get("GITHUB_STEP_SUMMARY") + if summary_path: + with open(summary_path, "a") as f: + f.write(summary + "\n") + PYEOF diff --git a/api/models/finding.py b/api/models/finding.py index 90b86623..8cdab3f6 100644 --- a/api/models/finding.py +++ b/api/models/finding.py @@ -20,6 +20,7 @@ "cis": "cis_azure_benchmark.json", "nist": "nist_csf.json", "iso27001": "iso27001.json", + "soc2": "soc2.json", } diff --git a/api/routes/compliance.py b/api/routes/compliance.py index e3b68a23..6a3b104f 100644 --- a/api/routes/compliance.py +++ b/api/routes/compliance.py @@ -7,7 +7,7 @@ compliance_bp = Blueprint("compliance", __name__) -SUPPORTED_FRAMEWORKS = ("cis", "nist", "iso27001") +SUPPORTED_FRAMEWORKS = ("cis", "nist", "iso27001", "soc2") def _get_db() -> DatabaseManager: @@ -20,7 +20,7 @@ def _get_db() -> DatabaseManager: def get_compliance(framework: str): """Return pass/fail compliance breakdown for a framework. - Supported frameworks: cis, nist, iso27001 + Supported frameworks: cis, nist, iso27001, soc2 Returns control-level pass/fail status mapped to current open findings. """ diff --git a/compliance/frameworks/iso27001.json b/compliance/frameworks/iso27001.json index a3792b1a..9d78a930 100644 --- a/compliance/frameworks/iso27001.json +++ b/compliance/frameworks/iso27001.json @@ -89,26 +89,13 @@ "description": "Virtual machines with public IPs and no NSG have unrestricted network access. Network controls should be applied to all compute resources accessible from the internet." }, "AZ-KV-001": { -<<<<<<< feat/network-rules-expansion "control_id": "A.12.3.1", "control_name": "Information backup", "description": "Key Vault soft delete protects against loss of secrets, keys and certificates. Backup copies of information should be taken and tested regularly in accordance with an agreed backup policy." } -======= - "control_id": "A.17.2.1", - "control_name": "Availability of information processing facilities", - "description": "Information processing facilities shall be implemented with sufficient redundancy to meet availability requirements. Disabling soft delete on Key Vault removes the ability to recover deleted secrets, keys, and certificates, creating a single point of failure for critical cryptographic material and violating availability and recovery requirements." - }, - "AZ-STOR-003": { - "control_id": "A.8.3.1", - "control_name": "Management of removable media", - "description": "Information stored on Azure storage accounts should be subject to formal lifecycle management controls governing retention and disposal. Storage accounts without lifecycle policies retain data indefinitely with no automated disposal mechanism, violating information handling and disposal requirements under this control." - }, - "AZ-KV-002": { - "control_id": "A.13.1.1", - "control_name": "Network controls", - "description": "Networks should be managed and controlled to protect information systems and applications. Allowing public network access to Azure Key Vault increases exposure of sensitive secrets, keys, and certificates to external networks. Access should be restricted to trusted networks using private endpoints or network controls." - } ->>>>>>> dev + "control_id": "A.13.1.1", + "control_name": "Network controls", + "description": "Networks should be managed and controlled to protect information systems and applications. Allowing public network access to Azure Key Vault increases exposure of sensitive secrets, keys, and certificates to external networks. Access should be restricted to trusted networks using private endpoints or network controls." + } } } diff --git a/compliance/frameworks/nist_csf.json b/compliance/frameworks/nist_csf.json index 2c4ddf88..7a9ebbaf 100644 --- a/compliance/frameworks/nist_csf.json +++ b/compliance/frameworks/nist_csf.json @@ -91,7 +91,12 @@ "AZ-KV-001": { "control_id": "PR.IP-4", "control_name": "Backups of information are conducted, maintained, and tested", - "description": "Key Vault soft delete protects against accidental or malicious deletion of secrets, keys and certificates. Without soft delete, deleted vault objects cannot be recovered, causing potential data loss." + "description": "Key material in Azure Key Vault must be recoverable after accidental or malicious deletion. Soft delete provides a recoverable state for secrets, keys, and certificates, supporting backup and recovery requirements for critical cryptographic material." + }, + "AZ-STOR-003": { + "control_id": "PR.DS-3", + "control_name": "Assets are formally managed throughout removal, transfers, and disposition", + "description": "NIST CSF PR.DS-3 requires that data assets are managed through their full lifecycle including secure disposal. Storage accounts without a lifecycle management policy have no automated mechanism for expiring or deleting aged data, meaning data subject to disposal requirements persists indefinitely and is never formally retired from the asset inventory." } } } diff --git a/compliance/frameworks/soc2.json b/compliance/frameworks/soc2.json new file mode 100644 index 00000000..7de22578 --- /dev/null +++ b/compliance/frameworks/soc2.json @@ -0,0 +1,107 @@ +{ + "framework": "SOC 2 Type II", + "version": "2017", + "published": "2017-04", + "controls": { + "AZ-STOR-001": { + "control_id": "CC6.6", + "control_name": "Restricts Access to Information Assets", + "description": "Public blob access allows unauthenticated users from outside the network boundary to read storage data without credentials. CC6.6 requires that access from outside the network perimeter is restricted and controlled. Disabling public access enforces this boundary by requiring authentication for all storage operations." + }, + "AZ-STOR-002": { + "control_id": "CC6.7", + "control_name": "Protects Data in Transit", + "description": "Allowing unencrypted HTTP traffic to a storage account exposes data in transit to interception and tampering. CC6.7 requires that data transmitted over networks is protected using encryption. Enforcing HTTPS-only ensures all storage traffic is encrypted in transit." + }, + "AZ-STOR-003": { + "control_id": "CC8.1", + "control_name": "Change Management", + "description": "A storage account with no lifecycle management policy allows data to accumulate indefinitely with no automatic expiry or tiering. CC8.1 requires that infrastructure and data are managed through formal processes. Implementing a lifecycle policy ensures data retention is controlled and old data is automatically moved or deleted according to organisational policy." + }, + "AZ-NET-001": { + "control_id": "CC6.6", + "control_name": "Restricts Access from Outside the Network Boundary", + "description": "An NSG allowing unrestricted RDP access from the internet permits any external party to attempt remote access to virtual machines. CC6.6 requires that logical access from outside the network boundary is restricted. Limiting RDP to known IP ranges enforces this boundary and eliminates unauthorised remote access attempts." + }, + "AZ-NET-002": { + "control_id": "CC6.6", + "control_name": "Restricts Access from Outside the Network Boundary", + "description": "An NSG allowing unrestricted SSH access from the internet exposes virtual machines to brute force and credential attacks from any external party. CC6.6 requires that access from outside the network perimeter is restricted and controlled. Restricting SSH to known IP ranges or removing it in favour of Azure Bastion enforces this boundary." + }, + "AZ-NET-003": { + "control_id": "CC6.6", + "control_name": "Restricts Access from Outside the Network Boundary", + "description": "An NSG permitting unrestricted inbound access on port 443 from the internet exposes web services to automated scanning and exploitation attempts from any external source. CC6.6 requires that access from outside the network boundary is restricted to authorised sources. Public-facing services should be fronted by a WAF-enabled Application Gateway rather than exposed directly." + }, + "AZ-NET-004": { + "control_id": "CC6.6", + "control_name": "Restricts Access from Outside the Network Boundary", + "description": "A Network Security Group with no custom rules provides no meaningful boundary control and relies entirely on Azure defaults. CC6.6 requires that logical access from outside the network perimeter is explicitly restricted. Explicit least-privilege rules must be defined to enforce the network boundary." + }, + "AZ-NET-005": { + "control_id": "A1.1", + "control_name": "Capacity and Performance Monitoring", + "description": "Virtual networks without DDoS Protection Standard are vulnerable to volumetric attacks that can exhaust capacity and cause service outages. A1.1 requires that current processing capacity is monitored and resources are available to meet objectives. DDoS Protection Standard ensures network availability is maintained under attack conditions." + }, + "AZ-NET-006": { + "control_id": "CC6.6", + "control_name": "Restricts Access from Outside the Network Boundary", + "description": "Unassociated public IP addresses represent unnecessary exposure on the internet and may indicate leftover resources from decommissioned workloads. CC6.6 requires that the network boundary is tightly controlled with only necessary resources exposed. Removing unassociated public IPs reduces the external attack surface." + }, + "AZ-NET-007": { + "control_id": "CC6.6", + "control_name": "Restricts Access from Outside the Network Boundary", + "description": "An Application Gateway without WAF enabled provides no protection against web application attacks from external sources including OWASP Top 10 vulnerabilities. CC6.6 requires that access from outside the network boundary is controlled and filtered. WAF in Prevention mode enforces application-layer boundary protection for public-facing services." + }, + "AZ-NET-008": { + "control_id": "CC8.1", + "control_name": "Change Management", + "description": "A load balancer with no backend pool configured is either misconfigured or a leftover resource from a decommissioned workload that was not properly cleaned up. CC8.1 requires that infrastructure changes are managed, tracked and that unused resources are removed through a formal process. Removing empty load balancers maintains an accurate and controlled infrastructure state." + }, + "AZ-NET-009": { + "control_id": "CC6.7", + "control_name": "Protects Data in Transit", + "description": "VPN gateway connections using IKEv1 use an outdated protocol with known vulnerabilities that weaken the confidentiality and integrity of data transmitted between networks. CC6.7 requires that data transmitted over networks is protected using current secure protocols. Migrating to IKEv2 ensures VPN traffic is protected with a modern and secure key exchange mechanism." + }, + "AZ-NET-010": { + "control_id": "CC6.6", + "control_name": "Restricts Access from Outside the Network Boundary", + "description": "A subnet without an NSG attached has no network layer access controls leaving all resources in that subnet reachable from other subnets or the internet with no filtering. CC6.6 requires that logical access from outside the network boundary is restricted. Attaching an NSG with explicit rules enforces boundary protection at the subnet level." + }, + "AZ-IDN-001": { + "control_id": "CC6.1", + "control_name": "Logical Access Security Measures", + "description": "A service principal with Contributor role at subscription scope has unrestricted ability to create, modify and delete any resource in the environment. CC6.1 requires that logical access to information assets is restricted to authorised users and service accounts with least-privilege permissions. Scoping role assignments to the minimum required resource enforces this control." + }, + "AZ-IDN-002": { + "control_id": "CC6.1", + "control_name": "Logical Access Security Measures", + "description": "Without MFA enforced on privileged accounts, a single compromised password grants full administrative access to the Azure environment. CC6.1 requires that logical access controls include strong authentication mechanisms. Enforcing MFA via Conditional Access policies ensures privileged access requires multiple factors of authentication." + }, + "AZ-DB-001": { + "control_id": "CC6.7", + "control_name": "Protects Data in Transit", + "description": "SQL Server without Transparent Data Encryption stores database files in plain text on disk. CC6.7 requires that data is protected using encryption both in transit and at rest. Enabling TDE ensures database files, backups and transaction logs are encrypted and unreadable without the encryption key." + }, + "AZ-DB-002": { + "control_id": "CC6.6", + "control_name": "Restricts Access from Outside the Network Boundary", + "description": "A SQL Server firewall rule allowing all IP addresses makes the database reachable from anywhere on the internet. CC6.6 requires that access from outside the network boundary is restricted to authorised sources. Locking the firewall to specific application IP ranges ensures only authorised systems can connect to the database." + }, + "AZ-CMP-001": { + "control_id": "CC6.6", + "control_name": "Restricts Access from Outside the Network Boundary", + "description": "A virtual machine with a public IP and no NSG has unrestricted inbound network access from the internet with no filtering in place. CC6.6 requires that logical access from outside the network perimeter is restricted and controlled. Attaching an NSG with explicit rules enforces the network boundary and controls what traffic can reach the VM." + }, + "AZ-KV-001": { + "control_id": "A1.2", + "control_name": "Environmental Threats and Recovery", + "description": "Key Vault without soft delete enabled allows permanent deletion of secrets, keys and certificates with no recovery possible. A1.2 requires that environmental threats to availability are identified and mitigated including protection against accidental or malicious data loss. Enabling soft delete ensures deleted vault objects can be recovered within the retention period." + }, + "AZ-KV-002": { + "control_id": "CC6.6", + "control_name": "Restricts Access from Outside the Network Boundary", + "description": "A Key Vault accessible from the public internet allows any external party to attempt access to secrets, keys and certificates. CC6.6 requires that access from outside the network boundary is restricted and controlled. Locking Key Vault access to private endpoints or specific VNet service endpoints enforces this boundary and protects sensitive credentials from external exposure." + } + } +} diff --git a/docs/az-stor-003-test-plan.md b/docs/az-stor-003-test-plan.md new file mode 100644 index 00000000..65fb2729 --- /dev/null +++ b/docs/az-stor-003-test-plan.md @@ -0,0 +1,392 @@ +# Test Plan — AZ-STOR-003 +# Storage Account Has No Lifecycle Management Policy +# ============================================================ + +## 1. Overview + +This test plan covers verification of the AZ-STOR-003 scanner rule +and its remediation playbook. The goal is to confirm: + +- The rule correctly identifies non-compliant storage accounts +- The rule correctly ignores compliant storage accounts +- The playbook successfully creates a lifecycle policy +- The rule finds zero issues after the playbook runs + +--- + +## 2. Files Under Test + +| File | Purpose | +|---|---| +| scanner/rules/az_stor_003.py | Scanner rule | +| playbooks/cli/fix_az_stor_003.sh | Remediation script | +| scanner/azure_client.py | New method: get_storage_lifecycle_policy() | +| compliance/frameworks/cis_azure_benchmark.json | CIS mapping | +| compliance/frameworks/nist_csf.json | NIST mapping | +| compliance/frameworks/iso27001.json | ISO 27001 mapping | + +--- + +## 3. Test Environment Setup + +### 3.1 Prerequisites + +- Python 3.10+ +- Azure free trial account (portal.azure.com) +- Azure CLI installed and logged in (az login) +- OpenShield repo cloned and dependencies installed (pip install -r requirements.txt) +- .env file populated with AZURE_SUBSCRIPTION_ID, AZURE_CLIENT_ID, + AZURE_CLIENT_SECRET, AZURE_TENANT_ID +- StorageV2 accounts used for all tests + +### 3.2 Create Test Resources in Azure + +Run these commands once before testing. They create two storage accounts: +one without a lifecycle policy (should be flagged) and one with a policy +(should NOT be flagged). + + # Create resource group + az group create --name openshield-test-rg --location eastus + + # Storage account WITHOUT lifecycle policy (will be flagged) + az storage account create \ + --name oshieldtestnopolicy \ + --resource-group openshield-test-rg \ + --sku Standard_LRS \ + --location eastus + + # Storage account WITH lifecycle policy (will NOT be flagged) + az storage account create \ + --name oshieldtestpolicyon \ + --resource-group openshield-test-rg \ + --sku Standard_LRS \ + --location eastus + + # Manually apply a policy to the second account + az storage account management-policy create \ + --account-name oshieldtestpolicyon \ + --resource-group openshield-test-rg \ + --policy '{ + "rules": [{ + "name": "test-policy", + "enabled": true, + "type": "Lifecycle", + "definition": { + "filters": {"blobTypes": ["blockBlob"]}, + "actions": { + "baseBlob": { + "delete": {"daysAfterLastAccessTimeGreaterThan": 365} + } + } + } + }] + }' + +--- + +## 4. Test Cases + +--- + +### TC-001 — Rule detects non-compliant account (POSITIVE TEST) + +**Purpose:** Confirm the rule flags a storage account with no lifecycle policy. + +**Pre-condition:** oshieldtestnopolicy exists with no lifecycle policy. + +**Steps:** + + python -c " + from dotenv import load_dotenv; load_dotenv() + import os + from scanner.azure_client import AzureClient + from scanner.rules import az_stor_003 as rule + + client = AzureClient(os.environ['AZURE_SUBSCRIPTION_ID']) + findings = rule.scan(client, os.environ['AZURE_SUBSCRIPTION_ID']) + print(f'Total findings: {len(findings)}') + for f in findings: + print(f' [{f[\"severity\"]}] {f[\"resource_name\"]}') + " + +**Expected result:** +- At minimum one finding returned +- oshieldtestnopolicy appears in the findings list +- Finding has severity = MEDIUM +- Finding has rule_id = AZ-STOR-003 +- Finding dict contains all required keys: + rule_id, rule_name, severity, category, resource_id, resource_name, + resource_type, description, remediation, playbook, frameworks + +**Pass criteria:** oshieldtestnopolicy is in findings list. + +--- + +### TC-002 — Rule ignores compliant account (NEGATIVE TEST) + +**Purpose:** Confirm the rule does NOT flag accounts that already have a policy. + +**Pre-condition:** oshieldtestpolicyon exists WITH a lifecycle policy applied. + +**Steps:** Same script as TC-001. Inspect the findings list. + +**Expected result:** +- oshieldtestpolicyon does NOT appear in the findings list. + +**Pass criteria:** oshieldtestpolicyon absent from findings. + +--- + +### TC-003 — Finding dict has correct structure + +**Purpose:** Confirm every required field is present and correctly typed. + +**Steps:** + + python -c " + from dotenv import load_dotenv; load_dotenv() + import os, json + from scanner.azure_client import AzureClient + from scanner.rules import az_stor_003 as rule + + REQUIRED_KEYS = [ + 'rule_id', 'rule_name', 'severity', 'category', + 'resource_id', 'resource_name', 'resource_type', + 'description', 'remediation', 'playbook', 'frameworks' + ] + + client = AzureClient(os.environ['AZURE_SUBSCRIPTION_ID']) + findings = rule.scan(client, os.environ['AZURE_SUBSCRIPTION_ID']) + + for f in findings: + missing = [k for k in REQUIRED_KEYS if k not in f] + if missing: + print(f'FAIL — missing keys: {missing}') + else: + print(f'PASS — {f[\"resource_name\"]} has all required keys') + print(f' frameworks: {f[\"frameworks\"]}') + print(f' severity: {f[\"severity\"]}') + " + +**Expected result:** +- No missing keys reported +- severity = MEDIUM +- frameworks dict contains CIS, NIST, ISO27001 keys + +**Pass criteria:** All required keys present in every finding. + +--- + +### TC-004 — Full scan engine picks up the rule + +**Purpose:** Confirm the rule loads automatically when the engine runs — +no manual registration needed. + +**Steps:** + + python -c " + from dotenv import load_dotenv; load_dotenv() + import json, os + from scanner.engine import ScanEngine + + engine = ScanEngine(os.environ['AZURE_SUBSCRIPTION_ID']) + rule_ids = [getattr(r, 'RULE_ID', 'UNKNOWN') for r in engine.rules] + print('Loaded rules:', rule_ids) + print('AZ-STOR-003 loaded:', 'AZ-STOR-003' in rule_ids) + " + +**Expected result:** +- AZ-STOR-003 appears in the loaded rules list. + +**Pass criteria:** 'AZ-STOR-003 loaded: True' in output. + +--- + +### TC-005 — Playbook prints usage when called with no arguments + +**Purpose:** Confirm the script does not crash silently and has clear usage. + +**Steps:** + + bash playbooks/cli/fix_az_stor_003.sh + +**Expected result:** +- Prints usage instructions +- Exits with a non-zero exit code (1) +- Does NOT make any changes to Azure + +**Pass criteria:** Usage text displayed, script exits cleanly. + +--- + +### TC-006 — Playbook remediates the non-compliant account + +**Purpose:** Confirm the playbook successfully creates a lifecycle policy. + +**Pre-condition:** oshieldtestnopolicy has no lifecycle policy. + +**Steps:** + + bash playbooks/cli/fix_az_stor_003.sh \ + openshield-test-rg \ + oshieldtestnopolicy \ + 365 + + # Verify the policy was created + az storage account management-policy show \ + --account-name oshieldtestnopolicy \ + --resource-group openshield-test-rg + +**Expected result:** +- Script prints confirmation message +- az management-policy show returns a JSON policy object +- Policy contains a rule named openshield-lifecycle-rule +- Policy shows tierToCool at 30 days, tierToArchive at 90 days, + delete at 365 days + +**Pass criteria:** Policy visible in Azure portal and via CLI show command. + +--- + +### TC-007 — Rule returns zero findings after remediation + +**Purpose:** Full end-to-end — confirm the rule clears after the fix is applied. + +**Pre-condition:** TC-006 has run successfully (oshieldtestnopolicy now has a policy). + +**Steps:** Re-run TC-001 script. + +**Expected result:** +- oshieldtestnopolicy no longer appears in findings. + +**Pass criteria:** Previously flagged account no longer in findings list. + +--- + +### TC-008 — Script handles non-existent account gracefully + +**Purpose:** Confirm the script fails cleanly when given a valid-format name +that does not exist in Azure — the failure comes from the Azure CLI, not +from our validation. + +**Steps:** + + bash playbooks/cli/fix_az_stor_003.sh \ + openshield-test-rg \ + oshieldaccountxyz999 \ + 365 + + # When prompted, enter "y" to proceed past the confirmation. + +**Expected result:** +- Passes all input validation (name format is valid) +- Azure CLI returns a ResourceNotFound error +- Script exits with a non-zero exit code from set -euo pipefail +- Error from Azure CLI is visible in output + +**Pass criteria:** Script exits with Azure error, does not silently continue. + +--- + +### TC-009 — Playbook rejects invalid days-to-delete value + +**Purpose:** Confirm integer validation works — prevents broken JSON policy. + +**Steps:** + + bash playbooks/cli/fix_az_stor_003.sh \ + openshield-test-rg \ + oshieldtestnopolicy \ + "not-a-number" + +**Expected result:** +- Prints: `ERROR: days-to-delete must be a positive integer` +- Exits with code 1 +- Makes no changes to Azure + +**Pass criteria:** Error message displayed, exit code 1. + +--- + +### TC-010 — Playbook rejects shell-unsafe characters in arguments + +**Purpose:** Confirm input sanitisation prevents shell injection. + +**Steps:** + + bash playbooks/cli/fix_az_stor_003.sh \ + "my-rg; echo INJECTED" \ + oshieldtestnopolicy + +**Expected result:** +- Prints: `ERROR: resource-group contains invalid characters` +- Exits with code 1 +- The string "INJECTED" does NOT appear in output + +**Pass criteria:** Error shown, no command injection executed. + +--- + +### TC-011 — Playbook enables last access tracking before applying policy + +**Purpose:** Confirm the prerequisite step runs before the policy is created. +Without last access tracking enabled, `daysAfterLastAccessTimeGreaterThan` +policies are accepted by Azure but never fire — a silent failure. + +**Steps:** + + # Confirm tracking is OFF before the test + az storage account blob-service-properties show \ + --account-name oshieldtestnopolicy \ + --resource-group openshield-test-rg \ + --query "lastAccessTimeTrackingPolicy.enable" + # Should return: null or false + + # Run the playbook (enter "y" when prompted) + bash playbooks/cli/fix_az_stor_003.sh \ + openshield-test-rg \ + oshieldtestnopolicy \ + 365 + + # Confirm tracking is now ON + az storage account blob-service-properties show \ + --account-name oshieldtestnopolicy \ + --resource-group openshield-test-rg \ + --query "lastAccessTimeTrackingPolicy.enable" + # Must return: true + +**Expected result:** +- Before playbook: tracking disabled or null +- After playbook: tracking enabled = true +- Policy also present (verify with management-policy show) + +**Pass criteria:** `lastAccessTimeTrackingPolicy.enable` is `true` after the +playbook runs. + +--- + +## 5. Cleanup + +After all tests pass, delete the test resources to avoid charges: + + az group delete --name openshield-test-rg --yes --no-wait + +--- + +## 6. Pass / Fail Summary Table + +| Test Case | Description | Expected | Status | +|---|---|---|---| +| TC-001 | Rule detects non-compliant account | Finding returned | [ ] | +| TC-002 | Rule ignores compliant account | No finding | [ ] | +| TC-003 | Finding dict structure | All required keys present | [ ] | +| TC-004 | Engine loads rule automatically | AZ-STOR-003 in loaded list | [ ] | +| TC-005 | Playbook prints usage on no args | Usage text + exit 1 | [ ] | +| TC-006 | Playbook creates lifecycle policy | Policy visible in Azure | [ ] | +| TC-007 | Rule clears after remediation | Zero findings post-fix | [ ] | +| TC-008 | Script handles non-existent account | Exits with Azure error | [ ] | +| TC-009 | Playbook rejects non-integer days | Error + exit 1 | [ ] | +| TC-010 | Playbook rejects unsafe characters | Error, no injection | [ ] | +| TC-011 | Playbook enables last access tracking | Tracking = true after run | [ ] | + +All 11 test cases must pass before opening the PR. diff --git a/docs/ci-pipeline.md b/docs/ci-pipeline.md new file mode 100644 index 00000000..e79edb59 --- /dev/null +++ b/docs/ci-pipeline.md @@ -0,0 +1,357 @@ +# CI Pipeline + +OpenShield runs a GitHub Actions workflow on every pull request to `dev` and `main`. The workflow contains seven checks. All seven must pass before a PR can merge. + +This document explains what each check does, how to run every check locally before opening a PR, and the reasoning behind the testing methods chosen. + +--- + +## Checks at a glance + +| # | Check | What fails | +|---|---|---| +| 1 | Python syntax (rule files) | Any `az_*.py` with a syntax error | +| 2 | Rule structure + RULE_ID uniqueness | Missing required fields, invalid SEVERITY, non-dict FRAMEWORKS, duplicate RULE_IDs | +| 3 | Hardcoded credential scan | Literal secrets, keys, or connection strings in source files | +| 4 | Playbook existence + bash syntax | Missing `.sh` for any rule file, or a `.sh` with a bash syntax error | +| 5 | Compliance JSON validation | Missing framework file, invalid JSON, empty object | +| 6 | API syntax check | Any `api/**/*.py` with a syntax error | +| 7 | Compliance rule cross-reference | A rule ID referenced in a framework JSON that has no matching rule file | + +The final step always runs and writes a per-check pass/fail table to the GitHub Actions summary panel so reviewers can see the result without reading through logs. + +--- + +## Setup for local runs + +Before running any checks locally, install the project dependencies including `pyyaml`, which is required to validate the workflow file as valid YAML. + +```bash +pip install -r requirements.txt +``` + +If you prefer to install only what the local checks need without the full Azure SDK stack: + +```bash +pip install pyyaml==6.0.1 +``` + +To verify the workflow file itself is valid YAML before pushing: + +```bash +python -c " +import yaml +with open('.github/workflows/ci.yml') as f: + yaml.safe_load(f) +print('YAML is valid') +" +``` + +This catches structural problems in the workflow file — misaligned indentation, duplicate keys, bad anchors — that GitHub Actions would reject silently or with a confusing error message. + +--- + +## Running checks locally + +Run these from the root of the repository. If any command exits non-zero, CI will also fail. + +### Check 1 — Python syntax (rule files) + +```bash +for f in scanner/rules/az_*.py; do + python -m py_compile "$f" && echo "OK: $f" || echo "FAIL: $f" +done +``` + +A clean run prints `OK:` for every file and exits 0. + +--- + +### Check 2 — Rule structure and RULE_ID uniqueness + +```python +python - <<'PYEOF' +import os, importlib.util, sys +from collections import defaultdict + +rules_dir = "scanner/rules" +required_fields = ["RULE_ID", "SEVERITY", "FRAMEWORKS"] +valid_severities = {"CRITICAL", "HIGH", "MEDIUM", "LOW", "INFO"} +failures = [] +seen_ids = defaultdict(list) + +for filename in sorted(os.listdir(rules_dir)): + if not filename.startswith("az_") or not filename.endswith(".py"): + continue + filepath = os.path.join(rules_dir, filename) + spec = importlib.util.spec_from_file_location("rule", filepath) + mod = importlib.util.module_from_spec(spec) + try: + spec.loader.exec_module(mod) + except Exception as e: + failures.append(f"{filename}: import error — {e}") + continue + for field in required_fields: + if not hasattr(mod, field): + failures.append(f"{filename}: missing field '{field}'") + if hasattr(mod, "SEVERITY") and mod.SEVERITY not in valid_severities: + failures.append(f"{filename}: SEVERITY '{mod.SEVERITY}' is not valid") + if hasattr(mod, "FRAMEWORKS") and (not isinstance(mod.FRAMEWORKS, dict) or len(mod.FRAMEWORKS) == 0): + failures.append(f"{filename}: FRAMEWORKS must be a non-empty dict") + if hasattr(mod, "RULE_ID"): + seen_ids[mod.RULE_ID].append(filename) + +for rule_id, files in seen_ids.items(): + if len(files) > 1: + failures.append(f"DUPLICATE RULE_ID '{rule_id}' in: {', '.join(files)}") + +if failures: + print("FAILURES:") + for f in failures: print(f" - {f}") + sys.exit(1) +else: + print(f"All {len(seen_ids)} rule files passed.") +PYEOF +``` + +--- + +### Check 3 — Hardcoded credential scan + +```bash +PATTERNS=( + "password\s*=" + "secret\s*=" + "api_key\s*=" + "client_secret\s*=" + "AZURE_CLIENT_SECRET\s*=\s*['\"][^'\"]\+" + "-----BEGIN.*PRIVATE KEY-----" + "AccountKey=" +) + +FAIL=0 +for pattern in "${PATTERNS[@]}"; do + matches=$(grep -rniE "$pattern" \ + --include="*.py" --include="*.sh" --include="*.json" --include="*.yml" \ + --exclude-dir=".git" --exclude-dir="venv" --exclude="ci.yml" \ + . 2>/dev/null | \ + grep -v "\.env" | grep -v "os\.environ" | grep -v "os\.getenv" | \ + grep -v "#" | grep -v "example" | grep -v "placeholder" || true) + if [ -n "$matches" ]; then + echo "POTENTIAL LEAK — pattern '$pattern':" + echo "$matches" + FAIL=1 + fi +done +[ "$FAIL" -eq 0 ] && echo "No hardcoded credentials found." || echo "FAIL" +``` + +If this flags a match in your code, replace the literal value with `os.environ["VAR_NAME"]` and store the real value in your `.env` file (which is gitignored). + +--- + +### Check 4 — Playbook existence and bash syntax + +```bash +FAIL=0 +for rule_file in scanner/rules/az_*.py; do + filename=$(basename "$rule_file" .py) + playbook="playbooks/cli/fix_${filename}.sh" + if [ ! -f "$playbook" ]; then + echo "MISSING: $playbook" + FAIL=1 + elif ! bash -n "$playbook" 2>&1; then + echo "BASH SYNTAX ERROR: $playbook" + FAIL=1 + else + echo "OK: $playbook" + fi +done +[ "$FAIL" -eq 0 ] && echo "All playbooks OK." +``` + +`bash -n` parses the script without executing it. It catches undefined syntax such as mismatched `if`/`fi`, unclosed quotes, and bad redirects. It does not execute any Azure CLI commands. + +--- + +### Check 5 — Compliance JSON validation + +```python +python - <<'PYEOF' +import json, os, sys + +framework_dir = "compliance/frameworks" +expected = ["cis_azure_benchmark.json", "nist_csf.json", "iso27001.json", "soc2.json"] +failures = [] + +for fname in expected: + fpath = os.path.join(framework_dir, fname) + if not os.path.exists(fpath): + failures.append(f"MISSING: {fpath}") + continue + try: + data = json.load(open(fpath)) + n = len(data.get("controls", {})) + print(f"OK: {fname} ({n} controls)") + except json.JSONDecodeError as e: + failures.append(f"{fname}: invalid JSON — {e}") + +if failures: + for f in failures: print(f" - {f}") + sys.exit(1) +PYEOF +``` + +--- + +### Check 6 — API syntax check + +```bash +FAIL=0 +if [ -d "api" ]; then + while IFS= read -r -d '' f; do + python -m py_compile "$f" && echo "OK: $f" || { echo "FAIL: $f"; FAIL=1; } + done < <(find api/ -name "*.py" -print0) +else + echo "No api/ directory — skipping" +fi +[ "$FAIL" -eq 0 ] && echo "API syntax OK." +``` + +--- + +### Check 7 — Compliance rule cross-reference + +```python +python - <<'PYEOF' +import json, os, importlib.util, sys + +rules_dir = "scanner/rules" +framework_dir = "compliance/frameworks" + +existing_ids = set() +for filename in os.listdir(rules_dir): + if not filename.startswith("az_") or not filename.endswith(".py"): + continue + spec = importlib.util.spec_from_file_location("rule", os.path.join(rules_dir, filename)) + mod = importlib.util.module_from_spec(spec) + try: + spec.loader.exec_module(mod) + if hasattr(mod, "RULE_ID"): + existing_ids.add(mod.RULE_ID) + except Exception: + pass + +failures = [] +for fname in os.listdir(framework_dir): + if not fname.endswith(".json"): + continue + try: + data = json.load(open(os.path.join(framework_dir, fname))) + except Exception: + continue + for rule_id in data.get("controls", {}): + if rule_id not in existing_ids: + failures.append(f"{fname}: references '{rule_id}' but no rule file found") + +if failures: + for f in failures: print(f" - {f}") + sys.exit(1) +else: + print(f"All compliance controls verified. ({len(existing_ids)} rules checked)") +PYEOF +``` + +--- + +## Testing method rationale + +### Why `py_compile` and not `flake8` or `pylint` + +`py_compile` checks only for syntax errors — the kind that prevent the file from loading at all. Linters add style and convention rules that differ across contributors and would generate noise on code written before the linter was introduced. A syntax check has a binary, objective outcome. That is the right scope for a CI gate on an open source project where contributors are writing their first rules. + +### Why `importlib` and not regex for structure validation + +Regex on Python source is fragile. A field could be assigned via a helper function, computed from a base class, or split across continuation lines. `importlib.util.spec_from_file_location` actually executes the module and then `hasattr()` checks the resulting object — the only way to be certain the attribute is present and accessible at runtime. This is the same mechanism the scanner engine uses when loading rules, so the CI check mirrors what production does. + +### Why `bash -n` and not just checking file existence + +An earlier version of this check only verified that a playbook file existed. A `.sh` file with a bash syntax error — an unclosed `if`, a bad heredoc, a missing `fi` — will crash immediately when an operator runs it in response to a real finding. `bash -n` parses without executing, so it catches structural errors at zero risk of touching any Azure resource. Existence alone is not sufficient. + +### Why the credential scan uses grep exclusions rather than an allowlist + +The patterns being scanned (`password=`, `secret=`, `api_key=`) appear legitimately in two contexts: environment variable lookups (`os.environ`, `os.getenv`) and inline comments. Both are explicitly excluded. The scan is scoped to literal assignment — the pattern that indicates a value is hardcoded in source. A grep-based approach is auditable: every exclusion is visible in one place and any contributor can read exactly what is and is not excluded. + +### Why the credential scan excludes `venv/` + +On GitHub Actions the checkout is clean with no `venv/`. Locally, `venv/` contains thousands of lines from third-party packages that match patterns like `password=None` as function arguments. Excluding `venv/` prevents false positives when contributors run the check locally without creating a confusing discrepancy between local and CI results. + +### Why the cross-reference check walks compliance JSONs rather than rule files + +The check is designed to catch a deletion scenario: a rule file is removed but its entry in one or more compliance JSONs is not. Walking the JSONs and looking up each referenced rule ID against the set of existing rule files catches stale references. The inverse check — verifying every rule file has a compliance entry — is not enforced because a rule may legitimately not map to every framework. + +--- + +## Edge cases handled + +**Rule file has syntax error but passes `py_compile`** +Not possible. `py_compile` detects all syntax errors that prevent the AST from parsing. If `py_compile` passes, the file can be imported. + +**Rule file imports a package not in `requirements.txt`** +Check 2 will fail with `import error` when `spec.loader.exec_module` raises `ModuleNotFoundError`. The error message names the missing package. Add it to `requirements.txt`. + +**Two rule files define the same `RULE_ID`** +Check 2 collects all IDs with `defaultdict(list)` before reporting, so it catches every duplicate in a single run rather than stopping at the first. The failure message names both files. + +**A playbook file exists but contains only a shebang and no logic** +`bash -n` passes — a script with only `#!/bin/bash` is syntactically valid. This is intentional: a stub playbook during development is acceptable; a broken playbook is not. + +**A compliance JSON has a `controls` key with no entries** +Check 5 reports the number of controls but does not fail on zero. An empty `controls` block is structurally valid JSON. Check 7 will simply find nothing to cross-reference. If you want to enforce minimum control counts, add a `len(controls) == 0` check to Check 5. + +**The `api/` directory does not exist** +Check 6 prints `No api/ directory found — skipping` and exits 0. The check is designed to be safe to include before the API module is added. + +**A framework JSON file references a rule ID that was renamed** +Check 7 catches this. The referenced ID will not be in `existing_ids` (which is built from the current `RULE_ID` attribute of each rule file) and CI fails with the exact JSON file and rule ID that is stale. + +**Trailing comma in a compliance JSON** +Check 5 catches this. Python's `json.load` raises `json.JSONDecodeError` on trailing commas, and the failure message includes the line number from the decoder. + +**Local `venv/` directory triggers credential scan false positives** +The scan excludes `--exclude-dir=venv`. On GitHub Actions there is no `venv/` to exclude, so the flag is harmless there. + +--- + +## How the CI summary works + +The final step uses `if: always()` so it runs regardless of whether earlier steps passed or failed. Each check step has a unique `id`. The summary step reads the outcome of every step via environment variables: + +```yaml +- name: CI Summary + if: always() + env: + SYNTAX: ${{ steps.syntax_check.outcome }} + STRUCTURE: ${{ steps.structure_check.outcome }} + ... +``` + +GitHub Actions sets `outcome` to `success`, `failure`, `skipped`, or `cancelled`. The summary step writes a markdown table to `$GITHUB_STEP_SUMMARY`, which GitHub renders as a panel on the Actions run page. This means a reviewer can see which check failed without opening any log. + +When running locally (no `$GITHUB_STEP_SUMMARY` environment variable), the summary is printed to stdout only. + +--- + +## Fixing common failures + +| Failure message | Cause | Fix | +|---|---|---| +| `SYNTAX ERROR: scanner/rules/az_xxx_000.py` | Invalid Python syntax | Open the file, find the syntax error, fix it | +| `missing field 'RULE_ID'` | Rule file does not define `RULE_ID` at module level | Add `RULE_ID = "AZ-XXX-000"` at the top of the file | +| `SEVERITY 'MEDIUM-HIGH' not in {...}` | SEVERITY value is not one of the five allowed strings | Change to `CRITICAL`, `HIGH`, `MEDIUM`, `LOW`, or `INFO` | +| `DUPLICATE RULE_ID 'AZ-NET-003'` | Two rule files declare the same ID | Assign a unique ID to the newer file | +| `POTENTIAL CREDENTIAL LEAK` | A literal secret is present in source | Replace with `os.environ["VAR_NAME"]` | +| `MISSING PLAYBOOK: playbooks/cli/fix_az_xxx_000.sh` | No playbook created for the new rule | Create `playbooks/cli/fix_az_xxx_000.sh` | +| `BASH SYNTAX ERROR: playbooks/cli/fix_az_xxx_000.sh` | Shell script has invalid syntax | Run `bash -n playbooks/cli/fix_az_xxx_000.sh` locally to see the error | +| `invalid JSON — ...` | Trailing comma or other JSON error in a framework file | Open the file, find the bad line (error message includes line number), fix it | +| `references 'AZ-XXX-000' but no matching rule file found` | A compliance JSON references a rule that does not exist | Either create the rule file or remove the entry from the compliance JSON | diff --git a/playbooks/cli/fix_az_stor_003.sh b/playbooks/cli/fix_az_stor_003.sh new file mode 100755 index 00000000..7231d7b8 --- /dev/null +++ b/playbooks/cli/fix_az_stor_003.sh @@ -0,0 +1,195 @@ +#!/bin/bash +# OpenShield Remediation Playbook +# Rule: AZ-STOR-003 — Storage Account Has No Lifecycle Management Policy +# Usage: ./fix_az_stor_003.sh [days-to-delete] +# Severity: MEDIUM +# +# What this script does: +# 1. Enables last access time tracking on the storage account (required +# prerequisite for daysAfterLastAccessTimeGreaterThan policies). +# 2. Creates a lifecycle management policy with three tiers: +# - Move to Cool tier after 30 days of no access +# - Move to Archive tier after 90 days of no access +# - Delete blobs after days (default 365) +# 3. The same delete rule applies to blob snapshots. +# +# Prerequisites: +# - Azure CLI installed and logged in (az login) +# - Contributor or Storage Account Contributor role on the target account +# - The storage account must use StorageV2 or BlobStorage kind for lifecycle +# management. Classic and premium accounts are not supported. +# +# Example: +# ./fix_az_stor_003.sh my-resource-group my-storage-account 365 + +set -euo pipefail + +RESOURCE_GROUP="${1:-}" +STORAGE_ACCOUNT="${2:-}" +DAYS_TO_DELETE="${3:-365}" + +# ── Argument validation ────────────────────────────────────────────────────── + +if [ -z "$RESOURCE_GROUP" ] || [ -z "$STORAGE_ACCOUNT" ]; then + echo "Usage: $0 [days-to-delete]" + echo "" + echo "Arguments:" + echo " resource-group Name of the Azure resource group" + echo " storage-account-name Name of the storage account to remediate" + echo " days-to-delete Days before blobs are permanently deleted (default: 365)" + echo "" + echo "Example:" + echo " $0 my-resource-group my-storage-account 365" + exit 1 +fi + +# ── Validate days-to-delete is a positive integer ──────────────────────────── + +if ! [[ "$DAYS_TO_DELETE" =~ ^[1-9][0-9]*$ ]]; then + echo "ERROR: days-to-delete must be a positive integer (got: '$DAYS_TO_DELETE')" + exit 1 +fi + +# ── Validate names contain only Azure-safe characters ─────────────────────── +# Resource group: letters, numbers, hyphens, underscores, dots, parentheses +# Storage account: lowercase letters and numbers only (Azure naming constraint) + +if ! [[ "$RESOURCE_GROUP" =~ ^[a-zA-Z0-9._()-]+$ ]]; then + echo "ERROR: resource-group contains invalid characters: '$RESOURCE_GROUP'" + exit 1 +fi + +if ! [[ "$STORAGE_ACCOUNT" =~ ^[a-z0-9]{3,24}$ ]]; then + echo "ERROR: storage-account-name must be 3-24 lowercase letters and numbers only." + exit 1 +fi + +# ── Validate DAYS_TO_DELETE range is sane ──────────────────────────────────── +# Azure requires tier transition <= delete threshold. Archive at 90 < delete. + +if [ "$DAYS_TO_DELETE" -lt 91 ]; then + echo "ERROR: days-to-delete must be at least 91 (must exceed the Archive tier at 90 days)" + exit 1 +fi + +# ── Secure temp file with guaranteed cleanup on exit or error ───────────────── + +POLICY_FILE=$(mktemp) +chmod 600 "$POLICY_FILE" + +cleanup() { + rm -f "$POLICY_FILE" +} +trap cleanup EXIT + +# ── Confirm before making changes ──────────────────────────────────────────── + +echo "============================================================" +echo " OpenShield Remediation — AZ-STOR-003" +echo "============================================================" +echo "" +echo " Storage account : $STORAGE_ACCOUNT" +echo " Resource group : $RESOURCE_GROUP" +echo " Delete after : $DAYS_TO_DELETE days" +echo "" +echo " Steps:" +echo " 1. Enable last access time tracking (required prerequisite)" +echo " 2. Create lifecycle policy with three tiers:" +echo " - Move to Cool tier after 30 days of no access" +echo " - Move to Archive after 90 days of no access" +echo " - Delete permanently after $DAYS_TO_DELETE days of no access" +echo "" +echo " NOTE: This requires StorageV2 or BlobStorage account kind." +echo " Premium and Classic accounts do not support lifecycle management." +echo "" +read -r -p "Proceed? [y/N] " CONFIRM +if [[ "$CONFIRM" != "y" && "$CONFIRM" != "Y" ]]; then + echo "Aborted. No changes were made." + exit 0 +fi + +# ── Step 1: Enable last access time tracking ───────────────────────────────── +# REQUIRED before daysAfterLastAccessTimeGreaterThan can be used in a policy. +# Without this, the Azure API accepts the policy JSON but the tier transitions +# never fire — the account stays non-compliant silently. + +echo "" +echo "[1/2] Enabling last access time tracking on: $STORAGE_ACCOUNT ..." + +az storage account blob-service-properties update \ + --account-name "$STORAGE_ACCOUNT" \ + --resource-group "$RESOURCE_GROUP" \ + --enable-last-access-tracking true + +echo " Last access tracking enabled." + +# ── Step 2: Write and apply the lifecycle policy ────────────────────────────── +# DAYS_TO_DELETE is validated as a positive integer >= 91 above. +# All variable expansions inside the heredoc are safe. + +echo "" +echo "[2/2] Applying lifecycle management policy to: $STORAGE_ACCOUNT ..." + +cat > "$POLICY_FILE" << EOF +{ + "rules": [ + { + "name": "openshield-lifecycle-rule", + "enabled": true, + "type": "Lifecycle", + "definition": { + "filters": { + "blobTypes": ["blockBlob"] + }, + "actions": { + "baseBlob": { + "tierToCool": { + "daysAfterLastAccessTimeGreaterThan": 30 + }, + "tierToArchive": { + "daysAfterLastAccessTimeGreaterThan": 90 + }, + "delete": { + "daysAfterLastAccessTimeGreaterThan": ${DAYS_TO_DELETE} + } + }, + "snapshot": { + "delete": { + "daysAfterCreationGreaterThan": ${DAYS_TO_DELETE} + } + } + } + } + } + ] +} +EOF + +az storage account management-policy create \ + --account-name "$STORAGE_ACCOUNT" \ + --resource-group "$RESOURCE_GROUP" \ + --policy "@${POLICY_FILE}" + +# Temp file removed automatically by trap on EXIT. + +# ── Confirmation ───────────────────────────────────────────────────────────── + +echo "" +echo "============================================================" +echo " Remediation complete for: $STORAGE_ACCOUNT" +echo "============================================================" +echo "" +echo " Applied:" +echo " Last access time tracking : enabled" +echo " Move to Cool after 30 days of no access" +echo " Move to Archive after 90 days of no access" +echo " Delete after $DAYS_TO_DELETE days of no access" +echo "" +echo " To verify the policy was applied:" +echo " az storage account management-policy show \\" +echo " --account-name $STORAGE_ACCOUNT \\" +echo " --resource-group $RESOURCE_GROUP" +echo "" +echo " NOTE: Adjust tier thresholds and delete day to match your" +echo " organisation's data retention and compliance policy." +echo "============================================================" diff --git a/requirements.txt b/requirements.txt index ee813479..74c911fb 100644 --- a/requirements.txt +++ b/requirements.txt @@ -14,3 +14,4 @@ psycopg2-binary==2.9.9 python-dotenv==1.0.0 pyjwt==2.8.0 requests==2.31.0 +pyyaml==6.0.1 diff --git a/scanner/azure_client.py b/scanner/azure_client.py index e7381b5d..e68c06c0 100644 --- a/scanner/azure_client.py +++ b/scanner/azure_client.py @@ -3,6 +3,7 @@ import logging from typing import Any, Dict, List, Optional +from azure.core.exceptions import HttpResponseError, ResourceNotFoundError from azure.identity import DefaultAzureCredential from azure.mgmt.authorization import AuthorizationManagementClient from azure.mgmt.compute import ComputeManagementClient @@ -59,6 +60,74 @@ def get_storage_accounts(self) -> List[Any]: logger.error("get_storage_accounts failed: %s", exc) return [] + def get_storage_lifecycle_policy( + self, resource_group: str, account_name: str + ) -> Optional[bool]: + """Check whether a storage account has a lifecycle management policy. + + Three-state return — the calling rule uses strict identity checks + (is False / is None) to distinguish these states: + + True — policy exists and contains at least one enabled rule. + False — ResourceNotFoundError: no policy configured (non-compliant). + None — any other error (permissions, network, SDK bug). + Caller must NOT create a finding — skip with a warning + to avoid false positives. + + The StorageManagementClient is created fresh here following the same + pattern as every other method in AzureClient (one client per call). + The credential is reused from self.credential so no new auth round- + trip occurs. + + Args: + resource_group: Resource group containing the storage account. + account_name: Name of the storage account. + + Returns: + Optional[bool] — True, False, or None as described above. + """ + try: + client = StorageManagementClient(self.credential, self.subscription_id) + policy = client.management_policies.get( + resource_group, account_name, "default" + ) + # A policy shell can exist with an empty rules list — + # treat that the same as no policy (non-compliant). + rules = getattr(getattr(policy, "policy", None), "rules", None) + return bool(rules) + + except ResourceNotFoundError: + # Expected path: the account genuinely has no lifecycle policy. + # This is the non-compliant condition — return False to flag it. + logger.debug( + "get_storage_lifecycle_policy(%s): ResourceNotFound — no policy", + account_name, + ) + return False + + except HttpResponseError as exc: + # 403 = service principal lacks + # Microsoft.Storage/storageAccounts/managementPolicies/read. + # Return None — cannot determine compliance, do not flag. + logger.error( + "get_storage_lifecycle_policy(%s) HTTP %s — " + "check service principal permissions: %s", + account_name, + exc.status_code, + exc, + ) + return None + + except Exception as exc: + # Unexpected failure (network, SDK bug, etc.). + # Return None — skip rather than create a false positive. + logger.error( + "get_storage_lifecycle_policy(%s) unexpected error: %s", + account_name, + exc, + ) + return None + # ------------------------------------------------------------------ # # Network # # ------------------------------------------------------------------ # @@ -83,6 +152,24 @@ def get_network_interface( logger.error("get_network_interface(%s) failed: %s", nic_name, exc) return None + def get_virtual_networks(self) -> List[Any]: + """List all virtual networks in the subscription.""" + try: + client = NetworkManagementClient(self.credential, self.subscription_id) + return list(client.virtual_networks.list_all()) + except Exception as exc: + logger.error("get_virtual_networks failed: %s", exc) + return [] + + def get_public_ip_addresses(self) -> List[Any]: + """List all public IP addresses in the subscription.""" + try: + client = NetworkManagementClient(self.credential, self.subscription_id) + return list(client.public_ip_addresses.list_all()) + except Exception as exc: + logger.error("get_public_ip_addresses failed: %s", exc) + return [] + # ------------------------------------------------------------------ # # Compute # # ------------------------------------------------------------------ # @@ -185,4 +272,4 @@ def get_conditional_access_policies(self) -> List[Any]: return response.json().get("value", []) except Exception as exc: logger.error("get_conditional_access_policies failed: %s", exc) - return [] + return [] \ No newline at end of file diff --git a/scanner/rules/az_net_003.py b/scanner/rules/az_net_003.py index 54d2ca14..a0a18e0f 100644 --- a/scanner/rules/az_net_003.py +++ b/scanner/rules/az_net_003.py @@ -9,7 +9,6 @@ CATEGORY = "Network" FRAMEWORKS = {"CIS": "9.3", "NIST": "SC-7", "ISO27001": "A.13.1.1"} DESCRIPTION = ( - DESCRIPTION = ( "A Network Security Group has an inbound rule allowing unrestricted access " "on port 443 from any source (0.0.0.0/0). While HTTPS traffic is encrypted, " "exposing port 443 to the entire internet unnecessarily increases the attack " @@ -18,7 +17,6 @@ "Review manually before remediating — do not auto-remediate without confirming " "the service is not meant to be publicly accessible." ) -) REMEDIATION = ( "Restrict the inbound rule on port 443 to known IP ranges or use an " "Application Gateway with WAF to front any public-facing HTTPS services. " diff --git a/scanner/rules/az_net_005.py b/scanner/rules/az_net_005.py index 9f6702c8..48f90bee 100644 --- a/scanner/rules/az_net_005.py +++ b/scanner/rules/az_net_005.py @@ -27,20 +27,7 @@ def scan(azure_client: Any, subscription_id: str) -> List[Dict[str, Any]]: """Detect virtual networks without DDoS protection enabled.""" findings: List[Dict[str, Any]] = [] - try: - # NOTE: This rule creates a NetworkManagementClient directly rather than - # going through azure_client. A get_virtual_networks() method should be - # added to AzureClient in a follow-up PR for consistency. - from azure.mgmt.network import NetworkManagementClient - client = NetworkManagementClient( - azure_client.credential, azure_client.subscription_id - ) - vnets = list(client.virtual_networks.list_all()) - except Exception as exc: - logger.error("Failed to list virtual networks: %s", exc) - return findings - - for vnet in vnets: + for vnet in azure_client.get_virtual_networks(): ddos = getattr(vnet, "ddos_protection_plan", None) enable_ddos = getattr(vnet, "enable_ddos_protection", False) if not ddos and not enable_ddos: diff --git a/scanner/rules/az_net_006.py b/scanner/rules/az_net_006.py index 180f49b9..26923d79 100644 --- a/scanner/rules/az_net_006.py +++ b/scanner/rules/az_net_006.py @@ -28,20 +28,7 @@ def scan(azure_client: Any, subscription_id: str) -> List[Dict[str, Any]]: """Detect public IP addresses not associated with any resource.""" findings: List[Dict[str, Any]] = [] - try: - # NOTE: This rule creates a NetworkManagementClient directly rather than - # going through azure_client. A get_public_ip_addresses() method should be - # added to AzureClient in a follow-up PR for consistency. - from azure.mgmt.network import NetworkManagementClient - client = NetworkManagementClient( - azure_client.credential, azure_client.subscription_id - ) - public_ips = list(client.public_ip_addresses.list_all()) - except Exception as exc: - logger.error("Failed to list public IP addresses: %s", exc) - return findings - - for pip in public_ips: + for pip in azure_client.get_public_ip_addresses(): ip_config = getattr(pip, "ip_configuration", None) nat_gateway = getattr(pip, "nat_gateway", None) if not ip_config and not nat_gateway: diff --git a/scanner/rules/az_stor_003.py b/scanner/rules/az_stor_003.py new file mode 100644 index 00000000..7758b45a --- /dev/null +++ b/scanner/rules/az_stor_003.py @@ -0,0 +1,117 @@ +"""AZ-STOR-003: Storage account has no lifecycle management policy configured.""" + +import logging +from typing import Any, Dict, List, Optional + +logger = logging.getLogger(__name__) + +# subscription_id is received by scan() and passed to AzureClient methods +# that need explicit scope. It is not read from the environment here — +# the engine always passes it as a parameter. Never read os.environ directly. + +# ── Required module-level constants ───────────────────────────────────────── + +RULE_ID = "AZ-STOR-003" +RULE_NAME = "Storage Account Has No Lifecycle Management Policy" +SEVERITY = "MEDIUM" +CATEGORY = "Storage" +FRAMEWORKS = { + "CIS": "3.7", + "NIST": "PR.DS-3", + "ISO27001": "A.8.3.1", +} +DESCRIPTION = ( + "The storage account has no lifecycle management policy configured. " + "Without a lifecycle policy, blobs accumulate indefinitely — old data " + "that is no longer needed remains accessible, increasing storage costs " + "and the attack surface. A compromised account exposes all historical " + "data with no automatic expiry or tiering in place." +) +REMEDIATION = ( + "Create a lifecycle management policy on the storage account that " + "transitions blobs to cooler tiers (Cool, Archive) after a defined " + "number of days, and deletes blobs that exceed the organisation's " + "maximum retention period. Navigate to: Storage Account > " + "Data management > Lifecycle management > Add a rule." +) +PLAYBOOK = "playbooks/cli/fix_az_stor_003.sh" + + +# ── Required scan function ─────────────────────────────────────────────────── + +def scan(azure_client: Any, subscription_id: str) -> List[Dict[str, Any]]: + """Detect storage accounts with no lifecycle management policy. + + The Azure Storage Management SDK exposes lifecycle policies via + ``management_policies.get(resource_group, account_name)``. + A ResourceNotFound (404) response means no policy exists — this is + the condition we flag as MEDIUM severity. + + Three-state return from get_storage_lifecycle_policy(): + True — policy exists and has rules → skip (compliant) + False — no policy exists → create finding + None — permissions error or unexpected failure → skip with warning + to avoid false positives + + Args: + azure_client: An AzureClient instance with all SDK clients + pre-configured. + subscription_id: The Azure subscription ID being scanned. + + Returns: + A list of finding dicts — one per storage account that has no + lifecycle policy. Accounts that could not be checked are skipped + and logged as warnings. + """ + findings: List[Dict[str, Any]] = [] + + for account in azure_client.get_storage_accounts(): + resource_id = getattr(account, "id", "") + account_name = getattr(account, "name", "") + location = getattr(account, "location", "") + + if not resource_id or not account_name: + continue + + parsed = azure_client.parse_resource_id(resource_id) + resource_group = parsed.get("resource_group", "") + if not resource_group: + continue + + # True = compliant, False = no policy, None = could not determine + policy_status: Optional[bool] = azure_client.get_storage_lifecycle_policy( + resource_group, account_name + ) + + if policy_status is None: + # Permissions error or unexpected SDK failure. + # Skip rather than flag — never create false positives. + logger.warning( + "AZ-STOR-003: Could not determine lifecycle policy for %s " + "— skipping. Ensure the service principal has " + "Microsoft.Storage/storageAccounts/managementPolicies/read " + "permission.", + account_name, + ) + continue + + if policy_status is False: + findings.append({ + "rule_id": RULE_ID, + "rule_name": RULE_NAME, + "severity": SEVERITY, + "category": CATEGORY, + "resource_id": resource_id, + "resource_name": account_name, + "resource_type": "Microsoft.Storage/storageAccounts", + "description": DESCRIPTION, + "remediation": REMEDIATION, + "playbook": PLAYBOOK, + "frameworks": FRAMEWORKS, + "metadata": { + "resource_group": resource_group, + "location": location, + }, + }) + + return findings \ No newline at end of file