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 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/patches/debian/apparmor_profiles_local_include.patch
-- 
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 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

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:
> 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 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 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 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

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/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

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