3 Commits

Author	SHA1	Message	Date
BlindMaster24	43e64993fc	fix(amneziawg): refuse a row's own relay port and keep a disabled row's slot reserved (#6544) ... * fix(amneziawg): refuse a WireGuard port that is the row's own relay port All three relay checks filter themselves out of the candidates with id != ignoreId, so nothing ever compared an AmneziaWG row's own WireGuard listen port with the relay port its own id derives. Saving a row on that exact port left the embedded device (UDP on the inbound's listen address, amneziawgnet/device.go:137) and its injected relay (TCP and UDP on 127.0.0.1, amneziawgnet/relay.go:47-61) bound to the same UDP port, so whichever loses the race dies -- and when the relay loses it, Xray refuses the whole config and takes every other protocol on the host with it. The first AmneziaWG inbound on port 65101 was enough to reach it: id 1 derives exactly that port. The row now states the rule its three siblings do: it owns the slot its id derives. A node-hosted row still keeps its own port, since it binds no relay on this host. TestAddInbound_AmneziawgRefusesItsOwnRelayPort and TestUpdateInbound_AmneziawgRefusesItsOwnRelayPort fail without this -- both were watched red first -- and pin the two separate call sites, AddInbound's post-Save block and checkPortConflictTx's ignoreId > 0 block. * fix(amneziawg): keep a disabled row's relay port reserved for port forwards loadPortConflictContext filtered its query with enable = true, so a client's ForwardedPorts spec could claim the relay port a disabled AmneziaWG row's id derives. That row's relay appears with its first client -- a path that runs no port check -- and when the relay then loses the loopback bind race to the forward listener, Xray refuses the whole config instead of losing one forward (#6542 review, arrived with #6540). The context now loads every local row and gates only the ordinary-port compare on enable, which is what a disabled row's own port is worth: free. Its relay slot is not free, which is the rule #6540 already states for the other two guards. TestCheckForwardedPortsConflict_DisabledAmneziawgRelayPortIsReserved fails without this -- watched red first -- and passes with it, while TestCheckForwardedPortsConflict_IgnoresDisabledInboundPort keeps proving that a disabled inbound's own port stays available. * fix(amneziawg): re-run the forward guard once a new row has its own ports normalizeAmneziaWGSettings validates every client's ForwardedPorts before the row is saved, and loadPortConflictContext then reads the database -- so the new AmneziaWG row is never a candidate for itself. A client could forward exactly the relay port the row's own id derives, or its own WireGuard listen port, and the create was accepted: at runtime the panel's wildcard forward listener and Xray's 127.0.0.1 relay race for the same port, and a lost relay bind makes Xray refuse the whole generated config (#6544 review, pre-existing). The post-Save block is the only place the id is known, so it re-runs the guard there. Both callers now share amneziaWGForwardedPortsConflict, so the collision message lives in one place instead of two. TestAddInbound_AmneziawgRefusesAClientForwardingItsOwnRelayPort fails without this -- watched red first -- and passes with it. * fix(amneziawg): stop blocking stored forward specs on a disabled row's slot Round 2 flagged this PR's widening as the one MEDIUM it introduced, and the code confirms it: UpdateInboundClient carries a stored ForwardedPorts spec forward for a partial edit (client_inbound_apply.go:763-765) and re-validates it (:772 and :909), so after an in-place upgrade an edit that never submitted the field -- a bot enable/expiry toggle -- is refused over a slot the operator did not touch, for a relay injectAmneziawgnetSocks does not emit while the row is disabled. The inbound-save path re-validates every stored spec the same way. The trade does not pay for itself: the slot this reserves is claimable only by a spec an operator authors onto 65101-65535, while the cost lands on unrelated operations. The precise fix -- refuse a newly claimed spec rather than a stored one, and check the enable transition in SetInboundEnable, where the conflict is actually created -- is larger than the hole, so the slot goes back to a documented pre-existing item with its own follow-up. The create-path re-run added in 80eb5712 is unaffected: it reads the settings submitted in the same request, so it never refuses a stored value, and its test still passes.	2026-09-15 13:31:07 +03:00
BlindMaster24	2d8d304850	fix(amneziawg): stop a disabled inbound's relay slot from being taken (#6540) ... * test(amneziawg): pin that a disabled row still owns its relay slot checkAmneziawgnetSocksConflict filters enable = true, so a disabled AmneziaWG row is not a candidate when an ordinary inbound's configured port is validated. SetInboundEnable then flips the column with no port check, so enabling that row later puts a second inbound on 127.0.0.1:65101 and Xray refuses the whole config. Expected red on this head; the fix follows. * fix(amneziawg): count a disabled inbound as owning its relay slot The forward port check filtered its candidates with enable = true, so a disabled AmneziaWG row was invisible when an ordinary inbound's configured port was validated. Nothing else covered the gap: the relay is not a database row, and SetInboundEnable flips the column with no port check, so re-enabling that row put a second inbound on 127.0.0.1:65101 and made Xray refuse its whole config, taking every other protocol on the host down with it. A row owns the slot its id derives for as long as the row exists, which is the rule the reverse-direction check already follows. TestCheckPortConflict_ DisabledAmneziawgStillOwnsItsRelaySlot fails without this, on a test-only head whose go-test run failed on exactly that test, and passes with it. * test(amneziawg): drop the disabled-row case that asserts the reversed rule TestCheckPortConflict_AmneziawgnetSocksRelayIgnoredWhenDisabled stated, in its name and its doc comment, that a disabled AmneziaWG inbound's port must not block anything -- the rule the parent commit reverses. It also never reached the predicate it named: its fixture seeds Settings: {}, which amneziawg.InstanceFromInbound rejects on parsed.Server == nil one statement before the enable column is read, so it passed with or without the filter. Leaving it would document both rules for the same operator state with nothing failing to flag the contradiction. The rule this PR pins is covered for real by TestCheckPortConflict_DisabledAmneziawgStillOwnsItsRelaySlot, whose fixture carries a qualifying server block and an enabled peer.	2026-09-15 11:33:02 +03:00
BlindMaster24	a036ddd66f	fix(amneziawg): wrap the relay port window instead of refusing ids past it (#6539) ... * fix(amneziawg): wrap the relay port window instead of refusing ids past it An AmneziaWG inbound's loopback relay port is SOCKSBasePort + row id, and AddInbound refused any id that pushed it past 65535. The inbounds table is AUTOINCREMENT, so an id is never reused and the counter is only reset when the table empties: the 435-port window was a lifetime budget, and a database that had ever created more inbounds could never create another AmneziaWG one -- the reporter's counter sits at 70350, so the protocol never worked there at all (#6537). Ids now wrap into the same 435 ports, which leaves every id up to 435 with the exact port it had, so no existing row, relay or generated config moves. Wrapping makes the id -> port map non-injective, and nothing compared two derived relay ports before -- two relays on one port would leave Xray with a duplicate listen and refuse to start, taking the whole panel's proxy down. checkAmneziawgnetSocksRelayCollision now refuses a create or an edit whose derived port another local AmneziaWG row already owns, disabled rows included: a row owns its slot for good, and enabling it later re-runs no port check. * test(amneziawg): give each relay-window fixture its own client email Every fixture built the same client email, and an email is unique across the whole panel, so AddInbound refused the second create with "Duplicate email" before either new guard ran -- CI exercised neither the wrap nor the collision refusal. Each fixture now derives its email from its own tag, which is what the tag already exists for. * fix(amneziawg): say relay port in the relay conflict message A refusal that named the port of the automatic loopback relay read as if the named inbound listened on an unrelated port -- its own port is the WireGuard one. portConflictDetail now carries Relay, and both messages that report a derived relay port say "relay port N"; messages that report a configured port render byte-for-byte as before. * test(amneziawg): pin that a node-assigned inbound owns no relay slot A row adopted from a node carries a NodeID and the protocol it arrived with (inbound_node.go:737), yet injectAmneziawgnetSocks skips it, so it binds no loopback relay. The gate this PR added to checkPortConflictTx never looked at NodeID, so editing such a row can be refused for a slot it does not own. Expected red on this head; the fix follows. * fix(amneziawg): skip the relay guards for node-assigned inbounds Round-2 review finding: the gate this PR added to checkPortConflictTx keyed on inbound.Protocol alone, so it also ran for a row adopted from a node. Such a row carries a NodeID and gets no loopback relay -- injectAmneziawgnetSocks skips it and the desired-instance query is node_id IS NULL -- so it owns no slot and can collide with nothing, yet editing it was refused with "relay port N ... already used by inbound '<local>'", naming a port the edited row never binds. Wrapping made this visible: before it, an adopted id above 435 derived a port above 65535 that no row could hold, so the pre-existing reverse check under the same gate could not fire. Both call sites now require NodeID == nil, matching the local-only predicate the forward check already used. TestCheckPortConflict_NodeAssignedAmneziawgOwnsNoRelaySlot fails without this, with the exact false refusal, and passes with it.	2026-09-15 10:07:14 +03:00