From d36c1217c862a0a2bc1c555d5ed71bd73af990a6 Mon Sep 17 00:00:00 2001 From: Kuzz007 Date: Mon, 3 Aug 2026 11:55:19 +0300 Subject: [PATCH] fix(xray): force a full restart for password-auth SOCKS5 hot-apply Real production incident: editing a client under an AmneziaWG inbound left its embedded SOCKS5 relay's settings byte-different (a new account list), and Xray's gRPC remove+add hot swap silently dropped the account for a peer whose email contained non-ASCII characters -- its tunnel kept handshaking fine but all its traffic got rejected at the SOCKS5 layer, while every other peer on the same relay was unaffected. A full restart (reading the same JSON straight from disk) always produced the correct account list. socks isn't in userDiffableProtocols (that only covers vless/vmess/trojan's clients+email shape, not accounts+user), so any settings drift on this inbound fell through to the generic remove+add path. Forces a restart instead, the same defensive choice already made for REALITY and TPROXY -- scoped to auth:"password" specifically so the other, noauth SOCKS5 bridges (panel/node/mtproto egress) keep the cheaper hot path. --- internal/xray/hot_diff.go | 43 +++++++++++++++++++ internal/xray/hot_diff_test.go | 75 ++++++++++++++++++++++++++++++++++ 2 files changed, 118 insertions(+) diff --git a/internal/xray/hot_diff.go b/internal/xray/hot_diff.go index 73dbc20d3..dd59a396e 100644 --- a/internal/xray/hot_diff.go +++ b/internal/xray/hot_diff.go @@ -136,6 +136,10 @@ func diffInbounds(oldCfg, newCfg *Config, diff *HotDiff) bool { logger.Debug("hot diff: inbound [", oldIb.Tag, "] is a TPROXY target; a gRPC add reports success but does not reliably bind a working listener, forcing a full restart instead of a hot swap") return false } + if exists && (inboundUsesSocksAccounts(oldIb) || inboundUsesSocksAccounts(newIb)) { + logger.Debug("hot diff: inbound [", oldIb.Tag, "] is a password-auth SOCKS5 inbound (e.g. internal/amneziawgnet's per-peer relay); a gRPC remove+add reports success but was observed in production to silently drop an account, forcing a full restart instead of a hot swap") + return false + } diff.RemovedInboundTags = append(diff.RemovedInboundTags, oldIb.Tag) if exists { raw, err := json.Marshal(newIb) @@ -157,6 +161,10 @@ func diffInbounds(oldCfg, newCfg *Config, diff *HotDiff) bool { logger.Debug("hot diff: new inbound [", newIb.Tag, "] is a TPROXY target (e.g. internal/amneziawg's Xray egress bridge); a gRPC add reports success but does not reliably bind a working listener, forcing a full restart instead of a hot add") return false } + if inboundUsesSocksAccounts(newIb) { + logger.Debug("hot diff: new inbound [", newIb.Tag, "] is a password-auth SOCKS5 inbound (e.g. internal/amneziawgnet's per-peer relay); forcing a full restart instead of a hot add, same reasoning as the existing-inbound case above") + return false + } raw, err := json.Marshal(newIb) if err != nil { return false @@ -296,6 +304,41 @@ func inboundUsesTproxy(ib *InboundConfig) bool { return stream.Sockopt.Tproxy != "" && stream.Sockopt.Tproxy != "off" } +// inboundUsesSocksAccounts reports whether an inbound is a password-auth +// SOCKS5 inbound with one or more named accounts -- the shape +// internal/amneziawgnet's per-inbound relay (internal/web/service/xray.go's +// injectAmneziawgnetSocks) is the only generator of in this fork; every +// other SOCKS5 bridge this fork builds (panel egress, per-node egress, +// mtproto egress) uses "noauth" with no per-account identity at all, so this +// check can't accidentally rope in one of those lower-churn bridges. +// +// Real production incident, not a theoretical concern: a single client +// edit under an AmneziaWG inbound left this inbound's settings unchanged in +// every way relevant to accounts.user was already correct in the freshly +// regenerated config, yet the account for a peer whose email contained +// non-ASCII characters silently vanished from the running Xray process +// after a gRPC remove+add hot swap -- while a full process restart (reading +// the same JSON straight from disk) always produced the correct account +// list. socks isn't in userDiffableProtocols (that only covers vless/vmess/ +// trojan, which use a wholly different clients+email shape, not +// accounts+user), so without this check any settings drift on this inbound +// -- even one unrelated to the account list itself -- falls through to the +// generic remove+add path and can reproduce the same silent drop. Forcing a +// full restart here is the same defensive choice already made above for +// REALITY and TPROXY. +func inboundUsesSocksAccounts(ib *InboundConfig) bool { + if ib == nil || ib.Protocol != "socks" || len(ib.Settings) == 0 { + return false + } + var settings struct { + Auth string `json:"auth"` + } + if err := json.Unmarshal(ib.Settings, &settings); err != nil { + return false + } + return settings.Auth == "password" +} + func inboundHasReverseClient(ib *InboundConfig) bool { if ib == nil { return false diff --git a/internal/xray/hot_diff_test.go b/internal/xray/hot_diff_test.go index 03644311b..15eb1f81b 100644 --- a/internal/xray/hot_diff_test.go +++ b/internal/xray/hot_diff_test.go @@ -436,3 +436,78 @@ func TestComputeHotDiff_TproxyStreamChangeNeedsRestart(t *testing.T) { t.Fatal("a TPROXY bridge's port change must force a full restart, not a gRPC hot swap") } } + +// TestComputeHotDiff_SocksAccountsSettingsChangeNeedsRestart reproduces a +// real incident: editing one client under an AmneziaWG inbound left its +// relay's settings.json byte-different (a new account list) while Xray was +// already running. A gRPC remove+add reported success but silently dropped +// an account with a non-ASCII email; a full restart always produced the +// correct account list. This must force a restart, not a hot swap. +func TestComputeHotDiff_SocksAccountsSettingsChangeNeedsRestart(t *testing.T) { + relayIb := InboundConfig{ + Listen: json_util.RawMessage(`"127.0.0.1"`), + Port: 65110, + Protocol: "socks", + Tag: "in-443-udp", + Settings: json_util.RawMessage(`{"auth":"password","udp":true,"accounts":[{"user":"Роутер_awg","pass":"p"}]}`), + } + oldCfg := makeHotConfig() + oldCfg.InboundConfigs = append(oldCfg.InboundConfigs, relayIb) + newCfg := makeHotConfig() + changedIb := relayIb + changedIb.Settings = json_util.RawMessage(`{"auth":"password","udp":true,"accounts":[{"user":"Роутер_awg","pass":"p"},{"user":"Майфун🛟","pass":"p"}]}`) + newCfg.InboundConfigs = append(newCfg.InboundConfigs, changedIb) + + if _, ok := ComputeHotDiff(oldCfg, newCfg); ok { + t.Fatal("a password-auth SOCKS5 relay's account-list change must force a full restart, not a gRPC hot swap") + } +} + +// TestComputeHotDiff_NewSocksAccountsInboundNeedsRestart mirrors the TPROXY +// new-inbound test above: a brand-new AmneziaWG relay inbound must also +// force a restart, for the same reliability reason. +func TestComputeHotDiff_NewSocksAccountsInboundNeedsRestart(t *testing.T) { + oldCfg := makeHotConfig() + newCfg := makeHotConfig() + newCfg.InboundConfigs = append(newCfg.InboundConfigs, InboundConfig{ + Listen: json_util.RawMessage(`"127.0.0.1"`), + Port: 65110, + Protocol: "socks", + Tag: "in-443-udp", + Settings: json_util.RawMessage(`{"auth":"password","udp":true,"accounts":[{"user":"Майфун🛟","pass":"p"}]}`), + }) + + if _, ok := ComputeHotDiff(oldCfg, newCfg); ok { + t.Fatal("adding a new password-auth SOCKS5 relay inbound must force a full restart, not a gRPC hot add") + } +} + +// TestComputeHotDiff_NoauthSocksBridgeStaysHot confirms the check above is +// scoped to password-auth accounts specifically: this fork's other SOCKS5 +// bridges (panel egress, per-node egress, mtproto egress) use "noauth" with +// no per-account identity, have no history of this failure mode, and must +// keep using the ordinary remove+add hot path rather than pay for an +// unnecessary restart on every port/tag change. +func TestComputeHotDiff_NoauthSocksBridgeStaysHot(t *testing.T) { + bridgeIb := InboundConfig{ + Listen: json_util.RawMessage(`"127.0.0.1"`), + Port: 62790, + Protocol: "socks", + Tag: "panel-egress", + Settings: json_util.RawMessage(`{"auth":"noauth","udp":false}`), + } + oldCfg := makeHotConfig() + oldCfg.InboundConfigs = append(oldCfg.InboundConfigs, bridgeIb) + newCfg := makeHotConfig() + changedIb := bridgeIb + changedIb.Port = 62791 + newCfg.InboundConfigs = append(newCfg.InboundConfigs, changedIb) + + diff, ok := ComputeHotDiff(oldCfg, newCfg) + if !ok { + t.Fatal("a noauth SOCKS5 bridge's port change should stay hot-appliable") + } + if len(diff.RemovedInboundTags) != 1 || len(diff.AddedInbounds) != 1 { + t.Fatalf("expected a plain remove+add for the changed bridge, got %+v", diff) + } +}