Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve IPS mode help to avoid blocking network access, issue #4257 #4271

Merged
merged 3 commits into from
Sep 18, 2020
Merged

Improve IPS mode help to avoid blocking network access, issue #4257 #4271

merged 3 commits into from
Sep 18, 2020

Conversation

OliverO2
Copy link
Contributor

Current help texts do not contain complete information to prevent users blocking network access if IPS mode and hardware accelerations are enabled. See issue discussion and this posting plus follow-ups.

This PR aims to include information for all relevant settings.

@AdSchellevis
Copy link
Member

I don't think we should repeat the same message everywhere on the system_advanced_network.php page, a more elegant fix would probably be to move all hardware toggles into their own section (Hardware offloading) so it matches the rest of the warnings all around. Which would keep "ARP Handling" in its own section, which probably should get a more logical name in that case as well.

@OliverO2
Copy link
Contributor Author

Fair enough. This PR contains just the minimum viable set of changes which would prevent people from falling into that freezing trap. Doing as you suggested is even better but would require someone more familiar with the UI framework than me.

If you'd prefer to go the latter route, would you consider switching the checkbox logic from negative (Disable ...) to positive language (Enable ...)? This would avoid double negatives down the line, e.g. a tutorial stating "Unchecking 'Disable ...' may freeze your network".

@AdSchellevis
Copy link
Member

can we please keep this simple? the page itself is a legacy php page, which shouldn't be too hard to refactor. Changing the language may sound nice, but has the disadvantage of constantly flipping the underlaying toggle (which is called disable...)

@OliverO2
Copy link
Contributor Author

I have been trying to come up with an improved introductory help text and more precise help texts for all of the options. I found it hard to get to an acceptable quality level when sticking to the current language.

I have also found that the Disable ... settings do more than they say: When turned off, they actively enable the respective interface option (instead of leaving it at its default). Only the VLAN Hardware Filtering has a dedicated Leave default, which avoids touching the respective interface options.

I remember having been hit hard by falling into the VLAN Hardware Filtering trap, losing network connectivity, not being able to access the router's web or shell interface, not being able to attach a monitor as the HDMI port would only work with a monitor attached at boot time... So I'd be willing to put in some extra effort if this would improve the user experience but I'm not sure if that would be consistent with your priorities:

Would these changes be acceptable to you if I'd clean up the underlying toggles as well? The code in interfaces.lib.inc is already pretty hard to read (!isset(... ['disablevlanhwfilter'])). Is there a precedent for dealing with backward compatibility resulting from changes in configuration files?

There are gettext() calls on the PHP page and I'm not aware of whether translations are used or not. Can these safely be ignored?

My proposal would be to split the Network Interfaces section into two like this (settings at their current defaults):


Hardware Offloading

The following options are disabled by default because some NICs and/or drivers are broken. Enabling a hardware offloading option may completely block network connectivity when IPS mode is enabled in Services – Intrusion Detection – Administration.

  • Hardware CRC offloading: [ ]

    Choose whether to process checksums in hardware. Note that when this is enabled, a packet capture will see empty (all zero) or flag incorrect packet checksums. This is expected behavior with hardware checksum handling.

  • Hardware TSO: [ ]

    Choose whether to process outgoing TCP segmentation in hardware, also known as TCP segmentation offloading (TSO, TSO4, TSO6).

  • Hardware LRO: [ ]

    Choose whether to process incoming TCP segmentation in hardware, also known as large receive offloading (LRO).

  • VLAN Hardware Processing: Disable

    Choose whether to process VLAN tagging, filtering, outgoing TCP segmentation (TSO) and checksums in hardware. Setting this to 'Default' means that each NIC will retain its own default setting.

Logging

  • ARP: [x] Log ARP address changes

    Choose whether to make a log entry in the main system log when an IP address moves to a different MAC address.

@AdSchellevis
Copy link
Member

Since we don't offer config migrations on legacy code, we really try to prevent changes in there. The format itself looks workable from my point of view.

Improves option structure and help texts to avoid users blocking
network access inadvertently.
@OliverO2
Copy link
Contributor Author

@AdSchellevis Implemented as suggested.

@AdSchellevis AdSchellevis merged commit 02d05f7 into opnsense:master Sep 18, 2020
@AdSchellevis
Copy link
Member

@OliverO2 The new layout will likely prevent some users from impractical misconfigurations, in this case it won't hurt to add an extra warning (in most cases I'm not a fan of adding a lot of additional texts, but this particular issue seems to warrant it). thanks!

@OliverO2
Copy link
Contributor Author

@AdSchellevis Yes, hopefully this will help as intended. One trap still lingering is the IPS settings, as the warning appears in the (initially hidden) help text only.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants