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

luci-base: Enforce maximal firewall zone length in the Create / Assign scenario as well. #7549

Merged
merged 1 commit into from
Jan 10, 2025

Conversation

adelton
Copy link
Contributor

@adelton adelton commented Jan 9, 2025

  • This PR is not from my main or master branch 💩, but a separate branch ✅
  • Each commit has a valid ✒️ Signed-off-by: <[email protected]> row (via git commit --signoff)
  • Each commit and PR title has a valid 📝 <package name>: title first line subject for packages
  • Incremented 🆙 any PKG_VERSION in the Makefile
  • Tested on: local x86_64 VM with OpenWrt 23.05.5 (r24106-10cc5fcd00)
  • ( Preferred ) Mention: @ the original code author for feedback
  • ( Preferred ) Screenshot or mp4 of changes: Screenshot_2025-01-09_21-45-21
  • ( Optional ) Closes luci-app-firewall OR luci-app-network: Can create firewall zone in interfaces of any length but firewall allows only 11 chars #7522
  • ( Optional ) Depends on: e.g. openwrt/packages#pr-number in sister repo
  • Description: adding check for maximum firewall zone length also to the widget used in the Create / Assign scenario.

@systemcrash
Copy link
Contributor

@dannil your approach was a bit different, right?

@systemcrash systemcrash merged commit a075566 into openwrt:master Jan 10, 2025
5 checks passed
@systemcrash
Copy link
Contributor

Merged. Thanks @adelton

@dannil
Copy link
Contributor

dannil commented Jan 10, 2025

@dannil your approach was a bit different, right?

I guess you're thinking of the kinda related #7289? Then yes since interfaces appends their protocol when persisted to UCI, and this combination of protocol + interface name needs to be validated together which isn't done today, but that problem shouldn't exist for firewall zones. The solution in this PR for this specific problem should definitely suffice.

@systemcrash
Copy link
Contributor

The solution in this PR for this specific problem should definitely suffice.

Good. That's what I figured. Thanks for the check!

@jow-
Copy link
Contributor

jow- commented Jan 10, 2025

This length constraint should only apply to fw3, fw4 (nft) has no zone name length limit iirc.

systemcrash added a commit that referenced this pull request Jan 10, 2025
firewall3 enforces the zone name length. firewall4 does not.

#7549 (comment)

Signed-off-by: Paul Donald <[email protected]>
@systemcrash
Copy link
Contributor

@jow- OK. I'll add a fix to relax that.

See a67d683

@adelton
Copy link
Contributor Author

adelton commented Jan 10, 2025

@systemcrash Please see #7557 as a followup to a67d683.

@adelton
Copy link
Contributor Author

adelton commented Jan 14, 2025

This PR caused a regression #7563 and was reverted in 7046a1c.

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