ROX-33034: Track xattr changes#790
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds end-to-end tracking of ChangesXattr tracking feature
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #790 +/- ##
==========================================
- Coverage 27.96% 27.34% -0.63%
==========================================
Files 21 21
Lines 2596 2655 +59
Branches 2596 2655 +59
==========================================
Hits 726 726
- Misses 1867 1926 +59
Partials 3 3 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@fact-ebpf/src/bpf/main.c`:
- Around line 434-443: The trace_inode_removexattr function has an unreachable
return statement. After the return handle_xattr(...) call on line 441, there is
a duplicate return 0; statement that serves as dead code. Remove the duplicate
return 0; statement that appears after the handle_xattr return, keeping only the
return handle_xattr(...) line to ensure the function properly returns the result
of the xattr handler call.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Enterprise
Run ID: 461c91e6-bfc7-42e7-97f6-99a4157926c8
📒 Files selected for processing (8)
fact-ebpf/src/bpf/events.hfact-ebpf/src/bpf/main.cfact-ebpf/src/bpf/types.hfact-ebpf/src/lib.rsfact/src/event/mod.rsfact/src/host_scanner.rsfact/src/metrics/kernel_metrics.rstests/test_inode_xattr.py
Molter73
left a comment
Molter73
left a comment
There was a problem hiding this comment.
As discussed offline, we should make the protobuf changes for supporting xattr as part of these changes, so I expect some changes incoming.
There are also 2 small comments on the eBPF code.
JoukoVirtanen
force-pushed
the
jv-ROX-33034-track-xattr-changes
branch
from
7e93fef to
dd9da9b
Compare
Molter73
left a comment
Molter73
left a comment
There was a problem hiding this comment.
Overall looks good. There's a small comment on the operation field which shouldn't be there, that kind of information should be encoded in the type system.
The integration tests need a bit of work.
| pub struct XattrFileData { | ||
| inner: BaseFileData, | ||
| xattr_name: String, | ||
| operation: XattrOperation, |
There was a problem hiding this comment.
You shouldn't need this field. This is used in FileData with the variants SetXattr and RemoveXattr which already encode the operation as part of the type. I left a comment on the protobuf changes that should help with this: stackrox/stackrox#21148 (comment)
| # In strict mode, fail when the event type matches but | ||
| # the content differs. Events of a different type are | ||
| # skipped since they may be system noise that the test | ||
| # cannot predict (e.g. SELinux xattr changes). | ||
| if 'event_type' not in diff: | ||
| raise ValueError(json.dumps(diff, indent=4)) |
There was a problem hiding this comment.
Does this mean that now any test can just have random noise from xattr changes? Why is that? Are we certain it is random or is it that there already were xattr changes we were not seeing when the original tests were written and are now being emitted? Since the tests limit themselves to monitoring a single empty directory I find it very hard to believe events emitted are random.
In any case, we need to either:
- Check what the new events in tests are, whether they truly are random and, if they are not, add any missing events to the sequence of events on the existing tests.
- If the events truly are random, double check if we are not emitting events that we shouldn't be emitting and figure out a way to disable xattr events for tests to reduce noise.
As is, this check will probably lead to some hard to understand errors due to events being skipped when they shouldn't. strict mode means STRICT MODE, there should be no room for speculation when using this.
| Event( | ||
| process=process, | ||
| event_type=EventType.XATTR, | ||
| file='', | ||
| host_path=test_file, | ||
| xattr_name='user.fact_remove', | ||
| ), |
There was a problem hiding this comment.
The test performs 2 xattr operations, why is there just one here?
| os.setxattr(test_file, 'user.attr1', b'value1') | ||
| os.setxattr(test_file, 'user.attr2', b'value2') | ||
| os.setxattr(test_file, 'user.attr3', b'value3') |
There was a problem hiding this comment.
Add os.removexattr operations here as well.
| ) | ||
|
|
||
|
|
||
| def test_setxattr_ignored( |
There was a problem hiding this comment.
Extend this test with removexattr.
| ) | ||
|
|
||
|
|
||
| def test_setxattr_new_file( |
There was a problem hiding this comment.
Extend this test with removexattr
| pytest.param(b'xattr\xff\xfe.txt', id='InvalidUTF8'), | ||
| ], | ||
| ) | ||
| def test_setxattr_utf8_filenames( |
There was a problem hiding this comment.
Extend this test with removexattr.
There was a problem hiding this comment.
We probably want to add a test similar to test_setxattr_utf8_filenames, but setting xattr names with utf8 strings.
3 participants
Description
Adds tracking for setting and removing xattr. For now this only adds metrics for xattr events. In the future gRPC messages will be sent to sensor when these event occur. Sending gRPC messages to sensor will require changes to the stackrox/stackrox protobuf definitions and will be done later. The added integration tests rely on metrics. Once gRPC messages are sent the integration tests will not check the metrics and will instead check the gRPC messages.
A change was made to relax the success criteria of
_wait_eventsused by the integration tests. Events of unexpected type are now ignored. This change was needed, because xattr changes are made by SELinux in many of the tests.Protobuf changes are made at stackrox/stackrox#21148
Checklist
Automated testing
If any of these don't apply, please comment below.
Testing Performed
CI is sufficient
Summary by CodeRabbit
Release Notes
New Features
Tests