浏览代码

fix(amneziawg): wrap the relay port window instead of refusing ids past it (#6539)

* fix(amneziawg): wrap the relay port window instead of refusing ids past it

An AmneziaWG inbound's loopback relay port is SOCKSBasePort + row id, and
AddInbound refused any id that pushed it past 65535. The inbounds table is
AUTOINCREMENT, so an id is never reused and the counter is only reset when the
table empties: the 435-port window was a lifetime budget, and a database that
had ever created more inbounds could never create another AmneziaWG one --
the reporter's counter sits at 70350, so the protocol never worked there at all
(#6537).

Ids now wrap into the same 435 ports, which leaves every id up to 435 with the
exact port it had, so no existing row, relay or generated config moves.

Wrapping makes the id -> port map non-injective, and nothing compared two
derived relay ports before -- two relays on one port would leave Xray with a
duplicate listen and refuse to start, taking the whole panel's proxy down.
checkAmneziawgnetSocksRelayCollision now refuses a create or an edit whose
derived port another local AmneziaWG row already owns, disabled rows included:
a row owns its slot for good, and enabling it later re-runs no port check.

* test(amneziawg): give each relay-window fixture its own client email

Every fixture built the same client email, and an email is unique across the
whole panel, so AddInbound refused the second create with "Duplicate email"
before either new guard ran -- CI exercised neither the wrap nor the collision
refusal. Each fixture now derives its email from its own tag, which is what the
tag already exists for.

* fix(amneziawg): say relay port in the relay conflict message

A refusal that named the port of the automatic loopback relay read as if the
named inbound listened on an unrelated port -- its own port is the WireGuard
one. portConflictDetail now carries Relay, and both messages that report a
derived relay port say "relay port N"; messages that report a configured port
render byte-for-byte as before.

* test(amneziawg): pin that a node-assigned inbound owns no relay slot

A row adopted from a node carries a NodeID and the protocol it arrived with
(inbound_node.go:737), yet injectAmneziawgnetSocks skips it, so it binds no
loopback relay. The gate this PR added to checkPortConflictTx never looked at
NodeID, so editing such a row can be refused for a slot it does not own.
Expected red on this head; the fix follows.

* fix(amneziawg): skip the relay guards for node-assigned inbounds

Round-2 review finding: the gate this PR added to checkPortConflictTx keyed on
inbound.Protocol alone, so it also ran for a row adopted from a node. Such a row
carries a NodeID and gets no loopback relay -- injectAmneziawgnetSocks skips it
and the desired-instance query is node_id IS NULL -- so it owns no slot and can
collide with nothing, yet editing it was refused with "relay port N ... already
used by inbound '<local>'", naming a port the edited row never binds.

Wrapping made this visible: before it, an adopted id above 435 derived a port
above 65535 that no row could hold, so the pre-existing reverse check under the
same gate could not fire.

Both call sites now require NodeID == nil, matching the local-only predicate the
forward check already used. TestCheckPortConflict_NodeAssignedAmneziawgOwnsNoRelaySlot
fails without this, with the exact false refusal, and passes with it.
BlindMaster24 11 小时之前
父节点
当前提交
a036ddd66f

+ 5 - 2
internal/amneziawgnet/socks_config.go

@@ -11,10 +11,13 @@ import (
 // own Xray SOCKS5 relay inbound (see relay.go/SocksInboundSettings).
 const SOCKSBasePort = 65100
 
+// relayPortSlots is how many ids fit above SOCKSBasePort before wrapping.
+const relayPortSlots = 65535 - SOCKSBasePort
+
 // SOCKSPortForInbound derives one inbound's loopback SOCKS5 relay port from
-// its id, so config generation and the dialing relay never need to negotiate.
+// its id, wrapping ids past relayPortSlots so no id ever lacks a port.
 func SOCKSPortForInbound(inboundID int) int {
-	return SOCKSBasePort + inboundID
+	return SOCKSBasePort + 1 + (inboundID-1)%relayPortSlots
 }
 
 var (

+ 26 - 0
internal/amneziawgnet/socks_config_test.go

@@ -0,0 +1,26 @@
+package amneziawgnet
+
+import "testing"
+
+// An inbound id past the slot count used to be refused outright, which capped a
+// database at 435 AmneziaWG inbounds for its whole life (#6537).
+func TestSOCKSPortForInboundKeepsEverySlotInsideTheWindow(t *testing.T) {
+	t.Run("no id derives a port outside the window", func(t *testing.T) {
+		for _, id := range []int{1, 2, 434, 435, 436, 437, 870, 871, 6537, 70350, 1_000_000} {
+			port := SOCKSPortForInbound(id)
+			if port < SOCKSBasePort+1 || port > 65535 {
+				t.Errorf("id %d derives relay port %d, outside %d..65535", id, port, SOCKSBasePort+1)
+			}
+		}
+	})
+
+	// Every id the old formula reached must keep its exact port, or upgrading
+	// moves a running relay. Ids 1..435 also leave SOCKSBasePort itself unused.
+	t.Run("ids up to the slot count keep the port they always had", func(t *testing.T) {
+		for id := 1; id <= 435; id++ {
+			if got, want := SOCKSPortForInbound(id), SOCKSBasePort+id; got != want {
+				t.Errorf("id %d moved from relay port %d to %d", id, want, got)
+			}
+		}
+	})
+}

+ 10 - 8
internal/web/service/inbound.go

@@ -17,7 +17,6 @@ import (
 	"github.com/google/uuid"
 
 	"github.com/mhsanaei/3x-ui/v3/internal/amneziawg"
-	"github.com/mhsanaei/3x-ui/v3/internal/amneziawgnet"
 	"github.com/mhsanaei/3x-ui/v3/internal/database"
 	"github.com/mhsanaei/3x-ui/v3/internal/database/model"
 	"github.com/mhsanaei/3x-ui/v3/internal/logger"
@@ -1238,14 +1237,17 @@ func (s *InboundService) AddInbound(inbound *model.Inbound) (*model.Inbound, boo
 		if err := tx.Omit("ClientStats").Save(inbound).Error; err != nil {
 			return err
 		}
-		// The relay port is derived from the id, only known after Save; checkPortConflictTx
-		// ran the reverse-direction check above with ignoreId==0, so it couldn't yet.
-		if inbound.Protocol == model.AmneziaWG {
-			if amneziawgnet.SOCKSPortForInbound(inbound.Id) > 65535 {
-				return common.NewErrorf("amneziawg: inbound id %d exceeds the relay port window (ids above %d are not supported)",
-					inbound.Id, 65535-amneziawgnet.SOCKSBasePort)
+		// The relay port is derived from the id, only known after Save, and only a
+		// local row owns one: checkPortConflictTx ran neither check with ignoreId==0.
+		if inbound.NodeID == nil && inbound.Protocol == model.AmneziaWG {
+			conflict, cErr := checkAmneziawgnetSocksRelayCollision(tx, inbound.Id)
+			if cErr != nil {
+				return cErr
+			}
+			if conflict != nil {
+				return common.NewError(conflict.String())
 			}
-			conflict, cErr := checkAmneziawgnetSocksReverseConflict(tx, inbound.Id)
+			conflict, cErr = checkAmneziawgnetSocksReverseConflict(tx, inbound.Id)
 			if cErr != nil {
 				return cErr
 			}

+ 173 - 0
internal/web/service/inbound_amneziawg_relay_window_test.go

@@ -0,0 +1,173 @@
+package service
+
+import (
+	"fmt"
+	"strings"
+	"testing"
+
+	"github.com/mhsanaei/3x-ui/v3/internal/amneziawgnet"
+	"github.com/mhsanaei/3x-ui/v3/internal/database"
+	"github.com/mhsanaei/3x-ui/v3/internal/database/model"
+	wgutil "github.com/mhsanaei/3x-ui/v3/internal/util/wireguard"
+)
+
+// awgRelayWindowSettings builds an AmneziaWG settings blob AddInbound accepts:
+// real X25519 keys, one enabled peer, and an email unique to tag.
+func awgRelayWindowSettings(t *testing.T, tag string) string {
+	t.Helper()
+	_, clientPub, err := wgutil.GenerateWireguardKeypair()
+	if err != nil {
+		t.Fatalf("generate client keypair: %v", err)
+	}
+	return `{"server":{"privateKey":"` + awgTestPrivateKey + `","publicKey":"` + awgTestPublicKey +
+		`","subnetIp":"10.8.1.0","subnetCidr":24},"clients":[{"email":"` + tag + `@relay-window","enable":true,"publicKey":"` +
+		clientPub + `","allowedIPs":["10.8.1.2/32"]}]}`
+}
+
+// pushInboundIDSequence makes the next inbounds insert land on nextID, standing
+// in for a long-lived database whose AUTOINCREMENT counter has climbed there.
+func pushInboundIDSequence(t *testing.T, nextID int) {
+	t.Helper()
+	// The counter is a sqlite_sequence row, so this has no PostgreSQL equivalent.
+	if database.IsPostgres() {
+		t.Skip("the inbounds AUTOINCREMENT counter is a SQLite row")
+	}
+	res := database.GetDB().Exec("UPDATE sqlite_sequence SET seq = ? WHERE name = ?", nextID-1, "inbounds")
+	if res.Error != nil {
+		t.Fatalf("push the inbounds sequence to %d: %v", nextID, res.Error)
+	}
+	if res.RowsAffected != 1 {
+		t.Fatalf("inbounds has no AUTOINCREMENT counter row to push (%d rows updated)", res.RowsAffected)
+	}
+}
+
+func addAmneziaWGInbound(t *testing.T, tag string, port int, enable bool) *model.Inbound {
+	t.Helper()
+	created, _, err := (&InboundService{}).AddInbound(&model.Inbound{
+		Tag:      tag,
+		Enable:   enable,
+		Listen:   "0.0.0.0",
+		Port:     port,
+		Protocol: model.AmneziaWG,
+		Settings: awgRelayWindowSettings(t, tag),
+	})
+	if err != nil {
+		t.Fatalf("AddInbound(%s): %v", tag, err)
+	}
+	return created
+}
+
+// An id past the slot count used to be refused outright, which capped a
+// database at 435 AmneziaWG inbounds for its entire life (#6537).
+func TestAddInbound_AmneziawgPastTheRelayPortWindowStillCreates(t *testing.T) {
+	setupConflictDB(t)
+
+	// Self-check: the low-id path must work, or the assertion below could pass
+	// because the fixture never created an AmneziaWG inbound at all.
+	addAmneziaWGInbound(t, "awg-low-id", 51820, true)
+
+	pushInboundIDSequence(t, 70001)
+	created := addAmneziaWGInbound(t, "awg-past-window", 51821, true)
+	if created.Id < 436 {
+		t.Fatalf("fixture: inbound id %d is still inside the old window", created.Id)
+	}
+	if port := amneziawgnet.SOCKSPortForInbound(created.Id); port < amneziawgnet.SOCKSBasePort+1 || port > 65535 {
+		t.Fatalf("inbound %d derived relay port %d, outside %d..65535", created.Id, port, amneziawgnet.SOCKSBasePort+1)
+	}
+}
+
+// Wrapping ids makes the id -> relay-port map non-injective, so a create can
+// land on a port an existing inbound's relay already owns.
+func TestAddInbound_AmneziawgRefusesAClaimedRelayPort(t *testing.T) {
+	for _, blockerEnabled := range []bool{true, false} {
+		t.Run(fmt.Sprintf("blocker enabled=%t", blockerEnabled), func(t *testing.T) {
+			setupConflictDB(t)
+			blocker := addAmneziaWGInbound(t, "awg-blocker", 51820, blockerEnabled)
+
+			// One slot-window further on is the id that derives the blocker's port.
+			collidingID := blocker.Id + 435
+			pushInboundIDSequence(t, collidingID)
+
+			_, _, err := (&InboundService{}).AddInbound(&model.Inbound{
+				Tag:      "awg-collides",
+				Enable:   true,
+				Listen:   "0.0.0.0",
+				Port:     51821,
+				Protocol: model.AmneziaWG,
+				Settings: awgRelayWindowSettings(t, "awg-collides"),
+			})
+			if err == nil {
+				t.Fatalf("inbound %d derives relay port %d, already owned by %q; the create must be refused",
+					collidingID, amneziawgnet.SOCKSPortForInbound(blocker.Id), blocker.Tag)
+			}
+			if !strings.Contains(err.Error(), blocker.Tag) {
+				t.Fatalf("the conflict must name the inbound owning the port, got %v", err)
+			}
+			// The blocker's own port is its WireGuard one, so without this the
+			// message reads as if that inbound listened on an unrelated port.
+			if !strings.Contains(err.Error(), "relay port") {
+				t.Fatalf("the refusal must say the port is an automatic relay one, got %v", err)
+			}
+		})
+	}
+}
+
+// Wrapping makes id -> relay port non-injective, so an edit landing on a slot a
+// local inbound already owns has to be refused: the create guard never sees it.
+func TestCheckPortConflict_LocalAmneziawgRelayCollisionBlocksTheEdit(t *testing.T) {
+	setupConflictDB(t)
+	blocker := addAmneziaWGInbound(t, "awg-blocker", 51820, true)
+
+	local := &model.Inbound{
+		Tag:      "awg-edited",
+		Enable:   true,
+		Listen:   "0.0.0.0",
+		Port:     51821,
+		Protocol: model.AmneziaWG,
+		Settings: awgRelayWindowSettings(t, "awg-edited"),
+	}
+	collidingID := blocker.Id + 435
+
+	got, err := (&InboundService{}).checkPortConflict(local, collidingID)
+	if err != nil {
+		t.Fatalf("checkPortConflict: %v", err)
+	}
+	if got == nil {
+		t.Fatalf("id %d derives relay port %d, already owned by %q; the save must be refused",
+			collidingID, amneziawgnet.SOCKSPortForInbound(blocker.Id), blocker.Tag)
+	}
+	if !strings.Contains(got.String(), blocker.Tag) {
+		t.Fatalf("the conflict must name the inbound owning the port, got %q", got.String())
+	}
+}
+
+// A row adopted from a node keeps the protocol it arrived with and its central
+// id (inbound_node.go:737), but gets no relay -- so its slot can never be taken.
+func TestCheckPortConflict_NodeAssignedAmneziawgOwnsNoRelaySlot(t *testing.T) {
+	setupConflictDB(t)
+	blocker := addAmneziaWGInbound(t, "awg-blocker", 51820, true)
+
+	nodeID := 7
+	adopted := &model.Inbound{
+		Tag:      "awg-adopted",
+		Enable:   true,
+		Listen:   "0.0.0.0",
+		Port:     51821,
+		Protocol: model.AmneziaWG,
+		Settings: awgRelayWindowSettings(t, "awg-adopted"),
+		NodeID:   &nodeID,
+	}
+	collidingID := blocker.Id + 435
+	if amneziawgnet.SOCKSPortForInbound(collidingID) != amneziawgnet.SOCKSPortForInbound(blocker.Id) {
+		t.Fatalf("fixture: id %d does not derive the blocker's relay port", collidingID)
+	}
+
+	got, err := (&InboundService{}).checkPortConflict(adopted, collidingID)
+	if err != nil {
+		t.Fatalf("checkPortConflict: %v", err)
+	}
+	if got != nil {
+		t.Fatalf("id %d is node-assigned and binds no relay, so it cannot collide; got %q",
+			collidingID, got.String())
+	}
+}

+ 52 - 11
internal/web/service/port_conflict.go

@@ -103,11 +103,13 @@ func isAnyListen(s string) bool {
 }
 
 type portConflictDetail struct {
-	InboundID  int
-	Remark     string
-	Tag        string
-	Listen     string
-	Port       int
+	InboundID int
+	Remark    string
+	Tag       string
+	Listen    string
+	Port      int
+	// Relay marks Port as an automatic loopback relay port, not a configured one.
+	Relay      bool
 	Transports transportBits
 }
 
@@ -129,8 +131,12 @@ func (d *portConflictDetail) String() string {
 	if isAnyListen(listen) {
 		listen = "*"
 	}
-	return fmt.Sprintf("port %d (%s) already used by inbound %s on %s",
-		d.Port, transportTagSuffix(d.Transports), name, listen)
+	port := fmt.Sprintf("port %d", d.Port)
+	if d.Relay {
+		port = fmt.Sprintf("relay port %d", d.Port)
+	}
+	return fmt.Sprintf("%s (%s) already used by inbound %s on %s",
+		port, transportTagSuffix(d.Transports), name, listen)
 }
 
 // defaultXrayAPIPort is the loopback port of the internal Xray API inbound
@@ -215,10 +221,17 @@ func checkPortConflictTx(db *gorm.DB, inbound *model.Inbound, ignoreId int) (*po
 		}
 	}
 
-	// The reverse direction, only meaningful once the id is known (create's
-	// ignoreId==0 means AddInbound must run this itself after Save assigns one).
-	if inbound.Protocol == model.AmneziaWG && ignoreId > 0 {
-		conflict, err := checkAmneziawgnetSocksReverseConflict(db, ignoreId)
+	// The reverse direction, only meaningful once the id is known -- AddInbound
+	// runs it after Save. Only a local row owns a relay slot (#6537 review).
+	if inbound.NodeID == nil && inbound.Protocol == model.AmneziaWG && ignoreId > 0 {
+		conflict, err := checkAmneziawgnetSocksRelayCollision(db, ignoreId)
+		if err != nil {
+			return nil, err
+		}
+		if conflict != nil {
+			return conflict, nil
+		}
+		conflict, err = checkAmneziawgnetSocksReverseConflict(db, ignoreId)
 		if err != nil {
 			return nil, err
 		}
@@ -300,6 +313,33 @@ func checkAmneziawgnetSocksConflict(db *gorm.DB, inbound *model.Inbound, ignoreI
 	return nil, nil
 }
 
+// checkAmneziawgnetSocksRelayCollision reports whether id's derived relay port
+// is already claimed by another local AmneziaWG inbound, disabled rows included.
+func checkAmneziawgnetSocksRelayCollision(db *gorm.DB, id int) (*portConflictDetail, error) {
+	relayPort := amneziawgnet.SOCKSPortForInbound(id)
+	var candidates []*model.Inbound
+	if err := db.Model(model.Inbound{}).
+		Where("protocol = ? AND node_id IS NULL AND id != ?", model.AmneziaWG, id).
+		Find(&candidates).Error; err != nil {
+		return nil, err
+	}
+	for _, c := range candidates {
+		if amneziawgnet.SOCKSPortForInbound(c.Id) != relayPort {
+			continue
+		}
+		return &portConflictDetail{
+			InboundID:  c.Id,
+			Remark:     c.Remark,
+			Tag:        c.Tag,
+			Listen:     "127.0.0.1",
+			Port:       relayPort,
+			Relay:      true,
+			Transports: transportTCP,
+		}, nil
+	}
+	return nil, nil
+}
+
 // checkAmneziawgnetSocksReverseConflict mirrors checkAmneziawgnetSocksConflict:
 // does id's own derived relay port collide with some other inbound's port.
 func checkAmneziawgnetSocksReverseConflict(db *gorm.DB, id int) (*portConflictDetail, error) {
@@ -320,6 +360,7 @@ func checkAmneziawgnetSocksReverseConflict(db *gorm.DB, id int) (*portConflictDe
 			Tag:        c.Tag,
 			Listen:     c.Listen,
 			Port:       relayPort,
+			Relay:      true,
 			Transports: transportTCP,
 		}, nil
 	}