[libvirt] [PATCH] fix parsing security labels from virt-aa-helper
When parsing labels virt-aa-helper does no more pass VIR_DOMAIN_DEF_PARSE_INACTIVE due to dfbc9a83 that tried to mitigate the changes of a89f05ba. For those it had to switch from VIR_DOMAIN_DEF_PARSE_INACTIVE to active since we need the domain id (ctl->def->id) as it is part of the socket path now which is needed for the aa profile. But that turned out to break non apparmor seclabels as well as apparmor seclabels in xmls without labels. In those cases due to VIR_DOMAIN_DEF_PARSE_INACTIVE now not set anymore virSecurityLabelDefParseXML insists on finding labels. Cases: - non-apparmor seclabel - virt-aa-helper breaks - apparmor seclabel without labels on a defined domain - virt-aa-helper breaks This was not spotted due to labels getting dynamically created on definition. So "define, start, stop" works. But "define, edit (add label), start" does not. Now turning back on VIR_DOMAIN_DEF_PARSE_INACTIVE would cause the old bug, so we have to differ those more fine grained. This is done by the new flag VIR_DOMAIN_DEF_PARSE_SKIP_ACTIVE_LABEL which is like VIR_DOMAIN_DEF_PARSE_INACTIVE but only for the security labels. So far only set by virt-aa-helper. Testcase with virt-aa-helper on xml file: virt-aa-helper -d -r -p 0 -u libvirt-<uuid> < your-guest.xml virt-aa-helper: error: could not parse XML virt-aa-helper: error: could not get VM definition (That should have printed a valid apparmor profile) Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com> --- src/conf/domain_conf.c | 6 ++++-- src/conf/domain_conf.h | 3 +++ src/security/virt-aa-helper.c | 1 + 3 files changed, 8 insertions(+), 2 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 03506cb..9eb7883 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -6626,7 +6626,8 @@ virSecurityLabelDefParseXML(xmlXPathContextPtr ctxt, * if the 'live' VM XML is requested */ if (seclabel->type == VIR_DOMAIN_SECLABEL_STATIC || - (!(flags & VIR_DOMAIN_DEF_PARSE_INACTIVE) && + (!(flags & (VIR_DOMAIN_DEF_PARSE_SKIP_ACTIVE_LABEL | + VIR_DOMAIN_DEF_PARSE_INACTIVE)) && seclabel->type != VIR_DOMAIN_SECLABEL_NONE)) { p = virXPathStringLimit("string(./label[1])", VIR_SECURITY_LABEL_BUFLEN-1, ctxt); @@ -6642,7 +6643,8 @@ virSecurityLabelDefParseXML(xmlXPathContextPtr ctxt, /* Only parse imagelabel, if requested live XML with relabeling */ if (seclabel->relabel && - (!(flags & VIR_DOMAIN_DEF_PARSE_INACTIVE) && + (!(flags & (VIR_DOMAIN_DEF_PARSE_SKIP_ACTIVE_LABEL | + VIR_DOMAIN_DEF_PARSE_INACTIVE)) && seclabel->type != VIR_DOMAIN_SECLABEL_NONE)) { p = virXPathStringLimit("string(./imagelabel[1])", VIR_SECURITY_LABEL_BUFLEN-1, ctxt); diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 24aa79c..90693c6 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2657,6 +2657,9 @@ 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, + /* in regard to security labels, skip parts of the XML that would only be + * present in an active libvirt XML. */ + VIR_DOMAIN_DEF_PARSE_SKIP_ACTIVE_LABEL = 1 << 11, } virDomainDefParseFlags; typedef enum { diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c index 77eeaff..0ca4c83 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_ACTIVE_LABEL | VIR_DOMAIN_DEF_PARSE_SKIP_VALIDATE); if (ctl->def == NULL) { -- 2.7.4
On Mon, Oct 31, 2016 at 11:32 AM, Christian Ehrhardt < christian.ehrhardt@canonical.com> wrote:
But that turned out to break non apparmor seclabels as well as apparmor seclabels in xmls without labels.
FYI - For a bit extra info on the case, debugging it and in general more background that neither fitted a cover page nor the actual patch description please feel free to look at https://bugs.launchpad.net/ubuntu/+source/libvirt/+bug/1633207 -- Christian Ehrhardt Software Engineer, Ubuntu Server Canonical Ltd
Sorry to bother, but "ping" for the list and adding some more people to CC - for review or comments on this.
Hi,x On Mon, Oct 31, 2016 at 11:32:44AM +0100, Christian Ehrhardt wrote:
When parsing labels virt-aa-helper does no more pass VIR_DOMAIN_DEF_PARSE_INACTIVE due to dfbc9a83 that tried to mitigate the changes of a89f05ba. For those it had to switch from
I wouldn't call it mitigate. It was rather a fix.
VIR_DOMAIN_DEF_PARSE_INACTIVE to active since we need the domain id (ctl->def->id) as it is part of the socket path now which is needed for the aa profile.
But that turned out to break non apparmor seclabels as well as apparmor seclabels in xmls without labels.
While I understand the "non apparmor seclabel" part I don't understand what you mean by "apparmor seclabels in xmls without labels". On Debian based distros e.g. virtinst creates all VMs without seclabels and we let the dac and apparmor security drivers do the work. I.e. this gets added to the domains live xml upon start: <seclabel type='dynamic' model='apparmor' relabel='yes'> <label>libvirt-a9287b6e-ca06-42fe-b1a2-06830752843a</label> <imagelabel>libvirt-a9287b6e-ca06-42fe-b1a2-06830752843a</imagelabel> </seclabel> <seclabel type='dynamic' model='dac' relabel='yes'> <label>+117:+128</label> <imagelabel>+117:+128</imagelabel> </seclabel>
In those cases due to VIR_DOMAIN_DEF_PARSE_INACTIVE now not set anymore virSecurityLabelDefParseXML insists on finding labels. Cases: - non-apparmor seclabel - virt-aa-helper breaks - apparmor seclabel without labels on a defined domain - virt-aa-helper breaks This was not spotted due to labels getting dynamically created on definition.
As far as I understand it dynamic labels get created on domain start not domain definition (and are updated on e.g. device plug).
So "define, start, stop" works. But "define, edit (add label), start" does not.
What do you add during the edit step. Can you provide an example?
Now turning back on VIR_DOMAIN_DEF_PARSE_INACTIVE would cause the old bug, so we have to differ those more fine grained. This is done by the new flag VIR_DOMAIN_DEF_PARSE_SKIP_ACTIVE_LABEL which is like VIR_DOMAIN_DEF_PARSE_INACTIVE but only for the security labels. So far only set by virt-aa-helper.
Testcase with virt-aa-helper on xml file: virt-aa-helper -d -r -p 0 -u libvirt-<uuid> < your-guest.xml virt-aa-helper: error: could not parse XML virt-aa-helper: error: could not get VM definition (That should have printed a valid apparmor profile)
/usr/lib/libvirt/virt-aa-helper -d -r -p 0 -u libvirt-a9287b6e-ca06-42fe-b1a2-06830752843a < /etc/libvirt/qemu/wheezy.xml virt-aa-helper: /etc/apparmor.d/libvirt/libvirt-a9287b6e-ca06-42fe-b1a2-06830752843a.files virt-aa-helper: "/var/log/libvirt/**/wheezy.log" w, "/var/lib/libvirt/qemu/domain-wheezy/monitor.sock" rw, "/var/lib/libvirt/qemu/domain--1-wheezy/*" rw, "/var/lib/libvirt/qemu/channel/target/domain--1-wheezy/*" rw, "/var/run/libvirt/**/wheezy.pid" rwk, "/run/libvirt/**/wheezy.pid" rwk, "/var/run/libvirt/**/*.tunnelmigrate.dest.wheezy" rw, "/run/libvirt/**/*.tunnelmigrate.dest.wheezy" rw, "/var/scratch/sdcard/vmimages/wheezy.qcow2" rw, "/var/scratch/src/lts/rails/**" rw, "/var/scratch/src/lts/rails/" r, /dev/vhost-net rw, I don't understand why VIR_DOMAIN_DEF_PARSE_SKIP_VALIDATE is not sufficient. Cheers, -- Guido
Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com> --- src/conf/domain_conf.c | 6 ++++-- src/conf/domain_conf.h | 3 +++ src/security/virt-aa-helper.c | 1 + 3 files changed, 8 insertions(+), 2 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 03506cb..9eb7883 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -6626,7 +6626,8 @@ virSecurityLabelDefParseXML(xmlXPathContextPtr ctxt, * if the 'live' VM XML is requested */ if (seclabel->type == VIR_DOMAIN_SECLABEL_STATIC || - (!(flags & VIR_DOMAIN_DEF_PARSE_INACTIVE) && + (!(flags & (VIR_DOMAIN_DEF_PARSE_SKIP_ACTIVE_LABEL | + VIR_DOMAIN_DEF_PARSE_INACTIVE)) && seclabel->type != VIR_DOMAIN_SECLABEL_NONE)) { p = virXPathStringLimit("string(./label[1])", VIR_SECURITY_LABEL_BUFLEN-1, ctxt); @@ -6642,7 +6643,8 @@ virSecurityLabelDefParseXML(xmlXPathContextPtr ctxt,
/* Only parse imagelabel, if requested live XML with relabeling */ if (seclabel->relabel && - (!(flags & VIR_DOMAIN_DEF_PARSE_INACTIVE) && + (!(flags & (VIR_DOMAIN_DEF_PARSE_SKIP_ACTIVE_LABEL | + VIR_DOMAIN_DEF_PARSE_INACTIVE)) && seclabel->type != VIR_DOMAIN_SECLABEL_NONE)) { p = virXPathStringLimit("string(./imagelabel[1])", VIR_SECURITY_LABEL_BUFLEN-1, ctxt); diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 24aa79c..90693c6 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2657,6 +2657,9 @@ 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, + /* in regard to security labels, skip parts of the XML that would only be + * present in an active libvirt XML. */ + VIR_DOMAIN_DEF_PARSE_SKIP_ACTIVE_LABEL = 1 << 11, } virDomainDefParseFlags;
typedef enum { diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c index 77eeaff..0ca4c83 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_ACTIVE_LABEL | VIR_DOMAIN_DEF_PARSE_SKIP_VALIDATE);
if (ctl->def == NULL) { -- 2.7.4
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Thu, Nov 3, 2016 at 6:15 PM, Guido Günther <agx@sigxcpu.org> wrote: Thanks for your feedback Guido! On Mon, Oct 31, 2016 at 11:32:44AM +0100, Christian Ehrhardt wrote:
When parsing labels virt-aa-helper does no more pass VIR_DOMAIN_DEF_PARSE_INACTIVE due to dfbc9a83 that tried to mitigate the changes of a89f05ba. For those it had to switch from
I wouldn't call it mitigate. It was rather a fix.
True, I really meant no offense nor wanted to degrade the fix by wording it badly. I reworded the commit message locally so that the next re-submission (if needed) will call it a fix correctly.
(ctl->def->id) as it is part of the socket path now which is needed for
VIR_DOMAIN_DEF_PARSE_INACTIVE to active since we need the domain id the
aa profile.
But that turned out to break non apparmor seclabels as well as apparmor seclabels in xmls without labels.
While I understand the "non apparmor seclabel" part I don't understand what you mean by "apparmor seclabels in xmls without labels".
Yeah what I meant with "apparmor seclabels in xmls without labels" was misleading. The whole reproduction of the case started very shaky for me which I think has slipped into the description of this paragraph. What I wanted to make clear is this: If you are in the error case e.g. after adding: <seclabel type='dynamic' model='dac' relabel='yes'/> Note: Remember the error is now "... cannot load AppArmor profile ..." pointing to apparmor That neither adding an "apparmor seclabel without label" ... <seclabel type='dynamic' model='apparmor' relabel='yes'/> Nor adding "apparmor seclabel with label" ... <seclabel type='dynamic' model='apparmor' relabel='yes'> <label>libvirt-a4b7c764-988b-4a88-a614-5399b745ca94</label> <imagelabel>libvirt-a4b7c764-988b-4a88-a614-5399b745ca94</imagelabel> </seclabel> ... will get you going again. So while the message "...cannot load AppArmor profile..." is pointing to apparmor, it actually is an error parsing the dac seclabel.
On Debian based distros e.g. virtinst creates all VMs without seclabels and we let the dac and apparmor security drivers do the work. I.e. this gets added to the domains live xml upon start.
That works the same for my environment - if nothing is in the xml the security drivers add their stuff as needed and it can be seen in the active domain dumpxml with label and imagelabel set. Once shut down all content of the seclabel is gone in the dumpxml. That is the reason why in the common cases there is no issue to be seen. There is nothing in the xml to be parsed and the drivers add their stuff. But IMHO it is valid to add the seclabel to the xml explicitly and in that case the parsing as called from virt-aa-helper fails.
In those cases due to VIR_DOMAIN_DEF_PARSE_INACTIVE now not set anymore
virSecurityLabelDefParseXML insists on finding labels. Cases: - non-apparmor seclabel - virt-aa-helper breaks - apparmor seclabel without labels on a defined domain - virt-aa-helper breaks This was not spotted due to labels getting dynamically created on definition.
As far as I understand it dynamic labels get created on domain start not domain definition (and are updated on e.g. device plug).
Yes I agree. If I define an xml without a label the drivers add their stuff. On live (dumpxml) I then see it completely with all labels just as you posted. And once I shut down the guest and dumpxml it again it does not contain the seclabel anymore.
So "define, start, stop" works. But "define, edit (add label), start" does not.
What do you add during the edit step. Can you provide an example?
As shown above add a dac seclabel without label/imagelabel child elements: <seclabel type='dynamic' model='dac' relabel='yes'/> [...]
Testcase with virt-aa-helper on xml file: virt-aa-helper -d -r -p 0 -u libvirt-<uuid> < your-guest.xml virt-aa-helper: error: could not parse XML virt-aa-helper: error: could not get VM definition (That should have printed a valid apparmor profile)
/usr/lib/libvirt/virt-aa-helper -d -r -p 0 -u libvirt-a9287b6e-ca06-42fe-b1a2-06830752843a < /etc/libvirt/qemu/wheezy.xml virt-aa-helper: /etc/apparmor.d/libvirt/libvirt-a9287b6e-ca06-42fe-b1a2-06830752843a.files virt-aa-helper: "/var/log/libvirt/**/wheezy.log" w,
[...] Yep that is how the good case. As outlined that is either without seclabel in the xml (default) or with fully complete seclabels (+imagelabel +label). But as soon as there is the simple dac seclabel added it should show the issue. I don't understand why VIR_DOMAIN_DEF_PARSE_SKIP_VALIDATE is not sufficient.
As far as I understand the reason is that It is not a part of the validation that fails. The flag VIR_DOMAIN_DEF_PARSE_SKIP_VALIDATE does not skip the parsing of the seclabels, but that is where it fails already. In the function virSecurityLabelDefParseXML the only flag that skips the insisting on labels being present always was VIR_DOMAIN_DEF_PARSE_INACTIVE. Now I understand that you had to drop VIR_DOMAIN_DEF_PARSE_INACTIVE for the fix in dfbc9a83. But that caused the regression that "<seclabel type='dynamic' model='dac' relabel='yes'/>" can no more be added explicitly. So since we now have this orthogonal need to not set VIR_DOMAIN_DEF_PARSE_INACTIVE but at the same time need to "skip insisting on labels being set" I added the new flag to imply the latter when called from virt-aa-helper.
Sorry, I seem to become a pest more than I'd like to, but my timer on this thread expired again :-) Was the feedback I gave to the questions last week ok to understand the case and maybe reproduce to achieve a ack or do we need to discuss more?
Hi Christian, On Mon, Oct 31, 2016 at 11:32:44AM +0100, Christian Ehrhardt wrote:
When parsing labels virt-aa-helper does no more pass VIR_DOMAIN_DEF_PARSE_INACTIVE due to dfbc9a83 that tried to mitigate the changes of a89f05ba. For those it had to switch from VIR_DOMAIN_DEF_PARSE_INACTIVE to active since we need the domain id (ctl->def->id) as it is part of the socket path now which is needed for the aa profile.
But that turned out to break non apparmor seclabels as well as apparmor seclabels in xmls without labels. In those cases due to VIR_DOMAIN_DEF_PARSE_INACTIVE now not set anymore virSecurityLabelDefParseXML insists on finding labels. Cases: - non-apparmor seclabel - virt-aa-helper breaks - apparmor seclabel without labels on a defined domain - virt-aa-helper breaks This was not spotted due to labels getting dynamically created on definition. So "define, start, stop" works. But "define, edit (add label), start" does not.
Now turning back on VIR_DOMAIN_DEF_PARSE_INACTIVE would cause the old bug, so we have to differ those more fine grained. This is done by the new flag VIR_DOMAIN_DEF_PARSE_SKIP_ACTIVE_LABEL which is like VIR_DOMAIN_DEF_PARSE_INACTIVE but only for the security labels. So far only set by virt-aa-helper.
Testcase with virt-aa-helper on xml file: virt-aa-helper -d -r -p 0 -u libvirt-<uuid> < your-guest.xml virt-aa-helper: error: could not parse XML virt-aa-helper: error: could not get VM definition (That should have printed a valid apparmor profile)
This should be shortened and clarified (see the other part of the thread). IMHO the root cause is that we parse the active domain XML but the live part of the seclabel is not filled in yet.
Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com> --- src/conf/domain_conf.c | 6 ++++-- src/conf/domain_conf.h | 3 +++ src/security/virt-aa-helper.c | 1 + 3 files changed, 8 insertions(+), 2 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 03506cb..9eb7883 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -6626,7 +6626,8 @@ virSecurityLabelDefParseXML(xmlXPathContextPtr ctxt, * if the 'live' VM XML is requested */ if (seclabel->type == VIR_DOMAIN_SECLABEL_STATIC || - (!(flags & VIR_DOMAIN_DEF_PARSE_INACTIVE) && + (!(flags & (VIR_DOMAIN_DEF_PARSE_SKIP_ACTIVE_LABEL | + VIR_DOMAIN_DEF_PARSE_INACTIVE)) && seclabel->type != VIR_DOMAIN_SECLABEL_NONE)) { p = virXPathStringLimit("string(./label[1])", VIR_SECURITY_LABEL_BUFLEN-1, ctxt); @@ -6642,7 +6643,8 @@ virSecurityLabelDefParseXML(xmlXPathContextPtr ctxt,
/* Only parse imagelabel, if requested live XML with relabeling */ if (seclabel->relabel && - (!(flags & VIR_DOMAIN_DEF_PARSE_INACTIVE) && + (!(flags & (VIR_DOMAIN_DEF_PARSE_SKIP_ACTIVE_LABEL | + VIR_DOMAIN_DEF_PARSE_INACTIVE)) && seclabel->type != VIR_DOMAIN_SECLABEL_NONE)) { p = virXPathStringLimit("string(./imagelabel[1])", VIR_SECURITY_LABEL_BUFLEN-1, ctxt); diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 24aa79c..90693c6 100644 --- a/src/conf/domain_conf.hp +++ b/src/conf/domain_conf.h @@ -2657,6 +2657,9 @@ 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, + /* in regard to security labels, skip parts of the XML that would only be + * present in an active libvirt XML. */ + VIR_DOMAIN_DEF_PARSE_SKIP_ACTIVE_LABEL = 1 << 11,
/* skip parsing of seclabel */ VIR_DOMAIN_DEF_PARSE_SKIP_SECLABEL = 1 << 11, is IMHO shorter and I would then change the code to skip the whole seclabel parsing since it's of no need for virt-aa-helper. Another possibility is to not introduce a new flag but filter out seclabels in virt-aa-helper before parsing the XML without cluttering domain_conf.c even more for this special case. Cheers, -- Guido
} virDomainDefParseFlags;
typedef enum { diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c index 77eeaff..0ca4c83 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_ACTIVE_LABEL | VIR_DOMAIN_DEF_PARSE_SKIP_VALIDATE);
if (ctl->def == NULL) { -- 2.7.4
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Mon, Nov 21, 2016 at 9:03 AM, Guido Günther <agx@sigxcpu.org> wrote:
This should be shortened and clarified (see the other part of the thread). IMHO the root cause is that we parse the active domain XML but the live part of the seclabel is not filled in yet.
Ok, reasonable to keep the actual commit slimmed down after the discussion is done. Will be shortened on the next revision. I also have rewritten the steps to reproduce to be more straight forward. Let me know if you would like those also out of the commit messages scope. [...]
+ VIR_DOMAIN_DEF_PARSE_SKIP_ACTIVE_LABEL = 1 << 11,
/* skip parsing of seclabel */ VIR_DOMAIN_DEF_PARSE_SKIP_SECLABEL = 1 << 11,
is IMHO shorter and I would then change the code to skip the whole seclabel parsing since it's of no need for virt-aa-helper.
I agree that this shorter naming is better. Will do so on the next revision I submit later today.
Another possibility is to not introduce a new flag but filter out seclabels in virt-aa-helper before parsing the XML without cluttering domain_conf.c even more for this special case.
I liked the idea but failed to implement it this way - I guess due to my lack of experience on libxml (or virXML) functions. A version that felt to be "almost there" based on an Xpath can be found here: http://paste.ubuntu.com/23511691/ Most of the complexity is the back and forth of conversion to get it back into the string and not the actual stripping. If it really is close, feedback is welcome - currently it just doesn't strip anything while the same xpath string does work as intended on xmllint.
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@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)) { + 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
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@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
On Tue, Nov 22, 2016 at 8:01 AM, Guido Günther <agx@sigxcpu.org> wrote:
This looks good to me (and looks more terse now) . It would be great to have another review though.
No other eye looking at it over the last week :-/ Therefore here another ping to the List. As the common apparmor users usually are Debian (~you), Ubuntu (~me), and Suse I added the most common recent contributors to Suse rpms to CC. (As found in the changelogs linked from on https://build.opensuse.org/package/revisions/openSUSE:Factory/libvirt) Maybe that increases the chance for an ack or a renewed discussion. -- Christian Ehrhardt Software Engineer, Ubuntu Server Canonical Ltd
Hi Christian, On Mon, Nov 28, 2016 at 01:29:02PM +0100, Christian Ehrhardt wrote:
On Tue, Nov 22, 2016 at 8:01 AM, Guido Günther <agx@sigxcpu.org> wrote:
This looks good to me (and looks more terse now) . It would be great to have another review though.
No other eye looking at it over the last week :-/ Therefore here another ping to the List.
As the common apparmor users usually are Debian (~you), Ubuntu (~me), and Suse I added the most common recent contributors to Suse rpms to CC. (As found in the changelogs linked from on https://build.opensuse.org/package/revisions/openSUSE:Factory/libvirt) Maybe that increases the chance for an ack or a renewed discussion.
Lets give it a few more days and I push it if no one else objects. Cheers, -- Guido
On 11/28/2016 05:29 AM, Christian Ehrhardt wrote:
On Tue, Nov 22, 2016 at 8:01 AM, Guido Günther <agx@sigxcpu.org <mailto:agx@sigxcpu.org>> wrote:
This looks good to me (and looks more terse now) . It would be great to have another review though.
No other eye looking at it over the last week :-/ Therefore here another ping to the List.
As the common apparmor users usually are Debian (~you), Ubuntu (~me), and Suse I added the most common recent contributors to Suse rpms to CC.
I've fixed the occasional bug in this area of code, but am certainly no expert. That said, your V2 LGTM. Regards, Jim
Hi Christian, Sorry for being sitting on that one. Your v2 looks good to me. ACK from me. -- Cedric On Mon, 2016-11-28 at 13:29 +0100, Christian Ehrhardt wrote:
On Tue, Nov 22, 2016 at 8:01 AM, Guido Günther <agx@sigxcpu.org> wrote:
This looks good to me (and looks more terse now) . It would be great to have another review though.
No other eye looking at it over the last week :-/ Therefore here another ping to the List.
As the common apparmor users usually are Debian (~you), Ubuntu (~me), and Suse I added the most common recent contributors to Suse rpms to CC. (As found in the changelogs linked from on https://build.opensuse.org/package/revisions/openSUSE:Factory/libvirt) Maybe that increases the chance for an ack or a renewed discussion.
-- Christian Ehrhardt Software Engineer, Ubuntu Server Canonical Ltd
On Mon, Nov 28, 2016 at 01:29:02PM +0100, Christian Ehrhardt wrote:
On Tue, Nov 22, 2016 at 8:01 AM, Guido Günther <agx@sigxcpu.org> wrote:
This looks good to me (and looks more terse now) . It would be great to have another review though.
No other eye looking at it over the last week :-/ Therefore here another ping to the List.
As the common apparmor users usually are Debian (~you), Ubuntu (~me), and Suse I added the most common recent contributors to Suse rpms to CC. (As found in the changelogs linked from on https://build.opensuse.org/package/revisions/openSUSE:Factory/libvirt) Maybe that increases the chance for an ack or a renewed discussion.
Pushed now with a slightly tweaked commit message. Thanks! -- Guido
participants (4)
-
Cedric Bosdonnat -
Christian Ehrhardt -
Guido Günther -
Jim Fehlig