[libvirt] [PATCH 0/4] apparmor fixes triggered by multi disk snapshots
Hi, the bugs [1][2] that made me debug into this actually only need the last patch (one line), but while coming along I found several opportunities for minor improvements of the apparmor code in libvirt. But that way it became a 4 patch series around apparmor. [1]: https://bugzilla.redhat.com/show_bug.cgi?id=1746684 [2]: https://bugs.launchpad.net/libvirt/+bug/1845506 Christian Ehrhardt (4): virt-aa-helper: clarify command line options apparmor: drop useless call to get_profile_name apparmor: refactor AppArmorSetSecurityImageLabel apparmor: let AppArmorSetSecurityImageLabel append rules src/security/security_apparmor.c | 52 +++++++------------------------- src/security/virt-aa-helper.c | 14 +++++---- 2 files changed, 19 insertions(+), 47 deletions(-) -- 2.23.0
While only used internally from libvirt the options still are misleading enough to cause issues every now and then. Group modes, options and an adding extra file and extend the wording of the latter which had the biggest lack of clarity. Both add a file to the end of the rules, but one re-generates the rules from XML and the other keeps the existing rules as-is not considering the XML content. Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com> --- src/security/virt-aa-helper.c | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c index 9157411133..e0c72e1b9c 100644 --- a/src/security/virt-aa-helper.c +++ b/src/security/virt-aa-helper.c @@ -95,18 +95,20 @@ vahDeinit(vahControl * ctl) static void vah_usage(void) { - printf(_("\n%s [options] [< def.xml]\n\n" - " Options:\n" + printf(_("\n%s mode [options] [extra file] [< def.xml]\n\n" + " Modes:\n" " -a | --add load profile\n" " -c | --create create profile from template\n" - " -d | --dryrun dry run\n" " -D | --delete unload and delete profile\n" - " -f | --add-file <file> add file to profile\n" - " -F | --append-file <file> append file to profile\n" " -r | --replace reload profile\n" " -R | --remove unload profile\n" - " -h | --help this help\n" + " Options:\n" + " -d | --dryrun dry run\n" " -u | --uuid <uuid> uuid (profile name)\n" + " -h | --help this help\n" + " Extra File:\n" + " -f | --add-file <file> add file to a profile generated from XML\n" + " -F | --append-file <file> append file to an existing profile\n" "\n"), progname); puts(_("This command is intended to be used by libvirtd " -- 2.23.0
On Wed, 16 Oct 2019, Christian Ehrhardt wrote:
While only used internally from libvirt the options still are misleading enough to cause issues every now and then. Group modes, options and an adding extra file and extend the wording of the latter which had the biggest lack of clarity. Both add a file to the end of the rules, but one re-generates the rules from XML and the other keeps the existing rules as-is not considering the XML content.
Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com> --- src/security/virt-aa-helper.c | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-)
diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c index 9157411133..e0c72e1b9c 100644 --- a/src/security/virt-aa-helper.c +++ b/src/security/virt-aa-helper.c @@ -95,18 +95,20 @@ vahDeinit(vahControl * ctl) static void vah_usage(void) { - printf(_("\n%s [options] [< def.xml]\n\n" - " Options:\n" + printf(_("\n%s mode [options] [extra file] [< def.xml]\n\n" + " Modes:\n" " -a | --add load profile\n" " -c | --create create profile from template\n" - " -d | --dryrun dry run\n" " -D | --delete unload and delete profile\n" - " -f | --add-file <file> add file to profile\n" - " -F | --append-file <file> append file to profile\n" " -r | --replace reload profile\n" " -R | --remove unload profile\n" - " -h | --help this help\n" + " Options:\n" + " -d | --dryrun dry run\n" " -u | --uuid <uuid> uuid (profile name)\n" + " -h | --help this help\n" + " Extra File:\n" + " -f | --add-file <file> add file to a profile generated from XML\n" + " -F | --append-file <file> append file to an existing profile\n" "\n"), progname);
puts(_("This command is intended to be used by libvirtd "
This LGTM -- Jamie Strandboge | http://www.canonical.com
reload_profile calls get_profile_name for no particular gain, lets remove that call. The string isn't used in that function later on and not registered/passed anywhere. It can only fail if it either can't allocate or if the virDomainDefPtr would have no uuid set (which isn't allowed). Thereby the only "check" it really provides is if it can allocate the string to then free it again. This was initially added in [1] when the code was still in AppArmorRestoreSecurityImageLabel (later moved) and even back then had no further effect than described above. [1]: https://libvirt.org/git/?p=libvirt.git;a=blob;f=src/security/security_apparm... Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com> --- src/security/security_apparmor.c | 14 ++------------ 1 file changed, 2 insertions(+), 12 deletions(-) diff --git a/src/security/security_apparmor.c b/src/security/security_apparmor.c index 75203cc43a..691833eb4b 100644 --- a/src/security/security_apparmor.c +++ b/src/security/security_apparmor.c @@ -282,17 +282,12 @@ reload_profile(virSecurityManagerPtr mgr, const char *fn, bool append) { - int rc = -1; - char *profile_name = NULL; virSecurityLabelDefPtr secdef = virDomainDefGetSecurityLabelDef( def, SECURITY_APPARMOR_NAME); if (!secdef || !secdef->relabel) return 0; - if ((profile_name = get_profile_name(def)) == NULL) - return rc; - /* Update the profile only if it is loaded */ if (profile_loaded(secdef->imagelabel) >= 0) { if (load_profile(mgr, secdef->imagelabel, def, fn, append) < 0) { @@ -300,15 +295,10 @@ reload_profile(virSecurityManagerPtr mgr, _("cannot update AppArmor profile " "\'%s\'"), secdef->imagelabel); - goto cleanup; + return -1; } } - - rc = 0; - cleanup: - VIR_FREE(profile_name); - - return rc; + return 0; } static int -- 2.23.0
On Wed, 16 Oct 2019, Christian Ehrhardt wrote:
reload_profile calls get_profile_name for no particular gain, lets remove that call. The string isn't used in that function later on and not registered/passed anywhere.
It can only fail if it either can't allocate or if the virDomainDefPtr would have no uuid set (which isn't allowed).
Thereby the only "check" it really provides is if it can allocate the string to then free it again.
This was initially added in [1] when the code was still in AppArmorRestoreSecurityImageLabel (later moved) and even back then had no further effect than described above.
[1]: https://libvirt.org/git/?p=libvirt.git;a=blob;f=src/security/security_apparm...
Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com> --- src/security/security_apparmor.c | 14 ++------------ 1 file changed, 2 insertions(+), 12 deletions(-)
diff --git a/src/security/security_apparmor.c b/src/security/security_apparmor.c index 75203cc43a..691833eb4b 100644 --- a/src/security/security_apparmor.c +++ b/src/security/security_apparmor.c @@ -282,17 +282,12 @@ reload_profile(virSecurityManagerPtr mgr, const char *fn, bool append) { - int rc = -1; - char *profile_name = NULL; virSecurityLabelDefPtr secdef = virDomainDefGetSecurityLabelDef( def, SECURITY_APPARMOR_NAME);
if (!secdef || !secdef->relabel) return 0;
- if ((profile_name = get_profile_name(def)) == NULL) - return rc; - /* Update the profile only if it is loaded */ if (profile_loaded(secdef->imagelabel) >= 0) { if (load_profile(mgr, secdef->imagelabel, def, fn, append) < 0) { @@ -300,15 +295,10 @@ reload_profile(virSecurityManagerPtr mgr, _("cannot update AppArmor profile " "\'%s\'"), secdef->imagelabel); - goto cleanup; + return -1; } } - - rc = 0; - cleanup: - VIR_FREE(profile_name); - - return rc; + return 0; }
static int
LGTM. I don't recall why this was there initially but guessing it was obviated by a refactor at some point (perhaps before I initially submitted). -- Jamie Strandboge | http://www.canonical.com
A lot of the code in AppArmorSetSecurityImageLabel is a duplicate of what is in reload_profile, this refactors AppArmorSetSecurityImageLabel to use reload_profile instead. Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com> --- src/security/security_apparmor.c | 38 ++++++++------------------------ 1 file changed, 9 insertions(+), 29 deletions(-) diff --git a/src/security/security_apparmor.c b/src/security/security_apparmor.c index 691833eb4b..320d69e52a 100644 --- a/src/security/security_apparmor.c +++ b/src/security/security_apparmor.c @@ -792,8 +792,6 @@ AppArmorSetSecurityImageLabel(virSecurityManagerPtr mgr, virStorageSourcePtr src, virSecurityDomainImageLabelFlags flags G_GNUC_UNUSED) { - int rc = -1; - char *profile_name = NULL; virSecurityLabelDefPtr secdef; if (!src->path || !virStorageSourceIsLocalStorage(src)) @@ -803,36 +801,18 @@ AppArmorSetSecurityImageLabel(virSecurityManagerPtr mgr, if (!secdef || !secdef->relabel) return 0; - if (secdef->imagelabel) { - /* if the device doesn't exist, error out */ - if (!virFileExists(src->path)) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("\'%s\' does not exist"), - src->path); - return -1; - } - - if ((profile_name = get_profile_name(def)) == NULL) - return -1; + if (!secdef->imagelabel) + return 0; - /* update the profile only if it is loaded */ - if (profile_loaded(secdef->imagelabel) >= 0) { - if (load_profile(mgr, secdef->imagelabel, def, - src->path, false) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("cannot update AppArmor profile " - "\'%s\'"), - secdef->imagelabel); - goto cleanup; - } - } + /* if the device doesn't exist, error out */ + if (!virFileExists(src->path)) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("\'%s\' does not exist"), + src->path); + return -1; } - rc = 0; - cleanup: - VIR_FREE(profile_name); - - return rc; + return reload_profile(mgr, def, src->path, false); } static int -- 2.23.0
On Wed, 16 Oct 2019, Christian Ehrhardt wrote:
A lot of the code in AppArmorSetSecurityImageLabel is a duplicate of what is in reload_profile, this refactors AppArmorSetSecurityImageLabel to use reload_profile instead.
Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com> --- src/security/security_apparmor.c | 38 ++++++++------------------------ 1 file changed, 9 insertions(+), 29 deletions(-)
diff --git a/src/security/security_apparmor.c b/src/security/security_apparmor.c index 691833eb4b..320d69e52a 100644 --- a/src/security/security_apparmor.c +++ b/src/security/security_apparmor.c @@ -792,8 +792,6 @@ AppArmorSetSecurityImageLabel(virSecurityManagerPtr mgr, virStorageSourcePtr src, virSecurityDomainImageLabelFlags flags G_GNUC_UNUSED) { - int rc = -1; - char *profile_name = NULL; virSecurityLabelDefPtr secdef;
if (!src->path || !virStorageSourceIsLocalStorage(src)) @@ -803,36 +801,18 @@ AppArmorSetSecurityImageLabel(virSecurityManagerPtr mgr, if (!secdef || !secdef->relabel) return 0;
- if (secdef->imagelabel) { - /* if the device doesn't exist, error out */ - if (!virFileExists(src->path)) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("\'%s\' does not exist"), - src->path); - return -1; - } - - if ((profile_name = get_profile_name(def)) == NULL) - return -1; + if (!secdef->imagelabel) + return 0;
- /* update the profile only if it is loaded */ - if (profile_loaded(secdef->imagelabel) >= 0) { - if (load_profile(mgr, secdef->imagelabel, def, - src->path, false) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("cannot update AppArmor profile " - "\'%s\'"), - secdef->imagelabel); - goto cleanup; - } - } + /* if the device doesn't exist, error out */ + if (!virFileExists(src->path)) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("\'%s\' does not exist"), + src->path); + return -1; } - rc = 0;
- cleanup: - VIR_FREE(profile_name); - - return rc; + return reload_profile(mgr, def, src->path, false);
The logic of the refactor looks fine, but note by calling reload_profile() here, it will call virDomainDefGetSecurityLabelDef() which we've already done in this function, so we are adding a needless extra call. This doesn't seem to be a particularly expensive call (see src/conf/domain_conf.c), so +1 as is, though you may want to consider trying to get rid of it. -- Jamie Strandboge | http://www.canonical.com
On Tue, Nov 19, 2019 at 9:59 PM Jamie Strandboge <jamie@canonical.com> wrote:
On Wed, 16 Oct 2019, Christian Ehrhardt wrote:
A lot of the code in AppArmorSetSecurityImageLabel is a duplicate of what is in reload_profile, this refactors AppArmorSetSecurityImageLabel to use reload_profile instead.
Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com> --- src/security/security_apparmor.c | 38 ++++++++------------------------ 1 file changed, 9 insertions(+), 29 deletions(-)
diff --git a/src/security/security_apparmor.c b/src/security/security_apparmor.c index 691833eb4b..320d69e52a 100644 --- a/src/security/security_apparmor.c +++ b/src/security/security_apparmor.c @@ -792,8 +792,6 @@ AppArmorSetSecurityImageLabel(virSecurityManagerPtr mgr, virStorageSourcePtr src, virSecurityDomainImageLabelFlags flags G_GNUC_UNUSED) { - int rc = -1; - char *profile_name = NULL; virSecurityLabelDefPtr secdef;
if (!src->path || !virStorageSourceIsLocalStorage(src)) @@ -803,36 +801,18 @@ AppArmorSetSecurityImageLabel(virSecurityManagerPtr mgr, if (!secdef || !secdef->relabel) return 0;
- if (secdef->imagelabel) { - /* if the device doesn't exist, error out */ - if (!virFileExists(src->path)) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("\'%s\' does not exist"), - src->path); - return -1; - } - - if ((profile_name = get_profile_name(def)) == NULL) - return -1; + if (!secdef->imagelabel) + return 0;
- /* update the profile only if it is loaded */ - if (profile_loaded(secdef->imagelabel) >= 0) { - if (load_profile(mgr, secdef->imagelabel, def, - src->path, false) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("cannot update AppArmor profile " - "\'%s\'"), - secdef->imagelabel); - goto cleanup; - } - } + /* if the device doesn't exist, error out */ + if (!virFileExists(src->path)) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("\'%s\' does not exist"), + src->path); + return -1; } - rc = 0;
- cleanup: - VIR_FREE(profile_name); - - return rc; + return reload_profile(mgr, def, src->path, false);
The logic of the refactor looks fine, but note by calling reload_profile() here, it will call virDomainDefGetSecurityLabelDef() which we've already done in this function, so we are adding a needless extra call. This doesn't seem to be a particularly expensive call (see src/conf/domain_conf.c), so +1 as is
The problem here is that AppArmorSetSecurityImageLabel does an additional check on "if (!secdef->imagelabel)" which won't be done in reload_profile. Therefore without changing behavior (and that was the intention) I could only remove the first check Also I looked at the common pattern as an example AppArmorSetFDLabel also checks !secdef->imagelabel on its own before later calling reload_profile.
, though you may want to consider trying to get rid of it.
Yes, might be an opportunity to further streamline later on, but works fine as-is for now. Thanks for the review!
-- Jamie Strandboge | http://www.canonical.com
-- Christian Ehrhardt Staff Engineer, Ubuntu Server Canonical Ltd
There are currently broken use cases, e.g. snapshotting more than one disk at once like: $ virsh snapshot-create-as --domain eoan --disk-only --atomic --diskspec vda,snapshot=no --diskspec vdb,snapshot=no --diskspec vdc,file=/test/disk1.snapshot1.qcow,snapshot=external --diskspec vdd,file=/test/disk2.snapshot1.qcow,snapshot=external The command above will iterate from qemuDomainSnapshotCreateDiskActive and eventually add /test/disk1.snapshot1.qcow first (appears in the rules) to then later add /test/disk2.snapshot1.qcow and while doing so throwing away the former rule causing it to fail. All other calls to (re)load_profile already use append=true when adding rules append=false is only used when restoring rules [1]. Fix this by letting AppArmorSetSecurityImageLabel use append=true as well. Bugs: https://bugs.launchpad.net/libvirt/+bug/1845506 https://bugzilla.redhat.com/show_bug.cgi?id=1746684 [1]: https://bugs.launchpad.net/libvirt/+bug/1845506/comments/13 Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com> --- src/security/security_apparmor.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/security/security_apparmor.c b/src/security/security_apparmor.c index 320d69e52a..4dd7ba20b4 100644 --- a/src/security/security_apparmor.c +++ b/src/security/security_apparmor.c @@ -812,7 +812,7 @@ AppArmorSetSecurityImageLabel(virSecurityManagerPtr mgr, return -1; } - return reload_profile(mgr, def, src->path, false); + return reload_profile(mgr, def, src->path, true); } static int -- 2.23.0
On Wed, 16 Oct 2019, Christian Ehrhardt wrote:
There are currently broken use cases, e.g. snapshotting more than one disk at once like: $ virsh snapshot-create-as --domain eoan --disk-only --atomic --diskspec vda,snapshot=no --diskspec vdb,snapshot=no --diskspec vdc,file=/test/disk1.snapshot1.qcow,snapshot=external --diskspec vdd,file=/test/disk2.snapshot1.qcow,snapshot=external The command above will iterate from qemuDomainSnapshotCreateDiskActive and eventually add /test/disk1.snapshot1.qcow first (appears in the rules) to then later add /test/disk2.snapshot1.qcow and while doing so throwing away the former rule causing it to fail.
All other calls to (re)load_profile already use append=true when adding rules append=false is only used when restoring rules [1].
Fix this by letting AppArmorSetSecurityImageLabel use append=true as well.
Bugs: https://bugs.launchpad.net/libvirt/+bug/1845506 https://bugzilla.redhat.com/show_bug.cgi?id=1746684
[1]: https://bugs.launchpad.net/libvirt/+bug/1845506/comments/13
Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com> --- src/security/security_apparmor.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/security/security_apparmor.c b/src/security/security_apparmor.c index 320d69e52a..4dd7ba20b4 100644 --- a/src/security/security_apparmor.c +++ b/src/security/security_apparmor.c @@ -812,7 +812,7 @@ AppArmorSetSecurityImageLabel(virSecurityManagerPtr mgr, return -1; }
- return reload_profile(mgr, def, src->path, false); + return reload_profile(mgr, def, src->path, true);
src/security/security_manager.c shows that virSecurityManagerSetImageLabel() is called to label 'a storage image with the configured security label'. Based on your report, it seems that in addition to the underlying disk (vda in this case), it is also called for each additional disk (ie, vdc and vdd). While your patch to append makes sense for this scenario, what happens if you update the vm definition to use 'vde' instead of 'vda', is the vda access removed and vde added with vdc and vdd remaining? What if we remove vda and replace it with only vde, are vda, vdc and vdd removed? I fear that making this change will result in scenarios where the rule is (correctly) added, but previous rules are not removed. Can you comment on if this is working correctly? Is it possible to have tests that demonstrate everything is working as intended? -- Jamie Strandboge | http://www.canonical.com
On Tue, Nov 19, 2019 at 10:12 PM Jamie Strandboge <jamie@canonical.com> wrote:
On Wed, 16 Oct 2019, Christian Ehrhardt wrote:
There are currently broken use cases, e.g. snapshotting more than one disk at once like: $ virsh snapshot-create-as --domain eoan --disk-only --atomic --diskspec vda,snapshot=no --diskspec vdb,snapshot=no --diskspec vdc,file=/test/disk1.snapshot1.qcow,snapshot=external --diskspec vdd,file=/test/disk2.snapshot1.qcow,snapshot=external The command above will iterate from qemuDomainSnapshotCreateDiskActive and eventually add /test/disk1.snapshot1.qcow first (appears in the rules) to then later add /test/disk2.snapshot1.qcow and while doing so throwing away the former rule causing it to fail.
All other calls to (re)load_profile already use append=true when adding rules append=false is only used when restoring rules [1].
Fix this by letting AppArmorSetSecurityImageLabel use append=true as well.
Bugs: https://bugs.launchpad.net/libvirt/+bug/1845506 https://bugzilla.redhat.com/show_bug.cgi?id=1746684
[1]: https://bugs.launchpad.net/libvirt/+bug/1845506/comments/13
Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com> --- src/security/security_apparmor.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/security/security_apparmor.c b/src/security/security_apparmor.c index 320d69e52a..4dd7ba20b4 100644 --- a/src/security/security_apparmor.c +++ b/src/security/security_apparmor.c @@ -812,7 +812,7 @@ AppArmorSetSecurityImageLabel(virSecurityManagerPtr mgr, return -1; }
- return reload_profile(mgr, def, src->path, false); + return reload_profile(mgr, def, src->path, true);
src/security/security_manager.c shows that virSecurityManagerSetImageLabel() is called to label 'a storage image with the configured security label'. Based on your report, it seems that in addition to the underlying disk (vda in this case), it is also called for each additional disk (ie, vdc and vdd).
Yes in the "transaction" for e.g. a snapshot on multiple disks there will be calls for both disks before any of them becomes part of the VM-definition. That is where the second call "removes" the first rule and then things fail. To be clear, I didn't add any "disk" in the broken use-case it adds further backing image chain elements to existing disks as part of the snapshot action. So if I snapshot in one shot vdb and vdc it is called for each of those adding the new paths for the respective backing chain that grows. If you ever power cycle that it will be part of the definition and virt-aa-helper resolves backing chains as needed.
While your patch to append makes sense for this scenario, what happens if you update the vm definition to use 'vde' instead of 'vda', is the vda access removed and vde added with vdc and vdd remaining?
Hmm - not sure what exactly you mean, but lets try to break it ... This splits into three major paths to consider: a) adding/removing disks while the guest is off. This isn't interesting as that way it will be part (or not) of the definition when it starts. The guest will get initial rules based on that definition on start, nothing special. b) adding/removing disks at runtime of a guest b1) you can edit a live definition in XML, but that will only add/modify the disk on next start Even if you now add another disk via proper hot-add later the first will not be added as it isn't really part of the active definition yet. b2) you can hot add/remove a disk, those will be properly labelled and added/removed via their labelling calls on that action c) snapshots at runtime of the guest (that was the broken case) As mentioned above it will need to add new elements to the backing-chain c1) snapshot one disk will work, the call to AppArmorSetSecurityImageLabel will add the new rule needed c2) snapshot multiple disks at once then it will fail without the fix here the second call to AppArmorSetSecurityImageLabel will revoke the former rule Sorry, I don't find the " update the vm definition to use 'vde' instead of 'vda'" that you meant in either of the paths a/b/c above. I'll try to reach you on IRC later on to sort this out ...
What if we remove vda and replace it with only vde, are vda, vdc and vdd removed? I fear that making this change will result in scenarios where the rule is (correctly) added, but previous rules are not removed.
Can you comment on if this is working correctly? Is it possible to have tests that demonstrate everything is working as intended?
-- Jamie Strandboge | http://www.canonical.com
-- Christian Ehrhardt Staff Engineer, Ubuntu Server Canonical Ltd
On Wed, Nov 20, 2019 at 3:40 PM Christian Ehrhardt <christian.ehrhardt@canonical.com> wrote:
On Tue, Nov 19, 2019 at 10:12 PM Jamie Strandboge <jamie@canonical.com> wrote:
On Wed, 16 Oct 2019, Christian Ehrhardt wrote:
There are currently broken use cases, e.g. snapshotting more than one disk at once like: $ virsh snapshot-create-as --domain eoan --disk-only --atomic --diskspec vda,snapshot=no --diskspec vdb,snapshot=no --diskspec vdc,file=/test/disk1.snapshot1.qcow,snapshot=external --diskspec vdd,file=/test/disk2.snapshot1.qcow,snapshot=external The command above will iterate from qemuDomainSnapshotCreateDiskActive and eventually add /test/disk1.snapshot1.qcow first (appears in the rules) to then later add /test/disk2.snapshot1.qcow and while doing so throwing away the former rule causing it to fail.
All other calls to (re)load_profile already use append=true when adding rules append=false is only used when restoring rules [1].
Fix this by letting AppArmorSetSecurityImageLabel use append=true as well.
Bugs: https://bugs.launchpad.net/libvirt/+bug/1845506 https://bugzilla.redhat.com/show_bug.cgi?id=1746684
[1]: https://bugs.launchpad.net/libvirt/+bug/1845506/comments/13
Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com> --- src/security/security_apparmor.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/security/security_apparmor.c b/src/security/security_apparmor.c index 320d69e52a..4dd7ba20b4 100644 --- a/src/security/security_apparmor.c +++ b/src/security/security_apparmor.c @@ -812,7 +812,7 @@ AppArmorSetSecurityImageLabel(virSecurityManagerPtr mgr, return -1; }
- return reload_profile(mgr, def, src->path, false); + return reload_profile(mgr, def, src->path, true);
src/security/security_manager.c shows that virSecurityManagerSetImageLabel() is called to label 'a storage image with the configured security label'. Based on your report, it seems that in addition to the underlying disk (vda in this case), it is also called for each additional disk (ie, vdc and vdd).
Yes in the "transaction" for e.g. a snapshot on multiple disks there will be calls for both disks before any of them becomes part of the VM-definition. That is where the second call "removes" the first rule and then things fail.
To be clear, I didn't add any "disk" in the broken use-case it adds further backing image chain elements to existing disks as part of the snapshot action. So if I snapshot in one shot vdb and vdc it is called for each of those adding the new paths for the respective backing chain that grows.
If you ever power cycle that it will be part of the definition and virt-aa-helper resolves backing chains as needed.
While your patch to append makes sense for this scenario, what happens if you update the vm definition to use 'vde' instead of 'vda', is the vda access removed and vde added with vdc and vdd remaining?
Hmm - not sure what exactly you mean, but lets try to break it ...
This splits into three major paths to consider:
a) adding/removing disks while the guest is off. This isn't interesting as that way it will be part (or not) of the definition when it starts. The guest will get initial rules based on that definition on start, nothing special.
b) adding/removing disks at runtime of a guest b1) you can edit a live definition in XML, but that will only add/modify the disk on next start Even if you now add another disk via proper hot-add later the first will not be added as it isn't really part of the active definition yet. b2) you can hot add/remove a disk, those will be properly labelled and added/removed via their labelling calls on that action
c) snapshots at runtime of the guest (that was the broken case) As mentioned above it will need to add new elements to the backing-chain c1) snapshot one disk will work, the call to AppArmorSetSecurityImageLabel will add the new rule needed c2) snapshot multiple disks at once then it will fail without the fix here the second call to AppArmorSetSecurityImageLabel will revoke the former rule
Sorry, I don't find the " update the vm definition to use 'vde' instead of 'vda'" that you meant in either of the paths a/b/c above. I'll try to reach you on IRC later on to sort this out ...
After discussing on IRC (thanks) and understanding that it wasn't so much about the new change in this patch but a general "are rules revoked as expected" I came up with this tests: Setup: vca (root) vdb (helper disk not used here) vdc /var/lib/uvtool/libvirt/images/focal-test2.qcow (present at guest start) vdd /var/lib/uvtool/libvirt/images/focal-test1.qcow (hot-added after boot) Then I'll remove one or the other and ensure that the rules are gone. Furthermore I then will call a snapshot on vda to ensure that it doesn't e.g. add the rule back from some unexpected source. - Initially there was no rule (obviously) - after guest start vdc had a rule - after attaching disk2 it has vdc+vdd rules - after detaching disk1 it has only the vdd rule - after detaching disk2 it had none of the rules - snapshot vda to a new path, new path added no vdc/vdd Thereby the checks you wondered about are done, thanks for helping me to make sure this is fine.
What if we remove vda and replace it with only vde, are vda, vdc and vdd removed? I fear that making this change will result in scenarios where the rule is (correctly) added, but previous rules are not removed.
Can you comment on if this is working correctly? Is it possible to have tests that demonstrate everything is working as intended?
-- Jamie Strandboge | http://www.canonical.com
-- Christian Ehrhardt Staff Engineer, Ubuntu Server Canonical Ltd
-- Christian Ehrhardt Staff Engineer, Ubuntu Server Canonical Ltd
On Wed, 20 Nov 2019, Christian Ehrhardt wrote:
On Wed, Nov 20, 2019 at 3:40 PM Christian Ehrhardt <christian.ehrhardt@canonical.com> wrote:
On Tue, Nov 19, 2019 at 10:12 PM Jamie Strandboge <jamie@canonical.com> wrote:
On Wed, 16 Oct 2019, Christian Ehrhardt wrote:
There are currently broken use cases, e.g. snapshotting more than one disk at once like: $ virsh snapshot-create-as --domain eoan --disk-only --atomic --diskspec vda,snapshot=no --diskspec vdb,snapshot=no --diskspec vdc,file=/test/disk1.snapshot1.qcow,snapshot=external --diskspec vdd,file=/test/disk2.snapshot1.qcow,snapshot=external The command above will iterate from qemuDomainSnapshotCreateDiskActive and eventually add /test/disk1.snapshot1.qcow first (appears in the rules) to then later add /test/disk2.snapshot1.qcow and while doing so throwing away the former rule causing it to fail.
All other calls to (re)load_profile already use append=true when adding rules append=false is only used when restoring rules [1].
Fix this by letting AppArmorSetSecurityImageLabel use append=true as well.
Bugs: https://bugs.launchpad.net/libvirt/+bug/1845506 https://bugzilla.redhat.com/show_bug.cgi?id=1746684
[1]: https://bugs.launchpad.net/libvirt/+bug/1845506/comments/13
Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com> --- src/security/security_apparmor.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/security/security_apparmor.c b/src/security/security_apparmor.c index 320d69e52a..4dd7ba20b4 100644 --- a/src/security/security_apparmor.c +++ b/src/security/security_apparmor.c @@ -812,7 +812,7 @@ AppArmorSetSecurityImageLabel(virSecurityManagerPtr mgr, return -1; }
- return reload_profile(mgr, def, src->path, false); + return reload_profile(mgr, def, src->path, true);
src/security/security_manager.c shows that virSecurityManagerSetImageLabel() is called to label 'a storage image with the configured security label'. Based on your report, it seems that in addition to the underlying disk (vda in this case), it is also called for each additional disk (ie, vdc and vdd).
Yes in the "transaction" for e.g. a snapshot on multiple disks there will be calls for both disks before any of them becomes part of the VM-definition. That is where the second call "removes" the first rule and then things fail.
To be clear, I didn't add any "disk" in the broken use-case it adds further backing image chain elements to existing disks as part of the snapshot action. So if I snapshot in one shot vdb and vdc it is called for each of those adding the new paths for the respective backing chain that grows.
If you ever power cycle that it will be part of the definition and virt-aa-helper resolves backing chains as needed.
While your patch to append makes sense for this scenario, what happens if you update the vm definition to use 'vde' instead of 'vda', is the vda access removed and vde added with vdc and vdd remaining?
Hmm - not sure what exactly you mean, but lets try to break it ...
This splits into three major paths to consider:
a) adding/removing disks while the guest is off. This isn't interesting as that way it will be part (or not) of the definition when it starts. The guest will get initial rules based on that definition on start, nothing special.
b) adding/removing disks at runtime of a guest b1) you can edit a live definition in XML, but that will only add/modify the disk on next start Even if you now add another disk via proper hot-add later the first will not be added as it isn't really part of the active definition yet. b2) you can hot add/remove a disk, those will be properly labelled and added/removed via their labelling calls on that action
c) snapshots at runtime of the guest (that was the broken case) As mentioned above it will need to add new elements to the backing-chain c1) snapshot one disk will work, the call to AppArmorSetSecurityImageLabel will add the new rule needed c2) snapshot multiple disks at once then it will fail without the fix here the second call to AppArmorSetSecurityImageLabel will revoke the former rule
Sorry, I don't find the " update the vm definition to use 'vde' instead of 'vda'" that you meant in either of the paths a/b/c above. I'll try to reach you on IRC later on to sort this out ...
After discussing on IRC (thanks) and understanding that it wasn't so much about the new change in this patch but a general "are rules revoked as expected" I came up with this tests:
Setup: vca (root) vdb (helper disk not used here) vdc /var/lib/uvtool/libvirt/images/focal-test2.qcow (present at guest start) vdd /var/lib/uvtool/libvirt/images/focal-test1.qcow (hot-added after boot)
Then I'll remove one or the other and ensure that the rules are gone. Furthermore I then will call a snapshot on vda to ensure that it doesn't e.g. add the rule back from some unexpected source.
- Initially there was no rule (obviously) - after guest start vdc had a rule - after attaching disk2 it has vdc+vdd rules - after detaching disk1 it has only the vdd rule - after detaching disk2 it had none of the rules - snapshot vda to a new path, new path added no vdc/vdd
Thereby the checks you wondered about are done, thanks for helping me to make sure this is fine.
Thanks! LGTM. Feel free to add some language to the commit message to summarize your testing, but not blocking on that. -- Jamie Strandboge | http://www.canonical.com
On 10/16/19 10:27 AM, Christian Ehrhardt wrote:
Hi, the bugs [1][2] that made me debug into this actually only need the last patch (one line), but while coming along I found several opportunities for minor improvements of the apparmor code in libvirt. But that way it became a 4 patch series around apparmor.
[1]: https://bugzilla.redhat.com/show_bug.cgi?id=1746684 [2]: https://bugs.launchpad.net/libvirt/+bug/1845506
Christian Ehrhardt (4): virt-aa-helper: clarify command line options apparmor: drop useless call to get_profile_name apparmor: refactor AppArmorSetSecurityImageLabel apparmor: let AppArmorSetSecurityImageLabel append rules
src/security/security_apparmor.c | 52 +++++++------------------------- src/security/virt-aa-helper.c | 14 +++++---- 2 files changed, 19 insertions(+), 47 deletions(-)
Not runtime tested, but: Reviewed-by: Cole Robinson <crobinso@redhat.com> - Cole
On Thu, Nov 14, 2019 at 1:23 AM Cole Robinson <crobinso@redhat.com> wrote:
On 10/16/19 10:27 AM, Christian Ehrhardt wrote:
Hi, the bugs [1][2] that made me debug into this actually only need the last patch (one line), but while coming along I found several opportunities for minor improvements of the apparmor code in libvirt. But that way it became a 4 patch series around apparmor.
[1]: https://bugzilla.redhat.com/show_bug.cgi?id=1746684 [2]: https://bugs.launchpad.net/libvirt/+bug/1845506
Christian Ehrhardt (4): virt-aa-helper: clarify command line options apparmor: drop useless call to get_profile_name apparmor: refactor AppArmorSetSecurityImageLabel apparmor: let AppArmorSetSecurityImageLabel append rules
src/security/security_apparmor.c | 52 +++++++------------------------- src/security/virt-aa-helper.c | 14 +++++---- 2 files changed, 19 insertions(+), 47 deletions(-)
Not runtime tested, but:
Reviewed-by: Cole Robinson <crobinso@redhat.com>
Thank you, I added the tag in my local series, but that is not worth a v2 submission. Before pushing I'm still waiting for someone with apparmor experience to take a look, just to be somewhat on the safe side.
- Cole
On Thu, Nov 14, 2019 at 12:23 PM Christian Ehrhardt <christian.ehrhardt@canonical.com> wrote:
On Thu, Nov 14, 2019 at 1:23 AM Cole Robinson <crobinso@redhat.com> wrote:
On 10/16/19 10:27 AM, Christian Ehrhardt wrote:
Hi, the bugs [1][2] that made me debug into this actually only need the last patch (one line), but while coming along I found several opportunities for minor improvements of the apparmor code in libvirt. But that way it became a 4 patch series around apparmor.
[1]: https://bugzilla.redhat.com/show_bug.cgi?id=1746684 [2]: https://bugs.launchpad.net/libvirt/+bug/1845506
Christian Ehrhardt (4): virt-aa-helper: clarify command line options apparmor: drop useless call to get_profile_name apparmor: refactor AppArmorSetSecurityImageLabel apparmor: let AppArmorSetSecurityImageLabel append rules
src/security/security_apparmor.c | 52 +++++++------------------------- src/security/virt-aa-helper.c | 14 +++++---- 2 files changed, 19 insertions(+), 47 deletions(-)
Not runtime tested, but:
Reviewed-by: Cole Robinson <crobinso@redhat.com>
Thank you, I added the tag in my local series, but that is not worth a v2 submission. Before pushing I'm still waiting for someone with apparmor experience to take a look, just to be somewhat on the safe side.
Thanks Jamie for also adding Review and Discusions. Pushing this with your Ack/Review tags after a final build/check
- Cole
-- Christian Ehrhardt Staff Engineer, Ubuntu Server Canonical Ltd
participants (3)
-
Christian Ehrhardt -
Cole Robinson -
Jamie Strandboge