On Wed, Sep 11, 2024 at 09:16:41PM -0400, Laine Stump wrote:
> On 9/11/24 7:42 PM, Demi Marie Obenour wrote:
> > On Wed, Sep 11, 2024 at 05:09:03PM -0600, Jim Fehlig wrote:
> > > On 9/11/24 16:24, Laine Stump wrote:
> > > > On 9/11/24 5:02 PM, Jim Fehlig via Devel wrote:
> > > > > The Xen libxl driver does not support nwfilter. Add a check for nwfilters
> > > > > to the devicesPostParseCallback, returning VIR_ERR_CONFIG_UNSUPPORTED if
> > > > > any are found.
> > > > >
> > > > > It's generally preferred for drivers to ignore unsupported XML features,
> > > >
> > > > I would instead characterize it as "drivers generally ignore
> > > > *unrecognized* XML", but it's quite common for a bit of XML that's
> > > > understood and supported in one context within libvirt to generate an
> > > > UNSUPPORTED error when attempting to use it in a place where it isn't
> > > > supported.
> > > >
> > > > > but ignoring a user's request to filter VM network traffic can be viewed
> > > > > as a security issue.
> > > >
> > > > Definitely.
> > > >
> > > > >
> > > > > Signed-off-by: Jim Fehlig <jfehlig@xxxxxxxx>
> > > > > ---
> > > > > src/libxl/libxl_domain.c | 7 +++++++
> > > > > 1 file changed, 7 insertions(+)
> > > > >
> > > > > diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c
> > > > > index 0f129ec69c..2f6cebb8ae 100644
> > > > > --- a/src/libxl/libxl_domain.c
> > > > > +++ b/src/libxl/libxl_domain.c
> > > > > @@ -131,6 +131,13 @@ libxlDomainDeviceDefPostParse(virDomainDeviceDef *dev,
> > > > > void *opaque G_GNUC_UNUSED,
> > > > > void *parseOpaque G_GNUC_UNUSED)
> > > > > {
> > > > > + if (dev->type == VIR_DOMAIN_DEVICE_NET && dev->data.net->filter) {
> > > > > + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> > > > > + _("filterref is not supported in %1$s"),
> > > > > + virDomainVirtTypeToString(def->virtType));
> > > > > + return -1;
> > > > > + }
> > > > > +
> > > >
> > > > This more properly should be in a function called
> > > > libxlValidateDomainDeviceDef(), which would look something like
> > > > qemuValidateDomainDeviceDef() and be added into
> > > > libxlDomainDefParserConfig with this initialization:
> > > >
> > > > .deviceValidateCallback = libxlValidateDomainDeviceDef,
> > >
> > > Yes, good point! The libxl driver already has domainValidateCallback, but
> > > now needs a deviceValidateCallback for this code. I'll make that change in
> > > V2.
> > >
> > > Before sending another version, I'd like to hear opinions on Demi's question
> > > about the other hypervisor drivers. Do they need a similar change?
>
> Now that we're talking more about this, I'm having flashbacks of new
> features being added in, and the author (e.g. me) basically updating the
> hypervisor drivers they were personally interested in to support the feature
> (or else explicitly log an error), but leaving the other drivers untouched
> because "I can't test it, so I don't want to add code that could potentially
> end up failing when somebody actually *can* test it" (or some such other
> copout :-P) (e.g. in my case that used to be qemu and lxc, but for a long
> time has been only qemu).
In this case I think this should be pretty low-risk, but I would leave
that to the authors of the other drivers to decide. For what it is
worth, the only other driver I can think of for which filtering could
potentially be added is VirtualBox on Linux, unless libvirt wants to
interface with whatever Hyper-V, VMware ESXi, VMware on desktop, and
other platforms use for their filtering.
> > I'm not familiar with how libvirt works internally, but to me it seems
> > like one option would be to have a flag set on the drivers that support
> > network filtering. Generic code would then check the flag and fail if
> > filtering is requested with a driver that doesn't support it.
> >
> > This has the advantage of not requiring changes to each and every
> > driver.
>
> An interesting idea, but then we would really want to do that for every
> individual XML knob; essentially generic "capabilities flags" similar to the
> QEMU capabilities flags we use to determine whether a particular feature is
> available for a particular QEMU binary.
This could be the first, but I also understand if this is out of scope
for the current change.
--
Sincerely,
Demi Marie Obenour (she/her/hers)
Invisible Things Lab
Attachment:
signature.asc
Description: PGP signature
