net/fwblack: add new package - #30507
Conversation
DNS-based nftables blocklist daemon with LuCI app. Watches conntrack, reverse-resolves public IPs, drops TCP 80/443 toward blocklist matches in nftables sets. Dual-stack, procd-managed, UCI-configured with blocklist editor and reverse-DNS tester in LuCI. Upstream: https://github.com/EdgeBites/fw-black-luci Signed-off-by: Calin Vlad <calin@edgebites.com>
Formality Check: FailedWe checked this pull request against the contribution guidelines. Here is what needs your attention: 🛑 CRITICAL ERRORS
Tip Do not close this pull request to make corrections. Instead, modify your existing commits (e.g. Something broken? Consider reporting an issue. |
openwrt-ai
left a comment
openwrt-ai
left a comment
There was a problem hiding this comment.
Commit checks
0ec7c14"net/fwblack: add new package" — every body line is separated by a blank line, so each sentence becomes its own paragraph. Reflow the body into normal paragraphs and keepUpstream:/Signed-off-by:as trailers at the end.
Generated by Claude Code
| define Build/Prepare | ||
| endef |
There was a problem hiding this comment.
Overriding Build/Prepare with an empty body means the tarball is never unpacked — $(STAMP_PREPARED) only does mkdir -p $(PKG_BUILD_DIR), so every $(PKG_BUILD_DIR)/files/... path in both install recipes (and PKG_LICENSE_FILES) is missing at package time. Either drop this override and set PKG_BUILD_DIR:=$(BUILD_DIR)/fw-black-luci-$(PKG_VERSION) (the tarball's top-level dir; PKG_UNPACK extracts into $(PKG_BUILD_DIR)/..), or drop PKG_SOURCE/PKG_SOURCE_URL/PKG_HASH and install from the ./files tree this PR already vendors. Shipping both the tarball reference and a 1800-line copy of the same tree in the feed should not stay as-is either way.
Generated by Claude Code
|
|
||
| cache_clear: { | ||
| call: function(args) { | ||
| sh('rm -f ' + cache_path()); |
There was a problem hiding this comment.
cache_path() is a raw UCI value spliced into a root shell command, and the ACL grants "write": { "uci": [ "fwblack" ] } — so a session holding only this ACL can set fwblack.global.cache_file to /tmp/x; <cmd> and get arbitrary root execution via cache_clear. The same holds for tbl()/set_v4()/set_v6() at lines 99, 246 and 247. Validate every UCI-derived value before it reaches sh() (e.g. reject anything not matching ^[A-Za-z0-9_./-]+$), or build these commands from a fixed argument list instead of string concatenation.
Generated by Claude Code
| done < "$IFILE" | ||
| if [ -n "${batch_v4:-}" ]; then | ||
| # shellcheck disable=SC2086 | ||
| nft add element inet "$TABLE" "$SET_V4" "{ $batch_v4 }" || echo "fw-black: failed to block batch v4" >&2 |
There was a problem hiding this comment.
$IFILE is append-only (resips.sh never truncates /tmp/blacklist.ips), so from the second cycle on the batch re-adds elements that are already in the set and nft add element aborts the whole batch with EEXIST — every newly matched IP after cycle 1 is silently dropped, with only failed to block batch v4 in the log. Same at line 248. Make the update idempotent, e.g. feed flush set + add element to nft -f - in one transaction, or add elements individually and ignore an already-exists failure.
Generated by Claude Code
| # 99-fwblack - UCI defaults: migrate legacy /etc/fw.black layout once. | ||
| # Runs at first boot after opkg install (OpenWrt uci-defaults mechanism). | ||
|
|
||
| [ -e /etc/config/fwblack ] || { |
There was a problem hiding this comment.
nit: /etc/config/fwblack is installed by the package (and listed in conffiles), so this test is always true and the heredoc is dead code that duplicates files/etc/config/fwblack — a third place to keep in sync. Drop the block and keep only the legacy migration below.
Generated by Claude Code
| exit 0 | ||
| endef | ||
|
|
||
| define Package/luci-app-fwblack |
There was a problem hiding this comment.
nit: this feed carries no standalone luci-app-* package — LuCI applications normally live in openwrt/luci under applications/. Is bundling the app here intentional rather than submitting it to openwrt/luci?
Generated by Claude Code
| # elements — the daemon re-adds them from /tmp/blacklist.ips each cycle. | ||
|
|
||
| table inet fwblack | ||
| flush table inet fwblack |
There was a problem hiding this comment.
table/flush table/table at ruleset-post scope runs on every fw4 reload, wiping both sets while the daemon sleeps up to interval + jitter seconds before repopulating them — blocking silently lapses for minutes after any firewall reload. Consider having the daemon subscribe to a firewall reload trigger, or giving the sets a timeout/persisting them so the gap is bounded.
Generated by Claude Code
| endef | ||
|
|
||
| $(eval $(call BuildPackage,fwblack)) | ||
| $(eval $(call BuildPackage,luci-app-fwblack)) |
There was a problem hiding this comment.
Why we should have LuCI app in this packages repository? 🤔 🤔
3 participants
DNS-based nftables blocklist daemon with LuCI app.
Watches conntrack, reverse-resolves public IPs, drops TCP 80/443 toward blocklist matches in nftables sets. Dual-stack, procd-managed, UCI-configured with blocklist editor and reverse-DNS tester in LuCI.
Upstream: https://github.com/EdgeBites/fw-black-luci (v1.0.1, PKG_HASH verified via codeload tarball).
Tested: sh -n clean, JSON valid, PKG_VERSION==upstream tag, local smoke in openwrt/rootfs container per repo CI.
Signed-off-by: Calin Vlad calin@edgebites.com