Enable unprivileged on macos by pchila · Pull Request #4362 · elastic/elastic-agent

pchila · 2024-03-05T20:15:33Z

What does this PR do?

This PR allows running elastic-agent (either installed or just extracted from the package) as an unprivileged user, using a file-based vault as fallback instead of the keychain-based one.

When creating an EncryptedDiskStore, we can specify an unprivileged flag which will instantiate the file-based vault on Mac OS (the default value of such flag is defined as whether elastic-agent is running with administrative rights).

In most cases, such default value is enough to choose the correct vault implementation: the exception is elastic-agent uninstall command where a specific check for a file vault containing the agent secret has been added.
The additional check during uninstall is necessary since the command needs to run with administrative rights for both privileged and unprivileged elastic-agent installs (a simple check of the user permissions would not be useful as we would always come up with privileged permissions)

Why is it important?

This change enables elastic-agent to be installed and run as an unprivileged user on Mac OS

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

Author's Checklist

[ ]

How to test this PR locally

Related issues

Closes [Unprivileged] Enable --unprivileged on macOS #3867

Use cases

Screenshots

Logs

Questions to ask yourself

How are we going to support this in production?
How are we going to measure its adoption?
How are we going to debug this?
What are the metrics I should take care of?
...

mergify · 2024-03-05T20:17:03Z

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

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

NOTE: backport-skip has been added to this pull request.

elasticmachine · 2024-03-11T14:15:32Z

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

blakerouse · 2024-03-11T15:01:30Z

internal/pkg/agent/cmd/run.go

+	// Deprecated: MacOS can be run/installed without root privileges
 	cmd.Flags().Bool("disable-encrypted-store", false, "Disable the encrypted disk storage (Only useful on Mac OS)")
+	_ = cmd.Flags().MarkHidden("disable-encrypted-store")
+	_ = cmd.Flags().MarkDeprecated("disable-encrypted-store", "agent on Mac OS can be run/installed without root privileges, see elastic-agent install --help")


blakerouse · 2024-03-11T15:02:35Z

internal/pkg/agent/cmd/run.go

+	// try early to check if running as root
+	isRoot, err := utils.HasRoot()
+	if err != nil {
+		return fmt.Errorf("failed to check for root permissions: %w", err)


The others you added used the word administrator and this one used root. I believe that was just because this was moved. I would like to see this consitent, maybe root/Administrator would be better and work for unix and windows.

cleaned up a bit in b508570

blakerouse · 2024-03-11T15:03:37Z

internal/pkg/agent/storage/encrypted_disk_store.go

+
+	hasRoot, err := utils.HasRoot()
+	if err != nil || !hasRoot {
+		// TODO log error


Propagated the error in https://github.com/elastic/elastic-agent/pull/4362/files/6b2dc5973182afc60dd44aad888f291e57c3adba..78425bf5734c815486b57c4430bb5e6c0daf642a

cmacknz · 2024-03-11T17:47:26Z

We don't have a MacOS runner for integration tests yet. What is the plan for ensuring this works with the same level of confidence as Windows and Linux? We have a manual QA team that can help verify this, but we'd have to provide a detailed list of test cases first.

pchila · 2024-03-11T18:29:16Z

We don't have a MacOS runner for integration tests yet. What is the plan for ensuring this works with the same level of confidence as Windows and Linux? We have a manual QA team that can help verify this, but we'd have to provide a detailed list of test cases first.

Not sure what other testcases may come to mind but here's what I have tested:

elastic-agent run from an uncompressed elastic-agent package as a regular Mac user
sudo elastic-agent install --unprivileged for standalone mode and fleet managed

Aside from checking manually that privileged still works I cannot think of specific additional scenarios to test.
Maybe we can add a testcase with endpoint to verify that it doesn't run unless we install agent as privileged?

We have a real problem with automated testing for this kind of changes, unit tests are unfeasible without massive refactoring in the touched parts of code (I would like to avoid a big-bang scenario) and we don't have runners for integration tests on Mac as you pointed out.

blakerouse

Looks great!

Thanks for adding tests where you could, a lot of these areas are really hard to unit tests.

cmacknz · 2024-03-12T18:21:04Z

Not sure what other testcases may come to mind but here's what I have tested:

elastic-agent run from an uncompressed elastic-agent package as a regular Mac user

sudo elastic-agent install --unprivileged for standalone mode and fleet managed

Do upgrades work?

We have a real problem with automated testing for this kind of changes, unit tests are unfeasible without massive refactoring in the touched parts of code (I would like to avoid a big-bang scenario) and we don't have runners for integration tests on Mac as you pointed out.

Long term the answer is to add a MacOS integration test runner, I don't expect this to be covered purely with unit tests.

Checking if we are running as root when creating the EncryptedDiskStore removes the need for propagating an unprivileged flag from a lot of places in the code. Only exception is the uninstall command, since it needs to run with administrator privileges: in there we check for existence of a FileVault containing the agent secret to detect if we have to load the agent configuration as unprivileged (currently relevant only on Darwin)

elastic-sonarqube · 2024-03-13T06:57:36Z

Quality Gate failed

Failed conditions

33.3% 33.3% Coverage on New Code (required ≥ 40%)

See analysis details on SonarQube

pchila · 2024-03-13T07:01:27Z

Do upgrades work?

@cmacknz I just tested by repackaging the agent (we cannot downgrade as the old agent versions do not support running unprivileged) and everything works as expected when launching the upgrade with sudo -u elastic-agent elastic-agent upgrade ... so at least that still works

pierrehilbert · 2024-03-13T07:54:18Z

Long term the answer is to add a MacOS integration test runner, I don't expect this to be covered purely with unit tests.

cc @rowlandgeoff to ensure this is under your radar.

pierrehilbert · 2024-03-14T09:14:29Z

As discussed in our weekly meeting, this PR contains what is possible to be tested until we have our MacOS runner so I'm force merging even if SonarQube is not happy with the coverage level.

pchila added enhancement New feature or request Team:Elastic-Agent-Control-Plane Label for the Agent Control Plane team Team:Elastic-Agent Label for the Agent team labels Mar 5, 2024

pchila self-assigned this Mar 5, 2024

pchila force-pushed the enable-unprivileged-on-macos branch from 3a4bd8c to 29e64a8 Compare March 5, 2024 20:16

mergify bot added the backport-skip label Mar 5, 2024

pchila force-pushed the enable-unprivileged-on-macos branch 2 times, most recently from 980f9f8 to e5d7ea9 Compare March 11, 2024 14:14

pchila requested a review from blakerouse March 11, 2024 14:15

pchila marked this pull request as ready for review March 11, 2024 14:15

pchila requested a review from a team as a code owner March 11, 2024 14:15

pchila requested a review from leehinman March 11, 2024 14:15

blakerouse reviewed Mar 11, 2024

View reviewed changes

pchila mentioned this pull request Mar 11, 2024

Update MacOS keychain support to use non-deprecated API #2961

Open

pchila force-pushed the enable-unprivileged-on-macos branch from 87b84e7 to b508570 Compare March 12, 2024 13:30

pchila requested a review from blakerouse March 12, 2024 13:31

blakerouse approved these changes Mar 12, 2024

View reviewed changes

pchila added 8 commits March 13, 2024 07:28

create vault as unprivileged when running as non-root

2aa530b

Fix AgentInfo creation

24b3f21

Allow install --unprivileged on darwin

7dd2e71

Add --unprivileged flag to uninstall

e0edf2c

Deprecate '--disable-encrypted-store' switch on elastic-agent run

5c9218c

add unit test for uninstall unprivileged check

13b2542

fixup! add unit test for uninstall unprivileged check

f2ead68

pchila added 10 commits March 13, 2024 07:28

propagate errors when creating encrypted disk store

89feefa

fixup! propagate errors when creating encrypted disk store

d3a5630

changelog

f4a7bcf

fixup! fixup! propagate errors when creating encrypted disk store

9342415

Fix order of overrides in EncryptedDiskStore

60bf29f

fixup! Fix order of overrides in EncryptedDiskStore

0988b29

fixup! fixup! Fix order of overrides in EncryptedDiskStore

3f29f75

fixup! fixup! fixup! Fix order of overrides in EncryptedDiskStore

7700ef6

fixup! propagate errors when creating encrypted disk store

feb040d

fix log and error strings

795bbbe

pchila force-pushed the enable-unprivileged-on-macos branch from b508570 to 795bbbe Compare March 13, 2024 06:28

pierrehilbert merged commit 6135d60 into elastic:main Mar 14, 2024

ycombinator mentioned this pull request May 9, 2024

Document pre-requisites for running Elastic Agent in unprivileged mode #4705

Closed

Conversation

pchila commented Mar 5, 2024 • 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?

Checklist

Author's Checklist

How to test this PR locally

Related issues

Use cases

Screenshots

Logs

Questions to ask yourself

Uh oh!

mergify bot commented Mar 5, 2024

Uh oh!

elasticmachine commented Mar 11, 2024

Uh oh!

blakerouse Mar 11, 2024

Choose a reason for hiding this comment

Uh oh!

blakerouse Mar 11, 2024

Choose a reason for hiding this comment

Uh oh!

pchila Mar 12, 2024

Choose a reason for hiding this comment

Uh oh!

blakerouse Mar 11, 2024

Choose a reason for hiding this comment

Uh oh!

pchila Mar 11, 2024

Choose a reason for hiding this comment

Uh oh!

cmacknz commented Mar 11, 2024

Uh oh!

pchila commented Mar 11, 2024

Uh oh!

blakerouse left a comment

Choose a reason for hiding this comment

Uh oh!

cmacknz commented Mar 12, 2024

Uh oh!

elastic-sonarqube bot commented Mar 13, 2024

Quality Gate failed

Failed conditions

Uh oh!

pchila commented Mar 13, 2024

Uh oh!

pierrehilbert commented Mar 13, 2024

Uh oh!

pierrehilbert commented Mar 14, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

pchila commented Mar 5, 2024 •

edited

Loading