diff --git a/internal/api/handler_federation_azure_escape_test.go b/internal/api/handler_federation_azure_escape_test.go index 0d66efb33..31feb5e07 100644 --- a/internal/api/handler_federation_azure_escape_test.go +++ b/internal/api/handler_federation_azure_escape_test.go @@ -5,11 +5,13 @@ import ( "bytes" "context" "encoding/base64" + "encoding/json" "net/mail" "os" "os/exec" "path/filepath" "regexp" + "strings" "testing" "github.com/LeanerCloud/cloud-commitments-platform/internal/iacfiles" @@ -45,32 +47,77 @@ func azureZipEntry(t *testing.T, zipBytes []byte, name string) string { return "" } -// evalShellWord evaluates one line of shell in a fresh bash, in an empty -// temporary directory and with an empty environment, and returns what it -// printed plus the directory, so callers can check that no command -// substitution created a file there. Only printf and variable assignments run. -func evalShellWord(t *testing.T, bashPath, script string) (out, dir string) { +// evalShellWord evaluates a snippet of shell in a fresh bash, in an empty +// temporary directory and with an environment of PATH plus env, and returns +// what it printed plus the directory, so callers can check that no command +// substitution created a file there. +func evalShellWord(t *testing.T, bashPath, script string, env ...string) (out, dir string) { t.Helper() dir = t.TempDir() cmd := exec.Command(bashPath, "--noprofile", "--norc", "-c", script) cmd.Dir = dir - cmd.Env = []string{"PATH=" + os.Getenv("PATH")} + cmd.Env = append([]string{"PATH=" + os.Getenv("PATH")}, env...) got, err := cmd.Output() require.NoError(t, err, "bash must evaluate %q", script) return string(got), dir } var ( - contactEmailLineRe = regexp.MustCompile(`(?m)^CONTACT_EMAIL=.*$`) - registerURLWordRe = regexp.MustCompile(`(?m)-X POST ("[^\n]*/api/register") \\$`) + // registrationBlockRe matches deploy-azure.sh from the CONTACT_EMAIL + // assignment through the end of the PAYLOAD heredoc: the part that + // turns the pre-filled email into the registration JSON, stopping + // before the curl call. + registrationBlockRe = regexp.MustCompile(`(?ms)^CONTACT_EMAIL=.*?^JSON$`) + registerURLWordRe = regexp.MustCompile(`(?m)-X POST ("[^\n]*/api/register") \\$`) ) +// registration is what evalRegistration observed. +type registration struct { + contactEmail string + payload map[string]string + dir string +} + +// evalRegistration runs the registration block of deploy-azure.sh in the +// evalShellWord sandbox, with az stubbed by a shell function (no Azure +// CLI, no network; curl is past the end of the block). It returns the +// resulting CONTACT_EMAIL and the PAYLOAD heredoc parsed as JSON. +func evalRegistration(t *testing.T, bashPath, script string, env ...string) registration { + t.Helper() + block := registrationBlockRe.FindString(script) + require.NotEmpty(t, block, "deploy-azure.sh must build the registration payload") + stub := "set -euo pipefail\naz() { printf '%s\\n' 00000000-0000-0000-0000-0000000000aa; }\n" + out, dir := evalShellWord(t, bashPath, + stub+block+"\nprintf '%s\\0%s' \"$CONTACT_EMAIL\" \"$PAYLOAD\"", env...) + email, payload, ok := strings.Cut(out, "\x00") + require.True(t, ok, "sandbox output must hold CONTACT_EMAIL and PAYLOAD: %q", out) + var fields map[string]string + require.NoError(t, json.Unmarshal([]byte(payload), &fields), "PAYLOAD must be valid JSON: %s", payload) + return registration{contactEmail: email, payload: fields, dir: dir} +} + +// hostileContactEmails all pass mail.ParseAddress. They cover command +// substitution (#958), a "}" that would close a ${VAR:-default} early, and +// the characters JSON needs escaped ("\", '"', tab). +var hostileContactEmails = []string{ + "\"a`touch pwned`b\"@example.com", + `"a$(touch pwned)b"@example.com`, + "a`id`b@example.com", + `"a\"b\\c$HOME"@example.com`, + `a}b@example.com`, + `"a}b"@example.com`, + "\"a}$(touch pwned)`touch pwned`}b\"@example.com", + `"{a}${b}"@example.com`, + "\"a\tb\"@example.com", +} + // TestGetFederationIaC_AzureDeployScriptEscapesSessionEmail is the // regression test for #958. deploy-azure.sh in the Bicep and ARM bundles // embeds the session user's email inside a double-quoted bash string. The -// email only has to pass mail.ParseAddress, which accepts backticks and -// "$(...)", so the script must carry it shell-escaped: running the -// CONTACT_EMAIL assignment must yield the email verbatim and run nothing. +// email only has to pass mail.ParseAddress, which accepts backticks, +// "$(...)", "}", quotes, backslashes and tabs. Running the registration +// block must leave CONTACT_EMAIL equal to the email verbatim, put it into +// valid JSON unchanged, and run nothing. func TestGetFederationIaC_AzureDeployScriptEscapesSessionEmail(t *testing.T) { bashPath, err := exec.LookPath("bash") if err != nil { @@ -78,13 +125,7 @@ func TestGetFederationIaC_AzureDeployScriptEscapesSessionEmail(t *testing.T) { } t.Setenv("CUDLY_SOURCE_CLOUD", "gcp") - emails := []string{ - "\"a`touch pwned`b\"@example.com", - `"a$(touch pwned)b"@example.com`, - "a`id`b@example.com", - `"a\"b\\c$HOME"@example.com`, - } - for _, email := range emails { + for _, email := range hostileContactEmails { _, err := mail.ParseAddress(email) require.NoError(t, err, "fixture %q must be an address the auth service accepts", email) @@ -112,17 +153,55 @@ func TestGetFederationIaC_AzureDeployScriptEscapesSessionEmail(t *testing.T) { out, err := syntax.CombinedOutput() require.NoError(t, err, "deploy-azure.sh must parse: %s", out) - assign := contactEmailLineRe.FindString(script) - require.NotEmpty(t, assign, "deploy-azure.sh must assign CONTACT_EMAIL") - got, dir := evalShellWord(t, bashPath, assign+`; printf '%s' "$CONTACT_EMAIL"`) - assert.Equal(t, email, got, "CONTACT_EMAIL must evaluate to the email verbatim") - assert.NoFileExists(t, filepath.Join(dir, "pwned"), + got := evalRegistration(t, bashPath, script) + assert.Equal(t, email, got.contactEmail, "CONTACT_EMAIL must evaluate to the email verbatim") + assert.Equal(t, email, got.payload["contact_email"], "the payload must carry the email verbatim") + assert.NoFileExists(t, filepath.Join(got.dir, "pwned"), "a command substitution in the email must not run") }) } } } +// TestBuildAzureTemplateZip_RegistrationEnvOverrides pins the env-override +// semantics of the restructured CONTACT_EMAIL assignment (a set, non-empty +// CUDLY_CONTACT_EMAIL wins; unset or empty falls back to the pre-filled +// email, as ${VAR:-default} did) and that an operator-supplied +// CUDLY_ACCOUNT_NAME is JSON-escaped too. +func TestBuildAzureTemplateZip_RegistrationEnvOverrides(t *testing.T) { + bashPath, err := exec.LookPath("bash") + if err != nil { + t.Skip("bash not available") + } + const prefilled = `"pre}filled"@example.com` + data := buildGenericIaCData("azure", "gcp", "https://cudly.example.com") + data.ContactEmail = prefilled + zipBytes, _, err := buildAzureTemplateZip("bicep", data, "azure", "target") + require.NoError(t, err) + script := azureDeployScriptFromZip(t, zipBytes) + + cases := []struct { + name, wantEmail string + env []string + }{ + {"unset", prefilled, nil}, + {"empty", prefilled, []string{"CUDLY_CONTACT_EMAIL="}}, + {"set", `"o}p\s"@example.com`, []string{`CUDLY_CONTACT_EMAIL="o}p\s"@example.com`}}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + got := evalRegistration(t, bashPath, script, tc.env...) + assert.Equal(t, tc.wantEmail, got.contactEmail) + assert.Equal(t, tc.wantEmail, got.payload["contact_email"]) + assert.Equal(t, "Azure 00000000-0000-0000-0000-0000000000aa", got.payload["account_name"]) + }) + } + + const name = "Ops \"prod\" \\ team\tB" + got := evalRegistration(t, bashPath, script, "CUDLY_ACCOUNT_NAME="+name) + assert.Equal(t, name, got.payload["account_name"], "account_name must be carried verbatim as JSON") +} + // TestBuildAzureTemplateZip_DeployScriptEscapesEveryField covers the other // interpolation in deploy-azure.sh, the CUDly API URL in the registration // curl call, with the builder called directly on hostile data. @@ -166,7 +245,7 @@ func TestBuildAzureTemplateZip_BenignDataRendersUnchanged(t *testing.T) { data.ContactEmail = "admin@example.com" want, err := renderTemplate("templates/azure-wif-bicep-deploy.sh.tmpl", data) require.NoError(t, err) - require.Contains(t, want, `CONTACT_EMAIL="${CUDLY_CONTACT_EMAIL:-admin@example.com}"`) + require.Contains(t, want, `CONTACT_EMAIL="admin@example.com"`) for _, format := range []string{"bicep", "arm"} { t.Run(format, func(t *testing.T) { diff --git a/internal/api/handler_federation_test.go b/internal/api/handler_federation_test.go index 07240ae6d..3117d7437 100644 --- a/internal/api/handler_federation_test.go +++ b/internal/api/handler_federation_test.go @@ -1002,7 +1002,8 @@ func TestBuildAzureTemplateZip_IncludesRenderedDeployScript(t *testing.T) { "deploy script must handle success codes") assert.Contains(t, script, "409", "deploy script must handle 409 already-pending") - assert.Contains(t, script, `CONTACT_EMAIL="${CUDLY_CONTACT_EMAIL:-`, + assert.Contains(t, script, `CONTACT_EMAIL="${CUDLY_CONTACT_EMAIL:-}"`+"\n"+ + `[ -n "$CONTACT_EMAIL" ] || CONTACT_EMAIL="admin@example.com"`, "deploy script must pre-fill CONTACT_EMAIL with env-override support") }) } diff --git a/internal/iacfiles/templates/azure-wif-bicep-deploy.sh.tmpl b/internal/iacfiles/templates/azure-wif-bicep-deploy.sh.tmpl index fc2bcfd61..5c9016524 100644 --- a/internal/iacfiles/templates/azure-wif-bicep-deploy.sh.tmpl +++ b/internal/iacfiles/templates/azure-wif-bicep-deploy.sh.tmpl @@ -61,18 +61,30 @@ echo "Set azure_auth_mode=workload_identity_federation in CUDly." # --------------------------------------------------------------------------- echo "" echo "Registering account with CUDly..." -CONTACT_EMAIL="${CUDLY_CONTACT_EMAIL:-{{.ContactEmail}}}" +# The pre-filled email is assigned on its own line, not as a ${VAR:-default}, +# so a "}" in it cannot close the expansion early (#958). +CONTACT_EMAIL="${CUDLY_CONTACT_EMAIL:-}" +[ -n "$CONTACT_EMAIL" ] || CONTACT_EMAIL="{{.ContactEmail}}" SUBSCRIPTION_ID="${AZURE_SUBSCRIPTION_ID:-$(az account show --query id -o tsv)}" TENANT_ID=$(az account show --query tenantId -o tsv) ACCOUNT_NAME="${CUDLY_ACCOUNT_NAME:-Azure ${SUBSCRIPTION_ID}}" # AZURE_CLIENT_ID is written by the preceding azure-wif-cli.sh step. CLIENT_ID="${AZURE_CLIENT_ID:-}" +# json_escape prints $1 escaped for use inside a JSON string: \ and " are +# backslash-escaped and a tab becomes \t. CR and LF are not handled: the +# email cannot hold them (net/mail rejects them), and CUDLY_ACCOUNT_NAME is +# set by the operator running this script. +json_escape() { + printf '%s' "$1" | sed -e 's/\\/\\\\/g' -e 's/"/\\"/g' -e "s/$(printf '\t')/\\\\t/g" +} +CONTACT_EMAIL_JSON=$(json_escape "$CONTACT_EMAIL") +ACCOUNT_NAME_JSON=$(json_escape "$ACCOUNT_NAME") read -r -d '' PAYLOAD <