[libvirt] [PATCH] qemu: Enforce qemuSecurity wrappers

[Daniel P. Berrange]

On Mon, Feb 20, 2017 at 12:50:23PM +0100, Peter Krempa wrote:
> On Tue, Feb 14, 2017 at 15:30:44 +0100, Michal Privoznik wrote:
> > Now that we have some qemuSecurity wrappers over
> > virSecurityManager APIs, lets make sure everybody sticks with
> > them. We have them for a reason and calling virSecurityManager
> > API directly instead of wrapper may lead into accidentally
> > labelling a file on the host instead of namespace.
> > 
> > Signed-off-by: Michal Privoznik <mprivozn at redhat.com>
> > ---
> > 
> > This is an alternative approach to:
> > 
> > https://www.redhat.com/archives/libvir-list/2017-February/msg00271.html
> > 
> >  cfg.mk                    |  5 ++++
> >  src/qemu/qemu_command.c   |  7 +++---
> >  src/qemu/qemu_conf.c      |  9 ++++---
> >  src/qemu/qemu_domain.c    | 17 ++++++-------
> >  src/qemu/qemu_driver.c    | 63 ++++++++++++++++++++++-------------------------
> >  src/qemu/qemu_hotplug.c   |  4 +--
> >  src/qemu/qemu_migration.c | 13 +++++-----
> >  src/qemu/qemu_process.c   | 61 ++++++++++++++++++++++-----------------------
> >  src/qemu/qemu_security.h  | 32 ++++++++++++++++++++++++
> >  9 files changed, 122 insertions(+), 89 deletions(-)
> > 
> 
> [...]
> 
> > diff --git a/src/qemu/qemu_security.h b/src/qemu/qemu_security.h
> > index 54638908d..d86db3f6b 100644
> > --- a/src/qemu/qemu_security.h
> > +++ b/src/qemu/qemu_security.h
> > @@ -28,6 +28,7 @@
> >  
> >  # include "qemu_conf.h"
> >  # include "domain_conf.h"
> > +# include "security/security_manager.h"
> >  
> >  int qemuSecuritySetAllLabel(virQEMUDriverPtr driver,
> >                              virDomainObjPtr vm,
> > @@ -60,4 +61,35 @@ int qemuSecuritySetHostdevLabel(virQEMUDriverPtr driver,
> >  int qemuSecurityRestoreHostdevLabel(virQEMUDriverPtr driver,
> >                                      virDomainObjPtr vm,
> >                                      virDomainHostdevDefPtr hostdev);
> > +
> > +/* Please note that for these APIs there is no wrapper yet. Do NOT blindly add
> > + * new APIs here. If an API can touch a /dev file add a proper wrapper instead.
> > + */
> > +# define qemuSecurityCheckAllLabel virSecurityManagerCheckAllLabel
> > +# define qemuSecurityClearSocketLabel virSecurityManagerClearSocketLabel
> > +# define qemuSecurityDomainSetPathLabel virSecurityManagerDomainSetPathLabel
> > +# define qemuSecurityGenLabel virSecurityManagerGenLabel
> > +# define qemuSecurityGetBaseLabel virSecurityManagerGetBaseLabel
> > +# define qemuSecurityGetDOI virSecurityManagerGetDOI
> > +# define qemuSecurityGetModel virSecurityManagerGetModel
> > +# define qemuSecurityGetMountOptions virSecurityManagerGetMountOptions
> > +# define qemuSecurityGetNested virSecurityManagerGetNested
> > +# define qemuSecurityGetProcessLabel virSecurityManagerGetProcessLabel
> > +# define qemuSecurityNew virSecurityManagerNew
> > +# define qemuSecurityNewDAC virSecurityManagerNewDAC
> > +# define qemuSecurityNewStack virSecurityManagerNewStack
> > +# define qemuSecurityPostFork virSecurityManagerPostFork
> > +# define qemuSecurityPreFork virSecurityManagerPreFork
> > +# define qemuSecurityReleaseLabel virSecurityManagerReleaseLabel
> > +# define qemuSecurityReserveLabel virSecurityManagerReserveLabel
> > +# define qemuSecurityRestoreSavedStateLabel virSecurityManagerRestoreSavedStateLabel
> > +# define qemuSecuritySetChildProcessLabel virSecurityManagerSetChildProcessLabel
> > +# define qemuSecuritySetDaemonSocketLabel virSecurityManagerSetDaemonSocketLabel
> > +# define qemuSecuritySetImageFDLabel virSecurityManagerSetImageFDLabel
> > +# define qemuSecuritySetSavedStateLabel virSecurityManagerSetSavedStateLabel
> > +# define qemuSecuritySetSocketLabel virSecurityManagerSetSocketLabel
> > +# define qemuSecuritySetTapFDLabel virSecurityManagerSetTapFDLabel
> > +# define qemuSecurityStackAddNested virSecurityManagerStackAddNested
> > +# define qemuSecurityVerify virSecurityManagerVerify
> 
> I don't like this either for similar reasons that I've stated on the
> original series.

I think the interesting question here is whether we can figure out a
way to push the qemuSecurity* functionality into the security drivers
and thus remove the need for all these wrappers....

Currently all the wrappers look the same doing

  if (namespace enabled) {
     ...start transaction...
  }
  ... call real method...
  if (namespace enabled) {
     ...commit transaction...
  }

The key observation to me is that there is only ever a single method
called inside the transaction. That ought to mean we can push the
transaction functionality down a layer. eg in the virSecurityManagerNew*
methods we could pass in a "bool useTransactions" flag and then the
security_manager.c could take care of start/commit/rollback

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://entangle-photo.org       -o-    http://search.cpan.org/~danberr/ :|