Return actionable error message when enrolling #6144

kaanyalti · 2024-11-25T23:44:36Z

Enhancement

What does this PR do?

This PR adds checks to the enroll command to respond with an error message in case the user executing the command and the user that's the owner of the elastic program files don't match. Replaces #6038 based on the following comment

Why is it important?

Currently there are no checks in place to prevent or correct the situation where a user who is not the owner of the program files executes enroll. This leads to a broken state.

Checklist

My code follows the style guidelines of this project
I have commented my code, particularly in hard-to-understand areas
~~[ ] I have made corresponding changes to the documentation~~
~~[ ] I have made corresponding change to the default configuration files~~
~~[ ] I have added tests that prove my fix is effective or that my feature works~~
I have added an entry in ./changelog/fragments using the changelog tool
I have added an integration test or an E2E test

How to test this PR locally

The testing steps are only for linux and mac

Deploy ess, install agent in unprivileged mode and enroll into fleet
Unenroll the agent from the fleet ui
Run the enroll command as root user and validate the error message

Related issues

Closes Actionable error message when attempting to enroll an unprivileged Agent as a privileged user #4889

mergify · 2024-11-25T23:45:14Z

This pull request does not have a backport label. Could you fix it @kaanyalti? 🙏
To fixup this pull request, you need to add the backport labels for the needed
branches, such as:

backport-./d./d is the label to automatically backport to the 8./d branch. /d is the digit

mergify · 2024-11-25T23:45:14Z

backport-v8.x has been added to help with the transition to the new branch 8.x.
If you don't need it please use backport-skip label and remove the backport-8.x label.

elasticmachine · 2024-11-26T21:36:00Z

Pinging @elastic/elastic-agent-control-plane (Team:Elastic-Agent-Control-Plane)

internal/pkg/agent/cmd/enroll_match_fileowner_unix.go

blakerouse · 2024-11-28T01:57:43Z

internal/pkg/agent/cmd/enroll.go

+			return fmt.Errorf("ran into an error while figuring out if user is allowed to execute the enroll command")
+		}
+		if !isEnroll {
+			return UserOwnerMismatchError


On Windows you cannot actually open a shell as the elastic-agent-user so how would they be able to run enroll command on Windows as the elastic-agent-user.

I can return an error message stating the fact that user cannot execute the command as admin for windows. Linux/mac error message will can still mention running the command as the elastic-agent-user

internal/pkg/agent/cmd/enroll_match_fileowner_unix.go

mergify · 2024-12-05T00:29:12Z

This pull request is now in conflicts. Could you fix it? 🙏
To fixup this pull request, you can check out it locally. See documentation: https://help.github.com/articles/checking-out-pull-requests-locally/

git fetch upstream
git checkout -b enhancement/4889_enroll_match_file_owner upstream/enhancement/4889_enroll_match_file_owner
git merge upstream/main
git push upstream enhancement/4889_enroll_match_file_owner

changelog/fragments/1732656422-add-actionable-error-message-for-enroll-command.yaml

internal/pkg/agent/cmd/enroll.go

internal/pkg/agent/cmd/enroll_match_fileowner_unix.go

internal/pkg/agent/cmd/enroll_match_fileowner_windows.go

pchila

Looks good overall, some refactoring may be needed on the tests to make sure we keep relevant behavior tests rather than unit tests that are too small and too fine-grained

A couple of comments about error handling to avoid swallowing inner errors that should be addressed before merging though

internal/pkg/agent/cmd/enroll.go

internal/pkg/agent/cmd/enroll_match_fileowner_unix.go

internal/pkg/agent/cmd/enroll_match_fileowner_windows.go

pchila · 2024-12-11T13:57:27Z

internal/pkg/agent/cmd/enroll_match_fileowner_windows_test.go

+func TestIsFileOwnerWindows(t *testing.T) {
+	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)
+
+	tStr := tokenUser.User.Sid.String()
+
+	eq, err := isFileOwner(tStr, tStr)
+	require.NoError(t, err)
+	require.True(t, eq, fmt.Sprintf("expected \"true\" received \"%v\"", eq))
+}


Not sure if this and the corresponding unix unit test really add value.. If I am reading this correctly in both cases we are testing equality of a user id (UID or Sid) to itself and the equality boils down to a string comparison (with some extra steps in windows case, granted)
I feel this is kinda implied in TestGetFileOwner* and (as a bonus) we can write a single test for both unix and windows implementations without the need of tags in the test files (I am assuming that the ownership of the files and dirs in windows is correctly set when creating the directories and files, if that is not the case we may still need some OS-specific setup sadly

I mainly added the unit tests to satisfy sonarqube requirements. I think the integration test covers the behaviour well enough. That being said, the TestGetFileOwner and TestIsFileOwner tests are redundant when TestIsOwnerExec already covers what the other two tests cover. I will remove the unnecessary tests right away. When it comes to having a single test file, unfortunately as you suspected, windows does not set the file ownership correctly. I have to explicitly set it. So I don't think I can combine them both, unless I go with runtime.GOOS check.

testing/integration/enroll_unprivileged_test.go

michalpristas

tested on darwin and windows. works ok

thanks for addressing all the comments

Co-authored-by: Paolo Chilà <[email protected]>

…rror

…problems with stat.Uid

elastic-sonarqube · 2024-12-18T17:01:35Z

Quality Gate passed

Issues
1 New issue
0 Fixed issues
0 Accepted issues

Measures
0 Security Hotspots
64.9% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube

* enhancement(4889): updated enroll command to reexec the command if the executing user and binary owner don't match, added tests * enhancement(4889): added savepassword function * enhancement(4889): removed user impersonation, implemented noop for windows, updated integration tests * enhancement(4889): added changelog * enhancement(4889): added windows implementation * enhancement(4889): added license headers * enhancement(4889): update test case to execute different commands based on os * enhancement(4889): ran mage clean * enhancement(4889): updated function name, updated integration tests * enhancmenet(4889): updated function name * enhancement(4889): updated function name, fixed test assertions * enhancement(4889): update error messages * enhancement(4889): ran mage update * enhancements(4889): fix integration test * enhancement(4889): remove commented code * enhancement(4889): commiting now, don't push though * enhancement(4889): added windows unit test * enhancement(4889): close test file * enhancement(4889): updated isOwnerExec function, added tests * enchancement(4889): added log in test * enhancement(4889): remove unused types * enhancement(4889): added test for isOwnerExec for windows * enhancement(4889): added unix tests, ran mage addLicenseHeaders * enhancement(4889): added testisfileownerunix test * enhancement(4889): added TestIsOwnerExecUnix test * enhancement(4889): updated test function name * enhancement(4889): set file ownership of the test file * enhancement(4889): remove unnecessary change * enhancment(4889): updated iOwnerExec windows test * enhancement(4889): fix file creation in tests * Update internal/pkg/agent/cmd/enroll.go Co-authored-by: Paolo Chilà <[email protected]> * Update internal/pkg/agent/cmd/enroll_match_fileowner_windows.go Co-authored-by: Paolo Chilà <[email protected]> * Update internal/pkg/agent/cmd/enroll_match_fileowner_unix.go Co-authored-by: Paolo Chilà <[email protected]> * enhancement(4889): updated summary and issue in changelog * enhancement(4889): updated variable name, wrap error when returning error * enhancement(4889): added comments describing isOwnerExec. fixed type problems with stat.Uid * enhancement(4889): refactored isOwnerExec return * enhancement(4889): differentiating between sid errors * enhancment(4889): removed redundant unit tests * enhancement(4889): update command execution in integration tests --------- Co-authored-by: Paolo Chilà <[email protected]> (cherry picked from commit c4835ca)

* enhancement(4889): updated enroll command to reexec the command if the executing user and binary owner don't match, added tests * enhancement(4889): added savepassword function * enhancement(4889): removed user impersonation, implemented noop for windows, updated integration tests * enhancement(4889): added changelog * enhancement(4889): added windows implementation * enhancement(4889): added license headers * enhancement(4889): update test case to execute different commands based on os * enhancement(4889): ran mage clean * enhancement(4889): updated function name, updated integration tests * enhancmenet(4889): updated function name * enhancement(4889): updated function name, fixed test assertions * enhancement(4889): update error messages * enhancement(4889): ran mage update * enhancements(4889): fix integration test * enhancement(4889): remove commented code * enhancement(4889): commiting now, don't push though * enhancement(4889): added windows unit test * enhancement(4889): close test file * enhancement(4889): updated isOwnerExec function, added tests * enchancement(4889): added log in test * enhancement(4889): remove unused types * enhancement(4889): added test for isOwnerExec for windows * enhancement(4889): added unix tests, ran mage addLicenseHeaders * enhancement(4889): added testisfileownerunix test * enhancement(4889): added TestIsOwnerExecUnix test * enhancement(4889): updated test function name * enhancement(4889): set file ownership of the test file * enhancement(4889): remove unnecessary change * enhancment(4889): updated iOwnerExec windows test * enhancement(4889): fix file creation in tests * Update internal/pkg/agent/cmd/enroll.go Co-authored-by: Paolo Chilà <[email protected]> * Update internal/pkg/agent/cmd/enroll_match_fileowner_windows.go Co-authored-by: Paolo Chilà <[email protected]> * Update internal/pkg/agent/cmd/enroll_match_fileowner_unix.go Co-authored-by: Paolo Chilà <[email protected]> * enhancement(4889): updated summary and issue in changelog * enhancement(4889): updated variable name, wrap error when returning error * enhancement(4889): added comments describing isOwnerExec. fixed type problems with stat.Uid * enhancement(4889): refactored isOwnerExec return * enhancement(4889): differentiating between sid errors * enhancment(4889): removed redundant unit tests * enhancement(4889): update command execution in integration tests --------- Co-authored-by: Paolo Chilà <[email protected]> (cherry picked from commit c4835ca) Co-authored-by: Kaan Yalti <[email protected]>

jlind23 · 2024-12-19T06:29:49Z

@kaanyalti shall we backport this to 8.17 too? For now by backporting to 8.x it will only be included in 8.18

kaanyalti · 2024-12-19T21:38:42Z

@jlind23 I think we can, I don't see any reason not to backport it to 8.17.

* enhancement(4889): updated enroll command to reexec the command if the executing user and binary owner don't match, added tests * enhancement(4889): added savepassword function * enhancement(4889): removed user impersonation, implemented noop for windows, updated integration tests * enhancement(4889): added changelog * enhancement(4889): added windows implementation * enhancement(4889): added license headers * enhancement(4889): update test case to execute different commands based on os * enhancement(4889): ran mage clean * enhancement(4889): updated function name, updated integration tests * enhancmenet(4889): updated function name * enhancement(4889): updated function name, fixed test assertions * enhancement(4889): update error messages * enhancement(4889): ran mage update * enhancements(4889): fix integration test * enhancement(4889): remove commented code * enhancement(4889): commiting now, don't push though * enhancement(4889): added windows unit test * enhancement(4889): close test file * enhancement(4889): updated isOwnerExec function, added tests * enchancement(4889): added log in test * enhancement(4889): remove unused types * enhancement(4889): added test for isOwnerExec for windows * enhancement(4889): added unix tests, ran mage addLicenseHeaders * enhancement(4889): added testisfileownerunix test * enhancement(4889): added TestIsOwnerExecUnix test * enhancement(4889): updated test function name * enhancement(4889): set file ownership of the test file * enhancement(4889): remove unnecessary change * enhancment(4889): updated iOwnerExec windows test * enhancement(4889): fix file creation in tests * Update internal/pkg/agent/cmd/enroll.go Co-authored-by: Paolo Chilà <[email protected]> * Update internal/pkg/agent/cmd/enroll_match_fileowner_windows.go Co-authored-by: Paolo Chilà <[email protected]> * Update internal/pkg/agent/cmd/enroll_match_fileowner_unix.go Co-authored-by: Paolo Chilà <[email protected]> * enhancement(4889): updated summary and issue in changelog * enhancement(4889): updated variable name, wrap error when returning error * enhancement(4889): added comments describing isOwnerExec. fixed type problems with stat.Uid * enhancement(4889): refactored isOwnerExec return * enhancement(4889): differentiating between sid errors * enhancment(4889): removed redundant unit tests * enhancement(4889): update command execution in integration tests --------- Co-authored-by: Paolo Chilà <[email protected]> (cherry picked from commit c4835ca)

* enhancement(4889): updated enroll command to reexec the command if the executing user and binary owner don't match, added tests * enhancement(4889): added savepassword function * enhancement(4889): removed user impersonation, implemented noop for windows, updated integration tests * enhancement(4889): added changelog * enhancement(4889): added windows implementation * enhancement(4889): added license headers * enhancement(4889): update test case to execute different commands based on os * enhancement(4889): ran mage clean * enhancement(4889): updated function name, updated integration tests * enhancmenet(4889): updated function name * enhancement(4889): updated function name, fixed test assertions * enhancement(4889): update error messages * enhancement(4889): ran mage update * enhancements(4889): fix integration test * enhancement(4889): remove commented code * enhancement(4889): commiting now, don't push though * enhancement(4889): added windows unit test * enhancement(4889): close test file * enhancement(4889): updated isOwnerExec function, added tests * enchancement(4889): added log in test * enhancement(4889): remove unused types * enhancement(4889): added test for isOwnerExec for windows * enhancement(4889): added unix tests, ran mage addLicenseHeaders * enhancement(4889): added testisfileownerunix test * enhancement(4889): added TestIsOwnerExecUnix test * enhancement(4889): updated test function name * enhancement(4889): set file ownership of the test file * enhancement(4889): remove unnecessary change * enhancment(4889): updated iOwnerExec windows test * enhancement(4889): fix file creation in tests * Update internal/pkg/agent/cmd/enroll.go Co-authored-by: Paolo Chilà <[email protected]> * Update internal/pkg/agent/cmd/enroll_match_fileowner_windows.go Co-authored-by: Paolo Chilà <[email protected]> * Update internal/pkg/agent/cmd/enroll_match_fileowner_unix.go Co-authored-by: Paolo Chilà <[email protected]> * enhancement(4889): updated summary and issue in changelog * enhancement(4889): updated variable name, wrap error when returning error * enhancement(4889): added comments describing isOwnerExec. fixed type problems with stat.Uid * enhancement(4889): refactored isOwnerExec return * enhancement(4889): differentiating between sid errors * enhancment(4889): removed redundant unit tests * enhancement(4889): update command execution in integration tests --------- Co-authored-by: Paolo Chilà <[email protected]> (cherry picked from commit c4835ca) Co-authored-by: Kaan Yalti <[email protected]>

mergify bot assigned kaanyalti Nov 25, 2024

mergify bot added the backport-8.x Automated backport to the 8.x branch with mergify label Nov 25, 2024

kaanyalti mentioned this pull request Nov 25, 2024

enhancement(4889): added check to abort enrolling if user is privileg… #6038

Closed

4 tasks

pierrehilbert added the Team:Elastic-Agent-Control-Plane Label for the Agent Control Plane team label Nov 26, 2024

kaanyalti changed the title ~~Reexec enroll command if the user and file owner don't match~~ Add error message to linux and mac for the enroll command Nov 26, 2024

kaanyalti force-pushed the enhancement/4889_enroll_match_file_owner branch from 6834e0b to 38be49e Compare November 26, 2024 21:35

kaanyalti marked this pull request as ready for review November 26, 2024 21:35

kaanyalti requested a review from a team as a code owner November 26, 2024 21:35

kaanyalti requested review from michalpristas and pchila November 26, 2024 21:35

kaanyalti changed the title ~~Add error message to linux and mac for the enroll command~~ Return actionable error message when enrolling Nov 26, 2024

kaanyalti force-pushed the enhancement/4889_enroll_match_file_owner branch from 5718b15 to 1d02094 Compare November 27, 2024 10:25

michalpristas reviewed Nov 27, 2024

View reviewed changes

internal/pkg/agent/cmd/enroll_match_fileowner_unix.go Outdated Show resolved Hide resolved

blakerouse reviewed Nov 28, 2024

View reviewed changes

kaanyalti force-pushed the enhancement/4889_enroll_match_file_owner branch 3 times, most recently from 4da5297 to 7e8b714 Compare December 7, 2024 03:33

kaanyalti requested review from blakerouse and michalpristas December 7, 2024 03:38

michalpristas reviewed Dec 11, 2024

View reviewed changes

pchila reviewed Dec 11, 2024

View reviewed changes

kaanyalti force-pushed the enhancement/4889_enroll_match_file_owner branch 2 times, most recently from 346eed7 to 49d887b Compare December 13, 2024 15:47

kaanyalti requested review from pchila and michalpristas December 13, 2024 23:05

michalpristas approved these changes Dec 16, 2024

View reviewed changes

kaanyalti and others added 17 commits December 18, 2024 11:08

enhancement(4889): added testisfileownerunix test

9c2fba8

enhancement(4889): added TestIsOwnerExecUnix test

df5ddb7

enhancement(4889): updated test function name

0ab7181

enhancement(4889): set file ownership of the test file

5a3df58

enhancement(4889): remove unnecessary change

e89e126

enhancment(4889): updated iOwnerExec windows test

9c67aea

enhancement(4889): fix file creation in tests

ab4e753

Update internal/pkg/agent/cmd/enroll.go

1f66c83

Co-authored-by: Paolo Chilà <[email protected]>

Update internal/pkg/agent/cmd/enroll_match_fileowner_windows.go

8e05185

Co-authored-by: Paolo Chilà <[email protected]>

Update internal/pkg/agent/cmd/enroll_match_fileowner_unix.go

ffba277

Co-authored-by: Paolo Chilà <[email protected]>

enhancement(4889): updated summary and issue in changelog

81b3e60

enhancement(4889): updated variable name, wrap error when returning e…

f152fcc

…rror

enhancement(4889): added comments describing isOwnerExec. fixed type …

f7a9367

…problems with stat.Uid

enhancement(4889): refactored isOwnerExec return

58b7e1e

enhancement(4889): differentiating between sid errors

4ef5ca7

enhancment(4889): removed redundant unit tests

65f6592

enhancement(4889): update command execution in integration tests

ce809c1

kaanyalti force-pushed the enhancement/4889_enroll_match_file_owner branch from 340cd78 to ce809c1 Compare December 18, 2024 16:08

kaanyalti merged commit c4835ca into elastic:main Dec 19, 2024
13 checks passed

kaanyalti deleted the enhancement/4889_enroll_match_file_owner branch December 19, 2024 02:58

mergify bot mentioned this pull request Dec 19, 2024

[8.x](backport #6144) Return actionable error message when enrolling #6397

Merged

4 tasks

ycombinator added the backport-8.17 Automated backport with mergify label Dec 20, 2024

mergify bot mentioned this pull request Dec 20, 2024

[8.17](backport #6144) Return actionable error message when enrolling #6421

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Return actionable error message when enrolling #6144

Return actionable error message when enrolling #6144

kaanyalti commented Nov 25, 2024 •

edited

Loading

mergify bot commented Nov 25, 2024

mergify bot commented Nov 25, 2024

elasticmachine commented Nov 26, 2024

blakerouse Nov 28, 2024

kaanyalti Dec 2, 2024

mergify bot commented Dec 5, 2024

pchila left a comment

pchila Dec 11, 2024

kaanyalti Dec 11, 2024 •

edited

Loading

michalpristas left a comment •

edited

Loading

elastic-sonarqube bot commented Dec 18, 2024

jlind23 commented Dec 19, 2024

kaanyalti commented Dec 19, 2024

Return actionable error message when enrolling #6144

Return actionable error message when enrolling #6144

Conversation

kaanyalti commented Nov 25, 2024 • edited Loading

What does this PR do?

Why is it important?

Checklist

How to test this PR locally

Related issues

mergify bot commented Nov 25, 2024

mergify bot commented Nov 25, 2024

elasticmachine commented Nov 26, 2024

blakerouse Nov 28, 2024

Choose a reason for hiding this comment

kaanyalti Dec 2, 2024

Choose a reason for hiding this comment

mergify bot commented Dec 5, 2024

pchila left a comment

Choose a reason for hiding this comment

pchila Dec 11, 2024

Choose a reason for hiding this comment

kaanyalti Dec 11, 2024 • edited Loading

Choose a reason for hiding this comment

michalpristas left a comment • edited Loading

Choose a reason for hiding this comment

elastic-sonarqube bot commented Dec 18, 2024

Quality Gate passed

jlind23 commented Dec 19, 2024

kaanyalti commented Dec 19, 2024

kaanyalti commented Nov 25, 2024 •

edited

Loading

kaanyalti Dec 11, 2024 •

edited

Loading

michalpristas left a comment •

edited

Loading