On Mon, Nov 23, 2020 at 2:33 AM Peter Krempa <pkrempa@redhat.com> wrote:
On Sat, Nov 21, 2020 at 11:20:57 -0600, Dustan B Helm wrote:
> We plan to support NFS protocol according to the example XML from Issue 90
> <http://gitlab.com/libvirt/libvirt/-/issues/90>. Since there is already
> support for network disks of different protocol types and host information,
> we think that the only new XML information we will add is an <nfs> element
> which will be a subelement of <source>, with attributes “user” and “group”
> (both strings). This element will only be generated if the source protocol
> is “nfs” and we assume that both “user” and “group” will be required.
>
> Here is the XML example given in the issue for reference:
>
> <disk type='network' device='disk'>
>
>   <driver name='qemu' type='raw'/>
>
>   <source protocol='nfs' name='PATH'>
>
>     <host name='example.com' port='2049'/
>
>     <nfs user='USER' group='GROUP'/>
>
>   </source>
>
>   <target dev='vda' bus='virtio'/>
>
> </disk>

Sounds reasonable to me. We tend to name elements equivalent to <nfs>
you propose by their purpose (such as <auth> <initiator> <cookies> for
other protocols) but in this case I don't have a better suggestion so
going with <nfs> is reasonable.

Since you are proposing 'user' and 'group' to be strings while qemu
accepts only numeric UID/GID, please use the same conversion code we
have for the <inituser> and <initgroup> values in regards to forcing
numeric value to skip being interpreded:

https://www.libvirt.org/formatdomain.html#container-boot

The linked documentation suggests that providing a “+” prefix will force attribute data to be interpreted numerically. Since QEMU requires that the group and user attributes be numeric id values, would we want to simply insert a “+” prefix to these attributes when they are provided explicitly (instead of using the hypervisor default values, which we assume will just be numeric data)?

> What do you think of these proposed changes? Should either of the <nfs>
> tag's string attributes be optional?

In this case qemu doesn't mandate the use of the user/group field so you
can make the nfs element and both user and group optional especially
since it's only a workaround for the broken-by design NFS "security".

You can claim that a hypervisor-default uid/gid is used when the fields
are not present.

We have assumed that because “group” and “user” are optional, they are not mutually required- that is, one or the other may be provided explicitly, in which case the other is assumed to be the hypervisor-default. Is this correct?

Additionally, when both default values for the hypervisor are to be used (that is, there is no explicitly given nfs-user or nfs-group attribute), would we simply remove the entire <nfs> tag as it would be empty? Or is it still necessary in order to bypass the “broken by design” NFS security?

 You also probably want to mention in the documentation that in most
cases qemu is running as non-root and thus doesn't have access to
privileged ports. Thus the export has to use the 'insecure' option to
allow non-privileged ports.

One further thing possibly worth mentioning is that the name=''
attribute starts with the NFS export name.

Does the NFS export requiring the “insecure” option in most cases have implications on the code we add to support the protocol beyond documentation of the usage semantics? Also, what specifically is referred to by the “NFS export”- is this simply the directory containing all the files/folders to be shared across the NFS (i.e. etc/exports/), or some representation thereof in memory?