From 1025db6ba70792e9c35198dd419910c417b2a8ea Mon Sep 17 00:00:00 2001 From: Blake Rouse Date: Tue, 30 Apr 2024 14:52:37 -0400 Subject: [PATCH 1/9] Add unprivileged upgrade testing for Windows. --- testing/integration/upgrade_fleet_test.go | 7 ++++++- .../upgrade_standalone_same_commit_test.go | 15 +++++++-------- testing/integration/upgrade_standalone_test.go | 15 +++++++-------- testing/upgradetest/upgrader.go | 7 ++++++- testing/upgradetest/versions.go | 4 +++- 5 files changed, 29 insertions(+), 19 deletions(-) diff --git a/testing/integration/upgrade_fleet_test.go b/testing/integration/upgrade_fleet_test.go index 05180faaf99..70a71a4b135 100644 --- a/testing/integration/upgrade_fleet_test.go +++ b/testing/integration/upgrade_fleet_test.go @@ -222,7 +222,12 @@ func testUpgradeFleetManagedElasticAgent( t.Skipf("Ending version %s is less than 8.13 and doesn't support --unprivileged", endParsedVersion.String()) } if runtime.GOOS != define.Linux { - t.Skip("Unprivileged mode is currently only supported on Linux") + if startParsedVersion.Less(*upgradetest.Version_8_14_0) { + t.Skipf("Starting version %s is less than 8.14 and doesn't support --unprivileged on Windows", startParsedVersion.String()) + } + if endParsedVersion.Less(*upgradetest.Version_8_14_0) { + t.Skipf("Ending version %s is less than 8.14 and doesn't support --unprivileged on Windows", endParsedVersion.String()) + } } } diff --git a/testing/integration/upgrade_standalone_same_commit_test.go b/testing/integration/upgrade_standalone_same_commit_test.go index 4da4af6ae7e..4214802248c 100644 --- a/testing/integration/upgrade_standalone_same_commit_test.go +++ b/testing/integration/upgrade_standalone_same_commit_test.go @@ -52,15 +52,14 @@ func TestStandaloneUpgradeSameCommit(t *testing.T) { t.Skipf("Minimum version for running this test is %q, current version: %q", *upgradetest.Version_8_13_0_SNAPSHOT, currentVersion) } - unprivilegedAvailable := true - if runtime.GOOS != define.Linux { - // only available on Linux at the moment - unprivilegedAvailable = false - } + unprivilegedAvailable := false // This is probably redundant: see the skip statement above - if unprivilegedAvailable && currentVersion.Less(*upgradetest.Version_8_13_0) { - // only available if both versions are 8.13+ - unprivilegedAvailable = false + if runtime.GOOS == define.Linux && !currentVersion.Less(*upgradetest.Version_8_13_0) { + // only available if version is 8.13+ on Linux + unprivilegedAvailable = true + } else if !currentVersion.Less(*upgradetest.Version_8_14_0) { + // 8.14+ its always available + unprivilegedAvailable = true } unPrivilegedString := "unprivileged" diff --git a/testing/integration/upgrade_standalone_test.go b/testing/integration/upgrade_standalone_test.go index 9be5344e2c3..5b6865eeb11 100644 --- a/testing/integration/upgrade_standalone_test.go +++ b/testing/integration/upgrade_standalone_test.go @@ -36,14 +36,13 @@ func TestStandaloneUpgrade(t *testing.T) { require.NoError(t, err) for _, startVersion := range versionList { - unprivilegedAvailable := true - if runtime.GOOS != define.Linux { - // only available on Linux at the moment - unprivilegedAvailable = false - } - if unprivilegedAvailable && (startVersion.Less(*upgradetest.Version_8_13_0) || endVersion.Less(*upgradetest.Version_8_13_0)) { - // only available if both versions are 8.13+ - unprivilegedAvailable = false + unprivilegedAvailable := false + if runtime.GOOS == define.Linux && !startVersion.Less(*upgradetest.Version_8_13_0) && !endVersion.Less(*upgradetest.Version_8_13_0) { + // unprivileged available if both versions are 8.13+ on Linux + unprivilegedAvailable = true + } else if !startVersion.Less(*upgradetest.Version_8_14_0) && !endVersion.Less(*upgradetest.Version_8_14_0) { + // always available if both versions are 8.14+ + unprivilegedAvailable = true } t.Run(fmt.Sprintf("Upgrade %s to %s (privileged)", startVersion, define.Version()), func(t *testing.T) { testStandaloneUpgrade(t, startVersion, define.Version(), false) diff --git a/testing/upgradetest/upgrader.go b/testing/upgradetest/upgrader.go index 3acefff8f9f..b8ec1cc89ba 100644 --- a/testing/upgradetest/upgrader.go +++ b/testing/upgradetest/upgrader.go @@ -210,7 +210,12 @@ func PerformUpgrade( // in the case that its explicitly set then we ensure the version supports it if upgradeOpts.unprivileged == nil { if !startVersion.Less(*Version_8_13_0) && !endVersion.Less(*Version_8_13_0) && runtime.GOOS == define.Linux { - // both version support --unprivileged + // both version support --unprivileged on Linux + unprivileged := true + upgradeOpts.unprivileged = &unprivileged + logger.Logf("installation of Elastic Agent will use --unprivileged as both start and end version support --unprivileged mode on Linux") + } else if !startVersion.Less(*Version_8_14_0) && !endVersion.Less(*Version_8_14_0) { + // both version support --unprivileged on all platforms unprivileged := true upgradeOpts.unprivileged = &unprivileged logger.Logf("installation of Elastic Agent will use --unprivileged as both start and end version support --unprivileged mode") diff --git a/testing/upgradetest/versions.go b/testing/upgradetest/versions.go index cdc66666fd7..1efdf0e9ddd 100644 --- a/testing/upgradetest/versions.go +++ b/testing/upgradetest/versions.go @@ -35,8 +35,10 @@ var ( Version_8_11_0_SNAPSHOT = version.NewParsedSemVer(8, 11, 0, "SNAPSHOT", "") // Version_8_13_0_SNAPSHOT is the minimum version for testing upgrading agent with the same hash Version_8_13_0_SNAPSHOT = version.NewParsedSemVer(8, 13, 0, "SNAPSHOT", "") - // Version_8_13_0 is the minimum version for proper unprivileged execution + // Version_8_13_0 is the minimum version for proper unprivileged execution on Linux Version_8_13_0 = version.NewParsedSemVer(8, 13, 0, "", "") + // Version_8_14_0 is the minimum version for proper unprivileged execution on all platforms + Version_8_14_0 = version.NewParsedSemVer(8, 14, 0, "", "") // ErrNoSnapshot is returned when a requested snapshot is not on the version list. ErrNoSnapshot = errors.New("failed to find a snapshot on the version list") From b06dfdbdf31a322c5285c8f2e6e382a6d7ed3237 Mon Sep 17 00:00:00 2001 From: Blake Rouse Date: Tue, 30 Apr 2024 19:35:09 -0400 Subject: [PATCH 2/9] Adjust to 8.14.0-SNAPSHOT as minimum. --- testing/integration/upgrade_fleet_test.go | 16 ++++++++-------- .../upgrade_standalone_same_commit_test.go | 4 ++-- testing/integration/upgrade_standalone_test.go | 4 ++-- testing/upgradetest/upgrader.go | 8 ++++---- testing/upgradetest/versions.go | 6 ++---- 5 files changed, 18 insertions(+), 20 deletions(-) diff --git a/testing/integration/upgrade_fleet_test.go b/testing/integration/upgrade_fleet_test.go index 70a71a4b135..92e4d8b6a4a 100644 --- a/testing/integration/upgrade_fleet_test.go +++ b/testing/integration/upgrade_fleet_test.go @@ -215,18 +215,18 @@ func testUpgradeFleetManagedElasticAgent( require.NoError(t, err) if unprivileged { - if startParsedVersion.Less(*upgradetest.Version_8_13_0) { - t.Skipf("Starting version %s is less than 8.13 and doesn't support --unprivileged", startParsedVersion.String()) + if startParsedVersion.Less(*upgradetest.Version_8_13_0_SNAPSHOT) { + t.Skipf("Starting version %s is less than 8.13-SNAPSHOT and doesn't support --unprivileged", startParsedVersion.String()) } - if endParsedVersion.Less(*upgradetest.Version_8_13_0) { - t.Skipf("Ending version %s is less than 8.13 and doesn't support --unprivileged", endParsedVersion.String()) + if endParsedVersion.Less(*upgradetest.Version_8_13_0_SNAPSHOT) { + t.Skipf("Ending version %s is less than 8.13-SNAPSHOT and doesn't support --unprivileged", endParsedVersion.String()) } if runtime.GOOS != define.Linux { - if startParsedVersion.Less(*upgradetest.Version_8_14_0) { - t.Skipf("Starting version %s is less than 8.14 and doesn't support --unprivileged on Windows", startParsedVersion.String()) + if startParsedVersion.Less(*upgradetest.Version_8_14_0_SNAPSHOT) { + t.Skipf("Starting version %s is less than 8.14-SNAPSHOT and doesn't support --unprivileged on Windows", startParsedVersion.String()) } - if endParsedVersion.Less(*upgradetest.Version_8_14_0) { - t.Skipf("Ending version %s is less than 8.14 and doesn't support --unprivileged on Windows", endParsedVersion.String()) + if endParsedVersion.Less(*upgradetest.Version_8_14_0_SNAPSHOT) { + t.Skipf("Ending version %s is less than 8.14-SNAPSHOT and doesn't support --unprivileged on Windows", endParsedVersion.String()) } } } diff --git a/testing/integration/upgrade_standalone_same_commit_test.go b/testing/integration/upgrade_standalone_same_commit_test.go index 4214802248c..09eca8f060e 100644 --- a/testing/integration/upgrade_standalone_same_commit_test.go +++ b/testing/integration/upgrade_standalone_same_commit_test.go @@ -54,10 +54,10 @@ func TestStandaloneUpgradeSameCommit(t *testing.T) { unprivilegedAvailable := false // This is probably redundant: see the skip statement above - if runtime.GOOS == define.Linux && !currentVersion.Less(*upgradetest.Version_8_13_0) { + if runtime.GOOS == define.Linux && !currentVersion.Less(*upgradetest.Version_8_13_0_SNAPSHOT) { // only available if version is 8.13+ on Linux unprivilegedAvailable = true - } else if !currentVersion.Less(*upgradetest.Version_8_14_0) { + } else if !currentVersion.Less(*upgradetest.Version_8_14_0_SNAPSHOT) { // 8.14+ its always available unprivilegedAvailable = true } diff --git a/testing/integration/upgrade_standalone_test.go b/testing/integration/upgrade_standalone_test.go index 5b6865eeb11..99f3795d7cc 100644 --- a/testing/integration/upgrade_standalone_test.go +++ b/testing/integration/upgrade_standalone_test.go @@ -37,10 +37,10 @@ func TestStandaloneUpgrade(t *testing.T) { for _, startVersion := range versionList { unprivilegedAvailable := false - if runtime.GOOS == define.Linux && !startVersion.Less(*upgradetest.Version_8_13_0) && !endVersion.Less(*upgradetest.Version_8_13_0) { + if runtime.GOOS == define.Linux && !startVersion.Less(*upgradetest.Version_8_13_0_SNAPSHOT) && !endVersion.Less(*upgradetest.Version_8_13_0_SNAPSHOT) { // unprivileged available if both versions are 8.13+ on Linux unprivilegedAvailable = true - } else if !startVersion.Less(*upgradetest.Version_8_14_0) && !endVersion.Less(*upgradetest.Version_8_14_0) { + } else if !startVersion.Less(*upgradetest.Version_8_14_0_SNAPSHOT) && !endVersion.Less(*upgradetest.Version_8_14_0_SNAPSHOT) { // always available if both versions are 8.14+ unprivilegedAvailable = true } diff --git a/testing/upgradetest/upgrader.go b/testing/upgradetest/upgrader.go index b8ec1cc89ba..4c5dcfa3911 100644 --- a/testing/upgradetest/upgrader.go +++ b/testing/upgradetest/upgrader.go @@ -209,12 +209,12 @@ func PerformUpgrade( // in the unprivileged is unset we adjust it to use unprivileged when the version allows it // in the case that its explicitly set then we ensure the version supports it if upgradeOpts.unprivileged == nil { - if !startVersion.Less(*Version_8_13_0) && !endVersion.Less(*Version_8_13_0) && runtime.GOOS == define.Linux { + if !startVersion.Less(*Version_8_13_0_SNAPSHOT) && !endVersion.Less(*Version_8_13_0_SNAPSHOT) && runtime.GOOS == define.Linux { // both version support --unprivileged on Linux unprivileged := true upgradeOpts.unprivileged = &unprivileged logger.Logf("installation of Elastic Agent will use --unprivileged as both start and end version support --unprivileged mode on Linux") - } else if !startVersion.Less(*Version_8_14_0) && !endVersion.Less(*Version_8_14_0) { + } else if !startVersion.Less(*Version_8_14_0_SNAPSHOT) && !endVersion.Less(*Version_8_14_0_SNAPSHOT) { // both version support --unprivileged on all platforms unprivileged := true upgradeOpts.unprivileged = &unprivileged @@ -225,10 +225,10 @@ func PerformUpgrade( upgradeOpts.unprivileged = &unprivileged } } else if *upgradeOpts.unprivileged { - if startVersion.Less(*Version_8_13_0) { + if startVersion.Less(*Version_8_13_0_SNAPSHOT) { return errors.New("cannot install starting version with --unprivileged (which is default) because the it is older than 8.13") } - if endVersion.Less(*Version_8_13_0) { + if endVersion.Less(*Version_8_13_0_SNAPSHOT) { return errors.New("cannot upgrade to ending version as end version doesn't support running with --unprivileged (which is default) because it is older than 8.13") } } diff --git a/testing/upgradetest/versions.go b/testing/upgradetest/versions.go index 1efdf0e9ddd..7126c32676f 100644 --- a/testing/upgradetest/versions.go +++ b/testing/upgradetest/versions.go @@ -35,10 +35,8 @@ var ( Version_8_11_0_SNAPSHOT = version.NewParsedSemVer(8, 11, 0, "SNAPSHOT", "") // Version_8_13_0_SNAPSHOT is the minimum version for testing upgrading agent with the same hash Version_8_13_0_SNAPSHOT = version.NewParsedSemVer(8, 13, 0, "SNAPSHOT", "") - // Version_8_13_0 is the minimum version for proper unprivileged execution on Linux - Version_8_13_0 = version.NewParsedSemVer(8, 13, 0, "", "") - // Version_8_14_0 is the minimum version for proper unprivileged execution on all platforms - Version_8_14_0 = version.NewParsedSemVer(8, 14, 0, "", "") + // Version_8_14_0_SNAPSHOT is the minimum version for proper unprivileged execution on all platforms + Version_8_14_0_SNAPSHOT = version.NewParsedSemVer(8, 14, 0, "SNAPSHOT", "") // ErrNoSnapshot is returned when a requested snapshot is not on the version list. ErrNoSnapshot = errors.New("failed to find a snapshot on the version list") From a645ed0e52ea518af36e1fc2b93d60178ade10f2 Mon Sep 17 00:00:00 2001 From: Blake Rouse Date: Wed, 1 May 2024 09:02:38 -0400 Subject: [PATCH 3/9] Add SeCreateSymbolicLinkPrivilege --- internal/pkg/agent/install/user_windows.go | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/internal/pkg/agent/install/user_windows.go b/internal/pkg/agent/install/user_windows.go index ffe545899f2..357b5c0a779 100644 --- a/internal/pkg/agent/install/user_windows.go +++ b/internal/pkg/agent/install/user_windows.go @@ -39,6 +39,8 @@ const ( USER_UF_SCRIPT = 1 USER_UF_NORMAL_ACCOUNT = 512 USER_UF_DONT_EXPIRE_PASSWD = 65536 + + accountRightCreateSymbolicLink gowin32.AccountRightName = "SeCreateSymbolicLinkPrivilege" ) // FindGID returns the group's GID on the machine. @@ -151,6 +153,10 @@ func CreateUser(name string, _ string) (string, error) { if err != nil { return "", fmt.Errorf("failed to set service logon: %w", err) } + err = sp.AddAccountRight(sid, accountRightCreateSymbolicLink) + if err != nil { + return "", fmt.Errorf("failed to add right to create symbolic link: %w", err) + } return FindUID(name) } From 75911b5c77c6b8ce83533ba4d9beba2500d3694d Mon Sep 17 00:00:00 2001 From: Blake Rouse Date: Thu, 2 May 2024 11:21:15 -0400 Subject: [PATCH 4/9] Simplify if unprivileged mode is supported. --- testing/integration/upgrade_fleet_test.go | 15 ++------------- .../upgrade_standalone_same_commit_test.go | 9 +-------- testing/integration/upgrade_standalone_test.go | 7 +------ testing/upgradetest/upgrader.go | 17 +++-------------- testing/upgradetest/versions.go | 14 ++++++++++++++ 5 files changed, 21 insertions(+), 41 deletions(-) diff --git a/testing/integration/upgrade_fleet_test.go b/testing/integration/upgrade_fleet_test.go index 92e4d8b6a4a..58c4973293e 100644 --- a/testing/integration/upgrade_fleet_test.go +++ b/testing/integration/upgrade_fleet_test.go @@ -215,19 +215,8 @@ func testUpgradeFleetManagedElasticAgent( require.NoError(t, err) if unprivileged { - if startParsedVersion.Less(*upgradetest.Version_8_13_0_SNAPSHOT) { - t.Skipf("Starting version %s is less than 8.13-SNAPSHOT and doesn't support --unprivileged", startParsedVersion.String()) - } - if endParsedVersion.Less(*upgradetest.Version_8_13_0_SNAPSHOT) { - t.Skipf("Ending version %s is less than 8.13-SNAPSHOT and doesn't support --unprivileged", endParsedVersion.String()) - } - if runtime.GOOS != define.Linux { - if startParsedVersion.Less(*upgradetest.Version_8_14_0_SNAPSHOT) { - t.Skipf("Starting version %s is less than 8.14-SNAPSHOT and doesn't support --unprivileged on Windows", startParsedVersion.String()) - } - if endParsedVersion.Less(*upgradetest.Version_8_14_0_SNAPSHOT) { - t.Skipf("Ending version %s is less than 8.14-SNAPSHOT and doesn't support --unprivileged on Windows", endParsedVersion.String()) - } + if !upgradetest.SupportsUnprivileged(startParsedVersion, endParsedVersion) { + t.Skipf("Either starting version %s or ending version %s doesn't support --unprivileged", startParsedVersion.String(), endParsedVersion.String()) } } diff --git a/testing/integration/upgrade_standalone_same_commit_test.go b/testing/integration/upgrade_standalone_same_commit_test.go index 09eca8f060e..e7ab8aa60a4 100644 --- a/testing/integration/upgrade_standalone_same_commit_test.go +++ b/testing/integration/upgrade_standalone_same_commit_test.go @@ -18,7 +18,6 @@ import ( "os" "path" "path/filepath" - "runtime" "strings" "testing" "time" @@ -53,15 +52,9 @@ func TestStandaloneUpgradeSameCommit(t *testing.T) { } unprivilegedAvailable := false - // This is probably redundant: see the skip statement above - if runtime.GOOS == define.Linux && !currentVersion.Less(*upgradetest.Version_8_13_0_SNAPSHOT) { - // only available if version is 8.13+ on Linux - unprivilegedAvailable = true - } else if !currentVersion.Less(*upgradetest.Version_8_14_0_SNAPSHOT) { - // 8.14+ its always available + if upgradetest.SupportsUnprivileged(currentVersion) { unprivilegedAvailable = true } - unPrivilegedString := "unprivileged" if !unprivilegedAvailable { unPrivilegedString = "privileged" diff --git a/testing/integration/upgrade_standalone_test.go b/testing/integration/upgrade_standalone_test.go index 99f3795d7cc..973dc033758 100644 --- a/testing/integration/upgrade_standalone_test.go +++ b/testing/integration/upgrade_standalone_test.go @@ -9,7 +9,6 @@ package integration import ( "context" "fmt" - "runtime" "testing" "time" @@ -37,11 +36,7 @@ func TestStandaloneUpgrade(t *testing.T) { for _, startVersion := range versionList { unprivilegedAvailable := false - if runtime.GOOS == define.Linux && !startVersion.Less(*upgradetest.Version_8_13_0_SNAPSHOT) && !endVersion.Less(*upgradetest.Version_8_13_0_SNAPSHOT) { - // unprivileged available if both versions are 8.13+ on Linux - unprivilegedAvailable = true - } else if !startVersion.Less(*upgradetest.Version_8_14_0_SNAPSHOT) && !endVersion.Less(*upgradetest.Version_8_14_0_SNAPSHOT) { - // always available if both versions are 8.14+ + if upgradetest.SupportsUnprivileged(startVersion, endVersion) { unprivilegedAvailable = true } t.Run(fmt.Sprintf("Upgrade %s to %s (privileged)", startVersion, define.Version()), func(t *testing.T) { diff --git a/testing/upgradetest/upgrader.go b/testing/upgradetest/upgrader.go index 4c5dcfa3911..1e0dafea9d6 100644 --- a/testing/upgradetest/upgrader.go +++ b/testing/upgradetest/upgrader.go @@ -11,7 +11,6 @@ import ( "fmt" "os" "path/filepath" - "runtime" "strings" "time" @@ -21,7 +20,6 @@ import ( v1client "github.com/elastic/elastic-agent/pkg/control/v1/client" v2proto "github.com/elastic/elastic-agent/pkg/control/v2/cproto" atesting "github.com/elastic/elastic-agent/pkg/testing" - "github.com/elastic/elastic-agent/pkg/testing/define" "github.com/elastic/elastic-agent/pkg/version" ) @@ -209,13 +207,7 @@ func PerformUpgrade( // in the unprivileged is unset we adjust it to use unprivileged when the version allows it // in the case that its explicitly set then we ensure the version supports it if upgradeOpts.unprivileged == nil { - if !startVersion.Less(*Version_8_13_0_SNAPSHOT) && !endVersion.Less(*Version_8_13_0_SNAPSHOT) && runtime.GOOS == define.Linux { - // both version support --unprivileged on Linux - unprivileged := true - upgradeOpts.unprivileged = &unprivileged - logger.Logf("installation of Elastic Agent will use --unprivileged as both start and end version support --unprivileged mode on Linux") - } else if !startVersion.Less(*Version_8_14_0_SNAPSHOT) && !endVersion.Less(*Version_8_14_0_SNAPSHOT) { - // both version support --unprivileged on all platforms + if SupportsUnprivileged(startVersion, endVersion) { unprivileged := true upgradeOpts.unprivileged = &unprivileged logger.Logf("installation of Elastic Agent will use --unprivileged as both start and end version support --unprivileged mode") @@ -225,11 +217,8 @@ func PerformUpgrade( upgradeOpts.unprivileged = &unprivileged } } else if *upgradeOpts.unprivileged { - if startVersion.Less(*Version_8_13_0_SNAPSHOT) { - return errors.New("cannot install starting version with --unprivileged (which is default) because the it is older than 8.13") - } - if endVersion.Less(*Version_8_13_0_SNAPSHOT) { - return errors.New("cannot upgrade to ending version as end version doesn't support running with --unprivileged (which is default) because it is older than 8.13") + if !SupportsUnprivileged(startVersion, endVersion) { + return fmt.Errorf("cannot install with forced --unprivileged because either start version %s or end version %s doesn't support --unprivileged mode", startVersion.String(), endVersion.String()) } } diff --git a/testing/upgradetest/versions.go b/testing/upgradetest/versions.go index 7126c32676f..ddb90097d5c 100644 --- a/testing/upgradetest/versions.go +++ b/testing/upgradetest/versions.go @@ -11,6 +11,7 @@ import ( "fmt" "os" "path/filepath" + "runtime" "sort" "strings" @@ -254,3 +255,16 @@ func EnsureSnapshot(version string) string { } return version } + +// SupportsUnprivileged returns true when the version supports unprivileged mode. +func SupportsUnprivileged(versions ...*version.ParsedSemVer) bool { + for _, ver := range versions { + if ver.Less(*Version_8_13_0_SNAPSHOT) { + return false + } + if runtime.GOOS != define.Linux && ver.Less(*Version_8_14_0_SNAPSHOT) { + return false + } + } + return true +} From 13ee3bb334812f623b87b5a756313df7c91bbb37 Mon Sep 17 00:00:00 2001 From: Blake Rouse Date: Thu, 2 May 2024 21:09:28 -0400 Subject: [PATCH 5/9] Fix file access for upgrade. --- testing/upgradetest/upgrader.go | 35 ++++++++++++++++++++++++++++++++- 1 file changed, 34 insertions(+), 1 deletion(-) diff --git a/testing/upgradetest/upgrader.go b/testing/upgradetest/upgrader.go index 1e0dafea9d6..0b9cbb9d286 100644 --- a/testing/upgradetest/upgrader.go +++ b/testing/upgradetest/upgrader.go @@ -9,8 +9,10 @@ import ( "encoding/json" "errors" "fmt" + "github.com/hectane/go-acl" "os" "path/filepath" + "runtime" "strings" "time" @@ -568,7 +570,19 @@ func getSourceURI(ctx context.Context, f *atesting.Fixture, unprivileged bool) ( } if unprivileged { // move the file to temp directory - dir, err := os.MkdirTemp("", "agent-upgrade-*") + baseTmp := "" + if runtime.GOOS == "windows" { + // `elastic-agent-user` needs to have access to the file, default + // will place this in C:\Users\windows\AppData\Local\Temp\ which + // `elastic-agent-user` doesn't have access. + + // create C:\Temp with world read/write to use for temp directory + baseTmp, err = windowsBaseTemp() + if err != nil { + return "", fmt.Errorf("failed to create windows base temp path: %w", err) + } + } + dir, err := os.MkdirTemp(baseTmp, "agent-upgrade-*") if err != nil { return "", fmt.Errorf("failed to create temp directory: %w", err) } @@ -590,3 +604,22 @@ func getSourceURI(ctx context.Context, f *atesting.Fixture, unprivileged bool) ( } return "file://" + filepath.Dir(srcPkg), nil } + +func windowsBaseTemp() (string, error) { + baseTmp := "C:\\Temp" + _, err := os.Stat(baseTmp) + if err != nil { + if !errors.Is(err, os.ErrNotExist) { + return "", fmt.Errorf("failed to stat %s: %w", baseTmp, err) + } + err = os.Mkdir(baseTmp, 0777) + if err != nil { + return "", fmt.Errorf("failed to mkdir %s: %w", baseTmp, err) + } + } + err = acl.Chmod(baseTmp, 0777) + if err != nil { + return "", fmt.Errorf("failed to chmod %s: %w", baseTmp, err) + } + return baseTmp, nil +} From 351b32ff19ff598fde328f38c5e15a9bfd46d4f7 Mon Sep 17 00:00:00 2001 From: Blake Rouse Date: Mon, 6 May 2024 11:30:43 -0400 Subject: [PATCH 6/9] Fix kill watcher process on Windows in unprivileged mode. --- internal/pkg/agent/install/uninstall.go | 2 +- internal/pkg/agent/install/uninstall_unix.go | 10 ++++++++++ internal/pkg/agent/install/uninstall_windows.go | 15 +++++++++++++++ 3 files changed, 26 insertions(+), 1 deletion(-) diff --git a/internal/pkg/agent/install/uninstall.go b/internal/pkg/agent/install/uninstall.go index efcbafcaaff..f333e8c49a5 100644 --- a/internal/pkg/agent/install/uninstall.go +++ b/internal/pkg/agent/install/uninstall.go @@ -391,7 +391,7 @@ func killWatcher(pt *progressbar.ProgressBar) error { errs = errors.Join(errs, fmt.Errorf("failed to load watcher process with pid %d: %w", pid, err)) continue } - err = proc.Kill() + err = killNoneChildProcess(proc) if err != nil && !errors.Is(err, os.ErrProcessDone) { errs = errors.Join(errs, fmt.Errorf("failed to kill watcher process with pid %d: %w", pid, err)) continue diff --git a/internal/pkg/agent/install/uninstall_unix.go b/internal/pkg/agent/install/uninstall_unix.go index d63d4bcf2d4..ba851b4e981 100644 --- a/internal/pkg/agent/install/uninstall_unix.go +++ b/internal/pkg/agent/install/uninstall_unix.go @@ -6,6 +6,8 @@ package install +import "os" + func isBlockingOnExe(_ error) bool { return false } @@ -17,3 +19,11 @@ func removeBlockingExe(_ error) error { func isRetryableError(_ error) bool { return false } + +// killNoneChildProcess provides a way of killing a process that is not started as a child of this process. +// +// On Windows when running in unprivileged mode the internal way that golang uses DuplicateHandle to perform the kill +// only works when the process is a child of this process. +func killNoneChildProcess(proc *os.Process) error { + return proc.Kill() +} diff --git a/internal/pkg/agent/install/uninstall_windows.go b/internal/pkg/agent/install/uninstall_windows.go index 5339cd376b3..e4ce2488972 100644 --- a/internal/pkg/agent/install/uninstall_windows.go +++ b/internal/pkg/agent/install/uninstall_windows.go @@ -10,6 +10,7 @@ import ( "errors" "fmt" "io/fs" + "os" "syscall" "unsafe" @@ -160,3 +161,17 @@ type fileRenameInfo struct { type fileDispositionInfo struct { DeleteFile bool } + +// killNoneChildProcess provides a way of killing a process that is not started as a child of this process. +// +// On Windows when running in unprivileged mode the internal way that golang uses DuplicateHandle to perform the kill +// only works when the process is a child of this process. +func killNoneChildProcess(proc *os.Process) error { + h, e := syscall.OpenProcess(syscall.PROCESS_TERMINATE, false, uint32(proc.Pid)) + if e != nil { + return os.NewSyscallError("OpenProcess", e) + } + defer syscall.CloseHandle(h) + e = syscall.TerminateProcess(h, 1) + return os.NewSyscallError("TerminateProcess", e) +} From d10ffa2d1b81ee8667a034cfc68a7d8dd778c386 Mon Sep 17 00:00:00 2001 From: Blake Rouse Date: Mon, 6 May 2024 11:35:59 -0400 Subject: [PATCH 7/9] Fix imports. --- testing/upgradetest/upgrader.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/testing/upgradetest/upgrader.go b/testing/upgradetest/upgrader.go index 0b9cbb9d286..8ed20dd71fd 100644 --- a/testing/upgradetest/upgrader.go +++ b/testing/upgradetest/upgrader.go @@ -9,13 +9,13 @@ import ( "encoding/json" "errors" "fmt" - "github.com/hectane/go-acl" "os" "path/filepath" "runtime" "strings" "time" + "github.com/hectane/go-acl" "github.com/otiai10/copy" "github.com/elastic/elastic-agent/internal/pkg/agent/application/upgrade/details" From 51be64bef70da341f75bade825339e0e8bc44ecc Mon Sep 17 00:00:00 2001 From: Blake Rouse Date: Mon, 6 May 2024 16:38:00 -0400 Subject: [PATCH 8/9] Fix kill non child process in tests. --- internal/pkg/agent/install/uninstall_unix.go | 3 +-- testing/upgradetest/watcher.go | 2 +- testing/upgradetest/watcher_unix.go | 16 ++++++++++++ testing/upgradetest/watcher_windows.go | 26 ++++++++++++++++++++ 4 files changed, 44 insertions(+), 3 deletions(-) create mode 100644 testing/upgradetest/watcher_unix.go create mode 100644 testing/upgradetest/watcher_windows.go diff --git a/internal/pkg/agent/install/uninstall_unix.go b/internal/pkg/agent/install/uninstall_unix.go index ba851b4e981..2181a6db481 100644 --- a/internal/pkg/agent/install/uninstall_unix.go +++ b/internal/pkg/agent/install/uninstall_unix.go @@ -22,8 +22,7 @@ func isRetryableError(_ error) bool { // killNoneChildProcess provides a way of killing a process that is not started as a child of this process. // -// On Windows when running in unprivileged mode the internal way that golang uses DuplicateHandle to perform the kill -// only works when the process is a child of this process. +// On Unix systems it just calls the native golang kill. func killNoneChildProcess(proc *os.Process) error { return proc.Kill() } diff --git a/testing/upgradetest/watcher.go b/testing/upgradetest/watcher.go index fd34dc82a7f..ab2312874fc 100644 --- a/testing/upgradetest/watcher.go +++ b/testing/upgradetest/watcher.go @@ -101,7 +101,7 @@ func WaitForNoWatcher(ctx context.Context, timeout time.Duration, interval time. for _, pid := range pids { proc, err := os.FindProcess(pid) if err == nil { - _ = proc.Kill() + _ = killNoneChildProcess(proc) } } // next interval ticker will check for no watcher and exit diff --git a/testing/upgradetest/watcher_unix.go b/testing/upgradetest/watcher_unix.go new file mode 100644 index 00000000000..858392145d2 --- /dev/null +++ b/testing/upgradetest/watcher_unix.go @@ -0,0 +1,16 @@ +// Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one +// or more contributor license agreements. Licensed under the Elastic License; +// you may not use this file except in compliance with the Elastic License. + +//go:build !windows + +package upgradetest + +import "os" + +// killNoneChildProcess provides a way of killing a process that is not started as a child of this process. +// +// On Unix systems it just calls the native golang kill. +func killNoneChildProcess(proc *os.Process) error { + return proc.Kill() +} diff --git a/testing/upgradetest/watcher_windows.go b/testing/upgradetest/watcher_windows.go new file mode 100644 index 00000000000..b2951b05ff2 --- /dev/null +++ b/testing/upgradetest/watcher_windows.go @@ -0,0 +1,26 @@ +// Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one +// or more contributor license agreements. Licensed under the Elastic License; +// you may not use this file except in compliance with the Elastic License. + +//go:build windows + +package upgradetest + +import ( + "os" + "syscall" +) + +// killNoneChildProcess provides a way of killing a process that is not started as a child of this process. +// +// On Windows when running in unprivileged mode the internal way that golang uses DuplicateHandle to perform the kill +// only works when the process is a child of this process. +func killNoneChildProcess(proc *os.Process) error { + h, e := syscall.OpenProcess(syscall.PROCESS_TERMINATE, false, uint32(proc.Pid)) + if e != nil { + return os.NewSyscallError("OpenProcess", e) + } + defer syscall.CloseHandle(h) + e = syscall.TerminateProcess(h, 1) + return os.NewSyscallError("TerminateProcess", e) +} From 556a37ed8ea6bbdb466faaa0175ddf2d924e41b5 Mon Sep 17 00:00:00 2001 From: Blake Rouse Date: Mon, 6 May 2024 16:48:48 -0400 Subject: [PATCH 9/9] Fix lint. --- internal/pkg/agent/install/uninstall_windows.go | 4 +++- testing/upgradetest/watcher_windows.go | 4 +++- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/internal/pkg/agent/install/uninstall_windows.go b/internal/pkg/agent/install/uninstall_windows.go index e4ce2488972..07b6d2ddeaa 100644 --- a/internal/pkg/agent/install/uninstall_windows.go +++ b/internal/pkg/agent/install/uninstall_windows.go @@ -171,7 +171,9 @@ func killNoneChildProcess(proc *os.Process) error { if e != nil { return os.NewSyscallError("OpenProcess", e) } - defer syscall.CloseHandle(h) + defer func() { + _ = syscall.CloseHandle(h) + }() e = syscall.TerminateProcess(h, 1) return os.NewSyscallError("TerminateProcess", e) } diff --git a/testing/upgradetest/watcher_windows.go b/testing/upgradetest/watcher_windows.go index b2951b05ff2..15716f11bfd 100644 --- a/testing/upgradetest/watcher_windows.go +++ b/testing/upgradetest/watcher_windows.go @@ -20,7 +20,9 @@ func killNoneChildProcess(proc *os.Process) error { if e != nil { return os.NewSyscallError("OpenProcess", e) } - defer syscall.CloseHandle(h) + defer func() { + _ = syscall.CloseHandle(h) + }() e = syscall.TerminateProcess(h, 1) return os.NewSyscallError("TerminateProcess", e) }