virSecurityManagerDomainSetPathLabel is used to make a path known
to the security modules, but today is used interchangably for
- paths to files/dirs to be accessed directly
- paths to a dir, but the access will actually be to files therein
Depending on the security module it is important to know which of
these types it will be.
The argument fullpath augments the call to the implementations of
DomainSetPathLabel that can - per security module - decide if extra
actions shall be taken.
For now dac/selinux handle this as before, but apparmor will make
use of it to add a wildcard to the path that was passed.
Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com>
---
src/qemu/qemu_domain.c | 2 +-
src/qemu/qemu_process.c | 4 ++--
src/security/security_apparmor.c | 17 +++++++++++++++--
src/security/security_dac.c | 3 ++-
src/security/security_driver.h | 3 ++-
src/security/security_manager.c | 5 +++--
src/security/security_manager.h | 3 ++-
src/security/security_selinux.c | 3 ++-
src/security/security_stack.c | 5 +++--
9 files changed, 32 insertions(+), 13 deletions(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 70fb406..ac3e182 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -692,7 +692,7 @@ qemuDomainWriteMasterKeyFile(virQEMUDriverPtr driver,
}
if (qemuSecurityDomainSetPathLabel(driver->securityManager,
- vm->def, path) < 0)
+ vm->def, path, false) < 0)
goto cleanup;
ret = 0;
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index a0f430f..1a0923a 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -3401,7 +3401,7 @@ qemuProcessBuildDestroyMemoryPathsImpl(virQEMUDriverPtr driver,
}
if (qemuSecurityDomainSetPathLabel(driver->securityManager,
- def, path) < 0) {
+ def, path, true) < 0) {
virReportError(VIR_ERR_INTERNAL_ERROR,
_("Unable to label %s"), path);
return -1;
@@ -4514,7 +4514,7 @@ qemuProcessMakeDir(virQEMUDriverPtr driver,
}
if (qemuSecurityDomainSetPathLabel(driver->securityManager,
- vm->def, path) < 0)
+ vm->def, path, true) < 0)
goto cleanup;
ret = 0;
diff --git a/src/security/security_apparmor.c b/src/security/security_apparmor.c
index dcd6f52..60a8e08 100644
--- a/src/security/security_apparmor.c
+++ b/src/security/security_apparmor.c
@@ -956,9 +956,22 @@ AppArmorSetSavedStateLabel(virSecurityManagerPtr mgr,
static int
AppArmorSetPathLabel(virSecurityManagerPtr mgr,
virDomainDefPtr def,
- const char *path)
+ const char *path,
+ bool fullpath)
{
- return reload_profile(mgr, def, path, true);
+ int rc = -1;
+ char *full_path = NULL;
+
+ if (fullpath) {
+ if (virAsprintf(&full_path, "%s/{,**}", path) < 0)
+ return -1;
+ rc = reload_profile(mgr, def, full_path, true);
+ VIR_FREE(full_path);
+ }
+ else
+ rc = reload_profile(mgr, def, path, true);
+
+ return rc;
}
static int
diff --git a/src/security/security_dac.c b/src/security/security_dac.c
index 609d259..60c4f09 100644
--- a/src/security/security_dac.c
+++ b/src/security/security_dac.c
@@ -2081,7 +2081,8 @@ virSecurityDACGetBaseLabel(virSecurityManagerPtr mgr,
static int
virSecurityDACDomainSetPathLabel(virSecurityManagerPtr mgr,
virDomainDefPtr def,
- const char *path)
+ const char *path,
+ bool fullpath ATTRIBUTE_UNUSED)
{
virSecurityDACDataPtr priv = virSecurityManagerGetPrivateData(mgr);
virSecurityLabelDefPtr seclabel;
diff --git a/src/security/security_driver.h b/src/security/security_driver.h
index 47dad8b..20168a6 100644
--- a/src/security/security_driver.h
+++ b/src/security/security_driver.h
@@ -139,7 +139,8 @@ typedef int (*virSecurityDomainRestoreInputLabel) (virSecurityManagerPtr mgr,
virDomainInputDefPtr input);
typedef int (*virSecurityDomainSetPathLabel) (virSecurityManagerPtr mgr,
virDomainDefPtr def,
- const char *path);
+ const char *path,
+ bool fullpath);
typedef int (*virSecurityDomainSetChardevLabel) (virSecurityManagerPtr mgr,
virDomainDefPtr def,
virDomainChrSourceDefPtr dev_source,
diff --git a/src/security/security_manager.c b/src/security/security_manager.c
index 9249aba..fbd4333 100644
--- a/src/security/security_manager.c
+++ b/src/security/security_manager.c
@@ -1048,12 +1048,13 @@ virSecurityManagerGetNested(virSecurityManagerPtr mgr)
int
virSecurityManagerDomainSetPathLabel(virSecurityManagerPtr mgr,
virDomainDefPtr vm,
- const char *path)
+ const char *path,
+ bool fullpath)
{
if (mgr->drv->domainSetPathLabel) {
int ret;
virObjectLock(mgr);
- ret = mgr->drv->domainSetPathLabel(mgr, vm, path);
+ ret = mgr->drv->domainSetPathLabel(mgr, vm, path, fullpath);
virObjectUnlock(mgr);
return ret;
}
diff --git a/src/security/security_manager.h b/src/security/security_manager.h
index 013e3b9..4ef6bd8 100644
--- a/src/security/security_manager.h
+++ b/src/security/security_manager.h
@@ -182,7 +182,8 @@ int virSecurityManagerRestoreInputLabel(virSecurityManagerPtr mgr,
int virSecurityManagerDomainSetPathLabel(virSecurityManagerPtr mgr,
virDomainDefPtr vm,
- const char *path);
+ const char *path,
+ bool fullpath);
int virSecurityManagerSetChardevLabel(virSecurityManagerPtr mgr,
virDomainDefPtr def,
diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c
index 0815a02..9a24b30 100644
--- a/src/security/security_selinux.c
+++ b/src/security/security_selinux.c
@@ -3028,7 +3028,8 @@ virSecuritySELinuxGetSecurityMountOptions(virSecurityManagerPtr mgr,
static int
virSecuritySELinuxDomainSetPathLabel(virSecurityManagerPtr mgr,
virDomainDefPtr def,
- const char *path)
+ const char *path,
+ bool fullpath ATTRIBUTE_UNUSED)
{
virSecurityLabelDefPtr seclabel;
diff --git a/src/security/security_stack.c b/src/security/security_stack.c
index 0375e7d..5ad4d99 100644
--- a/src/security/security_stack.c
+++ b/src/security/security_stack.c
@@ -704,7 +704,8 @@ virSecurityStackRestoreInputLabel(virSecurityManagerPtr mgr,
static int
virSecurityStackDomainSetPathLabel(virSecurityManagerPtr mgr,
virDomainDefPtr vm,
- const char *path)
+ const char *path,
+ bool fullpath)
{
virSecurityStackDataPtr priv = virSecurityManagerGetPrivateData(mgr);
virSecurityStackItemPtr item = priv->itemsHead;
@@ -712,7 +713,7 @@ virSecurityStackDomainSetPathLabel(virSecurityManagerPtr mgr,
for (; item; item = item->next) {
if (virSecurityManagerDomainSetPathLabel(item->securityManager,
- vm, path) < 0)
+ vm, path, fullpath) < 0)
rc = -1;
}
--
2.7.4
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 01/03/2018 06:00 PM, Christian Ehrhardt wrote:
> virSecurityManagerDomainSetPathLabel is used to make a path known
> to the security modules, but today is used interchangably for
> - paths to files/dirs to be accessed directly
> - paths to a dir, but the access will actually be to files therein
>
> Depending on the security module it is important to know which of
> these types it will be.
>
> The argument fullpath augments the call to the implementations of
> DomainSetPathLabel that can - per security module - decide if extra
> actions shall be taken.
>
> For now dac/selinux handle this as before, but apparmor will make
> use of it to add a wildcard to the path that was passed.
>
> Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com>
> ---
> src/qemu/qemu_domain.c | 2 +-
> src/qemu/qemu_process.c | 4 ++--
> src/security/security_apparmor.c | 17 +++++++++++++++--
> src/security/security_dac.c | 3 ++-
> src/security/security_driver.h | 3 ++-
> src/security/security_manager.c | 5 +++--
> src/security/security_manager.h | 3 ++-
> src/security/security_selinux.c | 3 ++-
> src/security/security_stack.c | 5 +++--
> 9 files changed, 32 insertions(+), 13 deletions(-)
>
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index 70fb406..ac3e182 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -692,7 +692,7 @@ qemuDomainWriteMasterKeyFile(virQEMUDriverPtr driver,
> }
>
> if (qemuSecurityDomainSetPathLabel(driver->securityManager,
> - vm->def, path) < 0)
> + vm->def, path, false) < 0)
> goto cleanup;
>
> ret = 0;
> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> index a0f430f..1a0923a 100644
> --- a/src/qemu/qemu_process.c
> +++ b/src/qemu/qemu_process.c
> @@ -3401,7 +3401,7 @@ qemuProcessBuildDestroyMemoryPathsImpl(virQEMUDriverPtr driver,
> }
>
> if (qemuSecurityDomainSetPathLabel(driver->securityManager,
> - def, path) < 0) {
> + def, path, true) < 0) {
> virReportError(VIR_ERR_INTERNAL_ERROR,
> _("Unable to label %s"), path);
> return -1;
> @@ -4514,7 +4514,7 @@ qemuProcessMakeDir(virQEMUDriverPtr driver,
> }
>
> if (qemuSecurityDomainSetPathLabel(driver->securityManager,
> - vm->def, path) < 0)
> + vm->def, path, true) < 0)
> goto cleanup;
>
> ret = 0;
> diff --git a/src/security/security_apparmor.c b/src/security/security_apparmor.c
> index dcd6f52..60a8e08 100644
> --- a/src/security/security_apparmor.c
> +++ b/src/security/security_apparmor.c
> @@ -956,9 +956,22 @@ AppArmorSetSavedStateLabel(virSecurityManagerPtr mgr,
> static int
> AppArmorSetPathLabel(virSecurityManagerPtr mgr,
> virDomainDefPtr def,
> - const char *path)
> + const char *path,
> + bool fullpath)
@fullpath seems misleading to me. At first I though that this is
absolute vs. relative path. Maybe allowSubtree is a better name?
Also, I know we don't do it everywhere, but given how ambiguous this
argument's name is can we have a comment describing the function and its
arguments please?
> {
> - return reload_profile(mgr, def, path, true);
> + int rc = -1;
> + char *full_path = NULL;
> +
> + if (fullpath) {
> + if (virAsprintf(&full_path, "%s/{,**}", path) < 0)
> + return -1;
> + rc = reload_profile(mgr, def, full_path, true);
> + VIR_FREE(full_path);
> + }
> + else
> + rc = reload_profile(mgr, def, path, true);
Almost. Curly braces and else should be at one line. But then you get a
syntax-check error because there's another rule saying that if one
branch has curly braces the other one has to have them too.
Michal
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Tue, Jan 9, 2018 at 11:02 AM, Michal Privoznik <mprivozn@redhat.com> wrote:
> On 01/03/2018 06:00 PM, Christian Ehrhardt wrote:
[...]
>> AppArmorSetPathLabel(virSecurityManagerPtr mgr,
>> virDomainDefPtr def,
>> - const char *path)
>> + const char *path,
>> + bool fullpath)
>
> @fullpath seems misleading to me. At first I though that this is
> absolute vs. relative path. Maybe allowSubtree is a better name?
Yes it is
> Also, I know we don't do it everywhere, but given how ambiguous this
> argument's name is can we have a comment describing the function and its
> arguments please?
Yes reasonable, since this is implemented multiple times (by each
security module) I'll add the details to the header.
Otherwise I'd spread it all over the place in a redundant way which seems worse.
>> {
>> - return reload_profile(mgr, def, path, true);
>> + int rc = -1;
>> + char *full_path = NULL;
>> +
>> + if (fullpath) {
>> + if (virAsprintf(&full_path, "%s/{,**}", path) < 0)
>> + return -1;
>> + rc = reload_profile(mgr, def, full_path, true);
>> + VIR_FREE(full_path);
>> + }
>> + else
>> + rc = reload_profile(mgr, def, path, true);
>
> Almost. Curly braces and else should be at one line. But then you get a
> syntax-check error because there's another rule saying that if one
> branch has curly braces the other one has to have them too.
Ok same line AND curly braces for both.
TL;DR ok with all suggestions - thanks for the review.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
© 2016 - 2025 Red Hat, Inc.