linux capabilities: normalization and auditbeat process support #37453
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
botelastic
bot
added
the
needs_team
|
This pull request does not have a backport label.
To fixup this pull request, you need to add the backport labels for the needed
|
❕ Build Aborted
Expand to view the summary
Build stats
Test stats 🧪
Test errors
Expand to view the tests failures> Show only the first 10 test failures
|
❕ Build Aborted
Expand to view the summary
Build stats
Test stats 🧪
Test errors
Expand to view the tests failures> Show only the first 10 test failures
|
❕ Build Aborted
Expand to view the summary
Build stats
Test stats 🧪
Test errors
Expand to view the tests failures> Show only the first 10 test failures
|
|
/test |
❕ Build Aborted
Expand to view the summary
Build stats
Test stats 🧪
Test errors
Expand to view the tests failures> Show only the first 10 test failures
|
❕ Build Aborted
Expand to view the summary
Build stats
Test stats 🧪
Test errors
Expand to view the tests failures> Show only the first 10 test failures
|
❕ Build Aborted
Expand to view the summary
Build stats
🤖 GitHub commentsExpand to view the GitHub comments
To re-run your PR in the CI, just comment with:
|
💚 Build Succeeded
Expand to view the summary
Build stats
❕ Flaky test reportNo test was executed to be analysed. 🤖 GitHub commentsExpand to view the GitHub comments
To re-run your PR in the CI, just comment with:
|
This draft implements `process.thread.capabilities.{effective,permitted}` to
auditbeat/system/process and normalizes the other uses of linux capabilities
across beats (only two other cases).
Introduces libbeat/common/capabilities that returns strings in the format ECS
expects. It also condenses "all capabilities" into CAP_ALL as suggested.
I've tested metricbeat, auditbeat and filebear+journald.
These fields are only present in ECS 8.10, it seems beats is still tracking ECS 8.0, is this the right way to do until we track 8.10+? It's enough for `mage integTest` to pass, otherwise it will complain about undocumented new fields
haesbaert
added
the
Team:Security-Linux Platform
botelastic
bot
removed
the
needs_team
💚 Build Succeeded
Expand to view the summary
Build stats
Test stats 🧪
💚 Flaky test reportTests succeeded. 🤖 GitHub commentsExpand to view the GitHub comments
To re-run your PR in the CI, just comment with:
|
❕ Build Aborted
Expand to view the summary
Build stats
🤖 GitHub commentsExpand to view the GitHub comments
To re-run your PR in the CI, just comment with:
|
❕ Build Aborted
Expand to view the summary
Build stats
🤖 GitHub commentsExpand to view the GitHub comments
To re-run your PR in the CI, just comment with:
|
💚 Build Succeeded
Expand to view the summary
Build stats
Test stats 🧪
💚 Flaky test reportTests succeeded. 🤖 GitHub commentsExpand to view the GitHub comments
To re-run your PR in the CI, just comment with:
|
leehinman
reviewed
Dec 21, 2023
| assert.Equal(t, len(sl), 0) | ||
|
|
||
| // assumes non root has no capabilities | ||
| if os.Geteuid() != 0 { |
There was a problem hiding this comment.
I don't think it must be true that euid != 0 has no capabilities. If that were to be not true in CI, that could lead to an interesting debugging scenario. Can this be reworked or mock out the os.Geteuid() call?
There was a problem hiding this comment.
Indeed, you can still have capabilities as a non-root user, I've added the comment to reflect that, I guess it's just rare enough that we could live with it.
I can't think of a good way to mock it, one way would be to drop all capabilities that are "Effective & Permitted", then test Effective for empty and restore the old set, but this is too impure and doesn't sound very appealing to me.
I'd either leave it and improve the comment or just remove it all together, let me know what you think.
💚 Build Succeeded
Expand to view the summary
Build stats
Test stats 🧪
💚 Flaky test reportTests succeeded. 🤖 GitHub commentsExpand to view the GitHub comments
To re-run your PR in the CI, just comment with:
|
It only errors out on in valid parameters, still makes the lint happier
💚 Build Succeeded
Expand to view the summary
Build stats
❕ Flaky test reportNo test was executed to be analysed. 🤖 GitHub commentsExpand to view the GitHub comments
To re-run your PR in the CI, just comment with:
|
|
This pull request is now in conflicts. Could you fix it? 🙏 |
|
@haesbaert is it ready for another review? |
Yes, nothing considerable changed, I've pinged @andrewkroh to get his input on the CAP_ALL semantics. I've also spoken to Dan about just removing CAP_ALL in order to make this PR go forward and then bring it in a subsequent PR. |
💚 Build Succeeded
Expand to view the summary
Build stats
❕ Flaky test reportNo test was executed to be analysed. 🤖 GitHub commentsExpand to view the GitHub comments
To re-run your PR in the CI, just comment with:
|
mmat11
reviewed
Jan 29, 2024
|
I have removed the CAP_ALL optimization the interesting of expediting this PR, I think it makes more sense to get this in on an easy to review PR and then discuss CAP_ALL later, as I also have the process processor implementation to get in after this.
|
Co-authored-by: Mattia Meleleo <mattia.meleleo@elastic.co>
💚 Build Succeeded
Expand to view the summary
Build stats
❕ Flaky test reportNo test was executed to be analysed. 🤖 GitHub commentsExpand to view the GitHub comments
To re-run your PR in the CI, just comment with:
|
efd6
approved these changes
Jan 31, 2024
leehinman
approved these changes
Jan 31, 2024
|
This pull request does not have a backport label.
To fixup this pull request, you need to add the backport labels for the needed
|
6 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Proposed commit message
Add process capabilities to auditbeat/system and normalize the existing capabilities uses in beats.
The main goal is to add
process.thread.capabilities.{permitted, effective}only, but since there are a couple of other uses of capabilities in beats already, it makes sense to standarize them into a common way to generate the required ECS fields.I noticed that on root processes, where all capabilities are set, we end up generating a ton of output, so I proposed to condense them into CAP_ALL here.
This change implements said CAP_ALL and also makes sure the other beats end up outputting CAP_ALL as well by usingCAP_ALL removed from this PR, to be discussed in the future.libbeat/common/capabilities/capabilities_*.The original idea was to rely on go-sysinfo that parses
/proc, and while that works fine, we need more interfaces: like being able to get capabilities from an uint64 that comes from a kprobe for example, so instead I'm using the already importedcapmodule that is shipped with libcap(3) that retrieves process capabilities via capget(2), it's also maintained by libcap people so hopefully we don't have to care about maintaining our own capability table in the future. Additionally it also handles old kernels that have a slightly different ABI.capdoes runtime discovery of the maximum capability set of the host (cap.MaxBits()) and that is the criteria to issuing aCAP_ALL, note that if the capability being converted contains more bits (say we're using an oldcapmodule in a newer kernel), the capability will still be exported but as "CAP_42, 43...", and CAP_ALL will be suppressed, I believe this is way less confusing than CAP_ALL+CAP_42.Beats seem to be tracking ECS 8.0.0-dev, these new fields appeared in 8.10 so I've added them to the auditbeat fields in order to pass the integration tests, otherwise it complains about undocumented fields.
I've added capget(2) to all beats syscomp policy, this is because
cap.GetProc()is written with the assumption that you can never get EPERM for the current process.I've also raised the point that there is a canonical text representation for capabilities https://linux.die.net/man/3/cap_to_text
And I wonder if it's too late and/or makes any sense to adopt something like that in ECS.
Checklist
My code follows the style guidelines of this projectCHANGELOG.next.asciidocorCHANGELOG-developer.next.asciidoc.Author's Checklist
I've added tests and converted the journald ones to use the same backing, and verified it still works. I've also tested teh journald output on an elastic deployment to be sure.
There is an obvious TOCTOU in fetching the capabilities since when we attempt to retrieve them the process might be gone (ESRCH), in this case we set
process.Errorif not already set, I wonder if it makes sense to not error out in this case and just mark CAP_UNKNOWN or whatever, I'm not familiar enough with beats to judge this.How to test this PR locally
auditbeat configuration:
Related issues
Screenshots