[PATCH] apparmor: allow kvm-spice compat wrapper

Christian Ehrhardt posted 1 patch 3 years, 4 months ago
Test syntax-check failed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/20201116122616.115056-1-christian.ehrhardt@canonical.com
src/security/apparmor/libvirt-qemu | 1 +
1 file changed, 1 insertion(+)
[PATCH] apparmor: allow kvm-spice compat wrapper
Posted by Christian Ehrhardt 3 years, 4 months ago
'kvm-spice' is a binary name used to call 'kvm' which actually is a wrapper
around qemu-system-x86_64 enabling kvm acceleration. This isn't in use
for quite a while anymore, but required to work for compatibility e.g.
when migrating in old guests.

For years this was a symlink kvm-spice->kvm and therefore covered
apparmor-wise by the existing entry:
   /usr/bin/kvm rmix,
But due to a recent change [1] in qemu packaging this now is no symlink,
but a wrapper on its own and therefore needs an own entry that allows it
to be executed.

[1]: https://salsa.debian.org/qemu-team/qemu/-/commit/9944836d3

Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com>
---
 src/security/apparmor/libvirt-qemu | 1 +
 1 file changed, 1 insertion(+)

diff --git a/src/security/apparmor/libvirt-qemu b/src/security/apparmor/libvirt-qemu
index a03e9e2c94..85c9e61d6c 100644
--- a/src/security/apparmor/libvirt-qemu
+++ b/src/security/apparmor/libvirt-qemu
@@ -102,6 +102,7 @@
 
   # the various binaries
   /usr/bin/kvm rmix,
+  /usr/bin/kvm-spice rmix,
   /usr/bin/qemu rmix,
   /usr/bin/qemu-aarch64 rmix,
   /usr/bin/qemu-alpha rmix,
-- 
2.29.2

Re: [PATCH] apparmor: allow kvm-spice compat wrapper
Posted by Michal Privoznik 3 years, 4 months ago
On 11/16/20 1:26 PM, Christian Ehrhardt wrote:
> 'kvm-spice' is a binary name used to call 'kvm' which actually is a wrapper
> around qemu-system-x86_64 enabling kvm acceleration. This isn't in use
> for quite a while anymore, but required to work for compatibility e.g.
> when migrating in old guests.
> 
> For years this was a symlink kvm-spice->kvm and therefore covered
> apparmor-wise by the existing entry:
>     /usr/bin/kvm rmix,
> But due to a recent change [1] in qemu packaging this now is no symlink,
> but a wrapper on its own and therefore needs an own entry that allows it
> to be executed.
> 
> [1]: https://salsa.debian.org/qemu-team/qemu/-/commit/9944836d3
> 
> Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com>
> ---
>   src/security/apparmor/libvirt-qemu | 1 +
>   1 file changed, 1 insertion(+)
> 

Reviewed-by: Michal Privoznik <mprivozn@redhat.com>

Michal

Re: [PATCH] apparmor: allow kvm-spice compat wrapper
Posted by Christian Ehrhardt 3 years, 4 months ago
On Mon, Nov 16, 2020 at 3:28 PM Michal Privoznik <mprivozn@redhat.com> wrote:
>
> On 11/16/20 1:26 PM, Christian Ehrhardt wrote:
> > 'kvm-spice' is a binary name used to call 'kvm' which actually is a wrapper
> > around qemu-system-x86_64 enabling kvm acceleration. This isn't in use
> > for quite a while anymore, but required to work for compatibility e.g.
> > when migrating in old guests.
> >
> > For years this was a symlink kvm-spice->kvm and therefore covered
> > apparmor-wise by the existing entry:
> >     /usr/bin/kvm rmix,
> > But due to a recent change [1] in qemu packaging this now is no symlink,
> > but a wrapper on its own and therefore needs an own entry that allows it
> > to be executed.
> >
> > [1]: https://salsa.debian.org/qemu-team/qemu/-/commit/9944836d3
> >
> > Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com>
> > ---
> >   src/security/apparmor/libvirt-qemu | 1 +
> >   1 file changed, 1 insertion(+)
> >
>
> Reviewed-by: Michal Privoznik <mprivozn@redhat.com>

Thank you Michal,
it also passed fine through my tests (as backport to 6.8 and 6.9).
We are not in any freeze, review has happened, tests LGTM - pushed to git.

> Michal
>


-- 
Christian Ehrhardt
Staff Engineer, Ubuntu Server
Canonical Ltd

Re: [PATCH] apparmor: allow kvm-spice compat wrapper
Posted by Neal Gompa 3 years, 4 months ago
On Tue, Nov 17, 2020 at 11:49 AM Christian Ehrhardt
<christian.ehrhardt@canonical.com> wrote:
>
> On Mon, Nov 16, 2020 at 3:28 PM Michal Privoznik <mprivozn@redhat.com> wrote:
> >
> > On 11/16/20 1:26 PM, Christian Ehrhardt wrote:
> > > 'kvm-spice' is a binary name used to call 'kvm' which actually is a wrapper
> > > around qemu-system-x86_64 enabling kvm acceleration. This isn't in use
> > > for quite a while anymore, but required to work for compatibility e.g.
> > > when migrating in old guests.
> > >
> > > For years this was a symlink kvm-spice->kvm and therefore covered
> > > apparmor-wise by the existing entry:
> > >     /usr/bin/kvm rmix,
> > > But due to a recent change [1] in qemu packaging this now is no symlink,
> > > but a wrapper on its own and therefore needs an own entry that allows it
> > > to be executed.
> > >
> > > [1]: https://salsa.debian.org/qemu-team/qemu/-/commit/9944836d3
> > >
> > > Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com>
> > > ---
> > >   src/security/apparmor/libvirt-qemu | 1 +
> > >   1 file changed, 1 insertion(+)
> > >
> >
> > Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
>
> Thank you Michal,
> it also passed fine through my tests (as backport to 6.8 and 6.9).
> We are not in any freeze, review has happened, tests LGTM - pushed to git.
>

Hold up, why was this merged? Did anyone validate whether this would
break the other AppArmor user (SUSE)?

Unlike SELinux, AppArmor functionality is quite fragmented between
Ubuntu and SUSE distributions (the two major users of AppArmor), and
there did not seem to be any indication that this AppArmor patch was
validated with openSUSE before merging. My personal experience with
AppArmor across the two distribution families is that it's really easy
to make profiles that work for Ubuntu but fail on SUSE because of the
disparity of functionality. I also don't see Jim Fehlig stepping in to
indicate that this worked for him.

I haven't had a chance to test this myself, but I am immediately
suspicious of a change that references a commit based on Debian
packaging of QEMU.


-- 
真実はいつも一つ!/ Always, there's only one truth!


Re: [PATCH] apparmor: allow kvm-spice compat wrapper
Posted by Daniel P. Berrangé 3 years, 4 months ago
On Tue, Nov 17, 2020 at 09:11:48PM -0500, Neal Gompa wrote:
> On Tue, Nov 17, 2020 at 11:49 AM Christian Ehrhardt
> <christian.ehrhardt@canonical.com> wrote:
> >
> > On Mon, Nov 16, 2020 at 3:28 PM Michal Privoznik <mprivozn@redhat.com> wrote:
> > >
> > > On 11/16/20 1:26 PM, Christian Ehrhardt wrote:
> > > > 'kvm-spice' is a binary name used to call 'kvm' which actually is a wrapper
> > > > around qemu-system-x86_64 enabling kvm acceleration. This isn't in use
> > > > for quite a while anymore, but required to work for compatibility e.g.
> > > > when migrating in old guests.
> > > >
> > > > For years this was a symlink kvm-spice->kvm and therefore covered
> > > > apparmor-wise by the existing entry:
> > > >     /usr/bin/kvm rmix,
> > > > But due to a recent change [1] in qemu packaging this now is no symlink,
> > > > but a wrapper on its own and therefore needs an own entry that allows it
> > > > to be executed.
> > > >
> > > > [1]: https://salsa.debian.org/qemu-team/qemu/-/commit/9944836d3
> > > >
> > > > Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com>
> > > > ---
> > > >   src/security/apparmor/libvirt-qemu | 1 +
> > > >   1 file changed, 1 insertion(+)
> > > >
> > >
> > > Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
> >
> > Thank you Michal,
> > it also passed fine through my tests (as backport to 6.8 and 6.9).
> > We are not in any freeze, review has happened, tests LGTM - pushed to git.
> >
> 
> Hold up, why was this merged? Did anyone validate whether this would
> break the other AppArmor user (SUSE)?
> 
> Unlike SELinux, AppArmor functionality is quite fragmented between
> Ubuntu and SUSE distributions (the two major users of AppArmor), and
> there did not seem to be any indication that this AppArmor patch was
> validated with openSUSE before merging. My personal experience with
> AppArmor across the two distribution families is that it's really easy
> to make profiles that work for Ubuntu but fail on SUSE because of the
> disparity of functionality. I also don't see Jim Fehlig stepping in to
> indicate that this worked for him.
> 
> I haven't had a chance to test this myself, but I am immediately
> suspicious of a change that references a commit based on Debian
> packaging of QEMU.

Historically the AppArmor policy in libvirt has been exclusively
maintained and tested by the Debian and Ubuntu maintainers. We have
never considered SUSE in any changes made to it.

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: [PATCH] apparmor: allow kvm-spice compat wrapper
Posted by Christian Ehrhardt 3 years, 4 months ago
On Wed, Nov 18, 2020 at 10:38 AM Daniel P. Berrangé <berrange@redhat.com> wrote:
>
> On Tue, Nov 17, 2020 at 09:11:48PM -0500, Neal Gompa wrote:
> > On Tue, Nov 17, 2020 at 11:49 AM Christian Ehrhardt
> > <christian.ehrhardt@canonical.com> wrote:
> > >
> > > On Mon, Nov 16, 2020 at 3:28 PM Michal Privoznik <mprivozn@redhat.com> wrote:
> > > >
> > > > On 11/16/20 1:26 PM, Christian Ehrhardt wrote:
> > > > > 'kvm-spice' is a binary name used to call 'kvm' which actually is a wrapper
> > > > > around qemu-system-x86_64 enabling kvm acceleration. This isn't in use
> > > > > for quite a while anymore, but required to work for compatibility e.g.
> > > > > when migrating in old guests.
> > > > >
> > > > > For years this was a symlink kvm-spice->kvm and therefore covered
> > > > > apparmor-wise by the existing entry:
> > > > >     /usr/bin/kvm rmix,
> > > > > But due to a recent change [1] in qemu packaging this now is no symlink,
> > > > > but a wrapper on its own and therefore needs an own entry that allows it
> > > > > to be executed.
> > > > >
> > > > > [1]: https://salsa.debian.org/qemu-team/qemu/-/commit/9944836d3
> > > > >
> > > > > Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com>
> > > > > ---
> > > > >   src/security/apparmor/libvirt-qemu | 1 +
> > > > >   1 file changed, 1 insertion(+)
> > > > >
> > > >
> > > > Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
> > >
> > > Thank you Michal,
> > > it also passed fine through my tests (as backport to 6.8 and 6.9).
> > > We are not in any freeze, review has happened, tests LGTM - pushed to git.
> > >
> >
> > Hold up, why was this merged? Did anyone validate whether this would
> > break the other AppArmor user (SUSE)?
> >
> > Unlike SELinux, AppArmor functionality is quite fragmented between
> > Ubuntu and SUSE distributions (the two major users of AppArmor), and
> > there did not seem to be any indication that this AppArmor patch was
> > validated with openSUSE before merging. My personal experience with
> > AppArmor across the two distribution families is that it's really easy
> > to make profiles that work for Ubuntu but fail on SUSE because of the
> > disparity of functionality. I also don't see Jim Fehlig stepping in to
> > indicate that this worked for him.
> >
> > I haven't had a chance to test this myself, but I am immediately
> > suspicious of a change that references a commit based on Debian
> > packaging of QEMU.
>
> Historically the AppArmor policy in libvirt has been exclusively
> maintained and tested by the Debian and Ubuntu maintainers. We have
> never considered SUSE in any changes made to it.

Ack to what Daniel wrote.
In addition neither are other - be it Suse or 3rd party - changes
gated on Debian/Ubuntu testing them.
If I fail to catch the changes on the ML-discussion as part of staring
at my inbox, then the testing for us happens whenever we merge a new
upstream version.

The general rule of thumb that we not-strictly followed in recent times aret:
- logical changes e.g. to virt-aa-helper will have a build time
self-test associated
- labelling changes (related to hot add for example) are usually up
for discussion a
  bit longer and tested by the author in the context that the issues were found
- rule allow-additions (like the one here) are discussed and added if
there are no security concerns

I don't remember we've made anything more restrictive recently, that
would probably be somewhere between the latter two points above.

The duration also depends on the complexity - in this particular case
as Michal already stated there isn't a high chance of breaking
something with it.

OTOH I'm fine to work out something more official/established if you
want to define something - but keep in mind that many of us do this as
a fraction of a part of their duties. Due to that sometimes
human/machine time isn't available for short round trip times which
are needed for a formal gating process.

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

-- 
Christian Ehrhardt
Staff Engineer, Ubuntu Server
Canonical Ltd


Re: [PATCH] apparmor: allow kvm-spice compat wrapper
Posted by Michal Privoznik 3 years, 4 months ago
On 11/18/20 3:11 AM, Neal Gompa wrote:
> On Tue, Nov 17, 2020 at 11:49 AM Christian Ehrhardt
> <christian.ehrhardt@canonical.com> wrote:
>>
>> On Mon, Nov 16, 2020 at 3:28 PM Michal Privoznik <mprivozn@redhat.com> wrote:
>>>
>>> On 11/16/20 1:26 PM, Christian Ehrhardt wrote:
>>>> 'kvm-spice' is a binary name used to call 'kvm' which actually is a wrapper
>>>> around qemu-system-x86_64 enabling kvm acceleration. This isn't in use
>>>> for quite a while anymore, but required to work for compatibility e.g.
>>>> when migrating in old guests.
>>>>
>>>> For years this was a symlink kvm-spice->kvm and therefore covered
>>>> apparmor-wise by the existing entry:
>>>>      /usr/bin/kvm rmix,
>>>> But due to a recent change [1] in qemu packaging this now is no symlink,
>>>> but a wrapper on its own and therefore needs an own entry that allows it
>>>> to be executed.
>>>>
>>>> [1]: https://salsa.debian.org/qemu-team/qemu/-/commit/9944836d3
>>>>
>>>> Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com>
>>>> ---
>>>>    src/security/apparmor/libvirt-qemu | 1 +
>>>>    1 file changed, 1 insertion(+)
>>>>
>>>
>>> Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
>>
>> Thank you Michal,
>> it also passed fine through my tests (as backport to 6.8 and 6.9).
>> We are not in any freeze, review has happened, tests LGTM - pushed to git.
>>
> 
> Hold up, why was this merged? Did anyone validate whether this would
> break the other AppArmor user (SUSE)?
> 
> Unlike SELinux, AppArmor functionality is quite fragmented between
> Ubuntu and SUSE distributions (the two major users of AppArmor), and
> there did not seem to be any indication that this AppArmor patch was
> validated with openSUSE before merging. My personal experience with
> AppArmor across the two distribution families is that it's really easy
> to make profiles that work for Ubuntu but fail on SUSE because of the
> disparity of functionality. I also don't see Jim Fehlig stepping in to
> indicate that this worked for him.
> 
> I haven't had a chance to test this myself, but I am immediately
> suspicious of a change that references a commit based on Debian
> packaging of QEMU.
> 
> 

Maybe I'm misunderstanding something, but does this have a potential of 
breaking something? It's only allowing one binary more that can be executed.

Michal

Re: [PATCH] apparmor: allow kvm-spice compat wrapper
Posted by Jamie Strandboge 3 years, 4 months ago
On Tue, 17 Nov 2020, Neal Gompa wrote:

> On Tue, Nov 17, 2020 at 11:49 AM Christian Ehrhardt
> <christian.ehrhardt@canonical.com> wrote:
> >
> > On Mon, Nov 16, 2020 at 3:28 PM Michal Privoznik <mprivozn@redhat.com> wrote:
> > >
> > > On 11/16/20 1:26 PM, Christian Ehrhardt wrote:
> > > > 'kvm-spice' is a binary name used to call 'kvm' which actually is a wrapper
> > > > around qemu-system-x86_64 enabling kvm acceleration. This isn't in use
> > > > for quite a while anymore, but required to work for compatibility e.g.
> > > > when migrating in old guests.
> > > >
> > > > For years this was a symlink kvm-spice->kvm and therefore covered
> > > > apparmor-wise by the existing entry:
> > > >     /usr/bin/kvm rmix,
> > > > But due to a recent change [1] in qemu packaging this now is no symlink,
> > > > but a wrapper on its own and therefore needs an own entry that allows it
> > > > to be executed.
> > > >
> > > > [1]: https://salsa.debian.org/qemu-team/qemu/-/commit/9944836d3
> > > >
> > > > Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com>
> > > > ---
> > > >   src/security/apparmor/libvirt-qemu | 1 +
> > > >   1 file changed, 1 insertion(+)
> > > >
> > >
> > > Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
> >
> > Thank you Michal,
> > it also passed fine through my tests (as backport to 6.8 and 6.9).
> > We are not in any freeze, review has happened, tests LGTM - pushed to git.
> >
> 
> Hold up, why was this merged? Did anyone validate whether this would
> break the other AppArmor user (SUSE)?
> 
> Unlike SELinux, AppArmor functionality is quite fragmented between
> Ubuntu and SUSE distributions (the two major users of AppArmor), and
> there did not seem to be any indication that this AppArmor patch was
> validated with openSUSE before merging. My personal experience with
> AppArmor across the two distribution families is that it's really easy
> to make profiles that work for Ubuntu but fail on SUSE because of the
> disparity of functionality. I also don't see Jim Fehlig stepping in to
> indicate that this worked for him.
> 
> I haven't had a chance to test this myself, but I am immediately
> suspicious of a change that references a commit based on Debian
> packaging of QEMU.

Others have referred to how this list handles SUSE policies, but I'll
point out that the request was for a simple file rule that only adds
additional access. This should be no problem at all on SUSE.

Outside of this rule, the apparmor userspace understands kernel
differences and various rules and any modern SUSE would have a new
enough parser to handle the various rules syntax we use in the current
libvirt policy and be parseable without issues. The typical distro
pattern for new rule syntax would be that when a distro pulled in a new
libvirt with new policy syntax that the distro doesn't support, then it
would be abundantly clear to the distro maintainer when the parser
failed and the distro would either choose to upgrade apparmor or patch
out the problematic rules.

Hope this helps

-- 
Jamie Strandboge             | http://www.canonical.com

Re: [PATCH] apparmor: allow kvm-spice compat wrapper
Posted by Jim Fehlig 3 years, 4 months ago
On 11/17/20 7:11 PM, Neal Gompa wrote:
> On Tue, Nov 17, 2020 at 11:49 AM Christian Ehrhardt
> <christian.ehrhardt@canonical.com> wrote:
>>
>> On Mon, Nov 16, 2020 at 3:28 PM Michal Privoznik <mprivozn@redhat.com> wrote:
>>>
>>> On 11/16/20 1:26 PM, Christian Ehrhardt wrote:
>>>> 'kvm-spice' is a binary name used to call 'kvm' which actually is a wrapper
>>>> around qemu-system-x86_64 enabling kvm acceleration. This isn't in use
>>>> for quite a while anymore, but required to work for compatibility e.g.
>>>> when migrating in old guests.
>>>>
>>>> For years this was a symlink kvm-spice->kvm and therefore covered
>>>> apparmor-wise by the existing entry:
>>>>      /usr/bin/kvm rmix,
>>>> But due to a recent change [1] in qemu packaging this now is no symlink,
>>>> but a wrapper on its own and therefore needs an own entry that allows it
>>>> to be executed.
>>>>
>>>> [1]: https://salsa.debian.org/qemu-team/qemu/-/commit/9944836d3
>>>>
>>>> Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com>
>>>> ---
>>>>    src/security/apparmor/libvirt-qemu | 1 +
>>>>    1 file changed, 1 insertion(+)
>>>>
>>>
>>> Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
>>
>> Thank you Michal,
>> it also passed fine through my tests (as backport to 6.8 and 6.9).
>> We are not in any freeze, review has happened, tests LGTM - pushed to git.
>>
> 
> Hold up, why was this merged? Did anyone validate whether this would
> break the other AppArmor user (SUSE)?

Michal already mentioned it, but the change wouldn't break SUSE since it just 
allows execution of a qemu binary by yet another name. And AFAIK, SUSE never had 
a 'kvm-spice' qemu binary :-).

Regards,
Jim