provisioner: don't pass CODER_ variables (#4638)

provisioner/terraform/executor.go

@@ -32,7 +32,7 @@ type executor struct {
 func (e executor) basicEnv() []string {
 	// Required for "terraform init" to find "git" to
 	// clone Terraform modules.
-	env := os.Environ()
+	env := safeEnviron()
 	// Only Linux reliably works with the Terraform plugin
 	// cache directory. It's unknown why this is.
 	if e.cachePath != "" && runtime.GOOS == "linux" {
@@ -56,6 +56,10 @@ func (e executor) execWriteOutput(ctx, killCtx context.Context, args, env []stri
 		return ctx.Err()
 	}
 
+	if isCanarySet(env) {
+		return xerrors.New("environment variables not sanitized, this is a bug within Coder")
+	}
+
 	// #nosec
 	cmd := exec.CommandContext(killCtx, e.binaryPath, args...)
 	cmd.Dir = e.workdir

provisioner/terraform/provision.go

@@ -188,7 +188,7 @@ func provisionVars(start *proto.Provision_Start) ([]string, error) {
 }
 
 func provisionEnv(start *proto.Provision_Start) ([]string, error) {
-	env := os.Environ()
+	env := safeEnviron()
 	env = append(env,
 		"CODER_AGENT_URL="+start.Metadata.CoderUrl,
 		"CODER_WORKSPACE_TRANSITION="+strings.ToLower(start.Metadata.WorkspaceTransition.String()),
@@ -232,12 +232,12 @@ var (
 )
 
 func logTerraformEnvVars(logr logger) error {
-	env := os.Environ()
+	env := safeEnviron()
 	for _, e := range env {
 		if strings.HasPrefix(e, "TF_") {
 			parts := strings.SplitN(e, "=", 2)
 			if len(parts) != 2 {
-				panic("os.Environ() returned vars not in key=value form")
+				panic("safeEnviron() returned vars not in key=value form")
 			}
 			if !tfEnvSafeToPrint[parts[0]] {
 				parts[1] = "<value redacted>"

provisioner/terraform/provision_test.go

@@ -415,7 +415,7 @@ func TestProvision(t *testing.T) {
 // nolint:paralleltest
 func TestProvision_ExtraEnv(t *testing.T) {
 	// #nosec
-	secretValue := "oinae3uinxase"
+	const secretValue = "oinae3uinxase"
 	t.Setenv("TF_LOG", "INFO")
 	t.Setenv("TF_SUPERSECRET", secretValue)
 
@@ -459,3 +459,76 @@ func TestProvision_ExtraEnv(t *testing.T) {
 	}
 	require.True(t, found)
 }
+
+// nolint:paralleltest
+func TestProvision_SafeEnv(t *testing.T) {
+	// #nosec
+	const (
+		passedValue = "superautopets"
+		secretValue = "oinae3uinxase"
+	)
+
+	t.Setenv("VALID_USER_ENV", passedValue)
+
+	// We ensure random CODER_ variables aren't passed through to avoid leaking
+	// control plane secrets (e.g. PG URL).
+	t.Setenv("CODER_SECRET", secretValue)
+
+	const echoResource = `
+	resource "null_resource" "a" {
+		provisioner "local-exec" {
+		  command = "env"
+		}
+	  }
+
+	`
+
+	ctx, api := setupProvisioner(t, nil)
+
+	directory := t.TempDir()
+	path := filepath.Join(directory, "main.tf")
+	err := os.WriteFile(path, []byte(echoResource), 0o600)
+	require.NoError(t, err)
+
+	request := &proto.Provision_Request{
+		Type: &proto.Provision_Request_Start{
+			Start: &proto.Provision_Start{
+				Directory: directory,
+				Metadata: &proto.Provision_Metadata{
+					WorkspaceTransition: proto.WorkspaceTransition_START,
+				},
+			},
+		},
+	}
+	response, err := api.Provision(ctx)
+	require.NoError(t, err)
+	err = response.Send(request)
+	require.NoError(t, err)
+	var (
+		foundUserEnv = false
+		// Some CODER_ environment variables used by our Terraform provider
+		// must make it through.
+		foundCoderEnv = false
+	)
+	for {
+		msg, err := response.Recv()
+		require.NoError(t, err)
+
+		if log := msg.GetLog(); log != nil {
+			t.Log(log.Level.String(), log.Output)
+			if strings.Contains(log.Output, passedValue) {
+				foundUserEnv = true
+			}
+			if strings.Contains(log.Output, "CODER_") {
+				foundCoderEnv = true
+			}
+			require.NotContains(t, log.Output, secretValue)
+		}
+		if c := msg.GetComplete(); c != nil {
+			require.Empty(t, c.Error)
+			break
+		}
+	}
+	require.True(t, foundUserEnv)
+	require.True(t, foundCoderEnv)
+}

provisioner/terraform/safeenv.go

@@ -0,0 +1,55 @@
+package terraform
+
+import (
+	"os"
+	"strings"
+)
+
+// We must clean CODER_ environment variables to avoid accidentally passing in
+// secrets like the Postgres connection string. See
+// https://github.com/coder/coder/issues/4635.
+//
+// safeEnviron() is provided as an os.Environ() alternative that strips CODER_
+// variables. As an additional precaution, we check a canary variable before
+// provisioner exec.
+//
+// We cannot strip all CODER_ variables at exec because some are used to
+// configure the provisioner.
+
+const unsafeEnvCanary = "CODER_DONT_PASS"
+
+func init() {
+	os.Setenv(unsafeEnvCanary, "true")
+}
+
+func envName(env string) string {
+	parts := strings.SplitN(env, "=", 1)
+	if len(parts) > 0 {
+		return parts[0]
+	}
+	return ""
+}
+
+func isCanarySet(env []string) bool {
+	for _, e := range env {
+		if envName(e) == unsafeEnvCanary {
+			return true
+		}
+	}
+	return false
+}
+
+// safeEnviron wraps os.Environ but removes CODER_ environment variables.
+func safeEnviron() []string {
+	env := os.Environ()
+	strippedEnv := make([]string, 0, len(env))
+
+	for _, e := range env {
+		name := envName(e)
+		if strings.HasPrefix(name, "CODER_") {
+			continue
+		}
+		strippedEnv = append(strippedEnv, e)
+	}
+	return strippedEnv
+}