[Heartbeat] Unpack beats to enable capabilities inside container #30200

emilioalvap · 2022-02-03T22:56:01Z

What does this PR do?

This PR enables unpacking of beats inside the container at build time, so that required cap_net_raw, cap_setuid capabilities can be assigned to the binary.

Why is it important?

Without the required capabilities, heartbeat cannot execute ICMP pings or setuid calls. As it is now, agent is unpacking beats at runtime, most likely with a user that doesn't have permission to assign capabilities.

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.

Author's Checklist

[ ]

How to test this PR locally

Build elastic-agent containers, from x-pack/elastic-agent run:
env PLATFORMS="+all linux/amd64" mage dev:package
Run one of the built containers and provide some heartbeat configuration:

docker run --name agent -it -u root --env FLEET_ENROLL=1 --env \
FLEET_URL=<url> --env \ 
FLEET_ENROLLMENT_TOKEN=<token> \ 
docker.elastic.co/beats/elastic-agent:8.1.0-SNAPSHOT

Related issues

Relates [elastic-agent][heartbeat] Heartbeat binary should have setcap privs for ICMP ping #27651

Use cases

Screenshots

Logs

mergify · 2022-02-03T22:56:57Z

This pull request does not have a backport label. Could you fix it @emilioalvap? 🙏
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 7./d branch. /d is the digit

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

elasticmachine · 2022-02-03T23:28:15Z

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS

Expand to view the summary

Build stats

Start Time: 2022-02-24T14:42:42.541+0000
Duration: 229 min 34 sec

Test stats 🧪

Test	Results
Failed	0
Passed	38335
Skipped	3320
Total	41655

💚 Flaky test report

Tests succeeded.

🤖 GitHub comments

To re-run your PR in the CI, just comment with:

/test : Re-trigger the build.
/package : Generate the packages and run the E2E tests.
/beats-tester : Run the installation tests with beats-tester.
run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

andrewvc · 2022-02-04T01:19:43Z

dev-tools/packaging/packages.yml

@@ -482,6 +482,8 @@ shared:
      user: '{{ .BeatName }}'
      linux_capabilities: ''
      image_name: ''
+      unpack_beats: true


If we're saying this is the default behavior, do we need this option, or should we just always do this?

I think we should make this the default and remove this option.

Ok, removed the option and it's now unpacking by default

blakerouse

Overall looks acceptable, just some open questions and comments first.

blakerouse · 2022-02-08T18:28:28Z

dev-tools/packaging/packages.yml

@@ -482,6 +482,8 @@ shared:
      user: '{{ .BeatName }}'
      linux_capabilities: ''
      image_name: ''
+      unpack_beats: true


I think we should make this the default and remove this option.

blakerouse · 2022-02-08T18:29:11Z

dev-tools/packaging/templates/docker/Dockerfile.elastic-agent.tmpl

+RUN mkdir -p {{ $beatHome }}/data/{{.BeatName}}-{{ commit_short }}/{{ .beats_install_path }} && \
+    for beatPath in {{ $beatHome }}/data/{{.BeatName}}-{{ commit_short }}/downloads/*.tar.gz; do \
+    tar xf $beatPath -C {{ $beatHome }}/data/{{.BeatName}}-{{ commit_short }}/{{ .beats_install_path }} && \
+    rm $beatPath; \


We should not remove this $beatPath. It's not going to save any space in the image anyway because it is included in a previous layer.

True that, removed

blakerouse · 2022-02-08T18:29:28Z

dev-tools/packaging/templates/docker/Dockerfile.elastic-agent.tmpl

+    tar xf $beatPath -C {{ $beatHome }}/data/{{.BeatName}}-{{ commit_short }}/{{ .beats_install_path }} && \
+    rm $beatPath; \
+    done && \
+    chown -R {{ .user }} {{ $beatHome }}/data/{{.BeatName}}-{{ commit_short }}/{{ .beats_install_path }} && \


What does this leave the group as?

Group was root, changed that to be elastic-agent too.

blakerouse · 2022-02-08T18:30:14Z

dev-tools/packaging/templates/docker/Dockerfile.elastic-agent.tmpl

+    rm $beatPath; \
+    done && \
+    chown -R {{ .user }} {{ $beatHome }}/data/{{.BeatName}}-{{ commit_short }}/{{ .beats_install_path }} && \
+    setcap cap_net_raw,cap_setuid+p {{ $beatHome }}/data/{{.BeatName}}-{{ commit_short }}/{{ .beats_install_path }}/heartbeat-*/heartbeat


What is the cap_setuid+p for? Why is that needed?

Heartbeat requires it for switching user id at runtime

To add some detail here @blakerouse heartbeat runs node as a subprocess, and node hates to run as root. It's also just a best practice to setuid out of root if you can regardless.

Okay. It was just more of a question so I can understand. Thanks for the explanation.

blakerouse · 2022-03-08T21:04:17Z

@emilioalvap What is the status of this PR? Can we get it up for full review?

We believe this will also fix https://github.com/elastic/observability-dev/issues/1935.

emilioalvap · 2022-02-24T19:00:26Z

dev-tools/packaging/templates/docker/Dockerfile.elastic-agent.tmpl

+    chown -R root:root {{ $beatHome }}/data/{{.BeatName}}-{{ commit_short }}/{{ .beats_install_path }}/*/*.yml && \
+    chmod 0644 {{ $beatHome }}/data/{{.BeatName}}-{{ commit_short }}/{{ .beats_install_path }}/*/*.yml && \


@blakerouse @andrewvc These two are workarounds for a couple of issues I've found, I'd welcome your suggestions on how to fix those.

First one is related to permission checking in libbeat. This method is checking that config file owner is either root or executing user, which is not the case if container is extracted as elastic-agent and executed as root. At the moment, k8s templates for elastic-agent containers in docs portal use root user to execute pods, possibly for ECK templates too. The workaround is to change config file ownership to root.

The second one is related to this post-build check. Config files extracted from beat .tars do not follow the specified permission mask:

package_test.go:258: file usr/share/elastic-agent/data/elastic-agent-d732b3/install/osquerybeat-8.2.0-linux-x86_64/osquerybeat.yml has wrong permissions: expected=-rw-r--r-- actual=-rw-------

I wasn't sure what the rationale is behind this check and what the impact could be for another beats.

@emilioalvap I think the first workaround is okay, as Elastic Agent never writes this file.

As for the post-build check I see you do a chmod 0644 for the *.yml, can that be changed to chmod 0600 or would that break it?

@blakerouse Some of the beats' *.yml files come with 0600 and that is actually what breaks the test. 0644 seems to be the permission mask applied to *.yml files outside zipped beats. So now that we are unpacking beats, the test that used to check elastic.-agent *.yml files is also checking other beats config files.

@emilioalvap Still confused? So are you saying that its working with the change and the test is passing or we need to change either one?

@blakerouse Sure, let me clarify. I added the following to make the test pass:

chmod 0644 {{ $beatHome }}/data/{{.BeatName}}-{{ commit_short }}/{{ .beats_install_path }}/*/*.yml

But I wanted to make sure that this change wouldn't impact other beats in a way that I cannot foresee, as its changing file permission for all beats unpacked in the build phase.

emilioalvap added bug Team:obs-ds-hosted-services Label for the Observability Hosted Services team labels Feb 3, 2022

botelastic bot added needs_team Indicates that the issue/PR needs a Team:* label and removed needs_team Indicates that the issue/PR needs a Team:* label labels Feb 3, 2022

mergify bot added the backport-skip Skip notification from the automated backport with mergify label Feb 3, 2022

andrewvc reviewed Feb 4, 2022

View reviewed changes

blakerouse reviewed Feb 8, 2022

View reviewed changes

mergify bot assigned emilioalvap Feb 8, 2022

andrewvc mentioned this pull request Feb 10, 2022

[heartbeat] Stop using cap_net_raw for docker image #28685

Closed

emilioalvap added 4 commits February 24, 2022 14:26

Unpack beats inside agent container

f8809a5

Change ownership to elastic-agent user

e799d53

Update elastic-agent docker template

a006bff

Remove config setting

d732b36

emilioalvap force-pushed the heartbeat-agent-capabilities branch from 32348ba to d732b36 Compare February 24, 2022 14:42

emilioalvap commented Mar 9, 2022

View reviewed changes

emilioalvap closed this Mar 15, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Heartbeat] Unpack beats to enable capabilities inside container #30200

[Heartbeat] Unpack beats to enable capabilities inside container #30200

emilioalvap commented Feb 3, 2022 •

edited

Loading

mergify bot commented Feb 3, 2022

elasticmachine commented Feb 3, 2022 •

edited by jenkins-beats-ci bot

Loading

Build stats

Test stats 🧪

andrewvc Feb 4, 2022

blakerouse Feb 8, 2022

emilioalvap Feb 24, 2022

blakerouse left a comment

blakerouse Feb 8, 2022

blakerouse Feb 8, 2022

emilioalvap Feb 24, 2022

blakerouse Feb 8, 2022

emilioalvap Feb 24, 2022 •

edited

Loading

blakerouse Feb 8, 2022

emilioalvap Feb 8, 2022

andrewvc Feb 8, 2022

blakerouse Feb 9, 2022

blakerouse commented Mar 8, 2022

emilioalvap Feb 24, 2022

blakerouse Mar 9, 2022

emilioalvap Mar 9, 2022 •

edited

Loading

blakerouse Mar 9, 2022

emilioalvap Mar 9, 2022

		chown -R root:root {{ $beatHome }}/data/{{.BeatName}}-{{ commit_short }}/{{ .beats_install_path }}//.yml && \
		chmod 0644 {{ $beatHome }}/data/{{.BeatName}}-{{ commit_short }}/{{ .beats_install_path }}//.yml && \

[Heartbeat] Unpack beats to enable capabilities inside container #30200

[Heartbeat] Unpack beats to enable capabilities inside container #30200

Conversation

emilioalvap commented Feb 3, 2022 • edited Loading

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

mergify bot commented Feb 3, 2022

elasticmachine commented Feb 3, 2022 • edited by jenkins-beats-ci bot Loading

💚 Build Succeeded

Build stats

Test stats 🧪

💚 Flaky test report

🤖 GitHub comments

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blakerouse left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emilioalvap Feb 24, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blakerouse commented Mar 8, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emilioalvap Mar 9, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emilioalvap commented Feb 3, 2022 •

edited

Loading

elasticmachine commented Feb 3, 2022 •

edited by jenkins-beats-ci bot

Loading

emilioalvap Feb 24, 2022 •

edited

Loading

emilioalvap Mar 9, 2022 •

edited

Loading