8:03 a.m.
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(a)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