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

On Fri, Apr 15, 2016 at 12:16:18PM -0400, Cole Robinson wrote: > 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 > v2 was alrady on the list, but it can be done the other way around. I'll send the rename rebased now so it's easier to review.