Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add unprivileged and privileged subcommand to Elastic Agent #4621

Merged
merged 29 commits into from
Jun 12, 2024

Conversation

blakerouse
Copy link
Contributor

@blakerouse blakerouse commented Apr 25, 2024

What does this PR do?

Adds a new unprivileged and privileged subcommand that converts an installed Elastic Agent from privileged to unprivileged or back again.

Why is it important?

To allow users to change the permission execution mode without having to re-install the Elastic Agent.

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 (will have integration tests)
  • 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

  • can convert from privileged to unprivileged
  • can convert from unprivileged to privileged

How to test this PR locally

$ sudo elastic-agent unprivileged
$ sudo elastic-agent privileged

Related issues

@blakerouse blakerouse added Team:Elastic-Agent-Control-Plane Label for the Agent Control Plane team backport-skip labels Apr 25, 2024
@blakerouse blakerouse self-assigned this Apr 25, 2024
@blakerouse blakerouse changed the title Work on privileged/unprivileged command. Add unprivileged and privileged subcommand to Elastic Agent Apr 25, 2024
@blakerouse blakerouse marked this pull request as ready for review May 9, 2024 14:50
@blakerouse blakerouse requested a review from a team as a code owner May 9, 2024 14:50
@elasticmachine
Copy link
Contributor

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

@blakerouse
Copy link
Contributor Author

SonarQube you are correct about unit tests, but you are very wrong about integration tests...

This has integration tests for both privileged and unprivileged going in both directions.

@ycombinator ycombinator removed the request for review from andrzej-stencel May 9, 2024 21:50
Comment on lines 69 to 70
// cannot switch to unprivileged when Elastic Defend exists in the policy
err = ensureNoElasticDefend()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Normally osquery is running as privileged. Quite a lot of functionality will be unavailable if running unprivileged or returning different results.

Osqueryd also checks for "safe permissions" when it loads our custom extension.

 When osquery launches, each path is evaluated for "safe permissions" 
(extension executable files must be owned by root or Administrator) 
and executed as a monitored child process

https://osquery.readthedocs.io/en/stable/deployment/extensions/#autoloading-extensions

If you observe a runtime error from osquery, Extension binary has unsafe permissions, 
you have to lock down the filesystem permissions on the extension executable. 
See the steps in "Extensions Binary Permissions," above.

https://osquery.readthedocs.io/en/stable/deployment/extensions/#troubleshooting

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For osquery, you should mark the integration as requiring root privileges like defend does in https:/elastic/endpoint-package/blob/ce921805467ab55953adc3721e7db8581915bd35/package/endpoint/manifest.yml#L23-L25. You can also do this for individual data streams but I don't think that applies to OSquery.

You could also add a prevention to the spec file to prevent us from even starting osquery if you aren't root, but since it doesn't fundamentally not work when not root, this might not be necessary.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you do update the spec, you would want to update the agentbeat spec.

return nil
}

func ensureNoServiceComponentIssues() error {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this has any test coverage. You could add a check in the endpoint test suite that the unprivileged command fails, or add a unit test.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True I will add an integration test, as I would prefer to see a true scenario be tested.

@blakerouse
Copy link
Contributor Author

I have validated manually that this works on macOS, because we do not have integration testing coverage on macOS. I was able to switch between the modes successfully and the vaults are handled properly.

return cmd
}

func unprivilegedCmd(streams *cli.IOStreams, cmd *cobra.Command) (err error) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: seems like there's quite some repetition meaning this and privilegedCmd function

@blakerouse blakerouse enabled auto-merge (squash) June 10, 2024 16:31
@blakerouse blakerouse disabled auto-merge June 10, 2024 16:31
@blakerouse blakerouse enabled auto-merge (squash) June 10, 2024 16:31
Copy link

Quality Gate failed Quality Gate failed

Failed conditions
2.3% Coverage on New Code (required ≥ 40%)

See analysis details on SonarQube

@jlind23
Copy link
Contributor

jlind23 commented Jun 12, 2024

@ycombinator feel free to force merge this is required as it seems sonar coverage is the only missing condition.
There are tests it is just that sonar calculates coverage using exclusively unit and not integration tests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-skip Team:Elastic-Agent-Control-Plane Label for the Agent Control Plane team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants