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

On 04/14/2016 03:04 AM, 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(a)redhat.com&gt; >>>> --- >>>> 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. > > 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? > > I'll send a v2 with the rename. Laine was hitting issues with this too, so I pushed this patch. The API rename isn't blocking anyone

Thanks, Cole