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