Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 19 additions & 1 deletion cmd/limactl/shell.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ import (
"github.com/sirupsen/logrus"
"github.com/spf13/cobra"

"github.com/lima-vm/lima/v2/pkg/envutil"
"github.com/lima-vm/lima/v2/pkg/instance"
"github.com/lima-vm/lima/v2/pkg/ioutilx"
"github.com/lima-vm/lima/v2/pkg/limayaml"
Expand Down Expand Up @@ -54,6 +55,7 @@ func newShellCommand() *cobra.Command {
shellCmd.Flags().String("shell", "", "Shell interpreter, e.g. /bin/bash")
shellCmd.Flags().String("workdir", "", "Working directory")
shellCmd.Flags().Bool("reconnect", false, "Reconnect to the SSH session")
shellCmd.Flags().Bool("preserve-env", false, "Propagate environment variables to the shell")
Copy link
Member

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

return shellCmd
}

Expand Down Expand Up @@ -178,7 +180,23 @@ func shellAction(cmd *cobra.Command, args []string) error {
} else {
shell = shellescape.Quote(shell)
}
script := fmt.Sprintf("%s ; exec %s --login", changeDirCmd, shell)
// Handle environment variable propagation
var envPrefix string
preserveEnv, err := cmd.Flags().GetBool("preserve-env")
if err != nil {
return err
}
if preserveEnv {
filteredEnv := envutil.FilterEnvironment()
if len(filteredEnv) > 0 {
envPrefix = "env "
for _, envVar := range filteredEnv {
envPrefix += shellescape.Quote(envVar) + " "
}
}
}

script := fmt.Sprintf("%s ; exec %s%s --login", changeDirCmd, envPrefix, shell)
if len(args) > 1 {
quotedArgs := make([]string, len(args[1:]))
parsingEnv := true
Expand Down
2 changes: 1 addition & 1 deletion cmd/nerdctl.lima
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about docker.lima, etc. ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was an oversight. I only mentioned nerdctl.lima because it always runs in the VM, but didn't think about docker.lima because I always have a local client.

So yes, docker.lima needs it too. I don't know about the others, but maybe we should add it to all of them.

In some ways you could argue that --preserve-env should be the default unless the instance is --plain, but I don't think we want to make this change because it is not backwards compatible (and having it work differently based on instance type is confusing).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

--preserve-env should be the default

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 "$@"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs a comment to explain the reason

135 changes: 135 additions & 0 deletions pkg/envutil/envutil.go
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",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And *PATH ? (e.g., GOPATH)

Copy link
Member

Choose a reason for hiding this comment

The 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 *), but would require expanded syntax, like suggested in #3852.

I'm not sure we should block *PATH by default, I think the rule should be to block all variables that would be incorrect inside the VM (which is why I was hesitant to include the BASH* and ZSH* ones). So we don't want to override settings that the VM shell would normally provide.

Is there a reason that the GOPATH should not be exported into the VM? It would allow you to take advantage of the package cache on the host.

"PWD",
"SHELL",
"SHLVL",
"SSH_*",
"TERM",
"TERMINFO",
"TMPDIR",
"UID",
"USER",
"XAUTHORITY",
"XDG_*",
"ZDOTDIR",
"ZSH*",
"_*", // Variables starting with underscore are typically internal
}
Copy link
Member

@jandubois jandubois Aug 14, 2025

Choose a reason for hiding this comment

The 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:

LD_*
DYLD_*
SHLVL
UID
EUID
GROUP
GID
HOSTNAME

Edit: I had LINES and COLUMNS in the list originally, but changed my mind and removed them.

Not sure about excluding shell-specific variables:

BASH*
ZSH*
FPATH
ZDOTDIR

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was this list taken from somewhere else?
Where is the upstream of this list?

Copy link
Member

Choose a reason for hiding this comment

The 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 BASH* and ZSH* blocks.


func getBlockList() []string {
blockEnv := os.Getenv("LIMA_SHELLENV_BLOCK")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 {
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
}

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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we move this function to the top, right after the var defaultBlockList?

Usually, unexported functions go below exported ones.

Copy link
Member

Choose a reason for hiding this comment

The 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.

210 changes: 210 additions & 0 deletions pkg/envutil/envutil_test.go
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, "+")
}

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"))
})
}

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()

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)
}
}
Loading
Loading