diff --git a/internal/amneziawgnet/socks_config.go b/internal/amneziawgnet/socks_config.go index a5ba41c99..2f6fa0bfc 100644 --- a/internal/amneziawgnet/socks_config.go +++ b/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 ( diff --git a/internal/amneziawgnet/socks_config_test.go b/internal/amneziawgnet/socks_config_test.go new file mode 100644 index 000000000..6f869f3d4 --- /dev/null +++ b/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) + } + } + }) +} diff --git a/internal/web/service/inbound.go b/internal/web/service/inbound.go index e858f78ec..1950f1008 100644 --- a/internal/web/service/inbound.go +++ b/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 } - conflict, cErr := checkAmneziawgnetSocksReverseConflict(tx, inbound.Id) + if conflict != nil { + return common.NewError(conflict.String()) + } + conflict, cErr = checkAmneziawgnetSocksReverseConflict(tx, inbound.Id) if cErr != nil { return cErr } diff --git a/internal/web/service/inbound_amneziawg_relay_window_test.go b/internal/web/service/inbound_amneziawg_relay_window_test.go new file mode 100644 index 000000000..c61297cb7 --- /dev/null +++ b/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()) + } +} diff --git a/internal/web/service/port_conflict.go b/internal/web/service/port_conflict.go index 757e50fdf..db4340235 100644 --- a/internal/web/service/port_conflict.go +++ b/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 }