From 65c5580e7dabfd3ae5929a125e40b362c44c3500 Mon Sep 17 00:00:00 2001 From: Sanaei Date: Wed, 9 Sep 2026 00:30:02 +0200 Subject: [PATCH] fix(clients): keep per-peer keys when a client spans several tunnel inbounds A client attached to several WireGuard/AmneziaWG inbounds is that many independent peers, each with its own keypair, preshared key and tunnel address. The client edit form can only represent one peer, so Update's per-inbound loop stamped that single field set onto every attached inbound: every peer ended up with identical keys and one inbound's address, and the tunnels on all the other nodes stopped working with no way to recover the overwritten values from the panel. The only guard covered AllowedIPs, and only for AmneziaWG. When more than one tunnel inbound is in scope and the caller sent no per-inbound override, clear the shared peer fields so UpdateInboundClient's existing carry-forward preserves each inbound's own. A scoped update (?inboundIds=) still narrows to one inbound and edits it normally. Closes #6372 --- internal/web/service/client_crud.go | 25 +++++ .../client_update_tunnel_peers_test.go | 98 +++++++++++++++++++ 2 files changed, 123 insertions(+) create mode 100644 internal/web/service/client_update_tunnel_peers_test.go diff --git a/internal/web/service/client_crud.go b/internal/web/service/client_crud.go index f6adcfddf..78327b6f1 100644 --- a/internal/web/service/client_crud.go +++ b/internal/web/service/client_crud.go @@ -648,6 +648,11 @@ func (s *ClientService) Update(inboundSvc *InboundService, id int, updated model } } + tunnelCount, tcErr := tunnelInboundCount(inboundIds) + if tcErr != nil { + return false, tcErr + } + // Built before any inbound is written, as in Create: fillProtocolDefaults // mints the shared credentials on the first inbound, later ones reuse them. applies := make([]inboundApply, 0, len(inboundIds)) @@ -673,6 +678,13 @@ func (s *ClientService) Update(inboundSvc *InboundService, id int, updated model clientForInbound := updated if ips, ok := updated.AllowedIPsByInbound[ibId]; ok { clientForInbound.AllowedIPs = ips + } else if tunnelCount > 1 && (inbound.Protocol == model.WireGuard || inbound.Protocol == model.AmneziaWG) { + // One shared peer field set cannot describe several peers: broadcast + // it and they all end up with the same keys and tunnel address. + clientForInbound.AllowedIPs = nil + clientForInbound.PrivateKey = "" + clientForInbound.PublicKey = "" + clientForInbound.PreSharedKey = "" } else if !addressesFitAmneziaWGInbound(clientForInbound.AllowedIPs, inbound) { // A single shared AllowedIPs field (the common case for a caller // that never sends AllowedIPsByInbound) must never overwrite an @@ -906,6 +918,19 @@ func (s *ClientService) hasTunnelAttachment(inboundSvc *InboundService, inboundI return false } +// tunnelInboundCount reports how many of inboundIds are WireGuard/AmneziaWG, +// i.e. how many independent peers one shared field set would be written to. +func tunnelInboundCount(inboundIds []int) (int64, error) { + if len(inboundIds) == 0 { + return 0, nil + } + var n int64 + err := database.GetDB().Model(&model.Inbound{}). + Where("id IN ? AND protocol IN ?", inboundIds, []model.Protocol{model.WireGuard, model.AmneziaWG}). + Count(&n).Error + return n, err +} + // addressesFitAmneziaWGInbound reports whether every entry in addrs falls // inside ib's own configured subnet(s). AmneziaWG only: its kernel interface // Address is exactly that subnet, so an address inherited from elsewhere (an diff --git a/internal/web/service/client_update_tunnel_peers_test.go b/internal/web/service/client_update_tunnel_peers_test.go new file mode 100644 index 000000000..2c18b204f --- /dev/null +++ b/internal/web/service/client_update_tunnel_peers_test.go @@ -0,0 +1,98 @@ +package service + +import ( + "encoding/base64" + "testing" + + "github.com/mhsanaei/3x-ui/v3/internal/database/model" + wgutil "github.com/mhsanaei/3x-ui/v3/internal/util/wireguard" +) + +func wgTestKeypair(t *testing.T, seed byte) (priv, pub string) { + t.Helper() + raw := make([]byte, 32) + for i := range raw { + raw[i] = seed + } + priv = base64.StdEncoding.EncodeToString(raw) + pub, err := wgutil.PublicKeyFromPrivate(priv) + if err != nil { + t.Fatalf("derive public key: %v", err) + } + return priv, pub +} + +func inboundPeer(t *testing.T, inboundSvc *InboundService, ibId int, email string) model.Client { + t.Helper() + ib, err := inboundSvc.GetInbound(ibId) + if err != nil { + t.Fatalf("GetInbound %d: %v", ibId, err) + } + clients, err := inboundSvc.GetClients(ib) + if err != nil { + t.Fatalf("GetClients %d: %v", ibId, err) + } + for i := range clients { + if clients[i].Email == email { + return clients[i] + } + } + t.Fatalf("email %q not found on inbound %d", email, ibId) + return model.Client{} +} + +// A client on several WireGuard inbounds is several independent peers, each +// with its own keypair and tunnel address. The edit form can only carry one +// field set, so a save that broadcasts it leaves every peer but one with keys +// and an address belonging to a different node, breaking those tunnels. +func TestUpdateDoesNotBroadcastPeerCredentialsAcrossTunnelInbounds(t *testing.T) { + setupBulkDB(t) + inboundSvc := &InboundService{} + svc := &ClientService{} + + const email = "multi@wg" + privA, pubA := wgTestKeypair(t, 0x11) + privB, pubB := wgTestKeypair(t, 0x22) + + peerA := model.Client{ + Email: email, SubID: "sub-multi", Enable: true, + PrivateKey: privA, PublicKey: pubA, AllowedIPs: []string{"10.10.151.5/32"}, + } + peerB := model.Client{ + Email: email, SubID: "sub-multi", Enable: true, + PrivateKey: privB, PublicKey: pubB, AllowedIPs: []string{"10.10.152.5/32"}, + } + + ibA := mkInbound(t, 51821, model.WireGuard, clientsSettings(t, []model.Client{peerA})) + if err := svc.SyncInbound(nil, ibA.Id, []model.Client{peerA}); err != nil { + t.Fatalf("seed inbound A linkage: %v", err) + } + ibB := mkInbound(t, 51822, model.WireGuard, clientsSettings(t, []model.Client{peerB})) + if err := svc.SyncInbound(nil, ibB.Id, []model.Client{peerB}); err != nil { + t.Fatalf("seed inbound B linkage: %v", err) + } + recId := lookupClientRecord(t, email).Id + + // What the client edit form sends: inbound A's peer fields, once, for + // a save that only meant to change an unrelated field. + updated := model.Client{ + Email: email, Enable: true, Comment: "renamed", + PrivateKey: privA, PublicKey: pubA, AllowedIPs: []string{"10.10.151.5/32"}, + } + if _, err := svc.Update(inboundSvc, recId, updated, 0); err != nil { + t.Fatalf("Update: %v", err) + } + + gotB := inboundPeer(t, inboundSvc, ibB.Id, email) + if gotB.PrivateKey != privB || gotB.PublicKey != pubB { + t.Fatalf("inbound B peer keys were overwritten with inbound A's: private=%q public=%q", gotB.PrivateKey, gotB.PublicKey) + } + if len(gotB.AllowedIPs) != 1 || gotB.AllowedIPs[0] != "10.10.152.5/32" { + t.Fatalf("inbound B AllowedIPs = %v, want unchanged [10.10.152.5/32]", gotB.AllowedIPs) + } + + gotA := inboundPeer(t, inboundSvc, ibA.Id, email) + if gotA.PrivateKey != privA || len(gotA.AllowedIPs) != 1 || gotA.AllowedIPs[0] != "10.10.151.5/32" { + t.Fatalf("inbound A peer must keep its own values, got %+v", gotA) + } +}