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

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(a)redhat.com&gt; >> --- >> >> 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