10:10 a.m.
On Thu, May 09, 2024 at 06:40:01 -0700, Andrea Bolognani wrote:
On Thu, May 09, 2024 at 01:58:21PM GMT, Peter Krempa wrote:
> On Thu, May 02, 2024 at 19:39:39 +0200, Andrea Bolognani wrote:
> > +# libvirt will normally prevent migration if the storage backing the VM is
not
> > +# on a shared filesystems. Sometimes, however, the storage *is* shared
despite
> > +# not being detected as such: for example, this is the case when one of the
> > +# hosts involved in the migration is exporting its local storage to the other
> > +# one via NFS.
>
> I wanted to suggest using VIR_MIGRATE_UNSAFE flag to bypass this check,
> but doing so without disabling dynamic ownership on the image itself
> would still make the security driver cut off access to this file.
This was suggested in the past, but the argument against it was that
it's too vague: we want to skip the check on whether the storage is
shared, and that one only, while "unsafe migration" could potentially
cover a lot of unrelated scenarios.
The other argument against using a migration flag was that it's an
all-or-nothing proposition: either you perform the check for all
paths, or you skip it for all paths. A configuration option allows us
more granular control, where we can designate exactly which paths
should not be subject to the check, retaining the default behavior
for all the other ones.
Fair enough.
> > +# Any directory listed here will be assumed to live on a
shared filesystem,
> > +# making migration possible in scenarios such as the one described above.
> > +#
> > +# Due to how label remembering is implemented, it will generally be necessary
> > +# to set remember_owner=0 *on both sides of the migration* as well.
>
> So IIUC the problem here is that on the box where the storage is local
> this will create the XATTR entries used for remembering the lablels
> (which also includes a refcount), which given that older NFS doesn't
> support security labelling would then be leaked?
Yeah, the expectation when remembering is enabled is that both sides
of the migration will keep the record updated.
This works when XATTRs/SELinux labels are supported by the shared
filesystem or are *not* supported and both sides access it through
it, in which case it all ends up being no-op and works by omission :)
In our scenario, migrating from local to NFS will work but then
migrating back won't, because the existing information will lead
libvirt to believe that the image is already in use - which it is,
but not in the way that it expects :)
There are two distinct things in play here when migrating in scenario
that this patch is dealing with:
1) Actual security/selinux labels
- these must properly allow access to the image during the whole
time and partially also from two hosts at once
2) The additional XATTR props libvirt uses to remember security labels
- these need to be refcounted properly
In addition to that there's two directions of the migration
o) Migration out from the host that uses this feature
i) Migration in to the host using this feature
Also note that for migration in you need to consider also cases when the
VM was never started at the host using this feature yet, and is freshly
migrated in.
Now things I see as problem in case when NFS not supporting xattr is
used. This means that the remote VM can set XATTRs and must use
'virt_use_nfs' sebool.
1.o) The selinux label will be leaked/kept on the image as the target of
the migration isn't able to remove the label
1.i) A new selinux label must be applied to the image
- this is what also patch 5/5 does for TPM
- applying security label might actually prevent the NFS server from
exporting the image if configured poorly
2.o) The libvirt XATTR label will be leaked with a refcount (preventing
further local start)
2.i) This should work IIUC as the label will simply be added
I'll definitely will need to check the behaviour of selinux labels
first myself.
While just removing 2 out of existence by disabling it may seem
lucrative I think we even might need the feature if we'll need to solve
1.o).
IMO the only proper option to do this across the XATTR boundary will be
to have an additional step in the finalizing phase of migration that
will unref the libvirt labels. In case when the last reference is gone
it'd need to also restore the label, same as it does now. During
migration there'll need to be a period while two refs are on the libvirt
xattrs.
As said I'll need to actually check what's really happening in regards
of the selinux labels.
In general, all the bookkeeping and relabeling is pretty much broken
by design when migration is involved, and things only appear to work
because requests are silently ignored.
As shown above the libvirt XATTRs are only a part of this.
> Looking at the code we seem to be calling
'qemuSecurityRestoreAllLabel'
> with the 'migrated' flag set when migrating at all times. This should
> make it theoretically possible to simply clear out the XATTRs for any
> file which resides on the paths in question if we're migrating away
> rather than ignoring them, which would solve the issue.
This would involve touching the file's attributes on the source side
after the VM is already running on the destination side, which is
something that the existing code understandably steers very clear of.
I've tried fairly hard to make things work "just so" and by now I'm
convinced that there is no solution which gets it right without
adding a lot of additional complexity/fragility and introducing its
own trade-offs.
Asking users to disable owner remembering is not too unreasonable
IMO. Especially when we take into account the fact that I'm targeting
KubeVirt's needs specifically with this feature, and they *already*
do that anyway.
Well, doing it like this makes it not very attractive for any other use.
Doing this just replaces the ugly hack in kubevirt with a rather
unattractive implementation of this in libvirt.
For anyone wanting to use this outside of Kubevirt they'd need to give
up the label remembering.
Also as noted above the libvirt xattr stuff migt be needed to actually
prevent leaking selinux labels.
> This would go together with the 'syntax-sugar'-ish
flavor to this
> feature. Additionally it would help in case, when the user configures
> these paths after the VM was started and thus the XATTRs are already
> applied. Those would not then be cleared on migration.
That sounds like a corner case of a corner case, and a direct result
of the admin failing to configure the host correctly in the first
place. I wouldn't be too concerned about it.
Sure, but proper support for using it together with security label
remembering would fix it anyways.