Security labelling of disks consists of labelling of the disk image
itself and it's backing chain. Modify
virSecurityManager[Set|Restore]ImageLabel to take a boolean flag that
will label the full chain rather than the top image itself.
This allows to delete/unify some parts of the code and will also
simplify callers in some cases.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
---
src/qemu/qemu_security.c | 6 ++--
src/security/security_apparmor.c | 24 +++------------
src/security/security_dac.c | 40 +++++++------------------
src/security/security_driver.h | 15 +++-------
src/security/security_manager.c | 20 ++++++++-----
src/security/security_manager.h | 6 ++--
src/security/security_nop.c | 25 +++-------------
src/security/security_selinux.c | 42 ++++++++-------------------
src/security/security_stack.c | 50 +++++---------------------------
9 files changed, 60 insertions(+), 168 deletions(-)
diff --git a/src/qemu/qemu_security.c b/src/qemu/qemu_security.c
index 5faa34a4fd..4940195216 100644
--- a/src/qemu/qemu_security.c
+++ b/src/qemu/qemu_security.c
@@ -170,8 +170,7 @@ qemuSecuritySetImageLabel(virQEMUDriverPtr driver,
goto cleanup;
if (virSecurityManagerSetImageLabel(driver->securityManager,
- vm->def,
- src) < 0)
+ vm->def, src, false) < 0)
goto cleanup;
if (virSecurityManagerTransactionCommit(driver->securityManager,
@@ -201,8 +200,7 @@ qemuSecurityRestoreImageLabel(virQEMUDriverPtr driver,
goto cleanup;
if (virSecurityManagerRestoreImageLabel(driver->securityManager,
- vm->def,
- src) < 0)
+ vm->def, src, false) < 0)
goto cleanup;
if (virSecurityManagerTransactionCommit(driver->securityManager,
diff --git a/src/security/security_apparmor.c b/src/security/security_apparmor.c
index 43310361ba..a61105cbb7 100644
--- a/src/security/security_apparmor.c
+++ b/src/security/security_apparmor.c
@@ -691,7 +691,8 @@ AppArmorClearSecuritySocketLabel(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED,
static int
AppArmorRestoreSecurityImageLabel(virSecurityManagerPtr mgr,
virDomainDefPtr def,
- virStorageSourcePtr src)
+ virStorageSourcePtr src,
+ bool backingStore ATTRIBUTE_UNUSED)
{
if (!virStorageSourceIsLocalStorage(src))
return 0;
@@ -699,13 +700,6 @@ AppArmorRestoreSecurityImageLabel(virSecurityManagerPtr mgr,
return reload_profile(mgr, def, NULL, false);
}
-static int
-AppArmorRestoreSecurityDiskLabel(virSecurityManagerPtr mgr,
- virDomainDefPtr def,
- virDomainDiskDefPtr disk)
-{
- return AppArmorRestoreSecurityImageLabel(mgr, def, disk->src);
-}
/* Called when hotplugging */
static int
@@ -799,7 +793,8 @@ AppArmorRestoreInputLabel(virSecurityManagerPtr mgr,
static int
AppArmorSetSecurityImageLabel(virSecurityManagerPtr mgr,
virDomainDefPtr def,
- virStorageSourcePtr src)
+ virStorageSourcePtr src,
+ bool backingStore ATTRIBUTE_UNUSED)
{
int rc = -1;
char *profile_name = NULL;
@@ -844,14 +839,6 @@ AppArmorSetSecurityImageLabel(virSecurityManagerPtr mgr,
return rc;
}
-static int
-AppArmorSetSecurityDiskLabel(virSecurityManagerPtr mgr,
- virDomainDefPtr def,
- virDomainDiskDefPtr disk)
-{
- return AppArmorSetSecurityImageLabel(mgr, def, disk->src);
-}
-
static int
AppArmorSecurityVerify(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED,
virDomainDefPtr def)
@@ -1188,9 +1175,6 @@ virSecurityDriver virAppArmorSecurityDriver = {
.domainSecurityVerify = AppArmorSecurityVerify,
- .domainSetSecurityDiskLabel = AppArmorSetSecurityDiskLabel,
- .domainRestoreSecurityDiskLabel = AppArmorRestoreSecurityDiskLabel,
-
.domainSetSecurityImageLabel = AppArmorSetSecurityImageLabel,
.domainRestoreSecurityImageLabel = AppArmorRestoreSecurityImageLabel,
diff --git a/src/security/security_dac.c b/src/security/security_dac.c
index 533d990de1..08ff0d89c0 100644
--- a/src/security/security_dac.c
+++ b/src/security/security_dac.c
@@ -897,22 +897,17 @@ virSecurityDACSetImageLabelInternal(virSecurityManagerPtr mgr,
static int
virSecurityDACSetImageLabel(virSecurityManagerPtr mgr,
virDomainDefPtr def,
- virStorageSourcePtr src)
+ virStorageSourcePtr src,
+ bool backingChain)
{
- return virSecurityDACSetImageLabelInternal(mgr, def, src, NULL);
-}
-
-static int
-virSecurityDACSetDiskLabel(virSecurityManagerPtr mgr,
- virDomainDefPtr def,
- virDomainDiskDefPtr disk)
+ virStorageSourcePtr n;
-{
- virStorageSourcePtr next;
-
- for (next = disk->src; virStorageSourceIsBacking(next); next = next->backingStore) {
- if (virSecurityDACSetImageLabelInternal(mgr, def, next, disk->src) < 0)
+ for (n = src; virStorageSourceIsBacking(n); n = n->backingStore) {
+ if (virSecurityDACSetImageLabelInternal(mgr, def, n, src) < 0)
return -1;
+
+ if (!backingChain)
+ break;
}
return 0;
@@ -969,21 +964,13 @@ virSecurityDACRestoreImageLabelInt(virSecurityManagerPtr mgr,
static int
virSecurityDACRestoreImageLabel(virSecurityManagerPtr mgr,
virDomainDefPtr def,
- virStorageSourcePtr src)
+ virStorageSourcePtr src,
+ bool backingChain ATTRIBUTE_UNUSED)
{
return virSecurityDACRestoreImageLabelInt(mgr, def, src, false);
}
-static int
-virSecurityDACRestoreDiskLabel(virSecurityManagerPtr mgr,
- virDomainDefPtr def,
- virDomainDiskDefPtr disk)
-{
- return virSecurityDACRestoreImageLabelInt(mgr, def, disk->src, false);
-}
-
-
static int
virSecurityDACSetHostdevLabelHelper(const char *file,
void *opaque)
@@ -1853,9 +1840,7 @@ virSecurityDACSetAllLabel(virSecurityManagerPtr mgr,
/* XXX fixme - we need to recursively label the entire tree :-( */
if (virDomainDiskGetType(def->disks[i]) == VIR_STORAGE_TYPE_DIR)
continue;
- if (virSecurityDACSetDiskLabel(mgr,
- def,
- def->disks[i]) < 0)
+ if (virSecurityDACSetImageLabel(mgr, def, def->disks[i]->src, true) < 0)
return -1;
}
@@ -2295,9 +2280,6 @@ virSecurityDriver virSecurityDriverDAC = {
.domainSecurityVerify = virSecurityDACVerify,
- .domainSetSecurityDiskLabel = virSecurityDACSetDiskLabel,
- .domainRestoreSecurityDiskLabel = virSecurityDACRestoreDiskLabel,
-
.domainSetSecurityImageLabel = virSecurityDACSetImageLabel,
.domainRestoreSecurityImageLabel = virSecurityDACRestoreImageLabel,
diff --git a/src/security/security_driver.h b/src/security/security_driver.h
index 70c8cde50b..df270cdc02 100644
--- a/src/security/security_driver.h
+++ b/src/security/security_driver.h
@@ -54,18 +54,12 @@ typedef int (*virSecurityDriverTransactionCommit) (virSecurityManagerPtr mgr,
bool lock);
typedef void (*virSecurityDriverTransactionAbort) (virSecurityManagerPtr mgr);
-typedef int (*virSecurityDomainRestoreDiskLabel) (virSecurityManagerPtr mgr,
- virDomainDefPtr def,
- virDomainDiskDefPtr disk);
typedef int (*virSecurityDomainSetDaemonSocketLabel)(virSecurityManagerPtr mgr,
virDomainDefPtr vm);
typedef int (*virSecurityDomainSetSocketLabel) (virSecurityManagerPtr mgr,
virDomainDefPtr def);
typedef int (*virSecurityDomainClearSocketLabel)(virSecurityManagerPtr mgr,
virDomainDefPtr def);
-typedef int (*virSecurityDomainSetDiskLabel) (virSecurityManagerPtr mgr,
- virDomainDefPtr def,
- virDomainDiskDefPtr disk);
typedef int (*virSecurityDomainRestoreHostdevLabel) (virSecurityManagerPtr mgr,
virDomainDefPtr def,
virDomainHostdevDefPtr dev,
@@ -119,10 +113,12 @@ typedef int (*virSecurityDomainSetHugepages) (virSecurityManagerPtr mgr,
const char *path);
typedef int (*virSecurityDomainSetImageLabel) (virSecurityManagerPtr mgr,
virDomainDefPtr def,
- virStorageSourcePtr src);
+ virStorageSourcePtr src,
+ bool backingChain);
typedef int (*virSecurityDomainRestoreImageLabel) (virSecurityManagerPtr mgr,
virDomainDefPtr def,
- virStorageSourcePtr src);
+ virStorageSourcePtr src,
+ bool backingChain);
typedef int (*virSecurityDomainSetMemoryLabel) (virSecurityManagerPtr mgr,
virDomainDefPtr def,
virDomainMemoryDefPtr mem);
@@ -171,9 +167,6 @@ struct _virSecurityDriver {
virSecurityDomainSecurityVerify domainSecurityVerify;
- virSecurityDomainSetDiskLabel domainSetSecurityDiskLabel;
- virSecurityDomainRestoreDiskLabel domainRestoreSecurityDiskLabel;
-
virSecurityDomainSetImageLabel domainSetSecurityImageLabel;
virSecurityDomainRestoreImageLabel domainRestoreSecurityImageLabel;
diff --git a/src/security/security_manager.c b/src/security/security_manager.c
index f6b4c2d5d5..5493f0f66b 100644
--- a/src/security/security_manager.c
+++ b/src/security/security_manager.c
@@ -418,10 +418,10 @@ virSecurityManagerRestoreDiskLabel(virSecurityManagerPtr mgr,
virDomainDefPtr vm,
virDomainDiskDefPtr disk)
{
- if (mgr->drv->domainRestoreSecurityDiskLabel) {
+ if (mgr->drv->domainRestoreSecurityImageLabel) {
int ret;
virObjectLock(mgr);
- ret = mgr->drv->domainRestoreSecurityDiskLabel(mgr, vm, disk);
+ ret = mgr->drv->domainRestoreSecurityImageLabel(mgr, vm, disk->src, true);
virObjectUnlock(mgr);
return ret;
}
@@ -436,6 +436,7 @@ virSecurityManagerRestoreDiskLabel(virSecurityManagerPtr mgr,
* @mgr: security manager object
* @vm: domain definition object
* @src: disk source definition to operate on
+ * @backingChain: Restore labels also on backingChains of @src
*
* Removes security label from a single storage image.
*
@@ -444,12 +445,13 @@ virSecurityManagerRestoreDiskLabel(virSecurityManagerPtr mgr,
int
virSecurityManagerRestoreImageLabel(virSecurityManagerPtr mgr,
virDomainDefPtr vm,
- virStorageSourcePtr src)
+ virStorageSourcePtr src,
+ bool backingChain)
{
if (mgr->drv->domainRestoreSecurityImageLabel) {
int ret;
virObjectLock(mgr);
- ret = mgr->drv->domainRestoreSecurityImageLabel(mgr, vm, src);
+ ret = mgr->drv->domainRestoreSecurityImageLabel(mgr, vm, src, backingChain);
virObjectUnlock(mgr);
return ret;
}
@@ -526,10 +528,10 @@ virSecurityManagerSetDiskLabel(virSecurityManagerPtr mgr,
virDomainDefPtr vm,
virDomainDiskDefPtr disk)
{
- if (mgr->drv->domainSetSecurityDiskLabel) {
+ if (mgr->drv->domainSetSecurityImageLabel) {
int ret;
virObjectLock(mgr);
- ret = mgr->drv->domainSetSecurityDiskLabel(mgr, vm, disk);
+ ret = mgr->drv->domainSetSecurityImageLabel(mgr, vm, disk->src, true);
virObjectUnlock(mgr);
return ret;
}
@@ -544,6 +546,7 @@ virSecurityManagerSetDiskLabel(virSecurityManagerPtr mgr,
* @mgr: security manager object
* @vm: domain definition object
* @src: disk source definition to operate on
+ * @backingChain: set labels also on backing chain of @src
*
* Labels a single storage image with the configured security label.
*
@@ -552,12 +555,13 @@ virSecurityManagerSetDiskLabel(virSecurityManagerPtr mgr,
int
virSecurityManagerSetImageLabel(virSecurityManagerPtr mgr,
virDomainDefPtr vm,
- virStorageSourcePtr src)
+ virStorageSourcePtr src,
+ bool backingChain)
{
if (mgr->drv->domainSetSecurityImageLabel) {
int ret;
virObjectLock(mgr);
- ret = mgr->drv->domainSetSecurityImageLabel(mgr, vm, src);
+ ret = mgr->drv->domainSetSecurityImageLabel(mgr, vm, src, backingChain);
virObjectUnlock(mgr);
return ret;
}
diff --git a/src/security/security_manager.h b/src/security/security_manager.h
index f7beb29f86..0207113b14 100644
--- a/src/security/security_manager.h
+++ b/src/security/security_manager.h
@@ -156,10 +156,12 @@ virSecurityManagerPtr* virSecurityManagerGetNested(virSecurityManagerPtr mgr);
int virSecurityManagerSetImageLabel(virSecurityManagerPtr mgr,
virDomainDefPtr vm,
- virStorageSourcePtr src);
+ virStorageSourcePtr src,
+ bool backingChain);
int virSecurityManagerRestoreImageLabel(virSecurityManagerPtr mgr,
virDomainDefPtr vm,
- virStorageSourcePtr src);
+ virStorageSourcePtr src,
+ bool backingChain);
int virSecurityManagerSetMemoryLabel(virSecurityManagerPtr mgr,
virDomainDefPtr vm,
diff --git a/src/security/security_nop.c b/src/security/security_nop.c
index ff739f8199..21e668c169 100644
--- a/src/security/security_nop.c
+++ b/src/security/security_nop.c
@@ -55,14 +55,6 @@ virSecurityDriverGetDOINop(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED)
return "0";
}
-static int
-virSecurityDomainRestoreDiskLabelNop(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED,
- virDomainDefPtr vm ATTRIBUTE_UNUSED,
- virDomainDiskDefPtr disk ATTRIBUTE_UNUSED)
-{
- return 0;
-}
-
static int
virSecurityDomainSetDaemonSocketLabelNop(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED,
virDomainDefPtr vm ATTRIBUTE_UNUSED)
@@ -84,14 +76,6 @@ virSecurityDomainClearSocketLabelNop(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED,
return 0;
}
-static int
-virSecurityDomainSetDiskLabelNop(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED,
- virDomainDefPtr vm ATTRIBUTE_UNUSED,
- virDomainDiskDefPtr disk ATTRIBUTE_UNUSED)
-{
- return 0;
-}
-
static int
virSecurityDomainRestoreHostdevLabelNop(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED,
virDomainDefPtr vm ATTRIBUTE_UNUSED,
@@ -225,7 +209,8 @@ virSecurityGetBaseLabel(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED,
static int
virSecurityDomainRestoreImageLabelNop(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED,
virDomainDefPtr def ATTRIBUTE_UNUSED,
- virStorageSourcePtr src ATTRIBUTE_UNUSED)
+ virStorageSourcePtr src ATTRIBUTE_UNUSED,
+ bool backingChain ATTRIBUTE_UNUSED)
{
return 0;
}
@@ -233,7 +218,8 @@ virSecurityDomainRestoreImageLabelNop(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED
static int
virSecurityDomainSetImageLabelNop(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED,
virDomainDefPtr def ATTRIBUTE_UNUSED,
- virStorageSourcePtr src ATTRIBUTE_UNUSED)
+ virStorageSourcePtr src ATTRIBUTE_UNUSED,
+ bool backingChain ATTRIBUTE_UNUSED)
{
return 0;
}
@@ -292,9 +278,6 @@ virSecurityDriver virSecurityDriverNop = {
.domainSecurityVerify = virSecurityDomainVerifyNop,
- .domainSetSecurityDiskLabel = virSecurityDomainSetDiskLabelNop,
- .domainRestoreSecurityDiskLabel = virSecurityDomainRestoreDiskLabelNop,
-
.domainSetSecurityImageLabel = virSecurityDomainSetImageLabelNop,
.domainRestoreSecurityImageLabel = virSecurityDomainRestoreImageLabelNop,
diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c
index 5cdb839c13..106494ff3a 100644
--- a/src/security/security_selinux.c
+++ b/src/security/security_selinux.c
@@ -1771,20 +1771,11 @@ virSecuritySELinuxRestoreImageLabelInt(virSecurityManagerPtr mgr,
}
-static int
-virSecuritySELinuxRestoreDiskLabel(virSecurityManagerPtr mgr,
- virDomainDefPtr def,
- virDomainDiskDefPtr disk)
-{
- return virSecuritySELinuxRestoreImageLabelInt(mgr, def, disk->src,
- false);
-}
-
-
static int
virSecuritySELinuxRestoreImageLabel(virSecurityManagerPtr mgr,
virDomainDefPtr def,
- virStorageSourcePtr src)
+ virStorageSourcePtr src,
+ bool backingChain ATTRIBUTE_UNUSED)
{
return virSecuritySELinuxRestoreImageLabelInt(mgr, def, src, false);
}
@@ -1869,28 +1860,23 @@ virSecuritySELinuxSetImageLabelInternal(virSecurityManagerPtr mgr,
static int
virSecuritySELinuxSetImageLabel(virSecurityManagerPtr mgr,
virDomainDefPtr def,
- virStorageSourcePtr src)
-{
- return virSecuritySELinuxSetImageLabelInternal(mgr, def, src, NULL);
-}
-
-
-static int
-virSecuritySELinuxSetDiskLabel(virSecurityManagerPtr mgr,
- virDomainDefPtr def,
- virDomainDiskDefPtr disk)
-
+ virStorageSourcePtr src,
+ bool backingChain)
{
- virStorageSourcePtr next;
+ virStorageSourcePtr n;
- for (next = disk->src; virStorageSourceIsBacking(next); next = next->backingStore) {
- if (virSecuritySELinuxSetImageLabelInternal(mgr, def, next, disk->src) < 0)
+ for (n = src; virStorageSourceIsBacking(n); n = n->backingStore) {
+ if (virSecuritySELinuxSetImageLabelInternal(mgr, def, n, src) < 0)
return -1;
+
+ if (!backingChain)
+ break;
}
return 0;
}
+
static int
virSecuritySELinuxSetHostdevLabelHelper(const char *file, void *opaque)
{
@@ -3026,8 +3012,7 @@ virSecuritySELinuxSetAllLabel(virSecurityManagerPtr mgr,
def->disks[i]->dst);
continue;
}
- if (virSecuritySELinuxSetDiskLabel(mgr,
- def, def->disks[i]) < 0)
+ if (virSecuritySELinuxSetImageLabel(mgr, def, def->disks[i]->src, true) < 0)
return -1;
}
/* XXX fixme process def->fss if relabel == true */
@@ -3441,9 +3426,6 @@ virSecurityDriver virSecurityDriverSELinux = {
.domainSecurityVerify = virSecuritySELinuxVerify,
- .domainSetSecurityDiskLabel = virSecuritySELinuxSetDiskLabel,
- .domainRestoreSecurityDiskLabel = virSecuritySELinuxRestoreDiskLabel,
-
.domainSetSecurityImageLabel = virSecuritySELinuxSetImageLabel,
.domainRestoreSecurityImageLabel = virSecuritySELinuxRestoreImageLabel,
diff --git a/src/security/security_stack.c b/src/security/security_stack.c
index 3e60d5d2b7..e1c98a75e3 100644
--- a/src/security/security_stack.c
+++ b/src/security/security_stack.c
@@ -267,42 +267,6 @@ virSecurityStackReserveLabel(virSecurityManagerPtr mgr,
}
-static int
-virSecurityStackSetDiskLabel(virSecurityManagerPtr mgr,
- virDomainDefPtr vm,
- virDomainDiskDefPtr disk)
-{
- virSecurityStackDataPtr priv = virSecurityManagerGetPrivateData(mgr);
- virSecurityStackItemPtr item = priv->itemsHead;
- int rc = 0;
-
- for (; item; item = item->next) {
- if (virSecurityManagerSetDiskLabel(item->securityManager, vm, disk) < 0)
- rc = -1;
- }
-
- return rc;
-}
-
-
-static int
-virSecurityStackRestoreDiskLabel(virSecurityManagerPtr mgr,
- virDomainDefPtr vm,
- virDomainDiskDefPtr disk)
-{
- virSecurityStackDataPtr priv = virSecurityManagerGetPrivateData(mgr);
- virSecurityStackItemPtr item = priv->itemsHead;
- int rc = 0;
-
- for (; item; item = item->next) {
- if (virSecurityManagerRestoreDiskLabel(item->securityManager, vm, disk) < 0)
- rc = -1;
- }
-
- return rc;
-}
-
-
static int
virSecurityStackSetHostdevLabel(virSecurityManagerPtr mgr,
virDomainDefPtr vm,
@@ -600,14 +564,16 @@ virSecurityStackGetBaseLabel(virSecurityManagerPtr mgr, int virtType)
static int
virSecurityStackSetImageLabel(virSecurityManagerPtr mgr,
virDomainDefPtr vm,
- virStorageSourcePtr src)
+ virStorageSourcePtr src,
+ bool backingChain)
{
virSecurityStackDataPtr priv = virSecurityManagerGetPrivateData(mgr);
virSecurityStackItemPtr item = priv->itemsHead;
int rc = 0;
for (; item; item = item->next) {
- if (virSecurityManagerSetImageLabel(item->securityManager, vm, src) < 0)
+ if (virSecurityManagerSetImageLabel(item->securityManager, vm, src,
+ backingChain) < 0)
rc = -1;
}
@@ -617,7 +583,8 @@ virSecurityStackSetImageLabel(virSecurityManagerPtr mgr,
static int
virSecurityStackRestoreImageLabel(virSecurityManagerPtr mgr,
virDomainDefPtr vm,
- virStorageSourcePtr src)
+ virStorageSourcePtr src,
+ bool backingChain)
{
virSecurityStackDataPtr priv = virSecurityManagerGetPrivateData(mgr);
virSecurityStackItemPtr item = priv->itemsHead;
@@ -625,7 +592,7 @@ virSecurityStackRestoreImageLabel(virSecurityManagerPtr mgr,
for (; item; item = item->next) {
if (virSecurityManagerRestoreImageLabel(item->securityManager,
- vm, src) < 0)
+ vm, src, backingChain) < 0)
rc = -1;
}
@@ -816,9 +783,6 @@ virSecurityDriver virSecurityDriverStack = {
.domainSecurityVerify = virSecurityStackVerify,
- .domainSetSecurityDiskLabel = virSecurityStackSetDiskLabel,
- .domainRestoreSecurityDiskLabel = virSecurityStackRestoreDiskLabel,
-
.domainSetSecurityImageLabel = virSecurityStackSetImageLabel,
.domainRestoreSecurityImageLabel = virSecurityStackRestoreImageLabel,
--
2.20.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 1/23/19 11:10 AM, Peter Krempa wrote:
> Security labelling of disks consists of labelling of the disk image
*labeling
> itself and it's backing chain. Modify
> virSecurityManager[Set|Restore]ImageLabel to take a boolean flag that
> will label the full chain rather than the top image itself.
>
> This allows to delete/unify some parts of the code and will also
> simplify callers in some cases.
>
> Signed-off-by: Peter Krempa <pkrempa@redhat.com>
> ---
> src/qemu/qemu_security.c | 6 ++--
> src/security/security_apparmor.c | 24 +++------------
> src/security/security_dac.c | 40 +++++++------------------
> src/security/security_driver.h | 15 +++-------
> src/security/security_manager.c | 20 ++++++++-----
> src/security/security_manager.h | 6 ++--
> src/security/security_nop.c | 25 +++-------------
> src/security/security_selinux.c | 42 ++++++++-------------------
> src/security/security_stack.c | 50 +++++---------------------------
> 9 files changed, 60 insertions(+), 168 deletions(-)
>
I see two logical things happening...
1. Removing virSecurityDomain{Set|Restore}DiskLabel in favor of
virSecurityDomain{Set|Restore}ImageLabel
2. Adding parameter to virSecurityManager{Set|Restore}ImageLabel.
I think the parameter should be "unsigned int flags" instead of "bool
backingChain"? The latter is too specific. Then of course the first flag
defined is for backingChain. Also avoids some future change adding bool
myNewFlagName to the API. I do see a few other API's use bool's, but
does that mean they're right?
Beyond that - seems odd to remove the backend/inside of the
{Set|Restore}DiskLabel before replacing all the callers uses to
{Set|Restore}ImageLabel first. I see the bisect problem is handled by
changing virSecurityManager{Set|Restore}DiskLabel to call
domain{Set|Restore}SecurityImageLabel instead.
So, w/r/t commit message to "describe" what's happening consider
indicating the short term usage of *ImageLabel for the *DiskLabel. The
alternative is reordering patches, which I don't find necessary.
Assuming usage of @flags and I'm confident you can make that
alteration... So for the logic,
Reviewed-by: John Ferlan <jferlan@redhat.com>
John
[...]
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Mon, Jan 28, 2019 at 09:26:45 -0500, John Ferlan wrote:
>
>
> On 1/23/19 11:10 AM, Peter Krempa wrote:
> > Security labelling of disks consists of labelling of the disk image
>
> *labeling
>
> > itself and it's backing chain. Modify
> > virSecurityManager[Set|Restore]ImageLabel to take a boolean flag that
> > will label the full chain rather than the top image itself.
> >
> > This allows to delete/unify some parts of the code and will also
> > simplify callers in some cases.
> >
> > Signed-off-by: Peter Krempa <pkrempa@redhat.com>
> > ---
> > src/qemu/qemu_security.c | 6 ++--
> > src/security/security_apparmor.c | 24 +++------------
> > src/security/security_dac.c | 40 +++++++------------------
> > src/security/security_driver.h | 15 +++-------
> > src/security/security_manager.c | 20 ++++++++-----
> > src/security/security_manager.h | 6 ++--
> > src/security/security_nop.c | 25 +++-------------
> > src/security/security_selinux.c | 42 ++++++++-------------------
> > src/security/security_stack.c | 50 +++++---------------------------
> > 9 files changed, 60 insertions(+), 168 deletions(-)
> >
>
> I see two logical things happening...
>
> 1. Removing virSecurityDomain{Set|Restore}DiskLabel in favor of
> virSecurityDomain{Set|Restore}ImageLabel
>
> 2. Adding parameter to virSecurityManager{Set|Restore}ImageLabel.
>
> I think the parameter should be "unsigned int flags" instead of "bool
I'll use the correct type here.
> backingChain"? The latter is too specific. Then of course the first flag
> defined is for backingChain. Also avoids some future change adding bool
> myNewFlagName to the API. I do see a few other API's use bool's, but
> does that mean they're right?
It will be somewhat verbose:
+typedef enum {
+ VIR_SECURITY_DOMAIN_IMAGE_LABEL_BACKING_CHAIN = 1 << 0;
+} virSecurityDomainImageLabelFlags;
+
typedef int (*virSecurityDomainSetImageLabel) (virSecurityManagerPtr mgr,
virDomainDefPtr def,
virStorageSourcePtr src,
- bool backingChain);
+ virSecurityDomainImageLabelFlags flags);
typedef int (*virSecurityDomainRestoreImageLabel) (virSecurityManagerPtr mgr,
virDomainDefPtr def,
virStorageSourcePtr src,
- bool backingChain);
+ virSecurityDomainImageLabelFlags flags;
typedef int (*virSecurityDomainSetMemoryLabel) (virSecurityManagerPtr mgr,
virDomainDefPtr def,
virDomainMemoryDefPtr mem);
> Beyond that - seems odd to remove the backend/inside of the
> {Set|Restore}DiskLabel before replacing all the callers uses to
> {Set|Restore}ImageLabel first. I see the bisect problem is handled by
> changing virSecurityManager{Set|Restore}DiskLabel to call
> domain{Set|Restore}SecurityImageLabel instead.
It's not only "bisect problem" but genuine replacement of the internals
with a different internal impl without changing the callers.
The disk labelling function becomes a shim which adds a parameter.
This was done to limit scope change. This patch should in all callers
besides the ones in Set|RestoreDisk label add the 'false' flag (or the
equivalent.
>
> So, w/r/t commit message to "describe" what's happening consider
> indicating the short term usage of *ImageLabel for the *DiskLabel. The
> alternative is reordering patches, which I don't find necessary.
Reordering is impossible without adding the parameter first or squashing
them together, where it would drag in semantic changes from the places
calling {Set|Restore}DiskLabel.
>
> Assuming usage of @flags and I'm confident you can make that
> alteration... So for the logic,
>
> Reviewed-by: John Ferlan <jferlan@redhat.com>
>
> John
>
> [...]
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
© 2016 - 2025 Red Hat, Inc.