diff --git a/changelog/fragments/1756313785-enable-root-user-to-re-enroll-unprivileged-agent-for-mac-and-linux.yaml b/changelog/fragments/1756313785-enable-root-user-to-re-enroll-unprivileged-agent-for-mac-and-linux.yaml new file mode 100644 index 00000000000..73bc6c9e266 --- /dev/null +++ b/changelog/fragments/1756313785-enable-root-user-to-re-enroll-unprivileged-agent-for-mac-and-linux.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 root user to re-enroll unprivileged agent for mac and linux + +# 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/9603 + +# 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 diff --git a/internal/pkg/agent/cmd/enroll.go b/internal/pkg/agent/cmd/enroll.go index 0e6e548493e..fefc80f10b4 100644 --- a/internal/pkg/agent/cmd/enroll.go +++ b/internal/pkg/agent/cmd/enroll.go @@ -346,6 +346,40 @@ func buildEnrollmentFlags(cmd *cobra.Command, url string, token string) []string return args } +// getFileOwnFromCmdFunc, getOwnerFromPathFunc and computeFixPermissions are for +// testability. Instead of directly executing the code block in doEnroll, we +// are calling computeFixPermissions. computeFixPermissions is tested on its own. +type getFileOwnerFromCmdFunc func(*cobra.Command) (utils.FileOwner, error) +type getOwnerFromPathFunc func(string) (utils.FileOwner, error) + +func computeFixPermissions(fromInstall bool, hasRoot bool, os string, getFileOwnerFromCmd getFileOwnerFromCmdFunc, getOwnerFromPath getOwnerFromPathFunc, cmd *cobra.Command) (*utils.FileOwner, error) { + // On MacOS Ventura and above, fixing the permissions on enrollment during installation fails with the error: + // Error: failed to fix permissions: chown /Library/Elastic/Agent/data/elastic-agent-c13f91/elastic-agent.app: operation not permitted + // This is because we are fixing permissions twice, once during installation and again during the enrollment step. + // When we are enrolling as part of installation on MacOS, skip the second attempt to fix permissions. + if fromInstall { + if os == "darwin" { + return nil, nil + } + perms, err := getFileOwnerFromCmd(cmd) + if err != nil { + // no context is added because the error is clear and user facing + return nil, err + } + return &perms, nil + } + + if hasRoot && os != "windows" { // windows is a no-op, will be addressed in a separate PR + perms, err := getOwnerFromPath(paths.Top()) + if err != nil { + return nil, fmt.Errorf("failed to get owner from path %s: %w", paths.Top(), err) + } + return &perms, nil + } + + return nil, nil +} + func enroll(streams *cli.IOStreams, cmd *cobra.Command) error { err := validateEnrollFlags(cmd) if err != nil { @@ -354,24 +388,6 @@ func enroll(streams *cli.IOStreams, cmd *cobra.Command) error { fromInstall, _ := cmd.Flags().GetBool(fromInstallArg) - hasRoot, err := utils.HasRoot() - if err != nil { - return fmt.Errorf("checking if running with root/Administrator privileges: %w", err) - } - if hasRoot && !fromInstall { - binPath, err := os.Executable() - if err != nil { - return fmt.Errorf("error while getting executable path: %w", err) - } - isOwner, err := isOwnerExec(binPath) - if err != nil { - return fmt.Errorf("ran into an error while figuring out if user is allowed to execute the enroll command: %w", err) - } - if !isOwner { - return UserOwnerMismatchError - } - } - pathConfigFile := paths.ConfigFile() rawConfig, err := config.LoadFile(pathConfigFile) if err != nil { @@ -463,21 +479,14 @@ func enroll(streams *cli.IOStreams, cmd *cobra.Command) error { ctx := handleSignal(context.Background()) - // On MacOS Ventura and above, fixing the permissions on enrollment during installation fails with the error: - // Error: failed to fix permissions: chown /Library/Elastic/Agent/data/elastic-agent-c13f91/elastic-agent.app: operation not permitted - // This is because we are fixing permissions twice, once during installation and again during the enrollment step. - // When we are enrolling as part of installation on MacOS, skip the second attempt to fix permissions. - var fixPermissions *utils.FileOwner - if fromInstall { - perms, err := getFileOwnerFromCmd(cmd) - if err != nil { - // no context is added because the error is clear and user facing - return err - } - fixPermissions = &perms + hasRoot, err := utils.HasRoot() + if err != nil { + return fmt.Errorf("checking if running with root/Administrator privileges: %w", err) } - if runtime.GOOS == "darwin" { - fixPermissions = nil + + fixPermissions, err := computeFixPermissions(fromInstall, hasRoot, runtime.GOOS, getFileOwnerFromCmd, getOwnerFromPath, cmd) + if err != nil { + return err } options := enrollCmdOption{ diff --git a/internal/pkg/agent/cmd/enroll_match_fileowner_unix.go b/internal/pkg/agent/cmd/enroll_match_fileowner_unix.go deleted file mode 100644 index acc5996c244..00000000000 --- a/internal/pkg/agent/cmd/enroll_match_fileowner_unix.go +++ /dev/null @@ -1,55 +0,0 @@ -// 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 ( - "fmt" - "os" - "strconv" - "syscall" -) - -func getFileOwner(filePath string) (string, error) { - fileInfo, err := os.Stat(filePath) - if err != nil { - return "", fmt.Errorf("failed to get file info: %w", err) - } - - stat, ok := fileInfo.Sys().(*syscall.Stat_t) - if !ok { - return "", fmt.Errorf("failed to get system specific file info: %w", err) - } - return strconv.Itoa(int(stat.Uid)), nil -} - -func getCurrentUser() (string, error) { - return strconv.Itoa(os.Geteuid()), nil -} - -func isFileOwner(curUser string, fileOwner string) (bool, error) { - return curUser == fileOwner, nil -} - -// Checks if the provided file is owned by the user that initiated the process -func isOwnerExec(filePath string) (bool, error) { - owner, err := getFileOwner(filePath) - if err != nil { - return false, fmt.Errorf("failed to get file owner: %w", err) - } - - curUser, err := getCurrentUser() - if err != nil { - return false, fmt.Errorf("failed to get current user: %w", err) - } - - isOwner, err := isFileOwner(curUser, owner) - if err != nil { - return false, fmt.Errorf("error while checking if current user is the file owner: %w", err) - } - - return isOwner, nil -} diff --git a/internal/pkg/agent/cmd/enroll_match_fileowner_unix_test.go b/internal/pkg/agent/cmd/enroll_match_fileowner_unix_test.go deleted file mode 100644 index 9ad3b22db8c..00000000000 --- a/internal/pkg/agent/cmd/enroll_match_fileowner_unix_test.go +++ /dev/null @@ -1,28 +0,0 @@ -// 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 ( - "os" - "path/filepath" - "testing" - - "github.com/stretchr/testify/require" -) - -func TestIsOwnerExecUnix(t *testing.T) { - path := t.TempDir() - fp := filepath.Join(path, "testfile") - fi, err := os.Create(fp) - require.NoError(t, err) - defer fi.Close() - - isOwner, err := isOwnerExec(fp) - require.NoError(t, err) - - require.True(t, isOwner) -} diff --git a/internal/pkg/agent/cmd/enroll_match_fileowner_windows.go b/internal/pkg/agent/cmd/enroll_match_fileowner_windows.go deleted file mode 100644 index 1205617e27b..00000000000 --- a/internal/pkg/agent/cmd/enroll_match_fileowner_windows.go +++ /dev/null @@ -1,12 +0,0 @@ -// 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 - -func isOwnerExec(path string) (bool, error) { - // No-op for Windows: always allow - return true, nil -} diff --git a/internal/pkg/agent/cmd/enroll_match_fileowner_windows_test.go b/internal/pkg/agent/cmd/enroll_match_fileowner_windows_test.go deleted file mode 100644 index cfc1abcb6bb..00000000000 --- a/internal/pkg/agent/cmd/enroll_match_fileowner_windows_test.go +++ /dev/null @@ -1,28 +0,0 @@ -// 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 ( - "os" - "path/filepath" - "testing" - - "github.com/stretchr/testify/require" -) - -func TestIsOwnerExecWindows(t *testing.T) { - path := t.TempDir() - fp := filepath.Join(path, "testfile") - fi, err := os.Create(fp) - require.NoError(t, err) - defer fi.Close() - - isOwner, err := isOwnerExec(fp) - require.NoError(t, err) - - require.True(t, isOwner) -} diff --git a/internal/pkg/agent/cmd/enroll_test.go b/internal/pkg/agent/cmd/enroll_test.go new file mode 100644 index 00000000000..38fe5f28a00 --- /dev/null +++ b/internal/pkg/agent/cmd/enroll_test.go @@ -0,0 +1,146 @@ +// 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. + +package cmd + +import ( + "errors" + "testing" + + "github.com/spf13/cobra" + "github.com/stretchr/testify/require" + + "github.com/elastic/elastic-agent/internal/pkg/agent/application/paths" + "github.com/elastic/elastic-agent/pkg/utils" +) + +func TestComputeFixPermissions(t *testing.T) { + + type testCase struct { + fromInstall bool + hasRoot bool + goos string + ownerFromCmdOwner utils.FileOwner + ownerFromCmdErr error + ownerFromPathOwner utils.FileOwner + ownerFromPathErr error + wantOwner *utils.FileOwner + wantErr bool + expectOwnerFromCmdCalled bool + expectOwnerFromPathCalled bool + } + + owner := utils.FileOwner{} + testError := errors.New("test error") + + testCases := map[string]testCase{ + "should use owner from flags when enrolling from installer on non-darwin": { + fromInstall: true, + hasRoot: true, + goos: "linux", + ownerFromCmdOwner: owner, + wantOwner: &owner, + expectOwnerFromCmdCalled: true, + expectOwnerFromPathCalled: false, + }, + "should skip fixing permissions when enrolling from installer on darwin": { + fromInstall: true, + hasRoot: true, + goos: "darwin", + wantOwner: nil, + expectOwnerFromCmdCalled: false, + expectOwnerFromPathCalled: false, + }, + "should return error when getting owner from cmd fails during installer enroll": { + fromInstall: true, + hasRoot: true, + goos: "linux", + ownerFromCmdErr: testError, + wantErr: true, + expectOwnerFromCmdCalled: true, + expectOwnerFromPathCalled: false, + }, + "should use owner from binary path when not from installer with root on linux": { + fromInstall: false, + hasRoot: true, + goos: "linux", + ownerFromPathOwner: owner, + wantOwner: &owner, + expectOwnerFromCmdCalled: false, + expectOwnerFromPathCalled: true, + }, + "should skip fixing permissions when not from installer with root on windows": { + fromInstall: false, + hasRoot: true, + goos: "windows", + wantOwner: nil, + expectOwnerFromCmdCalled: false, + expectOwnerFromPathCalled: false, + }, + "should skip fixing permissions when not from installer without root": { + fromInstall: false, + hasRoot: false, + goos: "linux", + wantOwner: nil, + expectOwnerFromCmdCalled: false, + expectOwnerFromPathCalled: false, + }, + "should return error when getting owner from path fails": { + fromInstall: false, + hasRoot: true, + goos: "linux", + ownerFromPathErr: testError, + wantErr: true, + expectOwnerFromCmdCalled: false, + expectOwnerFromPathCalled: true, + }, + } + + for name, tc := range testCases { + t.Run(name, func(t *testing.T) { + var ownerFromCmdCalled, ownerFromPathCalled bool + var receivedPath string + + mockOwnerFromCmdFunc := func(_ *cobra.Command) (utils.FileOwner, error) { + ownerFromCmdCalled = true + if tc.ownerFromCmdErr != nil { + return utils.FileOwner{}, tc.ownerFromCmdErr + } + return tc.ownerFromCmdOwner, nil + } + mockOwnerFromPathFunc := func(p string) (utils.FileOwner, error) { + ownerFromPathCalled = true + receivedPath = p + if tc.ownerFromPathErr != nil { + return utils.FileOwner{}, tc.ownerFromPathErr + } + return tc.ownerFromPathOwner, nil + } + + got, err := computeFixPermissions(tc.fromInstall, tc.hasRoot, tc.goos, mockOwnerFromCmdFunc, mockOwnerFromPathFunc, &cobra.Command{}) + + if tc.wantErr { + require.Error(t, err, "expected error") + require.Nil(t, got, "expected nil owner for error") + return + } + + require.NoError(t, err, "expected no error") + + if tc.wantOwner == nil { + require.Nil(t, got, "expected nil owner") + } else { + require.NotNil(t, got, "expected non-nil owner") + require.Equal(t, tc.wantOwner, got, "owner mismatch") + } + + require.Equal(t, tc.expectOwnerFromCmdCalled, ownerFromCmdCalled, "ownerFromCmdCalled mismatch") + require.Equal(t, tc.expectOwnerFromPathCalled, ownerFromPathCalled, "ownerFromPathCalled mismatch") + + if tc.expectOwnerFromPathCalled { + require.Equal(t, paths.Top(), receivedPath, "received path mismatch") + } + }) + } +} diff --git a/internal/pkg/agent/cmd/enroll_unix.go b/internal/pkg/agent/cmd/enroll_unix.go index ff404aa2916..08fc5d8c6c9 100644 --- a/internal/pkg/agent/cmd/enroll_unix.go +++ b/internal/pkg/agent/cmd/enroll_unix.go @@ -8,6 +8,8 @@ package cmd import ( "fmt" + "os" + "syscall" "github.com/spf13/cobra" @@ -51,3 +53,16 @@ func getIDFromCmd(cmd *cobra.Command, param string) (int, error) { } return int(id), nil } + +// getOwnerFromPath returns the UID and GID owning the provided file path. +func getOwnerFromPath(path string) (utils.FileOwner, error) { + fi, err := os.Stat(path) + if err != nil { + return utils.FileOwner{}, fmt.Errorf("failed to stat %q: %w", path, err) + } + stat, ok := fi.Sys().(*syscall.Stat_t) + if !ok { + return utils.FileOwner{}, fmt.Errorf("failed to get system-specific stat for %q", path) + } + return utils.FileOwner{UID: int(stat.Uid), GID: int(stat.Gid)}, nil +} diff --git a/internal/pkg/agent/cmd/enroll_unix_test.go b/internal/pkg/agent/cmd/enroll_unix_test.go new file mode 100644 index 00000000000..3ffc0603aaf --- /dev/null +++ b/internal/pkg/agent/cmd/enroll_unix_test.go @@ -0,0 +1,78 @@ +// 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 ( + "os" + "path/filepath" + "testing" + + "github.com/stretchr/testify/require" +) + +type testCase struct { + setup func(t *testing.T) string + wantErr bool +} + +func TestGetFileOwnerFromPath(t *testing.T) { + baseDir := t.TempDir() + uid := os.Geteuid() + gid := os.Getegid() + + testCases := map[string]testCase{ + "returns current owner for regular file": { + setup: func(t *testing.T) string { + fp := filepath.Join(baseDir, "tmpfile") + f, err := os.Create(fp) + require.NoError(t, err, "failed to create temp file") + require.NoError(t, f.Close(), "failed to close temp file") + + // make sure the file is owned by the current user and group + err = os.Chown(fp, uid, gid) + require.NoError(t, err, "failed to chown temp file") + + return fp + }, + }, + "returns current owner for directory": { + setup: func(t *testing.T) string { + dirPath := filepath.Join(baseDir, "tmpdir") + err := os.Mkdir(dirPath, 0755) + require.NoError(t, err, "failed to create temp dir") + + // make sure the dir is owned by the current user and group + err = os.Chown(dirPath, uid, gid) + require.NoError(t, err, "failed to chown temp dir") + + return dirPath + }, + }, + "returns error for nonexistent path": { + setup: func(t *testing.T) string { + return filepath.Join(baseDir, "mockFile") + }, + wantErr: true, + }, + } + + for name, tc := range testCases { + t.Run(name, func(t *testing.T) { + path := tc.setup(t) + owner, err := getOwnerFromPath(path) + if tc.wantErr { + require.Error(t, err, "expected error for path %q", path) + require.Empty(t, owner, "expected empty owner for path %q", path) + return + } + require.NoError(t, err, "unexpected error for path %q", path) + + require.Equal(t, uid, owner.UID, "uid mismatch for %q", path) + require.Equal(t, gid, owner.GID, "gid mismatch for %q", path) + }) + } +} diff --git a/internal/pkg/agent/cmd/enroll_windows.go b/internal/pkg/agent/cmd/enroll_windows.go index 6ab1827529d..1a19fbc1fd7 100644 --- a/internal/pkg/agent/cmd/enroll_windows.go +++ b/internal/pkg/agent/cmd/enroll_windows.go @@ -53,3 +53,8 @@ func getSIDFromCmd(cmd *cobra.Command, param string) (*windows.SID, error) { } return nil, nil } + +func getOwnerFromPath(path string) (utils.FileOwner, error) { + // TODO: Implement this + return utils.FileOwner{}, nil +} diff --git a/testing/integration/ess/re-enroll_test.go b/testing/integration/ess/re-enroll_test.go index 55d7658b36f..2b4d779546f 100644 --- a/testing/integration/ess/re-enroll_test.go +++ b/testing/integration/ess/re-enroll_test.go @@ -16,7 +16,6 @@ import ( "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" @@ -33,11 +32,9 @@ type testCase struct { 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. +// 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 func TestReEnrollUnprivileged(t *testing.T) { info := define.Require(t, define.Requirements{ Group: integration.Default, @@ -54,8 +51,10 @@ func TestReEnrollUnprivileged(t *testing.T) { 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()) + if out != nil { + t.Log(string(out)) + } + require.NoError(t, err) assert.Eventuallyf(t, func() bool { err := fixture.IsHealthy(t.Context())