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

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