[libvirt] [PATCH v2] virt-aa-helper: fix parsing security labels

[Guido Günther]

On Mon, Nov 21, 2016 at 03:40:23PM +0100, Christian Ehrhardt wrote:
> When virt-aa-helper parses xml content it can fail on security labels.
> 
> It fails by requiring to parse active domain content on seclabels that
> is not yet filled in.
> 
> Testcase with virt-aa-helper on a minimal xml:
>  $ cat << EOF > /tmp/test.xml
> <domain type='kvm'>
>     <name>test-seclabel</name>
>     <uuid>12345678-9abc-def1-2345-6789abcdef00</uuid>
>     <memory unit='KiB'>1</memory>
>     <os><type arch='x86_64'>hvm</type></os>
>     <seclabel type='dynamic' model='apparmor' relabel='yes'/>
>     <seclabel type='dynamic' model='dac' relabel='yes'/>
> </domain>
> EOF
>  $ /usr/lib/libvirt/virt-aa-helper -d -r -p 0 \
>    -u libvirt-12345678-9abc-def1-2345-6789abcdef00 < /tmp/test.xml
> 
> Current Result:
>  virt-aa-helper: error: could not parse XML
>  virt-aa-helper: error: could not get VM definition
> Expected Result is a valid apparmor profile
> 
> Updates:
> v2
> - simplified and clarified commit message
> - make the flag skip all secabel parsing
> - shorten the new flag name
> 
> fixes: dfbc9a83 ("apparmor: QEMU monitor socket moved")
> 
> Signed-off-by: Christian Ehrhardt <christian.ehrhardt at canonical.com>
> ---
>  src/conf/domain_conf.c        | 6 ++++--
>  src/conf/domain_conf.h        | 2 ++
>  src/security/virt-aa-helper.c | 1 +
>  3 files changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 6e008e2..bbe550b 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -16289,8 +16289,10 @@ virDomainDefParseXML(xmlDocPtr xml,
>  
>      /* analysis of security label, done early even though we format it
>       * late, so devices can refer to this for defaults */
> -    if (virSecurityLabelDefsParseXML(def, ctxt, caps, flags) == -1)
> -        goto error;
> +    if (!(flags & VIR_DOMAIN_DEF_PARSE_SKIP_SECLABEL)) {
n> +        if (virSecurityLabelDefsParseXML(def, ctxt, caps, flags) == -1)
> +            goto error;
> +    }
>  
>      /* Extract domain memory */
>      if (virDomainParseMemory("./memory[1]", NULL, ctxt,
> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> index 541b600..3a9693c 100644
> --- a/src/conf/domain_conf.h
> +++ b/src/conf/domain_conf.h
> @@ -2666,6 +2666,8 @@ typedef enum {
>      VIR_DOMAIN_DEF_PARSE_ABI_UPDATE = 1 << 9,
>      /* skip definition validation checks meant to be executed on define time only */
>      VIR_DOMAIN_DEF_PARSE_SKIP_VALIDATE = 1 << 10,
> +    /* skip parsing of security labels */
> +    VIR_DOMAIN_DEF_PARSE_SKIP_SECLABEL        = 1 << 11,
>  } virDomainDefParseFlags;
>  
>  typedef enum {
> diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c
> index 77eeaff..5f5d1cd 100644
> --- a/src/security/virt-aa-helper.c
> +++ b/src/security/virt-aa-helper.c
> @@ -705,6 +705,7 @@ get_definition(vahControl * ctl, const char *xmlStr)
>  
>      ctl->def = virDomainDefParseString(xmlStr,
>                                         ctl->caps, ctl->xmlopt, NULL,
> +                                       VIR_DOMAIN_DEF_PARSE_SKIP_SECLABEL |
>                                         VIR_DOMAIN_DEF_PARSE_SKIP_VALIDATE);
>  
>      if (ctl->def == NULL) {
> -- 
> 2.7.4
> 

This looks good to me (and looks more terse now) . It would be great to
have another review though.
Cheers,
 -- Guido