libvirt restore exactly the old ownership of images

Bug #691590 reported by C de-Avillez
54
This bug affects 8 people
Affects Status Importance Assigned to Milestone
libvirt (Ubuntu)
Fix Released
Medium
Christian Ehrhardt 
Focal
Fix Released
Medium
Unassigned

Bug Description

[Impact]

 * A rather old bug which could have been solved much sooner. Attr support
   was disabled way back in time for triggering some test issues. Since
   then the test issues and also many more rough edges of ATTR support have
   been fixed.

 * This change shall enable attr support again which allows libvirt to
   remember and carry ownership information on image files as extended
   attributes.

[Test Case]

 * Prepare a guest that you can start/stop e.g. with uvtool-libvirt
 * Own the image by anything other than root:root
 * Start the guest (ownership will change to the user the guest runs as)
 * Stop the guest:
   - fail: will set root:root to the images effectively stealing them
   - correct: remembers the old ownership and restores that

[Regression Potential]

 * This mostly influences DAC control of files, which is exactly what we
   want to fix. There are a few lifecycle tasks which now also have to
   carry labels e.g. on any image handling. I don't expect regressions, but
   this is the place to look out for.
 * The behavior on File systems unable to XATTR matches that of the
   formerly disable feature, so on those cases where it has no positive
   change it will have no change at all.

[Other Info]

 * Technically we could backport this into all releases, but while I find
   it right to fix in Focal OTOH Bionic and even more so Xenial really are
   even "more stable" after their time in the field. Users either have
   adapted already or even rely/expect the semi-broken behavior. Therefore
   this is only targetting Focal intentionally.

 * (very) worst case one can set the FS the images are on to "nouser_xattr"
   as mount option.

---

Natty (and it was also the same on Maverick, IIRC).

When you assign an ISO to a VM, libvirt will take over onwership of the ISO. This creates problems if the ISO is updated.

For example, I am daily updating the Natty server ISOs, and running tests on them via KVM (all automated). The ISO updates will fail because libvirt chowns them.

I see no reason for this: libvirt only needs the ISO as input.

WORKAROUND:
  edit /etc/libvirt/qemu.conf, change 'dynamic_ownership = 0', restart qemu/KVM.

Related branches

Revision history for this message
In , Cristian (cristian-redhat-bugs) wrote :

Description of problem:
When I use an install ISO image labeled public_content_t, virt-manager will relabel it as virt_content_t without any warnings. It will also change its owner and group to qemu. It should allow virtual machines to read those files (which might also be shared via http, samba or nfs).

Version-Release number of selected component (if applicable):
virt-manager-0.8.2-1.fc12.noarch.rpm

How reproducible:
Every time.

Steps to Reproduce:
1. Create a new VM.
2. Select an ISO image labeled public_content_t.
3. Continue all the steps until the machine is started.

Actual results:
The ISO image will be labeled virt_content_t and its owner and group will be changed to qemu.

Expected results:
A warning should be displayed if the permissions of the file need to be changed or even better allow the virtual machine to read public_content_t files.

Additional info:
I'm also using the following packages:
libvirt-0.7.1-15.fc12.x86_64.rpm
selinux-policy-targeted-3.6.32-89.fc12.noarch.rpm

A related RFE is bug #568933.

Revision history for this message
In , Cole (cole-redhat-bugs) wrote :

Libvirt is doing the relabeling here. Reassigning.

Revision history for this message
In , Cristian (cristian-redhat-bugs) wrote :

It's still present in libvirt-0.7.7-4.fc13.x86_64.rpm.

Revision history for this message
In , Cristian (cristian-redhat-bugs) wrote :

It's still present in libvirt-0.8.2-1.fc13.x86_64.rpm.

C de-Avillez (hggdh2)
tags: added: natty qa
C de-Avillez (hggdh2)
tags: added: iso-testing
Revision history for this message
Serge Hallyn (serge-hallyn) wrote :

I don't think it would be safe at any rate to have the ISO images be
written to while kvm is reading them. Would it be ok to work around
this another way?

Perhaps the right way to update the ISOs is:

 cp orig.iso new.iso
 rsync -Pv mirror://updated_iso.iso new.iso
 rm orig.iso
 mv new.iso orig.iso

This way you can still minimize network traffic, while syncing to a
temporary copy. After the 'rm orig.iso', libvirt and kvm will
continue to use the original, deleted file, until they close it.
Then, the next time they open 'orig.iso', they'll get the new file.

Would that be conceivable with your mirroring setup?

Changed in libvirt (Ubuntu):
status: New → Incomplete
Revision history for this message
C de-Avillez (hggdh2) wrote :

Yes, this would work (as long as the process doing this move owns the directory -- otherwise it is still an error 13). The whole point, though, is that libvirt does not need to take ownership of a *read-only* file.

At least it could revert the ownership when the VM is closed, if you want to protect against an ISO update while the ISO is in use by libvirt. Or use flock, or something. But this (update-while-somebody-is-using) is a common issue on *IX, and still we do not see ownership being unilaterally changed.

Of course, we can also bypass by using 'sudo', but this would break the least privilege principle.

Changed in libvirt (Ubuntu):
status: Incomplete → New
Revision history for this message
Clint Byrum (clint-fewbar) wrote :

Serge, from what I understand of rsync, it never writes directly to the destination file, it will create a temporary hidden file and write to that, then unlink/rename when the transfer is complete.

So the steps can just be

rsync rsync://mirror/file.iso orig.iso

it won't interfere at all with anything that has the .iso opened.. it will be like any other unlinked open file.

Revision history for this message
Jamie Strandboge (jdstrand) wrote :

This whole bug is about libvirt's DAC security driver. It will chown files to the user that kvm runs as. On Ubuntu, this is the libvirt-qemu:kvm user (adjustable via /etc/libvirt/qemu.conf). If you look at the ISO file, its ownership should have been changed to this user. The DAC security driver cannot be disabled like the other security drivers (eg AppArmor and SELinux), but is instead either used alone or with one other security driver (AppArmor on Ubuntu).

I believe that if libvirt is configured to run kvm as root, then the DAC driver will not chown files (because it doesn't have to-- with DAC root can read anything). This was the case on Lucid iirc. As a workaround, you should be able to configure /etc/libvirt/qemu.conf to use:
user = "root"
group = "root"

and the problem should go away (not tested on maverick or natty libvirt). Because kvm is still confined by AppArmor in this configuration, the security stance is not greatly diminished. This was the default in Lucid.

I've not looked at how well libvirt handles chowning files, but I imagine one reason why it works the way it does is if libvirt chowned back to the user, this is a potential race condition and security issue-- ie, libvirt chowns the ISO to libvirt-qemu:kvm, then starts the machine. Now I hard link the ISO to /etc/shadow and shutdown the machine. libvirt chowns /etc/shadow to my user and group. Granted, members of the libvirtd group (ie access to qemu:///system) are considered privileged anyway (they have access to raw disks among other things), but with the above described scenario, it is far too easy to escalate privileges. Chowning to libvirt-qemu:kvm is potentially problematic as well, but the hard link to /etc/shadow is less interesting there since the user isn't libvirt-qemu and the kvm group membership doesn't gain you as much (setgid should be stripped on chgrp in Linux, and group writable files are not as common (though there are a few that are interesting)). Natty has some kernel protections that could help here, but they are upstream and upstream libvirt would not be able to rely on them being present.

Revision history for this message
C de-Avillez (hggdh2) wrote :

@Clint: zsync does the same (writes the updated file to a temp, then renames/unlinks/whatever -- did not check the source).

@Jamie: I just tried with qemu.conf setting user/group to root -- the ISO gets chown-ed to root:root, 0600. So, no dice here. Nevertheless, my whole point is it does not make much sense, security-wise, to chown a read-only file: it is an ISO image, and it is mounted on the CDROM:

        <devices>
                <disk type='file' device='disk'>
                        <driver name='qemu' type='qcow2'/>
                        <source file='/var/lib/ubuntu-server-iso-testing/tests/819f2dd9-0041-48aa-8dfe-c5899f2cafe7/disk1.qcow2'/>
                        <target dev='vda' bus='virtio'/>
                </disk>
                <disk type='file' device='cdrom'>
                        <source file='/var/lib/ubuntu-server-iso-testing/isos/ubuntu-server/natty-server-amd64.iso'/>
                        <target dev='hdc'/><readonly/>
                </disk>
(...)
        </devices>

If the file is never chown-ed to libvirt:kvm/whatever, then there is no race -- the file will keep the current ownership. Obviously, this does not apply to the qcow2 disc -- there is a clear exposure there. Now, why does libvirt in user-mode also chown the discs? I would expect the user-mode to run under the control (and ownership, at least for the disc images) of the effective userId that started the VM.

Revision history for this message
C de-Avillez (hggdh2) wrote :

A correction on the above "I just tried with qemu.conf setting user/group to root -- the ISO gets chown-ed to root:root, 0600.": Actually, the permissions are kept as they were.

Revision history for this message
Serge Hallyn (serge-hallyn) wrote :

So does that suffice for your needs?

Revision history for this message
C de-Avillez (hggdh2) wrote :

Actually, no... theonly change is the owner got to be root, from libvirt. I still am not convinced a read-only ISO has to be chown-ed to the libvirt account.

Revision history for this message
Serge Hallyn (serge-hallyn) wrote :

I intend to write a patch to make this behavior an option, and send it to the libvirt list for comment.

Changed in libvirt (Ubuntu):
status: New → Triaged
importance: Undecided → Low
Revision history for this message
Serge Hallyn (serge-hallyn) wrote :
Revision history for this message
Serge Hallyn (serge-hallyn) wrote :

A package with the proposed fix is available for natty in ppa:serge-hallyn/virt. If this does what you need, then we can proceed to talk to the libvirt community.

Revision history for this message
C de-Avillez (hggdh2) wrote :

Thank you, Serge. Testing it now.

Revision history for this message
C de-Avillez (hggdh2) wrote :

It seems the ISOs are hosed right now, I get a sudden reboot in the basic package install. But -- as far as this bug is concerned -- the ISOs ownership are maintained on the original owner.

Perfect, Serge.

Revision history for this message
Serge Hallyn (serge-hallyn) wrote :

Re-verified the bug and the patch, and sent the patch to the upstream mailing list:

https://www.redhat.com/archives/libvir-list/2011-September/msg00458.html

If upstream rejects this, then I will mark the bug wontfix.

Revision history for this message
Serge Hallyn (serge-hallyn) wrote :

See https://www.redhat.com/archives/libvir-list/2011-October/msg00104.html and https://www.redhat.com/archives/libvir-list/2011-October/msg00110.html for the upstream response. The first message describes the proper fix (switching from chown to acls in the dac security code). The second suggests using a readonly mount for the isos.

Is it possible to use a read-only bind mount of the mirror directory for your libvirt VMs? You can either mount it elsewhere, or else have /etc/init/libvirt unshare a new mount namespace and remount the mirror directory read-only in place before starting libvirtd.

Revision history for this message
C de-Avillez (hggdh2) wrote :

yes, I can set a readonly mount. Will have it set in a few. Thank you, Serge.

Changed in libvirt (Ubuntu):
status: Triaged → Won't Fix
Revision history for this message
In , Eric (eric-redhat-bugs) wrote :

Done with this commit in 0.9.9:
commit b43432931aef92325920953ff92beabfbe5224c8
Author: Eric Blake <email address hidden>
Date: Thu Dec 22 17:47:50 2011 -0700

    seclabel: allow a seclabel override on a disk src

    Implement the parsing and formatting of the XML addition of
    the previous commit. The new XML doesn't affect qemu command
    line, so we can now test round-trip XML->memory->XML handling.

    I chose to reuse the existing structure, even though per-device
    override doesn't use all of those fields, rather than create a
    new structure, in order to reuse more code.

    * src/conf/domain_conf.h (_virDomainDiskDef): Add seclabel member.
    * src/conf/domain_conf.c (virDomainDiskDefFree): Free it.
    (virSecurityLabelDefFree): New function.
    (virDomainDiskDefFormat): Print it.
    (virSecurityLabelDefFormat): Reduce output if model not present.
    (virDomainDiskDefParseXML): Alter signature, and parse seclabel.
    (virSecurityLabelDefParseXML): Split...
    (virSecurityLabelDefParseXMLHelper): ...into new helper.
    (virDomainDeviceDefParse, virDomainDefParseXML): Update callers.
    * tests/qemuxml2argvdata/qemuxml2argv-seclabel-dynamic-override.args:
    New file.
    * tests/qemuxml2xmltest.c (mymain): Enhance test.
    * tests/qemuxml2argvtest.c (mymain): Likewise.

C de-Avillez (hggdh2)
description: updated
C de-Avillez (hggdh2)
description: updated
Revision history for this message
Phillip Susi (psusi) wrote :

This really feels like a serious security bug. The whole point of running qemu as non root is to prevent it from accessing files that you haven't given it permission to. By blindly chowning files to the qemu user, you allow for the user who is given permission to run virtual machines to start one with direct access to your /boot partition and hack the host system.

Even if you do wish to bypass permissions and allow the vm access to whatever file a vm admin has configured it to ( under the assumption that they are trusted as if root ), you don't do that with the sledge hammer of chowning the file; you open the file while still root, and pass the open file descriptor to qemu.

Really, it should assume the identity of the user who is requesting that the vm be started and open the file as them rather than root, thus restricting access only to the files that user has access to, but that may be considered a separate issue.

For now I will focus on at least getting rid of the bad behavior of permanently chowning files.

Changed in libvirt (Ubuntu):
assignee: nobody → Phillip Susi (psusi)
status: Won't Fix → Triaged
Changed in libvirt:
importance: Unknown → Medium
status: Unknown → Fix Released
Revision history for this message
Soren Hansen (soren) wrote : Re: libvirt should not take ownership of ISO images

It's exceptionally frustrating.

It's been a while since it was solved upstream such that when a domain is terminated, the image ownership gets restored to what it was when the domain was launched. However, the way it tracks the original ownership is by storing it in xattr's of the image... but we've been passing --without-attr since 2014, due to a test failure on the Debian buildd's, and without attr, no ownership memory.

Back when it was added I guess it didn't make much of a difference, but now it's preventing this bug from being solved. The package builds just fine on our buildd's (at least in my PPA).

Attached debdiff to fix this.

Revision history for this message
Christian Ehrhardt  (paelzer) wrote :

Interesting indeed.
I remember the patch to restore file ownership, and at least in the cases I looked at it worked fine.
I'm using the normal Ubuntu libvirt in Focal

ii libvirt-daemon 6.0.0-0ubuntu8.1 amd64 Virtualization daemon

That is built without attr support as Soren outlined, a buildlog is here [1]

But with that build it clearly sets and restores file ownership in the common default case:

$ sudo ls -laF /var/lib/libvirt/images/ubuntu18.04.qcow2; virsh start ubuntu18.04; sleep 30s; sudo ls -laF /var/lib/libvirt/images/ubuntu18.04.qcow2; virsh shutdown ubuntu18.04; sleep 30s; sudo ls -laF /var/lib/libvirt/images/ubuntu18.04.qcow2

-rw------- 1 root root 16108814336 Mai 11 14:34 /var/lib/libvirt/images/ubuntu18.04.qcow2
Domain ubuntu18.04 started

-rw------- 1 libvirt-qemu kvm 16108814336 Jun 22 13:02 /var/lib/libvirt/images/ubuntu18.04.qcow2
Domain ubuntu18.04 is being shutdown

-rw------- 1 root root 16108814336 Jun 22 13:03 /var/lib/libvirt/images/ubuntu18.04.qcow2

Just as I remember, file ownership is restored using a build that had --without-attr.

Config for that is in /etc/libvirt/qemu.conf and defaults to
  #dynamic_ownership = 1

Code wise
 This is around virSecurityDACSetOwnership and virSecurityDACRestoreFileLabelInternal.
 I see the xattr store that you mentioned being used in virSecurityGetRememberedLabel
 which would fill the content as you reported.

VIR_SECURITY_MANAGER_DYNAMIC_OWNERSHIP
VIR_FILE_OPEN_FORCE_OWNER

To be clear - I'm not even against enabling attr support it is in main and should be fine, but we have to resolve why it seems to be important for your case but isn't otherwise.

Ok, maybe it generally is only ok because in 99% of the cases the default of root:root was what it began with.
Changing my image and retrying.

$ sudo ls -laF /var/lib/libvirt/images/ubuntu18.04.qcow2; virsh start ubuntu18.04; sleep 30s; sudo ls -laF /var/lib/libvirt/images/ubuntu18.04.qcow2; virsh shutdown ubuntu18.04; sleep 30s; sudo ls -laF /var/lib/libvirt/images/ubuntu18.04.qcow2
-rw------- 1 paelzer root 16108814336 Jun 22 13:27 /var/lib/libvirt/images/ubuntu18.04.qcow2
Domain ubuntu18.04 started

-rw------- 1 libvirt-qemu kvm 16108814336 Jun 22 13:28 /var/lib/libvirt/images/ubuntu18.04.qcow2
Domain ubuntu18.04 is being shutdown

-rw------- 1 root root 16108814336 Jun 22 13:28 /var/lib/libvirt/images/ubuntu18.04.qcow2

Ok, now things make sense.
And as I said libattr1 is in main, the error conditions if the underlying FS has no libattr support are covered by the libvirt code I think. While the impacts as an SRU to existing versions is unclear I agree that we should look into enabling this in >=20.10

[1]: https://launchpad.net/ubuntu/+source/libvirt/6.0.0-0ubuntu8.1/+build/19398123

Revision history for this message
Christian Ehrhardt  (paelzer) wrote :

Extra comment to not be lost - thanks Soren for the hint.

Changed in libvirt (Ubuntu):
assignee: Phillip Susi (psusi) → Christian Ehrhardt  (paelzer)
importance: Low → Medium
tags: added: libvirt-20.10
Revision history for this message
Christian Ehrhardt  (paelzer) wrote :

Note, this will be part of the libvirt merge (including all sorts of regression checks) which will be late July/early August.

Changed in libvirt (Ubuntu):
status: Triaged → In Progress
summary: - libvirt should not take ownership of ISO images
+ libvirt restore exactly the old ownership of images
Revision history for this message
Soren Hansen (soren) wrote :

A non-root process that uses qemu:///system to run virtual machines (which is required for most network configurations) will lose access to its disk images for good as soon as the domain is started.

First it'll be changed to libvirt-qemu:kvm and once the domain is terminated it will be set to root:root. As a regular user this is nuisance that can be worked around with "sudo chown" (the vast majority of users have this access), but for a daemon that doesn't run as root and doesn't have a human to take care of it and needs access to *its own* disk images, this makes libvirtd as shipped by ubuntu unusable.

I'd highly suggest an SRU for this.

I've been away from Ubuntu development for a while. Is there any particular reason not to just upload the fixed version now?

Revision history for this message
Soren Hansen (soren) wrote :

To clarify, I meant: Is there any particular reason to not just upload this fix *to Groovy* now? I know an SRU needs validation first.

Revision history for this message
Christian Ehrhardt  (paelzer) wrote :

Hi Soren,
yeah I've seen your old youtube video on what is new in virtualization for Ubuntu since the name felt known :-).

The reason to not upload immediately is that I'd want to see if there are any other bad effects of attr being enabled that we might miss.
I'd want to see if e.g. migrations on disk images on shared storage run into trouble or something like that. To do so I'd run a bunch of regression tests in addition to what comes to my mind for this case in particular. Those tests need quite some time and allocation of systems - therefore they are most effective if coupled with a bunch of other big changes where the tests will cover "more new code".

In addition I'm away for quite some time in July, so if I break things with an upload now I'm not around to clean up my mess.

And while the bug feels bad, it was open so long attracting only so few people/reports that while it feels important for the two of us now it might overall be not too urgent to squeeze it in asap.

I hope that explains why I have postponed the change to when I get to merge a newer libvirt for groovy anyway.

Revision history for this message
Soren Hansen (soren) wrote :

Gosh. I'd happily forgotten about that video. I was so young and so mumbly. :D

I'm not sure I understand. If you're not uploading to groovy, how are you collecting test results?

Also, I don't know what sort of test facilities or processes you have available, so I don't know how time consuming or expensive a complete regression test run is, but in general I find that smaller changes are better than larger, batched changes for finding problems and identifying their cause. If something doesn't work after pushing a big batch of changes, it can be really hard to identify which one of the many changes is the culprit.

Revision history for this message
Christian Ehrhardt  (paelzer) wrote :

I upload to a PPA and test against that when doing bigger changes.
Only when everything works there the same thing is uploaded to the -dev release.

I agree to the "many small changes" theory in general, except for things that either:
- a) are used in many other tests (which means breaking these breaks a lot)
or
- b) require a rather intensive testing (then there is an efficiency gain to batch to some extend)

In this case it is even a+b which makes me be careful.
If not for me being away in 10 days for a while I'd not be too concerned as it could be reverted.
You know what, I can throw it in a PPA and if magically everything works fine (after all most tests are automated - it is all the special cases that might happen which make this a real effort) without hickups I'll consider an earlier upload.

Consider it a favor for your old work :-)
I'll let you know how things turn out ...

Mathew Hodson (mhodson)
affects: libvirt → libvirt (Baltix)
Mathew Hodson (mhodson)
affects: libvirt (Baltix) → ubuntu-translations
no longer affects: ubuntu-translations
Revision history for this message
Christian Ehrhardt  (paelzer) wrote :

We are lucky - tests that ran over night LGTM nothing happened.
In the meantime MP [1] review by the team also completed.

[1]: https://code.launchpad.net/~paelzer/ubuntu/+source/libvirt/+git/libvirt/+merge/386212

Revision history for this message
Christian Ehrhardt  (paelzer) wrote :

Uploaded to groovy

Revision history for this message
Launchpad Janitor (janitor) wrote :

This bug was fixed in the package libvirt - 6.0.0-0ubuntu10

---------------
libvirt (6.0.0-0ubuntu10) groovy; urgency=medium

  * enable attr support to store XATTR labels. Among other things
    this allows to properly restore file ownership (LP: #691590)
      - d/control: build depend to libattr1-dev
      - d/rules: configure --with-attr

 -- Christian Ehrhardt <email address hidden> Mon, 22 Jun 2020 21:30:50 +0200

Changed in libvirt (Ubuntu):
status: In Progress → Fix Released
Revision history for this message
Christian Ehrhardt  (paelzer) wrote :

I think this won't fit for a SRU into a long released release like Bionic.
But for Focal I'd think this still makes sense IMHO - it is a behavior change yes, but a wanted one.

I'll prepare an upload for Focal ...

MP: https://code.launchpad.net/~paelzer/ubuntu/+source/libvirt/+git/libvirt/+merge/386376

description: updated
Changed in libvirt (Ubuntu Focal):
importance: Undecided → Medium
status: New → Triaged
Revision history for this message
Christian Ehrhardt  (paelzer) wrote :

Uploaded to focal-unapproved for the SRU team to evaluate and sent to Debian [1].

[1]: https://salsa.debian.org/libvirt-team/libvirt/-/merge_requests/52

Revision history for this message
Robie Basak (racb) wrote : Please test proposed package

Hello C, or anyone else affected,

Accepted libvirt into focal-proposed. The package will build now and be available at https://launchpad.net/ubuntu/+source/libvirt/6.0.0-0ubuntu8.2 in a few hours, and then in the -proposed repository.

Please help us by testing this new package. See https://wiki.ubuntu.com/Testing/EnableProposed for documentation on how to enable and use -proposed. Your feedback will aid us getting this update out to other Ubuntu users.

If this package fixes the bug for you, please add a comment to this bug, mentioning the version of the package you tested, what testing has been performed on the package and change the tag from verification-needed-focal to verification-done-focal. If it does not fix the bug for you, please add a comment stating that, and change the tag to verification-failed-focal. In either case, without details of your testing we will not be able to proceed.

Further information regarding the verification process can be found at https://wiki.ubuntu.com/QATeam/PerformingSRUVerification . Thank you in advance for helping!

N.B. The updated package will be released to -updates after the bug(s) fixed by this package have been verified and the package has been in -proposed for a minimum of 7 days.

Changed in libvirt (Ubuntu Focal):
status: Triaged → Fix Committed
tags: added: verification-needed verification-needed-focal
Revision history for this message
Robie Basak (racb) wrote :

The key SRU user impact justification I wanted I found in Soren's comment 27 - thank you.

Can I request that both the "filesystem supporting xattr" and "no filesystem xattr support" cases be verified for expected behaviour please?

Since we're changing behaviour I'm also concerned that this is high risk of regression for some use case we haven't considered where the previous behaviour is being depended upon. Thoughts on how to mitigate this would be appreciated. I appreciate that Christian has already come up with and tested a variety of use cases here - how comprehensive do you think these are when compared to "weird" things users might be doing?

Revision history for this message
Ubuntu SRU Bot (ubuntu-sru-bot) wrote : Autopkgtest regression report (libvirt/6.0.0-0ubuntu8.2)

All autopkgtests for the newly accepted libvirt (6.0.0-0ubuntu8.2) for focal have finished running.
The following regressions have been reported in tests triggered by the package:

libvirt-dbus/1.3.0-1 (armhf)
libvirt/6.0.0-0ubuntu8.2 (amd64)

Please visit the excuses page listed below and investigate the failures, proceeding afterwards as per the StableReleaseUpdates policy regarding autopkgtest regressions [1].

https://people.canonical.com/~ubuntu-archive/proposed-migration/focal/update_excuses.html#libvirt

[1] https://wiki.ubuntu.com/StableReleaseUpdates#Autopkgtest_Regressions

Thank you!

Revision history for this message
Christian Ehrhardt  (paelzer) wrote :

Hi Robie,
about:
"Since we're changing behaviour I'm also concerned that this is high risk of regression for some use case we haven't considered where the previous behaviour is being depended upon. Thoughts on how to mitigate this would be appreciated. I appreciate that Christian has already come up with and tested a variety of use cases here - how comprehensive do you think these are when compared to "weird" things users might be doing?"

Answer: the current workaround would server there as well, one can tweak in file /etc/libvirt/qemu.conf
  # Whether libvirt should dynamically change file ownership
  # to match the configured user/group above. Defaults to 1.
  # Set to 0 to disable file ownership changes.
  #dynamic_ownership = 1

Currently this is enabled, changing the file to the runtime perms needed and then back (the bug) to root.
If you just chown the files to root and disable this feature it will match the former behavior (which now restores the proper former owner).

Revision history for this message
Christian Ehrhardt  (paelzer) wrote :

FYI: Flaky Autopkgtests resolved and good now.

Revision history for this message
Christian Ehrhardt  (paelzer) wrote :
Download full text (5.7 KiB)

Verification:
The problem
$ sudo chown root:kvm /var/lib/uvtool/libvirt/images/focal-ipxe.qcow
[sudo] password for paelzer:
[✓]─[paelzer@Keschdeichel ~]──[145208]──[12:57 Do Jul 16]──
$ ll /var/lib/uvtool/libvirt/images/focal-ipxe.qcow
-rw------- 1 root kvm 2546794496 Jul 16 12:57 /var/lib/uvtool/libvirt/images/focal-ipxe.qcow
[✓]─[paelzer@Keschdeichel ~]──[145209]──[12:57 Do Jul 16]──
$ virsh start focal-ipxe
Domain focal-ipxe started

[✓]─[paelzer@Keschdeichel ~]──[145210]──[12:57 Do Jul 16]──
$ ll /var/lib/uvtool/libvirt/images/focal-ipxe.qcow
-rw------- 1 libvirt-qemu kvm 2546794496 Jul 16 12:57 /var/lib/uvtool/libvirt/images/focal-ipxe.qcow
[✓]─[paelzer@Keschdeichel ~]──[145211]──[12:57 Do Jul 16]──
$ virsh shutdown focal-ipxe
Domain focal-ipxe is being shutdown

[✓]─[paelzer@Keschdeichel ~]──[145212]──[13:01 Do Jul 16]──
$ ll /var/lib/uvtool/libvirt/images/focal-ipxe.qcow
-rw------- 1 root root 2548105216 Jul 16 13:01 /var/lib/uvtool/libvirt/images/focal-ipxe.qcow

^^ didn't restore original ownership.

Install upgrade:
$ sudo apt install libvirt-daemon-system libvirt-clients
Reading package lists... Done
Building dependency tree
Reading state information... Done
The following package was automatically installed and is no longer required:
  libllvm9
Use 'sudo apt autoremove' to remove it.
The following additional packages will be installed:
  libnss-libvirt libvirt-daemon libvirt-daemon-driver-qemu libvirt-daemon-driver-storage-rbd libvirt0
Suggested packages:
  libvirt-daemon-driver-lxc libvirt-daemon-driver-vbox libvirt-daemon-driver-xen libvirt-daemon-driver-storage-gluster libvirt-daemon-driver-storage-zfs auditd pm-utils systemtap
The following packages will be upgraded:
  libnss-libvirt libvirt-clients libvirt-daemon libvirt-daemon-driver-qemu libvirt-daemon-driver-storage-rbd libvirt-daemon-system libvirt0
7 upgraded, 0 newly installed, 0 to remove and 129 not upgraded.
Need to get 2.907 kB of archives.
After this operation, 0 B of additional disk space will be used.
Do you want to continue? [Y/n] Y
Get:1 http://de.archive.ubuntu.com/ubuntu focal-proposed/universe amd64 libnss-libvirt amd64 6.0.0-0ubuntu8.2 [14,4 kB]
Get:2 http://de.archive.ubuntu.com/ubuntu focal-proposed/main amd64 libvirt-daemon-driver-qemu amd64 6.0.0-0ubuntu8.2 [605 kB]
Get:3 http://de.archive.ubuntu.com/ubuntu focal-proposed/main amd64 libvirt-daemon-driver-storage-rbd amd64 6.0.0-0ubuntu8.2 [28,3 kB]
Get:4 http://de.archive.ubuntu.com/ubuntu focal-proposed/main amd64 libvirt-daemon-system amd64 6.0.0-0ubuntu8.2 [67,5 kB]
Get:5 http://de.archive.ubuntu.com/ubuntu focal-proposed/main amd64 libvirt-clients amd64 6.0.0-0ubuntu8.2 [343 kB]
Get:6 http://de.archive.ubuntu.com/ubuntu focal-proposed/main amd64 libvirt0 amd64 6.0.0-0ubuntu8.2 [1.444 kB]
Get:7 http://de.archive.ubuntu.com/ubuntu focal-proposed/main amd64 libvirt-daemon amd64 6.0.0-0ubuntu8.2 [404 kB]
Fetched 2.907 kB in 2s (1.211 kB/s)
The system does not support apt-btrfs-snapshot
Preconfiguring packages ...
(Reading database ... 384131 files and directories currently installed.)
Preparing to unpack .../0-libnss-libvirt_6.0.0-0ubuntu8.2_amd64.deb ...
Unpacking libnss-libvirt:amd64 (6.0...

Read more...

Revision history for this message
Christian Ehrhardt  (paelzer) wrote :

As I was asked to verify behavior on non-xattr systems as well I used one mounted like:
  /dev/nvme0n1p3 on /var/lib/uvtool/libvirt/images type ext4 (rw,relatime,nouser_xattr)

We see that even that isn't a showstopper as it is user_attrs and libvirtd runs as root.
# getfattr -d -m . /var/lib/uvtool/libvirt/images/focal.qcow
getfattr: Removing leading '/' from absolute path names
# file: var/lib/uvtool/libvirt/images/focal.qcow
trusted.libvirt.security.dac="+0:+100"
trusted.libvirt.security.ref_dac="1"
trusted.libvirt.security.timestamp_dac="1593600755"

I also used an ext2 which might support less attributes
  /dev/nvme0n1p3 on /var/lib/uvtool/libvirt/images type ext2 (rw,relatime,nouser_xattr)

But that still can store the xattr.

We don't build kernels without xattr to force it that way.

So finally I took a FS that can't do ownerships well in general.
/dev/nvme0n1p3 on /var/lib/uvtool/libvirt/images type vfat (rw,relatime,fmask=0022,dmask=0022,codepage=437,iocharset=iso8859-1,shortname=mixed,errors=remount-ro)

But that isn't able to be used (permission errors) if not at the right group right away:
-rwxr-xr-x 1 root root 245104640 Jul 16 11:11 /var/lib/uvtool/libvirt/images/focal.qcow*
root@node-horsea:~# virsh start focal
error: Failed to start domain focal
error: internal error: process exited while connecting to monitor: 2020-07-16T11:48:55.976986Z qemu-system-x86_64: -blockdev {"node-name":"libvirt-2-format","read-only":false,"driver":"qcow2","file":"libvirt-2-storage","backing":"libvirt-3-format"}: Could not reopen file: Permission denied

id libvirt-qemu
uid=64055(libvirt-qemu) gid=115(kvm) groups=115(kvm),117(libvirt),64055(libvirt-qemu)

Mounted as the required user we can use it and it is not changing IDs in anyway (while running at non xattr FS)
/dev/nvme0n1p3 on /var/lib/uvtool/libvirt/images type vfat (rw,relatime,uid=64055,gid=115,fmask=0022,dmask=0022,codepage=437,iocharset=iso8859-1,shortname=mixed,errors=remount-ro)

root@node-horsea:~# ll /var/lib/uvtool/libvirt/images/focal.qcow
-rwxr-xr-x 1 libvirt-qemu kvm 245104640 Jul 16 11:11 /var/lib/uvtool/libvirt/images/focal.qcow*
root@node-horsea:~# virsh start focal
Domain focal started

root@node-horsea:~# ll /var/lib/uvtool/libvirt/images/focal.qcow
-rwxr-xr-x 1 libvirt-qemu kvm 245104640 Jul 16 11:51 /var/lib/uvtool/libvirt/images/focal.qcow*
root@node-horsea:~# virsh shutdown focal
Domain focal is being shutdown

root@node-horsea:~# ll /var/lib/uvtool/libvirt/images/focal.qcow
-rwxr-xr-x 1 libvirt-qemu kvm 246546432 Jul 16 11:52 /var/lib/uvtool/libvirt/images/focal.qcow*

I think that covers all combinations that come to mind, excluding those only available in non-Ubuntu kernels.

tags: added: verification-done verification-done-focal
removed: verification-needed verification-needed-focal
Revision history for this message
Launchpad Janitor (janitor) wrote :

This bug was fixed in the package libvirt - 6.0.0-0ubuntu8.2

---------------
libvirt (6.0.0-0ubuntu8.2) focal; urgency=medium

  * enable attr support to store XATTR labels. Among other things
    this allows to properly restore file ownership (LP: #691590)
      - d/control: build depend to libattr1-dev
      - d/rules: configure --with-attr

 -- Christian Ehrhardt <email address hidden> Mon, 22 Jun 2020 21:30:50 +0200

Changed in libvirt (Ubuntu Focal):
status: Fix Committed → Fix Released
Revision history for this message
Łukasz Zemczak (sil2100) wrote : Update Released

The verification of the Stable Release Update for libvirt has completed successfully and the package is now being released to -updates. Subsequently, the Ubuntu Stable Release Updates Team is being unsubscribed and will not receive messages about this bug report. In the event that you encounter a regression using the package from -updates please report a new bug using ubuntu-bug and tag the bug report regression-update so we can easily find any regressions.

Revision history for this message
Christian Ehrhardt  (paelzer) wrote :

If you come by this more than a decade later and wonder, hmm this isn't working still/again please do mind bug 2002771 that explains that this is different for "normal"-files vs read-only-files.

See https://gitlab.com/libvirt/libvirt/-/blob/master/src/security/security_dac.c?ref_type=heads#L987
Quote:
    /* Don't restore labels on readoly/shared disks, because other VMs may
     * still be accessing these. Alternatively we could iterate over all
     * running domains and try to figure out if it is in use, but this would
     * not work for clustered filesystems, since we can't see running VMs using
     * the file on other nodes. Safest bet is thus to skip the restore step. */

Due to that it works since the fix above in >=Focal for files, but still (and never) not for .iso files which the very initial report was about.

To post a comment you must log in.
This report contains Public information  
Everyone can see this information.

Duplicates of this bug

Other bug subscribers

Remote bug watches

Bug watches keep track of this bug in other bug trackers.