[libvirt PATCH] apparmor: Enable passt support

Andrea Bolognani posted 1 patch 1 year, 3 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/20230307190237.521986-1-abologna@redhat.com
src/security/apparmor/libvirt-qemu | 15 +++++++++++++++
1 file changed, 15 insertions(+)
[libvirt PATCH] apparmor: Enable passt support
Posted by Andrea Bolognani 1 year, 3 months ago
passt provides an AppArmor abstraction that covers all the
inner details of its operation, so we can simply import that
and add the libvirt-specific parts on top: namely, passt
needs to be able to create a socket and pid file, while
the libvirt daemon needs to be able to kill passt.

Signed-off-by: Andrea Bolognani <abologna@redhat.com>
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
---
 src/security/apparmor/libvirt-qemu | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/src/security/apparmor/libvirt-qemu b/src/security/apparmor/libvirt-qemu
index 9af1333b22..44056b5f14 100644
--- a/src/security/apparmor/libvirt-qemu
+++ b/src/security/apparmor/libvirt-qemu
@@ -185,6 +185,21 @@
   /usr/{lib,lib64}/libswtpm_libtpms.so mr,
   /usr/lib/@{multiarch}/libswtpm_libtpms.so mr,
 
+  # support for passt network back-end
+  /usr/bin/passt Cx -> passt,
+
+  profile passt {
+    /usr/bin/passt r,
+
+    signal (receive) set=("term") peer=/usr/sbin/libvirtd,
+    signal (receive) set=("term") peer=libvirtd,
+    signal (receive) set=("term") peer=virtqemud,
+
+    owner /{,var/}run/libvirt/qemu/passt/* rw,
+
+    include if exists <abstractions/passt>
+  }
+
   # for save and resume
   /{usr/,}bin/dash rmix,
   /{usr/,}bin/dd rmix,
-- 
2.39.2
Re: [libvirt PATCH] apparmor: Enable passt support
Posted by Daniel P. Berrangé 1 year, 3 months ago
On Tue, Mar 07, 2023 at 08:02:37PM +0100, Andrea Bolognani wrote:
> passt provides an AppArmor abstraction that covers all the
> inner details of its operation, so we can simply import that
> and add the libvirt-specific parts on top: namely, passt
> needs to be able to create a socket and pid file, while
> the libvirt daemon needs to be able to kill passt.
> 
> Signed-off-by: Andrea Bolognani <abologna@redhat.com>
> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
> ---
>  src/security/apparmor/libvirt-qemu | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
> 
> diff --git a/src/security/apparmor/libvirt-qemu b/src/security/apparmor/libvirt-qemu
> index 9af1333b22..44056b5f14 100644
> --- a/src/security/apparmor/libvirt-qemu
> +++ b/src/security/apparmor/libvirt-qemu
> @@ -185,6 +185,21 @@
>    /usr/{lib,lib64}/libswtpm_libtpms.so mr,
>    /usr/lib/@{multiarch}/libswtpm_libtpms.so mr,
>  
> +  # support for passt network back-end
> +  /usr/bin/passt Cx -> passt,
> +
> +  profile passt {
> +    /usr/bin/passt r,
> +
> +    signal (receive) set=("term") peer=/usr/sbin/libvirtd,
> +    signal (receive) set=("term") peer=libvirtd,

What's the rationale for having both qualified & unqualified
here, but not below ?

> +    signal (receive) set=("term") peer=virtqemud,
> +
> +    owner /{,var/}run/libvirt/qemu/passt/* rw,
> +
> +    include if exists <abstractions/passt>
> +  }

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 :|
Re: [libvirt PATCH] apparmor: Enable passt support
Posted by Andrea Bolognani 1 year, 3 months ago
On Tue, Mar 07, 2023 at 07:04:25PM +0000, Daniel P. Berrangé wrote:
> On Tue, Mar 07, 2023 at 08:02:37PM +0100, Andrea Bolognani wrote:
> > +  # support for passt network back-end
> > +  /usr/bin/passt Cx -> passt,
> > +
> > +  profile passt {
> > +    /usr/bin/passt r,
> > +
> > +    signal (receive) set=("term") peer=/usr/sbin/libvirtd,
> > +    signal (receive) set=("term") peer=libvirtd,
>
> What's the rationale for having both qualified & unqualified
> here, but not below ?

Cargo cult. That's what the top-level profile does, so I figured it
would be good enough for the subprofile too.

I've seen stuff like peer=(label=libvirtd) as well, but I haven't
investigated the various notations and how exactly they differ.

There's plenty of room for improvement in the AppArmor profile in
general, but that's a task for another day :)

-- 
Andrea Bolognani / Red Hat / Virtualization
Re: [libvirt PATCH] apparmor: Enable passt support
Posted by Andrea Bolognani 1 year, 3 months ago
On Tue, Mar 07, 2023 at 01:28:41PM -0800, Andrea Bolognani wrote:
> On Tue, Mar 07, 2023 at 07:04:25PM +0000, Daniel P. Berrangé wrote:
> > On Tue, Mar 07, 2023 at 08:02:37PM +0100, Andrea Bolognani wrote:
> > > +  # support for passt network back-end
> > > +  /usr/bin/passt Cx -> passt,
> > > +
> > > +  profile passt {
> > > +    /usr/bin/passt r,
> > > +
> > > +    signal (receive) set=("term") peer=/usr/sbin/libvirtd,
> > > +    signal (receive) set=("term") peer=libvirtd,
> >
> > What's the rationale for having both qualified & unqualified
> > here, but not below ?
>
> Cargo cult. That's what the top-level profile does, so I figured it
> would be good enough for the subprofile too.
>
> I've seen stuff like peer=(label=libvirtd) as well, but I haven't
> investigated the various notations and how exactly they differ.

It looks like using the label= version is only needed when additional
information is passed, for example in

  unix (send, receive) type=stream addr=none peer=(label=unconfined addr=none),

Each profile has its own name attached as a label, so

  peer=libvirtd

and

  peer=(label=libvirtd)

are effectively equivalent. Similarly, the binary running under a
profile also becomes a label, making

  peer=libvirtd

and

  peer=/usr/sbin/libvirtd

also equivalent, which means that having both in the profile is
redundant.

> There's plenty of room for improvement in the AppArmor profile in
> general, but that's a task for another day :)

Based on the above, I'm working on a patch that cleans up the
situation by sticking with the simpler form. Plus some other stuff.

I don't want to make such a cleanup patch a requirement for this
functionally relevant one though, especially in the not entirely
remote chance that I have completely misunderstood how this stuff
work O:-)

So I'd prefer to keep the cargo cult line for now, and clean
everything up at once later.

-- 
Andrea Bolognani / Red Hat / Virtualization
Re: [libvirt PATCH] apparmor: Enable passt support
Posted by Daniel P. Berrangé 1 year, 3 months ago
On Wed, Mar 08, 2023 at 09:23:19AM -0800, Andrea Bolognani wrote:
> On Tue, Mar 07, 2023 at 01:28:41PM -0800, Andrea Bolognani wrote:
> > On Tue, Mar 07, 2023 at 07:04:25PM +0000, Daniel P. Berrangé wrote:
> > > On Tue, Mar 07, 2023 at 08:02:37PM +0100, Andrea Bolognani wrote:
> > > > +  # support for passt network back-end
> > > > +  /usr/bin/passt Cx -> passt,
> > > > +
> > > > +  profile passt {
> > > > +    /usr/bin/passt r,
> > > > +
> > > > +    signal (receive) set=("term") peer=/usr/sbin/libvirtd,
> > > > +    signal (receive) set=("term") peer=libvirtd,
> > >
> > > What's the rationale for having both qualified & unqualified
> > > here, but not below ?
> >
> > Cargo cult. That's what the top-level profile does, so I figured it
> > would be good enough for the subprofile too.
> >
> > I've seen stuff like peer=(label=libvirtd) as well, but I haven't
> > investigated the various notations and how exactly they differ.
> 
> It looks like using the label= version is only needed when additional
> information is passed, for example in
> 
>   unix (send, receive) type=stream addr=none peer=(label=unconfined addr=none),
> 
> Each profile has its own name attached as a label, so
> 
>   peer=libvirtd
> 
> and
> 
>   peer=(label=libvirtd)
> 
> are effectively equivalent. Similarly, the binary running under a
> profile also becomes a label, making
> 
>   peer=libvirtd
> 
> and
> 
>   peer=/usr/sbin/libvirtd
> 
> also equivalent, which means that having both in the profile is
> redundant.
> 
> > There's plenty of room for improvement in the AppArmor profile in
> > general, but that's a task for another day :)
> 
> Based on the above, I'm working on a patch that cleans up the
> situation by sticking with the simpler form. Plus some other stuff.
> 
> I don't want to make such a cleanup patch a requirement for this
> functionally relevant one though, especially in the not entirely
> remote chance that I have completely misunderstood how this stuff
> work O:-)
> 
> So I'd prefer to keep the cargo cult line for now, and clean
> everything up at once later.

Sure, if its already inconsistent, then this makes it no worse...

   Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> 

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