-
Notifications
You must be signed in to change notification settings - Fork 756
implement preserving env from host into vm in shell command #3830
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What about
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It was an oversight. I only mentioned So yes, In some ways you could argue that
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Scary to leak the credentials by default |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,3 +1,3 @@ | ||
| #!/bin/sh | ||
| set -eu | ||
| exec lima nerdctl "$@" | ||
| exec limactl shell --preserve-env default nerdctl "$@" | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Needs a comment to explain the reason |
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,135 @@ | ||
| // SPDX-FileCopyrightText: Copyright The Lima Authors | ||
| // SPDX-License-Identifier: Apache-2.0 | ||
|
|
||
| package envutil | ||
|
|
||
| import ( | ||
| "os" | ||
| "slices" | ||
| "strings" | ||
|
|
||
| "github.com/sirupsen/logrus" | ||
| ) | ||
|
|
||
| // defaultBlockList contains environment variables that should not be propagated by default. | ||
| var defaultBlockList = []string{ | ||
| "BASH*", | ||
| "DISPLAY", | ||
| "DYLD_*", | ||
| "EUID", | ||
| "FPATH", | ||
| "GID", | ||
| "GROUP", | ||
| "HOME", | ||
| "HOSTNAME", | ||
| "LD_*", | ||
| "LOGNAME", | ||
| "OLDPWD", | ||
| "PATH", | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. And
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This doesn't work with the current wildcard implementation (only the last character can be an I'm not sure we should block Is there a reason that the |
||
| "PWD", | ||
| "SHELL", | ||
| "SHLVL", | ||
| "SSH_*", | ||
| "TERM", | ||
| "TERMINFO", | ||
| "TMPDIR", | ||
| "UID", | ||
| "USER", | ||
| "XAUTHORITY", | ||
| "XDG_*", | ||
| "ZDOTDIR", | ||
| "ZSH*", | ||
| "_*", // Variables starting with underscore are typically internal | ||
| } | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would sort the builtin list alphabetically, so it is easier to see which variables are filtered. And I think we should add more variables, but it is of course subject to discussion. Some suggestions: Edit: I had Not sure about excluding shell-specific variables:
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Was this list taken from somewhere else?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There is no upstream afaik, we just made it up. Which is why I asked for feedback on it. Did check it against my own setup on macOS, and the blocks make sense to me. I'm unsure if we are blocking too much with the |
||
|
|
||
| func getBlockList() []string { | ||
| blockEnv := os.Getenv("LIMA_SHELLENV_BLOCK") | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please update https://lima-vm.io/docs/config/environment-variables/ |
||
| if blockEnv == "" { | ||
| return defaultBlockList | ||
| } | ||
| after, found := strings.CutPrefix(blockEnv, "+") | ||
| if !found { | ||
| return parseEnvList(blockEnv) | ||
| } | ||
| return slices.Concat(defaultBlockList, parseEnvList(after)) | ||
| } | ||
|
|
||
| func getAllowList() []string { | ||
| if allowEnv := os.Getenv("LIMA_SHELLENV_ALLOW"); allowEnv != "" { | ||
| return parseEnvList(allowEnv) | ||
| } | ||
| return nil | ||
| } | ||
|
|
||
| func parseEnvList(envList string) []string { | ||
| parts := strings.Split(envList, ",") | ||
| result := make([]string, 0, len(parts)) | ||
| for _, part := range parts { | ||
| if trimmed := strings.TrimSpace(part); trimmed != "" { | ||
| result = append(result, trimmed) | ||
| } | ||
| } | ||
|
|
||
| return result | ||
| } | ||
|
|
||
| func matchesPattern(name, pattern string) bool { | ||
| if pattern == name { | ||
| return true | ||
| } | ||
|
|
||
| prefix, found := strings.CutSuffix(pattern, "*") | ||
| return found && strings.HasPrefix(name, prefix) | ||
| } | ||
|
|
||
| func matchesAnyPattern(name string, patterns []string) bool { | ||
| return slices.ContainsFunc(patterns, func(pattern string) bool { | ||
| return matchesPattern(name, pattern) | ||
| }) | ||
| } | ||
|
|
||
| // FilterEnvironment filters environment variables based on configuration from environment variables. | ||
| // It returns a slice of environment variables that are not blocked by the current configuration. | ||
| // The filtering is controlled by LIMA_SHELLENV_BLOCK and LIMA_SHELLENV_ALLOW environment variables. | ||
| func FilterEnvironment() []string { | ||
olamilekan000 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| return filterEnvironmentWithLists( | ||
| os.Environ(), | ||
| getAllowList(), | ||
| getBlockList(), | ||
| ) | ||
| } | ||
|
|
||
| func filterEnvironmentWithLists(env, allowList, blockList []string) []string { | ||
| var filtered []string | ||
|
|
||
| for _, envVar := range env { | ||
| parts := strings.SplitN(envVar, "=", 2) | ||
| if len(parts) != 2 { | ||
| continue | ||
| } | ||
|
|
||
| name := parts[0] | ||
|
|
||
| if len(allowList) > 0 { | ||
| if !matchesAnyPattern(name, allowList) { | ||
| continue | ||
| } | ||
| filtered = append(filtered, envVar) | ||
| continue | ||
| } | ||
|
|
||
| if matchesAnyPattern(name, blockList) { | ||
| logrus.Debugf("Blocked env variable %q", name) | ||
| continue | ||
olamilekan000 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| } | ||
|
|
||
| filtered = append(filtered, envVar) | ||
| } | ||
|
|
||
| return filtered | ||
| } | ||
|
|
||
| // GetDefaultBlockList returns a copy of the default block list. | ||
| func GetDefaultBlockList() []string { | ||
| return slices.Clone(defaultBlockList) | ||
| } | ||
|
Comment on lines
+132
to
+135
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could we move this function to the top, right after the Usually, unexported functions go below exported ones.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree that it makes sense to have this function close to the definition because it is a small exported function directly tied to the variable. In general we don't follow a rules that unexported functions go to the bottom. Most of the code seems to follow the pattern of defining helper functions before the exported function that uses them. |
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,210 @@ | ||
| // SPDX-FileCopyrightText: Copyright The Lima Authors | ||
| // SPDX-License-Identifier: Apache-2.0 | ||
|
|
||
| package envutil | ||
|
|
||
| import ( | ||
| "os" | ||
| "slices" | ||
| "strings" | ||
| "testing" | ||
|
|
||
| "gotest.tools/v3/assert" | ||
| ) | ||
|
|
||
| func isUsingDefaultBlockList() bool { | ||
| blockEnv := os.Getenv("LIMA_SHELLENV_BLOCK") | ||
| return blockEnv == "" || strings.HasPrefix(blockEnv, "+") | ||
| } | ||
jandubois marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| func TestMatchesPattern(t *testing.T) { | ||
| tests := []struct { | ||
| name string | ||
| pattern string | ||
| expected bool | ||
| }{ | ||
| {"PATH", "PATH", true}, | ||
| {"PATH", "HOME", false}, | ||
| {"SSH_AUTH_SOCK", "SSH_*", true}, | ||
| {"SSH_AGENT_PID", "SSH_*", true}, | ||
| {"HOME", "SSH_*", false}, | ||
| {"XDG_CONFIG_HOME", "XDG_*", true}, | ||
| {"_LIMA_TEST", "_*", true}, | ||
| {"LIMA_HOME", "_*", false}, | ||
| } | ||
|
|
||
| for _, tt := range tests { | ||
| t.Run(tt.name+"_matches_"+tt.pattern, func(t *testing.T) { | ||
| result := matchesPattern(tt.name, tt.pattern) | ||
| assert.Equal(t, result, tt.expected) | ||
| }) | ||
| } | ||
| } | ||
|
|
||
| func TestMatchesAnyPattern(t *testing.T) { | ||
| patterns := []string{"PATH", "SSH_*", "XDG_*"} | ||
|
|
||
| tests := []struct { | ||
| name string | ||
| expected bool | ||
| }{ | ||
| {"PATH", true}, | ||
| {"HOME", false}, | ||
| {"SSH_AUTH_SOCK", true}, | ||
| {"XDG_CONFIG_HOME", true}, | ||
| {"USER", false}, | ||
| } | ||
|
|
||
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| result := matchesAnyPattern(tt.name, patterns) | ||
| assert.Equal(t, result, tt.expected) | ||
| }) | ||
| } | ||
| } | ||
|
|
||
| func TestParseEnvList(t *testing.T) { | ||
| tests := []struct { | ||
| input string | ||
| expected []string | ||
| }{ | ||
| {"", []string{}}, | ||
| {"PATH", []string{"PATH"}}, | ||
| {"PATH,HOME", []string{"PATH", "HOME"}}, | ||
| {"PATH, HOME , USER", []string{"PATH", "HOME", "USER"}}, | ||
| {" , , ", []string{}}, | ||
| } | ||
|
|
||
| for _, tt := range tests { | ||
| t.Run(tt.input, func(t *testing.T) { | ||
| result := parseEnvList(tt.input) | ||
| assert.DeepEqual(t, result, tt.expected) | ||
| }) | ||
| } | ||
| } | ||
|
|
||
| func TestGetBlockAndAllowLists(t *testing.T) { | ||
| t.Run("default config", func(t *testing.T) { | ||
| t.Setenv("LIMA_SHELLENV_BLOCK", "") | ||
| t.Setenv("LIMA_SHELLENV_ALLOW", "") | ||
|
|
||
| blockList := getBlockList() | ||
| allowList := getAllowList() | ||
|
|
||
| assert.Assert(t, isUsingDefaultBlockList()) | ||
| assert.DeepEqual(t, blockList, defaultBlockList) | ||
| assert.Equal(t, len(allowList), 0) | ||
| }) | ||
|
|
||
| t.Run("custom blocklist", func(t *testing.T) { | ||
| t.Setenv("LIMA_SHELLENV_BLOCK", "PATH,HOME") | ||
|
|
||
| blockList := getBlockList() | ||
| assert.Assert(t, !isUsingDefaultBlockList()) | ||
| expected := []string{"PATH", "HOME"} | ||
| assert.DeepEqual(t, blockList, expected) | ||
| }) | ||
|
|
||
| t.Run("additive blocklist", func(t *testing.T) { | ||
| t.Setenv("LIMA_SHELLENV_BLOCK", "+CUSTOM_VAR") | ||
|
|
||
| blockList := getBlockList() | ||
| assert.Assert(t, isUsingDefaultBlockList()) | ||
| expected := slices.Concat(GetDefaultBlockList(), []string{"CUSTOM_VAR"}) | ||
| assert.DeepEqual(t, blockList, expected) | ||
| }) | ||
|
|
||
| t.Run("allowlist", func(t *testing.T) { | ||
| t.Setenv("LIMA_SHELLENV_ALLOW", "FOO,BAR") | ||
|
|
||
| allowList := getAllowList() | ||
| expected := []string{"FOO", "BAR"} | ||
| assert.DeepEqual(t, allowList, expected) | ||
| }) | ||
| } | ||
|
|
||
| func TestFilterEnvironment(t *testing.T) { | ||
| testEnv := []string{ | ||
| "PATH=/usr/bin", | ||
| "HOME=/home/user", | ||
| "USER=testuser", | ||
| "FOO=bar", | ||
| "SSH_AUTH_SOCK=/tmp/ssh", | ||
| "XDG_CONFIG_HOME=/config", | ||
| "BASH_VERSION=5.0", | ||
| "_INTERNAL=secret", | ||
| "CUSTOM_VAR=value", | ||
| } | ||
|
|
||
| t.Run("default blocklist", func(t *testing.T) { | ||
| result := filterEnvironmentWithLists(testEnv, nil, defaultBlockList) | ||
|
|
||
| expected := []string{"FOO=bar", "CUSTOM_VAR=value"} | ||
| assert.Assert(t, containsAll(result, expected)) | ||
|
|
||
| blockedPrefixes := []string{ | ||
| "PATH=", | ||
| "HOME=", | ||
| "SSH_AUTH_SOCK=", | ||
| "XDG_CONFIG_HOME=", | ||
| "BASH_VERSION=", | ||
| "_INTERNAL=", | ||
| } | ||
| for _, prefix := range blockedPrefixes { | ||
| for _, envVar := range result { | ||
| assert.Assert(t, !strings.HasPrefix(envVar, prefix), "Expected result to not contain variable with prefix %q, but found %q", prefix, envVar) | ||
| } | ||
| } | ||
| }) | ||
|
|
||
| t.Run("custom blocklist", func(t *testing.T) { | ||
| result := filterEnvironmentWithLists(testEnv, nil, []string{"FOO"}) | ||
|
|
||
| assert.Assert(t, !slices.Contains(result, "FOO=bar")) | ||
|
|
||
| expected := []string{"PATH=/usr/bin", "HOME=/home/user", "USER=testuser"} | ||
| assert.Assert(t, containsAll(result, expected)) | ||
| }) | ||
|
|
||
| t.Run("allowlist", func(t *testing.T) { | ||
| result := filterEnvironmentWithLists(testEnv, []string{"FOO", "USER"}, nil) | ||
|
|
||
| expected := []string{"FOO=bar", "USER=testuser"} | ||
| assert.Equal(t, len(result), len(expected)) | ||
| assert.Assert(t, containsAll(result, expected)) | ||
| }) | ||
|
|
||
| t.Run("allowlist takes precedence over blocklist", func(t *testing.T) { | ||
| result := filterEnvironmentWithLists(testEnv, []string{"FOO", "CUSTOM_VAR"}, []string{"FOO", "USER"}) | ||
|
|
||
| expected := []string{"FOO=bar", "CUSTOM_VAR=value"} | ||
| assert.Assert(t, containsAll(result, expected)) | ||
|
|
||
| assert.Assert(t, !slices.Contains(result, "USER=testuser")) | ||
| }) | ||
| } | ||
jandubois marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| func containsAll(slice, items []string) bool { | ||
| for _, item := range items { | ||
| if !slices.Contains(slice, item) { | ||
| return false | ||
| } | ||
| } | ||
| return true | ||
| } | ||
|
|
||
| func TestGetDefaultBlockList(t *testing.T) { | ||
| blocklist := GetDefaultBlockList() | ||
|
|
||
olamilekan000 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| if &blocklist[0] == &defaultBlockList[0] { | ||
| t.Error("GetDefaultBlockList should return a copy, not the original slice") | ||
| } | ||
|
|
||
| assert.DeepEqual(t, blocklist, defaultBlockList) | ||
|
|
||
| expectedItems := []string{"PATH", "HOME", "SSH_*"} | ||
| for _, item := range expectedItems { | ||
| found := slices.Contains(blocklist, item) | ||
| assert.Assert(t, found, "Expected builtin blocklist to contain %q", item) | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needs an integration test and the document to clarify the block list