pkg/apparmor: scratch documentation of apparmor - #6559
Conversation
We're currently growing support for apparmor 2.13+ and I was thinking about how we structure the code related to that. I'm worried of the growing complexity in interfaces/apparmor (and release/apparmor.go) and would like to move that code to pkg/apparmor where it can be referenced, as appropriate from original locations. In addition I'd like to clearly separate old and new apparmor, using the chance to document how it behaves in each version. This patch is by no means complete but I'd like to use it to start a discussion about moving the code out, gardening it a little bit and dropping it here. I chose the directory pkg/apparmor as we can expect similar treatment to seccomp code without adding a new top-level element for each package and the pkg/foo naming scheme seems to be common in the go ecosystem. Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
|
There is no general agreement about the pkg/ style, I just saw over the weekend people discussing exactly that, I find it a bit low-content as naming style, it would also make us question whether to move more stuff. |
|
The name This code could, in theory, live in |
|
I think sandbox/apparmor sandbox/seccomp would follow more our current organisational style (which I don't see pressure to change) |
Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
|
I've renamed the package to |
jdstrand
left a comment
jdstrand
left a comment
There was a problem hiding this comment.
I don't quite understand the purpose of this PR. A lot of the comments aren't quite right and have no bearing on snapd and there is quite a bit of added complexity and assumptions about the apparmor layout. IME, snapd (these days, now that it manages the snap-confine profiles itself) needs to know where the cache directory is for the distribution it is working with (and even this could be removed if snapd wanted to ship its own systemd unit for loading snap profiles from, say /var/lib/snapd/apparmor/cache), and whether or not to support the new-style forest or the old-style flat dir. How profiles are named, what they contain, etc shouldn't be codified in snapd. The main point of profileIsRemovableOnCoreSetup() in PR 6549 is not about the names of other profiles, but about the names of profiles that snapd maintains so codifying things elsewhere seems to add complexity.
That said, I appreciate that it might be useful to understand where distros put things and perhaps that is worth a larger comment (perhaps in dirs/dirs.go though).
| "path/filepath" | ||
| ) | ||
|
|
||
| // Legacy represents apparmor user-space version up until 1.12 |
| // /etc/apparmor.d/* (just files): source profiles. | ||
| // /etc/apparmor.d/tunables/*: definitions included by source profiles. | ||
| // /etc/apparmor.d/abstractions/*: definitions included by source profiles. | ||
| // /etc/apparmor.d/cache/*: binary profiles. |
There was a problem hiding this comment.
This isn't strictly true. A distro could've use /var/cache/apparmor regardless of if it was 2.13 or not. It just happened to coincide with Debian's 2.13 packaging that they cleaned up up and used a unified directory.
| // In addition some locations are only used by snapd: | ||
| // | ||
| // /var/lib/snapd/apparmor/profiles: source profiles (snapd specific) | ||
| // /var/cache/apparmor/*: binary profiles (snapd specific) |
There was a problem hiding this comment.
No-- this was used elsewhere (eg, click on Ubuntu Touch) and could've been used by anyone.
| // /var/lib/snapd/apparmor/profiles: source profiles (snapd specific) | ||
| // /var/cache/apparmor/*: binary profiles (snapd specific) | ||
| // | ||
| // Source profiles are typically stored as files in /etc/apparmor.d/. |
There was a problem hiding this comment.
Not really. This is the default and used by convention only. Debian/Ubuntu debs would place profiles here, and administrators would usually put profiles here, but applications are free to manage profiles as needed (eg, snapd and later docker use somewhere else; libvirt is a subdir of /etc/apparmor.d as are apache hats).
| // | ||
| // Source profiles are typically stored as files in /etc/apparmor.d/. | ||
| // The file name is the absolute path of the executable with forward slashes | ||
| // replaced with dots. For example the file /etc/apparmor.d/usr.bin.man |
There was a problem hiding this comment.
Again, this is convention and not enforced. No tools should codify this. It is perfectly legal to ship a profile 'foo' that confines /opt/bin/bar. There is no enforced correlation between profile name, the binary it confines and the filename.
| // | ||
| // Source profiles routinely include definitions from | ||
| // /etc/apparmor.d/abstractions as well as tunables that can be adjusted by | ||
| // local administrator, from /etc/apparmor.d/tunables. |
There was a problem hiding this comment.
Again, this is default behavior but entirely dependent on what the profiles use. Eg Ubuntu Touch would #include files from /usr and the parser supports specifying a different base directory with --base.
| // | ||
| // Once compiled, binary profiles are typically cached as files in | ||
| // /etc/apparmor.d/cache. The cache is based on mtime of the source profile and | ||
| // of the cached profile. The cache is unaware of the kernel feature set. This |
There was a problem hiding this comment.
That's not true. /etc/apparmor.d/cache/.features is present prior to 2.13 and was used as part of the parser's decision to invalidate the cache.
| // Apart from the aforementioned directories in /etc/apparmor.d/, at least on | ||
| // some distributions, there are more binary profiles present in | ||
| // /var/cache/apparmor and more source profiles present in | ||
| // /var/lib/snapd/apparmor/profiles. Profiles stored there don't follow the |
| @@ -0,0 +1,7 @@ | |||
| package apparmor | |||
|
|
|||
| // modern represents apparmor user-space version since 1.13 | |||
|
@jdstrand thank you for all the comments. I will adjust this for another pass. I think some explanation is in order. I was looking at the 2.13 PR and was somewhat uncomfortable with clustering of apparmor specific logic 2.12 vs 2.13 along with snapd logic. I though it would be nice to abstract apparmor differences behind a new module, taking the opportunity to document how 2.12 and 2.13 differ. |
|
Legacy and modern are not clear names, Pre2_13 and 21_3Plus or something. Where/how do we pick one or the other? Also is still a bit unclear to me there is enough actual difference to warrant a full new package, vs a helper.go file in interfaces/apparmor. |
|
I will return here tomorrow. |
|
I'm closing this, is still a bit unclear if it's a win, and I don't think we have overall bandwidth ATM to work/think through it. It can be reopened later. |
3 participants
We're currently growing support for apparmor 2.13+ and I was thinking
about how we structure the code related to that. I'm worried of the
growing complexity in interfaces/apparmor (and release/apparmor.go) and
would like to move that code to pkg/apparmor where it can be referenced,
as appropriate from original locations. In addition I'd like to clearly
separate old and new apparmor, using the chance to document how it
behaves in each version.
This patch is by no means complete but I'd like to use it to start a
discussion about moving the code out, gardening it a little bit and
dropping it here. I chose the directory pkg/apparmor as we can expect
similar treatment to seccomp code without adding a new top-level element
for each package and the pkg/foo naming scheme seems to be common in the
go ecosystem.
Signed-off-by: Zygmunt Krynicki zygmunt.krynicki@canonical.com