[libvirt] [PATCH] Don't autogenerate seclabels of type 'none'
When security drivers are active and domain def contains no <seclabel> elements, there is no need to autogenerate seclabels when starting the domain, e.g. <seclabel type='none' model='apparmor'/> In fact, autogenerating the label can result in needless save/restore and migration failures when the security driver is not active on the restore/migration target. The virSecurityManagerGenLabel function in src/security_manager.c even has logic to skip autogenerated labels, but the logic is a bit flawed. Autogeneration should be skipped when the domain has not seclabels, i.e. vm->nseclabels == 0. Resolves: https://bugzilla.opensuse.org/show_bug.cgi?id=1051017 Signed-off-by: Jim Fehlig <jfehlig@suse.com> --- src/security/security_manager.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/security/security_manager.c b/src/security/security_manager.c index 013bbc37e..441c4d1fd 100644 --- a/src/security/security_manager.c +++ b/src/security/security_manager.c @@ -670,7 +670,7 @@ virSecurityManagerGenLabel(virSecurityManagerPtr mgr, virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("Unconfined guests are not allowed on this host")); goto cleanup; - } else if (vm->nseclabels && generated) { + } else if (vm->nseclabels == 0 && generated) { VIR_DEBUG("Skipping auto generated seclabel of type none"); virSecurityLabelDefFree(seclabel); seclabel = NULL; -- 2.13.1
On Mon, Aug 14, 2017 at 06:07:10PM -0600, Jim Fehlig wrote:
When security drivers are active and domain def contains no <seclabel> elements, there is no need to autogenerate seclabels when starting the domain, e.g.
<seclabel type='none' model='apparmor'/>
In fact, autogenerating the label can result in needless save/restore and migration failures when the security driver is not active on the restore/migration target.
The virSecurityManagerGenLabel function in src/security_manager.c even has logic to skip autogenerated labels, but the logic is a bit flawed. Autogeneration should be skipped when the domain has not seclabels, i.e. vm->nseclabels == 0.
Resolves: https://bugzilla.opensuse.org/show_bug.cgi?id=1051017 Signed-off-by: Jim Fehlig <jfehlig@suse.com> --- src/security/security_manager.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/security/security_manager.c b/src/security/security_manager.c index 013bbc37e..441c4d1fd 100644 --- a/src/security/security_manager.c +++ b/src/security/security_manager.c @@ -670,7 +670,7 @@ virSecurityManagerGenLabel(virSecurityManagerPtr mgr, virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("Unconfined guests are not allowed on this host")); goto cleanup; - } else if (vm->nseclabels && generated) { + } else if (vm->nseclabels == 0 && generated) {
This would likely cause a regression like we did prior to commit e4a28a3281 which introduced the condition you're changing, IOW if you specify a seclabel specifically, you're still going to autogenerate one of type='none'. So my question is, what's the point of autogenerating seclabel of type='none' anyway? Shouldn't we just skip type='none' altogether when it's us who generated it? Erik
On 08/16/2017 05:23 AM, Erik Skultety wrote:
On Mon, Aug 14, 2017 at 06:07:10PM -0600, Jim Fehlig wrote:
When security drivers are active and domain def contains no <seclabel> elements, there is no need to autogenerate seclabels when starting the domain, e.g.
<seclabel type='none' model='apparmor'/>
In fact, autogenerating the label can result in needless save/restore and migration failures when the security driver is not active on the restore/migration target.
The virSecurityManagerGenLabel function in src/security_manager.c even has logic to skip autogenerated labels, but the logic is a bit flawed. Autogeneration should be skipped when the domain has not seclabels, i.e. vm->nseclabels == 0.
Resolves: https://bugzilla.opensuse.org/show_bug.cgi?id=1051017 Signed-off-by: Jim Fehlig <jfehlig@suse.com> --- src/security/security_manager.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/security/security_manager.c b/src/security/security_manager.c index 013bbc37e..441c4d1fd 100644 --- a/src/security/security_manager.c +++ b/src/security/security_manager.c @@ -670,7 +670,7 @@ virSecurityManagerGenLabel(virSecurityManagerPtr mgr, virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("Unconfined guests are not allowed on this host")); goto cleanup; - } else if (vm->nseclabels && generated) { + } else if (vm->nseclabels == 0 && generated) {
This would likely cause a regression like we did prior to commit e4a28a3281 which introduced the condition you're changing, IOW if you specify a seclabel specifically, you're still going to autogenerate one of type='none'.
I had read that commit, but after looking again I misinterpreted it.
So my question is, what's the point of autogenerating seclabel of type='none' anyway? Shouldn't we just skip type='none' altogether when it's us who generated it?
IMO, yes, and that is what I was trying to do. One flaw in my thinking was not considering the security_default_confined setting. I changed the logic in virSecurityManagerGenLabel a bit, fixed the securityselinuxtest, and sent a V2 https://www.redhat.com/archives/libvir-list/2017-August/msg00473.html Regards, Jim
participants (2)
-
Erik Skultety -
Jim Fehlig