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@xxxxxxxxxx>
---
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
