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

On 04/18/2016 02:34 AM, Martin Kletzander wrote: > 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. > Oh sorry, I must have missed it? Though looking through the archives I don't see anything, only the original patch and the v3. Maybe it didn't hit the list...

- Cole