fix(clients): reject AllowedIPs already used on another WireGuard/AmneziaWG inbound

defaultWireguardClients/defaultAmneziaWGClients only ever checked uniqueness
against their own inbound's client list, so two inbounds sharing a subnet
(same protocol or not) could silently hand out or accept the same address --
the exact scenario behind a real duplicate-IP incident where a WireGuard and
an AmneziaWG client both ended up on the same address. otherTunnelAllowedIPs
now collects every address already claimed on every other tunnel inbound and
folds it into both the auto-allocation pool and the manual-entry collision
check, naming the other inbound in the error when it fires.
This commit is contained in:
Kuzz007
2026-08-03 22:06:40 +03:00
parent 966bab0d71
commit e7c6f92e7f
6 changed files with 326 additions and 17 deletions
+13 -1
View File
@@ -42,7 +42,13 @@ func defaultAmneziaWGSubnetBases(settingsJSON string) (v4Base, v6Base string, er
// client never rotates its keys. Mirrors defaultWireguardClients, reusing
// its IP allocation and validation helpers — the only real difference is
// where the allocation base comes from.
func defaultAmneziaWGClients(settingsJSON string, existing, clients []model.Client, interfaceClients []any) error {
//
// crossInboundUsed maps AllowedIPs already claimed by clients on every OTHER
// WireGuard/AmneziaWG inbound on this panel to a human-readable description
// of which inbound holds it (see otherTunnelAllowedIPs) — it only narrows
// which addresses are free to hand out or accept, and lets a manual-entry
// collision name the other inbound instead of just the address.
func defaultAmneziaWGClients(settingsJSON string, existing, clients []model.Client, interfaceClients []any, crossInboundUsed map[string]string) error {
v4Base, v6Base, err := defaultAmneziaWGSubnetBases(settingsJSON)
if err != nil {
return err
@@ -52,6 +58,9 @@ func defaultAmneziaWGClients(settingsJSON string, existing, clients []model.Clie
for i := range existing {
used = append(used, existing[i].AllowedIPs...)
}
for addr := range crossInboundUsed {
used = append(used, addr)
}
for i := range clients {
c := &clients[i]
if c.PrivateKey == "" && c.PublicKey == "" {
@@ -91,6 +100,9 @@ func defaultAmneziaWGClients(settingsJSON string, existing, clients []model.Clie
return common.NewError("amneziawg: allowedIPs has no usable entry")
}
if hit := wireguardAllowedIPsCollision(normalized, used); hit != "" {
if where := crossInboundUsed[hit]; where != "" {
return common.NewError("amneziawg: allowedIPs entry", hit, "is already used by a client on", where)
}
return common.NewError("amneziawg: allowedIPs entry already used by another client:", hit)
}
c.AllowedIPs = normalized
@@ -0,0 +1,128 @@
package service
import (
"strings"
"testing"
"github.com/mhsanaei/3x-ui/v3/internal/database/model"
)
const amneziawgClientTestSettings = `{"server":{"subnetIp":"10.8.1.0","subnetCidr":24}}`
func TestDefaultAmneziaWGSubnetBases(t *testing.T) {
v4, v6, err := defaultAmneziaWGSubnetBases(amneziawgClientTestSettings)
if err != nil {
t.Fatalf("defaultAmneziaWGSubnetBases: %v", err)
}
if v4 != "10.8.1.0/24" {
t.Fatalf("v4Base = %q, want 10.8.1.0/24", v4)
}
if v6 != "" {
t.Fatalf("v6Base = %q, want empty when IPv6 is not enabled", v6)
}
}
func TestDefaultAmneziaWGSubnetBasesIncludesIPv6WhenEnabled(t *testing.T) {
settings := `{"server":{"subnetIp":"10.8.1.0","subnetCidr":24,"ipv6Enabled":true,"ipv6Subnet":"fd00::/64"}}`
v4, v6, err := defaultAmneziaWGSubnetBases(settings)
if err != nil {
t.Fatalf("defaultAmneziaWGSubnetBases: %v", err)
}
if v4 != "10.8.1.0/24" || v6 != "fd00::/64" {
t.Fatalf("got v4=%q v6=%q", v4, v6)
}
}
func TestDefaultAmneziaWGSubnetBasesRejectsMissingServer(t *testing.T) {
if _, _, err := defaultAmneziaWGSubnetBases(`{}`); err == nil {
t.Fatal("expected an error when the settings have no server block")
}
}
func TestDefaultAmneziaWGClientsGeneratesKeypairAndAllocatesFromOwnSubnet(t *testing.T) {
clients := []model.Client{{Email: "a@awg"}}
ifaces := []any{map[string]any{"email": "a@awg"}}
if err := defaultAmneziaWGClients(amneziawgClientTestSettings, nil, clients, ifaces, nil); err != nil {
t.Fatalf("defaultAmneziaWGClients: %v", err)
}
c := clients[0]
if c.PrivateKey == "" || c.PublicKey == "" {
t.Fatalf("keypair not generated: priv=%q pub=%q", c.PrivateKey, c.PublicKey)
}
if len(c.AllowedIPs) != 1 || c.AllowedIPs[0] != "10.8.1.2/32" {
t.Fatalf("allowedIPs not allocated from the inbound's own subnet: %v", c.AllowedIPs)
}
}
func TestDefaultAmneziaWGClientsPreservesProvided(t *testing.T) {
clients := []model.Client{{
Email: "b@awg",
PrivateKey: "keep-priv",
PublicKey: "keep-pub",
AllowedIPs: []string{"10.8.1.50/32"},
}}
ifaces := []any{map[string]any{"email": "b@awg"}}
if err := defaultAmneziaWGClients(amneziawgClientTestSettings, nil, clients, ifaces, nil); err != nil {
t.Fatalf("defaultAmneziaWGClients: %v", err)
}
if clients[0].PrivateKey != "keep-priv" || clients[0].PublicKey != "keep-pub" {
t.Fatalf("provided keys were rotated: %+v", clients[0])
}
if clients[0].AllowedIPs[0] != "10.8.1.50/32" {
t.Fatalf("provided allowedIPs changed: %v", clients[0].AllowedIPs)
}
}
func TestDefaultAmneziaWGClientsRejectsSameInboundDuplicate(t *testing.T) {
existing := []model.Client{{Email: "old@awg", AllowedIPs: []string{"10.8.1.9/32"}}}
dup := []model.Client{{Email: "new@awg", AllowedIPs: []string{"10.8.1.9/32"}}}
err := defaultAmneziaWGClients(amneziawgClientTestSettings, existing, dup, []any{map[string]any{"email": "new@awg"}}, nil)
if err == nil {
t.Fatal("duplicate allowedIPs on the same inbound must be rejected")
}
}
// The exact real-world scenario that motivated crossInboundUsed: a WireGuard
// client and an AmneziaWG peer given the same address by habit. The
// collision must be caught even though the two live on different inbounds
// and neither appears in the other's own "existing" client list, and the
// error should name the other inbound so an admin isn't left guessing.
func TestDefaultAmneziaWGClientsRejectsCrossInboundDuplicate(t *testing.T) {
crossUsed := map[string]string{"10.8.1.21/32": "inbound 'wg' (#12)"}
dup := []model.Client{{Email: "c@awg", AllowedIPs: []string{"10.8.1.21/32"}}}
err := defaultAmneziaWGClients(amneziawgClientTestSettings, nil, dup, []any{map[string]any{"email": "c@awg"}}, crossUsed)
if err == nil {
t.Fatal("allowedIPs already used on another inbound must be rejected")
}
if !strings.Contains(err.Error(), "inbound 'wg' (#12)") {
t.Fatalf("error should name the other inbound holding the address, got: %v", err)
}
}
func TestDefaultAmneziaWGClientsAutoAllocateSkipsCrossInboundUsed(t *testing.T) {
crossUsed := map[string]string{"10.8.1.2/32": "inbound 'other-awg' (#3)"}
clients := []model.Client{{Email: "d@awg"}}
ifaces := []any{map[string]any{"email": "d@awg"}}
if err := defaultAmneziaWGClients(amneziawgClientTestSettings, nil, clients, ifaces, crossUsed); err != nil {
t.Fatalf("defaultAmneziaWGClients: %v", err)
}
if clients[0].AllowedIPs[0] != "10.8.1.3/32" {
t.Fatalf("auto-allocation should skip the cross-inbound-used .2 and pick .3, got %v", clients[0].AllowedIPs)
}
}
// Unlike WireGuard's allocation base (inferred from existing peers with a
// fallback), AmneziaWG's base always comes from the inbound's own configured
// subnet -- so this is really confirming crossInboundUsed can never change
// which subnet is used, only which addresses within it are free.
func TestDefaultAmneziaWGClientsCrossInboundUsedDoesNotChangeBase(t *testing.T) {
crossUsed := map[string]string{"192.168.99.5/32": "inbound 'unrelated' (#99)"}
clients := []model.Client{{Email: "e@awg"}}
ifaces := []any{map[string]any{"email": "e@awg"}}
if err := defaultAmneziaWGClients(amneziawgClientTestSettings, nil, clients, ifaces, crossUsed); err != nil {
t.Fatalf("defaultAmneziaWGClients: %v", err)
}
if got := clients[0].AllowedIPs[0]; got != "10.8.1.2/32" {
t.Fatalf("base subnet must stay the inbound's own 10.8.1.0/24; got %v", got)
}
}
+52 -7
View File
@@ -239,6 +239,45 @@ func (s *ClientService) delInboundClients(inboundSvc *InboundService, inboundId
return needRestart, nil
}
// otherTunnelAllowedIPs collects every AllowedIPs entry already claimed by a
// WireGuard/AmneziaWG client on any OTHER enabled inbound of either protocol
// on this panel, mapped to a short human-readable description of which
// inbound holds it. defaultWireguardClients/defaultAmneziaWGClients only
// ever check uniqueness against their OWN inbound's client list, so two
// inbounds (whether the same protocol or not) that happen to share a subnet
// could otherwise silently hand out or accept the same address — this is
// the cross-inbound half of that guarantee.
// Deliberately not filtered by enable: a disabled sibling inbound's
// addresses stay reserved so re-enabling it later can't collide with
// something handed out in the meantime.
func (s *ClientService) otherTunnelAllowedIPs(inboundSvc *InboundService, excludeID int) (map[string]string, error) {
var inbounds []*model.Inbound
err := database.GetDB().Model(model.Inbound{}).
Where("protocol IN ? AND id != ?", []model.Protocol{model.WireGuard, model.AmneziaWG}, excludeID).
Find(&inbounds).Error
if err != nil {
return nil, err
}
used := make(map[string]string)
for _, ib := range inbounds {
clients, cErr := inboundSvc.GetClients(ib)
if cErr != nil {
continue
}
name := ib.Remark
if name == "" {
name = ib.Tag
}
label := fmt.Sprintf("inbound '%s' (#%d)", name, ib.Id)
for _, c := range clients {
for _, addr := range c.AllowedIPs {
used[addr] = label
}
}
}
return used, nil
}
func (s *ClientService) checkEmailsExistForClients(inboundSvc *InboundService, clients []model.Client, emailSubIDs map[string]string) (string, error) {
if emailSubIDs == nil {
var err error
@@ -357,14 +396,20 @@ func (s *ClientService) addInboundClient(inboundSvc *InboundService, data *model
interfaceClients = keptWire
}
if oldInbound.Protocol == model.WireGuard {
if dErr := defaultWireguardClients(existingClients, clients, interfaceClients); dErr != nil {
return false, dErr
if oldInbound.Protocol == model.WireGuard || oldInbound.Protocol == model.AmneziaWG {
crossUsed, cErr := s.otherTunnelAllowedIPs(inboundSvc, oldInbound.Id)
if cErr != nil {
return false, cErr
}
}
if oldInbound.Protocol == model.AmneziaWG {
if dErr := defaultAmneziaWGClients(oldInbound.Settings, existingClients, clients, interfaceClients); dErr != nil {
return false, dErr
if oldInbound.Protocol == model.WireGuard {
if dErr := defaultWireguardClients(existingClients, clients, interfaceClients, crossUsed); dErr != nil {
return false, dErr
}
}
if oldInbound.Protocol == model.AmneziaWG {
if dErr := defaultAmneziaWGClients(oldInbound.Settings, existingClients, clients, interfaceClients, crossUsed); dErr != nil {
return false, dErr
}
}
}
@@ -0,0 +1,61 @@
package service
import (
"testing"
"github.com/mhsanaei/3x-ui/v3/internal/database"
"github.com/mhsanaei/3x-ui/v3/internal/database/model"
)
// otherTunnelAllowedIPs must see across protocols (a WireGuard inbound's
// client address collides with an AmneziaWG one just as easily as two
// AmneziaWG inbounds would), must exclude the inbound doing the asking, and
// must ignore inbounds that aren't WireGuard/AmneziaWG entirely.
func TestOtherTunnelAllowedIPs(t *testing.T) {
setupConflictDB(t)
seedInboundConflict(t, "wg-1", "0.0.0.0", 51820, model.WireGuard, ``, `{"clients":[{"email":"a@wg","allowedIPs":["10.0.0.5/32"]}]}`)
seedInboundConflict(t, "awg-1", "0.0.0.0", 443, model.AmneziaWG, ``, `{"server":{"subnetIp":"10.8.1.0","subnetCidr":24},"clients":[{"email":"b@awg","allowedIPs":["10.8.1.21/32"]}]}`)
seedInboundConflict(t, "vless-1", "0.0.0.0", 8443, model.VLESS, `{"network":"tcp"}`, `{"clients":[{"email":"c@vless"}]}`)
var wgInbound model.Inbound
if err := database.GetDB().Where("tag = ?", "wg-1").First(&wgInbound).Error; err != nil {
t.Fatalf("read seeded wg row: %v", err)
}
svc := &ClientService{}
inboundSvc := &InboundService{}
used, err := svc.otherTunnelAllowedIPs(inboundSvc, wgInbound.Id)
if err != nil {
t.Fatalf("otherTunnelAllowedIPs: %v", err)
}
if len(used) != 1 {
t.Fatalf("expected exactly one cross-inbound address (self excluded, vless ignored), got %v", used)
}
label, ok := used["10.8.1.21/32"]
if !ok {
t.Fatalf("expected the awg inbound's address to be reported as used, got %v", used)
}
if label == "" {
t.Fatal("expected a non-empty description of which inbound holds the address")
}
}
func TestOtherTunnelAllowedIPsEmptyWhenNoSiblings(t *testing.T) {
setupConflictDB(t)
seedInboundConflict(t, "wg-1", "0.0.0.0", 51820, model.WireGuard, ``, `{"clients":[{"email":"a@wg","allowedIPs":["10.0.0.5/32"]}]}`)
var wgInbound model.Inbound
if err := database.GetDB().Where("tag = ?", "wg-1").First(&wgInbound).Error; err != nil {
t.Fatalf("read seeded wg row: %v", err)
}
svc := &ClientService{}
inboundSvc := &InboundService{}
used, err := svc.otherTunnelAllowedIPs(inboundSvc, wgInbound.Id)
if err != nil {
t.Fatalf("otherTunnelAllowedIPs: %v", err)
}
if len(used) != 0 {
t.Fatalf("expected no cross-inbound addresses with only one tunnel inbound present, got %v", used)
}
}
+15 -1
View File
@@ -145,12 +145,23 @@ func wireguardAllowedIPsCollision(entries, used []string) string {
// inbound's subnet. It mutates both the typed clients and the parallel raw client
// maps that get persisted into the inbound settings. Existing values are never
// overwritten, so editing a client never rotates its keys.
func defaultWireguardClients(existing, clients []model.Client, interfaceClients []any) error {
//
// crossInboundUsed maps AllowedIPs already claimed by clients on every OTHER
// WireGuard/AmneziaWG inbound on this panel to a human-readable description
// of which inbound holds it (see otherTunnelAllowedIPs). It is folded into
// used only AFTER wireguardAllocationBase runs, so an unrelated inbound's
// subnet can never skew this inbound's own base-subnet inference — it only
// ever narrows which addresses are free to hand out or accept, and lets a
// manual-entry collision name the other inbound instead of just the address.
func defaultWireguardClients(existing, clients []model.Client, interfaceClients []any, crossInboundUsed map[string]string) error {
used := make([]string, 0)
for i := range existing {
used = append(used, existing[i].AllowedIPs...)
}
base := wireguardAllocationBase(used, defaultWireguardBase)
for addr := range crossInboundUsed {
used = append(used, addr)
}
for i := range clients {
c := &clients[i]
if c.PrivateKey == "" && c.PublicKey == "" {
@@ -182,6 +193,9 @@ func defaultWireguardClients(existing, clients []model.Client, interfaceClients
return common.NewError("wireguard: allowedIPs has no usable entry")
}
if hit := wireguardAllowedIPsCollision(normalized, used); hit != "" {
if where := crossInboundUsed[hit]; where != "" {
return common.NewError("wireguard: allowedIPs entry", hit, "is already used by a client on", where)
}
return common.NewError("wireguard: allowedIPs entry already used by another client:", hit)
}
c.AllowedIPs = normalized
+57 -8
View File
@@ -2,6 +2,7 @@ package service
import (
"fmt"
"strings"
"testing"
"github.com/mhsanaei/3x-ui/v3/internal/database/model"
@@ -46,7 +47,7 @@ func TestAllocateWireguardAddress(t *testing.T) {
func TestDefaultWireguardClientsGeneratesKeypair(t *testing.T) {
clients := []model.Client{{Email: "a@wg"}}
ifaces := []any{map[string]any{"email": "a@wg"}}
if err := defaultWireguardClients(nil, clients, ifaces); err != nil {
if err := defaultWireguardClients(nil, clients, ifaces, nil); err != nil {
t.Fatalf("defaultWireguardClients: %v", err)
}
c := clients[0]
@@ -73,7 +74,7 @@ func TestDefaultWireguardClientsDerivesPublicKey(t *testing.T) {
}
clients := []model.Client{{Email: "b@wg", PrivateKey: priv}}
ifaces := []any{map[string]any{"email": "b@wg"}}
if err := defaultWireguardClients(nil, clients, ifaces); err != nil {
if err := defaultWireguardClients(nil, clients, ifaces, nil); err != nil {
t.Fatalf("defaultWireguardClients: %v", err)
}
if clients[0].PublicKey != wantPub {
@@ -89,7 +90,7 @@ func TestDefaultWireguardClientsPreservesProvided(t *testing.T) {
AllowedIPs: []string{"10.0.0.50/32"},
}}
ifaces := []any{map[string]any{"email": "c@wg"}}
if err := defaultWireguardClients(nil, clients, ifaces); err != nil {
if err := defaultWireguardClients(nil, clients, ifaces, nil); err != nil {
t.Fatalf("defaultWireguardClients: %v", err)
}
if clients[0].PrivateKey != "keep-priv" || clients[0].PublicKey != "keep-pub" {
@@ -124,7 +125,7 @@ func TestDefaultWireguardClientsHonorsExistingSubnet(t *testing.T) {
existing := []model.Client{{Email: "old@wg", AllowedIPs: []string{"172.16.0.2/32"}}}
clients := []model.Client{{Email: "new@wg"}}
ifaces := []any{map[string]any{"email": "new@wg"}}
if err := defaultWireguardClients(existing, clients, ifaces); err != nil {
if err := defaultWireguardClients(existing, clients, ifaces, nil); err != nil {
t.Fatalf("defaultWireguardClients: %v", err)
}
if got := clients[0].AllowedIPs[0]; got != "172.16.0.3/32" {
@@ -183,7 +184,7 @@ func TestAllocateWireguardAddressWithoutWideningFailsClosed(t *testing.T) {
func TestDefaultWireguardClientsAllocatesDistinctIPs(t *testing.T) {
clients := []model.Client{{Email: "x@wg"}, {Email: "y@wg"}}
ifaces := []any{map[string]any{"email": "x@wg"}, map[string]any{"email": "y@wg"}}
if err := defaultWireguardClients(nil, clients, ifaces); err != nil {
if err := defaultWireguardClients(nil, clients, ifaces, nil); err != nil {
t.Fatalf("defaultWireguardClients: %v", err)
}
if clients[0].AllowedIPs[0] == clients[1].AllowedIPs[0] {
@@ -236,7 +237,7 @@ func TestDefaultWireguardClientsHonorsAndValidatesSuppliedAllowedIPs(t *testing.
clients := []model.Client{{Email: "c@wg", AllowedIPs: []string{"10.0.0.9"}}}
ifaces := []any{map[string]any{"email": "c@wg"}}
if err := defaultWireguardClients(existing, clients, ifaces); err != nil {
if err := defaultWireguardClients(existing, clients, ifaces, nil); err != nil {
t.Fatalf("defaultWireguardClients: %v", err)
}
if len(clients[0].AllowedIPs) != 1 || clients[0].AllowedIPs[0] != "10.0.0.9/32" {
@@ -244,13 +245,61 @@ func TestDefaultWireguardClientsHonorsAndValidatesSuppliedAllowedIPs(t *testing.
}
dup := []model.Client{{Email: "d@wg", AllowedIPs: []string{"10.0.0.2/32"}}}
err := defaultWireguardClients(existing, dup, []any{map[string]any{"email": "d@wg"}})
err := defaultWireguardClients(existing, dup, []any{map[string]any{"email": "d@wg"}}, nil)
if err == nil {
t.Fatal("duplicate allowedIPs across clients must be rejected")
}
bad := []model.Client{{Email: "e@wg", AllowedIPs: []string{"not-an-ip"}}}
if err := defaultWireguardClients(existing, bad, []any{map[string]any{"email": "e@wg"}}); err == nil {
if err := defaultWireguardClients(existing, bad, []any{map[string]any{"email": "e@wg"}}, nil); err == nil {
t.Fatal("invalid allowedIPs entry must be rejected")
}
}
// A duplicate manually-typed address is rejected even when the OTHER holder
// lives on a completely different inbound (e.g. a WireGuard client and an
// AmneziaWG peer given the same address by habit) -- this is the exact
// real-world scenario that motivated crossInboundUsed: two inbounds sharing
// a subnet must not be able to silently hand out or accept the same address.
func TestDefaultWireguardClientsRejectsCrossInboundDuplicate(t *testing.T) {
crossUsed := map[string]string{"10.8.1.21/32": "inbound 'awg' (#10)"}
dup := []model.Client{{Email: "d@wg", AllowedIPs: []string{"10.8.1.21/32"}}}
err := defaultWireguardClients(nil, dup, []any{map[string]any{"email": "d@wg"}}, crossUsed)
if err == nil {
t.Fatal("allowedIPs already used on another inbound must be rejected")
}
if !strings.Contains(err.Error(), "inbound 'awg' (#10)") {
t.Fatalf("error should name the other inbound holding the address, got: %v", err)
}
}
// Auto-allocation (no AllowedIPs supplied) must also skip addresses already
// claimed on another inbound, not just ones used on this one.
func TestDefaultWireguardClientsAutoAllocateSkipsCrossInboundUsed(t *testing.T) {
crossUsed := map[string]string{"10.0.0.2/32": "inbound 'other-wg' (#7)"}
clients := []model.Client{{Email: "f@wg"}}
ifaces := []any{map[string]any{"email": "f@wg"}}
if err := defaultWireguardClients(nil, clients, ifaces, crossUsed); err != nil {
t.Fatalf("defaultWireguardClients: %v", err)
}
if clients[0].AllowedIPs[0] != "10.0.0.3/32" {
t.Fatalf("auto-allocation should skip the cross-inbound-used .2 and pick .3, got %v", clients[0].AllowedIPs)
}
}
// crossInboundUsed must never influence which subnet THIS inbound's own new
// clients get allocated from -- only existing (this inbound's own clients)
// may do that. Otherwise a brand-new WireGuard inbound on a panel that
// already has an unrelated AmneziaWG inbound would infer the wrong base
// subnet purely from the other inbound's addresses.
func TestDefaultWireguardClientsCrossInboundUsedDoesNotSkewSubnetInference(t *testing.T) {
crossUsed := map[string]string{"10.8.1.21/32": "inbound 'awg' (#10)"}
clients := []model.Client{{Email: "g@wg"}}
ifaces := []any{map[string]any{"email": "g@wg"}}
if err := defaultWireguardClients(nil, clients, ifaces, crossUsed); err != nil {
t.Fatalf("defaultWireguardClients: %v", err)
}
if got := clients[0].AllowedIPs[0]; got != "10.0.0.2/32" {
t.Fatalf("base subnet must stay the default 10.0.0.0/24, not be skewed by a cross-inbound address; got %v", got)
}
}