Please add guest uuid and guest-generic local include files

[Christian Ehrhardt ]

No matter how much we improve the per guest dynamic profiles, there might always be edge cases not covered.

We do have a generic profile which all lbvirt-qemu are using via an include from the base guest template.
Each guest profile does:
#include <abstractions/libvirt-qemu> <- generic things
#include <libvirt/libvirt-d424f51d-4fee-409d-87d0-e613089f3ec4.files> <- generated for the guest

We have a local override for special cases that affect libvirt or virt-aa-helper:
grep -Hrn include /etc/apparmor.d/* | grep local | grep libvirt
/etc/apparmor.d/usr.lib.libvirt.virt-aa-helper:92: #include <local/usr.lib.libvirt.virt-aa-helper>
/etc/apparmor.d/usr.sbin.libvirtd:107: #include <local/usr.sbin.libvirtd>
Those overrides are for users to modify, and are not messed with by packaging updates and similar.

But we miss two things:
1. a local override for ALL GUESTS (that would be included from abstractions/libvirt-qemu)
2. a local override PER GUEST (to only allow something very specific for one guest) that would be
   included from the guests libvirt/libvirt-<uuid>.files

So I propose at:
libvirt/libvirt-<uuid>:
#include <local/libvirt-<uuid>.files> <- local override for the guest itself

And at:
abstractions/libvirt-qemu
#include <local/libvirt-qemu> <- local override for guest in general

I see that people might prefer names, but those might be ambiguous - did you really mean "test" yesterday to be the same as "test" today and similar?
So it shall be uuids.

So far this is for remembering the idea, need to prep something for upstream to ack on once I get to it.

[Christian Ehrhardt  (paelzer)]

Just now possible:
- Needs apparmor 2.12.1, 2.11.2, and 2.10.4
- So libvirt needs hard dependency on that
- New syntax is like:
  include if exists (<...>|"...")
  so just like regular includes with "if exists" between the include and the path

Next steps: wait until new apparmor is ready and then work on patches for upstream.

Note: might need a compile time check on apparmor version it is built against to not fail on older apparmor (OR the hard dependency in packaging, but for upstream some config/compile time check would be nicer).

[Christian Ehrhardt  (paelzer)]

Check if we could use this tech to also provide something like a conf.d directory for virt-aa-helper.
E.g. tools that use other image paths could drop conf files - need to check wildcard support on the include thou if that is possible.

[Christian Ehrhardt  (paelzer)]

The code is not in Cosmic, so not considering this this cycle yet

[Christian Ehrhardt  (paelzer)]

Split bug 1786019 for the non "if exists" code

[Christian Ehrhardt  (paelzer)]

Actually I'm stupid, the base file being /etc/apparmor.d/libvirt/libvirt-<uuid> is providing exactly that already.
I need to check if that would survive a lifecycle of destroy/undefine/define and such (I think recent versions remove it), but we should check before adding another.

[Christian Ehrhardt  (paelzer)]

As I assumed the old (cron) and new (virt-aa-helper call) cleanup code makes the libvirt-<uuid> files unusable.
Also all old approaches wanted to avoid cluttering /etc/apparmor.d/libvirt/* to not loose overview.
Best (not fastest, but best) solution would really the "include if avail" once available in apparmor.

That would provide clean config dirs for those who do not use it while at the same time allowing per guest overrides for those who need it.
Also from there people could even make groups like
libvirt-<uuid>
  include if -> libvirt-local-<uuid>

libvirt-local-<uuid> (a set of UUIDs would do that)
  include allow-my-special-conf-A

allow-my-special-conf-A
  rule for A

[Christian Ehrhardt  (paelzer)]

I summarized it in another bug nicely (I think), quoting it here to be on the main bug as well:

This had two phases with both having had their own issues:

Phase I: ~2013-2016: the libvirt-<uuid> stayed around and could be used for such overrides, but they cluttered the file system and overview was lost, therefore a cron daemon was established to clean files of currently undefined domains (which still could kill overrides that people wanted to stay).
Override: Was too unreliable to rely on it

Phase II: 2017-now the cleaning became part of libvirt itself due to [1]. This will immediately remove the file and unload profiles, keeping the config dir clean but even removed unreliable override capability we had.
Override: doesn't work at all

Phase III: future as planned in this bug.
It is intended to use the new "include if available" feature of apparmor to allow providing non-cleaned overrides to just those guests that you want/need.
That would mean on most installations there would be no extra config clutter at all.
On others where it is needed they can be used for overrides.

[Christian Ehrhardt  (paelzer)]

FYI - until we can make this work, please do realize that at least the global abstraction libvirt-qemu got a local override in Cosmic that can be used to extend rules for all guests without having to bother with conffile prompts later on.

[Christian Ehrhardt  (paelzer)]

I might have asked at a bad time of day, document it here
@jjohansen/jdstrand (now subscribed to the bug):

[13:50] does the 2.13.2-9ubuntu7 have the "include if exists" code that we once discussed for https://bugs.launchpad.net/ubuntu/+source/libvirt/+bug/1745114 ?
[13:50] if so, is there a way to compile time check on the feature?
[13:50] e.g. ./configure detects "if exists is available" -> define some VAR -> based on that render the code that uses "if exists"
[13:51] as IIRC if you use it on an apparmor that doesn't support it it will fail to parse
[13:52] we already do a config check in libvirt for aa_change_profile if there is any else for the if exists feature that would be great
[13:52] even if there is a version include I could really depend on
[13:54] I have taken a look at libapparmor-dev but found nothing, if there is anything I missed a hint would be great
[13:54] the only thing might be in the package config Version: 2.13.2
[13:55] if you are telling me that all >=2.13 have the feature that might be enough as well

[Christian Ehrhardt  (paelzer)]

I got these:

$ echo includetest
/f {
 # target doesn't need to exist to test if this rule can be compiled
 include if exists <foo>
}

$ apparmor_parser -QT ./includetest
AppArmor parser error for ./include.test in ./include.test at line 3: Could not open 'if'
$

OR (if supported)

$ echo includetest
# target doesn't need to exist to test if this rule can be compiled
include if exists <foo>

$ apparmor_parser -QT ./includetest
$

That is nice, but still a runtime test.
I'd like to have a compile time test, but that doesn't seem to exist right now :-/
Thanks @jjohansen for this test still!

As I assumed before I think the best chance we have for a real compile time test is to tap on the pkg-config of libapparmor-dev and check that it is >=2.13.
And we can ensure that on merging this the package also has a versioned dependency to *apparmor* things.

[Christian Ehrhardt  (paelzer)]

Turns out this isn't as complex as I thought.
It was meant to work anyway but broken inadvertently in 2017 by eba2225b.

That will allow per UUID overrides and is what one would want IMHO.

I started to upstream the given change and will include it in 6.6 if things work out.

[Launchpad Janitor (janitor)]

This bug was fixed in the package libvirt - 6.6.0-1ubuntu2

---------------
libvirt (6.6.0-1ubuntu2) groovy; urgency=medium

  * d/p/u/lp-1892826-Revert-m4-virt-xdr-rewrite-XDR-check.patch: avoid clashes
    between libtripc and glibc that break libvirt-lxc (LP: #1892826)
  * d/p/ubuntu-aa/lp-1892736-apparmor-allow-libvirtd-to-call-virtiofsd.patch:
    allow libvirt to control virtiofsd (LP: #1892736)

libvirt (6.6.0-1ubuntu1) groovy; urgency=medium

  * Merge with Debian 6.6.0-1 from experimental
    Among many other new features and fixes this includes fixes for:
    (LP: #1874647) - Stale libvirt cache leads to VM startup failures
    (LP: #1869796) - bad ordering and dependent restarts of services/sockets
    Remaining changes:
    - d/p/ubuntu-aa/lp-1847361-load-versioned-module.patch: allow loading
      versioned modules after qemu package upgrades (LP 1847361)
    - libvirt-uri.sh: Automatically switch default libvirt URI for users
      via user profile (xen URI on dom0, qemu:///system otherwise)
    - Disable libssh2 support (universe dependency)
    - Disable firewalld support (universe dependency)
    - Set qemu-group to kvm (for compat with older ubuntu)
    - Additional apport package-hook
    - Autostart default bridged network (As upstream does, but not Debian).
      In addition to just enabling it our solution provides:
      + do not autostart if subnet is already taken (e.g. in guests).
      + iterate some alternative subnets before giving up
    - d/p/ubuntu/Allow-libvirt-group-to-access-the-socket.patch: This is
      the group based access to libvirt functions as it was used in Ubuntu
      for quite long.
      + d/p/ubuntu/daemon-augeas-fix-expected.patch fix some related tests
        due to the group access change.
      + d/libvirt-daemon-system.postinst: add users in sudo to the libvirt
        group.
    - ubuntu/parallel-shutdown.patch: set parallel shutdown by default.
    - Update README.Debian with Ubuntu changes
    - d/p/ubuntu/ubuntu_machine_type.patch: accept ubuntu types as pci440fx
    - fix autopkgtests
      + d/t/control, d/t/smoke-qemu-session: fixup smoke-qemu-session by making
        vmlinuz available and accessible (Debian bug 848314)
      + d/t/control: fix smoke-qemu-session by ensuring the service will run
        installing libvirt-daemon-system
      + d/t/smoke-lxc: fix smoke-lxc by ignoring potential issues on destroy as
        long as the following undefine succeeds
      + d/t/smoke-lxc: use systemd instead of sysV to restart the service
    - dnsmasq related enhancements
      + run dnsmasq as libvirt-dnsmasq (LP: 1743718)
      + d/libvirt-daemon-system.postinst: add libvirt-dnsmasq user and group
      + d/libvirt-daemon-system.postrm: remove libvirt-dnsmasq user and group
        on purge
      + d/p/ubuntu/dnsmasq-as-priv-user: write dnsmasq config with user
        libvirt-dnsmasq and adapt the self tests to expect that config
      + d/libvirt-daemon-system.postinst: fix old libvirt-dnsmasq users group
      + Add dnsmasq configuration to work with system wide dnsmasq-base
    - debian/rules: disable the netcf backend. (LP: 1764314)
    - debian/patches/ubuntu/ovmf_paths.patch...