provisioner: don't pass CODER_ variables

coder · ammario · Oct 19, 2022 · Oct 18, 2022 · Oct 19, 2022 · Oct 19, 2022
commit 3ee667f9cd8b762f320d27e89b7846b5546060e2
diff --git a/provisioner/terraform/executor.go b/provisioner/terraform/executor.go
@@ -41,6 +41,41 @@ func (e executor) basicEnv() []string {
 	return env
 }
 
+func envName(env string) string {
+	parts := strings.Split(env, "=")
+
+	if len(parts) > 0 {
+		return parts[0]
+	}
+	return ""
+}
+
+// sanitizeCoderEnv removes CODER_ environment variables to prevent accidentally
+// passing in secrets. See https://github.com/coder/coder/issues/4635.
+func sanitizeCoderEnv(env []string) []string {
+	var coderSafeEnv = map[string]struct{}{
+		"CODER_AGENT_URL":             {},
+		"CODER_WORKSPACE_TRANSITION":  {},
+		"CODER_WORKSPACE_NAME":        {},
+		"CODER_WORKSPACE_OWNER":       {},
+		"CODER_WORKSPACE_OWNER_EMAIL": {},
+		"CODER_WORKSPACE_ID":          {},
+		"CODER_WORKSPACE_OWNER_ID":    {},
+	}
+
+	strippedEnv := make([]string, 0, len(env))
+	for _, e := range env {
+		name := envName(e)
+		if strings.HasPrefix(name, "CODER_") {
+			if _, isSafe := coderSafeEnv[name]; !isSafe {
+				continue
+			}
+		}
+		strippedEnv = append(strippedEnv, e)
+	}
+	return strippedEnv
+}
+
 func (e executor) execWriteOutput(ctx, killCtx context.Context, args, env []string, stdOutWriter, stdErrWriter io.WriteCloser) (err error) {
 	defer func() {
 		closeErr := stdOutWriter.Close()
@@ -59,7 +94,7 @@ func (e executor) execWriteOutput(ctx, killCtx context.Context, args, env []stri
 	// #nosec
 	cmd := exec.CommandContext(killCtx, e.binaryPath, args...)
 	cmd.Dir = e.workdir
-	cmd.Env = env
+	cmd.Env = sanitizeCoderEnv(env)
 
 	// We want logs to be written in the correct order, so we wrap all logging
 	// in a sync.Mutex.

diff --git a/provisioner/terraform/provision.go b/provisioner/terraform/provision.go
@@ -189,6 +189,8 @@ func provisionVars(start *proto.Provision_Start) ([]string, error) {
 
 func provisionEnv(start *proto.Provision_Start) ([]string, error) {
 	env := os.Environ()
+	// Be sure to add any values here to `sanitizeCoderEnv`, otherwise they will not
+	// get passed into the execute.
 	env = append(env,
 		"CODER_AGENT_URL="+start.Metadata.CoderUrl,
 		"CODER_WORKSPACE_TRANSITION="+strings.ToLower(start.Metadata.WorkspaceTransition.String()),

diff --git a/provisioner/terraform/provision_test.go b/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)
+}