fix(7794): update Windows re-enrollment logic and rename test (#8503) (#8570)

* fix(7794): update Windows re-enrollment logic and rename test

- Updated enroll.go to add a Windows-specific condition for root/Administrator privilege checks.
- Removed Windows-specific file owner check implementation and its test.
- Renamed integration test from enroll_unprivileged_test.go to re-enroll_test.go to better reflect its purpose.

* fix(7794): implement no-op ownership check for Windows - Adds a Windows-specific implementation of isOwnerExec that always returns true (no-op), addressing build and re-enrollment issues. - Retains platform-specific files for clarity and maintainability. - Adds a Windows-specific test to ensure code coverage is maintained. - Removes the Windows OS check from enroll.go, relying on the no-op for Windows.

* changelog: add fragment for Windows re-enrollment bugfix

* fix(7794): remove unnecessary build tag

* fix(7794): enhance re-enrollment tests with structured test cases

- Renamed the test function to TestRenEnroll for clarity.
- Introduced structured test cases for unprivileged and privileged agent re-enrollment scenarios.
- Removed Windows-specific checks and consolidated assertions into the test cases.
- Updated the test logic to improve readability and maintainability.

* fix(7794): enhance re-enrollment test structure and OS handling

* fix(7794): reverted to individual test functions because ci enforces where in the test function define is called

* fix(7794): removed unnecessary var definition

* fix(7794): fix bool value for privileged test case

* fix(7794): added status check after second enroll, moved file owner error to enroll.go

* fix(7794): remove unenroll step from integration tests

* fix(7794): refactored test code

* fix(7794): added comment explaining why windows is excluded from unprivileged test case

(cherry picked from commit 00f1662)

Co-authored-by: Kaan Yalti <[email protected]>

Author: mergify[bot]

changelog/fragments/1749923633-Windows-relax-file-ownership-check-for-re-enrollment.yaml

@@ -0,0 +1,33 @@
+# 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: relax file ownership check to allow admin re-enrollment on Windows
+
+# 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, the agent previously enforced a strict file ownership (SID) check during re-enrollment, which prevented legitimate admin users from re-enrolling the agent if the owner did not match. This PR changes the Windows-specific logic to a no-op, allowing any admin to re-enroll the agent. This restores usability for admin users, but reintroduces the risk that privileged re-enrollment can break unprivileged installs. The Unix-specific ownership check remains unchanged.
+
+# Affected component; usually one of "elastic-agent", "fleet-server", "filebeat", "metricbeat", "auditbeat", "all", etc.
+component: elastic-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/elastic/elastic-agent/pull/8503
+
+# 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/elastic/elastic-agent/issues/7794

internal/pkg/agent/cmd/enroll.go

@@ -29,6 +29,8 @@ import (
 	"github.com/elastic/elastic-agent/pkg/utils"
 )
 
+var UserOwnerMismatchError = errors.New("the command is executed as root but the program files are not owned by the root user. execute the command as the user that owns the program files")
+
 const (
 	fromInstallArg      = "from-install"
 	fromInstallUserArg  = "from-install-user"

internal/pkg/agent/cmd/enroll_match_fileowner_unix.go

@@ -11,12 +11,8 @@ import (
 	"os"
 	"strconv"
 	"syscall"
-
-	"github.com/elastic/elastic-agent/internal/pkg/agent/errors"
 )
 
-var UserOwnerMismatchError = errors.New("the command is executed as root but the program files are not owned by the root user. execute the command as the user that owns the program files")
-
 func getFileOwner(filePath string) (string, error) {
 	fileInfo, err := os.Stat(filePath)
 	if err != nil {

internal/pkg/agent/cmd/enroll_match_fileowner_windows.go

@@ -6,87 +6,7 @@
 
 package cmd
 
-import (
-	"fmt"
-
-	"golang.org/x/sys/windows"
-
-	"github.com/elastic/elastic-agent/internal/pkg/agent/errors"
-)
-
-var UserOwnerMismatchError = errors.New("the command is executed as root but the program files are not owned by the root user.")
-
-func getFileOwner(filePath string) (string, error) {
-	// Get security information of the file
-	sd, err := windows.GetNamedSecurityInfo(
-		filePath,
-		windows.SE_FILE_OBJECT,
-		windows.OWNER_SECURITY_INFORMATION,
-	)
-	if err != nil {
-		return "", fmt.Errorf("failed to get security info: %w", err)
-	}
-	owner, _, err := sd.Owner()
-	if err != nil {
-		return "", fmt.Errorf("failed to get security descriptor owner: %w", err)
-	}
-
-	return owner.String(), nil
-}
-
-// Helper to get the current user's SID
-func getCurrentUser() (string, error) {
-	// Get the token for the current process
-	var token windows.Token
-	err := windows.OpenProcessToken(windows.CurrentProcess(), windows.TOKEN_QUERY, &token)
-	if err != nil {
-		return "", fmt.Errorf("failed to open process token: %w", err)
-	}
-	defer token.Close()
-
-	// Get the token use
-	tokenUser, err := token.GetTokenUser()
-	if err != nil {
-		return "", fmt.Errorf("failed to get token user: %w", err)
-	}
-
-	return tokenUser.User.Sid.String(), nil
-}
-
-func isFileOwner(curUser string, fileOwner string) (bool, error) {
-	var cSid *windows.SID
-	err := windows.ConvertStringSidToSid(windows.StringToUTF16Ptr(curUser), &cSid)
-	if err != nil {
-		return false, fmt.Errorf("failed to convert user SID string to SID: %w", err)
-	}
-
-	var fSid *windows.SID
-	err = windows.ConvertStringSidToSid(windows.StringToUTF16Ptr(fileOwner), &fSid)
-	if err != nil {
-		return false, fmt.Errorf("failed to convert file SID string to SID: %w", err)
-	}
-
-	isEqual := fSid.Equals(cSid)
-
-	return isEqual, nil
-}
-
-// Checks if the provided file is owned by the user that initiated the process
-func isOwnerExec(filePath string) (bool, error) {
-	fileOwner, err := getFileOwner(filePath)
-	if err != nil {
-		return false, fmt.Errorf("getting file owner: %w", err)
-	}
-
-	user, err := getCurrentUser()
-	if err != nil {
-		return false, fmt.Errorf("ran into an error while retrieving current user: %w", err)
-	}
-
-	isOwner, err := isFileOwner(user, fileOwner)
-	if err != nil {
-		return false, fmt.Errorf("error while checking if current user is the file owner: %w", err)
-	}
-
-	return isOwner, nil
+func isOwnerExec(path string) (bool, error) {
+	// No-op for Windows: always allow
+	return true, nil
 }

internal/pkg/agent/cmd/enroll_match_fileowner_windows_test.go

@@ -7,13 +7,11 @@
 package cmd
 
 import (
-	"fmt"
 	"os"
 	"path/filepath"
 	"testing"
 
 	"github.com/stretchr/testify/require"
-	"golang.org/x/sys/windows"
 )
 
 func TestIsOwnerExecWindows(t *testing.T) {
@@ -23,30 +21,8 @@ func TestIsOwnerExecWindows(t *testing.T) {
 	require.NoError(t, err)
 	defer fi.Close()
 
-	var token windows.Token
-	err = windows.OpenProcessToken(windows.CurrentProcess(), windows.TOKEN_QUERY, &token)
-	require.NoError(t, err)
-	defer token.Close()
-
-	tokenUser, err := token.GetTokenUser()
-	require.NoError(t, err)
-
-	err = windows.SetNamedSecurityInfo(
-		fp,
-		windows.SE_FILE_OBJECT,
-		windows.OWNER_SECURITY_INFORMATION,
-		tokenUser.User.Sid,
-		nil,
-		nil,
-		nil,
-	)
-	require.NoError(t, err)
-
-	require.NoError(t, err)
-	defer fi.Close()
-
 	isOwner, err := isOwnerExec(fp)
 	require.NoError(t, err)
 
-	require.True(t, isOwner, fmt.Sprintf("expected isOwnerExec to return \"true\", received \"%v\"", isOwner))
+	require.True(t, isOwner)
 }

testing/integration/enroll_unprivileged_test.go

@@ -0,0 +1,124 @@
+// Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
+// or more contributor license agreements. Licensed under the Elastic License 2.0;
+// you may not use this file except in compliance with the Elastic License 2.0.
+
+//go:build integration
+
+package integration
+
+import (
+	"context"
+	"testing"
+	"time"
+
+	"github.com/gofrs/uuid/v5"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+
+	"github.com/elastic/elastic-agent-libs/kibana"
+	"github.com/elastic/elastic-agent/internal/pkg/agent/cmd"
+	atesting "github.com/elastic/elastic-agent/pkg/testing"
+	"github.com/elastic/elastic-agent/pkg/testing/define"
+	"github.com/elastic/elastic-agent/pkg/testing/tools"
+	"github.com/elastic/elastic-agent/pkg/testing/tools/fleettools"
+)
+
+type AssertFunc func(*testing.T, *atesting.Fixture, string, error)
+
+type testCase struct {
+	description string
+	privileged  bool
+	os          []define.OS
+	assertion   AssertFunc
+}
+
+// TestReEnrollUnprivileged verifies that re-enrollment as a privileged user fails
+// when the agent was installed unprivileged. This enforces the file ownership check
+// on Unix platforms. On Windows, this check is a no-op as of PR #8503, so this test
+// is not run for windows. See the discussion in PR #8503 (https://github.com/elastic/elastic-agent/pull/8503)
+// and comment (https://github.com/elastic/elastic-agent/pull/8503#discussion_r2152603141) for context.
+func TestReEnrollUnprivileged(t *testing.T) {
+	info := define.Require(t, define.Requirements{
+		Group: Default,
+		Stack: &define.Stack{},
+		Sudo:  true,
+		OS: []define.OS{
+			{Type: define.Darwin},
+			{Type: define.Linux},
+		},
+	})
+
+	ctx := t.Context()
+
+	fixture, enrollArgs := prepareAgentforReEnroll(t, ctx, info, false)
+
+	out, err := fixture.Exec(ctx, enrollArgs)
+	require.Error(t, err)
+	require.Contains(t, string(out), cmd.UserOwnerMismatchError.Error())
+
+	assert.Eventuallyf(t, func() bool {
+		err := fixture.IsHealthy(t.Context())
+		return err == nil
+	},
+		2*time.Minute, time.Second,
+		"Elastic-Agent did not report healthy. Agent status error: \"%v\"",
+		err,
+	)
+}
+
+func TestReEnrollPrivileged(t *testing.T) {
+	info := define.Require(t, define.Requirements{
+		Group: Default,
+		Stack: &define.Stack{},
+		Sudo:  true,
+	})
+
+	ctx := t.Context()
+
+	fixture, enrollArgs := prepareAgentforReEnroll(t, ctx, info, true)
+	_, err := fixture.Exec(ctx, enrollArgs)
+	require.NoError(t, err)
+
+	assert.Eventuallyf(t, func() bool {
+		err := fixture.IsHealthy(t.Context())
+		return err == nil
+	},
+		2*time.Minute, time.Second,
+		"Elastic-Agent did not report healthy. Agent status error: \"%v\"",
+		err,
+	)
+}
+
+func prepareAgentforReEnroll(t *testing.T, ctx context.Context, info *define.Info, privileged bool) (*atesting.Fixture, []string) {
+	fixture, err := define.NewFixtureFromLocalBuild(t, define.Version())
+	require.NoError(t, err)
+	installOpts := atesting.InstallOpts{
+		NonInteractive: true,
+		Force:          true,
+		Privileged:     privileged,
+	}
+
+	randId := uuid.Must(uuid.NewV4()).String()
+	policyReq := kibana.AgentPolicy{
+		Name:        "test-policy-" + randId,
+		Namespace:   "default",
+		Description: "Test policy " + randId,
+		MonitoringEnabled: []kibana.MonitoringEnabledOption{
+			kibana.MonitoringEnabledLogs,
+			kibana.MonitoringEnabledMetrics,
+		},
+	}
+	policy, err := info.KibanaClient.CreatePolicy(ctx, policyReq)
+	require.NoError(t, err)
+
+	enrollmentApiKey, err := tools.CreateEnrollmentToken(t, ctx, info.KibanaClient, policy.ID)
+	require.NoError(t, err)
+
+	_, err = tools.InstallAgentForPolicyWithToken(ctx, t, installOpts, fixture, info.KibanaClient, enrollmentApiKey)
+	require.NoError(t, err)
+
+	enrollUrl, err := fleettools.DefaultURL(ctx, info.KibanaClient)
+	require.NoError(t, err)
+
+	return fixture, []string{"enroll", "--url", enrollUrl, "--enrollment-token", enrollmentApiKey.APIKey, "--force"}
+}