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