diff --git a/docs/content/docs/en/reference/api/clients.mdx b/docs/content/docs/en/reference/api/clients.mdx index 58678d57c..4031fbb49 100644 --- a/docs/content/docs/en/reference/api/clients.mdx +++ b/docs/content/docs/en/reference/api/clients.mdx @@ -592,10 +592,14 @@ _openapi: the search to the containing /16 before giving up with `inbound : wireguard: no free address available in `, and an `allowedIPs` supplied by the caller is validated instead of allocated: `inbound - : wireguard: allowedIPs entry already used by another client: -
` when a different client of that same inbound already holds - it. The check is per inbound, so the same address on two different - inbounds is accepted. The same validation runs on POST + : wireguard: allowedIPs entry overlaps
used by + another client` when its range overlaps an address or prefix a + different client of that same inbound holds, or `... used by a client + on ` when the holder sits on another WireGuard or AmneziaWG + inbound. Ranges are compared, not strings, so `10.0.0.9/24` collides + with `10.0.0.5/32`; a `0.0.0.0/0` or `::/0` default route claims no + address. Allocation likewise skips every address inside a prefix + another client holds. The same validation runs on POST /panel/api/clients/{email}/attach, where a client that already carries an address brings it along. @@ -640,12 +644,12 @@ _openapi: heading: delete-a-client-by-email-removes-it-from-every-attached-inbound-and-drops-its-traffic-record-unless-keeptraffic1-is-passed - content: 'A WireGuard client brings its stored `allowedIPs` into the new inbound instead of being given a fresh address, so the call fails with - `inbound : wireguard: allowedIPs entry already used by another - client:
` when a different client of the target inbound - already holds it. Free the address on that inbound first — see POST - /panel/api/clients/add for the full rule. Inbounds are applied - independently, so the remaining ones are still attached and a - `success:false` response can be partial.' + `inbound : wireguard: allowedIPs entry overlaps
+ used by another client` when its range overlaps an address or prefix a + different client of the target inbound holds. Free the address on that + inbound first — see POST /panel/api/clients/add for the full rule. + Inbounds are applied independently, so the remaining ones are still + attached and a `success:false` response can be partial.' heading: attach-an-existing-client-to-one-or-more-additional-inbounds-body-is-json - content: 'The inbounds are applied concurrently and independently: one that fails no longer stops the others. Every inbound error names the diff --git a/docs/public/openapi.json b/docs/public/openapi.json index 163336845..4919f5664 100644 --- a/docs/public/openapi.json +++ b/docs/public/openapi.json @@ -8524,7 +8524,7 @@ ], "summary": "Create a new client and attach it to one or more inbounds in a single call. Body is JSON. Per-protocol secrets are generated server-side when omitted, so callers can send only the universal fields.", "operationId": "post_panel_api_clients_add", - "description": "Fields the server fills in when they are omitted — a valid value sent by the caller is never overwritten. Re-adding an email that already exists, with its stored `subId`, reuses the stored `id`, `password`, `auth` and `secret` instead of minting new ones, so the identity stays in sync across its inbounds.\n\n- **VLESS / VMess** — `id`, a fresh UUID\n- **Trojan** — `password`\n- **Shadowsocks** — `password`. On a `2022-blake3-*` inbound a supplied password that does not base64-decode to the key length of the cipher (16 or 32 bytes) is replaced by a generated key and the call still succeeds, so read the client back if you did not let the server pick. Legacy ciphers keep any non-empty password\n- **Hysteria** — `auth`\n- **mtproto** — `secret`, a FakeTLS secret derived from the fronting domain of the inbound, or from `www.cloudflare.com` when it has none\n- **WireGuard** — `privateKey` and `publicKey` when both are blank, or `publicKey` alone when only a `privateKey` was sent, plus `allowedIPs`: one free `/32` taken from the /24 the existing peers of that inbound already sit in, or from `10.0.0.0/24` when it has none\n\nAccepted on the same body but never generated: `preSharedKey` and `keepAlive` (WireGuard), `adTag` (mtproto).\n\nWireGuard is the only one of these that can fail. Allocation widens the search to the containing /16 before giving up with `inbound : wireguard: no free address available in `, and an `allowedIPs` supplied by the caller is validated instead of allocated: `inbound : wireguard: allowedIPs entry already used by another client:
` when a different client of that same inbound already holds it. The check is per inbound, so the same address on two different inbounds is accepted. The same validation runs on POST /panel/api/clients/{email}/attach, where a client that already carries an address brings it along.\n\nAn `inboundIds` entry that names no existing inbound rejects the whole call before anything is written. Past that, the inbounds are applied concurrently and independently: one that fails no longer stops the others, so a `success:false` response can still have created the client on the rest. Every error names the inbound it came from (`inbound 7: `), and several failures are reported together, one per line. `limitHwid` is applied only when every inbound succeeded, so re-run the call after fixing the failure.", + "description": "Fields the server fills in when they are omitted — a valid value sent by the caller is never overwritten. Re-adding an email that already exists, with its stored `subId`, reuses the stored `id`, `password`, `auth` and `secret` instead of minting new ones, so the identity stays in sync across its inbounds.\n\n- **VLESS / VMess** — `id`, a fresh UUID\n- **Trojan** — `password`\n- **Shadowsocks** — `password`. On a `2022-blake3-*` inbound a supplied password that does not base64-decode to the key length of the cipher (16 or 32 bytes) is replaced by a generated key and the call still succeeds, so read the client back if you did not let the server pick. Legacy ciphers keep any non-empty password\n- **Hysteria** — `auth`\n- **mtproto** — `secret`, a FakeTLS secret derived from the fronting domain of the inbound, or from `www.cloudflare.com` when it has none\n- **WireGuard** — `privateKey` and `publicKey` when both are blank, or `publicKey` alone when only a `privateKey` was sent, plus `allowedIPs`: one free `/32` taken from the /24 the existing peers of that inbound already sit in, or from `10.0.0.0/24` when it has none\n\nAccepted on the same body but never generated: `preSharedKey` and `keepAlive` (WireGuard), `adTag` (mtproto).\n\nWireGuard is the only one of these that can fail. Allocation widens the search to the containing /16 before giving up with `inbound : wireguard: no free address available in `, and an `allowedIPs` supplied by the caller is validated instead of allocated: `inbound : wireguard: allowedIPs entry overlaps
used by another client` when its range overlaps an address or prefix a different client of that same inbound holds, or `... used by a client on ` when the holder sits on another WireGuard or AmneziaWG inbound. Ranges are compared, not strings, so `10.0.0.9/24` collides with `10.0.0.5/32`; a `0.0.0.0/0` or `::/0` default route claims no address. Allocation likewise skips every address inside a prefix another client holds. The same validation runs on POST /panel/api/clients/{email}/attach, where a client that already carries an address brings it along.\n\nAn `inboundIds` entry that names no existing inbound rejects the whole call before anything is written. Past that, the inbounds are applied concurrently and independently: one that fails no longer stops the others, so a `success:false` response can still have created the client on the rest. Every error names the inbound it came from (`inbound 7: `), and several failures are reported together, one per line. `limitHwid` is applied only when every inbound succeeded, so re-run the call after fixing the failure.", "requestBody": { "required": true, "content": { @@ -8811,7 +8811,7 @@ ], "summary": "Attach an existing client to one or more additional inbounds. Body is JSON.", "operationId": "post_panel_api_clients_email_attach", - "description": "A WireGuard client brings its stored `allowedIPs` into the new inbound instead of being given a fresh address, so the call fails with `inbound : wireguard: allowedIPs entry already used by another client:
` when a different client of the target inbound already holds it. Free the address on that inbound first — see POST /panel/api/clients/add for the full rule. Inbounds are applied independently, so the remaining ones are still attached and a `success:false` response can be partial.", + "description": "A WireGuard client brings its stored `allowedIPs` into the new inbound instead of being given a fresh address, so the call fails with `inbound : wireguard: allowedIPs entry overlaps
used by another client` when its range overlaps an address or prefix a different client of the target inbound holds. Free the address on that inbound first — see POST /panel/api/clients/add for the full rule. Inbounds are applied independently, so the remaining ones are still attached and a `success:false` response can be partial.", "parameters": [ { "name": "email", diff --git a/frontend/public/openapi.json b/frontend/public/openapi.json index 163336845..4919f5664 100644 --- a/frontend/public/openapi.json +++ b/frontend/public/openapi.json @@ -8524,7 +8524,7 @@ ], "summary": "Create a new client and attach it to one or more inbounds in a single call. Body is JSON. Per-protocol secrets are generated server-side when omitted, so callers can send only the universal fields.", "operationId": "post_panel_api_clients_add", - "description": "Fields the server fills in when they are omitted — a valid value sent by the caller is never overwritten. Re-adding an email that already exists, with its stored `subId`, reuses the stored `id`, `password`, `auth` and `secret` instead of minting new ones, so the identity stays in sync across its inbounds.\n\n- **VLESS / VMess** — `id`, a fresh UUID\n- **Trojan** — `password`\n- **Shadowsocks** — `password`. On a `2022-blake3-*` inbound a supplied password that does not base64-decode to the key length of the cipher (16 or 32 bytes) is replaced by a generated key and the call still succeeds, so read the client back if you did not let the server pick. Legacy ciphers keep any non-empty password\n- **Hysteria** — `auth`\n- **mtproto** — `secret`, a FakeTLS secret derived from the fronting domain of the inbound, or from `www.cloudflare.com` when it has none\n- **WireGuard** — `privateKey` and `publicKey` when both are blank, or `publicKey` alone when only a `privateKey` was sent, plus `allowedIPs`: one free `/32` taken from the /24 the existing peers of that inbound already sit in, or from `10.0.0.0/24` when it has none\n\nAccepted on the same body but never generated: `preSharedKey` and `keepAlive` (WireGuard), `adTag` (mtproto).\n\nWireGuard is the only one of these that can fail. Allocation widens the search to the containing /16 before giving up with `inbound : wireguard: no free address available in `, and an `allowedIPs` supplied by the caller is validated instead of allocated: `inbound : wireguard: allowedIPs entry already used by another client:
` when a different client of that same inbound already holds it. The check is per inbound, so the same address on two different inbounds is accepted. The same validation runs on POST /panel/api/clients/{email}/attach, where a client that already carries an address brings it along.\n\nAn `inboundIds` entry that names no existing inbound rejects the whole call before anything is written. Past that, the inbounds are applied concurrently and independently: one that fails no longer stops the others, so a `success:false` response can still have created the client on the rest. Every error names the inbound it came from (`inbound 7: `), and several failures are reported together, one per line. `limitHwid` is applied only when every inbound succeeded, so re-run the call after fixing the failure.", + "description": "Fields the server fills in when they are omitted — a valid value sent by the caller is never overwritten. Re-adding an email that already exists, with its stored `subId`, reuses the stored `id`, `password`, `auth` and `secret` instead of minting new ones, so the identity stays in sync across its inbounds.\n\n- **VLESS / VMess** — `id`, a fresh UUID\n- **Trojan** — `password`\n- **Shadowsocks** — `password`. On a `2022-blake3-*` inbound a supplied password that does not base64-decode to the key length of the cipher (16 or 32 bytes) is replaced by a generated key and the call still succeeds, so read the client back if you did not let the server pick. Legacy ciphers keep any non-empty password\n- **Hysteria** — `auth`\n- **mtproto** — `secret`, a FakeTLS secret derived from the fronting domain of the inbound, or from `www.cloudflare.com` when it has none\n- **WireGuard** — `privateKey` and `publicKey` when both are blank, or `publicKey` alone when only a `privateKey` was sent, plus `allowedIPs`: one free `/32` taken from the /24 the existing peers of that inbound already sit in, or from `10.0.0.0/24` when it has none\n\nAccepted on the same body but never generated: `preSharedKey` and `keepAlive` (WireGuard), `adTag` (mtproto).\n\nWireGuard is the only one of these that can fail. Allocation widens the search to the containing /16 before giving up with `inbound : wireguard: no free address available in `, and an `allowedIPs` supplied by the caller is validated instead of allocated: `inbound : wireguard: allowedIPs entry overlaps
used by another client` when its range overlaps an address or prefix a different client of that same inbound holds, or `... used by a client on ` when the holder sits on another WireGuard or AmneziaWG inbound. Ranges are compared, not strings, so `10.0.0.9/24` collides with `10.0.0.5/32`; a `0.0.0.0/0` or `::/0` default route claims no address. Allocation likewise skips every address inside a prefix another client holds. The same validation runs on POST /panel/api/clients/{email}/attach, where a client that already carries an address brings it along.\n\nAn `inboundIds` entry that names no existing inbound rejects the whole call before anything is written. Past that, the inbounds are applied concurrently and independently: one that fails no longer stops the others, so a `success:false` response can still have created the client on the rest. Every error names the inbound it came from (`inbound 7: `), and several failures are reported together, one per line. `limitHwid` is applied only when every inbound succeeded, so re-run the call after fixing the failure.", "requestBody": { "required": true, "content": { @@ -8811,7 +8811,7 @@ ], "summary": "Attach an existing client to one or more additional inbounds. Body is JSON.", "operationId": "post_panel_api_clients_email_attach", - "description": "A WireGuard client brings its stored `allowedIPs` into the new inbound instead of being given a fresh address, so the call fails with `inbound : wireguard: allowedIPs entry already used by another client:
` when a different client of the target inbound already holds it. Free the address on that inbound first — see POST /panel/api/clients/add for the full rule. Inbounds are applied independently, so the remaining ones are still attached and a `success:false` response can be partial.", + "description": "A WireGuard client brings its stored `allowedIPs` into the new inbound instead of being given a fresh address, so the call fails with `inbound : wireguard: allowedIPs entry overlaps
used by another client` when its range overlaps an address or prefix a different client of the target inbound holds. Free the address on that inbound first — see POST /panel/api/clients/add for the full rule. Inbounds are applied independently, so the remaining ones are still attached and a `success:false` response can be partial.", "parameters": [ { "name": "email", diff --git a/frontend/src/pages/api-docs/endpoints.ts b/frontend/src/pages/api-docs/endpoints.ts index b6bfad445..3fca5dc01 100644 --- a/frontend/src/pages/api-docs/endpoints.ts +++ b/frontend/src/pages/api-docs/endpoints.ts @@ -1151,7 +1151,7 @@ export const sections: readonly Section[] = [ summary: 'Create a new client and attach it to one or more inbounds in a single call. Body is JSON. Per-protocol secrets are generated server-side when omitted, so callers can send only the universal fields.', description: - 'Fields the server fills in when they are omitted — a valid value sent by the caller is never overwritten. Re-adding an email that already exists, with its stored `subId`, reuses the stored `id`, `password`, `auth` and `secret` instead of minting new ones, so the identity stays in sync across its inbounds.\n\n- **VLESS / VMess** — `id`, a fresh UUID\n- **Trojan** — `password`\n- **Shadowsocks** — `password`. On a `2022-blake3-*` inbound a supplied password that does not base64-decode to the key length of the cipher (16 or 32 bytes) is replaced by a generated key and the call still succeeds, so read the client back if you did not let the server pick. Legacy ciphers keep any non-empty password\n- **Hysteria** — `auth`\n- **mtproto** — `secret`, a FakeTLS secret derived from the fronting domain of the inbound, or from `www.cloudflare.com` when it has none\n- **WireGuard** — `privateKey` and `publicKey` when both are blank, or `publicKey` alone when only a `privateKey` was sent, plus `allowedIPs`: one free `/32` taken from the /24 the existing peers of that inbound already sit in, or from `10.0.0.0/24` when it has none\n\nAccepted on the same body but never generated: `preSharedKey` and `keepAlive` (WireGuard), `adTag` (mtproto).\n\nWireGuard is the only one of these that can fail. Allocation widens the search to the containing /16 before giving up with `inbound : wireguard: no free address available in `, and an `allowedIPs` supplied by the caller is validated instead of allocated: `inbound : wireguard: allowedIPs entry already used by another client:
` when a different client of that same inbound already holds it. The check is per inbound, so the same address on two different inbounds is accepted. The same validation runs on POST /panel/api/clients/{email}/attach, where a client that already carries an address brings it along.\n\nAn `inboundIds` entry that names no existing inbound rejects the whole call before anything is written. Past that, the inbounds are applied concurrently and independently: one that fails no longer stops the others, so a `success:false` response can still have created the client on the rest. Every error names the inbound it came from (`inbound 7: `), and several failures are reported together, one per line. `limitHwid` is applied only when every inbound succeeded, so re-run the call after fixing the failure.', + 'Fields the server fills in when they are omitted — a valid value sent by the caller is never overwritten. Re-adding an email that already exists, with its stored `subId`, reuses the stored `id`, `password`, `auth` and `secret` instead of minting new ones, so the identity stays in sync across its inbounds.\n\n- **VLESS / VMess** — `id`, a fresh UUID\n- **Trojan** — `password`\n- **Shadowsocks** — `password`. On a `2022-blake3-*` inbound a supplied password that does not base64-decode to the key length of the cipher (16 or 32 bytes) is replaced by a generated key and the call still succeeds, so read the client back if you did not let the server pick. Legacy ciphers keep any non-empty password\n- **Hysteria** — `auth`\n- **mtproto** — `secret`, a FakeTLS secret derived from the fronting domain of the inbound, or from `www.cloudflare.com` when it has none\n- **WireGuard** — `privateKey` and `publicKey` when both are blank, or `publicKey` alone when only a `privateKey` was sent, plus `allowedIPs`: one free `/32` taken from the /24 the existing peers of that inbound already sit in, or from `10.0.0.0/24` when it has none\n\nAccepted on the same body but never generated: `preSharedKey` and `keepAlive` (WireGuard), `adTag` (mtproto).\n\nWireGuard is the only one of these that can fail. Allocation widens the search to the containing /16 before giving up with `inbound : wireguard: no free address available in `, and an `allowedIPs` supplied by the caller is validated instead of allocated: `inbound : wireguard: allowedIPs entry overlaps
used by another client` when its range overlaps an address or prefix a different client of that same inbound holds, or `... used by a client on ` when the holder sits on another WireGuard or AmneziaWG inbound. Ranges are compared, not strings, so `10.0.0.9/24` collides with `10.0.0.5/32`; a `0.0.0.0/0` or `::/0` default route claims no address. Allocation likewise skips every address inside a prefix another client holds. The same validation runs on POST /panel/api/clients/{email}/attach, where a client that already carries an address brings it along.\n\nAn `inboundIds` entry that names no existing inbound rejects the whole call before anything is written. Past that, the inbounds are applied concurrently and independently: one that fails no longer stops the others, so a `success:false` response can still have created the client on the rest. Every error names the inbound it came from (`inbound 7: `), and several failures are reported together, one per line. `limitHwid` is applied only when every inbound succeeded, so re-run the call after fixing the failure.', params: [ { name: 'client', @@ -1256,7 +1256,7 @@ export const sections: readonly Section[] = [ path: '/panel/api/clients/:email/attach', summary: 'Attach an existing client to one or more additional inbounds. Body is JSON.', description: - 'A WireGuard client brings its stored `allowedIPs` into the new inbound instead of being given a fresh address, so the call fails with `inbound : wireguard: allowedIPs entry already used by another client:
` when a different client of the target inbound already holds it. Free the address on that inbound first — see POST /panel/api/clients/add for the full rule. Inbounds are applied independently, so the remaining ones are still attached and a `success:false` response can be partial.', + 'A WireGuard client brings its stored `allowedIPs` into the new inbound instead of being given a fresh address, so the call fails with `inbound : wireguard: allowedIPs entry overlaps
used by another client` when its range overlaps an address or prefix a different client of the target inbound holds. Free the address on that inbound first — see POST /panel/api/clients/add for the full rule. Inbounds are applied independently, so the remaining ones are still attached and a `success:false` response can be partial.', params: [ { name: 'email', in: 'path', type: 'string', desc: 'Client email (unique identifier).' }, { diff --git a/internal/web/service/client_amneziawg.go b/internal/web/service/client_amneziawg.go index cd88151b8..cf5bd9bb9 100644 --- a/internal/web/service/client_amneziawg.go +++ b/internal/web/service/client_amneziawg.go @@ -93,11 +93,11 @@ func defaultAmneziaWGClients(settingsJSON string, existing, clients []model.Clie if len(normalized) == 0 { 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) + if entry, taken := wireguardAllowedIPsOverlap(normalized, used); taken != "" { + if where := crossInboundUsed[taken]; where != "" { + return common.NewError("amneziawg: allowedIPs entry", entry, "overlaps", taken, "used by a client on", where) } - return common.NewError("amneziawg: allowedIPs entry already used by another client:", hit) + return common.NewError("amneziawg: allowedIPs entry", entry, "overlaps", taken, "used by another client") } c.AllowedIPs = normalized } diff --git a/internal/web/service/client_inbound_apply.go b/internal/web/service/client_inbound_apply.go index 37ea1e2da..9598d4653 100644 --- a/internal/web/service/client_inbound_apply.go +++ b/internal/web/service/client_inbound_apply.go @@ -539,8 +539,8 @@ func (s *ClientService) AddInboundClient(inboundSvc *InboundService, data *model crossAddrs = append(crossAddrs, addr) } for i := range clients { - if hit := wireguardAllowedIPsCollision(clients[i].AllowedIPs, crossAddrs); hit != "" { - return common.NewError("allowedIPs entry", hit, "is already used by a client on", crossUsed[hit]) + if entry, taken := wireguardAllowedIPsOverlap(clients[i].AllowedIPs, crossAddrs); taken != "" { + return common.NewError("allowedIPs entry", entry, "overlaps", taken, "used by a client on", crossUsed[taken]) } } } @@ -760,8 +760,8 @@ func (s *ClientService) UpdateInboundClient(inboundSvc *InboundService, data *mo } peers = append(peers, oldClients[i].AllowedIPs...) } - if hit := wireguardAllowedIPsCollision(normalized, peers); hit != "" { - return false, common.NewError("wireguard: allowedIPs entry already used by another client:", hit) + if entry, taken := wireguardAllowedIPsOverlap(normalized, peers); taken != "" { + return false, common.NewError("wireguard: allowedIPs entry", entry, "overlaps", taken, "used by another client") } clients[0].AllowedIPs = normalized } diff --git a/internal/web/service/client_wireguard.go b/internal/web/service/client_wireguard.go index 2a14be36b..d3a64722f 100644 --- a/internal/web/service/client_wireguard.go +++ b/internal/web/service/client_wireguard.go @@ -105,10 +105,28 @@ func allocateWireguardAddress(used []string, base string, allowWidening bool) (s hostBits = "128" } taken := make(map[netip.Addr]struct{}, len(used)) + var wide []netip.Prefix for _, u := range used { - if a := wireguardHostAddr(u); a.IsValid() { - taken[a] = struct{}{} + p, ok := wireguardClaimedPrefix(u) + if !ok { + continue } + if p.IsSingleIP() { + taken[p.Addr()] = struct{}{} + } else { + wide = append(wide, p) + } + } + isTaken := func(a netip.Addr) bool { + if _, ok := taken[a]; ok { + return true + } + for _, p := range wide { + if p.Contains(a) { + return true + } + } + return false } scopes := []netip.Prefix{prefix} if allowWidening && prefix.Addr().Is4() && prefix.Bits() > wireguardPoolFloorBits { @@ -119,7 +137,7 @@ func allocateWireguardAddress(used []string, base string, allowWidening bool) (s for _, scope := range scopes { addr := scope.Masked().Addr().Next().Next() for scope.Contains(addr) { - if _, ok := taken[addr]; !ok { + if !isTaken(addr) { return addr.String() + "/" + hostBits, nil } addr = addr.Next() @@ -156,17 +174,44 @@ func normalizeWireguardAllowedIPs(values []string) ([]string, error) { return out, nil } -func wireguardAllowedIPsCollision(entries, used []string) string { - taken := make(map[string]struct{}, len(used)) - for _, u := range used { - taken[strings.TrimSpace(u)] = struct{}{} +// wireguardClaimedPrefix is the masked range an allowedIPs entry claims, as xray +// reads it. A /0 default route claims no tunnel address, as legacy peers carry it. +func wireguardClaimedPrefix(s string) (netip.Prefix, bool) { + s = strings.TrimSpace(s) + p, err := netip.ParsePrefix(s) + if err != nil { + a, aErr := netip.ParseAddr(s) + if aErr != nil { + return netip.Prefix{}, false + } + p = netip.PrefixFrom(a, a.BitLen()) + } + if p.Bits() == 0 { + return netip.Prefix{}, false + } + return p.Masked(), true +} + +// wireguardAllowedIPsOverlap returns the first entry whose range overlaps a used +// one, and that used entry; xray routes and attributes by containment, not equality. +func wireguardAllowedIPsOverlap(entries, used []string) (entry, taken string) { + usedPrefixes := make([]netip.Prefix, len(used)) + usedOK := make([]bool, len(used)) + for i, u := range used { + usedPrefixes[i], usedOK[i] = wireguardClaimedPrefix(u) } for _, e := range entries { - if _, ok := taken[e]; ok { - return e + ep, ok := wireguardClaimedPrefix(e) + if !ok { + continue + } + for i, up := range usedPrefixes { + if usedOK[i] && ep.Overlaps(up) { + return e, used[i] + } } } - return "" + return "", "" } // defaultWireguardClients fills in blank WireGuard credentials for newly added @@ -231,11 +276,11 @@ func defaultWireguardClients(settingsJSON string, existing, clients []model.Clie if len(normalized) == 0 { 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) + if entry, taken := wireguardAllowedIPsOverlap(normalized, used); taken != "" { + if where := crossInboundUsed[taken]; where != "" { + return common.NewError("wireguard: allowedIPs entry", entry, "overlaps", taken, "used by a client on", where) } - return common.NewError("wireguard: allowedIPs entry already used by another client:", hit) + return common.NewError("wireguard: allowedIPs entry", entry, "overlaps", taken, "used by another client") } c.AllowedIPs = normalized } diff --git a/internal/web/service/client_wireguard_test.go b/internal/web/service/client_wireguard_test.go index 04ebff340..7fc4860e4 100644 --- a/internal/web/service/client_wireguard_test.go +++ b/internal/web/service/client_wireguard_test.go @@ -362,3 +362,31 @@ func TestDefaultWireguardClientsFallsBackWhenNoExplicitSubnet(t *testing.T) { t.Fatalf("with no explicit subnet, inference from existing clients must still apply; got %v", got) } } + +// xray credits a packet to the first peer whose allowedIPs contain its source, +// so a prefix covering another client's address steals that client's traffic. +func TestDefaultWireguardClientsRejectsOverlappingAllowedIPs(t *testing.T) { + existing := []model.Client{{Email: "a@wg", AllowedIPs: []string{"10.10.2.9/24"}}} + clients := []model.Client{{Email: "b@wg", AllowedIPs: []string{"10.10.2.52/24"}}} + err := defaultWireguardClients("", existing, clients, []any{map[string]any{"email": "b@wg"}}, nil) + if err == nil || !strings.Contains(err.Error(), "10.10.2.52/24") || !strings.Contains(err.Error(), "10.10.2.9/24") { + t.Fatalf("overlapping allowedIPs must be rejected naming both entries, got: %v", err) + } + + crossUsed := map[string]string{"10.8.1.0/24": "inbound 'awg' (#10)"} + inside := []model.Client{{Email: "c@wg", AllowedIPs: []string{"10.8.1.21/32"}}} + err = defaultWireguardClients("", nil, inside, []any{map[string]any{"email": "c@wg"}}, crossUsed) + if err == nil || !strings.Contains(err.Error(), "inbound 'awg' (#10)") { + t.Fatalf("an address inside another inbound's prefix must be rejected naming that inbound, got: %v", err) + } +} + +func TestAllocateWireguardAddressSkipsAddressesInsideUsedPrefixes(t *testing.T) { + got, err := allocateWireguardAddress([]string{"10.0.0.2/31"}, "10.0.0.0/24", true) + if err != nil { + t.Fatalf("allocateWireguardAddress: %v", err) + } + if got != "10.0.0.4/32" { + t.Fatalf("got %s, want 10.0.0.4/32: .2 and .3 are both inside the used 10.0.0.2/31", got) + } +}