diff --git a/internal/web/service/client_inbound_apply.go b/internal/web/service/client_inbound_apply.go index a67a49eb1..fa5da1ecc 100644 --- a/internal/web/service/client_inbound_apply.go +++ b/internal/web/service/client_inbound_apply.go @@ -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 } diff --git a/internal/web/service/client_inbound_apply_test.go b/internal/web/service/client_inbound_apply_test.go index 0c83c4047..cc85cd208 100644 --- a/internal/web/service/client_inbound_apply_test.go +++ b/internal/web/service/client_inbound_apply_test.go @@ -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) }