ROX-30296: track POSIX ACL changes via inode_set_acl LSM hook#878
Merged
Conversation
📝 WalkthroughWalkthroughAdds inode_set_acl ACL tracking across eBPF capture, userspace decoding, metrics, kernel capability checks, and Python test coverage. The PR also updates the changelog and the vendored StackRox submodule pin. ChangesACL Event Tracking
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Kernel
participant check_inode_set_acl
participant trace_inode_set_acl
participant submit_acl_event
participant Ringbuffer
participant Userspace
Kernel->>check_inode_set_acl: inode_set_acl LSM hook
check_inode_set_acl-->>Kernel: return 0
Kernel->>trace_inode_set_acl: inode_set_acl tracepoint
trace_inode_set_acl->>trace_inode_set_acl: inode_is_monitored check
trace_inode_set_acl->>submit_acl_event: submit_acl_event(args, acl_name, kacl)
submit_acl_event->>Ringbuffer: reserve_event + submit FILE_ACTIVITY_ACL_SET
Ringbuffer->>Userspace: FileData::new decodes ACL payload
Userspace->>Userspace: FileData::AclSet -> FileAclChange protobuf
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #878 +/- ##
==========================================
+ Coverage 32.23% 32.65% +0.42%
==========================================
Files 21 21
Lines 2736 2915 +179
Branches 2736 2915 +179
==========================================
+ Hits 882 952 +70
- Misses 1851 1960 +109
Partials 3 3 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Stringy
force-pushed
the
giles/ROX-30296-track-acl
branch
4 times, most recently
from
46d84a9 to
11683ba
Compare
Stringy
force-pushed
the
giles/ROX-30296-track-acl
branch
from
67a3b12 to
be7fb22
Compare
9 tasks
Stringy
force-pushed
the
giles/ROX-30296-track-acl
branch
from
eae45f0 to
fc73ed7
Compare
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
fact/src/event/mod.rs (1)
538-555: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAdd the missing
Chownequality arm.While extending this manual matcher,
FileData::Chownstill falls through to_ => false, so two identical ownership-change events compare unequal.Proposed fix
(FileData::Unlink(this), FileData::Unlink(other)) => this == other, (FileData::Chmod(this), FileData::Chmod(other)) => this == other, + (FileData::Chown(this), FileData::Chown(other)) => this == other, (FileData::Rename(this), FileData::Rename(other)) => this == other, (FileData::SetXattr(this), FileData::SetXattr(other)) => this == other,🤖 Prompt for 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. In `@fact/src/event/mod.rs` around lines 538 - 555, The manual PartialEq implementation for FileData is missing the FileData::Chown arm, so identical ownership-change events currently fall through to false. Update the eq method in FileData’s PartialEq match to add a Chown(this), Chown(other) branch that compares the contained values the same way as the other variants, keeping the fallback _ => false only for truly different variants.
🧹 Nitpick comments (1)
fact/src/bpf/checks.rs (1)
34-42: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider logging the underlying error before treating any load failure as "unsupported."
probe_hookreturnsfalsefor missing programs, wrong program type, and load failures alike, discarding the error fromprog.load(...). This is fine for genuinely unsupported kernels, but it also silently masks a real bug (e.g. a verifier failure inchecks.c) as "capability not supported," making such regressions hard to diagnose.♻️ Suggested diagnostic logging
fn probe_hook(obj: &mut aya::Ebpf, prog_name: &str, hook: &str, btf: &Btf) -> bool { let Some(prog) = obj.program_mut(prog_name) else { return false; }; let Ok(prog): Result<&mut Lsm, _> = prog.try_into() else { return false; }; - prog.load(hook, btf).is_ok() + match prog.load(hook, btf) { + Ok(()) => true, + Err(e) => { + debug!("Failed to probe {hook} support via {prog_name}: {e:#?}"); + false + } + } }🤖 Prompt for 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. In `@fact/src/bpf/checks.rs` around lines 34 - 42, `probe_hook` is swallowing real load errors by returning false for every failure case. Update the function to log the underlying error from `prog.load(hook, btf)` before mapping it to unsupported, while keeping the existing false behavior for missing programs and wrong program types. Use the `probe_hook` function and the `Lsm` load path to locate the change, and make sure the log clearly distinguishes load failures from genuinely unsupported kernels.Source: Path instructions
🤖 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/events.h`:
- Around line 193-199: The ACL copy loop in events.h is always assigning
entry.e_uid.val to args->event->acl.entries[i].e_id, but posix_acl_entry.e_uid
is only valid for ACL_USER and ACL_GROUP. Update the logic in the ACL entry
population path to branch on entry.e_tag and set e_id from e_uid.val only for
those tags; for ACL_USER_OBJ, ACL_GROUP_OBJ, ACL_MASK, and ACL_OTHER, assign
ACL_UNDEFINED_ID instead. Keep the change localized to the ACL event-building
loop so the rest of the field copying in args->event->acl.entries remains
unchanged.
---
Outside diff comments:
In `@fact/src/event/mod.rs`:
- Around line 538-555: The manual PartialEq implementation for FileData is
missing the FileData::Chown arm, so identical ownership-change events currently
fall through to false. Update the eq method in FileData’s PartialEq match to add
a Chown(this), Chown(other) branch that compares the contained values the same
way as the other variants, keeping the fallback _ => false only for truly
different variants.
---
Nitpick comments:
In `@fact/src/bpf/checks.rs`:
- Around line 34-42: `probe_hook` is swallowing real load errors by returning
false for every failure case. Update the function to log the underlying error
from `prog.load(hook, btf)` before mapping it to unsupported, while keeping the
existing false behavior for missing programs and wrong program types. Use the
`probe_hook` function and the `Lsm` load path to locate the change, and make
sure the log clearly distinguishes load failures from genuinely unsupported
kernels.
🪄 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: 5b916de2-f11e-48d7-b345-c3ed9b660223
📒 Files selected for processing (16)
CHANGELOG.mdfact-ebpf/src/bpf/checks.cfact-ebpf/src/bpf/events.hfact-ebpf/src/bpf/main.cfact-ebpf/src/bpf/types.hfact-ebpf/src/lib.rsfact/src/bpf/checks.rsfact/src/bpf/mod.rsfact/src/event/mod.rsfact/src/metrics/kernel_metrics.rstests/event.pytests/server.pytests/test_acl.pytests/test_xattr.pytests/utils.pythird_party/stackrox
Molter73
reviewed
Jul 2, 2026
Molter73
left a comment
Molter73
left a comment
Collaborator
There was a problem hiding this comment.
Overall looks good! Thanks for tackling this one!
There's a couple comments, mostly questions so I expect we should be able to merge this soon.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
fact-ebpf/src/bpf/types.h (1)
82-103: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winConsider fixed-width types for wire-format enum fields.
acl_type_tandacl_tag_tare plain C enums with implementation-defined underlying size, but they're used directly as fields inacl_entry_t/theaclunion member (Line 100, Line 149) that gets copied byte-for-byte into a ring-buffer event later decoded by a separate Rust binary. If the enum's compiled width ever diverges from what the Rust decoder assumes, the ACL payload would be silently misparsed. Using an explicit fixed-width type (e.g.,unsigned char) fore_tag/acl_typeat the struct-field level (while keeping the enum for symbolic constants elsewhere) would make the cross-language layout contract explicit and robust.♻️ Proposed fix
struct acl_entry_t { - acl_tag_t e_tag; + unsigned char e_tag; // acl_tag_t value unsigned short e_perm; unsigned int e_id; };🤖 Prompt for 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. In `@fact-ebpf/src/bpf/types.h` around lines 82 - 103, `acl_type_t` and `acl_tag_t` are being used as wire-format field types in `acl_entry_t` and the `acl` union, but plain enums have implementation-defined size. Update the struct fields that are serialized across the ring buffer to use an explicit fixed-width type such as `unsigned char`, while keeping `acl_type_t` and `acl_tag_t` only as symbolic enums for constants. Make sure the `acl_entry_t` definition and the `acl` union member stay layout-compatible with the Rust decoder.
🤖 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.
Nitpick comments:
In `@fact-ebpf/src/bpf/types.h`:
- Around line 82-103: `acl_type_t` and `acl_tag_t` are being used as wire-format
field types in `acl_entry_t` and the `acl` union, but plain enums have
implementation-defined size. Update the struct fields that are serialized across
the ring buffer to use an explicit fixed-width type such as `unsigned char`,
while keeping `acl_type_t` and `acl_tag_t` only as symbolic enums for constants.
Make sure the `acl_entry_t` definition and the `acl` union member stay
layout-compatible with the Rust decoder.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Enterprise
Run ID: ba942558-b1cd-4991-8566-24551f09d46b
📒 Files selected for processing (6)
fact-ebpf/src/bpf/events.hfact-ebpf/src/bpf/types.hfact/src/bpf/checks.rsfact/src/bpf/mod.rsfact/src/event/mod.rstests/test_acl.py
🚧 Files skipped from review as they are similar to previous changes (5)
- fact/src/bpf/checks.rs
- fact-ebpf/src/bpf/events.h
- tests/test_acl.py
- fact/src/bpf/mod.rs
- fact/src/event/mod.rs
Molter73
approved these changes
Jul 6, 2026
Molter73
left a comment
Molter73
left a comment
Collaborator
There was a problem hiding this comment.
Just a couple minor nits, but overall LGTM!
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
Description
Adds ACL tracking via inode_set_acl hook. Due to lack of path info in this hook, it is only possible to monitor inode-tracked files (i.e. those on the host file system) this is similar to how xattrs are tracked.
An extension to the BPF checks.c file has been added, because this hook does not exist on older kernels (where ACLs were applied via xattrs, so we still have coverage on those old kernels, just through a different mechanism.)
Tests factor in this optional nature of the hook, and will only run when we know the kernel supports it.
Relies on stackrox/stackrox#21357
Checklist
Automated testing
If any of these don't apply, please comment below.
Testing Performed
Tested locally, with setfacl, and the integration tests.
Summary by CodeRabbit