[PATCH 2/3] security: Introduce virSecurityManagerDomainSetIncomingPathLabel

[Erik Skultety]

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.

>  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().
> + *
> + * Returns: 0 on success, -1 on error.
> + */
> +int
> +virSecurityManagerDomainSetIncomingPathLabel(virSecurityManagerPtr mgr,
> +                                             virDomainDefPtr vm,
> +                                             const char *path)
> +{
> +    if (mgr->drv->domainSetIncomingPathLabel) {
> +        int ret;
> +        virObjectLock(mgr);
> +        ret = mgr->drv->domainSetIncomingPathLabel(mgr, vm, path);
> +        virObjectUnlock(mgr);
> +        return ret;
> +    }
> +
> +    return 0;
> +}

Looks good and works:
Reviewed-by: Erik Skultety <eskultet at redhat.com>