From 747da5a356b513c94f1469e918741a84419a174a Mon Sep 17 00:00:00 2001 From: Mike Jensen Date: Mon, 6 Nov 2023 16:32:28 -0700 Subject: [PATCH 1/3] Prevent `.tsh/environment` values from overloading prior set values It's not possible to have duplicate environment values within an environment. And in fact the last value in the string slice will be preserved. Prior to this change that allows users to possibly change any environment values through the use of the `.tsh/environment` file. This is within user level control, where other environment value sources originate from a more protected location (for example the PAM configuration). Prior to this change that allows users to possibly change any environment passed configuration through the use of the `.tsh/environment` file. This change makes it so that the administrative set values will be preferred, and any duplicate records will be ignored. --- lib/srv/ctx.go | 6 +-- lib/srv/reexec.go | 7 +-- lib/utils/envutils/environment.go | 65 ++++++++++++++++++-------- lib/utils/envutils/environment_test.go | 25 +++++++++- 4 files changed, 77 insertions(+), 26 deletions(-) diff --git a/lib/srv/ctx.go b/lib/srv/ctx.go index 8dde7633a2697..0348412c6ee46 100644 --- a/lib/srv/ctx.go +++ b/lib/srv/ctx.go @@ -1207,9 +1207,6 @@ func (id *IdentityContext) GetUserMetadata() apievents.UserMetadata { func buildEnvironment(ctx *ServerContext) []string { env := &envutils.SafeEnv{} - // gather all dynamically defined environment variables - ctx.VisitEnv(env.Add) - // Parse the local and remote addresses to build SSH_CLIENT and // SSH_CONNECTION environment variables. remoteHost, remotePort, err := net.SplitHostPort(ctx.ServerConn.RemoteAddr().String()) @@ -1243,6 +1240,9 @@ func buildEnvironment(ctx *ServerContext) []string { env.Add(teleport.SSHTeleportClusterName, ctx.ClusterName) env.Add(teleport.SSHTeleportUser, ctx.Identity.TeleportUser) + // At the end gather all dynamically defined environment variables + ctx.VisitEnv(env.Add) + return *env } diff --git a/lib/srv/reexec.go b/lib/srv/reexec.go index efc0b3039f77b..a24187d0364c7 100644 --- a/lib/srv/reexec.go +++ b/lib/srv/reexec.go @@ -803,8 +803,12 @@ func buildCommand(c *ExecCommand, localUser *user.User, tty *os.File, pty *os.Fi // Add in Teleport specific environment variables. env.AddFull(c.Environment...) + // If any additional environment variables come from PAM, apply them as well. + env.AddFull(pamEnvironment...) + // If the server allows reading in of ~/.tsh/environment read it in // and pass environment variables along to new session. + // User controlled values are added last to ensure administrator controlled sources take priority (duplicates ignored) if c.PermitUserEnvironment { filename := filepath.Join(localUser.HomeDir, ".tsh", "environment") userEnvs, err := envutils.ReadEnvironmentFile(filename) @@ -814,9 +818,6 @@ func buildCommand(c *ExecCommand, localUser *user.User, tty *os.File, pty *os.Fi env.AddFull(userEnvs...) } - // If any additional environment variables come from PAM, apply them as well. - env.AddFull(pamEnvironment...) - // after environment is fully built, set it to cmd cmd.Env = *env diff --git a/lib/utils/envutils/environment.go b/lib/utils/envutils/environment.go index 4c7e39e6edf2f..3a5da67450299 100644 --- a/lib/utils/envutils/environment.go +++ b/lib/utils/envutils/environment.go @@ -86,16 +86,30 @@ func ReadEnvironmentFile(filename string) ([]string, error) { return *env, nil } -var unsafeEnvironmentVars = []string{ +var unsafeEnvironmentVars = map[string]bool{ // Linux - "LD_ASSUME_KERNEL", "LD_AUDIT", "LD_BIND_NOW", "LD_BIND_NOT", - "LD_DYNAMIC_WEAK", "LD_LIBRARY_PATH", "LD_ORIGIN_PATH", "LD_POINTER_GUARD", "LD_PREFER_MAP_32BIT_EXEC", - "LD_PRELOAD", "LD_PROFILE", "LD_RUNPATH", "LD_RPATH", "LD_USE_LOAD_BIAS", + "LD_ASSUME_KERNEL": true, + "LD_AUDIT": true, + "LD_BIND_NOW": true, + "LD_BIND_NOT": true, + "LD_DYNAMIC_WEAK": true, + "LD_LIBRARY_PATH": true, + "LD_ORIGIN_PATH": true, + "LD_POINTER_GUARD": true, + "LD_PREFER_MAP_32BIT_EXEC": true, + "LD_PRELOAD": true, + "LD_PROFILE": true, + "LD_RUNPATH": true, + "LD_RPATH": true, + "LD_USE_LOAD_BIAS": true, // macOS - "DYLD_INSERT_LIBRARIES", "DYLD_LIBRARY_PATH", + "DYLD_INSERT_LIBRARIES": true, + "DYLD_LIBRARY_PATH": true, } -// SafeEnv allows you to build a system environment while avoiding potentially dangerous environment conditions. +// SafeEnv allows you to build a system environment while avoiding potentially dangerous environment conditions. In +// addition, SafeEnv will ignore any values added if the key already exists. This allows earlier inserts to take +// priority and ensure there is no conflicting values. type SafeEnv []string // Add will add the key and value to the environment if it's a safe value to forward on for fork / exec. @@ -104,35 +118,48 @@ func (e *SafeEnv) Add(k, v string) { v = strings.TrimSpace(v) if k == "" || k == "=" { return - } - - for _, unsafeKey := range unsafeEnvironmentVars { - if strings.EqualFold(k, unsafeKey) { - return - } + } else if e.unsafeKey(k) { + return } *e = append(*e, fmt.Sprintf("%s=%s", k, v)) } -// AddFull adds an exact value, typically in KEY=VALUE format. This should only be used if they values are already -// combined. +// AddFull adds an exact value, in the KEY=VALUE format. This should only be used if they values are already +// combined. When the values are separate the [Add] function is generally preferred. func (e *SafeEnv) AddFull(fullValues ...string) { valueLoop: for _, kv := range fullValues { kv = strings.TrimSpace(kv) - for _, unsafeKey := range unsafeEnvironmentVars { - if strings.HasPrefix(strings.ToUpper(kv), unsafeKey) { - continue valueLoop - } + key := strings.SplitN(kv, "=", 2)[0] + if key == "" { // weird case if the string is empty or '=' + continue valueLoop + } else if e.unsafeKey(key) { + continue valueLoop } *e = append(*e, kv) } } -// AddExecEnvironment will add safe values from [os.Environ]. +func (e *SafeEnv) unsafeKey(key string) bool { + upperKey := strings.ToUpper(key) + if _, unsafe := unsafeEnvironmentVars[upperKey]; unsafe { + return true + } + + prefix := upperKey + "=" + for _, kv := range *e { + if strings.HasPrefix(strings.ToUpper(kv), prefix) { + return true + } + } + + return false +} + +// AddExecEnvironment will add safe values from [os.Environ], ignoring any duplicates that may have already been added. func (e *SafeEnv) AddExecEnvironment() { e.AddFull(os.Environ()...) } diff --git a/lib/utils/envutils/environment_test.go b/lib/utils/envutils/environment_test.go index 69ea7647397e3..1e02c17776134 100644 --- a/lib/utils/envutils/environment_test.go +++ b/lib/utils/envutils/environment_test.go @@ -36,6 +36,7 @@ foo=bar=baz foo= =bar +bar=foo LD_PRELOAD=attack `) @@ -53,7 +54,7 @@ LD_PRELOAD=attack require.NoError(t, err) // check we parsed it correctly - require.Empty(t, cmp.Diff(env, []string{"foo=bar", "foo=bar=baz", "foo="})) + require.Empty(t, cmp.Diff(env, []string{"foo=bar", "bar=foo"})) } func TestSafeEnvAdd(t *testing.T) { @@ -83,6 +84,18 @@ func TestSafeEnvAdd(t *testing.T) { values: []string{" bar "}, expected: []string{"foo=bar"}, }, + { + name: "duplicate ignore", + keys: []string{"one", "one"}, + values: []string{"v1", "v2"}, + expected: []string{"one=v1"}, + }, + { + name: "duplicate ignore different case", + keys: []string{"one", "ONE"}, + values: []string{"v1", "v2"}, + expected: []string{"one=v1"}, + }, { name: "skip dangerous exact", keys: []string{"foo", "LD_PRELOAD"}, @@ -140,6 +153,16 @@ func TestSafeEnvAddFull(t *testing.T) { fullValues: []string{" foo=bar "}, expected: []string{"foo=bar"}, }, + { + name: "duplicate ignore", + fullValues: []string{"one=v1", "one=v2"}, + expected: []string{"one=v1"}, + }, + { + name: "duplicate ignore different case", + fullValues: []string{"one=v1", "ONE=v2"}, + expected: []string{"one=v1"}, + }, { name: "skip dangerous exact", fullValues: []string{"foo=bar", "LD_PRELOAD=ignored"}, From fe2d96743b911d00ef53e8d097e014733e9957a6 Mon Sep 17 00:00:00 2001 From: Mike Jensen Date: Tue, 7 Nov 2023 10:05:00 -0700 Subject: [PATCH 2/3] Apply PR feedback --- lib/utils/envutils/environment.go | 46 +++++++++++++------------- lib/utils/envutils/environment_test.go | 5 +++ 2 files changed, 28 insertions(+), 23 deletions(-) diff --git a/lib/utils/envutils/environment.go b/lib/utils/envutils/environment.go index 3a5da67450299..3b3c6e359c380 100644 --- a/lib/utils/envutils/environment.go +++ b/lib/utils/envutils/environment.go @@ -86,25 +86,25 @@ func ReadEnvironmentFile(filename string) ([]string, error) { return *env, nil } -var unsafeEnvironmentVars = map[string]bool{ +var unsafeEnvironmentVars = map[string]struct{}{ // Linux - "LD_ASSUME_KERNEL": true, - "LD_AUDIT": true, - "LD_BIND_NOW": true, - "LD_BIND_NOT": true, - "LD_DYNAMIC_WEAK": true, - "LD_LIBRARY_PATH": true, - "LD_ORIGIN_PATH": true, - "LD_POINTER_GUARD": true, - "LD_PREFER_MAP_32BIT_EXEC": true, - "LD_PRELOAD": true, - "LD_PROFILE": true, - "LD_RUNPATH": true, - "LD_RPATH": true, - "LD_USE_LOAD_BIAS": true, + "LD_ASSUME_KERNEL": {}, + "LD_AUDIT": {}, + "LD_BIND_NOW": {}, + "LD_BIND_NOT": {}, + "LD_DYNAMIC_WEAK": {}, + "LD_LIBRARY_PATH": {}, + "LD_ORIGIN_PATH": {}, + "LD_POINTER_GUARD": {}, + "LD_PREFER_MAP_32BIT_EXEC": {}, + "LD_PRELOAD": {}, + "LD_PROFILE": {}, + "LD_RUNPATH": {}, + "LD_RPATH": {}, + "LD_USE_LOAD_BIAS": {}, // macOS - "DYLD_INSERT_LIBRARIES": true, - "DYLD_LIBRARY_PATH": true, + "DYLD_INSERT_LIBRARIES": {}, + "DYLD_LIBRARY_PATH": {}, } // SafeEnv allows you to build a system environment while avoiding potentially dangerous environment conditions. In @@ -116,9 +116,7 @@ type SafeEnv []string func (e *SafeEnv) Add(k, v string) { k = strings.TrimSpace(k) v = strings.TrimSpace(v) - if k == "" || k == "=" { - return - } else if e.unsafeKey(k) { + if e.unsafeKey(k) { return } @@ -133,9 +131,7 @@ valueLoop: kv = strings.TrimSpace(kv) key := strings.SplitN(kv, "=", 2)[0] - if key == "" { // weird case if the string is empty or '=' - continue valueLoop - } else if e.unsafeKey(key) { + if e.unsafeKey(key) { continue valueLoop } @@ -144,6 +140,10 @@ valueLoop: } func (e *SafeEnv) unsafeKey(key string) bool { + if key == "" || key == "=" { + return false + } + upperKey := strings.ToUpper(key) if _, unsafe := unsafeEnvironmentVars[upperKey]; unsafe { return true diff --git a/lib/utils/envutils/environment_test.go b/lib/utils/envutils/environment_test.go index 1e02c17776134..a3603761e167b 100644 --- a/lib/utils/envutils/environment_test.go +++ b/lib/utils/envutils/environment_test.go @@ -163,6 +163,11 @@ func TestSafeEnvAddFull(t *testing.T) { fullValues: []string{"one=v1", "ONE=v2"}, expected: []string{"one=v1"}, }, + { + name: "double equal value", + fullValues: []string{"foo=bar=badvalue"}, + expected: []string{"foo=bar=badvalue"}, + }, { name: "skip dangerous exact", fullValues: []string{"foo=bar", "LD_PRELOAD=ignored"}, From b4957315fbfe288f0591fa643f69e05d2802be7f Mon Sep 17 00:00:00 2001 From: Mike Jensen Date: Fri, 10 Nov 2023 09:38:48 -0700 Subject: [PATCH 3/3] Only exclude duplicate environment values sourced from .tsh/environment This change updates `SafeEnv` to be allow the caller to select if the value should be checked for duplicates. We then leverage this to avoid this check when sourced from a trusted source. But then exclude potential duplicates when sourced from .tsh/environment file or the local environment. --- lib/srv/ctx.go | 18 +-- lib/srv/reexec.go | 8 +- lib/utils/envutils/environment.go | 57 ++++++--- lib/utils/envutils/environment_test.go | 170 +++++++++++++++---------- 4 files changed, 155 insertions(+), 98 deletions(-) diff --git a/lib/srv/ctx.go b/lib/srv/ctx.go index 0348412c6ee46..ed06b6b2314f1 100644 --- a/lib/srv/ctx.go +++ b/lib/srv/ctx.go @@ -1217,8 +1217,8 @@ func buildEnvironment(ctx *ServerContext) []string { if err != nil { ctx.Logger.Debugf("Failed to split local address: %v.", err) } else { - env.Add("SSH_CLIENT", fmt.Sprintf("%s %s %s", remoteHost, remotePort, localPort)) - env.Add("SSH_CONNECTION", fmt.Sprintf("%s %s %s %s", remoteHost, remotePort, localHost, localPort)) + env.AddTrusted("SSH_CLIENT", fmt.Sprintf("%s %s %s", remoteHost, remotePort, localPort)) + env.AddTrusted("SSH_CONNECTION", fmt.Sprintf("%s %s %s %s", remoteHost, remotePort, localHost, localPort)) } } @@ -1226,22 +1226,22 @@ func buildEnvironment(ctx *ServerContext) []string { session := ctx.getSession() if session != nil { if session.term != nil { - env.Add("TERM", session.term.GetTermType()) - env.Add("SSH_TTY", session.term.TTY().Name()) + env.AddTrusted("TERM", session.term.GetTermType()) + env.AddTrusted("SSH_TTY", session.term.TTY().Name()) } if session.id != "" { - env.Add(teleport.SSHSessionID, string(session.id)) + env.AddTrusted(teleport.SSHSessionID, string(session.id)) } } // Set some Teleport specific environment variables: SSH_TELEPORT_USER, // SSH_TELEPORT_HOST_UUID, and SSH_TELEPORT_CLUSTER_NAME. - env.Add(teleport.SSHTeleportHostUUID, ctx.srv.ID()) - env.Add(teleport.SSHTeleportClusterName, ctx.ClusterName) - env.Add(teleport.SSHTeleportUser, ctx.Identity.TeleportUser) + env.AddTrusted(teleport.SSHTeleportHostUUID, ctx.srv.ID()) + env.AddTrusted(teleport.SSHTeleportClusterName, ctx.ClusterName) + env.AddTrusted(teleport.SSHTeleportUser, ctx.Identity.TeleportUser) // At the end gather all dynamically defined environment variables - ctx.VisitEnv(env.Add) + ctx.VisitEnv(env.AddUnique) return *env } diff --git a/lib/srv/reexec.go b/lib/srv/reexec.go index a24187d0364c7..b71a76efa0f53 100644 --- a/lib/srv/reexec.go +++ b/lib/srv/reexec.go @@ -801,10 +801,10 @@ func buildCommand(c *ExecCommand, localUser *user.User, tty *os.File, pty *os.Fi } // Add in Teleport specific environment variables. - env.AddFull(c.Environment...) + env.AddFullTrusted(c.Environment...) // If any additional environment variables come from PAM, apply them as well. - env.AddFull(pamEnvironment...) + env.AddFullTrusted(pamEnvironment...) // If the server allows reading in of ~/.tsh/environment read it in // and pass environment variables along to new session. @@ -815,7 +815,7 @@ func buildCommand(c *ExecCommand, localUser *user.User, tty *os.File, pty *os.Fi if err != nil { return nil, trace.Wrap(err) } - env.AddFull(userEnvs...) + env.AddFullUnique(userEnvs...) } // after environment is fully built, set it to cmd @@ -959,7 +959,7 @@ func ConfigureCommand(ctx *ServerContext, extraFiles ...*os.File) (*exec.Cmd, er // build env for `teleport exec` env := &envutils.SafeEnv{} - env.AddFull(cmdmsg.Environment...) + env.AddFullTrusted(cmdmsg.Environment...) env.AddExecEnvironment() // Build the "teleport exec" command. diff --git a/lib/utils/envutils/environment.go b/lib/utils/envutils/environment.go index 3b3c6e359c380..54ab4a5423e99 100644 --- a/lib/utils/envutils/environment.go +++ b/lib/utils/envutils/environment.go @@ -74,7 +74,8 @@ func ReadEnvironmentFile(filename string) ([]string, error) { continue } - env.Add(key, value) + // key is added trusted within this context, but should be "AddFullUnique" when combined with any other values + env.AddTrusted(key, value) } err = scanner.Err() @@ -112,34 +113,56 @@ var unsafeEnvironmentVars = map[string]struct{}{ // priority and ensure there is no conflicting values. type SafeEnv []string -// Add will add the key and value to the environment if it's a safe value to forward on for fork / exec. -func (e *SafeEnv) Add(k, v string) { +// AddTrusted will add the key and value to the environment if it's a safe value to forward on for fork / exec. This +// will not check for duplicates. +func (e *SafeEnv) AddTrusted(k, v string) { + e.add(false, k, v) +} + +// AddUnique will add the key and value to the environment if it's a safe value to forward on for fork / exec. If the +// key already exists (case-insensitive) it will be ignored. +func (e *SafeEnv) AddUnique(k, v string) { + e.add(true, k, v) +} + +func (e *SafeEnv) add(preventDuplicates bool, k, v string) { k = strings.TrimSpace(k) v = strings.TrimSpace(v) - if e.unsafeKey(k) { + if e.unsafeKey(preventDuplicates, k) { return } *e = append(*e, fmt.Sprintf("%s=%s", k, v)) } -// AddFull adds an exact value, in the KEY=VALUE format. This should only be used if they values are already -// combined. When the values are separate the [Add] function is generally preferred. -func (e *SafeEnv) AddFull(fullValues ...string) { -valueLoop: +// AddFullTrusted adds an exact value, in the KEY=VALUE format. This should only be used if they values are already +// combined. When the values are separate the [Add] function is generally preferred. This will not check for +// duplicates. +func (e *SafeEnv) AddFullTrusted(fullValues ...string) { + e.addFull(false, fullValues) +} + +// AddFullUnique adds an exact value, in the KEY=VALUE format. This should only be used if they values are already +// combined. When the values are separate the [Add] function is generally preferred. If any keys already exists +// (case-insensitive) they will be ignored. +func (e *SafeEnv) AddFullUnique(fullValues ...string) { + e.addFull(true, fullValues) +} + +func (e *SafeEnv) addFull(preventDuplicates bool, fullValues []string) { for _, kv := range fullValues { kv = strings.TrimSpace(kv) key := strings.SplitN(kv, "=", 2)[0] - if e.unsafeKey(key) { - continue valueLoop + if e.unsafeKey(preventDuplicates, key) { + continue } *e = append(*e, kv) } } -func (e *SafeEnv) unsafeKey(key string) bool { +func (e *SafeEnv) unsafeKey(preventDuplicates bool, key string) bool { if key == "" || key == "=" { return false } @@ -149,10 +172,12 @@ func (e *SafeEnv) unsafeKey(key string) bool { return true } - prefix := upperKey + "=" - for _, kv := range *e { - if strings.HasPrefix(strings.ToUpper(kv), prefix) { - return true + if preventDuplicates { + prefix := upperKey + "=" + for _, kv := range *e { + if strings.HasPrefix(strings.ToUpper(kv), prefix) { + return true + } } } @@ -161,5 +186,5 @@ func (e *SafeEnv) unsafeKey(key string) bool { // AddExecEnvironment will add safe values from [os.Environ], ignoring any duplicates that may have already been added. func (e *SafeEnv) AddExecEnvironment() { - e.AddFull(os.Environ()...) + e.addFull(true, os.Environ()) } diff --git a/lib/utils/envutils/environment_test.go b/lib/utils/envutils/environment_test.go index a3603761e167b..38b856fb2d014 100644 --- a/lib/utils/envutils/environment_test.go +++ b/lib/utils/envutils/environment_test.go @@ -54,65 +54,81 @@ LD_PRELOAD=attack require.NoError(t, err) // check we parsed it correctly - require.Empty(t, cmp.Diff(env, []string{"foo=bar", "bar=foo"})) + require.Empty(t, cmp.Diff(env, []string{"foo=bar", "foo=bar=baz", "foo=", "bar=foo"})) } func TestSafeEnvAdd(t *testing.T) { t.Parallel() testCases := []struct { - name string - keys []string - values []string - expected []string + name string + excludeDuplicate bool + keys []string + values []string + expected []string }{ { - name: "normal add", - keys: []string{"foo"}, - values: []string{"bar"}, - expected: []string{"foo=bar"}, + name: "normal add", + excludeDuplicate: true, + keys: []string{"foo"}, + values: []string{"bar"}, + expected: []string{"foo=bar"}, }, { - name: "double add", - keys: []string{"one", "two"}, - values: []string{"v1", "v2"}, - expected: []string{"one=v1", "two=v2"}, + name: "double add", + excludeDuplicate: true, + keys: []string{"one", "two"}, + values: []string{"v1", "v2"}, + expected: []string{"one=v1", "two=v2"}, }, { - name: "whitespace trim", - keys: []string{" foo "}, - values: []string{" bar "}, - expected: []string{"foo=bar"}, + name: "whitespace trim", + excludeDuplicate: true, + keys: []string{" foo "}, + values: []string{" bar "}, + expected: []string{"foo=bar"}, }, { - name: "duplicate ignore", - keys: []string{"one", "one"}, - values: []string{"v1", "v2"}, - expected: []string{"one=v1"}, + name: "duplicate ignore", + excludeDuplicate: true, + keys: []string{"one", "one"}, + values: []string{"v1", "v2"}, + expected: []string{"one=v1"}, }, { - name: "duplicate ignore different case", - keys: []string{"one", "ONE"}, - values: []string{"v1", "v2"}, - expected: []string{"one=v1"}, + name: "duplicate different case ignore", + excludeDuplicate: true, + keys: []string{"one", "ONE"}, + values: []string{"v1", "v2"}, + expected: []string{"one=v1"}, }, { - name: "skip dangerous exact", - keys: []string{"foo", "LD_PRELOAD"}, - values: []string{"bar", "ignored"}, - expected: []string{"foo=bar"}, + name: "duplicate allowed", + excludeDuplicate: false, + keys: []string{"one", "one"}, + values: []string{"v1", "v2"}, + expected: []string{"one=v1", "one=v2"}, }, { - name: "skip dangerous lowercase", - keys: []string{"foo", "ld_preload"}, - values: []string{"bar", "ignored"}, - expected: []string{"foo=bar"}, + name: "skip dangerous exact", + excludeDuplicate: true, + keys: []string{"foo", "LD_PRELOAD"}, + values: []string{"bar", "ignored"}, + expected: []string{"foo=bar"}, }, { - name: "skip dangerous with whitespace", - keys: []string{"foo", " LD_PRELOAD"}, - values: []string{"bar", "ignored"}, - expected: []string{"foo=bar"}, + name: "skip dangerous lowercase", + excludeDuplicate: true, + keys: []string{"foo", "ld_preload"}, + values: []string{"bar", "ignored"}, + expected: []string{"foo=bar"}, + }, + { + name: "skip dangerous with whitespace", + excludeDuplicate: true, + keys: []string{"foo", " LD_PRELOAD"}, + values: []string{"bar", "ignored"}, + expected: []string{"foo=bar"}, }, } @@ -123,7 +139,7 @@ func TestSafeEnvAdd(t *testing.T) { env := &SafeEnv{} for i := range tc.keys { - env.Add(tc.keys[i], tc.values[i]) + env.add(tc.excludeDuplicate, tc.keys[i], tc.values[i]) } result := []string(*env) @@ -134,54 +150,70 @@ func TestSafeEnvAdd(t *testing.T) { func TestSafeEnvAddFull(t *testing.T) { testCases := []struct { - name string - fullValues []string - expected []string + name string + excludeDuplicate bool + fullValues []string + expected []string }{ { - name: "normal add", - fullValues: []string{"foo=bar"}, - expected: []string{"foo=bar"}, + name: "normal add", + excludeDuplicate: true, + fullValues: []string{"foo=bar"}, + expected: []string{"foo=bar"}, + }, + { + name: "double add", + excludeDuplicate: true, + fullValues: []string{"one=v1", "two=v2"}, + expected: []string{"one=v1", "two=v2"}, }, { - name: "double add", - fullValues: []string{"one=v1", "two=v2"}, - expected: []string{"one=v1", "two=v2"}, + name: "whitespace trim", + excludeDuplicate: true, + fullValues: []string{" foo=bar "}, + expected: []string{"foo=bar"}, }, { - name: "whitespace trim", - fullValues: []string{" foo=bar "}, - expected: []string{"foo=bar"}, + name: "duplicate ignore", + excludeDuplicate: true, + fullValues: []string{"one=v1", "one=v2"}, + expected: []string{"one=v1"}, }, { - name: "duplicate ignore", - fullValues: []string{"one=v1", "one=v2"}, - expected: []string{"one=v1"}, + name: "duplicate ignore different case", + excludeDuplicate: true, + fullValues: []string{"one=v1", "ONE=v2"}, + expected: []string{"one=v1"}, }, { - name: "duplicate ignore different case", - fullValues: []string{"one=v1", "ONE=v2"}, - expected: []string{"one=v1"}, + name: "duplicate allowed", + excludeDuplicate: false, + fullValues: []string{"one=v1", "one=v2"}, + expected: []string{"one=v1", "one=v2"}, }, { - name: "double equal value", - fullValues: []string{"foo=bar=badvalue"}, - expected: []string{"foo=bar=badvalue"}, + name: "double equal value", + excludeDuplicate: true, + fullValues: []string{"foo=bar=badvalue"}, + expected: []string{"foo=bar=badvalue"}, }, { - name: "skip dangerous exact", - fullValues: []string{"foo=bar", "LD_PRELOAD=ignored"}, - expected: []string{"foo=bar"}, + name: "skip dangerous exact", + excludeDuplicate: true, + fullValues: []string{"foo=bar", "LD_PRELOAD=ignored"}, + expected: []string{"foo=bar"}, }, { - name: "skip dangerous lowercase", - fullValues: []string{"foo=bar", "ld_preload=ignored"}, - expected: []string{"foo=bar"}, + name: "skip dangerous lowercase", + excludeDuplicate: true, + fullValues: []string{"foo=bar", "ld_preload=ignored"}, + expected: []string{"foo=bar"}, }, { - name: "skip dangerous with whitespace", - fullValues: []string{"foo=bar", " LD_PRELOAD=ignored"}, - expected: []string{"foo=bar"}, + name: "skip dangerous with whitespace", + excludeDuplicate: true, + fullValues: []string{"foo=bar", " LD_PRELOAD=ignored"}, + expected: []string{"foo=bar"}, }, } @@ -191,7 +223,7 @@ func TestSafeEnvAddFull(t *testing.T) { t.Parallel() env := &SafeEnv{} - env.AddFull(tc.fullValues...) + env.addFull(tc.excludeDuplicate, tc.fullValues) result := []string(*env) require.Equal(t, tc.expected, result)