[libvirt] [PATCH] qemu: Label master key file

[Guido Günther]

On Thu, Apr 14, 2016 at 09:04:31AM +0200, Martin Kletzander wrote:
> On Wed, Apr 13, 2016 at 07:58:06PM -0400, Cole Robinson wrote:
> >On 04/13/2016 11:56 AM, Cole Robinson wrote:
> >>On 04/13/2016 11:17 AM, Martin Kletzander wrote:
> >>>When creating the master key, we used mode 0600 (which we should) but
> >>>because we were creating it as root, the file is not readable by any
> >>>qemu running as non-root.  Fortunately, it's just a matter of labelling
> >>>the file.  We are generating the file path few times already, so let's
> >>>label it in the same function that has access to the path already.
> >>>
> >>>Signed-off-by: Martin Kletzander <mkletzan at redhat.com>
> >>>---
> >>> src/qemu/qemu_domain.c  | 15 ++++++++++++---
> >>> src/qemu/qemu_domain.h  |  3 ++-
> >>> src/qemu/qemu_process.c |  2 +-
> >>> 3 files changed, 15 insertions(+), 5 deletions(-)
> >>>
> >>
> >>ACK, makes sense and fixes things for me. One comment below
> >>
> >>>diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> >>>index 5d54fffcfb98..83e765ef6868 100644
> >>>--- a/src/qemu/qemu_domain.c
> >>>+++ b/src/qemu/qemu_domain.c
> >>>@@ -504,11 +504,13 @@ qemuDomainGetMasterKeyFilePath(const char *libDir)
> >>>  * Returns 0 on success, -1 on failure with error message indicating failure
> >>>  */
> >>> static int
> >>>-qemuDomainWriteMasterKeyFile(qemuDomainObjPrivatePtr priv)
> >>>+qemuDomainWriteMasterKeyFile(virQEMUDriverPtr driver,
> >>>+                             virDomainObjPtr vm)
> >>> {
> >>>     char *path;
> >>>     int fd = -1;
> >>>     int ret = -1;
> >>>+    qemuDomainObjPrivatePtr priv = vm->privateData;
> >>>
> >>>     if (!(path = qemuDomainGetMasterKeyFilePath(priv->libDir)))
> >>>         return -1;
> >>>@@ -525,6 +527,10 @@ qemuDomainWriteMasterKeyFile(qemuDomainObjPrivatePtr priv)
> >>>         goto cleanup;
> >>>     }
> >>>
> >>>+    if (virSecurityManagerDomainSetDirLabel(driver->securityManager,
> >>>+                                            vm->def, path) < 0)
> >>>+        goto cleanup;
> >>>+
> >>>     ret = 0;
> >>>
> >>
> >>I looked briefly at fixing this but know if there was a function to ask the
> >>security driver 'just set a on this arbitrary path'. I saw DirLabel but was
> >>thrown off by the 'Dir' name. Maybe change it to something more generic?
> >>
> 
> Yes, it's just a naming; I had to add it for similar purpose when
> labelling directories, but it "Just Works" for arbitrary path.  I'll
> rename that.
> 
> >
> >Also adding some CC, I'm guessing virt-aa-helper.c needs to be extended to to
> >allow access to $libDir/master-key.aes
> >
> 
> Actually, it shouldn't.  It's in the per-domain directory and
> everything under that should be available.

If it's in the per domain directory we should be fine.

> Anyway, it's a pity that we're not very likely to have a test case for
> that.  But couldn't the paths in virt-aa-helper be created from a
> security driver?  It knows all the paths, doesn't it?

This is already being done by various functions calling reload_profile
with a path to add to the profile (see
e.g. AppArmorRestoreSecurityImageLabel). However SetDirLabel currently
isn't wired up which could be done by adding a "-d directory" in
addition to "-f filename" to virt-aa-helper.

I'm currently happy if I'm able to keep the libvirt packages in Debian
in a reasonable shape so I won't have time to work on this.

Cheers
 -- Guido