[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH] qemu: Enforce qemuSecurity wrappers



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 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 redhat com
https://www.redhat.com/mailman/listinfo/libvir-list

Attachment: signature.asc
Description: Digital signature


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]