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

[agentbeat] Fix agentbeat capabilities #40466

Merged
merged 8 commits into from
Aug 12, 2024

Conversation

VihasMakwana
Copy link
Contributor

@VihasMakwana VihasMakwana commented Aug 8, 2024

Proposed commit message

  • Allow agentbeat to elevate permissions to match the permitted set.
  • These are required for non-root use to operate agent in healthy 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.next.asciidoc or CHANGELOG-developer.next.asciidoc.

Disruptive User Impact

How to test this PR locally

securityContext:
  runAsUser: 1000
  privileged: false
  capabilities:
   add:
    - DAC_OVERRIDE
    - SYS_PTRACE
    - SETPCAP
    - CHOWN
   drop:
    - ALL

Related issues

Screenshots

Current behaviour, without enough capabilities:

Screenshot 2024-08-09 at 2 18 30 AM

Behaviour after the changes and elastic/elastic-agent#5271:

Screenshot 2024-08-09 at 2 20 37 AM

@VihasMakwana VihasMakwana requested a review from a team as a code owner August 8, 2024 20:52
@botelastic botelastic bot added the needs_team Indicates that the issue/PR needs a Team:* label label Aug 8, 2024
@VihasMakwana VihasMakwana changed the title [agentbeat] Fix agentbeat caps [agentbeat] Fix agentbeat capabilities Aug 8, 2024
@VihasMakwana VihasMakwana requested a review from blakerouse August 8, 2024 20:53
Copy link
Contributor

mergify bot commented Aug 8, 2024

This pull request does not have a backport label.
If this is a bug or security fix, could you label this PR @VihasMakwana? 🙏.
For such, you'll need to label your PR with:

  • The upcoming major version of the Elastic Stack
  • The upcoming minor version of the Elastic Stack (if you're not pushing a breaking change)

To fixup this pull request, you need to add the backport labels for the needed
branches, such as:

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

@VihasMakwana VihasMakwana added Team:Elastic-Agent-Data-Plane Label for the Agent Data Plane team backport-8.15 Automated backport to the 8.15 branch with mergify labels Aug 8, 2024
@elasticmachine
Copy link
Collaborator

Pinging @elastic/elastic-agent-data-plane (Team:Elastic-Agent-Data-Plane)

@botelastic botelastic bot removed the needs_team Indicates that the issue/PR needs a Team:* label label Aug 8, 2024
@pierrehilbert pierrehilbert added the Team:Elastic-Agent-Control-Plane Label for the Agent Control Plane team label Aug 9, 2024
@elasticmachine
Copy link
Collaborator

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

Copy link
Contributor

mergify bot commented Aug 9, 2024

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 fix-agentbeat-caps upstream/fix-agentbeat-caps
git merge upstream/main
git push upstream fix-agentbeat-caps

)

func initCapabilities() {
isRoot, err := utils.HasRoot()
Copy link
Member

Choose a reason for hiding this comment

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

This entire block of code is Linux specific, you don't need to also duplicate the Windows and MacOS HasRoot implementations. You can just make the implementation of initCapabilities a no-op on those OSes instead.

If we want to share HasRoot put them in elastic-agent-libs, the implementation particularly for Windows is non-trivial and has had bugs recently.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've removed it for other OSes.
Can you take a look?

@VihasMakwana VihasMakwana requested a review from cmacknz August 9, 2024 20:59
@VihasMakwana VihasMakwana merged commit 36cef2f into elastic:main Aug 12, 2024
11 checks passed
mergify bot pushed a commit that referenced this pull request Aug 12, 2024
* chore: add effective capabiltiies in agentbeat

* chore: comments

* fix: changelog

* fix: add license

* fix: lint

* fix: goimports

* fix: remove HasRoot for other OSes

---------

Co-authored-by: Pierre HILBERT <pierre.hilbert@elastic.co>
(cherry picked from commit 36cef2f)
Copy link
Contributor

@blakerouse blakerouse left a comment

Choose a reason for hiding this comment

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

I do not believe this should have been merged this way. Overall setting capabilities is not an agentbeat specific thing, it should be set per beat type and only for the types that the specific beat needs. This seems to just be a promote all from from effective and inheritable to the permitted set, from a security perspective this is not great. It should be limiting to only what each beat type needs. Agentbeat itself is not a beat it is a metabeat and should not be in-charge of this work directly.

I also feel like merging this as is, was not the correct thing todo. We have similar logic in Elastic Agent and this needs to be moved to each individual beat so this code should be moved to a generic location first, and not merged to then be moved later. We should strive for cleanliness even if it delays the merge some time. It would also be interesting to know if OTEL already has something like this? If so is it in a library that could be re-used? If not does another library already exist that can be re-used or should be make our own module for this specific logic?

I had been thinking about this PR over the weekend, I was not expecting it to be merged so quickly.

@pkoutsovasilis
Copy link
Contributor

I did an experiment and reverted this PR as in my thinking it shouldn't be necessary for this code to exist in agentbeat. This is because elastic-agent, after merging this PR, when running rootless is raising all capabilities from Bounding set (specified in k8s manifest) to it's Ambient set to allow subprocesses maintain them. As shown below k8s tests with this PR and without the code here are running without failures

Screenshot 2024-08-13 at 4 36 39 PM

vinit-chauhan pushed a commit to vinit-chauhan/beats that referenced this pull request Aug 13, 2024
* chore: add effective capabiltiies in agentbeat

* chore: comments

* fix: changelog

* fix: add license

* fix: lint

* fix: goimports

* fix: remove HasRoot for other OSes

---------

Co-authored-by: Pierre HILBERT <pierre.hilbert@elastic.co>
blakerouse added a commit that referenced this pull request Aug 13, 2024
blakerouse added a commit that referenced this pull request Aug 14, 2024
* Revert "[agentbeat] Fix agentbeat capabilities (#40466)"

This reverts commit 36cef2f.

* Update main.go
rdner added a commit that referenced this pull request Aug 15, 2024
* [agentbeat] Fix agentbeat capabilities (#40466)

* chore: add effective capabiltiies in agentbeat

* chore: comments

* fix: changelog

* fix: add license

* fix: lint

* fix: goimports

* fix: remove HasRoot for other OSes

---------

Co-authored-by: Pierre HILBERT <pierre.hilbert@elastic.co>
(cherry picked from commit 36cef2f)

* Fix the changelog

---------

Co-authored-by: VihasMakwana <121151420+VihasMakwana@users.noreply.github.com>
Co-authored-by: Denis <denis.rechkunov@elastic.co>
mergify bot pushed a commit that referenced this pull request Aug 21, 2024
* Revert "[agentbeat] Fix agentbeat capabilities (#40466)"

This reverts commit 36cef2f.

* Update main.go

(cherry picked from commit f6b8701)
blakerouse added a commit that referenced this pull request Aug 21, 2024
…s" (#40575)

* Revert "[agentbeat] Fix agentbeat capabilities" (#40510)

* Revert "[agentbeat] Fix agentbeat capabilities (#40466)"

This reverts commit 36cef2f.

* Update main.go

(cherry picked from commit f6b8701)

* Update CHANGELOG.next.asciidoc

---------

Co-authored-by: Blake Rouse <blake.rouse@elastic.co>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-8.15 Automated backport to the 8.15 branch with mergify Team:Elastic-Agent-Control-Plane Label for the Agent Control Plane team Team:Elastic-Agent-Data-Plane Label for the Agent Data Plane team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants