diff --git a/provisioner/terraform/executor.go b/provisioner/terraform/executor.go index 5c7d7199024c0..8a2d10727c5bf 100644 --- 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" { @@ -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 diff --git a/provisioner/terraform/provision.go b/provisioner/terraform/provision.go index 8a858e3bb3bb6..fd61d96773a39 100644 --- a/provisioner/terraform/provision.go +++ b/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] = "" diff --git a/provisioner/terraform/provision_test.go b/provisioner/terraform/provision_test.go index a1ab1584e7df6..8ee534b6e9d4c 100644 --- 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) +} diff --git a/provisioner/terraform/safeenv.go b/provisioner/terraform/safeenv.go new file mode 100644 index 0000000000000..0b3cedd71440e --- /dev/null +++ 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 index ab74f8271e0c6..18af176149632 100644 --- 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 }) => { > Latency - 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 + indicates the preferred relay. diff --git a/site/src/components/Resources/ResourceAgentLatency.stories.tsx b/site/src/components/Resources/ResourceAgentLatency.stories.tsx deleted file mode 100644 index 1d934d77b7064..0000000000000 --- a/site/src/components/Resources/ResourceAgentLatency.stories.tsx +++ /dev/null @@ -1,46 +0,0 @@ -import { Story } from "@storybook/react" -import { - ResourceAgentLatency, - ResourceAgentLatencyProps, -} from "./ResourceAgentLatency" - -export default { - title: "components/ResourceAgentLatency", - component: ResourceAgentLatency, -} - -const Template: Story = (args) => ( - -) - -export const Single = Template.bind({}) -Single.args = { - latency: { - "Coder DERP": { - latency_ms: 100.52, - preferred: true, - }, - }, -} - -export const Multiple = Template.bind({}) -Multiple.args = { - latency: { - Chicago: { - latency_ms: 22.25551, - preferred: false, - }, - "New York": { - latency_ms: 56.1111, - preferred: true, - }, - "San Francisco": { - latency_ms: 125.11, - preferred: false, - }, - Tokyo: { - latency_ms: 255, - preferred: false, - }, - }, -} diff --git a/site/src/components/Resources/ResourceAgentLatency.tsx b/site/src/components/Resources/ResourceAgentLatency.tsx deleted file mode 100644 index 2596448b81f43..0000000000000 --- a/site/src/components/Resources/ResourceAgentLatency.tsx +++ /dev/null @@ -1,70 +0,0 @@ -import { makeStyles } from "@material-ui/core/styles" -import StarIcon from "@material-ui/icons/Star" -import React from "react" -import { WorkspaceAgent } from "../../api/typesGenerated" -import { HelpTooltip, HelpTooltipText } from "../Tooltips/HelpTooltip" - -export interface ResourceAgentLatencyProps { - latency: WorkspaceAgent["latency"] -} - -export const ResourceAgentLatency: React.FC = ( - props, -) => { - const styles = useStyles() - if (!props.latency) { - return null - } - if (Object.keys(props.latency).length === 0) { - return null - } - const latency = props.latency - return ( -
-
- Latency - - - Latency from relay servers, used when connections cannot connect - peer-to-peer. Star indicates the preferred relay. - - -
- {Object.keys(latency) - .sort() - .map((region) => { - const value = latency[region] - return ( -
- {region}: {Math.round(value.latency_ms * 100) / 100}{" "} - ms - {value.preferred && } -
- ) - })} -
- ) -} - -const useStyles = makeStyles(() => ({ - root: { - display: "grid", - gap: 6, - }, - title: { - fontSize: "Something", - display: "flex", - alignItems: "center", - // This ensures that the latency aligns with other columns in the grid. - height: 20, - }, - region: { - display: "flex", - alignItems: "center", - }, - star: { - width: 14, - height: 14, - marginLeft: 4, - }, -}))