Add enterprise AKS and workload security controls - #293
Conversation
Signed-off-by: ritiksah141 <ritiksah141@gmail.com>
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.OpenSSF Scorecard
Scanned Files
|
There was a problem hiding this comment.
Good structure and solid failure-path coverage. Two blockers and two nits before merge.
1. policy_required set too broad (blocker)
scanner/rules/_aks_enterprise_common.py:197
Eight controls (privileged, host_network, host_pid, host_ipc, host_path, secret_protection, network_policy_namespaces, latest_image) return [] when OPENSHIELD_AKS_SECURITY_POLICY is not set, even though none of them need org-specific values. Each already handles policy=None safely downstream. Only four controls genuinely need a configured policy: api_restrictions, cluster_admin, untrusted_registry, and mutable_image. Remove the other eight from the set so workload rules run without a policy file.
2. Docker Hub short-form images cause false-positive AZ-AKS-019 findings (blocker)
scanner/rules/_aks_enterprise_common.py:1581
Images without a registry hostname (nginx, redis:7) fail the trusted-registry check even when docker.io/ is in trusted_registry_prefixes, because nginx.startswith(docker.io/) is False. Normalize short-form names before the prefix match.
3. Approved IP list rebuilt per cluster (efficiency)
scanner/rules/_aks_enterprise_common.py:1400
approved = [ipaddress.ip_network(item) for item in policy.approved_authorized_ip_ranges] is inside the per-cluster loop. Hoist it above for evidence in evidence_items.
4. Stale rule count in docs/learn/index.html (trivial)
Heading says 95 Azure security rules but the paragraph body still reads 80 dynamic rules.
Signed-off-by: ritiksah141 <ritiksah141@gmail.com>
|
Addressed the requested code changes and added regression coverage. I left the docs/learn/index.html rule count unchanged because the assignee handling docs/learn will update it. |
cool make sence now its good to go for me approving it |
There was a problem hiding this comment.
Thanks for taking on this AKS security pack. There is a lot of good structure here, especially around preserving unknown and partial evidence. Before we merge it, I need us to close three gaps that could produce incorrect results in a real multi-cluster environment:
-
Cluster evidence can be attached to the wrong AKS resource. The collector selects kubeconfig contexts using only
cluster.name. AKS names are scoped to a resource group, so two clusters in one subscription can share a name, and kubeconfig context names can also be customized. In that situation, both ARM resources may be evaluated through the same Kubernetes context. Please introduce an explicit, unambiguous resource-ID-to-context mapping, validate it, and treat missing or ambiguous mappings as UNKNOWN. Add a regression test with duplicate cluster names in different resource groups. -
The cluster-admin allowlist loses the ServiceAccount namespace. ServiceAccounts are namespace-scoped, but AZ-AKS-018 compares only
Kind:name. AllowingServiceAccount:builderwould therefore allow abuilderaccount from every namespace. Please use a namespace-aware identity such asServiceAccount:<namespace>:<name>, reject ambiguous policy entries, and test two same-named ServiceAccounts in different namespaces. Group and User identities can keep the appropriate non-namespaced form. -
Projected Secret volumes are not collected.
_workload()checks ordinary Secret volumes and environment references, but missesvolume.projected.sources[].secret.name. A workload using only a projected Secret can therefore be treated as not applicable when KMS is disabled. Please collect those references and add a regression test.
The GitHub checks are green. My focused local run could not collect because this checkout is missing azure.mgmt.authorization; that environment limitation is separate from the production evidence cases above.
Please let me know once these cases are covered and I will re-review promptly.
Signed-off-by: ritiksah141 <ritiksah141@gmail.com>
|
@m-khan-97, Addressed the tenant-scale review blockers in 9d6acb2. AKS contexts now use a validated resource-ID mapping, ServiceAccount allowlists are namespace-scoped, and projected Secret volumes are collected. Added regression coverage for each case. All local tests and vulnerability checks pass. |
|
@Vishnu2707, @m-khan-97, addressed the trusted-registry boundary issue in Registry trust now respects repository boundaries, so Verification: 98 AKS enterprise rule tests passed, with Ruff and formatting checks clean. I agree that the remaining items are project-wide dependencies rather than PR-specific defects. I’ll keep #293 open and rebase it after #263 establishes explicit UNKNOWN semantics and #308 lands the CRITICAL severity scoring contract. So, for now this PR is in draft and will be updated when its co-revelant parts land on the codebase. |
Signed-off-by: ritiksah141 <ritiksah141@gmail.com>
fde49be to
98ff7da
Compare
parthrohit22
left a comment
There was a problem hiding this comment.
Deep-reviewed _aks_enterprise_common.py (the shared evaluator all 15 new rules dispatch into), aks_security.py's collector, the azure_client.py diff, and traced logic through az_aks_007/009/011/013/018/019/020/021. This is a strong PR overall: the indeterminate-handling invariant is correctly applied across every control branch I walked (missing/None evidence, malformed IP ranges, malformed cluster-admin subjects, UNKNOWN status, missing/malformed policy all skip rather than assert compliance or a confirmed HIGH), the trusted-registry-prefix boundary bug this codebase apparently hit before (per the commit history) is correctly avoided here, RULE_IDs don't collide with anything existing, all 15 playbooks exist/pass bash -n and route through a shared confirmation-gated script that only auto-applies for the 4 rules with unambiguous low-blast-radius remediation, and framework mappings are complete and valid across all four compliance JSONs.
One real bug though: excluded_namespaces from the operator's AKS security policy is silently ignored by 8 of the 15 new rules, contradicting this PR's own documented safeguard. Verified with a standalone repro, not just by reading: built an AksClusterEvidence with excluded_namespaces: ["istio-system"] and a privileged workload with no NetworkPolicy in that namespace - az_aks_009.scan() and az_aks_013.scan() both still emit findings for it despite the explicit exclusion. Left an inline comment with the specific lines and root cause.
Why this got past the existing tests: every fixture that sets excluded_namespaces uses ["kube-system"], which is already unconditionally filtered at the collector level regardless of policy (SYSTEM_NAMESPACES) - so those tests pass whether or not the policy-based exclusion path actually runs. There's no test anywhere with a non-system excluded namespace for any of the 8 affected rules.
Concrete impact: an operator adds a service-mesh control-plane namespace (istio-system, linkerd) or a vendor-managed namespace to excluded_namespaces, per docs/aks-security-rules.md's own description of that field - findings keep firing on it anyway across network_policy_namespaces, secret_protection, privileged, host_network/pid/ipc/path, and latest_image. It's noise rather than a missed real risk (doesn't violate the "ambiguous evidence never confirmed HIGH" invariant), but it's broad and it's a documented claim that doesn't hold, which will train operators to distrust the tool's output on exactly the namespaces they explicitly told it to ignore.
Two non-blocking notes:
_normalize_image_registrymaps bare refs likenginx:latesttodocker.io/nginx:latest, not Docker's canonicaldocker.io/library/nginx:latest. Internally consistent (tested), not a bug as long as operators writedocker.io/in their policy - but a policy author who writesdocker.io/library/per Docker's own convention would be surprised when official-image workloads keep getting flagged. Worth a one-line callout in the docs or the example policy comment._collect_kubernetes's six workload-kind calls per namespace (Deployment/StatefulSet/DaemonSet/Job/CronJob/Pod) share one try/except, so an early failure (e.g. RBAC denies listing DaemonSets) aborts the remaining calls for that namespace and marks it wholly partial, even if later calls would have succeeded. Conservative in the safe direction (under-reports, never a false HIGH), so not flagging as a bug - just noting it's untested at the collector level and slightly reduces detection completeness under partial RBAC.
Ran the full suite after installing the kubernetes==36.0.0 dependency this PR adds (missing from this sandbox's .venv, an environment gap worth flagging in case CI needs it fresh too): 822 passed, 3 skipped, 0 failed. Test coverage for the new rules is substantive - compliant/noncompliant paths, empty inventory, inventory failure, unreachable cluster, malformed policy, missing policy split by policy-required-vs-independent rule sets, partial collection never inferring a missing NetworkPolicy, ServiceAccount-scoped cluster-admin allowlisting, IP-supernet containment, and registry prefix-boundary crossing are all exercised.
| if not evidence_items: | ||
| logger.info("%s: no AKS clusters exist; result is NOT_APPLICABLE", rule_id) | ||
| return [] | ||
| policy_required = control in { |
There was a problem hiding this comment.
policy_required only covers api_restrictions/cluster_admin/untrusted_registry/mutable_image, so policy stays None for every other control - network_policy_namespaces, secret_protection, privileged, host_network/host_pid/host_ipc/host_path, and latest_image. That means _workloads(evidence, policy) at line ~189 (if policy and namespace.lower() in policy.excluded_namespaces) and the network_policy_namespaces branch's excluded = policy.excluded_namespaces if policy else frozenset() around line 387 can never apply the operator's configured namespace exclusions for those 8 rules, even though docs/aks-security-rules.md documents excluded_namespaces as a general policy field with no such caveat, and the PR description states "System and policy-excluded namespaces are omitted from workload and namespace policy findings" without qualification.
Suggest loading the policy for namespace-exclusion purposes independent of policy_required - e.g. always call policy_from_env(rule_id) and only gate the stricter "policy is mandatory to run this control at all" behavior behind policy_required, so namespace exclusion applies uniformly regardless of which control is running.
|
@ritiksah141, Parth's namespace-exclusion finding is valid. Please update the shared evaluator so OPENSHIELD_AKS_SECURITY_POLICY is loaded when present for every control, while only the four policy-required controls fail closed when it is absent or invalid. That allows excluded_namespaces to apply consistently without making the policy-independent rules require configuration.\n\nPlease add regression coverage with a non-system namespace such as istio-system across the affected paths, including AZ-AKS-009 and at least the shared workload path used by AZ-AKS-011/013-017/020. Keep #293 draft while this is addressed and while #263/#308 remain dependencies. |
Summary
Implements the complete enterprise AKS and container workload security backlog from issue #255 with fifteen evidence-rich controls covering API exposure, Kubernetes network policy, Defender for Containers, secret protection, privileged and host access, cluster administration, and image supply chain policy.
What changed
AZ-AKS-007to require a private API server or organization-approved authorized IP ranges.AZ-AKS-008to require Azure, Calico, or Cilium network policy enforcement.AZ-AKS-009to detect eligible namespaces without a NetworkPolicy.AZ-AKS-010to require Microsoft Defender for Containers.AZ-AKS-011to detect native Kubernetes Secret references without Azure Key Vault KMS protection.AZ-AKS-012to require automatic rotation when the Key Vault Secrets Store CSI provider is enabled.AZ-AKS-013throughAZ-AKS-017as separate controls for privileged containers, hostNetwork, hostPID, hostIPC, and hostPath volumes.AZ-AKS-018to detect cluster-admin bindings outside the approved subject allowlist.AZ-AKS-019throughAZ-AKS-021as separate controls for untrusted registries, latest or implicit latest tags, and mutable non-digest image references.OPENSHIELD_AKS_SECURITY_POLICYand Kubernetes access throughOPENSHIELD_AKS_KUBECONFIG.N/A-*CIS decisions where no direct benchmark mapping exists./website.False-finding safeguards
Verification
Type of change
Testing
Checklist
Signed-off-bytrailerAZ-AKS-007throughAZ-AKS-021TBD-*framework mappings were introduceddevRelated issue
Closes #255
Files to review
scanner/aks_security.pyandscanner/azure_client.py- failure-aware ARM, Defender, and Kubernetes evidence collection.scanner/rules/_aks_enterprise_common.pyandscanner/rules/az_aks_007.pythroughscanner/rules/az_aks_021.py- strict policy evaluation and the fifteen independent controls.tests/test_aks_security_collector.py,tests/test_rules_aks_enterprise.py, andtests/test_azure_client_aks.py- positive, secure, empty, malformed, partial, unsupported, and failure coverage.playbooks/cli/fix_az_aks_007.shthroughplaybooks/cli/fix_az_aks_021.sh- individual review-gated remediation entry points.compliance/frameworks/*.json- verified CIS, NIST CSF, ISO 27001, and SOC 2 mappings.config/aks-security-policy.example.json,docs/aks-security-rules.md,docs/rules-reference.md, andCHANGELOG.md- policy schema, behavior, permissions, limitations, and release documentation.