diff --git a/internal/web/service/client_attach_test.go b/internal/web/service/client_attach_test.go index 9058cd0fa..cf53ada50 100644 --- a/internal/web/service/client_attach_test.go +++ b/internal/web/service/client_attach_test.go @@ -54,3 +54,44 @@ func TestHasTunnelAttachmentDetectsWireguardOrAmneziaWG(t *testing.T) { t.Error("an AmneziaWG inbound must count as a tunnel attachment") } } + +// TestAddressesFitAmneziaWGInbound is a regression test for a real +// production bug: hasTunnelAttachment only asked "does this identity have +// ANY tunnel attachment", not "is the address it would inherit actually +// valid for THIS inbound" -- so an identity whose stored address came from +// WireGuard's own fallback subnet (10.0.0.0/24, used when that inbound has +// no other clients to infer a base from) got that exact address silently +// carried over onto a second, AmneziaWG inbound configured for a completely +// different subnet (10.8.1.0/24). defaultAmneziaWGClients's already-set +// branch only checks for collisions, not subnet membership, so the mismatch +// was accepted with no error -- producing a peer that can never actually +// connect (an AmneziaWG address must fall inside the kernel interface's own +// configured subnet to be routable at all). addressesFitAmneziaWGInbound is +// the check Attach now runs per inbound before deciding whether to keep an +// inherited address or force a fresh allocation. +func TestAddressesFitAmneziaWGInbound(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":[]}`) + seedInboundConflict(t, "wg-1", "0.0.0.0", 51820, model.WireGuard, ``, `{"clients":[]}`) + + var awgInbound, wgInbound model.Inbound + if err := database.GetDB().Where("tag = ?", "awg-1").First(&awgInbound).Error; err != nil { + t.Fatalf("read seeded awg row: %v", err) + } + if err := database.GetDB().Where("tag = ?", "wg-1").First(&wgInbound).Error; err != nil { + t.Fatalf("read seeded wg row: %v", err) + } + + if !addressesFitAmneziaWGInbound(nil, &awgInbound) { + t.Error("no addresses at all must trivially fit (Attach's own fresh-allocate path)") + } + if !addressesFitAmneziaWGInbound([]string{"10.0.0.2/32"}, &wgInbound) { + t.Error("WireGuard has no strict subnet requirement -- must never be rejected here") + } + if addressesFitAmneziaWGInbound([]string{"10.0.0.2/32"}, &awgInbound) { + t.Fatal("the real bug: a WireGuard-fallback-subnet address must NOT be accepted as fitting an AmneziaWG inbound configured for a different subnet") + } + if !addressesFitAmneziaWGInbound([]string{"10.8.1.21/32"}, &awgInbound) { + t.Error("an address genuinely inside the awg inbound's own configured subnet must fit") + } +} diff --git a/internal/web/service/client_crud.go b/internal/web/service/client_crud.go index e675f43fd..5656920e1 100644 --- a/internal/web/service/client_crud.go +++ b/internal/web/service/client_crud.go @@ -5,6 +5,7 @@ import ( "encoding/json" "errors" "fmt" + "net/netip" "strings" "time" "unicode" @@ -620,6 +621,59 @@ func (s *ClientService) hasTunnelAttachment(inboundSvc *InboundService, inboundI return false } +// addressesFitAmneziaWGInbound reports whether every entry in addrs parses +// as a host address inside ib's own configured subnet(s). Only AmneziaWG is +// checked -- unlike WireGuard (allocateWireguardAddress can widen out to a +// /16 fallback pool for it), an AmneziaWG peer's address must fall inside +// the kernel interface's own configured Address subnet to be routable at +// all (see allocateWireguardAddress's allowWidening doc comment), so +// inheriting an address from an unrelated subnet isn't just cosmetically +// wrong for AmneziaWG, it produces a peer that can never actually connect. +// Real case this guards against: an identity's stored address came from +// WireGuard's own fallback subnet (10.0.0.0/24, used when that inbound has +// no other clients to infer a base from) and gets attached to a second, +// AmneziaWG inbound whose configured subnet is something else entirely +// (e.g. 10.8.1.0/24) -- addressesFitAmneziaWGInbound catches that mismatch +// so Attach can allocate fresh for this inbound instead of silently +// persisting an unroutable peer. +func addressesFitAmneziaWGInbound(addrs []string, ib *model.Inbound) bool { + if ib.Protocol != model.AmneziaWG || len(addrs) == 0 { + return true + } + v4Base, v6Base, err := defaultAmneziaWGSubnetBases(ib.Settings) + if err != nil { + return false + } + bases := make([]netip.Prefix, 0, 2) + for _, base := range []string{v4Base, v6Base} { + if base == "" { + continue + } + prefix, pErr := netip.ParsePrefix(base) + if pErr != nil { + return false + } + bases = append(bases, prefix) + } + for _, a := range addrs { + host := wireguardHostAddr(a) + if !host.IsValid() { + return false + } + fits := false + for _, prefix := range bases { + if prefix.Contains(host) { + fits = true + break + } + } + if !fits { + return false + } + } + return true +} + func (s *ClientService) Attach(inboundSvc *InboundService, id int, inboundIds []int) (bool, error) { existing, err := s.GetByID(id) if err != nil { @@ -669,6 +723,9 @@ func (s *ClientService) Attach(inboundSvc *InboundService, id int, inboundIds [] return needRestart, getErr } copyClient := *clientWire + if !addressesFitAmneziaWGInbound(copyClient.AllowedIPs, inbound) { + copyClient.AllowedIPs = nil + } if err := s.fillProtocolDefaults(©Client, inbound); err != nil { return needRestart, err }