[9.0] (backport #9623) Fix/8544 windows unprivileged reenroll

changelog/fragments/1756433743-enable-root-user-to-re-enroll-unprivileged-agent-for-windows.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: enable admin user to re-enroll unprivileged agent for 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:
+
+# 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/9623
+
+# 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/8544

internal/pkg/agent/cmd/enroll.go

@@ -381,7 +381,7 @@ func computeFixPermissions(fromInstall bool, hasRoot bool, os string, getFileOwn
 		return &perms, nil
 	}
 
-	if hasRoot && os != "windows" { // windows is a no-op, will be addressed in a separate PR
+	if hasRoot {
 		perms, err := getOwnerFromPath(paths.Top())
 		if err != nil {
 			return nil, fmt.Errorf("failed to get owner from path %s: %w", paths.Top(), err)

internal/pkg/agent/cmd/enroll_test.go

@@ -70,13 +70,14 @@ func TestComputeFixPermissions(t *testing.T) {
 			expectOwnerFromCmdCalled:  false,
 			expectOwnerFromPathCalled: true,
 		},
-		"should skip fixing permissions when not from installer with root on windows": {
+		"should return owner from path when not from install and has root on windows": {
 			fromInstall:               false,
 			hasRoot:                   true,
 			goos:                      "windows",
-			wantOwner:                 nil,
+			ownerFromPathOwner:        owner,
+			wantOwner:                 &owner,
 			expectOwnerFromCmdCalled:  false,
-			expectOwnerFromPathCalled: false,
+			expectOwnerFromPathCalled: true,
 		},
 		"should skip fixing permissions when not from installer without root": {
 			fromInstall:               false,

internal/pkg/agent/cmd/enroll_windows.go

@@ -12,6 +12,7 @@ import (
 	"github.com/spf13/cobra"
 	"golang.org/x/sys/windows"
 
+	"github.com/elastic/elastic-agent/internal/pkg/acl"
 	"github.com/elastic/elastic-agent/pkg/utils"
 )
 
@@ -54,7 +55,42 @@ func getSIDFromCmd(cmd *cobra.Command, param string) (*windows.SID, error) {
 	return nil, nil
 }
 
+// getOwnerFromPath calls getOwnerFromPathWindows for testability. This way we
+// can inject the windows specific functions.
 func getOwnerFromPath(path string) (utils.FileOwner, error) {
-	// TODO: Implement this
-	return utils.FileOwner{}, nil
+	return getOwnerFromPathWindows(path, acl.GetNamedSecurityInfo, windows.LocalFree)
+}
+
+type getNamedSecurityInfo func(objectName string, objectType int32, secInfo uint32, owner, group **windows.SID, dacl, sacl, secDesc *windows.Handle) error
+type localFree func(handle windows.Handle) (windows.Handle, error)
+
+func getOwnerFromPathWindows(path string, getNamedSecurityInfo getNamedSecurityInfo, localFree localFree) (utils.FileOwner, error) {
+	var ownerSID *windows.SID
+	var groupSID *windows.SID
+	var secDesc windows.Handle
+
+	if err := getNamedSecurityInfo(
+		path,
+		acl.SE_FILE_OBJECT,
+		acl.OWNER_SECURITY_INFORMATION|acl.GROUP_SECURITY_INFORMATION,
+		&ownerSID,
+		&groupSID,
+		nil,
+		nil,
+		&secDesc,
+	); err != nil {
+		return utils.FileOwner{}, fmt.Errorf("failed to get security info for %s: %w", path, err)
+	}
+
+	defer localFree(secDesc) //nolint:errcheck // not much we can do
+
+	var ownership utils.FileOwner
+	if ownerSID == nil || groupSID == nil {
+		return utils.FileOwner{}, fmt.Errorf("failed to get owner or group SID for %s", path)
+	}
+
+	ownership.UID = ownerSID.String()
+	ownership.GID = groupSID.String()
+
+	return ownership, nil
 }

internal/pkg/agent/cmd/enroll_windows_test.go

@@ -0,0 +1,74 @@
+// 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 windows
+
+package cmd
+
+import (
+	"errors"
+	"testing"
+
+	"github.com/stretchr/testify/require"
+	"golang.org/x/sys/windows"
+
+	"github.com/elastic/elastic-agent/pkg/utils"
+)
+
+func TestGetOwnerFromPathWindows(t *testing.T) {
+
+	ownerSID, err := windows.StringToSid(utils.AdministratorSID)
+	require.NoError(t, err)
+	groupSID, err := windows.StringToSid(utils.AdministratorSID)
+	require.NoError(t, err)
+
+	testError := errors.New("test error")
+
+	mockGetNamedSecurityInfoFactory := func(err error) getNamedSecurityInfo {
+		return func(objectName string, objectType int32, secInfo uint32, owner, group **windows.SID, dacl, sacl, secDesc *windows.Handle) error {
+			*owner = ownerSID
+			*group = groupSID
+			return err
+		}
+	}
+	mockLocalFree := func(handle windows.Handle) (windows.Handle, error) {
+		return windows.Handle(0), nil
+	}
+
+	type testCase struct {
+		mockGetNamedSecurityInfo getNamedSecurityInfo
+		mockLocalFree            localFree
+		wantOwner                utils.FileOwner
+		wantErr                  bool
+	}
+
+	testCases := map[string]testCase{
+		"returns owner when getNamedSecurityInfo succeeds": {
+			mockGetNamedSecurityInfo: mockGetNamedSecurityInfoFactory(nil),
+			mockLocalFree:            mockLocalFree,
+			wantOwner:                utils.FileOwner{UID: utils.AdministratorSID, GID: utils.AdministratorSID},
+			wantErr:                  false,
+		},
+		"returns error when getNamedSecurityInfo fails": {
+			mockGetNamedSecurityInfo: mockGetNamedSecurityInfoFactory(testError),
+			mockLocalFree:            mockLocalFree,
+			wantOwner:                utils.FileOwner{},
+			wantErr:                  true,
+		},
+	}
+
+	for name, tc := range testCases {
+		t.Run(name, func(t *testing.T) {
+			owner, err := getOwnerFromPathWindows("test", tc.mockGetNamedSecurityInfo, tc.mockLocalFree)
+			require.Equal(t, tc.wantOwner, owner)
+
+			if tc.wantErr {
+				require.Error(t, err)
+				return
+			}
+
+			require.NoError(t, err)
+		})
+	}
+}

testing/integration/ess/re-enroll_test.go

@@ -23,70 +23,42 @@ import (
 	"github.com/elastic/elastic-agent/testing/integration"
 )
 
-type AssertFunc func(*testing.T, *atesting.Fixture, string, error)
-
-type testCase struct {
-	description string
-	privileged  bool
-	os          []define.OS
-	assertion   AssertFunc
-}
-
-// Verifies that re-enrollment as a privileged user succeeds when the agent was
-// installed unprivileged. Windows implementation is a no-op and will be addressed
-// in a separate PR. Relevant issue: https://github.com/elastic/elastic-agent/issues/8544
+// Verifies that re-enrolling agent as a privileged user succeeds when the agent
+// is both unprivileged and privileged.
 func TestReEnrollUnprivileged(t *testing.T) {
 	info := define.Require(t, define.Requirements{
 		Group: integration.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)
-	if out != nil {
-		t.Log(string(out))
+	testCases := map[string]bool{
+		"unprivileged agent with privileged user": false,
+		"privileged agent with privileged user":   true,
 	}
-	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 TestReEnrollPrivileged(t *testing.T) {
-	info := define.Require(t, define.Requirements{
-		Group: integration.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,
-	)
+	for name, privileged := range testCases {
+		t.Run(name, func(t *testing.T) {
+			fixture, enrollArgs := prepareAgentforReEnroll(t, ctx, info, privileged)
+
+			out, err := fixture.Exec(ctx, enrollArgs)
+			if out != nil {
+				t.Log(string(out))
+			}
+			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) {