src/security/virt-aa-helper.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)
We attempted multiple ways to clean up dynamic files; however, we must
preserve user overrides, which requires keeping the file
/etc/apparmor.d/libvirt/libvirt-uuid
This commit proposes to move user overrides into
/etc/apparmor.d/libvirt/libvirt-uuid.local and include it, if present,
unconditionally. When we stop the domain, we remove libvirt.uuid and
libvirt-uuid.files, whereas we preserve libvirt-uuid.local if present.
Applying the patch, it produces the following:
root@virt-hv-lab002:/etc/apparmor.d/libvirt# ls -1 libvirt-e7424556-ffc1-4f6e-bafa-84e66c4dc033*
libvirt-e7424556-ffc1-4f6e-bafa-84e66c4dc033
libvirt-e7424556-ffc1-4f6e-bafa-84e66c4dc033.files
root@virt-hv-lab002:/etc/apparmor.d/libvirt# cat libvirt-e7424556-ffc1-4f6e-bafa-84e66c4dc033
profile libvirt-e7424556-ffc1-4f6e-bafa-84e66c4dc033 flags=(attach_disconnected) {
#include <abstractions/libvirt-qemu>
#include if exists <libvirt/libvirt-e7424556-ffc1-4f6e-bafa-84e66c4dc033.files>
#include if exists <libvirt/libvirt-e7424556-ffc1-4f6e-bafa-84e66c4dc033.local>
}
root@virt-hv-lab002:/etc/apparmor.d/libvirt# cat libvirt-e7424556-ffc1-4f6e-bafa-84e66c4dc033.files
"/var/log/libvirt/**/testing-9a4be628.log" w,
"/var/lib/libvirt/qemu/domain-testing-9a4be628/monitor.sock" rw,
"/var/lib/libvirt/qemu/domain-4-testing-9a4be628/*" rw,
"/var/run/libvirt/**/testing-9a4be628.pid" rwk,
"/var/run/libvirt/**/*.tunnelmigrate.dest.testing-9a4be628" rw,
"/var/lib/libvirt/images/testing-9a4be628.qcow2" rwk,
"/var/lib/libvirt/images/noble-server-cloudimg-amd64.img" rk,
# don't audit writes to readonly files
deny "/var/lib/libvirt/images/noble-server-cloudimg-amd64.img" w,
"/var/lib/libvirt/images/testing-9a4be628-ds.qcow2" rwk,
"/usr/share/OVMF/OVMF_CODE_4M.fd" rk,
# don't audit writes to readonly files
deny "/usr/share/OVMF/OVMF_CODE_4M.fd" w,
"/var/lib/libvirt/qemu/nvram/testing-9a4be628_VARS.fd" rwk,
"/dev/vhost-net" rw,
"/var/lib/libvirt/qemu/domain-4-testing-9a4be628/{,**}" rwk,
"/run/libvirt/qemu/channel/4-testing-9a4be628/{,**}" rwk,
"/var/lib/libvirt/qemu/domain-4-testing-9a4be628/master-key.aes" rwk,
"/dev/net/tun" rwk,
"/dev/userfaultfd" rwk,
Fixes: https://gitlab.com/libvirt/libvirt/-/issues/451
Signed-off-by: Alessandro <alessandro@0x65c.net>
---
src/security/virt-aa-helper.c | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c
index 034c042..6efe39c 100644
--- a/src/security/virt-aa-helper.c
+++ b/src/security/virt-aa-helper.c
@@ -1495,8 +1495,10 @@ main(int argc, char **argv)
rc = parserLoad(ctl->uuid);
} else if (ctl->cmd == 'R' || ctl->cmd == 'D') {
rc = parserRemove(ctl->uuid);
- if (ctl->cmd == 'D')
+ if (ctl->cmd == 'D') {
unlink(include_file);
+ unlink(profile);
+ }
} else if (ctl->cmd == 'c' || ctl->cmd == 'r') {
g_autofree char *included_files = NULL;
g_auto(virBuffer) buf = VIR_BUFFER_INITIALIZER;
@@ -1566,7 +1568,9 @@ main(int argc, char **argv)
#else
const char *ifexists = "";
#endif
- tmp = g_strdup_printf(" #include %s<libvirt/%s.files>\n", ifexists, ctl->uuid);
+ tmp = g_strdup_printf(" #include %s<libvirt/%s.files>\n" \
+ " #include %s<libvirt/%s.local>\n",
+ ifexists, ctl->uuid, ifexists, ctl->uuid);
if (ctl->dryrun) {
vah_info(profile);
--
2.49.0On Tue, Apr 01, 2025 at 10:55:28AM +0200, Alessandro wrote: > We attempted multiple ways to clean up dynamic files; however, we must > preserve user overrides, which requires keeping the file > /etc/apparmor.d/libvirt/libvirt-uuid > > This commit proposes to move user overrides into > /etc/apparmor.d/libvirt/libvirt-uuid.local and include it, if present, > unconditionally. When we stop the domain, we remove libvirt.uuid and > libvirt-uuid.files, whereas we preserve libvirt-uuid.local if present. The way you describe things, it sounds like the AppArmor driver already expects local overrides to exist. Is that documented anywhere? If so, an update is probably needed. And either way, this file you're introducing and its purpose will have to be documented. -- Andrea Bolognani / Red Hat / Virtualization
On Tue, 1 Apr 2025 at 14:22, Andrea Bolognani <abologna@redhat.com> wrote: > > On Tue, Apr 01, 2025 at 10:55:28AM +0200, Alessandro wrote: > > We attempted multiple ways to clean up dynamic files; however, we must > > preserve user overrides, which requires keeping the file > > /etc/apparmor.d/libvirt/libvirt-uuid > > > > This commit proposes to move user overrides into > > /etc/apparmor.d/libvirt/libvirt-uuid.local and include it, if present, > > unconditionally. When we stop the domain, we remove libvirt.uuid and > > libvirt-uuid.files, whereas we preserve libvirt-uuid.local if present. > > The way you describe things, it sounds like the AppArmor driver > already expects local overrides to exist. Is that documented > anywhere? If so, an update is probably needed. And either way, this > file you're introducing and its purpose will have to be documented. Thank you for your remark, Andrea. AFAICT, it's documented here https://gitlab.com/apparmor/apparmor/-/wikis/Libvirt#advanced-usage and in docs/drvqemu.rst. If my proposal is accepted, I'll update those pages accordingly with a separate patch, clearly stating that the behaviour has changed and the user overrides must be saved into the /etc/apparmor.d/libvirt/libvirt-uuid.local file. I don't know if I can modify the Gitlab wiki's sending a patch though :) Thank you, Best regards A.
On Tue, Apr 01, 2025 at 04:00:42PM +0200, Alessandro wrote:
> On Tue, 1 Apr 2025 at 14:22, Andrea Bolognani <abologna@redhat.com> wrote:
> > On Tue, Apr 01, 2025 at 10:55:28AM +0200, Alessandro wrote:
> > > We attempted multiple ways to clean up dynamic files; however, we must
> > > preserve user overrides, which requires keeping the file
> > > /etc/apparmor.d/libvirt/libvirt-uuid
> > >
> > > This commit proposes to move user overrides into
> > > /etc/apparmor.d/libvirt/libvirt-uuid.local and include it, if present,
> > > unconditionally. When we stop the domain, we remove libvirt.uuid and
> > > libvirt-uuid.files, whereas we preserve libvirt-uuid.local if present.
> >
> > The way you describe things, it sounds like the AppArmor driver
> > already expects local overrides to exist. Is that documented
> > anywhere? If so, an update is probably needed. And either way, this
> > file you're introducing and its purpose will have to be documented.
>
> Thank you for your remark, Andrea.
> AFAICT, it's documented here
> https://gitlab.com/apparmor/apparmor/-/wikis/Libvirt#advanced-usage
> and in docs/drvqemu.rst. If my proposal is accepted, I'll update those
> pages accordingly with a separate patch, clearly stating that the
> behaviour has changed and the user overrides must be saved into the
> /etc/apparmor.d/libvirt/libvirt-uuid.local file.
> I don't know if I can modify the Gitlab wiki's sending a patch though :)
Probably not, but that's something that we don't have control over
anyway so you'll need to figure it out together with the maintainers
of the AppArmor project.
For libvirt, the documentation should be updated at the same time as
the code it refers to is changed, meaning you should do so as part of
your patch rather than as a follow-up.
The fact that the profile itself now gets deleted when the domain is
undefined is a pretty significant behavioral change that IMO needs to
be mentioned in the release notes too.
More importantly, I'm not convinced that you can just start deleting
that file unconditionally. Since, as you note, it's explicitly
documented that the user might add local customizations to the
profile, deleting it might result in the loss of those local changes.
So I think you need something more like:
expected = generate_default_profile();
actual = read_existing_profile();
if (STREQ(actual, expected)) {
delete_profile();
}
In other words, only delete the existing profile if you can prove
that it contains no valuable information.
--
Andrea Bolognani / Red Hat / Virtualization
On Tue, Apr 01, 2025 at 07:15:46AM -0700, Andrea Bolognani via Devel wrote:
> On Tue, Apr 01, 2025 at 04:00:42PM +0200, Alessandro wrote:
> > On Tue, 1 Apr 2025 at 14:22, Andrea Bolognani <abologna@redhat.com> wrote:
> > > On Tue, Apr 01, 2025 at 10:55:28AM +0200, Alessandro wrote:
> > > > We attempted multiple ways to clean up dynamic files; however, we must
> > > > preserve user overrides, which requires keeping the file
> > > > /etc/apparmor.d/libvirt/libvirt-uuid
> > > >
> > > > This commit proposes to move user overrides into
> > > > /etc/apparmor.d/libvirt/libvirt-uuid.local and include it, if present,
> > > > unconditionally. When we stop the domain, we remove libvirt.uuid and
> > > > libvirt-uuid.files, whereas we preserve libvirt-uuid.local if present.
> > >
> > > The way you describe things, it sounds like the AppArmor driver
> > > already expects local overrides to exist. Is that documented
> > > anywhere? If so, an update is probably needed. And either way, this
> > > file you're introducing and its purpose will have to be documented.
> >
> > Thank you for your remark, Andrea.
> > AFAICT, it's documented here
> > https://gitlab.com/apparmor/apparmor/-/wikis/Libvirt#advanced-usage
> > and in docs/drvqemu.rst. If my proposal is accepted, I'll update those
> > pages accordingly with a separate patch, clearly stating that the
> > behaviour has changed and the user overrides must be saved into the
> > /etc/apparmor.d/libvirt/libvirt-uuid.local file.
> > I don't know if I can modify the Gitlab wiki's sending a patch though :)
>
> Probably not, but that's something that we don't have control over
> anyway so you'll need to figure it out together with the maintainers
> of the AppArmor project.
>
> For libvirt, the documentation should be updated at the same time as
> the code it refers to is changed, meaning you should do so as part of
> your patch rather than as a follow-up.
>
> The fact that the profile itself now gets deleted when the domain is
> undefined is a pretty significant behavioral change that IMO needs to
> be mentioned in the release notes too.
>
> More importantly, I'm not convinced that you can just start deleting
> that file unconditionally. Since, as you note, it's explicitly
> documented that the user might add local customizations to the
> profile, deleting it might result in the loss of those local changes.
>
> So I think you need something more like:>
> expected = generate_default_profile();
> actual = read_existing_profile();
> if (STREQ(actual, expected)) {
> delete_profile();
> }
>
> In other words, only delete the existing profile if you can prove
> that it contains no valuable information.
Not sure its that's simple as we've changed what's in the profile
over time, especially if the host in question upgraded from
apparmor 2.x to 3.x, so such a comparison is likely to get false
results a decent amount of the time.
It might need to be more of a config file setting to allow people
turn off, and/or a build time default if a distro really wants
to preserve old behaviour a bit longer
With regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On Tue, Apr 01, 2025 at 03:24:10PM +0100, Daniel P. Berrangé wrote:
> On Tue, Apr 01, 2025 at 07:15:46AM -0700, Andrea Bolognani via Devel wrote:
> > More importantly, I'm not convinced that you can just start deleting
> > that file unconditionally. Since, as you note, it's explicitly
> > documented that the user might add local customizations to the
> > profile, deleting it might result in the loss of those local changes.
> >
> > So I think you need something more like:>
>
> > expected = generate_default_profile();
> > actual = read_existing_profile();
> > if (STREQ(actual, expected)) {
> > delete_profile();
> > }
> >
> > In other words, only delete the existing profile if you can prove
> > that it contains no valuable information.
>
> Not sure its that's simple as we've changed what's in the profile
> over time, especially if the host in question upgraded from
> apparmor 2.x to 3.x, so such a comparison is likely to get false
> results a decent amount of the time.
You're right about the AppArmor 2/3 part. And of course the admin
might have made local modifications to the template over time too,
which a simplistic approach like the one I've described above would
not capture correctly.
Still, false positives (i.e. the profiles incorrectly being detected
as containing local modifications) are not that harmful, as they only
result in lack of cleanup, which is the previous status quo. So
perhaps that's tolerable.
> It might need to be more of a config file setting to allow people
> turn off, and/or a build time default if a distro really wants
> to preserve old behaviour a bit longer
I'm not a big fan of having to introduce yet another tunable. If it
really must exist, I think it should be a "force cleanup ignoring
local modification detection logic" bool.
--
Andrea Bolognani / Red Hat / Virtualization
On Tue, Apr 01, 2025 at 08:12:11AM -0700, Andrea Bolognani wrote:
> On Tue, Apr 01, 2025 at 03:24:10PM +0100, Daniel P. Berrangé wrote:
> > On Tue, Apr 01, 2025 at 07:15:46AM -0700, Andrea Bolognani via Devel wrote:
> > > More importantly, I'm not convinced that you can just start deleting
> > > that file unconditionally. Since, as you note, it's explicitly
> > > documented that the user might add local customizations to the
> > > profile, deleting it might result in the loss of those local changes.
> > >
> > > So I think you need something more like:>
> >
> > > expected = generate_default_profile();
> > > actual = read_existing_profile();
> > > if (STREQ(actual, expected)) {
> > > delete_profile();
> > > }
> > >
> > > In other words, only delete the existing profile if you can prove
> > > that it contains no valuable information.
> >
> > Not sure its that's simple as we've changed what's in the profile
> > over time, especially if the host in question upgraded from
> > apparmor 2.x to 3.x, so such a comparison is likely to get false
> > results a decent amount of the time.
>
> You're right about the AppArmor 2/3 part. And of course the admin
> might have made local modifications to the template over time too,
> which a simplistic approach like the one I've described above would
> not capture correctly.
>
> Still, false positives (i.e. the profiles incorrectly being detected
> as containing local modifications) are not that harmful, as they only
> result in lack of cleanup, which is the previous status quo. So
> perhaps that's tolerable.
>
> > It might need to be more of a config file setting to allow people
> > turn off, and/or a build time default if a distro really wants
> > to preserve old behaviour a bit longer
>
> I'm not a big fan of having to introduce yet another tunable. If it
> really must exist, I think it should be a "force cleanup ignoring
> local modification detection logic" bool.
I guess I'd like to see some input from the Canonical maintainers on what
upgrade strategy works best for them, since Ubuntu is the platform that
this change will have the biggest upgrade impact on.
With regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
© 2016 - 2025 Red Hat, Inc.