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

feat: check cap privileges instead of Geteuid during starting the agent #242

Merged
merged 14 commits into from
Jan 3, 2025

Conversation

spencercjh
Copy link
Contributor

@spencercjh spencercjh commented Jan 2, 2025

@dosubot dosubot bot added the size:S This PR changes 10-29 lines, ignoring generated files. label Jan 2, 2025
Copy link

vercel bot commented Jan 2, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
kyanos ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jan 3, 2025 11:51am

@spencercjh
Copy link
Contributor Author

This project uses containerd/v1, so I'm not using v2 here either.

agent/agent.go Outdated Show resolved Hide resolved
@hengyoush
Copy link
Owner

hengyoush commented Jan 2, 2025

I think we can add a test script to test it: create a container and copy kyanos and a script in to the container, then execute the copied script which will start kyanos(will failed) and check the log. This is a failed case.

If do better we can create a containter with CAP_BPF then start kyanos in it, expect successfully.

@spencercjh
Copy link
Contributor Author

I think we can add a test script to test it: create a container and copy kyanos and a script in to the container, then execute the copied script which will start kyanos(will failed) and check the log.

If do better we can create a containter with CAP_BPF then start kyanos in it, expect successfully.

Indeed, we need a dockerfile to build a container environment that can run kyanos.

@hengyoush
Copy link
Owner

hengyoush commented Jan 2, 2025

Just run an alpine container is sufficient, use --cap-add and --cap-drop to control capabilities. Just look at testdata/run_k8s_test.sh, this script use kind but we can simple use alpine image.

@Issues-translate-bot
Copy link

Bot detected the issue body's language is not English, translate it automatically. 👯👭🏻🧑‍🤝‍🧑👫🧑🏿‍🤝‍🧑🏻👩🏾‍🤝‍👨🏿👬🏿


Just run a alpine container is sufficient.

@spencercjh spencercjh force-pushed the feat/cap-bpf-check branch 2 times, most recently from 85e5e16 to eba535a Compare January 2, 2025 11:34
@hengyoush
Copy link
Owner

Try add this cap: CAP_SYS_RESOURCE to container .

Until kernel version v5.11 this mechanism was used to track and limit the memory usage of BPF maps which count towards the locked memory limit, so you commonly would have to increase or disable this rlimit which requires an additional capability CAP_SYS_RESOURCE.
Refer: https://docs.ebpf.io/linux/concepts/resource-limit/

@hengyoush
Copy link
Owner

Or we can just run test container in privileged mode, i think it is enough.

@spencercjh spencercjh marked this pull request as ready for review January 3, 2025 11:50
@dosubot dosubot bot added size:L This PR changes 100-499 lines, ignoring generated files. and removed size:S This PR changes 10-29 lines, ignoring generated files. labels Jan 3, 2025
@spencercjh spencercjh changed the title feat: check CAP_BPF privilege instead of root during starting the agent feat: check cap privileges instead of Geteuid during starting the agent Jan 3, 2025
Copy link
Owner

@hengyoush hengyoush left a comment

Choose a reason for hiding this comment

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

LGTM.

@dosubot dosubot bot added the lgtm This PR has been approved by a maintainer label Jan 3, 2025
@hengyoush hengyoush merged commit 6d0b142 into hengyoush:main Jan 3, 2025
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request lgtm This PR has been approved by a maintainer size:L This PR changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve the root privielge check when kyanos start
3 participants