[PATCH 2/3] security: Introduce virSecurityManagerDomainSetIncomingPathLabel

[Michal Privoznik]

On 4/17/20 12:57 PM, Erik Skultety wrote:
> On Fri, Apr 03, 2020 at 05:58:02PM +0200, Michal Privoznik wrote:
>> This API allows drivers to separate out handling of @stdin_path
>> of virSecurityManagerSetAllLabel(). The thing is, the QEMU driver
>> uses transactions for virSecurityManagerSetAllLabel() which
>> relabels devices from inside of domain's namespace. This is what
>> we usually want. Except when resuming domain from a file. The
>> file is opened before any namespace is set up and the FD is
>> passed to QEMU to read the migration stream from. Because of
>> this, the file lives outside of the namespace and if it so
>> happens that the file is a block device (i.e. it lives under
>> /dev) its copy will be created in the namespace. But the FD that
>> is passed to QEMU points to the original living in the host and
>> not in the namespace. So relabeling the file inside the namespace
>> helps nothing.
>>
>> But if we have a separate API for relabeling the restore file
>> then the QEMU driver can continue calling
>> virSecurityManagerSetAllLabel() with transactions enabled and
>> call this new API without transactions.
>>
>> We already have an API for relabeling a single file
>> (virSecurityManagerDomainSetPathLabel()) but in case of SELinux
>> it uses @imagelabel (which allows RW access) and we want to use
>> @content_context (which allows RO access).
> 
> Since this patch is only adding a generic driver API rather than a specific
> security backend API, I'd move ^this last paragraph to the next patch, I
> actually appreciated the info there much more than here during the review.
> 
>>
>> Signed-off-by: Michal Privoznik <mprivozn at redhat.com>
>> ---
>>   src/libvirt_private.syms        |  1 +
>>   src/security/security_driver.h  |  4 ++++
>>   src/security/security_manager.c | 29 +++++++++++++++++++++++++++++
>>   src/security/security_manager.h |  4 ++++
>>   src/security/security_stack.c   | 21 +++++++++++++++++++++
>>   5 files changed, 59 insertions(+)
>>
>> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
>> index e276f55bb1..2c63f37fc2 100644
>> --- a/src/libvirt_private.syms
>> +++ b/src/libvirt_private.syms
>> @@ -1523,6 +1523,7 @@ virSecurityDriverLookup;
>>   # security/security_manager.h
>>   virSecurityManagerCheckAllLabel;
>>   virSecurityManagerClearSocketLabel;
>> +virSecurityManagerDomainSetIncomingPathLabel;
> 
> IncomingPath sounds a bit confusing to me, I'd go with something like HostPath
> instead, in patch 3/3 you also have an explicit commentary why we're using the
> respective backend API for this rather than simply using SetPathLabel.

Yeah, naming is hard. Part of my reasoning was that if the name is not 
obvious people will not use it and default to 
virSecurityDomainSetPathLabel() which should be used as it guarantees RW 
access. But on the secdriver API level we don't distinguish transactions 
and thus host/namespaces. But if my reasoning was flawed and we might 
actually encourage people to use it? I mean, how about 
virSecurityDomainSetPathLabelRO() for the name, how does that sound?

We can leave the transactions aside and at hypervisor driver discretion 
because in fact, it's only QEMU who uses transactions.

> 
>>   virSecurityManagerDomainSetPathLabel;
>>   virSecurityManagerGenLabel;
>>   virSecurityManagerGetBaseLabel;
>> diff --git a/src/security/security_driver.h b/src/security/security_driver.h
>> index 3353955813..6cbe8613c9 100644
>> --- a/src/security/security_driver.h
>> +++ b/src/security/security_driver.h
>> @@ -140,6 +140,9 @@ typedef int (*virSecurityDomainSetPathLabel) (virSecurityManagerPtr mgr,
>>                                                 virDomainDefPtr def,
>>                                                 const char *path,
>>                                                 bool allowSubtree);
>> +typedef int (*virSecurityDomainSetIncomingPathLabel) (virSecurityManagerPtr mgr,
>> +                                                      virDomainDefPtr def,
>> +                                                      const char *path);
>>   typedef int (*virSecurityDomainSetChardevLabel) (virSecurityManagerPtr mgr,
>>                                                    virDomainDefPtr def,
>>                                                    virDomainChrSourceDefPtr dev_source,
>> @@ -211,6 +214,7 @@ struct _virSecurityDriver {
>>       virSecurityDriverGetBaseLabel getBaseLabel;
>>
>>       virSecurityDomainSetPathLabel domainSetPathLabel;
>> +    virSecurityDomainSetIncomingPathLabel domainSetIncomingPathLabel;
>>
>>       virSecurityDomainSetChardevLabel domainSetSecurityChardevLabel;
>>       virSecurityDomainRestoreChardevLabel domainRestoreSecurityChardevLabel;
>> diff --git a/src/security/security_manager.c b/src/security/security_manager.c
>> index fe032746ff..a76b953ee4 100644
>> --- a/src/security/security_manager.c
>> +++ b/src/security/security_manager.c
>> @@ -1077,6 +1077,35 @@ virSecurityManagerDomainSetPathLabel(virSecurityManagerPtr mgr,
>>   }
>>
>>
>> +/**
>> + * virSecurityManagerDomainSetIncomingPathLabel:
>> + * @mgr: security manager object
>> + * @vm: domain definition object
>> + * @path: path to label
>> + *
>> + * This function relabels given @path so that @vm can restore for
> 
> maybe add "host" @path here to make it even clearer.
> 
>> + * it.  This allows the driver backend to use different label than
>> + * virSecurityManagerDomainSetPathLabel().

This would then be:

This function relabels given @path for read only access, which is in 
contrast with virSecurityManagerDomainSetPathLabel() which gives read 
write access.

Michal