From 4affc94298b47c08f7d99cead8db0b2d425703ee Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Mon, 28 Sep 2026 01:30:08 +0200 Subject: [PATCH] sec(iac/azure): AKS module defaults to a private cluster with Entra-bound admin access azurerm_kubernetes_cluster.main set neither private_cluster_enabled nor api_server_access_profile, so the API server was reachable from 0.0.0.0/0. local_account_disabled was left at its false default, so the static cluster-admin client certificate (`az aks get-credentials --admin`) stayed usable. And role_based_access_control_enabled = true only turns on Kubernetes-native RBAC; the comment on that line claimed "RBAC and Azure AD integration", but no azure_active_directory_role_based_access_control block existed anywhere in the module. Verified via terraform/environments/azure/variables.tf that compute_platform defaults to "container-apps" (AKS is opt-in), and that all three checked-in tfvars use container-apps, so no currently-deployed environment uses this module; the fix hardens defaults rather than touching a live cluster. Fix (terraform/modules/compute/azure/aks/{main,variables}.tf): - private_cluster_enabled now defaults to true; disabling it requires a non-empty authorized_ip_ranges allowlist, and authorized_ip_ranges itself rejects "0.0.0.0/0". - local_account_disabled now defaults to true. - Added azure_active_directory_role_based_access_control with azure_rbac_enabled = true and an optional admin_group_object_ids input. - Fixed the misleading comment on role_based_access_control_enabled. Wired the new knobs through terraform/environments/azure/{variables,compute}.tf as aks_private_cluster_enabled / aks_authorized_ip_ranges / aks_local_account_disabled / aks_admin_group_object_ids. Review round 2 fixes: - BLOCKER: added managed = true to the azure_active_directory_role_based_access_control block. On azurerm ~> 3.0 (this module's pin, locked at 3.117.1), managed defaults to false, which selects the deprecated legacy AAD integration mode and makes apply fail demanding client_app_id/server_app_id/server_app_secret even with azure_rbac_enabled set -- terraform validate passes (attribute presence isn't checked there), but apply does not. managed = true selects the AKS-managed Entra integration that azure_rbac_enabled actually applies to; implied (no longer a real toggle) on azurerm 4.x. - Documented the operational consequence of both hardened defaults in their variable descriptions: with private_cluster_enabled = true (default), a standard GitHub-hosted runner (ubuntu-latest) cannot reach the API server to apply deploy_kubernetes_resources = true without VNet connectivity; with local_account_disabled = true (default), the kubernetes/helm providers' client_certificate/ client_key auth (as environments/azure/main.tf currently configures them) stops working, since AKS no longer issues that certificate -- exec-based auth (kubelogin) is needed instead. - Filed #372 (p2, triaged) for a separate, pre-existing bug surfaced while documenting the above: environments/azure/main.tf:64 reads module.compute_aks[0].kube_config.host, but the module's kube_config output is kube_config_raw (a YAML string, not an object) -- try() masks the resulting type error to "https://localhost", so the AKS path's kubernetes/helm provider wiring has never actually worked. Not fixed here: out of scope for this PR (a distinct, pre-existing defect, not introduced or worsened by this change) and needs its own design decision (swap to the module's real host/client_certificate outputs, then move to kubelogin exec auth so it survives local_account_disabled = true). Regression test: terraform/modules/compute/azure/aks_hardening_test.go asserts private_cluster_enabled/local_account_disabled are wired from variables, the AAD RBAC block exists with azure_rbac_enabled = true AND managed = true, both variables default to true, and authorized_ip_ranges validates against "0.0.0.0/0". Verified with GOTOOLCHAIN=go1.26.6 that it fails against the actual current origin/main source (git show origin/main:.../aks/main.tf swapped in, test left in place) with all four assertions failing, and passes with this fix restored. No ci-cd-permissions bootstrap re-apply is required; this only touches the AKS compute module and its (opt-in) environment wiring. Closes #121 Co-Authored-By: claude-flow --- terraform/environments/azure/compute.tf | 6 ++ terraform/environments/azure/variables.tf | 24 ++++++++ terraform/modules/compute/azure/aks/main.tf | 43 +++++++++++++- .../modules/compute/azure/aks/variables.tf | 34 +++++++++++ .../compute/azure/aks_hardening_test.go | 56 +++++++++++++++++++ 5 files changed, 162 insertions(+), 1 deletion(-) create mode 100644 terraform/modules/compute/azure/aks_hardening_test.go diff --git a/terraform/environments/azure/compute.tf b/terraform/environments/azure/compute.tf index 042024b58..d8fa403d0 100644 --- a/terraform/environments/azure/compute.tf +++ b/terraform/environments/azure/compute.tf @@ -186,6 +186,12 @@ module "compute_aks" { enable_azure_policy = var.aks_enable_azure_policy enable_log_analytics = var.aks_enable_log_analytics + # API server exposure and admin access (#121) + private_cluster_enabled = var.aks_private_cluster_enabled + authorized_ip_ranges = var.aks_authorized_ip_ranges + local_account_disabled = var.aks_local_account_disabled + admin_group_object_ids = var.aks_admin_group_object_ids + tags = local.common_tags depends_on = [module.networking, module.database, module.secrets] diff --git a/terraform/environments/azure/variables.tf b/terraform/environments/azure/variables.tf index f8574bd81..522b5d762 100644 --- a/terraform/environments/azure/variables.tf +++ b/terraform/environments/azure/variables.tf @@ -467,6 +467,30 @@ variable "aks_enable_log_analytics" { default = true } +variable "aks_private_cluster_enabled" { + description = "Disable the AKS API server's public endpoint. Defaults to true; set false only alongside a real, non-empty aks_authorized_ip_ranges allowlist." + type = bool + default = true +} + +variable "aks_authorized_ip_ranges" { + description = "CIDR allowlist for the public API server when aks_private_cluster_enabled=false. Must not include 0.0.0.0/0." + type = list(string) + default = [] +} + +variable "aks_local_account_disabled" { + description = "Disable AKS's static cluster-admin client certificate in favor of Azure RBAC / Entra-bound access. Defaults to true." + type = bool + default = true +} + +variable "aks_admin_group_object_ids" { + description = "Entra ID (Azure AD) group object IDs granted AKS admin access. Optional: Azure RBAC role assignments against the cluster resource work without this." + type = list(string) + default = [] +} + # ============================================== # Frontend (CDN) Configuration # ============================================== diff --git a/terraform/modules/compute/azure/aks/main.tf b/terraform/modules/compute/azure/aks/main.tf index c186309b7..eb743c828 100644 --- a/terraform/modules/compute/azure/aks/main.tf +++ b/terraform/modules/compute/azure/aks/main.tf @@ -83,9 +83,50 @@ resource "azurerm_kubernetes_cluster" "main" { azure_policy_enabled = var.enable_azure_policy - # RBAC and Azure AD integration + # Kubernetes RBAC (authorization within the cluster). Real identity binding + # is the azure_active_directory_role_based_access_control block below -- + # this flag alone does not add Entra integration. role_based_access_control_enabled = true + # Defaults to a private API server (no public endpoint). Set + # var.private_cluster_enabled = false only when var.authorized_ip_ranges is + # a real, non-empty allowlist (validated below) -- never leave the API + # server open to 0.0.0.0/0. + private_cluster_enabled = var.private_cluster_enabled + + dynamic "api_server_access_profile" { + for_each = var.private_cluster_enabled ? [] : [1] + content { + authorized_ip_ranges = var.authorized_ip_ranges + } + } + + # Disables the static cluster-admin client certificate obtained via + # `az aks get-credentials --admin`, which never expires, isn't tied to any + # Entra identity, and doesn't appear in Entra sign-in logs. With this + # disabled, admin access is granted through Azure RBAC role assignments + # (e.g. "Azure Kubernetes Service RBAC Cluster Admin") scoped to this + # cluster resource, or through var.admin_group_object_ids below -- both + # revocable per-identity and auditable. + local_account_disabled = var.local_account_disabled + + # Azure RBAC for Kubernetes Authorization: Kubernetes access control is + # driven by Azure RBAC role assignments against this cluster resource + # (or the admin group below) rather than a separate, unauditable + # in-cluster admin credential. + azure_active_directory_role_based_access_control { + # Required on azurerm ~> 3.0 (this module's pin): `managed` defaults to + # false, which selects the deprecated legacy AAD integration and makes + # the provider demand client_app_id/server_app_id/server_app_secret -- + # `terraform validate` passes without them, but `apply` fails. `managed + # = true` selects the current AKS-managed Entra integration that + # azure_rbac_enabled actually applies to. Implied (no longer a real + # toggle) on azurerm 4.x. + managed = true + azure_rbac_enabled = true + admin_group_object_ids = var.admin_group_object_ids + } + tags = local.common_tags } diff --git a/terraform/modules/compute/azure/aks/variables.tf b/terraform/modules/compute/azure/aks/variables.tf index 1266a85d9..9e8ec88cc 100644 --- a/terraform/modules/compute/azure/aks/variables.tf +++ b/terraform/modules/compute/azure/aks/variables.tf @@ -174,3 +174,37 @@ variable "tags" { type = map(string) default = {} } + +variable "private_cluster_enabled" { + description = "Disable the AKS API server's public endpoint. Defaults to true (private cluster). Set to false only alongside a real, non-empty var.authorized_ip_ranges allowlist -- an operator who disables this without one gets a plan-time error, not a public-by-accident cluster. CONSEQUENCE: with this true (the default), the API server is reachable only from inside the VNet, so terraform apply itself cannot reach it from a standard GitHub-hosted runner (ubuntu-latest) unless that runner has VNet connectivity (self-hosted runner, VPN, or a peered network) -- and the kubernetes/helm providers this module's deploy_kubernetes_resources=true path configures (see environments/azure/main.tf) will time out trying to reach a private endpoint from a non-VNet runner. Only set var.deploy_kubernetes_resources=true from a VNet-reachable apply context." + type = bool + default = true + + validation { + condition = var.private_cluster_enabled || length(var.authorized_ip_ranges) > 0 + error_message = "private_cluster_enabled=false requires a non-empty authorized_ip_ranges allowlist; otherwise the AKS API server is reachable from 0.0.0.0/0." + } +} + +variable "authorized_ip_ranges" { + description = "CIDR allowlist for the public API server when private_cluster_enabled=false (e.g. CI egress ranges plus an operator bastion). Ignored when private_cluster_enabled=true. Must not include 0.0.0.0/0." + type = list(string) + default = [] + + validation { + condition = !contains(var.authorized_ip_ranges, "0.0.0.0/0") + error_message = "authorized_ip_ranges must not contain 0.0.0.0/0; that reopens the public endpoint to the whole internet, exactly what private_cluster_enabled/authorized_ip_ranges exists to prevent." + } +} + +variable "local_account_disabled" { + description = "Disable AKS's static cluster-admin client certificate (obtained via `az aks get-credentials --admin`), which never expires and isn't tied to any Entra identity. Defaults to true; admin access is instead granted via Azure RBAC role assignments (e.g. \"Azure Kubernetes Service RBAC Cluster Admin\") or var.admin_group_object_ids. CONSEQUENCE: with this true (the default), the Kubernetes provider's `client_certificate`/`client_key` auth (as environments/azure/main.tf currently configures it) stops working, because AKS no longer issues that certificate -- terraform's kubernetes/helm providers need `exec`-based auth (e.g. kubelogin, or an Azure RBAC-authorized service principal token) instead. Do not set var.deploy_kubernetes_resources=true with the default (true) until the provider block is switched to exec auth." + type = bool + default = true +} + +variable "admin_group_object_ids" { + description = "Entra ID (Azure AD) group object IDs granted AKS admin access via azure_active_directory_role_based_access_control. Optional: with azure_rbac_enabled=true (always set by this module), admin access can also be granted purely through Azure RBAC role assignments against the cluster resource, so an empty list is a valid, non-locking-out default." + type = list(string) + default = [] +} diff --git a/terraform/modules/compute/azure/aks_hardening_test.go b/terraform/modules/compute/azure/aks_hardening_test.go new file mode 100644 index 000000000..dd914efdf --- /dev/null +++ b/terraform/modules/compute/azure/aks_hardening_test.go @@ -0,0 +1,56 @@ +// Regression test for issue #121: the AKS module left the API server public +// (no private_cluster_enabled / api_server_access_profile), left the static, +// non-expiring, non-Entra-bound local admin credential enabled (no +// local_account_disabled), and set role_based_access_control_enabled = true +// (Kubernetes RBAC only) with a comment claiming "RBAC and Azure AD +// integration" while no azure_active_directory_role_based_access_control +// block existed anywhere in the module. +package azure_test + +import ( + "os" + "regexp" + "testing" +) + +func TestAKSClusterIsPrivateWithEntraRBAC(t *testing.T) { + data, err := os.ReadFile("aks/main.tf") + if err != nil { + t.Fatalf("reading aks/main.tf: %v", err) + } + content := string(data) + + if !regexp.MustCompile(`\bprivate_cluster_enabled\s*=\s*var\.private_cluster_enabled\b`).MatchString(content) { + t.Error(`azurerm_kubernetes_cluster.main must set private_cluster_enabled from a module variable (issue #121); the API server was reachable from 0.0.0.0/0 with neither private_cluster_enabled nor api_server_access_profile set`) + } + + if !regexp.MustCompile(`\blocal_account_disabled\s*=\s*var\.local_account_disabled\b`).MatchString(content) { + t.Error(`azurerm_kubernetes_cluster.main must set local_account_disabled from a module variable (issue #121); the static, non-expiring, non-Entra-bound cluster-admin credential was left enabled by default`) + } + + if !regexp.MustCompile(`azure_active_directory_role_based_access_control\s*\{[^}]*azure_rbac_enabled\s*=\s*true`).MatchString(content) { + t.Error(`azurerm_kubernetes_cluster.main must have an azure_active_directory_role_based_access_control block with azure_rbac_enabled = true (issue #121); role_based_access_control_enabled = true alone is Kubernetes-native RBAC only and binds no Entra identity`) + } + + if !regexp.MustCompile(`azure_active_directory_role_based_access_control\s*\{[^}]*managed\s*=\s*true`).MatchString(content) { + t.Error(`azurerm_kubernetes_cluster.main's azure_active_directory_role_based_access_control block must set managed = true; on azurerm ~> 3.0, managed defaults to false (the deprecated legacy AAD integration), which makes apply fail demanding client_app_id/server_app_id/server_app_secret even though azure_rbac_enabled is set -- validate passes but apply does not`) + } + + varsData, err := os.ReadFile("aks/variables.tf") + if err != nil { + t.Fatalf("reading aks/variables.tf: %v", err) + } + vars := string(varsData) + + if !regexp.MustCompile(`variable\s+"private_cluster_enabled"\s*\{[^}]*default\s*=\s*true`).MatchString(vars) { + t.Error(`var.private_cluster_enabled must default to true, so a caller who does not override it gets a private cluster`) + } + + if !regexp.MustCompile(`variable\s+"local_account_disabled"\s*\{[^}]*default\s*=\s*true`).MatchString(vars) { + t.Error(`var.local_account_disabled must default to true, so a caller who does not override it has the local admin credential disabled`) + } + + if !regexp.MustCompile(`contains\(var\.authorized_ip_ranges,\s*"0\.0\.0\.0/0"\)`).MatchString(vars) { + t.Error(`var.authorized_ip_ranges must validate against "0.0.0.0/0", so disabling the private cluster can't silently reopen the API server to the whole internet`) + } +}