mirror of
https://github.com/MHSanaei/3x-ui.git
synced 2026-08-04 15:17:27 +00:00
Fix Attach reusing one identity's address across wg/awg inbounds
ClientService.Attach deliberately copies one identity's stored AllowedIPs into every WireGuard/AmneziaWG inbound it's attached to in the same call, so the same person gets the same tunnel address on every protocol they use. Its loop calls addInboundClient once per inbound, and each of those independently computes otherTunnelAllowedIPs -- so by the second inbound in the batch, the first inbound's just-written copy of this identity's own address looked like a cross-inbound collision against itself. Real production symptom this caused: detaching then re-attaching a client to both wg and awg failed with "wireguard: allowedIPs entry X is already used by a client on inbound 'awg' (#N)" -- the exact address the identity is supposed to keep, rejected as if it belonged to someone else. Add a selfEmails exclusion to otherTunnelAllowedIPs and populate it from the client(s) being processed at the one real call site. Safe unconditionally: ClientRecord.Email is globally unique, so a match can only ever be this same identity's own entry on a sibling inbound, never a genuine different client's address. Reproduced the underlying mechanism live (manual entry correctly rejected as a cross-inbound collision; fresh auto-allocation correctly avoided a used address) before writing the fix, to confirm the guard itself works and the bug is specifically in how Attach's per-inbound calls interact with it.
This commit is contained in:
parent
eed723e740
commit
3c974e4a71
2 changed files with 63 additions and 4 deletions
|
|
@ -250,7 +250,18 @@ func (s *ClientService) delInboundClients(inboundSvc *InboundService, inboundId
|
|||
// 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) {
|
||||
//
|
||||
// selfEmails excludes a sibling inbound's entry from being treated as a
|
||||
// collision when it belongs to one of these emails — the identity currently
|
||||
// being added/attached, not some other client. Since ClientRecord.Email is
|
||||
// globally unique, a match here can only ever be this same identity's own
|
||||
// entry on another inbound, never a genuine different-client collision.
|
||||
// This matters for Attach: it deliberately gives one identity the same
|
||||
// AllowedIPs on every inbound it's attached to (ClientService.Attach copies
|
||||
// the ClientRecord's stored address into each inbound it processes), so
|
||||
// attaching the same email to a second inbound right after the first must
|
||||
// not see the first inbound's now-fresh copy of its own address as taken.
|
||||
func (s *ClientService) otherTunnelAllowedIPs(inboundSvc *InboundService, excludeID int, selfEmails map[string]struct{}) (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).
|
||||
|
|
@ -270,6 +281,9 @@ func (s *ClientService) otherTunnelAllowedIPs(inboundSvc *InboundService, exclud
|
|||
}
|
||||
label := fmt.Sprintf("inbound '%s' (#%d)", name, ib.Id)
|
||||
for _, c := range clients {
|
||||
if _, self := selfEmails[strings.ToLower(c.Email)]; self {
|
||||
continue
|
||||
}
|
||||
for _, addr := range c.AllowedIPs {
|
||||
used[addr] = label
|
||||
}
|
||||
|
|
@ -397,7 +411,13 @@ func (s *ClientService) addInboundClient(inboundSvc *InboundService, data *model
|
|||
}
|
||||
|
||||
if oldInbound.Protocol == model.WireGuard || oldInbound.Protocol == model.AmneziaWG {
|
||||
crossUsed, cErr := s.otherTunnelAllowedIPs(inboundSvc, oldInbound.Id)
|
||||
selfEmails := make(map[string]struct{}, len(clients))
|
||||
for _, c := range clients {
|
||||
if c.Email != "" {
|
||||
selfEmails[strings.ToLower(c.Email)] = struct{}{}
|
||||
}
|
||||
}
|
||||
crossUsed, cErr := s.otherTunnelAllowedIPs(inboundSvc, oldInbound.Id, selfEmails)
|
||||
if cErr != nil {
|
||||
return false, cErr
|
||||
}
|
||||
|
|
|
|||
|
|
@ -24,7 +24,7 @@ func TestOtherTunnelAllowedIPs(t *testing.T) {
|
|||
|
||||
svc := &ClientService{}
|
||||
inboundSvc := &InboundService{}
|
||||
used, err := svc.otherTunnelAllowedIPs(inboundSvc, wgInbound.Id)
|
||||
used, err := svc.otherTunnelAllowedIPs(inboundSvc, wgInbound.Id, nil)
|
||||
if err != nil {
|
||||
t.Fatalf("otherTunnelAllowedIPs: %v", err)
|
||||
}
|
||||
|
|
@ -40,6 +40,45 @@ func TestOtherTunnelAllowedIPs(t *testing.T) {
|
|||
}
|
||||
}
|
||||
|
||||
// TestOtherTunnelAllowedIPsExcludesSelfEmail is a regression test for a real
|
||||
// bug in ClientService.Attach: attaching one identity to multiple
|
||||
// WireGuard/AmneziaWG inbounds in the same call copies that identity's own
|
||||
// stored AllowedIPs into every inbound it processes (by design -- the same
|
||||
// person should get the same tunnel address on every protocol they use).
|
||||
// Attach's loop calls addInboundClient once per inbound, and each of those
|
||||
// calls independently computes otherTunnelAllowedIPs -- so by the second
|
||||
// inbound in the loop, the first inbound's now-successful copy of the
|
||||
// identity's own address looked like a cross-inbound collision against
|
||||
// itself, and the attach failed with exactly the error a real user hit:
|
||||
// "wireguard: allowedIPs entry 10.8.1.21/32 is already used by a client on
|
||||
// inbound 'awg' (#10)". selfEmails must exclude this identity's own entries
|
||||
// on sibling inbounds -- safe to do unconditionally because ClientRecord.Email
|
||||
// is globally unique, so a same-email match can only ever be this identity,
|
||||
// never a genuine different client.
|
||||
func TestOtherTunnelAllowedIPsExcludesSelfEmail(t *testing.T) {
|
||||
setupConflictDB(t)
|
||||
seedInboundConflict(t, "awg-1", "0.0.0.0", 443, model.AmneziaWG, ``, `{"server":{"subnetIp":"10.8.1.0","subnetCidr":24},"clients":[{"email":"shared@id","allowedIPs":["10.8.1.21/32"]}]}`)
|
||||
seedInboundConflict(t, "wg-1", "0.0.0.0", 51820, model.WireGuard, ``, `{"clients":[{"email":"other@wg","allowedIPs":["10.8.1.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, map[string]struct{}{"shared@id": {}})
|
||||
if err != nil {
|
||||
t.Fatalf("otherTunnelAllowedIPs: %v", err)
|
||||
}
|
||||
if _, stillThere := used["10.8.1.21/32"]; stillThere {
|
||||
t.Fatalf("shared@id's own address on the awg inbound must be excluded from used, got %v", used)
|
||||
}
|
||||
if _, ok := used["10.8.1.5/32"]; !ok {
|
||||
t.Fatalf("a genuinely different client's address must still be reported as used, got %v", used)
|
||||
}
|
||||
}
|
||||
|
||||
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"]}]}`)
|
||||
|
|
@ -51,7 +90,7 @@ func TestOtherTunnelAllowedIPsEmptyWhenNoSiblings(t *testing.T) {
|
|||
|
||||
svc := &ClientService{}
|
||||
inboundSvc := &InboundService{}
|
||||
used, err := svc.otherTunnelAllowedIPs(inboundSvc, wgInbound.Id)
|
||||
used, err := svc.otherTunnelAllowedIPs(inboundSvc, wgInbound.Id, nil)
|
||||
if err != nil {
|
||||
t.Fatalf("otherTunnelAllowedIPs: %v", err)
|
||||
}
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue