From f45fc79b21d7be3ba8dd487ba5cc1851d22f49cf Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Mon, 28 Sep 2026 00:27:14 +0200 Subject: [PATCH] sec(iac/build): close the shell-injection surface on docker buildx build Two issues on the same local-exec provisioner in terraform/modules/build: 1. var.extra_build_args was interpolated as literal ${var.extra_build_args} text into the heredoc, so its value became shell source at apply time: unquoted ;, backticks, or $() in that value would execute with the CI deploy identity attached. 2. registry_login_command (documented example: a pipe into `docker login`) was undocumented on whether it may carry a static credential, which would then render in full in terraform plan output and CI job logs. Fix: - extra_build_args now flows through the local-exec provisioner's environment map (EXTRA_BUILD_ARGS) instead of being interpolated into the heredoc, so its value is never re-parsed as shell source; unquoted $EXTRA_BUILD_ARGS still preserves the original whitespace-split, multi-flag behavior and the original empty-default no-op. - registry_login_command's description now states the contract explicitly: it must be an identity-based login (the CLI resolves/mints the credential itself, e.g. az acr login, aws ecr get-login-password piped into docker login, gcloud auth configure-docker) and must never embed a static, long-lived credential literal. - Quoted the remaining heredoc interpolations that are safe to quote as single values (image_uri, git_commit, timestamp build args). Earlier revision of this PR also marked both variables sensitive = true. Reverted: Terraform suppresses a resource's ENTIRE local-exec output when any of its config contains a sensitive value ("(output suppressed due to sensitive value in config)"), verified locally on Terraform 1.14.7 against a minimal repro. That means a failing build or registry login would show nothing in CI, and deploy-aws-lambda.yml / deploy-azure.yml / deploy-gcp.yml tee and grep exactly this log to detect build failures. After PR #5 (ACR now authenticates via `az acr login`, an Entra identity, not the registry admin password) no caller in any of the three environments passes a static credential through registry_login_command, and extra_build_args is not a secret, so sensitive = true was blocking diagnostics without protecting anything live. The variable's description instead documents the "identity-based only, never a static credential" contract, and the injection fix (environment-map pass-through + quoting) holds without marking anything sensitive. registry_login_command itself still has to be interpolated as literal shell source, since its contract requires it to run as a (possibly piped) shell command; --network=host on the same local-exec is filed and fixed separately as #122. Regression test: no automated Terraform test exercises the rendered local-exec script. Verified manually by rendering the exact heredoc shape with bash -c / sh -c (the interpreters local-exec actually uses): the empty-default case still elides the argument (argc=2, `--push .`, unchanged from the pre-fix unquoted-and-empty case; quoting "${var.extra_build_args}" directly, the naive fix, would instead produce argc=3 with a stray empty-string arg that docker buildx build would reject), a multi-flag value still splits into separate argv entries, and an injected `; touch /tmp/x` value passed via the environment comes through as a single inert argv word rather than executing. Also verified with a standalone terraform_data/local-exec repro that removing sensitive = true restores visible provisioner output (present without it, "(output suppressed due to sensitive value in config)" with it). Closes #127 Co-Authored-By: claude-flow --- terraform/environments/azure/build.tf | 4 +++- terraform/modules/build/main.tf | 23 ++++++++++++++++++----- terraform/modules/build/variables.tf | 4 ++-- 3 files changed, 23 insertions(+), 8 deletions(-) diff --git a/terraform/environments/azure/build.tf b/terraform/environments/azure/build.tf index ad237360..4e9ef9b7 100644 --- a/terraform/environments/azure/build.tf +++ b/terraform/environments/azure/build.tf @@ -21,7 +21,9 @@ module "build" { # platform not set — auto-detected from builder host (Container Apps and AKS support arm64 and amd64) # Registry login with the caller's Entra identity (deploy SP in CI, az login - # locally); the principal needs AcrPush on the registry. + # locally); the principal needs AcrPush on the registry. Identity-based, so + # it carries no static credential, matching the module's + # registry_login_command contract (see its variable description). registry_login_command = "az acr login --name ${azurerm_container_registry.main.name}" # Build options diff --git a/terraform/modules/build/main.tf b/terraform/modules/build/main.tf index 4e93dd6a..c65a9c5b 100644 --- a/terraform/modules/build/main.tf +++ b/terraform/modules/build/main.tf @@ -68,7 +68,20 @@ resource "terraform_data" "docker_build" { provisioner "local-exec" { working_dir = var.source_path - command = <<-EOT + # extra_build_args is passed through the environment, not interpolated into + # the script text below, so shell metacharacters in its value (;, `, $()) + # are never parsed as script syntax. registry_login_command still has to be + # interpolated as literal shell source (it is documented to be a full + # command, e.g. a pipe into `docker login`), so it stays a trusted, + # identity-based-only input (see its variable description) rather than a + # secret: neither variable is marked sensitive, because that would + # suppress this resource's entire local-exec output (the build/push log + # deploy-*.yml workflows tee and grep to detect failures), not just the + # two variables' own values. + environment = { + EXTRA_BUILD_ARGS = var.extra_build_args + } + command = <<-EOT set -e echo "Logging in to registry..." ${var.registry_login_command} @@ -105,11 +118,11 @@ resource "terraform_data" "docker_build" { $PLATFORM_ARG \ --provenance=false \ --sbom=false \ - --tag ${local.image_uri} \ - --build-arg GIT_COMMIT=${local.git_commit} \ - --build-arg BUILD_DATE=${local.timestamp} \ + --tag "${local.image_uri}" \ + --build-arg "GIT_COMMIT=${local.git_commit}" \ + --build-arg "BUILD_DATE=${local.timestamp}" \ --push \ - ${var.extra_build_args} \ + $EXTRA_BUILD_ARGS \ . echo "Docker image built and pushed successfully" diff --git a/terraform/modules/build/variables.tf b/terraform/modules/build/variables.tf index 8133841f..a9bafd7a 100644 --- a/terraform/modules/build/variables.tf +++ b/terraform/modules/build/variables.tf @@ -35,13 +35,13 @@ variable "skip_docker_build" { } variable "extra_build_args" { - description = "Extra arguments to pass to docker build" + description = "Extra arguments to pass to docker build. Passed through the local-exec provisioner's environment (not interpolated into the script), then whitespace-split unquoted (no shell quoting is honored): a value like \"--build-arg LABEL=hello world\" splits into two docker buildx arguments, not one. No caller sets this today; if a value ever needs an embedded space, extend the module to accept a list(string) instead." type = string default = "" } variable "registry_login_command" { - description = "Command to authenticate with registry (e.g., aws ecr get-login-password | docker login...)" + description = "Command to authenticate with registry (e.g., az acr login --name ..., aws ecr get-login-password | docker login ..., gcloud auth configure-docker ...). Runs verbatim as shell input. Must be an identity-based login (the CLI resolves/mints the credential itself); never embed a static, long-lived credential literal (a password, API key, or JSON key) in this value; it is not redacted in terraform plan/apply output, and marking it sensitive would suppress this resource's entire local-exec log (build/push progress, docker error output), which deploy-*.yml workflows tee and grep to detect build failures." type = string }