luci-app-geoguard: add GeoGuard country allow-list IPS with login guard by chinoyan2002 · Pull Request #9027 · openwrt/luci

chinoyan2002 · 2026-09-13T20:14:29Z

New LuCI application: GeoGuard builds firewall4 ipset sets from IP-geolocation feeds (dual sources with auto-failover), plus a custom whitelist, a DDNS no-ban list and a LuCI/SSH login guard backed by a full-block nftables chain. English source strings (translations via Weblate post-merge), following the banIP precedent of shipping set builder and guard together.

Self-contained shell + JS, no compiled backend. Depends on luci-base, firewall4, wget-ssl and jsonfilter.

Verified live (not just built): installed and exercised on OpenWrt 25.12.5 x86/64 (apk) and 24.10 (opkg) real hardware — set sync incl. renames, guard bans with exemption layers, DDNS tracking, procd service and fw4 reloads. I do not have a full buildroot here, so buildroot compile coverage from CI is appreciated.

Supersedes #9023 (closed after a broken force-push rewrote the head commit without its parent; the branch now carries the identical payload as a single well-formed commit, addressing all findings of the openwrt-ai reviews).

Upstream repo: https://github.com/chinoyan2002/luci-app-geoguard (MIT), prebuilt packages under its Releases.

Signed-off-by: Yang Min Sheng chinoyan@gmail.com

openwrt-ai

Commit checks

436bae5 "luci-app-geoguard: add GeoGuard country allow-list IPS builder" — the fourth bullet ("Second-round review: package-owned state dir ...") describes deltas against an earlier, unmerged revision of this same commit; that revision is not in the tree, so the bullet is meaningless in history. Describe the shipped behaviour instead, or drop the bullet.
436bae5 — the sign-off name (Yang Min Sheng) does not match the one in the PR description (Yan Minsheng); make the two agree.

Generated by Claude Code

openwrt-ai · 2026-09-15T04:23:11Z

+  { line = $1;
+    if (line ~ /\//) { split(line, a, "/"); b = i2n(a[1]); m = a[2]+0;
+      if (m == 0) next;
+      if (rshift(needle, 32-m) == rshift(b, 32-m)) { found = 1; exit; } }


rshift() is a gawk extension; busybox awk (the awk on an OpenWrt target) has no bitwise builtins, so this program aborts with "function rshift never defined" and in_list exits non-zero for every address. That silently disables both exemption layers that use it — in_exempt and the whitelist-file check in ban_ip — so RFC1918 and whitelisted sources become bannable. Use plain arithmetic instead.

Suggested change

if (rshift(needle, 32-m) == rshift(b, 32-m)) { found = 1; exit; } }

if (int(needle / 2^(32-m)) == int(b / 2^(32-m))) { found = 1; exit; } }

Generated by Claude Code

fixed, thanks

Generated by Claude Code

openwrt-ai · 2026-09-15T04:23:17Z

+		"write": {
+			"uci": [ "geoguard" ],
+			"file": {
+				"/etc/geoguard/*.cidr": [ "write" ],


The view never calls fs.read/fs.write — all .cidr files are produced by the root helpers — so this grant is unused and lets anyone holding the luci-app-geoguard ACL push arbitrary content into files that fw4 consumes through option loadfile. Drop it, and the matching "read" entry on line 7.

Generated by Claude Code

fixed, thanks

Generated by Claude Code

openwrt-ai · 2026-09-15T04:23:21Z

+LUCI_DEPENDS:=+luci-base +firewall4 +wget-ssl +jsonfilter
+LUCI_PKGARCH:=all
+
+include ../../luci.mk


/etc/config/geoguard is not declared as a conffile and neither luci.mk nor package-pack.mk registers /etc/config/* automatically, so every package upgrade overwrites the user's country selection, whitelist and guard settings with the shipped defaults.

Suggested change

include ../../luci.mk

define Package/luci-app-geoguard/conffiles

/etc/config/geoguard

endef

include ../../luci.mk

Generated by Claude Code

fixed, thanks

Generated by Claude Code

openwrt-ai · 2026-09-15T04:23:27Z

+    grep -E '^((25[0-5]|2[0-4][0-9]|1[0-9]{2}|[1-9]?[0-9])\.){3}(25[0-5]|2[0-4][0-9]|1[0-9]{2}|[1-9]?[0-9])/([1-9]|[12][0-9]|3[0-2])$' "$clean" > "$clean.strict" 2>/dev/null
+    mv -f "$clean.strict" "$clean"
+    [ "$(wc -l < "$clean")" -ge 15 ] || continue
+    mv -f "$clean" "$BASE_DIR/${cc}.cidr"


Per-country files are rewritten on every scheduled run whether or not the feed changed, and $BASE_DIR is /etc/geoguard — i.e. hundreds of KB per country written to the overlay flash weekly, forever. geoguard-update already guards its own writes with cmp -s; do the same here.

Suggested change

mv -f "$clean" "$BASE_DIR/${cc}.cidr"

cmp -s "$clean" "$BASE_DIR/${cc}.cidr" || mv -f "$clean" "$BASE_DIR/${cc}.cidr"

Generated by Claude Code

fixed, thanks

Generated by Claude Code

openwrt-ai · 2026-09-15T04:23:31Z

+# H2: single-flight (cron vs manual clicks). mkdir is atomic; stale lock
+# (kill -9) is reclaimed via pid check.
+LOCKDIR="$BASE_DIR/.update.lockdir"
+if ! mkdir "$LOCKDIR" 2>/dev/null; then


geoguard-fetch is exec-granted in the ACL and wired to the "Update IP Sets Now" button, but it takes no lock, so a UI click races a cron geoguard-update: both mv into $BASE_DIR/<cc>.cidr and both truncate/read $BASE_DIR/.fetch-results, which can leave the merge reading a half-written result set. Move this lock acquisition into geoguard-fetch (or make it a shared helper both scripts source).

Generated by Claude Code

fixed, thanks

Generated by Claude Code

openwrt-ai · 2026-09-15T04:23:37Z

+[ "$DDNS_I" -lt 1 ] && DDNS_I=1
+[ "$DDNS_I" -gt 60 ] && DDNS_I=60
+DNOTE="$NOTE_DDNS"; UNOTE="$NOTE_UPDATE"
+echo "*/$DDNS_I * * * * /usr/bin/geoguard-ddns $DNOTE" >> "$CRON_FILE"


/etc/crontabs/root is frequently left without a trailing newline (the LuCI scheduled-tasks editor saves exactly what was typed), and appending then concatenates GeoGuard's entry onto the user's last cron line, breaking both. Normalize first; line 42 and geoguard-ddns:25 append to the same file and are covered once this runs before them.

Suggested change

echo "*/$DDNS_I * * * * /usr/bin/geoguard-ddns $DNOTE" >> "$CRON_FILE"

[ -s "$CRON_FILE" ] && [ -n "$(tail -c 1 "$CRON_FILE")" ] && echo "" >> "$CRON_FILE"

echo "*/$DDNS_I * * * * /usr/bin/geoguard-ddns $DNOTE" >> "$CRON_FILE"

Generated by Claude Code

fixed, thanks

Generated by Claude Code

openwrt-ai · 2026-09-15T04:23:40Z

+#!/bin/sh
+# 41-luci-app-geoguard-ban: generate the guard from the live WAN interface,
+# then enable/start the procd service (runs once at install).
+chmod +x /usr/bin/geoguard-ban /usr/bin/geoguard-ban-status /usr/bin/geoguard-ban-unban /etc/init.d/geoguard-ban


nit: all four of these ship with mode 100755 in the tree, so this line is a no-op — drop it.

Generated by Claude Code

fixed, thanks

Generated by Claude Code

openwrt-ai · 2026-09-15T04:23:46Z

+	iifname "pppoe-wan" ip saddr @geoguard_ban limit rate over 10/minute burst 5 packets log prefix "geoguard-ban-drop: " drop
+	iifname "pppoe-wan" ip saddr @geoguard_ban drop


nit: this file is a packaged file that geoguard-ban-guard rewrites at runtime, so a package upgrade restores this pppoe-wan default and the guard stops matching on every non-PPPoE WAN until the 60 s self-heal in geoguard-ban notices. Either declare it a conffile too, or generate it into a non-packaged path.

Generated by Claude Code

solved by declaring it a conffile in the Makefile, thanks

Generated by Claude Code

chinoyan2002 · 2026-09-15T14:05:11Z

All 9 findings from the latest review addressed in 41c2514 (single commit, same branch): portable awk division instead of rshift() in geoguard-ban in_list; unused /etc/geoguard/*.cidr read/write ACL grants dropped (view has no fs.read/fs.write); conffiles stanza covering /etc/config/geoguard and the runtime-regenerated guard nft file; cmp -s guard before installing country files in geoguard-fetch; shared lockdir single-flight between fetch (standalone) and update (via GEOGUARD_LOCKED bypass); crontab trailing-newline normalization at all three append sites; no-op chmod line dropped (helpers/init ship 100755); commit message reworded to describe the payload instead of deltas vs unmerged revisions.

openwrt-ai

Commit checks

41c2514 "luci-app-geoguard: add GeoGuard country allow-list IPS builder" — the sign-off name (Yang Min Sheng) still disagrees with the one in the PR description (Yan Minsheng); make the two match.

Generated by Claude Code

openwrt-ai · 2026-09-15T16:13:15Z

+mkdir -p "$BASE_DIR" "$TEMP_DIR"
+# Single-flight with geoguard-update: same lockdir path as update's H2 lock.
+# Skipped when invoked by geoguard-update itself (GEOGUARD_LOCKED=1 exported
+# by the caller, which already holds the lock).
+LOCKDIR="$BASE_DIR/.update.lockdir"
+if [ -z "$GEOGUARD_LOCKED" ]; then
+  if ! mkdir "$LOCKDIR" 2>/dev/null; then
+    LPID=$(cat "$LOCKDIR/pid" 2>/dev/null)
+    if [ -n "$LPID" ] && kill -0 "$LPID" 2>/dev/null; then
+      logger -t geoguard "fetch skipped: another run in progress (pid $LPID)"
+      exit 0
+    fi
+    rm -rf "$LOCKDIR"
+    mkdir "$LOCKDIR" 2>/dev/null || { logger -t geoguard "fetch skipped: lock contention"; exit 0; }
+  fi
+  echo "$$" > "$LOCKDIR/pid"
+  trap 'rm -rf "$TEMP_DIR" "$LOCKDIR"' EXIT
+fi
+trap 'rm -rf "$TEMP_DIR"' EXIT


The unconditional trap on line 36 replaces the one set on line 34, so a standalone fetch never removes $BASE_DIR/.update.lockdir — every later run has to reclaim a stale lock, and if the recorded pid has been reused by an unrelated live process both fetch and geoguard-update silently exit 0 forever. The two exit 0 contention paths also run before any trap is installed, leaking /tmp/cafetch-$$. Install the temp-only trap before the lock block and upgrade it in place once the lock is held.

Suggested change

mkdir -p "$BASE_DIR" "$TEMP_DIR"

# Single-flight with geoguard-update: same lockdir path as update's H2 lock.

# Skipped when invoked by geoguard-update itself (GEOGUARD_LOCKED=1 exported

# by the caller, which already holds the lock).

LOCKDIR="$BASE_DIR/.update.lockdir"

if [ -z "$GEOGUARD_LOCKED" ]; then

if ! mkdir "$LOCKDIR" 2>/dev/null; then

LPID=$(cat "$LOCKDIR/pid" 2>/dev/null)

if [ -n "$LPID" ] && kill -0 "$LPID" 2>/dev/null; then

logger -t geoguard "fetch skipped: another run in progress (pid $LPID)"

exit 0

fi

rm -rf "$LOCKDIR"

mkdir "$LOCKDIR" 2>/dev/null || { logger -t geoguard "fetch skipped: lock contention"; exit 0; }

fi

echo "$$" > "$LOCKDIR/pid"

trap 'rm -rf "$TEMP_DIR" "$LOCKDIR"' EXIT

fi

trap 'rm -rf "$TEMP_DIR"' EXIT

mkdir -p "$BASE_DIR" "$TEMP_DIR"

trap 'rm -rf "$TEMP_DIR"' EXIT

# Single-flight with geoguard-update: same lockdir path as update's H2 lock.

# Skipped when invoked by geoguard-update itself (GEOGUARD_LOCKED=1 exported

# by the caller, which already holds the lock).

LOCKDIR="$BASE_DIR/.update.lockdir"

if [ -z "$GEOGUARD_LOCKED" ]; then

if ! mkdir "$LOCKDIR" 2>/dev/null; then

LPID=$(cat "$LOCKDIR/pid" 2>/dev/null)

if [ -n "$LPID" ] && kill -0 "$LPID" 2>/dev/null; then

logger -t geoguard "fetch skipped: another run in progress (pid $LPID)"

exit 0

fi

rm -rf "$LOCKDIR"

mkdir "$LOCKDIR" 2>/dev/null || { logger -t geoguard "fetch skipped: lock contention"; exit 0; }

fi

echo "$$" > "$LOCKDIR/pid"

trap 'rm -rf "$TEMP_DIR" "$LOCKDIR"' EXIT

fi

Generated by Claude Code

fixed, thanks

Generated by Claude Code

openwrt-ai · 2026-09-15T16:13:21Z

+		"read": {
+			"uci": [ "geoguard" ],
+			"file": {
+			"/usr/bin/geoguard-status": [ "exec" ],


nit: three tabs instead of four — indentation no longer lines up with the sibling entries. Same on line 18.

Suggested change

"/usr/bin/geoguard-status": [ "exec" ],

"/usr/bin/geoguard-status": [ "exec" ],

Generated by Claude Code

fixed, thanks

Generated by Claude Code

chinoyan2002 · 2026-09-16T18:14:56Z

Follow-up c8f66e1 addresses the re-review: fetch installs the temp-only trap before the lock block and upgrades it once the lock is held (no more leaked lockdir/tmpdir); ACL entry indentation fixed to four tabs; PR description sign-off name aligned to Yang Min Sheng.

chinoyan2002 · 2026-09-16T19:01:46Z

Follow-up ab0b9f9 (motivation: state wiped by sysupgrade while firewall kept referencing the loadfiles, plus an empty-set window on every slow refill): new /lib/upgrade/keep.d/luci-app-geoguard covering /etc/geoguard (same mechanism as luci-app-rustdesk-server, works for both opkg and apk sysupgrades); live refresh is now a single nft -f transaction (flush + refill atomically, old content stays on error); install seeds an empty state once in the background when a selection exists (non-blocking, never fails install).

chinoyan2002 · 2026-09-16T20:12:04Z

Follow-up bc9869d: update now logs a loud WARNING naming the missing state file (fw4 silently materializes empty sets for dangling loadfiles, so the wipe previously failed without a sound). No other changes.

openwrt-ai · 2026-09-17T04:17:57Z

@@ -0,0 +1 @@
+/etc/geoguard


This carries the whole directory across sysupgrade, including the transient .update.lockdir/pid. The restored pid is then fed to kill -0 in geoguard-update:26-34, so any unrelated live process holding that pid on the new system makes both update and fetch exit 0 forever. Move the lockdir to /var/lock (tmpfs, cleared every boot) so only real state lives under /etc/geoguard.

Generated by Claude Code

Fixed in 1339e1a: the single-flight lockdir moved to /var/lock (tmpfs) in both fetch and update; keep.d still covers /etc/geoguard for real state only, so no stale pid survives sysupgrade.

openwrt-ai · 2026-09-17T04:18:05Z

+  if [ -n "$(uci -q get geoguard.main.selected 2>/dev/null)$(uci -q get geoguard.main.whitelist 2>/dev/null)" ]; then
+    logger -t geoguard "empty state with selection, seeding in background"
+    ( /usr/bin/geoguard-update >/dev/null 2>&1 & )


The shipped config already contains list selected 'tw', so this condition is true on every fresh install and on first boot of any image built with the package — each one silently starts background downloads from ipdeny.com/github and commits firewall ipset sections before the user has ever opened the UI. Line 18 still tells them to press update-merge once, so the script contradicts itself. Gate the seeding on a marker the UI writes on first save, not on the shipped default selection.

Generated by Claude Code

Fixed in 1339e1a: background seeding is now gated on the ui_saved marker that the view writes on first save; the shipped default selection no longer triggers silent downloads and firewall commits.

openwrt-ai · 2026-09-17T04:18:11Z

+  if [ ! -s "$fl" ]; then
+    # Loud missing-state warning: fw4 silently materializes empty sets for
+    # dangling loadfiles, so say it here instead of failing quietly.
+    logger -t geoguard "WARNING state file missing, skipped live refresh: $fl (wiped? restore backup or run update-merge)"
+    continue


nit: unreachable. $DESIRED only takes a country entry whose file already passed [ -s ] at line 127, and $OUTPUT/$WOUTPUT are guaranteed non-empty by the no data, skip / merged empty, skip guards at lines 117-120 plus the writes at 141-150 — the wipe case this announces exits long before here.

Generated by Claude Code

Fixed in 1339e1a: dropped the unreachable block.

chinoyan2002 · 2026-09-17T19:55:01Z

Follow-up 1339e1a addresses the re-review: single-flight lockdir lives on tmpfs (/var/lock) in both fetch and update, so keep.d can no longer carry a stale pid across sysupgrade; install-time background seeding is gated on the ui_saved first-save marker instead of the shipped default selection; the unreachable missing-state warning in live refresh is dropped. Single commit, same branch; sign-off unchanged.

chinoyan2002 · 2026-09-17T20:02:35Z

Follow-up b85849e: message-only amend, rewrapped the commit body to the 100-column limit flagged by Formality Check (no code changes). All FormalityCheck suites pass.

openwrt-ai · 2026-09-18T04:25:46Z

+# scheduled run (or manual update) picks it up later.
+SETNAME=$(uci -q get geoguard.main.setname 2>/dev/null)
+[ -z "$SETNAME" ] && SETNAME="allowed-IPList"
+if [ "$(uci -q get geoguard.main.ui_saved 2>/dev/null)" = "1" ] && [ ! -s "/etc/geoguard/$SETNAME.cidr" ]; then


The marker only gates this one path: line 5 runs geoguard-cron unconditionally, and with the shipped option auto_update '1' plus list selected 'tw' that installs the weekly geoguard-update job, so an untouched box still downloads the tw feed and commits firewall ipset sections at the first scheduled run. Gate the schedule on the same marker, or ship auto_update '0' / no default selection.

Generated by Claude Code

Fixed in 01ee42e: applied both suggested alternatives. Shipped config now has auto_update 0 and no default selection, and 40 installs no schedule before first UI save. On upgrade with a user-saved config, 40 re-syncs cron and restores missing state so referenced sets do not go empty.

openwrt-ai · 2026-09-18T04:25:52Z

+SELECTED="$SELECTED $(uci -q get geoguard.main.selected 2>/dev/null)"
+WHITELIST=$(uci -q get geoguard.main.whitelist 2>/dev/null)
+
+mkdir -p "$BASE_DIR" "$TEMP_DIR"


nit: the two lock-contention exit 0 paths below run before the trap on line 39 is installed, so /tmp/geoguard-$$ is left behind on every skipped run — same pattern that geoguard-fetch now installs a temp-only trap for.

Suggested change

mkdir -p "$BASE_DIR" "$TEMP_DIR"

mkdir -p "$BASE_DIR" "$TEMP_DIR"

trap 'rm -rf "$TEMP_DIR"' EXIT

Generated by Claude Code

Fixed in 01ee42e: temp-only trap installed right after mkdir, upgraded once the lock is held. Same pattern as fetch.

chinoyan2002 · 2026-09-18T05:00:18Z

Follow-up 01ee42e: fresh-install-quiet policy. Shipped defaults all off (no selection, schedule off, guard off); uci-defaults stay inert without a user-saved config and restore schedule, missing state and guard service on upgrade. Update installs the temp-only trap before the lock; guard save persists boot enablement per flag (two added ACL exec entries). Single commit, same branch; sign-off unchanged; body within 100 cols.

openwrt-ai · 2026-09-18T16:18:54Z

+
+		/* ---- Login guard tab (compact 7-row layout) ---- */
+		o = s.taboption('ban', form.Flag, 'ban_enabled', _('Enable this option to block IP addresses with too many failed logins'));
+		o.default = '1';


nit: the shipped config now carries option ban_enabled '0' and line 825 falls back to '0', so this is the last place that still treats an absent flag as "guard on" — a config without the option renders the checkbox ticked and saves 1.

Suggested change

o.default = '1';

o.default = '0';

Generated by Claude Code

Fixed in 34f6dec: Flag default flipped to 0, matching shipped config. An absent option now renders unticked.

openwrt-ai · 2026-09-18T16:19:01Z

+if [ "$(uci -q get geoguard.main.ban_enabled 2>/dev/null)" != "1" ]; then
+  logger -t geoguard "ban installed, guard stays off until enabled in the UI"
+  exit 0


nit: /etc/nftables.d/10-geoguard-guard.nft ships in the package and fw4 pulls it in unconditionally via include "/etc/nftables.d/*.nft", so even on this inert path a fresh install ends up with the luci_guard prerouting base chain and the geoguard_ban set in inet fw4, keyed on a hardcoded pppoe-wan. Harmless in effect (empty set, policy accept), but it does contradict the "fresh install stays inert" claim — intentional, or should the guard file be generated on enable rather than packaged?

Generated by Claude Code

Fixed in 34f6dec: packaged guard file is now an inert comment-only placeholder, so fresh installs get no chain and no set. The live chain and set are generated by geoguard-ban-guard on first enable (it already rewrites the file wholesale and reloads fw4 on change). Conffiles entry kept so upgrades never clobber a live guard.

chinoyan2002 · 2026-09-18T17:44:26Z

Follow-up 34f6dec: fail-quiet flag default (absent ban_enabled renders unticked) and inert packaged guard file (live chain and set generated on first enable; conffiles entry kept). Single commit, same branch; sign-off unchanged; body within 100 cols.

openwrt-ai · 2026-09-19T04:25:57Z

+# GeoGuard login guard (inert until enabled in the UI).
+# geoguard-ban-guard writes the live chain + set on first enable
+# and reloads fw4 only when the content changes.


saveBan calls geoguard-ban-guard at geoguard.js:821`` on every guard-tab save regardless of ban_enabled, so the `luci_guard` chain and `geoguard_ban` set land in `inet fw4` on the first save even with the flag left off — not "on first enable". Nothing ever writes this inert body back either, so disabling the guard leaves the chain installed for good. Gate the regen on the flag and have `geoguard-ban-guard` emit this placeholder when disabled.

Generated by Claude Code

Fixed in c1d3c5b: ban-guard now branches on the flag itself. Disabled emits the inert placeholder (chain and set removed, fw4 reload only on change), so any guard-tab save with the flag off restores inert instead of installing the chain. saveBan keeps calling it unconditionally.

openwrt-ai · 2026-09-19T04:26:04Z

+g() { uci get $CFG.main."$1" 2>/dev/null; }
+num() { case "$1" in ''|*[!0-9]*) echo "$2" ;; *) echo "$1" ;; esac; }
+
+EN=$(g ban_enabled); [ -z "$EN" ] && EN=1


nit: an absent ban_enabled still means "guard on" here, which now disagrees with the shipped option ban_enabled '0' and the view's new '0' default. start_service at init.d/geoguard-ban:7 has the same polarity — its = "0" test starts the service when the option is missing.

Suggested change

EN=$(g ban_enabled); [ -z "$EN" ] && EN=1

EN=$(g ban_enabled); [ -z "$EN" ] && EN=0

Generated by Claude Code

Fixed in c1d3c5b: both default to off when absent (ban line 14 EN=0 per suggestion; init.d start_service returns unless the value is 1), matching shipped 0 and the view. Full-repo audit: all eight ban_enabled sites now fail-quiet.

GeoGuard builds firewall4 ipset sets from IP-geolocation feeds for selected countries plus a custom whitelist, with a DDNS guard exemption list and a LuCI/SSH login guard. - 217 countries, dual feeds with auto-failover, daily/weekly/monthly refresh - Per-helper rpcd ACL entries; LUCI_DEPENDS on luci-base, firewall4, wget-ssl and jsonfilter - Guard and schedule generated on first UI save (fresh install is inert); English source strings with source references in the .pot template - State in package-owned /etc/geoguard (upgrade migration, conffiles, sysupgrade backup), single-flight lock on tmpfs, reboot-safe ban persistence, atomic live refresh, missing-state restore on upgrade, EN-only search placeholder, no double _(), ASCII punctuation, English comments throughout Signed-off-by: Yang Min Sheng <chinoyan@gmail.com>

chinoyan2002 · 2026-09-19T07:25:44Z

Follow-up c1d3c5b: guard regen gated on the flag inside ban-guard (disabled emits the placeholder, removing chain and set); absent ban_enabled means off in ban, init.d and the view (full-repo polarity audit). Single commit, same branch; sign-off unchanged; body within 100 cols.

openwrt-ai · 2026-09-19T16:12:45Z

+		o = s.taboption('ban', form.Flag, 'ban_enabled', _('Enable this option to block IP addresses with too many failed logins'));
+		o.default = '0';
+		o.rmempty = false;
+		o.description = _('Disabling stops the guard service; active bans stay until expiry.');


Disabling now writes the placeholder, which removes the luci_guard chain and the geoguard_ban set on the next fw4 reload, so live bans stop being enforced immediately instead of running to expiry. Update this string and the matching msgid in po/templates/geoguard.pot.

Suggested change

o.description = _('Disabling stops the guard service; active bans stay until expiry.');

o.description = _('Disabling stops the guard service and removes the guard chain and ban set; active bans are dropped.');

Generated by Claude Code

openwrt-ai · 2026-09-19T16:12:55Z

+START=99
+
+start_service() {
+  [ "$(uci get geoguard.main.ban_enabled 2>/dev/null)" = "1" ] || return 0


With the flag off the guard file is now the placeholder, so geoguard_ban no longer exists; an enable done outside the UI (uci set + start) then dies on the nft list set … || exit 0 bail at geoguard-ban:9 before the self-heal at lines 30-33 can regenerate it, and the service loops doing nothing with only a "restore skipped (set not ready)" line in the log. Regenerate the guard here before restoring the persisted bans.

Suggested change

[ "$(uci get geoguard.main.ban_enabled 2>/dev/null)" = "1" ] || return 0

[ "$(uci get geoguard.main.ban_enabled 2>/dev/null)" = "1" ] || return 0

# the guard file may still be the inert placeholder (enable outside the UI)

/usr/bin/geoguard-ban-guard >/dev/null 2>&1

Generated by Claude Code

openwrt-ai · 2026-09-19T16:13:01Z

+else
+


nit: the whole else body (lines 22-49) stays at column 0 while the rest of the file indents two spaces per level, and a blank line is left hanging after else. Indent the branch so the block boundaries are visible.

Generated by Claude Code

dibdot · 2026-09-19T16:20:43Z

Disclosure up front: I maintain banIP, so take the overlap argument below with that in mind — but it isn't the main reason I'd say no to this.

Wrong repo. This isn't a LuCI app, it's a complete backend — feed fetcher, merge logic, ban daemon, DDNS tracker, cron management, nftables includes, uci-defaults, /etc/config — with a view bolted on top. luci.git holds frontends; backends belong in openwrt/packages. The description cites banIP as precedent for "shipping set builder and guard together", and that isn't accurate: banIP's backend is net/banip in packages.git, luci-app-banip is frontend-only and talks to it over rpcd/ubus. The same split applies here.

Duplication. Country feeds in allow-list-only mode plus banning of failed LuCI/SSH logins off the system log is what banIP does today. Plain geoblocking is covered by net/geoip-shell. I don't see a capability here that OpenWrt is currently missing, and the cost of a third implementation lands on maintainers, translators and users rather than on a feature gap.

The verification claim doesn't hold. The first review round found rshift() in geoguard-ban's in_list. busybox awk has no bitwise builtins, so the program aborts, in_list returns non-zero for every address, and both exemption layers — RFC1918 and the whitelist file — silently stop matching, making those sources bannable. That cannot have run on a real target. It contradicts "verified live (not just built)" in the description, and it sits in exactly the path where a silent failure locks you out of your own router.

Platform fit. Most of these were addressed during review; the pattern is what worries me, not the individual findings. Country lists written into /etc (overlay flash) and carried across sysupgrade via keep.d; hand-appending to /etc/crontabs/root; a self-rolled lockdir with pid reclaim instead of procd or flock; hardcoded pppoe-wan in the packaged nft file; CIDR matching done in awk arithmetic instead of an nft set lookup; an ACL that granted write access to files fw4 reads back through loadfile. For a package whose job is keeping traffic out, that's not something I'd sign off on after a bot-only review.

NAK to this package from my side. What I'd suggest, in the order I think it's worth your time:

Keep it in your own repo with a release feed. Users who want it can install it, and you keep full control over your release pace and your design decisions — no Weblate cycle, no distro review latency.
If you do want it in the distro: split it. Backend as its own package in openwrt/packages, thin luci-app here on top of a documented ubus interface, and a commitment to maintain both long-term.

BTW, "IPS" in the title oversells this. It's a country allow-list plus a failed-login banner; naming it as such would set fairer expectations.

chinoyan2002 · 2026-09-19T18:51:59Z

Withdrawing this PR. The backend-in-luci concern and the duplication call are the maintainers to make; the package continues in my own repo with a release feed. For the record: the rshift slipped through because live testing never exercised the ban-exemption paths, so verified-live overstated that, lesson recorded. Several cited items (ACL write grants, lockdir under keep.d, hardcoded packaged guard, missing conffiles) were already fixed in-tree during review, and the round-8 nits are fixed in my own tree. Thanks to openwrt-ai for the thorough rounds and to dibdot for the direct feedback.

openwrt Bot added the add package Introduces a new package Makefile build script label Sep 13, 2026

chinoyan2002 mentioned this pull request Sep 13, 2026

luci-app-geoguard: add GeoGuard country allow-list IPS with login guard #9023

Closed

openwrt-ai reviewed Sep 15, 2026

View reviewed changes

chinoyan2002 force-pushed the add-luci-app-geoguard branch from 436bae5 to 41c2514 Compare September 15, 2026 14:04

openwrt-ai reviewed Sep 15, 2026

View reviewed changes

chinoyan2002 force-pushed the add-luci-app-geoguard branch from 41c2514 to c8f66e1 Compare September 16, 2026 18:14

chinoyan2002 force-pushed the add-luci-app-geoguard branch from c8f66e1 to ab0b9f9 Compare September 16, 2026 19:01

chinoyan2002 force-pushed the add-luci-app-geoguard branch from ab0b9f9 to bc9869d Compare September 16, 2026 20:11

openwrt-ai reviewed Sep 17, 2026

View reviewed changes

chinoyan2002 force-pushed the add-luci-app-geoguard branch from bc9869d to 1339e1a Compare September 17, 2026 19:54

openwrt Bot added the not following guidelines Pull request does not follow formatting guidelines label Sep 17, 2026

chinoyan2002 force-pushed the add-luci-app-geoguard branch from 1339e1a to b85849e Compare September 17, 2026 20:00

openwrt Bot removed the not following guidelines Pull request does not follow formatting guidelines label Sep 17, 2026

openwrt-ai reviewed Sep 18, 2026

View reviewed changes

chinoyan2002 force-pushed the add-luci-app-geoguard branch from b85849e to 01ee42e Compare September 18, 2026 04:58

openwrt-ai reviewed Sep 18, 2026

View reviewed changes

chinoyan2002 force-pushed the add-luci-app-geoguard branch from 01ee42e to 34f6dec Compare September 18, 2026 17:44

openwrt-ai reviewed Sep 19, 2026

View reviewed changes

chinoyan2002 force-pushed the add-luci-app-geoguard branch from 34f6dec to c1d3c5b Compare September 19, 2026 07:25

openwrt-ai reviewed Sep 19, 2026

View reviewed changes

chinoyan2002 closed this Sep 19, 2026

	if (rshift(needle, 32-m) == rshift(b, 32-m)) { found = 1; exit; } }
	if (int(needle / 2^(32-m)) == int(b / 2^(32-m))) { found = 1; exit; } }

	mv -f "$clean" "$BASE_DIR/${cc}.cidr"
	cmp -s "$clean" "$BASE_DIR/${cc}.cidr" \|\| mv -f "$clean" "$BASE_DIR/${cc}.cidr"

	echo "/$DDNS_I * * * /usr/bin/geoguard-ddns $DNOTE" >> "$CRON_FILE"
	[ -s "$CRON_FILE" ] && [ -n "$(tail -c 1 "$CRON_FILE")" ] && echo "" >> "$CRON_FILE"
	echo "/$DDNS_I * * * /usr/bin/geoguard-ddns $DNOTE" >> "$CRON_FILE"

		iifname "pppoe-wan" ip saddr @geoguard_ban limit rate over 10/minute burst 5 packets log prefix "geoguard-ban-drop: " drop
		iifname "pppoe-wan" ip saddr @geoguard_ban drop

	"/usr/bin/geoguard-status": [ "exec" ],
	"/usr/bin/geoguard-status": [ "exec" ],

	mkdir -p "$BASE_DIR" "$TEMP_DIR"
	mkdir -p "$BASE_DIR" "$TEMP_DIR"
	trap 'rm -rf "$TEMP_DIR"' EXIT

	EN=$(g ban_enabled); [ -z "$EN" ] && EN=1
	EN=$(g ban_enabled); [ -z "$EN" ] && EN=0

	o.description = _('Disabling stops the guard service; active bans stay until expiry.');
	o.description = _('Disabling stops the guard service and removes the guard chain and ban set; active bans are dropped.');

Conversation

chinoyan2002 commented Sep 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openwrt-ai left a comment

Choose a reason for hiding this comment

Commit checks

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chinoyan2002 commented Sep 15, 2026

Uh oh!

openwrt-ai left a comment

Choose a reason for hiding this comment

Commit checks

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chinoyan2002 commented Sep 16, 2026

Uh oh!

chinoyan2002 commented Sep 16, 2026

Uh oh!

chinoyan2002 commented Sep 16, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chinoyan2002 commented Sep 17, 2026

Uh oh!

chinoyan2002 commented Sep 17, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chinoyan2002 commented Sep 13, 2026 •

edited

Loading