fix(wireguard): reject allowedIPs that overlap another client's range

xray's WireGuard inbound credits a packet to the first peer whose allowedIPs
contain its source address, and routes replies by the same table. The panel
only rejected an allowedIPs entry that was string-equal to another client's,
so a pre-assigned address typed in interface notation (10.10.2.9/24, as other
WireGuard tools export it) was accepted and claimed the whole /24: other
clients' traffic and online IPs were credited to that one email, and replies
went to a peer with no endpoint ("no known endpoint for peer").

The collision check now compares masked ranges on every path that uses it:
add, edit, the cross-inbound recheck inside the write transaction, and
AmneziaWG. Auto-allocation skips any address inside a prefix another client
holds. A /0 default route still claims nothing, as before, because legacy
migrated peers carry one. Clients already saved with overlapping ranges keep
working as they do today until edited. The API docs describing the error are
updated, including the stale claim that cross-inbound duplicates are accepted.

Closes #6623
This commit is contained in:
MHSanaei
2026-09-27 18:18:50 +02:00
parent 4df570b3b0
commit c8a182b6fb
8 changed files with 115 additions and 38 deletions
+14 -10
View File
@@ -592,10 +592,14 @@ _openapi:
the search to the containing /16 before giving up with `inbound <id>: the search to the containing /16 before giving up with `inbound <id>:
wireguard: no free address available in <scope>`, and an `allowedIPs` wireguard: no free address available in <scope>`, and an `allowedIPs`
supplied by the caller is validated instead of allocated: `inbound supplied by the caller is validated instead of allocated: `inbound
<id>: wireguard: allowedIPs entry already used by another client: <id>: wireguard: allowedIPs entry <entry> overlaps <address> used by
<address>` when a different client of that same inbound already holds another client` when its range overlaps an address or prefix a
it. The check is per inbound, so the same address on two different different client of that same inbound holds, or `... used by a client
inbounds is accepted. The same validation runs on POST on <inbound>` 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 /panel/api/clients/{email}/attach, where a client that already carries
an address brings it along. 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 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 - content: 'A WireGuard client brings its stored `allowedIPs` into the new inbound
instead of being given a fresh address, so the call fails with instead of being given a fresh address, so the call fails with
`inbound <id>: wireguard: allowedIPs entry already used by another `inbound <id>: wireguard: allowedIPs entry <entry> overlaps <address>
client: <address>` when a different client of the target inbound used by another client` when its range overlaps an address or prefix a
already holds it. Free the address on that inbound first — see POST different client of the target inbound holds. Free the address on that
/panel/api/clients/add for the full rule. Inbounds are applied inbound first — see POST /panel/api/clients/add for the full rule.
independently, so the remaining ones are still attached and a Inbounds are applied independently, so the remaining ones are still
`success:false` response can be partial.' attached and a `success:false` response can be partial.'
heading: attach-an-existing-client-to-one-or-more-additional-inbounds-body-is-json heading: attach-an-existing-client-to-one-or-more-additional-inbounds-body-is-json
- content: 'The inbounds are applied concurrently and independently: one that - content: 'The inbounds are applied concurrently and independently: one that
fails no longer stops the others. Every inbound error names the fails no longer stops the others. Every inbound error names the
+2 -2
View File
@@ -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.", "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", "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 <id>: wireguard: no free address available in <scope>`, and an `allowedIPs` supplied by the caller is validated instead of allocated: `inbound <id>: wireguard: allowedIPs entry already used by another client: <address>` 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: <message>`), 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 <id>: wireguard: no free address available in <scope>`, and an `allowedIPs` supplied by the caller is validated instead of allocated: `inbound <id>: wireguard: allowedIPs entry <entry> overlaps <address> 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 <inbound>` 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: <message>`), 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": { "requestBody": {
"required": true, "required": true,
"content": { "content": {
@@ -8811,7 +8811,7 @@
], ],
"summary": "Attach an existing client to one or more additional inbounds. Body is JSON.", "summary": "Attach an existing client to one or more additional inbounds. Body is JSON.",
"operationId": "post_panel_api_clients_email_attach", "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 <id>: wireguard: allowedIPs entry already used by another client: <address>` 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 <id>: wireguard: allowedIPs entry <entry> overlaps <address> 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": [ "parameters": [
{ {
"name": "email", "name": "email",
+2 -2
View File
@@ -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.", "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", "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 <id>: wireguard: no free address available in <scope>`, and an `allowedIPs` supplied by the caller is validated instead of allocated: `inbound <id>: wireguard: allowedIPs entry already used by another client: <address>` 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: <message>`), 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 <id>: wireguard: no free address available in <scope>`, and an `allowedIPs` supplied by the caller is validated instead of allocated: `inbound <id>: wireguard: allowedIPs entry <entry> overlaps <address> 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 <inbound>` 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: <message>`), 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": { "requestBody": {
"required": true, "required": true,
"content": { "content": {
@@ -8811,7 +8811,7 @@
], ],
"summary": "Attach an existing client to one or more additional inbounds. Body is JSON.", "summary": "Attach an existing client to one or more additional inbounds. Body is JSON.",
"operationId": "post_panel_api_clients_email_attach", "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 <id>: wireguard: allowedIPs entry already used by another client: <address>` 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 <id>: wireguard: allowedIPs entry <entry> overlaps <address> 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": [ "parameters": [
{ {
"name": "email", "name": "email",
+2 -2
View File
@@ -1151,7 +1151,7 @@ export const sections: readonly Section[] = [
summary: 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.', '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: 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 <id>: wireguard: no free address available in <scope>`, and an `allowedIPs` supplied by the caller is validated instead of allocated: `inbound <id>: wireguard: allowedIPs entry already used by another client: <address>` 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: <message>`), 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 <id>: wireguard: no free address available in <scope>`, and an `allowedIPs` supplied by the caller is validated instead of allocated: `inbound <id>: wireguard: allowedIPs entry <entry> overlaps <address> 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 <inbound>` 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: <message>`), 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: [ params: [
{ {
name: 'client', name: 'client',
@@ -1256,7 +1256,7 @@ export const sections: readonly Section[] = [
path: '/panel/api/clients/:email/attach', path: '/panel/api/clients/:email/attach',
summary: 'Attach an existing client to one or more additional inbounds. Body is JSON.', summary: 'Attach an existing client to one or more additional inbounds. Body is JSON.',
description: 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 <id>: wireguard: allowedIPs entry already used by another client: <address>` 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 <id>: wireguard: allowedIPs entry <entry> overlaps <address> 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: [ params: [
{ name: 'email', in: 'path', type: 'string', desc: 'Client email (unique identifier).' }, { name: 'email', in: 'path', type: 'string', desc: 'Client email (unique identifier).' },
{ {
+4 -4
View File
@@ -93,11 +93,11 @@ func defaultAmneziaWGClients(settingsJSON string, existing, clients []model.Clie
if len(normalized) == 0 { if len(normalized) == 0 {
return common.NewError("amneziawg: allowedIPs has no usable entry") return common.NewError("amneziawg: allowedIPs has no usable entry")
} }
if hit := wireguardAllowedIPsCollision(normalized, used); hit != "" { if entry, taken := wireguardAllowedIPsOverlap(normalized, used); taken != "" {
if where := crossInboundUsed[hit]; where != "" { if where := crossInboundUsed[taken]; where != "" {
return common.NewError("amneziawg: allowedIPs entry", hit, "is already used by a client on", 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 c.AllowedIPs = normalized
} }
+4 -4
View File
@@ -539,8 +539,8 @@ func (s *ClientService) AddInboundClient(inboundSvc *InboundService, data *model
crossAddrs = append(crossAddrs, addr) crossAddrs = append(crossAddrs, addr)
} }
for i := range clients { for i := range clients {
if hit := wireguardAllowedIPsCollision(clients[i].AllowedIPs, crossAddrs); hit != "" { if entry, taken := wireguardAllowedIPsOverlap(clients[i].AllowedIPs, crossAddrs); taken != "" {
return common.NewError("allowedIPs entry", hit, "is already used by a client on", crossUsed[hit]) 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...) peers = append(peers, oldClients[i].AllowedIPs...)
} }
if hit := wireguardAllowedIPsCollision(normalized, peers); hit != "" { if entry, taken := wireguardAllowedIPsOverlap(normalized, peers); taken != "" {
return false, common.NewError("wireguard: allowedIPs entry already used by another client:", hit) return false, common.NewError("wireguard: allowedIPs entry", entry, "overlaps", taken, "used by another client")
} }
clients[0].AllowedIPs = normalized clients[0].AllowedIPs = normalized
} }
+59 -14
View File
@@ -105,10 +105,28 @@ func allocateWireguardAddress(used []string, base string, allowWidening bool) (s
hostBits = "128" hostBits = "128"
} }
taken := make(map[netip.Addr]struct{}, len(used)) taken := make(map[netip.Addr]struct{}, len(used))
var wide []netip.Prefix
for _, u := range used { for _, u := range used {
if a := wireguardHostAddr(u); a.IsValid() { p, ok := wireguardClaimedPrefix(u)
taken[a] = struct{}{} 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} scopes := []netip.Prefix{prefix}
if allowWidening && prefix.Addr().Is4() && prefix.Bits() > wireguardPoolFloorBits { 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 { for _, scope := range scopes {
addr := scope.Masked().Addr().Next().Next() addr := scope.Masked().Addr().Next().Next()
for scope.Contains(addr) { for scope.Contains(addr) {
if _, ok := taken[addr]; !ok { if !isTaken(addr) {
return addr.String() + "/" + hostBits, nil return addr.String() + "/" + hostBits, nil
} }
addr = addr.Next() addr = addr.Next()
@@ -156,17 +174,44 @@ func normalizeWireguardAllowedIPs(values []string) ([]string, error) {
return out, nil return out, nil
} }
func wireguardAllowedIPsCollision(entries, used []string) string { // wireguardClaimedPrefix is the masked range an allowedIPs entry claims, as xray
taken := make(map[string]struct{}, len(used)) // reads it. A /0 default route claims no tunnel address, as legacy peers carry it.
for _, u := range used { func wireguardClaimedPrefix(s string) (netip.Prefix, bool) {
taken[strings.TrimSpace(u)] = struct{}{} 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 { for _, e := range entries {
if _, ok := taken[e]; ok { ep, ok := wireguardClaimedPrefix(e)
return 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 // 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 { if len(normalized) == 0 {
return common.NewError("wireguard: allowedIPs has no usable entry") return common.NewError("wireguard: allowedIPs has no usable entry")
} }
if hit := wireguardAllowedIPsCollision(normalized, used); hit != "" { if entry, taken := wireguardAllowedIPsOverlap(normalized, used); taken != "" {
if where := crossInboundUsed[hit]; where != "" { if where := crossInboundUsed[taken]; where != "" {
return common.NewError("wireguard: allowedIPs entry", hit, "is already used by a client on", 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 c.AllowedIPs = normalized
} }
@@ -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) 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)
}
}