On Wed, Sep 11, 2024 at 03:02:41PM -0600, Jim Fehlig wrote: > Enhance the 'since' annotation of <filterref> documentation to note > it's only supported by the QEMU, LXC, and ch hypervisor drivers. > > Signed-off-by: Jim Fehlig <jfehlig(a)suse.com&gt; > --- > docs/formatdomain.rst | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst > index 47d3e2125e..8e8a9660fc 100644 > --- a/docs/formatdomain.rst > +++ b/docs/formatdomain.rst > @@ -6205,10 +6205,10 @@ hypervisor tries to reconnect. > Traffic filtering with NWFilter > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ > > -:since:`Since 0.8.0` an ``nwfilter`` profile can be assigned to a domain > -interface, which allows configuring traffic filter rules for the virtual > -machine. See the `nwfilter <formatnwfilter.html>`__ documentation for more > -complete details. > +:since:`Since 0.8.0 (QEMU), 0.9.3 (LXC), 10.1.0 (Cloud Hypervisor)` an ``nwfilter`` > +profile can be assigned to a domain interface, which allows configuring traffic > +filter rules for the virtual machine. See the `nwfilter <formatnwfilter.html>`__ > +documentation for more complete details. > > :: > > -- > 2.35.3 Should this have a Suggested-by: Demi Marie Obenour <demi(a)invisiblethingslab.com&gt; tag?

In any case: Acked-by: Demi Marie Obenour <demi(a)invisiblethingslab.com&gt;

Enhance the 'since' annotation of <filterref> documentation to note it's only supported by the QEMU, LXC, and ch hypervisor drivers. Signed-off-by: Jim Fehlig <jfehlig(a)suse.com&gt; --- docs/formatdomain.rst | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst index 47d3e2125e..8e8a9660fc 100644 --- a/docs/formatdomain.rst +++ b/docs/formatdomain.rst @@ -6205,10 +6205,10 @@ hypervisor tries to reconnect. Traffic filtering with NWFilter ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ -:since:`Since 0.8.0` an ``nwfilter`` profile can be assigned to a domain -interface, which allows configuring traffic filter rules for the virtual -machine. See the `nwfilter <formatnwfilter.html>`__ documentation for more -complete details. +:since:`Since 0.8.0 (QEMU), 0.9.3 (LXC), 10.1.0 (Cloud Hypervisor)` an ``nwfilter`` +profile can be assigned to a domain interface, which allows configuring traffic +filter rules for the virtual machine. See the `nwfilter <formatnwfilter.html>`__ +documentation for more complete details.

On 9/11/24 16:08, Laine Stump wrote: > On 9/11/24 5:02 PM, Jim Fehlig via Devel wrote: >> Enhance the 'since' annotation of <filterref> documentation to note >> it's only supported by the QEMU, LXC, and ch hypervisor drivers. >> >> Signed-off-by: Jim Fehlig <jfehlig(a)suse.com&gt; >> --- >>   docs/formatdomain.rst | 8 ++++---- >>   1 file changed, 4 insertions(+), 4 deletions(-) >> >> diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst >> index 47d3e2125e..8e8a9660fc 100644 >> --- a/docs/formatdomain.rst >> +++ b/docs/formatdomain.rst >> @@ -6205,10 +6205,10 @@ hypervisor tries to reconnect. >>   Traffic filtering with NWFilter >>   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ >> -:since:`Since 0.8.0` an ``nwfilter`` profile can be assigned to a domain >> -interface, which allows configuring traffic filter rules for the virtual >> -machine. See the `nwfilter <formatnwfilter.html>`__ documentation for more >> -complete details. >> +:since:`Since 0.8.0 (QEMU), 0.9.3 (LXC), 10.1.0 (Cloud Hypervisor)` an >> ``nwfilter`` >> +profile can be assigned to a domain interface, which allows configuring traffic >> +filter rules for the virtual machine. See the `nwfilter >> <formatnwfilter.html>`__ >> +documentation for more complete details. > > It's preexisting, but I would have said "allows configuring network traffic > filter rules". I've added 'network' to that sentence in my local branch. > > Reviewed-by: Laine Stump <laine(a)redhat.com&gt;

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,

but ignoring a user's request to filter VM network traffic can be viewed as a security issue.

Signed-off-by: Jim Fehlig <jfehlig(a)suse.com&gt; --- 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; + } +

if (dev->type == VIR_DOMAIN_DEVICE_CHR && dev->data.chr->deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_CONSOLE && dev->data.chr->targetType == VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_NONE &&

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(a)suse.com&gt; > > --- > >   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?

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(a)suse.com&gt; > > > > --- > > > >   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).

> 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.

On Wed, Sep 11, 2024 at 18:24:07 -0400, 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(a)suse.com&gt; >> --- >> 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, > > (my understanding of the purpose of the two has always been that the > PostParse callback is intended for performing post-parse > modifications/fixups to the domain def, while the Validate only checks for > correctness, without modifying anything. There are multiple cases of having > validation in the postparse function, but I think those are the 1) leftovers > from before the introduction of the validate callbacks, and 2) the result of > misunderstanding and/or sloth (e.g. in cases where you want to validate > something, but the driver you're adding this validation to doesn't already > have a deviceValidateCallback) The main difference between the two is that 'postParse' is called on every parse of a XML. That means also for parsing the XMLs of persistently defined domains. If you reject parsing of a XML in a 'postParse' it fails to load the persistent definition so the VM "vanishes", which is something we don't want to do.

Thus doing validation in postParse is really valid only when it's a new attribute or configuration that can't exist in "defined" state. In contrast the validate callback is applied in a limited set of XML entrypoints which then don't make the VM to vanish and certain other situations. For that reason the 'Validate' callback/step needs to be re-tried when starting the VM.