diff --git a/changelog/fragments/1706123313-on-Windows-prevent-uninstall-from-within-installed-directory.yaml b/changelog/fragments/1706123313-on-Windows-prevent-uninstall-from-within-installed-directory.yaml new file mode 100644 index 00000000000..5f4673be326 --- /dev/null +++ b/changelog/fragments/1706123313-on-Windows-prevent-uninstall-from-within-installed-directory.yaml @@ -0,0 +1,32 @@ +# Kind can be one of: +# - breaking-change: a change to previously-documented behavior +# - deprecation: functionality that is being removed in a later release +# - bug-fix: fixes a problem in a previous version +# - enhancement: extends functionality but does not break or fix existing behavior +# - feature: new functionality +# - known-issue: problems that we are aware of in a given version +# - security: impacts on the security of a product or a user’s deployment. +# - upgrade: important information for someone upgrading from a prior version +# - other: does not fit into any of the other categories +kind: bug-fix + +# Change summary; a 80ish characters long description of the change. +summary: On Windows prevent uninstall from within installed directory + +# Long description; in case the summary is not enough to describe the change +# this field accommodate a description without length limits. +# NOTE: This field will be rendered only for breaking-change and known-issue kinds at the moment. +description: 'On Windows, prevent uninstall from within installed directory to prevent a sharing violation. Also for install and uninstall, print debug logs to stderr if command fails.' + +# Affected component; a word indicating the component this changeset affects. +component: agent + +# PR URL; optional; the PR number that added the changeset. +# If not present is automatically filled by the tooling finding the PR where this changelog fragment has been added. +# NOTE: the tooling supports backports, so it's able to fill the original PR number instead of the backport PR number. +# Please provide it if you are adding a fragment for a different PR. +#pr: https://github.com/owner/repo/1234 + +# Issue URL; optional; the GitHub issue related to this changeset (either closes or is part of). +# If not present is automatically filled by the tooling with the issue linked to the PR number. +#issue: https://github.com/owner/repo/1234 diff --git a/internal/pkg/agent/application/paths/common.go b/internal/pkg/agent/application/paths/common.go index 61ff0bcdc49..c4492a2aa9c 100644 --- a/internal/pkg/agent/application/paths/common.go +++ b/internal/pkg/agent/application/paths/common.go @@ -326,3 +326,17 @@ func ControlSocketFromPath(platform string, path string) string { // for it to be used, but needs to be unique per Agent (in the case that multiple are running) return utils.SocketURLWithFallback(socketPath, path) } + +func pathSplit(path string) []string { + dir, file := filepath.Split(path) + if dir == "" && file == "" { + return []string{} + } + if dir == "" && file != "" { + return []string{file} + } + if dir == path { + return []string{} + } + return append(pathSplit(filepath.Clean(dir)), file) +} diff --git a/internal/pkg/agent/application/paths/common_test.go b/internal/pkg/agent/application/paths/common_test.go index 27a9cf80ebd..4f9eeaafb47 100644 --- a/internal/pkg/agent/application/paths/common_test.go +++ b/internal/pkg/agent/application/paths/common_test.go @@ -91,3 +91,55 @@ func TestExecDir(t *testing.T) { }) } } + +func TestPathSplitUnix(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("Skipping Unix tests on Windows") + } + tests := map[string]struct { + path string + want []string + }{ + "empty string": {path: "", want: []string{}}, + "just file": {path: "test.txt", want: []string{"test.txt"}}, + "just dir": {path: "/", want: []string{}}, + "top dir": {path: "/test.txt", want: []string{"test.txt"}}, + "simple": {path: "/a/b", want: []string{"a", "b"}}, + "long": {path: "/a/b c/d-e/f_g", want: []string{"a", "b c", "d-e", "f_g"}}, + } + + for name, tc := range tests { + t.Run(name, func(t *testing.T) { + got := pathSplit(tc.path) + if !cmp.Equal(tc.want, got) { + t.Fatalf("not equal got: %v, want: %v", got, tc.want) + } + }) + } +} + +func TestPathSplitWindows(t *testing.T) { + if runtime.GOOS != "windows" { + t.Skip("Skipping Windows tests on non Windows") + } + tests := map[string]struct { + path string + want []string + }{ + "empty string": {path: "", want: []string{}}, + "just file": {path: "test.txt", want: []string{"test.txt"}}, + "just dir": {path: "C:\\", want: []string{}}, + "top dir": {path: "C:\\test.txt", want: []string{"test.txt"}}, + "simple": {path: "C:\\a\\b", want: []string{"a", "b"}}, + "long": {path: "C:\\a\\b c\\d-e\\f_g", want: []string{"a", "b c", "d-e", "f_g"}}, + } + + for name, tc := range tests { + t.Run(name, func(t *testing.T) { + got := pathSplit(tc.path) + if !cmp.Equal(tc.want, got) { + t.Fatalf("not equal got: %v, want: %v", got, tc.want) + } + }) + } +} diff --git a/internal/pkg/agent/application/paths/paths_test.go b/internal/pkg/agent/application/paths/paths_test.go index 2fd7584eb13..7e0979e8371 100644 --- a/internal/pkg/agent/application/paths/paths_test.go +++ b/internal/pkg/agent/application/paths/paths_test.go @@ -31,3 +31,62 @@ func TestEqual(t *testing.T) { }) } } + +func TestHasPrefixUnix(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("Skipping unix HasPrefix tests on Windows host") + } + tests := map[string]struct { + path string + prefix string + want bool + }{ + "simple true": {path: "/a/b", prefix: "/a", want: true}, + "just root": {path: "/", prefix: "/", want: true}, + "root one dir": {path: "/a", prefix: "/", want: true}, + "simple false": {path: "/a/b", prefix: "/c/d", want: false}, + "prefix too long": {path: "/a/b", prefix: "/a/b/c/d", want: false}, + "trailing slash": {path: "/a/b/", prefix: "/a", want: true}, + "no path": {path: "", prefix: "/a", want: false}, + "no prefix": {path: "/a/b", prefix: "", want: false}, + } + + for name, tc := range tests { + t.Run(name, func(t *testing.T) { + got := HasPrefix(tc.path, tc.prefix) + if got != tc.want { + t.Fatalf("got %v, expected %v", got, tc.want) + } + }) + } +} + +func TestHasPrefixWindows(t *testing.T) { + if runtime.GOOS != "windows" { + t.Skip("Skipping windows HasPrefix tests on non Windows host") + } + tests := map[string]struct { + path string + prefix string + want bool + }{ + "simple true": {path: "c:\\a\\b", prefix: "c:\\a", want: true}, + "just root": {path: "c:\\", prefix: "c:\\", want: true}, + "root one dir": {path: "c:\\a", prefix: "c:\\", want: true}, + "simple false": {path: "c:\\a\\b", prefix: "c:\\c\\d", want: false}, + "prefix too long": {path: "c:\\a\\b", prefix: "c:\\a\\b\\c\\d", want: false}, + "trailing slash": {path: "c:\\a\\b\\", prefix: "c:\\a", want: true}, + "no path": {path: "", prefix: "c:\\a", want: false}, + "no prefix": {path: "c:\\a\\b", prefix: "", want: false}, + "case insensitive": {path: "C:\\A\\B", prefix: "c:\\a", want: true}, + } + + for name, tc := range tests { + t.Run(name, func(t *testing.T) { + got := HasPrefix(tc.path, tc.prefix) + if got != tc.want { + t.Fatalf("got %v, expected %v", got, tc.want) + } + }) + } +} diff --git a/internal/pkg/agent/application/paths/paths_unix.go b/internal/pkg/agent/application/paths/paths_unix.go index da905a4bb84..99d8cbe1ad5 100644 --- a/internal/pkg/agent/application/paths/paths_unix.go +++ b/internal/pkg/agent/application/paths/paths_unix.go @@ -7,6 +7,7 @@ package paths import ( + "path/filepath" "runtime" ) @@ -16,3 +17,28 @@ func initialControlSocketPath(topPath string) string { // ResolveControlSocket does nothing on non-Windows hosts. func ResolveControlSocket() {} + +// HasPrefix tests if the path starts with the prefix. +func HasPrefix(path string, prefix string) bool { + if path == "" || prefix == "" { + return false + } + + if filepath.VolumeName(path) != filepath.VolumeName(prefix) { + return false + } + + prefixParts := pathSplit(filepath.Clean(prefix)) + pathParts := pathSplit(filepath.Clean(path)) + + if len(prefixParts) > len(pathParts) { + return false + } + + for i := 0; i < len(prefixParts); i++ { + if prefixParts[i] != pathParts[i] { + return false + } + } + return true +} diff --git a/internal/pkg/agent/application/paths/paths_windows.go b/internal/pkg/agent/application/paths/paths_windows.go index a6f7b14afe7..1f6c7dc231b 100644 --- a/internal/pkg/agent/application/paths/paths_windows.go +++ b/internal/pkg/agent/application/paths/paths_windows.go @@ -67,3 +67,28 @@ func ResolveControlSocket() { SetControlSocket(WindowsControlSocketInstalledPath) } } + +// HasPrefix tests if the path starts with the prefix. +func HasPrefix(path string, prefix string) bool { + if path == "" || prefix == "" { + return false + } + + if !strings.EqualFold(filepath.VolumeName(path), filepath.VolumeName(prefix)) { + return false + } + + prefixParts := pathSplit(filepath.Clean(prefix)) + pathParts := pathSplit(filepath.Clean(path)) + + if len(prefixParts) > len(pathParts) { + return false + } + + for i := 0; i < len(prefixParts); i++ { + if !strings.EqualFold(prefixParts[0], pathParts[0]) { + return false + } + } + return true +} diff --git a/internal/pkg/agent/cmd/install.go b/internal/pkg/agent/cmd/install.go index 7a5ee5cacc9..7b4c40e7a27 100644 --- a/internal/pkg/agent/cmd/install.go +++ b/internal/pkg/agent/cmd/install.go @@ -14,10 +14,12 @@ import ( "github.com/spf13/cobra" + "github.com/elastic/elastic-agent-libs/logp" "github.com/elastic/elastic-agent/internal/pkg/agent/application/filelock" "github.com/elastic/elastic-agent/internal/pkg/agent/application/paths" "github.com/elastic/elastic-agent/internal/pkg/agent/install" "github.com/elastic/elastic-agent/internal/pkg/cli" + "github.com/elastic/elastic-agent/pkg/core/logger" "github.com/elastic/elastic-agent/pkg/utils" ) @@ -54,7 +56,9 @@ would like the Agent to operate. } func installCmd(streams *cli.IOStreams, cmd *cobra.Command) error { - err := validateEnrollFlags(cmd) + var err error + + err = validateEnrollFlags(cmd) if err != nil { return fmt.Errorf("could not validate flags: %w", err) } @@ -189,10 +193,34 @@ func installCmd(streams *cli.IOStreams, cmd *cobra.Command) error { progBar := install.CreateAndStartNewSpinner(streams.Out, "Installing Elastic Agent...") + logCfg := logp.DefaultConfig(logp.DefaultEnvironment) + logCfg.Level = logp.DebugLevel + // Using in memory logger, so we don't write logs to the + // directory we are trying to delete + logp.ToObserverOutput()(&logCfg) + + err = logp.Configure(logCfg) + if err != nil { + return fmt.Errorf("error creating logging config: %w", err) + } + + log := logger.NewWithoutConfig("") + + defer func() { + if err == nil { + return + } + oLogs := logp.ObserverLogs().TakeAll() + fmt.Fprintf(os.Stderr, "Error uninstalling. Printing logs\n") + for _, oLog := range oLogs { + fmt.Fprintf(os.Stderr, "%v\n", oLog.Entry) + } + }() + var ownership utils.FileOwner cfgFile := paths.ConfigFile() if status != install.PackageInstall { - ownership, err = install.Install(cfgFile, topPath, unprivileged, progBar, streams) + ownership, err = install.Install(cfgFile, topPath, unprivileged, log, progBar, streams) if err != nil { return fmt.Errorf("error installing package: %w", err) } @@ -200,7 +228,7 @@ func installCmd(streams *cli.IOStreams, cmd *cobra.Command) error { defer func() { if err != nil { progBar.Describe("Uninstalling") - innerErr := install.Uninstall(cfgFile, topPath, "", progBar) + innerErr := install.Uninstall(cfgFile, topPath, "", log, progBar) if innerErr != nil { progBar.Describe("Failed to Uninstall") } else { diff --git a/internal/pkg/agent/cmd/uninstall.go b/internal/pkg/agent/cmd/uninstall.go index 34d07c51683..d14c2b51840 100644 --- a/internal/pkg/agent/cmd/uninstall.go +++ b/internal/pkg/agent/cmd/uninstall.go @@ -10,9 +10,11 @@ import ( "github.com/spf13/cobra" + "github.com/elastic/elastic-agent-libs/logp" "github.com/elastic/elastic-agent/internal/pkg/agent/application/paths" "github.com/elastic/elastic-agent/internal/pkg/agent/install" "github.com/elastic/elastic-agent/internal/pkg/cli" + "github.com/elastic/elastic-agent/pkg/core/logger" "github.com/elastic/elastic-agent/pkg/utils" ) @@ -39,6 +41,8 @@ Unless -f is used this command will ask confirmation before performing removal. } func uninstallCmd(streams *cli.IOStreams, cmd *cobra.Command) error { + var err error + isAdmin, err := utils.HasRoot() if err != nil { return fmt.Errorf("unable to perform command while checking for administrator rights, %w", err) @@ -81,7 +85,31 @@ func uninstallCmd(streams *cli.IOStreams, cmd *cobra.Command) error { progBar := install.CreateAndStartNewSpinner(streams.Out, "Uninstalling Elastic Agent...") - err = install.Uninstall(paths.ConfigFile(), paths.Top(), uninstallToken, progBar) + logCfg := logp.DefaultConfig(logp.DefaultEnvironment) + logCfg.Level = logp.DebugLevel + // Using in memory logger, so we don't write logs to the + // directory we are trying to delete + logp.ToObserverOutput()(&logCfg) + + err = logp.Configure(logCfg) + if err != nil { + return fmt.Errorf("error creating logging config: %w", err) + } + + log := logger.NewWithoutConfig("") + + defer func() { + if err == nil { + return + } + oLogs := logp.ObserverLogs().TakeAll() + fmt.Fprintf(os.Stderr, "Error uninstalling. Printing logs\n") + for _, oLog := range oLogs { + fmt.Fprintf(os.Stderr, "%v\n", oLog.Entry) + } + }() + + err = install.Uninstall(paths.ConfigFile(), paths.Top(), uninstallToken, log, progBar) if err != nil { progBar.Describe("Failed to uninstall agent") return fmt.Errorf("error uninstalling agent: %w", err) diff --git a/internal/pkg/agent/install/install.go b/internal/pkg/agent/install/install.go index bc5fa26a148..2ff0ef7dacc 100644 --- a/internal/pkg/agent/install/install.go +++ b/internal/pkg/agent/install/install.go @@ -16,6 +16,7 @@ import ( "github.com/otiai10/copy" "github.com/schollz/progressbar/v3" + "github.com/elastic/elastic-agent-libs/logp" "github.com/elastic/elastic-agent/internal/pkg/agent/application/paths" "github.com/elastic/elastic-agent/internal/pkg/agent/errors" "github.com/elastic/elastic-agent/internal/pkg/cli" @@ -30,7 +31,7 @@ const ( ) // Install installs Elastic Agent persistently on the system including creating and starting its service. -func Install(cfgFile, topPath string, unprivileged bool, pt *progressbar.ProgressBar, streams *cli.IOStreams) (utils.FileOwner, error) { +func Install(cfgFile, topPath string, unprivileged bool, log *logp.Logger, pt *progressbar.ProgressBar, streams *cli.IOStreams) (utils.FileOwner, error) { dir, err := findDirectory() if err != nil { return utils.FileOwner{}, errors.New(err, "failed to discover the source directory for installation", errors.TypeFilesystem) @@ -45,7 +46,7 @@ func Install(cfgFile, topPath string, unprivileged bool, pt *progressbar.Progres // Uninstall will fail on protected agent. // The protected Agent will need to be uninstalled first before it can be installed. pt.Describe("Uninstalling current Elastic Agent") - err = Uninstall(cfgFile, topPath, "", pt) + err = Uninstall(cfgFile, topPath, "", log, pt) if err != nil { pt.Describe("Failed to uninstall current Elastic Agent") return utils.FileOwner{}, errors.New( diff --git a/internal/pkg/agent/install/uninstall.go b/internal/pkg/agent/install/uninstall.go index 8f2234d0f4d..216ee898035 100644 --- a/internal/pkg/agent/install/uninstall.go +++ b/internal/pkg/agent/install/uninstall.go @@ -33,7 +33,15 @@ import ( ) // Uninstall uninstalls persistently Elastic Agent on the system. -func Uninstall(cfgFile, topPath, uninstallToken string, pt *progressbar.ProgressBar) error { +func Uninstall(cfgFile, topPath, uninstallToken string, log *logp.Logger, pt *progressbar.ProgressBar) error { + cwd, err := os.Getwd() + if err != nil { + return fmt.Errorf("unable to get current working directory") + } + + if runtime.GOOS == "windows" && paths.HasPrefix(cwd, topPath) { + return fmt.Errorf("uninstall must be run from outside the installed path '%s'", topPath) + } // uninstall the current service // not creating the service, so no need to set the username and group to any value svc, err := newService(topPath) @@ -61,7 +69,7 @@ func Uninstall(cfgFile, topPath, uninstallToken string, pt *progressbar.Progress } // Uninstall components first - if err := uninstallComponents(context.Background(), cfgFile, uninstallToken, pt); err != nil { + if err := uninstallComponents(context.Background(), cfgFile, uninstallToken, log, pt); err != nil { // If service status was running it was stopped to uninstall the components. // If the components uninstall failed start the service again if status == service.StatusRunning { @@ -180,11 +188,7 @@ func containsString(str string, a []string, caseSensitive bool) bool { return false } -func uninstallComponents(ctx context.Context, cfgFile string, uninstallToken string, pt *progressbar.ProgressBar) error { - log, err := logger.NewWithLogpLevel("", logp.ErrorLevel, false) - if err != nil { - return fmt.Errorf("error creating logger: %w", err) - } +func uninstallComponents(ctx context.Context, cfgFile string, uninstallToken string, log *logp.Logger, pt *progressbar.ProgressBar) error { platform, err := component.LoadPlatformDetail() if err != nil { @@ -217,7 +221,7 @@ func uninstallComponents(ctx context.Context, cfgFile string, uninstallToken str } // Need to read the features from config on uninstall, in order to set the tamper protection feature flag correctly - if err := features.Apply(cfg); err != nil { + if err = features.Apply(cfg); err != nil { return fmt.Errorf("could not parse and apply feature flags config: %w", err) } @@ -234,7 +238,7 @@ func uninstallComponents(ctx context.Context, cfgFile string, uninstallToken str // This component is not active continue } - if err := uninstallServiceComponent(ctx, log, comp, uninstallToken, pt); err != nil { + if err = uninstallServiceComponent(ctx, log, comp, uninstallToken, pt); err != nil { os.Stderr.WriteString(fmt.Sprintf("failed to uninstall component %q: %s\n", comp.ID, err)) // The decision was made to change the behaviour and leave the Agent installed if Endpoint uninstall fails // https://github.com/elastic/elastic-agent/pull/2708#issuecomment-1574251911 diff --git a/testing/integration/install_test.go b/testing/integration/install_test.go index 40697b15e9c..43afd1fc2bc 100644 --- a/testing/integration/install_test.go +++ b/testing/integration/install_test.go @@ -75,6 +75,20 @@ func TestInstallWithoutBasePath(t *testing.T) { // Check that Agent was installed in default base path checkInstallSuccess(t, topPath, opts.IsUnprivileged(runtime.GOOS)) t.Run("check agent package version", testAgentPackageVersion(ctx, fixture, true)) + // Make sure uninstall from within the topPath fails on Windows + if runtime.GOOS == "windows" { + cwd, err := os.Getwd() + require.NoErrorf(t, err, "GetWd failed: %s", err) + err = os.Chdir(topPath) + require.NoErrorf(t, err, "Chdir to topPath failed: %s", err) + t.Cleanup(func() { + _ = os.Chdir(cwd) + }) + out, err = fixture.Uninstall(ctx, &atesting.UninstallOpts{Force: true}) + require.Error(t, err, "uninstall should have failed") + require.Containsf(t, string(out), "uninstall must be run from outside the installed path", "expected error string not found in: %s err: %s", out, err) + } + } func TestInstallWithBasePath(t *testing.T) { @@ -138,6 +152,19 @@ func TestInstallWithBasePath(t *testing.T) { topPath := filepath.Join(basePath, "Elastic", "Agent") checkInstallSuccess(t, topPath, opts.IsUnprivileged(runtime.GOOS)) t.Run("check agent package version", testAgentPackageVersion(ctx, fixture, true)) + // Make sure uninstall from within the topPath fails on Windows + if runtime.GOOS == "windows" { + cwd, err := os.Getwd() + require.NoErrorf(t, err, "GetWd failed: %s", err) + err = os.Chdir(topPath) + require.NoErrorf(t, err, "Chdir to topPath failed: %s", err) + t.Cleanup(func() { + _ = os.Chdir(cwd) + }) + out, err = fixture.Uninstall(ctx, &atesting.UninstallOpts{Force: true}) + require.Error(t, err, "uninstall should have failed") + require.Containsf(t, string(out), "uninstall must be run from outside the installed path", "expected error string not found in: %s err: %s", out, err) + } } func checkInstallSuccess(t *testing.T, topPath string, unprivileged bool) {