[libvirt] [PATCH] qemu: Enforce qemuSecurity wrappers

[Michal Privoznik]

On 20.02.2017 12:55, 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

That would not fly. As Martin pointed out we need to use transactions on
per-domain basis. The decision whether to use transactions is based on a
boolean stored in the domain object and not the domain definition (well,
technically it's not a boolean rather than bitmap, but that's not the
point here).
I thought of registering a callback in the secdriver which would return
true/false depending whether transactions should be used. But that would
not fly either - secdriver takes domain def and not domain obj. So it
can only feed the callback with domain def which lacks the information
whether namespaces are used or not.

The other solution I can come up with is to use transactions
unconditionally - even for domains that don't run in a namespace. The
only drawback there is useless fork to enter the namespace we are
already in.

Michal