[PATCH] apparmor: Add support for local profile customizations
Apparmor profiles in /etc/apparmor.d/ are config files that can and should be replaced on package upgrade, which introduces the potential to overwrite any local changes. Apparmor supports local profile customizations via /etc/apparmor.d/local/<service> [1]. This change makes the support explicit by adding libvirtd, virtqemud, and virtxend profile customization stubs to /etc/apparmor.d/local/. The stubs are conditionally included by the corresponding main profiles. [1] https://ubuntu.com/server/docs/security-apparmor See "Profile customization" section Signed-off-by: Jim Fehlig <jfehlig@suse.com> --- This patch was inspired by an internal bug report. The SUSE libvirt package has marked /etc/apparmor.d/<some-libvirt-service> profiles as 'config(noreplace)' for as long as I can remember. On rare occasions a profile receives a change that is required to avoid regression. And on rarer occasions a user might have made local customizations to the profile. With 'noreplace', the trap is set for the user to experience the regression. Unless other apparmor users convince me otherwise, I'm planning to make this change in the SUSE package, along with changing the main /etc/apparmor.d/ profiles to 'config' and using 'config(noreplace)' for the local customizations only. Note: I'm fine keeping this as a downstream-only patch if upstream isn't interested in the clutter. src/security/apparmor/meson.build | 12 +++++++----- src/security/apparmor/usr.sbin.libvirtd.in | 3 +++ src/security/apparmor/usr.sbin.libvirtd.local | 1 + src/security/apparmor/usr.sbin.virtqemud.in | 3 +++ src/security/apparmor/usr.sbin.virtqemud.local | 1 + src/security/apparmor/usr.sbin.virtxend.in | 3 +++ src/security/apparmor/usr.sbin.virtxend.local | 1 + 7 files changed, 19 insertions(+), 5 deletions(-) diff --git a/src/security/apparmor/meson.build b/src/security/apparmor/meson.build index 58b4024b85..02a6d098ad 100644 --- a/src/security/apparmor/meson.build +++ b/src/security/apparmor/meson.build @@ -34,8 +34,10 @@ install_data( install_dir: apparmor_dir / 'libvirt', ) -install_data( - 'usr.lib.libvirt.virt-aa-helper.local', - install_dir: apparmor_dir / 'local', - rename: 'usr.lib.libvirt.virt-aa-helper', -) +foreach name : apparmor_gen_profiles + install_data( + '@0@.local'.format(name), + install_dir: apparmor_dir / 'local', + rename: name, + ) +endforeach diff --git a/src/security/apparmor/usr.sbin.libvirtd.in b/src/security/apparmor/usr.sbin.libvirtd.in index edb8dd8e26..41bdef53ec 100644 --- a/src/security/apparmor/usr.sbin.libvirtd.in +++ b/src/security/apparmor/usr.sbin.libvirtd.in @@ -139,4 +139,7 @@ profile libvirtd @sbindir@/libvirtd flags=(attach_disconnected) { /usr/{lib,lib64,lib/qemu,libexec}/qemu-bridge-helper rmix, } + + # Site-specific additions and overrides. See local/README for details. + include if exists <local/usr.sbin.libvirtd> } diff --git a/src/security/apparmor/usr.sbin.libvirtd.local b/src/security/apparmor/usr.sbin.libvirtd.local new file mode 100644 index 0000000000..3716400022 --- /dev/null +++ b/src/security/apparmor/usr.sbin.libvirtd.local @@ -0,0 +1 @@ +# Site-specific additions and overrides for 'usr.sbin.libvirtd' diff --git a/src/security/apparmor/usr.sbin.virtqemud.in b/src/security/apparmor/usr.sbin.virtqemud.in index f269c60809..3ebdbf2a8f 100644 --- a/src/security/apparmor/usr.sbin.virtqemud.in +++ b/src/security/apparmor/usr.sbin.virtqemud.in @@ -132,4 +132,7 @@ profile virtqemud @sbindir@/virtqemud flags=(attach_disconnected) { /usr/{lib,lib64,lib/qemu,libexec}/qemu-bridge-helper rmix, } + + # Site-specific additions and overrides. See local/README for details. + include if exists <local/usr.sbin.virtqemud> } diff --git a/src/security/apparmor/usr.sbin.virtqemud.local b/src/security/apparmor/usr.sbin.virtqemud.local new file mode 100644 index 0000000000..2ac68bb069 --- /dev/null +++ b/src/security/apparmor/usr.sbin.virtqemud.local @@ -0,0 +1 @@ +# Site-specific additions and overrides for 'usr.sbin.virtqemud' diff --git a/src/security/apparmor/usr.sbin.virtxend.in b/src/security/apparmor/usr.sbin.virtxend.in index 72e0d801e5..719766a0c1 100644 --- a/src/security/apparmor/usr.sbin.virtxend.in +++ b/src/security/apparmor/usr.sbin.virtxend.in @@ -52,4 +52,7 @@ profile virtxend @sbindir@/virtxend flags=(attach_disconnected) { @libexecdir@/libvirt_iohelper ix, /etc/libvirt/hooks/** rmix, /etc/xen/scripts/** rmix, + + # Site-specific additions and overrides. See local/README for details. + include if exists <local/usr.sbin.virtxend> } diff --git a/src/security/apparmor/usr.sbin.virtxend.local b/src/security/apparmor/usr.sbin.virtxend.local new file mode 100644 index 0000000000..2ade86d4df --- /dev/null +++ b/src/security/apparmor/usr.sbin.virtxend.local @@ -0,0 +1 @@ +# Site-specific additions and overrides for 'usr.sbin.virtxend' -- 2.40.1
On 6/7/23 00:06, Jim Fehlig wrote:
Apparmor profiles in /etc/apparmor.d/ are config files that can and should be replaced on package upgrade, which introduces the potential to overwrite any local changes. Apparmor supports local profile customizations via /etc/apparmor.d/local/<service> [1].
This change makes the support explicit by adding libvirtd, virtqemud, and virtxend profile customization stubs to /etc/apparmor.d/local/. The stubs are conditionally included by the corresponding main profiles.
[1] https://ubuntu.com/server/docs/security-apparmor See "Profile customization" section
Signed-off-by: Jim Fehlig <jfehlig@suse.com> ---
This patch was inspired by an internal bug report. The SUSE libvirt package has marked /etc/apparmor.d/<some-libvirt-service> profiles as 'config(noreplace)' for as long as I can remember. On rare occasions a profile receives a change that is required to avoid regression. And on rarer occasions a user might have made local customizations to the profile. With 'noreplace', the trap is set for the user to experience the regression.
Unless other apparmor users convince me otherwise, I'm planning to make this change in the SUSE package, along with changing the main /etc/apparmor.d/ profiles to 'config' and using 'config(noreplace)' for the local customizations only.
Note: I'm fine keeping this as a downstream-only patch if upstream isn't interested in the clutter.
src/security/apparmor/meson.build | 12 +++++++----- src/security/apparmor/usr.sbin.libvirtd.in | 3 +++ src/security/apparmor/usr.sbin.libvirtd.local | 1 + src/security/apparmor/usr.sbin.virtqemud.in | 3 +++ src/security/apparmor/usr.sbin.virtqemud.local | 1 + src/security/apparmor/usr.sbin.virtxend.in | 3 +++ src/security/apparmor/usr.sbin.virtxend.local | 1 + 7 files changed, 19 insertions(+), 5 deletions(-)
Reviewed-by: Michal Privoznik <mprivozn@redhat.com> Michal
On Tue, Jun 06, 2023 at 04:06:12PM -0600, Jim Fehlig wrote:
Apparmor profiles in /etc/apparmor.d/ are config files that can and should be replaced on package upgrade, which introduces the potential to overwrite any local changes. Apparmor supports local profile customizations via /etc/apparmor.d/local/<service> [1].
This change makes the support explicit by adding libvirtd, virtqemud, and virtxend profile customization stubs to /etc/apparmor.d/local/. The stubs are conditionally included by the corresponding main profiles.
[1] https://ubuntu.com/server/docs/security-apparmor See "Profile customization" section
Signed-off-by: Jim Fehlig <jfehlig@suse.com> ---
This patch was inspired by an internal bug report. The SUSE libvirt package has marked /etc/apparmor.d/<some-libvirt-service> profiles as 'config(noreplace)' for as long as I can remember. On rare occasions a profile receives a change that is required to avoid regression. And on rarer occasions a user might have made local customizations to the profile. With 'noreplace', the trap is set for the user to experience the regression.
Unless other apparmor users convince me otherwise, I'm planning to make this change in the SUSE package, along with changing the main /etc/apparmor.d/ profiles to 'config' and using 'config(noreplace)' for the local customizations only.
Note: I'm fine keeping this as a downstream-only patch if upstream isn't interested in the clutter.
I think this is a good change. Note that the Debian package has included this patch[1] for many years, and while it partially overlaps with what you've added here, I see that local overrides for abstractions are missing. Is there a specific reason why you skipped them? Or should we add those too? [1] https://salsa.debian.org/libvirt-team/libvirt/-/blob/debian/latest/debian/pa... -- Andrea Bolognani / Red Hat / Virtualization
On 6/8/23 08:11, Andrea Bolognani wrote:
On Tue, Jun 06, 2023 at 04:06:12PM -0600, Jim Fehlig wrote:
Apparmor profiles in /etc/apparmor.d/ are config files that can and should be replaced on package upgrade, which introduces the potential to overwrite any local changes. Apparmor supports local profile customizations via /etc/apparmor.d/local/<service> [1].
This change makes the support explicit by adding libvirtd, virtqemud, and virtxend profile customization stubs to /etc/apparmor.d/local/. The stubs are conditionally included by the corresponding main profiles.
[1] https://ubuntu.com/server/docs/security-apparmor See "Profile customization" section
Signed-off-by: Jim Fehlig <jfehlig@suse.com> ---
This patch was inspired by an internal bug report. The SUSE libvirt package has marked /etc/apparmor.d/<some-libvirt-service> profiles as 'config(noreplace)' for as long as I can remember. On rare occasions a profile receives a change that is required to avoid regression. And on rarer occasions a user might have made local customizations to the profile. With 'noreplace', the trap is set for the user to experience the regression.
Unless other apparmor users convince me otherwise, I'm planning to make this change in the SUSE package, along with changing the main /etc/apparmor.d/ profiles to 'config' and using 'config(noreplace)' for the local customizations only.
Note: I'm fine keeping this as a downstream-only patch if upstream isn't interested in the clutter.
I think this is a good change.
Note that the Debian package has included this patch[1] for many years, and while it partially overlaps with what you've added here, I see that local overrides for abstractions are missing.
Is there a specific reason why you skipped them? Or should we add those too?
I assumed users would make VM customizations in the per-VM profiles. And I suppose overrides of abstractions seems a little odd to me, but that's subjective :-). I'm fine adding it if there's agreement. Regards, Jim
On Thu, Jun 08, 2023 at 10:37:43AM -0600, Jim Fehlig wrote:
On 6/8/23 08:11, Andrea Bolognani wrote:
Note that the Debian package has included this patch[1] for many years, and while it partially overlaps with what you've added here, I see that local overrides for abstractions are missing.
Is there a specific reason why you skipped them? Or should we add those too?
I assumed users would make VM customizations in the per-VM profiles. And I suppose overrides of abstractions seems a little odd to me, but that's subjective :-). I'm fine adding it if there's agreement.
The per-VM profile is generated at runtime based on the template, no? AFAIK there is no way for the admin to inject changes that affect a single VM, but I could be wrong about this. Anyway, there might be some changes that are local only but apply to all VMs, and allowing overrides to the abstractions would cater to that use case, so it makes sense to me to implement those as well. Do you mind cooking up a patch so that we can have the whole sha-bang included in the upcoming release? Thanks in advance! -- Andrea Bolognani / Red Hat / Virtualization
On 6/22/23 08:50, Andrea Bolognani wrote:
On Thu, Jun 08, 2023 at 10:37:43AM -0600, Jim Fehlig wrote:
On 6/8/23 08:11, Andrea Bolognani wrote:
Note that the Debian package has included this patch[1] for many years, and while it partially overlaps with what you've added here, I see that local overrides for abstractions are missing.
Is there a specific reason why you skipped them? Or should we add those too?
I assumed users would make VM customizations in the per-VM profiles. And I suppose overrides of abstractions seems a little odd to me, but that's subjective :-). I'm fine adding it if there's agreement.
The per-VM profile is generated at runtime based on the template, no? AFAIK there is no way for the admin to inject changes that affect a single VM, but I could be wrong about this.
The per-VM profile is only generated once, right? So in theory admins could amend existing per-VM profiles with custom config.
Anyway, there might be some changes that are local only but apply to all VMs, and allowing overrides to the abstractions would cater to that use case, so it makes sense to me to implement those as well.
Do you mind cooking up a patch so that we can have the whole sha-bang included in the upcoming release? Thanks in advance!
I should have time to do that today. Regards, Jim
On 6/22/23 11:08, Jim Fehlig wrote:
On 6/22/23 08:50, Andrea Bolognani wrote:
On Thu, Jun 08, 2023 at 10:37:43AM -0600, Jim Fehlig wrote:
On 6/8/23 08:11, Andrea Bolognani wrote:
Note that the Debian package has included this patch[1] for many years, and while it partially overlaps with what you've added here, I see that local overrides for abstractions are missing.
Is there a specific reason why you skipped them? Or should we add those too?
I assumed users would make VM customizations in the per-VM profiles. And I suppose overrides of abstractions seems a little odd to me, but that's subjective :-). I'm fine adding it if there's agreement.
The per-VM profile is generated at runtime based on the template, no? AFAIK there is no way for the admin to inject changes that affect a single VM, but I could be wrong about this.
The per-VM profile is only generated once, right? So in theory admins could amend existing per-VM profiles with custom config.
Anyway, there might be some changes that are local only but apply to all VMs, and allowing overrides to the abstractions would cater to that use case, so it makes sense to me to implement those as well.
Do you mind cooking up a patch so that we can have the whole sha-bang included in the upcoming release? Thanks in advance!
I should have time to do that today.
While working on this I noticed there is no /etc/apparmor.d/local/abstractions directory on SUSE-based distros. A lot of packages put files in /etc/apparmor.d/local, but I couldn't find any adding files to /etc/apparmor.d/local/abstractions. Nor could I find any apparmor documentation regarding the use of that directory. Do you know if it's common practice? Or is that Debian patch the only prior art? I can continue working on the patch, but I'm not sure I want it downstream and will likely revert it anyway. Regards, Jim
On Thu, Jun 22, 2023 at 03:03:56PM -0600, Jim Fehlig wrote:
On 6/22/23 11:08, Jim Fehlig wrote:
On 6/22/23 08:50, Andrea Bolognani wrote:
On Thu, Jun 08, 2023 at 10:37:43AM -0600, Jim Fehlig wrote:
I assumed users would make VM customizations in the per-VM profiles. And I suppose overrides of abstractions seems a little odd to me, but that's subjective :-). I'm fine adding it if there's agreement.
The per-VM profile is generated at runtime based on the template, no? AFAIK there is no way for the admin to inject changes that affect a single VM, but I could be wrong about this.
The per-VM profile is only generated once, right? So in theory admins could amend existing per-VM profiles with custom config.
While working on this I noticed there is no /etc/apparmor.d/local/abstractions directory on SUSE-based distros. A lot of packages put files in /etc/apparmor.d/local, but I couldn't find any adding files to /etc/apparmor.d/local/abstractions. Nor could I find any apparmor documentation regarding the use of that directory. Do you know if it's common practice? Or is that Debian patch the only prior art?
In Debian, the directory and the files within it are created as part of the 'postinst' maintainer script. This makes some amount of sense, as having these files managed by the package manager would kinda defeat the purpose. Unfortunately, it also makes it way harder to figure out whether any packages in the archive besides libvirt are implementing this pattern :( I've come away empty-handed from my attempts, but I have also found a number of references[1] to libvirt's local abstractions overrides in other project's code and documentation, which IMO validates the idea that such a feature is indeed useful and should be available to all distros that use AppArmor, not just the Debian-based ones. That said, I've also found a comment in a somewhat recent Debian bug[2] that explains how support for this pattern comes out-of-the-box with AppArmor 3.0, with the caveat that it expects abstractions/foo.d/bar instead of local/abstractions/foo. Easy enough to adopt for upstream/SUSE, though Debian will have to consider how to carefully migrate things. The catch is that apparently the "include if exists" statement doesn't work well before 3.0, and our support matrix will include distros that are still on AppArmor 2.x for a couple more years :( However, not only you've added a few such statements in your recent commit 9b743ee19053, but I myself have done the same a couple months back with commit 7a39b04d683f, as part of enabling passt support. So in a way we've already started depending on AppArmor 3.0, in open contrast with our platform support policy. I'm quite unclear on the best way forward :( [1] https://github.com/search?q=%2Fetc%2Fapparmor.d%2Flocal%2Fabstractions%2F&type=code&p=2 [2] https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=979500#10 -- Andrea Bolognani / Red Hat / Virtualization
On 6/23/23 07:11, Andrea Bolognani wrote:
On Thu, Jun 22, 2023 at 03:03:56PM -0600, Jim Fehlig wrote:
On 6/22/23 11:08, Jim Fehlig wrote:
On 6/22/23 08:50, Andrea Bolognani wrote:
On Thu, Jun 08, 2023 at 10:37:43AM -0600, Jim Fehlig wrote:
I assumed users would make VM customizations in the per-VM profiles. And I suppose overrides of abstractions seems a little odd to me, but that's subjective :-). I'm fine adding it if there's agreement.
The per-VM profile is generated at runtime based on the template, no? AFAIK there is no way for the admin to inject changes that affect a single VM, but I could be wrong about this.
The per-VM profile is only generated once, right? So in theory admins could amend existing per-VM profiles with custom config.
While working on this I noticed there is no /etc/apparmor.d/local/abstractions directory on SUSE-based distros. A lot of packages put files in /etc/apparmor.d/local, but I couldn't find any adding files to /etc/apparmor.d/local/abstractions. Nor could I find any apparmor documentation regarding the use of that directory. Do you know if it's common practice? Or is that Debian patch the only prior art?
In Debian, the directory and the files within it are created as part of the 'postinst' maintainer script. This makes some amount of sense, as having these files managed by the package manager would kinda defeat the purpose.
Unfortunately, it also makes it way harder to figure out whether any packages in the archive besides libvirt are implementing this pattern :(
I've come away empty-handed from my attempts, but I have also found a number of references[1] to libvirt's local abstractions overrides in other project's code and documentation, which IMO validates the idea that such a feature is indeed useful and should be available to all distros that use AppArmor, not just the Debian-based ones.
Agreed.
That said, I've also found a comment in a somewhat recent Debian bug[2] that explains how support for this pattern comes out-of-the-box with AppArmor 3.0, with the caveat that it expects abstractions/foo.d/bar instead of local/abstractions/foo. Easy enough to adopt for upstream/SUSE, though Debian will have to consider how to carefully migrate things.
Thanks a lot for all the sleuthing! I'm fine with the apparmor 3.0 approach and will consider supporting it in SUSE distros where apparmor >= 3.0.
The catch is that apparently the "include if exists" statement doesn't work well before 3.0, and our support matrix will include distros that are still on AppArmor 2.x for a couple more years :(
That is unfortunate :-(.
However, not only you've added a few such statements in your recent commit 9b743ee19053, but I myself have done the same a couple months back with commit 7a39b04d683f, as part of enabling passt support. So in a way we've already started depending on AppArmor 3.0, in open contrast with our platform support policy.
I'm quite unclear on the best way forward :(
I'd prefer to defer support for local customizations of abstractions until upstream libvirt can support apparmor >= 3.0. In the meantime commit 9b743ee19053 can be changed to 'include <local/foo>' since we provide local/foo. We'd need to drop the include entirely from your commit, and again defer until upstream supports apparmor >= 3.0. Regards, Jim
On Fri, Jun 23, 2023 at 11:31:04AM -0600, Jim Fehlig wrote:
On 6/23/23 07:11, Andrea Bolognani wrote:
However, not only you've added a few such statements in your recent commit 9b743ee19053, but I myself have done the same a couple months back with commit 7a39b04d683f, as part of enabling passt support. So in a way we've already started depending on AppArmor 3.0, in open contrast with our platform support policy.
I'm quite unclear on the best way forward :(
I'd prefer to defer support for local customizations of abstractions until upstream libvirt can support apparmor >= 3.0. In the meantime commit 9b743ee19053 can be changed to 'include <local/foo>' since we provide local/foo. We'd need to drop the include entirely from your commit, and again defer until upstream supports apparmor >= 3.0.
The problem is that passt support won't work if the abstraction is not included, and we can't make the include unconditional in that case. So we'd effectively have to wait two more years to make passt work with AppArmor, which I don't think is acceptable. My best idea at the moment is to make a second copy of the AppArmor profiles targeting 2.x specifically, with a reduced feature set: no passt, no local overrides for abstractions. At build time, we can decide which version of the profiles to install based on the AppArmor version detected on the system. It wouldn't be pretty, but it would get us out of the current situation without modern distros having to sacrifice anything and without causing issues for older distros. In two years, we can drop the additional stuff and go back to a more sane state. What do you think? -- Andrea Bolognani / Red Hat / Virtualization
On 6/26/23 03:52, Andrea Bolognani wrote:
On Fri, Jun 23, 2023 at 11:31:04AM -0600, Jim Fehlig wrote:
On 6/23/23 07:11, Andrea Bolognani wrote:
However, not only you've added a few such statements in your recent commit 9b743ee19053, but I myself have done the same a couple months back with commit 7a39b04d683f, as part of enabling passt support. So in a way we've already started depending on AppArmor 3.0, in open contrast with our platform support policy.
I'm quite unclear on the best way forward :(
I'd prefer to defer support for local customizations of abstractions until upstream libvirt can support apparmor >= 3.0. In the meantime commit 9b743ee19053 can be changed to 'include <local/foo>' since we provide local/foo. We'd need to drop the include entirely from your commit, and again defer until upstream supports apparmor >= 3.0.
The problem is that passt support won't work if the abstraction is not included, and we can't make the include unconditional in that case. So we'd effectively have to wait two more years to make passt work with AppArmor, which I don't think is acceptable.
My best idea at the moment is to make a second copy of the AppArmor profiles targeting 2.x specifically, with a reduced feature set: no passt, no local overrides for abstractions. At build time, we can decide which version of the profiles to install based on the AppArmor version detected on the system.
Specifying which copy to use via a build time option is also an option :-). Does your idea include preserving commit 9b743ee19053 and adjusting the 'include if exists' to 'include'?
It wouldn't be pretty, but it would get us out of the current situation without modern distros having to sacrifice anything and without causing issues for older distros. In two years, we can drop the additional stuff and go back to a more sane state.
What do you think?
As you say, it's not pretty, but I don't have any better ideas. Perhaps Christian B. can give us some hints towards a nicer solution. @cboltz: if needed there's a bit more context a few messages up the thread https://listman.redhat.com/archives/libvir-list/2023-June/240424.html Regards, Jim
On Mon, Jun 26, 2023 at 09:42:32AM -0600, Jim Fehlig wrote:
On 6/26/23 03:52, Andrea Bolognani wrote:
On Fri, Jun 23, 2023 at 11:31:04AM -0600, Jim Fehlig wrote:
On 6/23/23 07:11, Andrea Bolognani wrote:
However, not only you've added a few such statements in your recent commit 9b743ee19053, but I myself have done the same a couple months back with commit 7a39b04d683f, as part of enabling passt support. So in a way we've already started depending on AppArmor 3.0, in open contrast with our platform support policy.
I'm quite unclear on the best way forward :(
I'd prefer to defer support for local customizations of abstractions until upstream libvirt can support apparmor >= 3.0. In the meantime commit 9b743ee19053 can be changed to 'include <local/foo>' since we provide local/foo. We'd need to drop the include entirely from your commit, and again defer until upstream supports apparmor >= 3.0.
The problem is that passt support won't work if the abstraction is not included, and we can't make the include unconditional in that case. So we'd effectively have to wait two more years to make passt work with AppArmor, which I don't think is acceptable.
My best idea at the moment is to make a second copy of the AppArmor profiles targeting 2.x specifically, with a reduced feature set: no passt, no local overrides for abstractions. At build time, we can decide which version of the profiles to install based on the AppArmor version detected on the system.
Specifying which copy to use via a build time option is also an option :-). Does your idea include preserving commit 9b743ee19053 and adjusting the 'include if exists' to 'include'?
The modern (3.x) version would install the profile as it is currently, while the legacy (2.x) version would have the "include if exists" replaced with "include". We could also decide to leave the include out altogether for the legacy version, and simply keep it as the less-featureful version it has been until now. In fact, I wonder if we should install placeholder files for the profiles at all? For abstractions on 3.x, the abstractions/foo.d approach means that such placeholders do not exist. On the other hand, you could argue that the empty abstractions/foo.d directory and the empty local/foo file both serve the same purpose of providing a hint for the user that they can customize things using that mechanism... FWIW, Debian seems to consistently create placeholders for profiles. I think this is done automatically by the dh-apparmor utility that's used as standard for packaging. But that feels like a decision better left to each downstream... Maybe we should look into what upstream AppArmor does for its own profiles and abstractions. -- Andrea Bolognani / Red Hat / Virtualization
[Please CC me, I'm not subscribed to the mailinglist] Hello, regarding the initial patch in this thread: The patch looks good and should go upstream IMHO. (Maybe except creating the dummy local/* files for AppArmor 3.x - see below for details.) A note about what you mentioned in the patch comment: If someone uses aa-logprof to update a profile, it will modify the profile, _not_ the local/ file. (Changing that is on the TODO list, but so far nobody did it.) Therefore I'm not sure if switching from %config(noreplace) to %config is a good idea. Am Montag, 26. Juni 2023, 18:29:11 CEST schrieb Andrea Bolognani:
On Mon, Jun 26, 2023 at 09:42:32AM -0600, Jim Fehlig wrote: [...]
Specifying which copy to use via a build time option is also an option :-). Does your idea include preserving commit 9b743ee19053 and adjusting the 'include if exists' to 'include'?
The modern (3.x) version would install the profile as it is currently, while the legacy (2.x) version would have the "include if exists" replaced with "include".
Sounds like a good idea. Note that you'll have to install dummy files for "include", while they don't have to exist when using "include if exists".
We could also decide to leave the include out altogether for the legacy version, and simply keep it as the less-featureful version it has been until now. In fact, I wonder if we should install placeholder files for the profiles at all?
For AppArmor 3.x, I'd tend to "no". And I say that despite knowing that upstream still creates all the local/ dummy files. (Which reminds me that this should maybe stop doig this in the upcoming 4.0 release. If you want to follow the discussion: https://gitlab.com/apparmor/apparmor/-/issues/337 ) These local sniplets are mostly "noise" (empty/comment-only) files, and it's harder than needed to find files that were actually modified. (Besides that, I'm currently testing a set of ~1000 AppArmor profiles, and having 1000 empty local/ files would make things much harder.)
For abstractions on 3.x, the abstractions/foo.d approach means that such placeholders do not exist. On the other hand, you could argue that the empty abstractions/foo.d directory and the empty local/foo file both serve the same purpose of providing a hint for the user that they can customize things using that mechanism...
If all abstractions would create empty *.d directories by default, that would be quite some noise and useless directories. So please don't do that ;-)
FWIW, Debian seems to consistently create placeholders for profiles. I think this is done automatically by the dh-apparmor utility that's used as standard for packaging. But that feels like a decision better left to each downstream... Maybe we should look into what upstream AppArmor does for its own profiles and abstractions.
See above - IMHO the current upstream behaviour is not perfect, and will hopefully change to not creating the local/ files by default in 4.0. Regards, Christian Boltz -- Social Media News: Instagram is down Science News: Scientists baffled as average human IQ increases over 25% in less than an hour [https://twitter.com/PaulRigbywrites/status/1146505629491781632]
On Mon, Jun 26, 2023 at 10:46:40PM +0200, Christian Boltz wrote:
Am Montag, 26. Juni 2023, 18:29:11 CEST schrieb Andrea Bolognani:
On Mon, Jun 26, 2023 at 09:42:32AM -0600, Jim Fehlig wrote:
Specifying which copy to use via a build time option is also an option :-). Does your idea include preserving commit 9b743ee19053 and adjusting the 'include if exists' to 'include'?
The modern (3.x) version would install the profile as it is currently, while the legacy (2.x) version would have the "include if exists" replaced with "include".
Sounds like a good idea.
Okay, looks like we have at least a rough plan. Great! But this will take some time to implement and test properly, and the freeze for 9.5 has already started. I recommend reverting 9b743ee for now, and target 9.6 with the more complete solution. Jim, does that sound reasonable to you?
These local sniplets are mostly "noise" (empty/comment-only) files, and it's harder than needed to find files that were actually modified. (Besides that, I'm currently testing a set of ~1000 AppArmor profiles, and having 1000 empty local/ files would make things much harder.)
[...]
If all abstractions would create empty *.d directories by default, that would be quite some noise and useless directories. So please don't do that ;-)
I fully agree. As long as the convention is well documented, creating the additional files and directories is unnecessary at best.
FWIW, Debian seems to consistently create placeholders for profiles. I think this is done automatically by the dh-apparmor utility that's used as standard for packaging. But that feels like a decision better left to each downstream... Maybe we should look into what upstream AppArmor does for its own profiles and abstractions.
See above - IMHO the current upstream behaviour is not perfect, and will hopefully change to not creating the local/ files by default in 4.0.
This last bit was more about Debian specifically than upstream, since IIUC it's dh-apparmor that creates the files. But I see that you're one of the maintainers for AppArmor in Debian, so I guess you'll be on top of that on both fronts :) By the way, is there any plan to move from local/foo to foo.d/ for profiles too? I imagine that the main concern would be keeping existing configurations working, but it would be nice to have consistency between the two, and the foo.d/ approach is generally much more flexible... 4.0 material, perhaps? -- Andrea Bolognani / Red Hat / Virtualization
On 6/27/23 10:49, Andrea Bolognani wrote:
On Mon, Jun 26, 2023 at 10:46:40PM +0200, Christian Boltz wrote:
Am Montag, 26. Juni 2023, 18:29:11 CEST schrieb Andrea Bolognani:
On Mon, Jun 26, 2023 at 09:42:32AM -0600, Jim Fehlig wrote:
Specifying which copy to use via a build time option is also an option :-). Does your idea include preserving commit 9b743ee19053 and adjusting the 'include if exists' to 'include'?
The modern (3.x) version would install the profile as it is currently, while the legacy (2.x) version would have the "include if exists" replaced with "include".
Sounds like a good idea.
Okay, looks like we have at least a rough plan. Great!
But this will take some time to implement and test properly, and the freeze for 9.5 has already started. I recommend reverting 9b743ee for now, and target 9.6 with the more complete solution.
Jim, does that sound reasonable to you?
Yes, that's fine. Regards, Jim
On 6/27/23 11:28, Jim Fehlig wrote:
On 6/27/23 10:49, Andrea Bolognani wrote:
On Mon, Jun 26, 2023 at 10:46:40PM +0200, Christian Boltz wrote:
Am Montag, 26. Juni 2023, 18:29:11 CEST schrieb Andrea Bolognani:
On Mon, Jun 26, 2023 at 09:42:32AM -0600, Jim Fehlig wrote:
Specifying which copy to use via a build time option is also an option :-). Does your idea include preserving commit 9b743ee19053 and adjusting the 'include if exists' to 'include'?
The modern (3.x) version would install the profile as it is currently, while the legacy (2.x) version would have the "include if exists" replaced with "include".
Sounds like a good idea.
Okay, looks like we have at least a rough plan. Great!
But this will take some time to implement and test properly, and the freeze for 9.5 has already started. I recommend reverting 9b743ee for now, and target 9.6 with the more complete solution.
Jim, does that sound reasonable to you?
Yes, that's fine.
Done: https://listman.redhat.com/archives/libvir-list/2023-June/240512.html Regards, Jim
Hello, Am Dienstag, 27. Juni 2023, 18:49:04 CEST schrieb Andrea Bolognani:
On Mon, Jun 26, 2023 at 10:46:40PM +0200, Christian Boltz wrote: [...]
See above - IMHO the current upstream behaviour is not perfect, and will hopefully change to not creating the local/ files by default in 4.0. This last bit was more about Debian specifically than upstream, since IIUC it's dh-apparmor that creates the files. But I see that you're one of the maintainers for AppArmor in Debian, so I guess you'll be on top of that on both fronts :)
Thanks for the flowers, but I wouldn't call myself maintainer in Debian. I "only" have an eye on the Debian bugtracker for AppArmor-related bugs, but don't do packaging work there. (Same for Ubuntu, BTW.) If you want AppArmor packages maintained by me, you'll have to install openSUSE ;-) (and yes, I'm fully aware of the irony telling this to someone with a @redhat.com address ;-)
By the way, is there any plan to move from local/foo to foo.d/ for profiles too? I imagine that the main concern would be keeping existing configurations working, but it would be nice to have consistency between the two, and the foo.d/ approach is generally much more flexible... 4.0 material, perhaps?
Interesting idea, I'll bring it up upstream. Regards, Christian Boltz -- My point is that we might be able to maintain a "latest and greatest" less in utilities, rather than in Base:System. Well... if "latest and greatest" applies to less in the first place.. erm... probably not. [Pascal Bleser in opensuse-packaging]
On 6/26/23 14:46, Christian Boltz wrote:
[Please CC me, I'm not subscribed to the mailinglist]
Hello,
regarding the initial patch in this thread: The patch looks good and should go upstream IMHO. (Maybe except creating the dummy local/* files for AppArmor 3.x - see below for details.)
A note about what you mentioned in the patch comment: If someone uses aa-logprof to update a profile, it will modify the profile, _not_ the local/ file. (Changing that is on the TODO list, but so far nobody did it.) Therefore I'm not sure if switching from %config(noreplace) to %config is a good idea.
Hmm. The impetus for that change was a scenario where a new rule in the libvirtd profile was needed for correct VM operation, but the updated profile was not replaced due to local edits. It seems either approach will eventually result in bug reports :-(. Regards, Jim
On 6/23/23 07:11, Andrea Bolognani wrote:
The catch is that apparently the "include if exists" statement doesn't work well before 3.0, and our support matrix will include distros that are still on AppArmor 2.x for a couple more years :(
I'm working on a V2 of this patch and need help understanding what distros falling under the support matrix are on 2.x. Regards, Jim
On 6/28/23 15:25, Jim Fehlig wrote:
On 6/23/23 07:11, Andrea Bolognani wrote:
The catch is that apparently the "include if exists" statement doesn't work well before 3.0, and our support matrix will include distros that are still on AppArmor 2.x for a couple more years :(
I'm working on a V2 of this patch and need help understanding what distros falling under the support matrix are on 2.x.
I'm still personally interested in an answer to this question, but it's not needed for my latest approach https://listman.redhat.com/archives/libvir-list/2023-June/240531.html Regards, Jim
On Wed, Jun 28, 2023 at 03:25:49PM -0600, Jim Fehlig wrote:
On 6/23/23 07:11, Andrea Bolognani wrote:
The catch is that apparently the "include if exists" statement doesn't work well before 3.0, and our support matrix will include distros that are still on AppArmor 2.x for a couple more years :(
I'm working on a V2 of this patch and need help understanding what distros falling under the support matrix are on 2.x.
Looking at [1] and cross-referencing it with the criteria outlined in [2], I believe that the list is limited to Debian 11 and Ubuntu 20.04. [1] https://repology.org/project/apparmor/versions [2] https://libvirt.org/platforms.html -- Andrea Bolognani / Red Hat / Virtualization
On Thu, Jun 29, 2023 at 06:27:10AM -0700, Andrea Bolognani wrote:
On Wed, Jun 28, 2023 at 03:25:49PM -0600, Jim Fehlig wrote:
On 6/23/23 07:11, Andrea Bolognani wrote:
The catch is that apparently the "include if exists" statement doesn't work well before 3.0, and our support matrix will include distros that are still on AppArmor 2.x for a couple more years :(
I'm working on a V2 of this patch and need help understanding what distros falling under the support matrix are on 2.x.
Looking at [1] and cross-referencing it with the criteria outlined in [2], I believe that the list is limited to Debian 11 and Ubuntu 20.04.
I'm now noticing that Debian releases officially go EOL *one year* after the next version is released[3], not two! So we're only stuck supporting AppArmor 2.x until July 2024 :) [3] https://wiki.debian.org/DebianReleases -- Andrea Bolognani / Red Hat / Virtualization
On Thu, Jun 22, 2023 at 7:11 PM Jim Fehlig <jfehlig@suse.com> wrote:
On 6/22/23 08:50, Andrea Bolognani wrote:
On Thu, Jun 08, 2023 at 10:37:43AM -0600, Jim Fehlig wrote:
On 6/8/23 08:11, Andrea Bolognani wrote:
Note that the Debian package has included this patch[1] for many years, and while it partially overlaps with what you've added here, I see that local overrides for abstractions are missing.
Is there a specific reason why you skipped them? Or should we add those too?
I assumed users would make VM customizations in the per-VM profiles. And I suppose overrides of abstractions seems a little odd to me, but that's subjective :-). I'm fine adding it if there's agreement.
The per-VM profile is generated at runtime based on the template, no? AFAIK there is no way for the admin to inject changes that affect a single VM, but I could be wrong about this.
The per-VM profile is only generated once, right? So in theory admins could amend existing per-VM profiles with custom config.
You are both right - one is generated once, the other always :-) In /etc/apparmor.d/libvirt/ you'll have 2 generated files per guest: 1. libvirt-<uuid> 2. libvirt-<uuid>.files #1 is a wrapper - it includes abstractions/libvirt-qemu which hold the global rules - it includes #2 - it is only regenerated when being deleted or in some error conditions, due to that admins could drop in per-guest rules here - this worked, but was so far too uncomfortable (UUID as index, how to find the file, ...) and unreliable (potential to be regenerated in some conditions, we never guaranteed and therefore never tested, due to that this was sometimes broken in the past) to be recommended so far #2 is the set of rules generated based on the guest config - This is regenerated on each start as it has to reflect changes to guest XML => We might improve on that with the discussions started here to make the behavior of #1 expected, reliable and tested.
Anyway, there might be some changes that are local only but apply to all VMs, and allowing overrides to the abstractions would cater to that use case, so it makes sense to me to implement those as well.
Do you mind cooking up a patch so that we can have the whole sha-bang included in the upcoming release? Thanks in advance!
I should have time to do that today.
Regards, Jim
-- Christian Ehrhardt Senior Staff Engineer and acting Director, Ubuntu Server Canonical Ltd
participants (5)
-
Andrea Bolognani -
Christian Boltz -
Christian Ehrhardt -
Jim Fehlig -
Michal Prívozník