From a2864f8dab05144dd22d7a9837f2b449fbd91c85 Mon Sep 17 00:00:00 2001 From: Anton Kosyakov Date: Sun, 8 Aug 2021 06:07:00 +0000 Subject: [PATCH 1/2] [supervisor] fix #5104: don't truncate env values after `=` char --- components/supervisor/pkg/supervisor/supervisor.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/components/supervisor/pkg/supervisor/supervisor.go b/components/supervisor/pkg/supervisor/supervisor.go index 19ae8fec2ef8bf..eb4232cefbc98b 100644 --- a/components/supervisor/pkg/supervisor/supervisor.go +++ b/components/supervisor/pkg/supervisor/supervisor.go @@ -599,7 +599,7 @@ func prepareIDELaunch(cfg *Config) *exec.Cmd { func buildChildProcEnv(cfg *Config) []string { envs := make(map[string]string) for _, e := range os.Environ() { - segs := strings.Split(e, "=") + segs := strings.SplitN(e, "=", 2) if len(segs) < 2 { log.Printf("\"%s\" has invalid format, not including in IDE environment", e) continue From ed571bec8495386969e3509d22baabbd5207baf4 Mon Sep 17 00:00:00 2001 From: Christian Weichel Date: Sun, 8 Aug 2021 12:40:16 +0000 Subject: [PATCH 2/2] [supervisor] Add buildChildProcEnv unit test --- .../supervisor/pkg/supervisor/supervisor.go | 20 ++- .../pkg/supervisor/supervisor_test.go | 128 ++++++++++++++++++ 2 files changed, 142 insertions(+), 6 deletions(-) create mode 100644 components/supervisor/pkg/supervisor/supervisor_test.go diff --git a/components/supervisor/pkg/supervisor/supervisor.go b/components/supervisor/pkg/supervisor/supervisor.go index eb4232cefbc98b..b9a1dfef4a376b 100644 --- a/components/supervisor/pkg/supervisor/supervisor.go +++ b/components/supervisor/pkg/supervisor/supervisor.go @@ -194,7 +194,7 @@ func Run(options ...RunOption) { go analyseConfigChanges(ctx, cfg, analytics, gitpodConfigService) termMuxSrv.DefaultWorkdir = cfg.RepoRoot - termMuxSrv.Env = buildChildProcEnv(cfg) + termMuxSrv.Env = buildChildProcEnv(cfg, nil) termMuxSrv.DefaultCreds = &syscall.Credential{ Uid: gitpodUID, Gid: gitpodGID, @@ -580,7 +580,7 @@ func prepareIDELaunch(cfg *Config) *exec.Cmd { Gid: gitpodGID, }, } - cmd.Env = buildChildProcEnv(cfg) + cmd.Env = buildChildProcEnv(cfg, nil) // Here we must resist the temptation to "neaten up" the IDE output for headless builds. // This would break the JSON parsing of the headless builds. @@ -596,9 +596,15 @@ func prepareIDELaunch(cfg *Config) *exec.Cmd { return cmd } -func buildChildProcEnv(cfg *Config) []string { +// buildChildProcEnv computes the environment variables passed to a child process, based on the total list +// of envvars. If envvars is nil, os.Environ() is used. +func buildChildProcEnv(cfg *Config, envvars []string) []string { + if envvars == nil { + envvars = os.Environ() + } + envs := make(map[string]string) - for _, e := range os.Environ() { + for _, e := range envvars { segs := strings.SplitN(e, "=", 2) if len(segs) < 2 { log.Printf("\"%s\" has invalid format, not including in IDE environment", e) @@ -633,7 +639,9 @@ func buildChildProcEnv(cfg *Config) []string { envs["USER"] = "gitpod" // Particular Java optimisation: Java pre v10 did not gauge it's available memory correctly, and needed explicitly setting "-Xmx" for all Hotspot/openJDK VMs - envs["JAVA_TOOL_OPTIONS"] += fmt.Sprintf(" -Xmx%sm", os.Getenv("GITPOD_MEMORY")) + if mem, ok := envs["GITPOD_MEMORY"]; ok { + envs["JAVA_TOOL_OPTIONS"] += fmt.Sprintf(" -Xmx%sm", mem) + } var env, envn []string for nme, val := range envs { @@ -926,7 +934,7 @@ func startSSHServer(ctx context.Context, cfg *Config, wg *sync.WaitGroup) { } cmd := exec.Command(dropbear, "-F", "-E", "-w", "-s", "-p", fmt.Sprintf(":%d", cfg.SSHPort), "-r", hostkeyFN.Name()) - cmd.Env = buildChildProcEnv(cfg) + cmd.Env = buildChildProcEnv(cfg, nil) cmd.Stdout = os.Stdout cmd.Stderr = os.Stderr err = cmd.Start() diff --git a/components/supervisor/pkg/supervisor/supervisor_test.go b/components/supervisor/pkg/supervisor/supervisor_test.go new file mode 100644 index 00000000000000..b52f15ae3b958b --- /dev/null +++ b/components/supervisor/pkg/supervisor/supervisor_test.go @@ -0,0 +1,128 @@ +// Copyright (c) 2020 Gitpod GmbH. All rights reserved. +// Licensed under the GNU Affero General Public License (AGPL). +// See License-AGPL.txt in the project root for license information. + +package supervisor + +import ( + "fmt" + "os" + "sort" + "testing" + + "github.com/google/go-cmp/cmp" +) + +func TestBuildChildProcEnv(t *testing.T) { + withBaseline := func(i []string) []string { + return append(i, + "SUPERVISOR_ADDR=localhost:8080", + "HOME=/home/gitpod", + "USER=gitpod", + ) + } + + tests := []struct { + Name string + Input []string + Expectation []string + Assert func(t *testing.T, act []string) + }{ + { + Name: "empty set", + Input: []string{}, + Expectation: withBaseline(nil), + }, + { + Name: "= in value", + Input: []string{"FOO=BAR=BAZ"}, + Expectation: withBaseline([]string{"FOO=BAR=BAZ"}), + }, + { + Name: "override baseline", + Input: []string{ + "SUPERVISOR_ADDR=foobar", + "HOME=foobar", + "USER=foobar", + }, + Expectation: withBaseline(nil), + }, + { + Name: "removes blacklisted vars", + Input: []string{"GITPOD_TOKENS=foobar"}, + Expectation: withBaseline(nil), + }, + { + Name: "invalid env var", + Input: []string{"FOOBAR"}, + Expectation: withBaseline(nil), + }, + { + // for testing purposes we can pass a set of envvars to buildChildProcEnv. + // When a caller passes nil, buildChildProcEnv calls os.Environ(). + // This test case ensures we make that call and don't break on behaviour introduced + // just for testing. + Name: "os.Environ", + Input: nil, + Assert: func(t *testing.T, act []string) { + // if we've called os.Environ, we expect PATH to be present + var ( + hasPath bool + path = fmt.Sprintf("PATH=%s", os.Getenv("PATH")) + ) + for _, e := range act { + if e == path { + hasPath = true + break + } + } + if !hasPath { + t.Errorf("no PATH envvar found - probably did not call os.Environ()") + } + }, + }, + } + + for _, test := range tests { + t.Run(test.Name, func(t *testing.T) { + assert := test.Assert + if assert == nil { + assert = func(t *testing.T, act []string) { + exp := test.Expectation + sort.Strings(act) + sort.Strings(exp) + if diff := cmp.Diff(exp, act); diff != "" { + t.Errorf("unexpected buildChildProcEnv (-want +got):\n%s", diff) + } + } + } + + act := buildChildProcEnv(&Config{StaticConfig: StaticConfig{APIEndpointPort: 8080}}, test.Input) + assert(t, act) + }) + } +} + +func TestIsBlacklistedEnvvar(t *testing.T) { + tests := []struct { + Name string + Input string + Expectation bool + }{ + {Name: "deprecated theia envvars", Input: "THEIA_SUPERVISOR_FOOBAR", Expectation: true}, + {Name: "gitpod tokens", Input: "GITPOD_TOKENS", Expectation: true}, + {Name: "gitpod tokens child", Input: "GITPOD_TOKENS_GITHUB", Expectation: true}, + {Name: "kubernetes services", Input: "KUBERNETES_SERVICE_FOOBAR", Expectation: true}, + {Name: "kubernetes service ports", Input: "KUBERNETES_PORT_FOOBAR", Expectation: true}, + {Name: "something with spaces", Input: " I_DO_NOT_UNDERSTAND", Expectation: true}, + {Name: "path", Input: "PATH", Expectation: false}, + } + for _, test := range tests { + t.Run(test.Name, func(t *testing.T) { + act := isBlacklistedEnvvar(test.Input) + if diff := cmp.Diff(test.Expectation, act); diff != "" { + t.Errorf("unexpected isBlacklistedEnvvar (-want +got):\n%s", diff) + } + }) + } +}