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

Firewall ipset GUI #6187

Closed
wants to merge 4 commits into from
Closed

Firewall ipset GUI #6187

wants to merge 4 commits into from

Conversation

systemcrash
Copy link
Contributor

@systemcrash systemcrash commented Jan 6, 2023

Needs some feedback before commit.

Variously tested on 22.03.2. This is unlikely to land in 22.03.2, unless someone specially pulls/builds with this PR, so we can probably remove the loadfile GUI warning. See also this which seems related.

We could probably drop the if...else blocks that check for fw4 if there are no cases where units would run e.g. fw3 in builds from now onward.

Edit: displays string for firewall version (4 or not 4).

ipsets_grid

ipsets_modal

@jow-
Copy link
Contributor

jow- commented Jan 6, 2023

Great work! Two remarks:

  • The src_ and dest_ prefix for the type matches is optional and defaults to src. It can also be specified in the rule referencing the set (e.g. list ipset 'setname src,dest' to match the first field match of the set in the source direction and the second in the destination direction). This makes set declarations more flexible as they can be shared by both usptream and downstream direction rules. I suggest to drop the src_ and dest_ prefixes in the predefined choice list or at least add non-prefixed choices before the prefixed ones. Also add a human readable label for the predefined choices, e.g. o.value('ip', _('Match IP address ("ip")')) or o.value('src_ip', _('Match source IP address ("src_ip")')).
  • Please remove the "boot hang" warning. By the time the support will be merged in 22.03, it is long fixed. Also the issue is of a temporary nature while the ui description will stay for a very long time

@systemcrash
Copy link
Contributor Author

systemcrash commented Jan 7, 2023

Thanks for the feedback! Largely done. I'm still concerned about the boot hang, however. People who use a file list are still vulnerable to a boot hang if the firewall bump does not make it into 22.03.3 (and must opkg update manually).

BTW: How does one submit PRs for fw4 project? As a patch on the dev mailing list? Unless there is a reason that the comment field needs to be a bool, I'd like to make it a text string field (and update this, perhaps at a later stage). The man page for ipset clearly documents it as such.

@systemcrash systemcrash marked this pull request as ready for review January 7, 2023 15:10
@jow-
Copy link
Contributor

jow- commented Jan 8, 2023

if the firewall bump does not make it into 22.03.3

The boot fix was added a few days after the 22.03.2 release already, it will definitely be part of 22.03.3

@systemcrash
Copy link
Contributor Author

last push changed 'enabled' into checkboxes outside of modal, and put it to the last column on right, more in line to be consistent with other dialogues.

@systemcrash
Copy link
Contributor Author

LGTM

aparcar pushed a commit to openwrt/firewall4 that referenced this pull request Feb 4, 2023
The comment option for ipset definitions is incorrectly declared as bool
and not actually used anywhere in the nftables output rendering.

Solve this issue by changing it to the proper "string" type and expose
the user configured comment as "comment" property in the generated nftables
output.

Also add some initial test coverage for ipset declarations to better spot
such inconsistencies in the future.

Ref: openwrt/luci#6187 (comment)
Reported-by: Paul Dee <itsascambutmailmeanyway@gmail.com>
Signed-off-by: Jo-Philipp Wich <jo@mein.io>
Copy link
Member

@feckert feckert left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a small remark

@systemcrash
Copy link
Contributor Author

Hi @jow- any final comments?

@jow-
Copy link
Contributor

jow- commented Feb 17, 2023

The symbol looks weird in my font and is nowhere else used in the ui [1]. In the interest of consistency, please remove it and just spell it Port.

1:

$ grep -riE '\bport\b' modules/luci-base/po/templates/
modules/luci-base/po/templates/base.pot:msgid "Bridge port specific options"
modules/luci-base/po/templates/base.pot:msgid "DAE-Port"
modules/luci-base/po/templates/base.pot:msgid "DNS query port"
modules/luci-base/po/templates/base.pot:msgid "DNS server port"
modules/luci-base/po/templates/base.pot:msgid "Destination port"
modules/luci-base/po/templates/base.pot:msgid "Destination port"
modules/luci-base/po/templates/base.pot:msgctxt "VLAN port state"
modules/luci-base/po/templates/base.pot:msgid "Dynamic Authorization Extension port."
modules/luci-base/po/templates/base.pot:msgctxt "VLAN port state"
modules/luci-base/po/templates/base.pot:msgctxt "VLAN port state"
modules/luci-base/po/templates/base.pot:"Secure\">HTTPS</abbr> port."
modules/luci-base/po/templates/base.pot:msgid "Endpoint Port"
modules/luci-base/po/templates/base.pot:msgid "Expected port number."
modules/luci-base/po/templates/base.pot:msgid "External system log server port"
modules/luci-base/po/templates/base.pot:msgid "Fixed source port for outbound DNS queries."
modules/luci-base/po/templates/base.pot:msgid "Full port randomization"
modules/luci-base/po/templates/base.pot:msgid "Listen Port"
modules/luci-base/po/templates/base.pot:msgid "Listening port for inbound DNS queries."
modules/luci-base/po/templates/base.pot:msgid "Mirror monitor port"
modules/luci-base/po/templates/base.pot:msgid "Mirror source port"
modules/luci-base/po/templates/base.pot:"No fixed interface listening port defined, peers might not be able to "
modules/luci-base/po/templates/base.pot:"a non-standard Relay To port(<code>addr#port</code>)."
modules/luci-base/po/templates/base.pot:msgid "Optional. Port of peer."
modules/luci-base/po/templates/base.pot:msgid "Optional. UDP port used for outgoing and incoming packets."
modules/luci-base/po/templates/base.pot:msgid "Port"
modules/luci-base/po/templates/base.pot:msgctxt "WireGuard listen port"
modules/luci-base/po/templates/base.pot:msgid "Port %d"
modules/luci-base/po/templates/base.pot:msgid "Port isolation"
modules/luci-base/po/templates/base.pot:msgid "Port status:"
modules/luci-base/po/templates/base.pot:msgctxt "VLAN port state"
modules/luci-base/po/templates/base.pot:msgid "RADIUS Accounting Port"
modules/luci-base/po/templates/base.pot:msgid "RADIUS Authentication Port"
modules/luci-base/po/templates/base.pot:msgid "Randomize source port mapping"
modules/luci-base/po/templates/base.pot:msgctxt "nft redirect to port"
modules/luci-base/po/templates/base.pot:msgid "Redirect to local port <strong>%h</strong>"
modules/luci-base/po/templates/base.pot:msgctxt "nft dnat ip to addr:port"
modules/luci-base/po/templates/base.pot:msgid "Rewrite destination to <strong>%h</strong>, port <strong>%h</strong>"
modules/luci-base/po/templates/base.pot:msgctxt "nft dnat ip6 to addr:port"
modules/luci-base/po/templates/base.pot:msgid "Rewrite destination to <strong>%h</strong>, port <strong>%h</strong>"
modules/luci-base/po/templates/base.pot:msgctxt "nft snat ip to addr:port"
modules/luci-base/po/templates/base.pot:msgid "Rewrite source to <strong>%h</strong>, port <strong>%h</strong>"
modules/luci-base/po/templates/base.pot:msgctxt "nft snat ip6 to addr:port"
modules/luci-base/po/templates/base.pot:msgid "Rewrite source to <strong>%h</strong>, port <strong>%h</strong>"
modules/luci-base/po/templates/base.pot:msgid "SSH server port"
modules/luci-base/po/templates/base.pot:msgid "Source port"
modules/luci-base/po/templates/base.pot:msgid "Switch port"
modules/luci-base/po/templates/base.pot:msgid "TCP destination port"
modules/luci-base/po/templates/base.pot:msgid "TCP source port"
modules/luci-base/po/templates/base.pot:"segments. Often there is by default one Uplink port for a connection to the "
modules/luci-base/po/templates/base.pot:msgid "Transport header destination port"
modules/luci-base/po/templates/base.pot:msgid "Transport header source port"
modules/luci-base/po/templates/base.pot:msgid "UDP destination port"
modules/luci-base/po/templates/base.pot:msgid "UDP source port"
modules/luci-base/po/templates/base.pot:msgid "VEPA (Virtual Ethernet Port Aggregator)"
modules/luci-base/po/templates/base.pot:msgid "VPN Local port"
modules/luci-base/po/templates/base.pot:msgid "VPN Server port"
modules/luci-base/po/templates/base.pot:msgid "valid IPv4 address:port"
modules/luci-base/po/templates/base.pot:msgid "valid address:port"
modules/luci-base/po/templates/base.pot:msgid "valid host:port"
modules/luci-base/po/templates/base.pot:msgid "valid port or port range (port1-port2)"
modules/luci-base/po/templates/base.pot:msgid "valid port value"

@systemcrash
Copy link
Contributor Author

Done

@systemcrash
Copy link
Contributor Author

Resolved.

Enable it and place it between snats and custom tabs

Tested on 22.03.2, 22.03.3

Signed-off-by: Paul Dee <itsascambutmailmeanyway@gmail.com>
Signed-off-by: Paul Dee <itsascambutmailmeanyway@gmail.com>
Signed-off-by: Paul Dee <itsascambutmailmeanyway@gmail.com>
Signed-off-by: Paul Dee <itsascambutmailmeanyway@gmail.com>
@systemcrash
Copy link
Contributor Author

Resolved.

@systemcrash
Copy link
Contributor Author

@jow- apparently you already picked these into master. I guess I can close this? I was confused as to why my rebase was coming up empty 😉

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

Successfully merging this pull request may close these issues.

3 participants