s/apparmor: include files in apparmor kernel features #15130
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #15130 +/- ##
==========================================
- Coverage 78.07% 78.06% -0.01%
==========================================
Files 1182 1185 +3
Lines 157743 158252 +509
==========================================
+ Hits 123154 123545 +391
- Misses 26943 27028 +85
- Partials 7646 7679 +33
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
|
Fri Feb 28 19:34:29 UTC 2025 Failures:Preparing:
Executing:
Restoring:
|
There was a problem hiding this comment.
LGTM
Looking at the code we could save some garbage by using a bytes.Buffer and passing it so that we edit the tail of the path. Separately I think we could simplify quite a bit of our "root dir is here" code with https://pkg.go.dev/io/fs#Sub - so to pass an fs.FS of appropriate subtype and use fs.Sub to peek into deeper layers.
| @@ -683,12 +681,12 @@ func probeKernelFeaturesInDirRecursively(dir string, prefix string) ([]string, e | |||
| } | |||
| features := make([]string, 0, len(dentries)) | |||
| for _, fi := range dentries { | |||
| featureName := fi.Name() | |||
There was a problem hiding this comment.
won't this make /sys/kernel/securtiy/apparmor/features/capability show up as a kernel feature now?
There was a problem hiding this comment.
Yes, as-is it would show up as a feature. The file itself won't be read, though. Is this something we want to avoid?
There was a problem hiding this comment.
I'm not sure if capability is indicative of any particular feature in AppArmor, I'm assuming it's not and as such it'd be better to skip it.
There was a problem hiding this comment.
It looks like the boolean files we care about should all be subdirectories of /sys/kernel/security/apparmor/features/policy/, so perhaps this change could be narrowed to only include files under that directory? I don't think there's harm in including all files, but we could be more selective.
More info on the tagging support checks here, for example: https://docs.google.com/document/d/1_WvEM9Qi2Je2Vwulzv5TzHwLJ8ld0W8gTNESZZd9VsY/edit?tab=t.0#heading=h.f728esb5to7y
There was a problem hiding this comment.
looking at https://elixir.bootlin.com/linux/v6.13.4/source/security/apparmor/apparmorfs.c#L2404 capability seems to be an actual 'feature'. It's actually quite confusing how we're approaching this e.g. there's just one network_v8 entry, even though network_v8/af_mask has multiple values inside, but we have 10+ entries for kernel:policy:permstable32:.*. Maybe we should have been more selective about the features we list and only include the ones we know/care about rather than adding all of them simply because entries are there.
There was a problem hiding this comment.
Reply from the kernel team about what it is/means was this:
it lists the capabilities that are supported in that kernel. capabilities are not encoded the same way that the other rules are (using the dfa). each capability is represented as a bit of a uint64_t
olivercalder
force-pushed
the
expand-apparmor-kernel-features-to-files
branch
from
354c448 to
15298ad
Compare
Previously, only directories under /sys/kernel/security/apparmor/features were included in the probed kernel features list. However, boolean files are sometimes used to indicate support for particular features as well. This commit extends the probed features to include the presence of files, as well as directories. Though we are looking in a sysfs, /sys/kernel/security/apparmor/features is entirely managed by apparmorfs, so it should not be a problem to include files there in the kernel features (and by extension in the system key). Signed-off-by: Oliver Calder <[email protected]>
olivercalder
force-pushed
the
expand-apparmor-kernel-features-to-files
branch
from
15298ad to
244ecd6
Compare
Previously, only directories under /sys/kernel/security/apparmor/features were included in the probed kernel features list. However, boolean files are sometimes used to indicate support for particular features as well.
This commit extends the probed features to include the presence of files, as well as directories.
Though we are looking in a sysfs, /sys/kernel/security/apparmor/features is entirely managed by apparmorfs, so it should not be a problem to include files there in the kernel features (and by extension in the system key).
This is required by #15089 so that the version support checks can use
apparmor.KernelFeaturesto check for the presence of the protocol version boolean files, indicating kernel support for particular versions.