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

kaanyalti · 2025-06-14T06:56:58Z

Type of change:

Bug

What does this PR do?

This PR updates the Windows enrollment logic in Elastic Agent, specifically addressing the overly strict file ownership checks that were introduced in a previous fix. Instead of removing the Windows-specific logic, this change implements the file ownership check as a no-op for Windows. The platform-specific files remain, but the check is now always allowed on Windows. This is a partial revert of the earlier logic, with the intent to restore expected behavior for privileged users on Windows.

Why is it important?

The original bug (see issue #4889, fixed in PR #6144) was that if the agent was installed unprivileged, running enroll as a privileged user would break the agent. The fix at the time was to disallow re-enrollment as root if the agent was installed unprivileged, using a strict file owner check. This worked for Unix, where impersonation is easy, but on Windows, due to how the elastic-agent-user is set up, impersonation is not feasible.

The strict SID check on Windows turned out to be too restrictive: even if the agent was installed as an admin, other admin users could not re-enroll, leading to the current issue (#7794). The workaround (changing file ownership to SYSTEM) was not reliable.

The changes in this PR implement the ownership check as a no-op for Windows, allowing any admin to re-enroll. The platform-specific files are retained for clarity and maintainability. This change also reintroduces the original risk: if the agent is installed unprivileged, a privileged user can still break the agent by re-enrolling. The platform-specific files are retained for clarity and maintainability.

Related issues

Fixes #7794: Unable to re-enrolled installed Elastic Agent on Windows
Original bug and fix: #4889, PR #6144

Checklist

I have read and understood the pull request guidelines of this project.
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

Disruptive User Impact

Windows users who previously could not re-enroll the agent as an admin should now be able to do so.
The original risk (privileged re-enrollment breaking unprivileged installs) is reintroduced for Windows.

How to test this PR locally

Build the agent on Windows.
Attempt to enroll, unenroll, and re-enroll the agent as both privileged and unprivileged users.
Privileged re-enroll should work. Unprivileged re-enroll should fail.
Run the renamed integration test (re-enroll_test.go) to verify correct behavior.

Related issues

- 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.

mergify · 2025-06-14T06:57:41Z

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 that automatically backports to the 8./d branch. /d is the digit
backport-active-all is the label that automatically backports to all active branches.
backport-active-8 is the label that automatically backports to all active minor branches for the 8 major.
backport-active-9 is the label that automatically backports to all active minor branches for the 9 major.

…ws-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.

elasticmachine · 2025-06-15T15:06:30Z

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

testing/integration/re-enroll_test.go

- 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.

…where in the test function define is called

swiatekm

The fix and added test look good to me. I'd like some clarity on what the follow-up will be. Are we planning to address the Windows case, or are we resigned to leaving it as-is?

kaanyalti · 2025-06-16T16:51:16Z

The fix and added test look good to me. I'd like some clarity on what the follow-up will be. Are we planning to address the Windows case, or are we resigned to leaving it as-is?

In short yes, we are going to address the issue. The actual fix would be to enable root users to execute a re-enroll without breaking the agent. I'll create an issue and link it here.

kaanyalti · 2025-06-16T17:13:10Z

Added the follow up bug ticket in the description #8544

testing/integration/re-enroll_test.go

cmacknz · 2025-06-16T20:08:31Z

I tested this manually on a GCP VM and confirmed that install+enroll followed by enroll does not work for 9.0.2 but does with this PR.

internal/pkg/agent/cmd/enroll_match_fileowner_windows.go

…rror to enroll.go

testing/integration/re-enroll_test.go

swiatekm

LGTM as long as Craig's comments are addressed.

…ivileged test case

elastic-sonarqube · 2025-06-17T18:31:05Z

Quality Gate passed

Issues
0 New issues
0 Fixed issues
0 Accepted issues

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

See analysis details on SonarQube

elasticmachine · 2025-06-17T19:55:03Z

💔 Build Failed

Buildkite Build
Commit: d4018c7

Failed CI Steps

History

💚 Build #22546 succeeded 5754a00
💛 Build #22537 was flaky cc4be8b
💔 Build #22529 failed b4e2825
💔 Build #22526 failed aa24f1c

cc @kaanyalti

* 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) # Conflicts: # testing/integration/enroll_unprivileged_test.go

* 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)

…#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 <kaan.yalti@elastic.co>

…#8571) * 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 <kaan.yalti@elastic.co>

…and rename test (#8568) * fix(7794): update Windows re-enrollment logic and rename test (#8503) * 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) # Conflicts: # testing/integration/enroll_unprivileged_test.go * Delete testing/integration/enroll_unprivileged_test.go --------- Co-authored-by: Kaan Yalti <kaan.yalti@elastic.co>

…#8569) * 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 <kaan.yalti@elastic.co>

kaanyalti requested a review from a team as a code owner June 14, 2025 06:56

kaanyalti requested review from michel-laterman and swiatekm June 14, 2025 06:56

mergify bot assigned kaanyalti Jun 14, 2025

kaanyalti added 3 commits June 14, 2025 13:48

changelog: add fragment for Windows re-enrollment bugfix

9465df4

fix(7794): remove unnecessary build tag

1cdec51

pierrehilbert added the Team:Elastic-Agent-Control-Plane Label for the Agent Control Plane team label Jun 15, 2025

swiatekm reviewed Jun 16, 2025

View reviewed changes

testing/integration/re-enroll_test.go Outdated Show resolved Hide resolved

cmacknz mentioned this pull request Jun 16, 2025

Unable to re-enrolled installed Elastic Agent on Windows #7794

Closed

kaanyalti added 2 commits June 16, 2025 11:26

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

12a10d0

kaanyalti requested a review from swiatekm June 16, 2025 16:06

fix(7794): reverted to individual test functions because ci enforces …

aa24f1c

…where in the test function define is called

swiatekm reviewed Jun 16, 2025

View reviewed changes

fix(7794): removed unnecessary var definition

b4e2825

kaanyalti mentioned this pull request Jun 16, 2025

Enable Safe Re-enrollment by Root/Admin for Unprivileged Elastic Agent Installs on All Platforms #8544

Closed

kaanyalti requested a review from swiatekm June 16, 2025 17:13

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

cc4be8b

cmacknz reviewed Jun 16, 2025

View reviewed changes

testing/integration/re-enroll_test.go Outdated Show resolved Hide resolved

cmacknz reviewed Jun 16, 2025

View reviewed changes

testing/integration/re-enroll_test.go Outdated Show resolved Hide resolved

kaanyalti requested a review from ycombinator June 16, 2025 19:28

cmacknz reviewed Jun 16, 2025

View reviewed changes

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

kaanyalti added 2 commits June 16, 2025 23:38

fix(7794): added status check after second enroll, moved file owner e…

5754a00

…rror to enroll.go

fix(7794): remove unenroll step from integration tests

f9eec79

kaanyalti requested a review from cmacknz June 17, 2025 14:40

cmacknz reviewed Jun 17, 2025

View reviewed changes

testing/integration/re-enroll_test.go Outdated Show resolved Hide resolved

swiatekm previously approved these changes Jun 17, 2025

View reviewed changes

fix(7794): refactored test code

5e27f53

kaanyalti dismissed swiatekm’s stale review via 5e27f53 June 17, 2025 17:03

fix(7794): added comment explaining why windows is excluded from unpr…

d4018c7

…ivileged test case

cmacknz approved these changes Jun 17, 2025

View reviewed changes

kaanyalti mentioned this pull request Jun 17, 2025

[REQUEST]: Document windows bug that prevents admin users to re-enroll agents. elastic/ingest-docs#1824

Open

ebeahan merged commit 00f1662 into elastic:main Jun 17, 2025
17 of 19 checks passed

mergify bot mentioned this pull request Jun 17, 2025

[8.17](backport #8503) fix(7794): update Windows re-enrollment logic and rename test #8568

Merged

5 tasks

This was referenced Jun 17, 2025

[8.18](backport #8503) fix(7794): update Windows re-enrollment logic and rename test #8569

Merged

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

Merged

mergify bot mentioned this pull request Jun 17, 2025

[9.0](backport #8503) fix(7794): update Windows re-enrollment logic and rename test #8571

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

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

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

Uh oh!

kaanyalti commented Jun 14, 2025 •

edited

Loading

mergify bot commented Jun 14, 2025

elasticmachine commented Jun 15, 2025

Uh oh!

swiatekm left a comment

kaanyalti commented Jun 16, 2025

kaanyalti commented Jun 16, 2025

Uh oh!

Uh oh!

cmacknz commented Jun 16, 2025

Uh oh!

Uh oh!

swiatekm left a comment

elastic-sonarqube bot commented Jun 17, 2025

elasticmachine commented Jun 17, 2025 •

edited

Loading

Uh oh!

Labels

6 participants

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

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

Uh oh!

Conversation

kaanyalti commented Jun 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What does this PR do?

Why is it important?

Related issues

Checklist

Disruptive User Impact

How to test this PR locally

Related issues

mergify bot commented Jun 14, 2025

elasticmachine commented Jun 15, 2025

Uh oh!

swiatekm left a comment

Choose a reason for hiding this comment

kaanyalti commented Jun 16, 2025

kaanyalti commented Jun 16, 2025

Uh oh!

Uh oh!

cmacknz commented Jun 16, 2025

Uh oh!

Uh oh!

swiatekm left a comment

Choose a reason for hiding this comment

elastic-sonarqube bot commented Jun 17, 2025

Quality Gate passed

elasticmachine commented Jun 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

💔 Build Failed

Failed CI Steps

History

Uh oh!

Labels

6 participants

kaanyalti commented Jun 14, 2025 •

edited

Loading

elasticmachine commented Jun 17, 2025 •

edited

Loading