Skip to content

Commit

Permalink
fix(wsl-pro-service): Do not translate empty paths (#858)
Browse files Browse the repository at this point in the history
I wanted this PR to be laser-focus, just a bugfix, thus I'm not
refactoring the executable-mocking just yet.

The issue is: when we invoke `wslpath -au ''` we get the absolute path
of the current working directory.
That's surely not what we expect of a Windows path.

It could be problematic if the user passes a custom Landscape config
file containing this line `ssl_public_key= `, as we'd modify the final
client.conf into `ssl_public_key = /` (systemd makes it so our current
working directory is `/`). Landscape client would tolerate the empty
value in the INI sintax, but this would force it to try to read `/` as a
certificate file. Clearly not what we want.

A similar thing could happen in `system.UserProfileDir()`, but that's
even funnier. Having %USERPROFILE% set to an empty string is indeed an
odd edge case, so I'm not 100% sure the added code is worth it, as we're
likely to err out attempting to open inexistant directories. I think it
worths it because we are sure on how we err out. Leaving as it was could
lead us into reading files we were not supposed to.

They issues were:

`echo %USERPROFILE%` would output `ECHO is on` if that environment
variable is empty. We fix it by running `echo.%USERPROFILE%` instead. It
outputs an empty line for that case. Finally, if the output is empty, we
bail out.


![image](https://github.com/user-attachments/assets/1fceae7f-3985-4148-b6e5-60a014068621)

I made some modifications in the wslpath and cmd.exe mocks to get closer
to the surprising behavior, so I could add test cases
demonstrating the surprising behavior.

---
Closes UDENG-3788
  • Loading branch information
CarlosNihelton authored Aug 7, 2024
2 parents a573e39 + 3c87442 commit 1d4bac6
Show file tree
Hide file tree
Showing 6 changed files with 50 additions and 14 deletions.
5 changes: 5 additions & 0 deletions wsl-pro-service/internal/system/landscape.go
Original file line number Diff line number Diff line change
Expand Up @@ -170,6 +170,11 @@ func overrideSSLCertificate(ctx context.Context, s *System, section *ini.Section

pathWindows := k.String()

if len(pathWindows) == 0 {
// Empty paths are translated by wslpath as the current working directory, which is not what we want.
return nil
}

cmd := s.backend.WslpathExecutable(ctx, "-ua", pathWindows)
out, err := runCommand(cmd)
if err != nil {
Expand Down
10 changes: 8 additions & 2 deletions wsl-pro-service/internal/system/system.go
Original file line number Diff line number Diff line change
Expand Up @@ -184,16 +184,22 @@ func (s *System) UserProfileDir(ctx context.Context) (wslPath string, err error)
return wslPath, err
}

cmd := s.backend.CmdExe(ctx, cmdExe, "/C", "echo %UserProfile%")
// Using the 'echo.' syntax instead of 'echo ' because if %USERPROFILE% was set to empty string it would cause the output to be 'ECHO is on'.
// With 'echo.%UserProfile%' it correctly prints empty line in that case.
cmd := s.backend.CmdExe(ctx, cmdExe, "/C", "echo.%UserProfile%")
winHome, err := runCommand(cmd)
if err != nil {
return wslPath, err
}

trimmed := strings.TrimSpace(string(winHome))
if len(trimmed) == 0 {
return wslPath, errors.New("%UserProfile% value is empty")
}
// We have the path from Windows' perspective ( C:\Users\... )
// It must be converted to linux ( /mnt/c/Users/... )

cmd = s.backend.WslpathExecutable(ctx, "-ua", string(winHome))
cmd = s.backend.WslpathExecutable(ctx, "-ua", trimmed)
winHomeLinux, err := runCommand(cmd)
if err != nil {
return wslPath, err
Expand Down
20 changes: 13 additions & 7 deletions wsl-pro-service/internal/system/system_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -199,12 +199,13 @@ func TestUserProfileDir(t *testing.T) {
t.Parallel()

testCases := map[string]struct {
cachedCmdExe bool
cmdExeNotExist bool
cmdExeErr bool
wslpathErr bool
wslpathBadOutput bool
overrideProcMount bool
cachedCmdExe bool
cmdExeNotExist bool
cmdExeErr bool
emptyUserprofileEnvVar bool
wslpathErr bool
wslpathBadOutput bool
overrideProcMount bool

wantErr bool
}{
Expand All @@ -218,6 +219,7 @@ func TestUserProfileDir(t *testing.T) {
"Error finding cmd.exe because there is no Windows FS in /proc/mounts": {wantErr: true, overrideProcMount: true},
"Error when cmd.exe does not exist": {cmdExeNotExist: true, overrideProcMount: true, wantErr: true},
"Error on cmd.exe error": {cmdExeErr: true, wantErr: true},
"Error when UserProfile env var is empty": {emptyUserprofileEnvVar: true, wantErr: true},
"Error on wslpath error": {wslpathErr: true, wantErr: true},
"Error when wslpath returns a bad path": {wslpathBadOutput: true, wantErr: true},
}
Expand All @@ -230,6 +232,9 @@ func TestUserProfileDir(t *testing.T) {
if tc.cmdExeErr {
mock.SetControlArg(testutils.CmdExeErr)
}
if tc.emptyUserprofileEnvVar {
mock.SetControlArg(testutils.EmptyUserprofileEnvVar)
}
if tc.wslpathErr {
mock.SetControlArg(testutils.WslpathErr)
}
Expand All @@ -251,7 +256,7 @@ func TestUserProfileDir(t *testing.T) {

got, err := system.UserProfileDir(context.Background())
if tc.wantErr {
require.Error(t, err, "Expected UserProfile to return an error")
require.Error(t, err, "Expected UserProfile to return an error, but returned %s intead", got)
return
}
require.NoError(t, err, "Expected UserProfile to return no errors")
Expand Down Expand Up @@ -669,6 +674,7 @@ func TestEnsureValidLandscapeConfig(t *testing.T) {
"Appends any required fields - no ssl": {systemLandscapeConfigFile: "minimal.conf"},
"Transform Windows SSL certificate path": {systemLandscapeConfigFile: "windows_ssl_only.conf"},
"Transform Windows SSL certificate path with forward slash": {systemLandscapeConfigFile: "windows_ssl_only_forward_slash.conf"},
"Do not transform Windows SSL certificate empty path": {systemLandscapeConfigFile: "windows_ssl_empty.conf", wantNoLandscapeConfigCmd: true},
"Refresh computer_title if changed": {systemLandscapeConfigFile: "old_computer_title.conf"},

"Regular with additional keys": {systemLandscapeConfigFile: "regular.conf"},
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
[client]
hostagent_uid = landscapeUID1234
ssl_public_key =
computer_title = TEST_DISTRO
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
[client]
hostagent_uid = landscapeUID1234
ssl_public_key =
computer_title = TEST_DISTRO
21 changes: 16 additions & 5 deletions wsl-pro-service/internal/testutils/mock_executables.go
Original file line number Diff line number Diff line change
Expand Up @@ -86,8 +86,9 @@ const (
LandscapeEnableErr = "UP4W_LANDSCAPE_ENABLE_ERR"
LandscapeDisableErr = "UP4W_LANDSCAPE_DISABLE_ERR"

WslpathErr = "UP4W_WSLPATH_ERR"
WslpathBadOutput = "UP4W_WSLPATH_BAD_OUTPUT"
WslpathErr = "UP4W_WSLPATH_ERR"
WslpathBadOutput = "UP4W_WSLPATH_BAD_OUTPUT"
EmptyUserprofileEnvVar = "UP4W_EMPTY_USERPROFILE_ENV_VAR"

CmdExeErr = "UP4W_CMDEXE_ERR"

Expand Down Expand Up @@ -511,12 +512,18 @@ func WslPathMock(t *testing.T) {
if envExists(WslpathErr) {
return exitError
}
// That's what wslpath returns when it's called with -ua followed by an empty string.
cwd, err := os.Getwd()
if err != nil {
fmt.Fprintf(os.Stderr, "Could not get current working directory: %v", err)
}

stdout, ok := map[string]string{
windowsUserProfileDir: linuxUserProfileDir,
`D:\Users\TestUser\certificate`: filepath.Join(defaultWindowsMount, "Users/TestUser/certificate"),
`D:/Users/TestUser/certificate`: filepath.Join(defaultWindowsMount, "Users/TestUser/certificate"),
`/idempotent/path/to/linux/certificate`: `/idempotent/path/to/linux/certificate`,
"D:/Users/TestUser/certificate": filepath.Join(defaultWindowsMount, "Users/TestUser/certificate"),
"/idempotent/path/to/linux/certificate": "/idempotent/path/to/linux/certificate",
"": cwd,
}[argv[1]]

if !ok {
Expand Down Expand Up @@ -602,14 +609,18 @@ func CmdExeMock(t *testing.T) {
return exitBadUsage
}

if argv[1] != "echo %UserProfile%" {
if argv[1] != "echo.%UserProfile%" {
fmt.Fprintf(os.Stderr, "Mock not implemented for args %q\n", argv)
return exitBadUsage
}

if envExists(CmdExeErr) {
return exitError
}
if envExists(EmptyUserprofileEnvVar) {
fmt.Print("\r\n")
return exitOk
}

fmt.Fprintln(os.Stdout, windowsUserProfileDir)
return exitOk
Expand Down

0 comments on commit 1d4bac6

Please sign in to comment.