(In reply to Martin Polednik from comment #0)
> Description of problem:
> Running in environment with dynamic_ownership=0 but using <seclabel
> model="dac" relabel="no" /> for disks, it seems that snapshot creation does
> not respect the seclabel setting.
> 
> Version-Release number of selected component (if applicable):
> libvirt-3.9.0-14.el7_5.5.x86_64
> libvirt-daemon-3.9.0-14.el7_5.5.x86_64
> libvirt-python-3.9.0-1.el7.x86_64
> libvirt-daemon-driver-qemu-3.9.0-14.el7_5.5.x86_64
> 
> How reproducible:
> 100%
> 
> Steps to Reproduce:
> 1. make sure that dynamic_ownership=1
> 
In description, you say dynamic_ownership=0 but dynamic_ownership=1 here. So what is your dynamic_ownership value?
> 2. prepare a disk for the VM
> # qemu-img create -f raw disk 20G
> # ls -l /disks
> total 0
> -rw-r--r--. 1 qemu qemu 21474836480 May 31 14:07 disk
> 
> 3. prepare a snapshot file
> # touch /disks/snap
> # touch /disks/memsnap
> # chown vdsm:qemu /disks/snap
> # chown vdsm:qemu /disks/memsnap
> # ls -l /disks
> total 0
> -rw-r--r--. 1 qemu qemu 21474836480 May 31 14:07 disk
> -rw-r--r--. 1 vdsm qemu           0 May 31 14:40 memsnap
> -rw-r--r--. 1 vdsm qemu           0 May 31 14:40 snap
> 
> 4. define a domain
> # dumpxml 0_snapshot_test
> <domain type='kvm'>
>   <name>0_snapshot_test</name>
>   <uuid>1ced07d0-a52b-4b9f-a85d-a1d51481dfa9</uuid>
>   <maxMemory slots='16' unit='KiB'>16777216</maxMemory>
>   <memory unit='KiB'>4194304</memory>
>   <currentMemory unit='KiB'>4194304</currentMemory>
>   <vcpu placement='static' current='4'>16</vcpu>
>   <os>
>     <type arch='x86_64' machine='pc-i440fx-rhel7.3.0'>hvm</type>
>     <boot dev='hd'/>
>   </os>
>   <cpu mode='custom' match='exact' check='partial'>
>     <model fallback='allow'>Westmere</model>
>     <topology sockets='16' cores='1' threads='1'/>
>     <numa>
>       <cell id='0' cpus='0-3' memory='4194304' unit='KiB'/>
>     </numa>
>   </cpu>
>   <clock offset='utc'/>
>   <on_poweroff>destroy</on_poweroff>
>   <on_reboot>restart</on_reboot>
>   <on_crash>destroy</on_crash>
>   <devices>
>     <emulator>/usr/libexec/qemu-kvm</emulator>
>     <disk type='file' device='disk' snapshot='no'>
>       <driver name='qemu' type='raw' cache='none' error_policy='stop'
> io='threads'/>
>       <source file='/disks/disk'/>
>       <target dev='vda' bus='virtio'/>
>       <address type='pci' domain='0x0000' bus='0x00' slot='0x03'
> function='0x0'/>
>     </disk>
>     <controller type='usb' index='0' model='piix3-uhci'>
>       <address type='pci' domain='0x0000' bus='0x00' slot='0x01'
> function='0x2'/>
>     </controller>
>     <controller type='pci' index='0' model='pci-root'/>
>     <controller type='virtio-serial' index='0'>
>       <address type='pci' domain='0x0000' bus='0x00' slot='0x02'
> function='0x0'/>
>     </controller>
>     <channel type='spicevmc'>
>       <target type='virtio' name='com.redhat.spice.0'/>
>       <address type='virtio-serial' controller='0' bus='0' port='1'/>
>     </channel>
>     <input type='mouse' bus='ps2'/>
>     <input type='keyboard' bus='ps2'/>
>     <memballoon model='virtio'>
>       <address type='pci' domain='0x0000' bus='0x00' slot='0x04'
> function='0x0'/>
>     </memballoon>
>   </devices>
> </domain>
> 
> 5. start the domain
> virsh # start 0_snapshot_test
> Domain 0_snapshot_test started
> 
> 6. prepare a snapshot XML desc
> # cat snapxml
> <domainsnapshot><disks><disk name="vda" snapshot="external"
> type="file"><source file="/disks/snap" type="file"><seclabel type='none'
> model='dac' relabel='no'/></source></disk></disks><memory
> file="/disks/memsnap" snapshot="external" /></domainsnapshot>
> 
> 7. create a snapshot
> # snapshot-create --domain 18 --xmlfile snapxml
> Domain snapshot 1527770242 created from 'snapxml'
> 
I tried with dynamic_ownership=０, in this step get different result with you:
# virsh snapshot-create A /tmp/snap.xml 
error: internal error: unable to execute QEMU command 'transaction': Could not create file: Permission denied

> Actual results:
> # ls -l /disks
> total 9996
> -rw-r--r--. 1 qemu qemu 21474836480 May 31 14:07 disk
> -rw-------. 1 root root    10032045 May 31 14:37 memsnap
> -rw-r--r--. 1 qemu qemu      196928 May 31 14:37 snap
> 
> Expected results:
> # ls -l /disks
> total 9996
> -rw-r--r--. 1 qemu qemu 21474836480 May 31 14:07 disk
> -rw-------. 1 vdsm qemu    10032045 May 31 14:37 memsnap
> -rw-r--r--. 1 vdsm qemu      196928 May 31 14:37 snap
>               ^^^^^^^^^

> In description, you say dynamic_ownership=0 but dynamic_ownership=1 here. So what is > your dynamic_ownership value?

Sorry for the description, the bug revolves around dynamic_ownership=1.

When dynamic_ownership=1, libvirt will change ownership to qemu:qemu(if user/group are not set in qemu.conf) when using a image. <seclabel..> is to set the selinux label. So the result in description is expected. 
It tends to be NOTABUG. Peter, do you agree it?

No, <seclabel> can be also used to set the DAC(unix) user:group and also turn labelling off.

The original reporter actually wants to stop libvirt from re-labelling the user:group of the image by specifying a "dac" seclabel with the "relabel='no'" attribute for the snapshot disk.

If that does not work it is a bug.

So first thing is that libvirt officially does not support seclabels for snapshot disk definitions, but that does not mean they don't work. I'll post patches to add them.

Secondly some parts of your reproducer would not work:

(In reply to Martin Polednik from comment #0)
> 3. prepare a snapshot file
> # touch /disks/snap
> # touch /disks/memsnap
> # chown vdsm:qemu /disks/snap
> # chown vdsm:qemu /disks/memsnap
> # ls -l /disks

So '/disks/snap' exists now ...

> total 0
> -rw-r--r--. 1 qemu qemu 21474836480 May 31 14:07 disk
> -rw-r--r--. 1 vdsm qemu           0 May 31 14:40 memsnap
> -rw-r--r--. 1 vdsm qemu           0 May 31 14:40 snap

[...]

> 5. start the domain
> virsh # start 0_snapshot_test
> Domain 0_snapshot_test started
> 
> 6. prepare a snapshot XML desc
> # cat snapxml
> <domainsnapshot><disks><disk name="vda" snapshot="external"
> type="file"><source file="/disks/snap" type="file"><seclabel type='none'
> model='dac' relabel='no'/></source></disk></disks><memory
> file="/disks/memsnap" snapshot="external" /></domainsnapshot>
> 
> 7. create a snapshot
> # snapshot-create --domain 18 --xmlfile snapxml
> Domain snapshot 1527770242 created from 'snapxml'

So this is impossible. If there is an existing file and you don't specify --reuse-external you get the following error:

$ virsh snapshot-create upstream snap.xml 
error: unsupported configuration: external snapshot file for disk vda already exists and is not a block device: /tmp/snap

With '--reuse-external' you need to format the file to qcow2 first using qemu-img.

> Actual results:
> # ls -l /disks
> total 9996
> -rw-r--r--. 1 qemu qemu 21474836480 May 31 14:07 disk
> -rw-------. 1 root root    10032045 May 31 14:37 memsnap
> -rw-r--r--. 1 qemu qemu      196928 May 31 14:37 snap

So I've tested this:

Snapshot XML (based on your example, but fixed to conform to our schema):
# cat snap.xml
<domainsnapshot>
  <disks>
    <disk name="vda" snapshot="external" type="file">
      <source file="/tmp/snap">
        <seclabel model='dac' relabel='no'/>
      </source>
    </disk>
  </disks>
  <memory file="/tmp/memsnap" snapshot="external"/>
</domainsnapshot>

# cd /tmp
# qemu-img create -f qcow2 -F raw -b /var/lib/libvirt/images/a snap
Formatting 'snap', fmt=qcow2 size=10485760 backing_file=/var/lib/libvirt/images/a backing_fmt=raw cluster_size=65536 lazy_refcounts=off refcount_bits=16
# chown nobody:nobody snap
# chmod 777 snap
# ls -lia snap
266715 -rwxrwxrwx 1 nobody nobody 196616 Jun  5 16:42 snap

# virsh snapshot-create upstream snap.xml --reuse-external
Domain snapshot 1528209813 created from 'snap.xml'

# ls -lia snap
266715 -rwxrwxrwx 1 nobody nobody 196616 Jun  5 16:42 snap

So it works. I've tested it also with relabelling enabled but that is not very useful with 'dac' security labels. Usually you want the one used with the VM anyways.
As of such I think there is no bug in libvirt.

After investigating with Petr, it does seem that seclabel works correctly for the disk snapshot part, but not for the memory snapshot. In case of oVirt/RHV, the memory snapshot lies on the shared storage that is handled by VDSM - so we need it to have specific permissions too.

I'm not sure whether it makes sense to continue in this bug or create an RFE for seclabel for memory snapshot?

I think we should keep this one for formally adding the <seclabel> support for disk snapshot sources as the XML schema does not allow it.

For the memory snapshot seclabel, please file a RFE.

Hi Peter, how is it going with this one?

Upstream now documents and allows the <seclabel> element in the schema for snapshot disks:

As pointed out above, this is purely cosmetical as the code accepted seclabels even before.

commit 3203681e5e8513425acdf3f25d0981f5157f0ddf (HEAD -> master, origin/master, origin/HEAD)
Author: Peter Krempa <pkrempa>
Date:   Tue Jun 5 16:02:11 2018 +0200

    tests: domainsnapshotxml2xml: make 'disk-seclabel' test operational
    
    Now that we added the seclabels to the schema we can test that they are
    parsed and formatted correctly.
    
    Signed-off-by: Peter Krempa <pkrempa>
    Reviewed-by: Martin Kletzander <mkletzan>

commit ac88a8cfad1c93897ddbbfa1cc1aabcf0245255c
Author: Peter Krempa <pkrempa>
Date:   Tue Jun 5 15:54:00 2018 +0200

    docs: schemas: Add 'seclabel' for external disk snapshot
    
    Allow using seclabels the same way as disk images allow it. Currently
    the snapshot code copies the seclabels from the original image if no
    seclabel is provided. Also there's no code change required as the
    snapshot XML parser actually uses parts of the disk parser thus
    seclabels are already parsed and formatted and even applied thus this is
    just a formalization of our support for this.
    
    Signed-off-by: Peter Krempa <pkrempa>
    Reviewed-by: Martin Kletzander <mkletzan>

commit c79ef73c37b1f91d38745cb1197026e702ce42fe
Author: Peter Krempa <pkrempa>
Date:   Thu Jun 20 09:57:54 2019 +0200

    docs: snapshot: Encourage people ot use disk 'target' to refer to disks
    
    Change the example and add a recommendation to use disk target rather
    than path.
    
    Signed-off-by: Peter Krempa <pkrempa>
    Reviewed-by: Martin Kletzander <mkletzan>

Created attachment 1638665 [details]
scripts to run snapshot seclabel test

Tested on libvirt-4.5.0-28.el7.x86_64 qemu-kvm-rhev-2.12.0-38.el7.x86_64
I run test by varialiables:
vm status: active, inactive
dynamic_ownership in qemu.conf: 1, 0
seclabel type: none, dynamic, static
seclabel relabel: yes, no
model: dac

Tests pass on these cases:
1. VM_STATUS:active, CONF:dynamic_ownership=1, TYPE:none, MODEL:dac, RELABEL:no
2. VM_STATUS:active, CONF:dynamic_ownership=1, TYPE:none, MODEL:dac, RELABEL:yes
3. VM_STATUS:active, CONF:dynamic_ownership=1, TYPE:dynamic, MODEL:dac, RELABEL:no
4. VM_STATUS:active, CONF:dynamic_ownership=1, TYPE:dynamic, MODEL:dac, RELABEL:yes
5. VM_STATUS:active, CONF:dynamic_ownership=1, TYPE:static, MODEL:dac, RELABEL:no

Failed on this case：
1. VM_STATUS:active, CONF:dynamic_ownership=1, TYPE:static, MODEL:dac, RELABEL:yes
actual: root root
expected: qemu root

So I wonder to know:
1. Whether static seclabel type works to relabel as user ownership in snapshot xml when dynamic_ownership=1 and RELABEL:yes?
2. Does dynamic_ownership=1 affect selinux relabel?
3. Does this bug contains the fix of selinux seclabel type relabeling on snapshot?
4. For inactive VM, will snapshot file ownership be changed when create a reused external disk snapshot with seclabel?

Questions of comment18

Created attachment 1642271 [details]
Domain snapshot xml to validate

Since the bugs is only a fix for docs and unit tests. Verified as following
version: libvirt-4.5.0-28.el7.x86_64
XML matrix:
type: file, block
seclabel: 2 seclabels, 1 seclabel, no seclabel

Results:
# for i in *;do virt-xml-validate $i;done
seclabel-block-both.xml validates
seclabel-block-no.xml validates
seclabel-block-single.xml validates
seclabel-file-both.xml validates
seclabel-file-no.xml validates
seclabel-file-single.xml validates

And I checked formatsnapshot.html, seclabel related contents are contained.

Since the problem described in this bug report should be
resolved in a recent advisory, it has been closed with a
resolution of ERRATA.

For information on the advisory, and where to find the updated
files, follow the link below.

If the solution does not work for you, open a new bug report.

https://access.redhat.com/errata/RHBA-2020:1094