Refactor to less fragile canary approach

... and remove some dead frontend code ... and cleanup English in new UI
coder · ammario · Oct 19, 2022 · Oct 18, 2022 · Oct 19, 2022 · Oct 19, 2022
commit 7e839ad25db0982a655fe8b8c2a88ae0a938b42b
diff --git a/provisioner/terraform/executor.go b/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" {
@@ -41,41 +41,6 @@ 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()
@@ -91,10 +56,14 @@ 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
-	cmd.Env = sanitizeCoderEnv(env)
+	cmd.Env = 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
@@ -188,9 +188,7 @@ 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 := safeEnviron()
 	env = append(env,
 		"CODER_AGENT_URL="+start.Metadata.CoderUrl,
 		"CODER_WORKSPACE_TRANSITION="+strings.ToLower(start.Metadata.WorkspaceTransition.String()),
@@ -234,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>"

diff --git a/provisioner/terraform/safeenv.go b/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
+}
diff --git a/site/src/components/Resources/AgentLatency.tsx b/site/src/components/Resources/AgentLatency.tsx
@@ -68,8 +68,8 @@ export const AgentLatency: FC<{ agent: WorkspaceAgent }> = ({ agent }) => {
       >
         <HelpTooltipTitle>Latency</HelpTooltipTitle>
         <HelpTooltipText>
-          Latency from relay servers, used when connections cannot connect
-          peer-to-peer. Star indicates the preferred relay.
+          This is the latency overhead on non peer to peer connections. The star
-          This is the latency overhead on non peer to peer connections. The star
+          This is the latency overhead on non peer-to-peer connections. The star
-          This is the latency overhead on non peer to peer connections. The star
+          This is the latency overhead on non peer-to-peer connections. The star
+          indicates the preferred relay.
         </HelpTooltipText>
 
         <HelpTooltipText>

diff --git a/site/src/components/Resources/ResourceAgentLatency.stories.tsx b/site/src/components/Resources/ResourceAgentLatency.stories.tsx
diff --git a/site/src/components/Resources/ResourceAgentLatency.tsx b/site/src/components/Resources/ResourceAgentLatency.tsx