[libvirt] [PATCH V2 2/2] Don't autogenerate seclabels of type 'none'

[Erik Skultety]

On Wed, Aug 16, 2017 at 05:54:08PM -0600, Jim Fehlig wrote:
> When security drivers are active but confinement is not enabled,
> there is no need to autogenerate <seclabel> elements when starting
> a domain def that contains no <seclabel> elements. In fact,
> autogenerating the elements can result in needless save/restore and
> migration failures when the security driver is not active on the
> restore/migration target.
>
> This patch changes the virSecurityManagerGenLabel function in
> src/security_manager.c to only autogenerate a <seclabel> element
> if none is already defined for the domain *and* default
> confinement is enabled. Otherwise the needless <seclabel>
> autogeneration is skipped.
>
> Resolves: https://bugzilla.opensuse.org/show_bug.cgi?id=1051017
> Signed-off-by: Jim Fehlig <jfehlig at suse.com>
> ---
>
> V2: Don't autogenerate a seclabel if domain does not contain one
>     and confinement is disabled.
>
>  src/security/security_manager.c | 42 +++++++++++++++++++++--------------------
>  1 file changed, 22 insertions(+), 20 deletions(-)
>
> diff --git a/src/security/security_manager.c b/src/security/security_manager.c
> index 013bbc37e..10515c314 100644
> --- a/src/security/security_manager.c
> +++ b/src/security/security_manager.c
> @@ -650,30 +650,32 @@ virSecurityManagerGenLabel(virSecurityManagerPtr mgr,
>      for (i = 0; sec_managers[i]; i++) {
>          generated = false;
>          seclabel = virDomainDefGetSecurityLabelDef(vm, sec_managers[i]->drv->name);
> -        if (!seclabel) {
> -            if (!(seclabel = virSecurityLabelDefNew(sec_managers[i]->drv->name)))
> -                goto cleanup;
> -            generated = seclabel->implicit = true;
> -        }
> +        if (seclabel) {

Just a tiny nitpick, generally we prefer the 'if' block to be shorter than the
corresponding 'else' block.

> +            if (seclabel->type == VIR_DOMAIN_SECLABEL_DEFAULT) {
> +                if (virSecurityManagerGetDefaultConfined(sec_managers[i])) {
> +                    seclabel->type = VIR_DOMAIN_SECLABEL_DYNAMIC;
> +                } else {
> +                    seclabel->type = VIR_DOMAIN_SECLABEL_NONE;
> +                    seclabel->relabel = false;
> +                }
> +            }
>
> -        if (seclabel->type == VIR_DOMAIN_SECLABEL_DEFAULT) {
> +            if (seclabel->type == VIR_DOMAIN_SECLABEL_NONE) {
> +                if (virSecurityManagerGetRequireConfined(sec_managers[i])) {
> +                    virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> +                                   _("Unconfined guests are not allowed on this host"));
> +                    goto cleanup;
> +                }
> +            }
> +        } else {
> +            /* Only generate seclabel if confinement is enabled */
>              if (virSecurityManagerGetDefaultConfined(sec_managers[i])) {

The same applies to this nested if-else block.

> +                if (!(seclabel = virSecurityLabelDefNew(sec_managers[i]->drv->name)))
> +                    goto cleanup;
> +                generated = seclabel->implicit = true;
>                  seclabel->type = VIR_DOMAIN_SECLABEL_DYNAMIC;
>              } else {
> -                seclabel->type = VIR_DOMAIN_SECLABEL_NONE;
> -                seclabel->relabel = false;
> -            }
> -        }
> -
> -        if (seclabel->type == VIR_DOMAIN_SECLABEL_NONE) {
> -            if (virSecurityManagerGetRequireConfined(sec_managers[i])) {
> -                virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> -                               _("Unconfined guests are not allowed on this host"));
> -                goto cleanup;
> -            } else if (vm->nseclabels && generated) {
> -                VIR_DEBUG("Skipping auto generated seclabel of type none");
> -                virSecurityLabelDefFree(seclabel);
> -                seclabel = NULL;
> +                VIR_DEBUG("Skipping auto generated seclabel");
>                  continue;
>              }
>          }

ACK with the adjustment.

Erik