Core: Traffic shaper ACK packet selection is too wide and should be narrowed down.

[Nimloth]

As discussed here: https://forum.opnsense.org/index.php?topic=17372.0
the drop-down entry 'tcp (ACK packets only)' in the 'Edit rule' dialog of the Traffic Shaper will match all packets with 'tcpflags ack'.

Since this will in effect match almost all traffic, it is not particularly useful.

So far as I could see this would only need changes in service/templates/OPNsense/IPFW/ipfw.conf
and the suggested changes are (so far) to replace 'tcpflags ack' by either 'tcpflags ack,!psh' or 'tcpflags ack iplen 52'.

It might also be important to communicate any modifications as it will change current behavior.

Note: the original issue that prompted the addition of the feature was https://github.com/opnsense/core/issues/528

[maxfield-allison]

Have you had a chance to make this change and test? I'm making the commit to my current fork and may set up a VM if no one else has had a chance to try yet.

[Nimloth]

Have you had a chance to make this change and test? I'm making the commit to my current fork and may set up a VM if no one else has had a chance to try yet.

While I have not changed and tested the template, I have run a production firewall with 'tcpflags ack iplen 52' for some time now - I edited the /usr/local/etc/ipfw.rules file directly, though.

[Nimloth]

Note: There was no consensus in the discussion in the forum as to what the appropriate rule should be, only that the current rule is to generic and therefore not very useful.

[maxfield-allison]

Could you point me to a resource that explains iplen 52? or give me a quick and dirty as to why you chose that instead of !psh?

Edit: I see in the ipfw documentation that it sets a match for packet length of 52. Is that the maximum for ack/syn etc?

[maxfield-allison]

I just made the change you suggested in my ipfw.rules and reloaded all. looks like it did the trick!

[Nimloth]

Sure. From what I can recall, 40 bytes is what you need for tcp+ip(v4) header, e.g. what you need for an ACK or RST packet. An ACK with timestamp needs 52 bytes. (See http://www.faqs.org/rfcs/rfc1323.html - 1.3 Using TCP options).

[maxfield-allison]

You're the best, I'm going to change the template on my test device and verify that it works for the drop down option and then submit a pull.

[maxfield-allison]

You were correct, The change in the template is tested and working for both tcp ack and non-ack

[Nimloth]

One more thing, I am not very firm on IPv6 stuff, but if memory serves me right, IPv6 has (or can have) larger headers.
The following page on bufferbloat.net says (https://www.bufferbloat.net/projects/bloat/wiki/ACK_prioritization/):
"Since ack packets are very small (less than 72 bytes in ipv4 and less than 140 bytes in IPv6, depending on encapsulation), shaping methods that depend more on packets than bytes tend to suffer."
So, I guess there will be situations where iplen 52 will be too narrow and not capture ACK packets.
(Btw., the less than 72 bytes for IPv4 is true because other (useful) options could be set in an ACK packet and thus increase the size.)
But, for me, iplen of 52 has always had the desired effect (with IPv4 at any rate).

[AdSchellevis]

maybe it's better to add iplen as an option in the model and form instead of shape it into the tcp_ack selection.https://github.com/opnsense/core/issues/528 wasn't very great and if someone else wants to set iplen in the future we're going to run into all new kinds of races again.

[Nimloth]

@AdSchellevis I like it. It would probably be useless to attempt to find a one-size-fits-all scenario, so adding the iplen flag as an option for defining rules sounds like a good idea!

[maxfield-allison]

I agree, having the option to set the length separately in the gui makes more sense.

[maxfield-allison]

I'll take a look at this when I have some time this weekend, @AdSchellevis which files are you aware of that would need to be changed? I can dig through and find them if need be.

I imagine the iplen flag could be toggled and the field would take a numerical value. The help text would indicate some of the standard maximum sizes for packet types. What would a safe default be? Is the mtu available as a variable?

[AdSchellevis]

@maxfield-allison it shouldn't be very hard (I think iplen is compatible with all other options, which should avoid the need for referential constraints).

These are the files that come to mind:
https://github.com/opnsense/core/blob/master/src/opnsense/mvc/app/models/OPNsense/TrafficShaper/TrafficShaper.xml
https://github.com/opnsense/core/blob/master/src/opnsense/mvc/app/controllers/OPNsense/TrafficShaper/forms/dialogRule.xml

...and the ipfw template you already found.

The default should be empty (omit option) in which case it's still compatible with existing setups.

Our docs contain quite some content on how these models work by the way, see https://docs.opnsense.org/develop.html

[mimugmail]

Or something like this as an example:
https://github.com/opnsense/core/pull/2794/files

forms = UI
model = whats written/loaded in config.xml
template = you already know :)

[maxfield-allison]

Thanks for all of this information, You guys have all been great. This will be my first real contribution to a project so I'm very excited to learn and contribute more in the future.

[maxfield-allison]

So I've made the changes to dialogueRules.xml and TrafficShaper.xml and loaded them onto a test system while setting the originals to .bak but I'm getting page not found when navigating to the traffic shaper pipes, queues and rules pages. Is there a checksum that OPNsense uses to validate files? What could be happening?

[AdSchellevis]

case sensitivity? no checksum is being used while serving files.

[maxfield-allison]

That's what I thought too, double checked and all is well. I'm not building from source, just replacing the file, could that be the issue? After replacing the bak for each and reloading all services I'm still seeing page not found.

[AdSchellevis]

what are you trying to access and what exact message is returned?

[maxfield-allison]

Firewall>Shaper>Rules, Queues, Pipes
"Page not found" in the title where each would normally be displayed.
Console shows this
Failed to load resource: the server responded with a status of 404 (Not Found)

[AdSchellevis]

browser network inspector? what is it trying to access?

[maxfield-allison]

Request URL: https://10.1.0.1/ui/trafficshaper
Request Method: GET
Status Code: 404 Not Found
Remote Address: 10.1.0.1:443
Referrer Policy: same-origin

Looks like it isn't getting any page for trafficshaper ui
Cleared cache and tried incognito already as well.

[AdSchellevis]

something in /tmp/PHP_errors.log ? if the controller is there it should just pass the template, nothing special

[maxfield-allison]

Looking at backend, webgui, and general logs, I'm seeing the Web GUI starts, I'm seeing the traffic shaper statistics calls `

configd.py: [0e2a7c0d-49e8-4703-9575-374b83dbbc2b] request dummynet stats

`
but nothing from when I try to access the rules, queues, or pipes. pages. I'll check the php errors log

[maxfield-allison]

I'm not even seeing a php_errors.log in /tmp/ or anywhere else for that matter
Edit: Running a health check audit from web GUI

[maxfield-allison]

figured it out. I fat fingered a filename in the most obscure way possible.

[maxfield-allison]

Ok one easy question, How should I set the default to omit? I'm currently using

                <iplen type="IntegerField">
                    <MinimumValue>1</MinimumValue>
                    <MaximumValue>65535</MaximumValue>
                    <default></default>
                    <Required>N</Required>
                    <ValidationMessage>The absolute limitation for packet size is 64K (65535 bytes)</ValidationMessage>
                </iplen>

[AdSchellevis]

just leave the tag out?

[maxfield-allison]

ok, even with:

    <field>
        <id>rule.iplen</id>
        <label>IP Packet Length</label>
        <type>select_multiple</type>
        <advanced>true</advanced>
        <style>tokenize</style>
        <allownew>true</allownew>
        <help>Matches packets whose length is in the set. Specified in the same way as ports, Examples: 52-72, 1500, 1474</help>
    </field>

My thoughts were that since Iplen can take a list this would be the best way to implement. Open to suggestions though of course.
When using the tokenized style, I see resolve in the UI and I can't see another example where the field is left blank.

[AdSchellevis]

the integerfield expects a single value. question is if there are use-cases where you would like to match exact packet lengths, maybe a max_iplen is more logical in that case.

not required, default empty?
https://github.com/opnsense/core/blob/6b6e3ed32d6c3b54aec5fadd49ee66c0824c99fe/src/opnsense/mvc/app/models/OPNsense/TrafficShaper/TrafficShaper.xml#L36-L41

[maxfield-allison]

You're probably right. In the case where a single maximum iplen is set, users could simply create additional rules for different uses. I'm probably overcomplicating it.

[maxfield-allison]

@AdSchellevis I'm taking a look at copying and modifying PortField.php to create PacketSizeField.php. I think the ability to specify a range of packet lengths to match would be beneficial in this case. After looking over the documentation for IPFW dummynet, it appears that setting one specific size would necessitate the creation of many rules to accomplish one primary task. By allowing a range, we could cut down on the complexity of implementing shaping rules.

[AdSchellevis]

I think you can simplify this by specifying a max and writing 1-[max] in your ipfw.conf. If it needs a field type, it should remain in the module (https://docs.opnsense.org/development/frontend/models_customfields.html) since packetsize isn't something generic in my opinion.

Maybe it's better to discuss the scope of the feature first, I'm a bit worried we're overcomplicating things here if we can't think of a reason why multiple ranges would make sense. (besides that max_iplen can easily be migrated into a single range in the future).

[maxfield-allison]

That makes perfect sense. I had gotten through the XML and started looking at the ipfw.conf so was not aware that I could simply use 1-[max] to set the value as a range while only taking a single integer in the model and form.

[AdSchellevis]

This issue has been automatically timed-out (after 180 days of inactivity).

For more information about the policies for this repository,
please read https://github.com/opnsense/core/blob/master/CONTRIBUTING.md for further details.

If someone wants to step up and work on this issue,
just let us know, so we can reopen the issue and assign an owner to it.