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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list