[libvirt] [PATCH] qemu: Enforce qemuSecurity wrappers

Martin Kletzander mkletzan at redhat.com
Mon Feb 20 12:18:12 UTC 2017


On Mon, Feb 20, 2017 at 11:55:06AM +0000, Daniel P. Berrange wrote:
>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
>

Yeah, I was thinking about that too before, one of the ideas I had
before.  However, you need the bool to be there per-domain, not
per-driver, and I haven't found out how to automatically take care of
commit (and rollback).  You'd have to call that anyway.

>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/ :|
>
>--
>libvir-list mailing list
>libvir-list at redhat.com
>https://www.redhat.com/mailman/listinfo/libvir-list
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: Digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20170220/3798cd64/attachment-0001.sig>


More information about the libvir-list mailing list