From 9ee4aa707325a127b6cedd05a9341a88d27381be Mon Sep 17 00:00:00 2001 From: ShenJiaqing <48464190+shenaba@users.noreply.github.com> Date: Wed, 16 Sep 2026 14:17:20 +0800 Subject: [PATCH 1/3] fix(core): cut the sessions of a user who was just removed Protocols that authenticate once per session keep serving a client after its user is gone from the inbound. Swapping the user table only decides who may start a *new* session, and ConnTracker closes the routed connections but not the session carrying them -- so the client opens another stream on the one it already has and is served. That is issue #175: a client DepleteJob disabled for running out of quota kept running, and the same hole covers every multiplex session, where one authenticated carrier connection serves every stream after it. core/usersession records, per inbound, which user the session at a source address authenticated as. Removing a user closes the sessions there is a closer to reach -- anytls, and the sing-mux carrier behind vless, vmess and trojan -- and mutes the ones there is not, which is all the QUIC protocols offer: their session lives inside sing-quic with no handle out. A muted source has nothing routed for it any more, so the traffic stops either way, and the block ages out after ten minutes rather than becoming a lockout. This sits at the inbound layer rather than in ConnTracker because a tracker-level gate only sees a connection after routing: it misses the ones the router answers itself, and refusing there still costs one real dial to the destination first. The two layers stay separate -- IP limits keep their gate in ConnTracker, whose ban state has to outlive a core restart. The hook is a router wrapper, not a field plus a block in every handler, so each copy under core/protocol carries a single added line: inbound.router = withUserSessions(inbound.router) Everything that line reaches lives in users.go, which the copy check skips, so the expected diffs grow by one each. anytls costs three instead: it holds its session in NewConnection, which the router never sees, so that call is redirected through users.go as well. Cutting hangs off each protocol's UpdateUsers, which leaves the service layer untouched and still covers all three paths that reach it -- DepleteJob, a panel save, and a node push. Two details are easy to undo by accident: - Idle entries are only dropped when they have no closer. A tracked session's lastSeen moves only when it opens another connection, so one carrying a single long-lived stream looks idle while it is perfectly alive; sweeping it would discard the only handle on it and the next removal would find nothing to cut. Tracked entries are cleaned up by their own deferred Untrack. - The user and the closer are recorded under one lock. Split apart, a removal landing in between sees a user with no closer, files the session as unclosable and mutes it -- and a mux carrier is never gated, so that mute does nothing at all. Verified on the test server, three protocol tunnels driven by a probe that reconnects every second, with the client expired and DepleteJob disabling it. Attempts made strictly after the disable: anytls hysteria2 vless+mux 1.8.2, no change 11 served 11 served 11 served with this change 0 served 0 served 0 served The control was built from main at the same version and sing-box, so the only variable is this change. --- core/protocol/anytls/inbound.go | 12 +- core/protocol/anytls/users.go | 52 +++++ core/protocol/hysteria/inbound.go | 13 +- core/protocol/hysteria/users.go | 17 ++ core/protocol/hysteria2/inbound.go | 13 +- core/protocol/hysteria2/users.go | 17 ++ core/protocol/trojan/inbound.go | 13 +- core/protocol/trojan/users.go | 32 ++- core/protocol/tuic/inbound.go | 13 +- core/protocol/tuic/users.go | 24 +++ core/protocol/vless/inbound.go | 13 +- core/protocol/vless/users.go | 25 +++ core/protocol/vmess/inbound.go | 13 +- core/protocol/vmess/users.go | 27 ++- core/usersession/registry.go | 318 ++++++++++++++++++++++++++++ core/usersession/registry_test.go | 327 +++++++++++++++++++++++++++++ core/usersession/router.go | 219 +++++++++++++++++++ core/usersession/router_test.go | 228 ++++++++++++++++++++ go.mod | 2 +- scripts/check-protocol-copies.sh | 45 ++-- 20 files changed, 1394 insertions(+), 29 deletions(-) create mode 100644 core/usersession/registry.go create mode 100644 core/usersession/registry_test.go create mode 100644 core/usersession/router.go create mode 100644 core/usersession/router_test.go diff --git a/core/protocol/anytls/inbound.go b/core/protocol/anytls/inbound.go index f84395c7..69561fe7 100644 --- a/core/protocol/anytls/inbound.go +++ b/core/protocol/anytls/inbound.go @@ -5,10 +5,15 @@ // tearing the listener down (see core/inbound_users.go). // // UPGRADING sing-box: re-copy this file from the new tag and re-apply the -// change below. Nothing here will fail to compile if you forget, it will just +// changes below. Nothing here will fail to compile if you forget, it will just // silently keep running the old implementation. // -// Local change vs sing-box: none, other than the package clause. +// Local change vs sing-box: two lines, both for the user-session registry. +// NewInbound wraps the router with it, and NewConnection -- which owns an +// anytls session for as long as that session lives -- is redirected through +// newSessionConnection in users.go, so the session can be registered there and +// closed later. Without it, removing a user only stops new sessions while the +// one the client already holds keeps being served. See core/usersession. package anytls import ( @@ -54,6 +59,7 @@ func NewInbound(ctx context.Context, router adapter.Router, logger log.ContextLo router: uot.NewRouter(router, logger), logger: logger, } + inbound.router = withUserSessions(inbound.router) if options.TLS != nil && options.TLS.Enabled { tlsConfig, err := tls.NewServer(ctx, logger, common.PtrValueOrDefault(options.TLS)) @@ -117,7 +123,7 @@ func (h *Inbound) NewConnection(ctx context.Context, conn net.Conn, metadata ada } conn = tlsConn } - err := h.service.NewConnection(adapter.WithContext(ctx, &metadata), conn, metadata.Source, onClose) + err := h.newSessionConnection(adapter.WithContext(ctx, &metadata), conn, metadata.Source, onClose) if err != nil { N.CloseOnHandshakeFailure(conn, onClose, err) h.logger.ErrorContext(ctx, E.Cause(err, "process connection from ", metadata.Source)) diff --git a/core/protocol/anytls/users.go b/core/protocol/anytls/users.go index 289319d1..907aa8fa 100644 --- a/core/protocol/anytls/users.go +++ b/core/protocol/anytls/users.go @@ -1,15 +1,67 @@ package anytls import ( + "context" + "net" + + "github.com/shenaba/2s-ui/core/usersession" + + "github.com/sagernet/sing-box/adapter" "github.com/sagernet/sing-box/option" "github.com/sagernet/sing/common" + M "github.com/sagernet/sing/common/metadata" + N "github.com/sagernet/sing/common/network" anytls "github.com/anytls/sing-anytls" ) +// UpdateUsers swaps the user table of a running inbound, and cuts the sessions +// of everyone who just left it. Both halves are needed: the table alone only +// decides who may open a *new* session, while an anytls client that +// authenticated before the change keeps opening streams on the one it has. func (h *Inbound) UpdateUsers(users []option.AnyTLSUser) error { h.service.UpdateUsers(common.Map(users, func(it option.AnyTLSUser) anytls.User { return (anytls.User)(it) })) + keep := make(map[string]struct{}, len(users)) + for _, user := range users { + keep[user.Name] = struct{}{} + } + h.sessions().CloseUsers(keep) return nil } + +// withUserSessions installs the session registry. anytls only needs the router +// to record who is connected -- the session itself is reached in +// newSessionConnection below, which is where it can be closed. +func withUserSessions(router adapter.ConnectionRouterEx) adapter.ConnectionRouterEx { + return usersession.WrapRouterEx(router, usersession.BindOnly) +} + +// sessions reaches the registry installed by withUserSessions. NewInbound wraps +// the router right after building the struct and nothing replaces the field +// afterwards, so this holds. It is a bare assertion on purpose: someone +// reordering that would otherwise turn every session hook here into a silent +// no-op, and a panic on the save path is the lesser outcome. +func (h *Inbound) sessions() *usersession.Registry { + return h.router.(*usersession.RouterEx).Registry() +} + +// newSessionConnection stands in for h.service.NewConnection at the one call +// site that owns an anytls session for its whole life. Registering the session +// here is what makes it closable; the streams it later opens reach the router +// individually and cannot be used to find it. +// +// Returning an error rather than closing the connection reuses the rejection +// the call site already has (N.CloseOnHandshakeFailure). +func (h *Inbound) newSessionConnection(ctx context.Context, conn net.Conn, source M.Socksaddr, onClose N.CloseHandlerFunc) error { + key := source.String() + // A session with a closer is cut outright rather than muted, so this only + // fires if closing one failed -- keeping the mute as the backstop. + if !h.sessions().Allowed(key) { + return usersession.ErrRemoved + } + h.sessions().Track(key, conn) + defer h.sessions().Untrack(key) + return h.service.NewConnection(ctx, conn, source, onClose) +} diff --git a/core/protocol/hysteria/inbound.go b/core/protocol/hysteria/inbound.go index f0c35c07..791e3706 100644 --- a/core/protocol/hysteria/inbound.go +++ b/core/protocol/hysteria/inbound.go @@ -5,7 +5,7 @@ // tearing the listener down (see core/inbound_users.go). // // UPGRADING sing-box: re-copy this file from the new tag and re-apply the -// change below. Nothing here will fail to compile if you forget, it will just +// changes below. Nothing here will fail to compile if you forget, it will just // silently keep running the old implementation. // // Local change vs sing-box: the service is keyed by user name @@ -14,6 +14,16 @@ // UpdateUsers does rewrite it, under live sessions, and a position is not. // Deleting a user shifted every later one, which mis-attributed traffic and // could index past the end of the name slice outright (upstream #1231). +// +// Second local change: one line installs the user-session registry +// +// inbound.router = withUserSessions(inbound.router) +// +// which is what lets an already authenticated session be cut, or refused, when +// its user is removed from the inbound -- swapping the user table alone only +// decides who may start a new one. That line is the whole of it here; +// everything it reaches lives in users.go, which the copy check skips. See +// core/usersession. package hysteria import ( @@ -68,6 +78,7 @@ func NewInbound(ctx context.Context, router adapter.Router, logger log.ContextLo }), tlsConfig: tlsConfig, } + inbound.router = withUserSessions(inbound.router) var sendBps, receiveBps uint64 if options.Up.Value() > 0 { sendBps = options.Up.Value() diff --git a/core/protocol/hysteria/users.go b/core/protocol/hysteria/users.go index b36b4711..9eadbc9b 100644 --- a/core/protocol/hysteria/users.go +++ b/core/protocol/hysteria/users.go @@ -1,9 +1,15 @@ package hysteria import ( + "github.com/shenaba/2s-ui/core/usersession" + + "github.com/sagernet/sing-box/adapter" "github.com/sagernet/sing-box/option" ) +// UpdateUsers swaps the user table of a running inbound and shuts out everyone +// who just left it -- see the note in tuic/users.go, hysteria reuses an +// authenticated session the same way. func (h *Inbound) UpdateUsers(users []option.HysteriaUser) error { userList := make([]string, 0, len(users)) userPasswordList := make([]string, 0, len(users)) @@ -18,5 +24,16 @@ func (h *Inbound) UpdateUsers(users []option.HysteriaUser) error { userPasswordList = append(userPasswordList, password) } h.service.UpdateUsers(userList, userPasswordList) + h.sessions().CloseUsers(usersession.KeepSet(userList)) return nil } + +// withUserSessions installs the session registry in front of the router. This +// inbound holds the full adapter.Router, so it takes the Router shim. +func withUserSessions(router adapter.Router) adapter.Router { + return usersession.WrapRouter(router, usersession.GateAndBind) +} + +func (h *Inbound) sessions() *usersession.Registry { + return h.router.(*usersession.Router).Registry() +} diff --git a/core/protocol/hysteria2/inbound.go b/core/protocol/hysteria2/inbound.go index 416d33e9..b69f13a1 100644 --- a/core/protocol/hysteria2/inbound.go +++ b/core/protocol/hysteria2/inbound.go @@ -5,7 +5,7 @@ // tearing the listener down (see core/inbound_users.go). // // UPGRADING sing-box: re-copy this file from the new tag and re-apply the -// change below. Nothing here will fail to compile if you forget, it will just +// changes below. Nothing here will fail to compile if you forget, it will just // silently keep running the old implementation. // // Local change vs sing-box: the service is keyed by user name @@ -14,6 +14,16 @@ // UpdateUsers does rewrite it, under live sessions, and a position is not. // Deleting a user shifted every later one, which mis-attributed traffic and // could index past the end of the name slice outright (upstream #1231). +// +// Second local change: one line installs the user-session registry +// +// inbound.router = withUserSessions(inbound.router) +// +// which is what lets an already authenticated session be cut, or refused, when +// its user is removed from the inbound -- swapping the user table alone only +// decides who may start a new one. That line is the whole of it here; +// everything it reaches lives in users.go, which the copy check skips. See +// core/usersession. package hysteria2 import ( @@ -139,6 +149,7 @@ func NewInbound(ctx context.Context, router adapter.Router, logger log.ContextLo }), tlsConfig: tlsConfig, } + inbound.router = withUserSessions(inbound.router) var udpTimeout time.Duration if options.UDPTimeout != 0 { udpTimeout = time.Duration(options.UDPTimeout) diff --git a/core/protocol/hysteria2/users.go b/core/protocol/hysteria2/users.go index 2038dc8e..d942ac3a 100644 --- a/core/protocol/hysteria2/users.go +++ b/core/protocol/hysteria2/users.go @@ -1,9 +1,15 @@ package hysteria2 import ( + "github.com/shenaba/2s-ui/core/usersession" + + "github.com/sagernet/sing-box/adapter" "github.com/sagernet/sing-box/option" ) +// UpdateUsers swaps the user table of a running inbound and shuts out everyone +// who just left it -- see the note in tuic/users.go, hysteria2 reuses an +// authenticated session the same way. func (h *Inbound) UpdateUsers(users []option.Hysteria2User) error { userList := make([]string, 0, len(users)) userPasswordList := make([]string, 0, len(users)) @@ -12,5 +18,16 @@ func (h *Inbound) UpdateUsers(users []option.Hysteria2User) error { userPasswordList = append(userPasswordList, user.Password) } h.service.UpdateUsers(userList, userPasswordList) + h.sessions().CloseUsers(usersession.KeepSet(userList)) return nil } + +// withUserSessions installs the session registry in front of the router. This +// inbound holds the full adapter.Router, so it takes the Router shim. +func withUserSessions(router adapter.Router) adapter.Router { + return usersession.WrapRouter(router, usersession.GateAndBind) +} + +func (h *Inbound) sessions() *usersession.Registry { + return h.router.(*usersession.Router).Registry() +} diff --git a/core/protocol/trojan/inbound.go b/core/protocol/trojan/inbound.go index 5dfb7077..b1b07223 100644 --- a/core/protocol/trojan/inbound.go +++ b/core/protocol/trojan/inbound.go @@ -5,7 +5,7 @@ // tearing the listener down (see core/inbound_users.go). // // UPGRADING sing-box: re-copy this file from the new tag and re-apply the -// change below. Nothing here will fail to compile if you forget, it will just +// changes below. Nothing here will fail to compile if you forget, it will just // silently keep running the old implementation. // // Local change vs sing-box: the service is keyed by user name @@ -14,6 +14,16 @@ // UpdateUsers does rewrite it, under live sessions, and a position is not. // Deleting a user shifted every later one, which mis-attributed traffic and // could index past the end of the name slice outright (upstream #1231). +// +// Second local change: one line installs the user-session registry +// +// inbound.router = withUserSessions(inbound.router) +// +// which is what lets an already authenticated session be cut, or refused, when +// its user is removed from the inbound -- swapping the user table alone only +// decides who may start a new one. That line is the whole of it here; +// everything it reaches lives in users.go, which the copy check skips. See +// core/usersession. package trojan import ( @@ -118,6 +128,7 @@ func NewInbound(ctx context.Context, router adapter.Router, logger log.ContextLo if err != nil { return nil, err } + inbound.router = withUserSessions(inbound.router) inbound.service = service inbound.listener = listener.New(listener.Options{ Context: ctx, diff --git a/core/protocol/trojan/users.go b/core/protocol/trojan/users.go index a90e1879..f4219b32 100644 --- a/core/protocol/trojan/users.go +++ b/core/protocol/trojan/users.go @@ -1,14 +1,44 @@ package trojan import ( + "github.com/shenaba/2s-ui/core/usersession" + + "github.com/sagernet/sing-box/adapter" "github.com/sagernet/sing-box/option" "github.com/sagernet/sing/common" ) +// UpdateUsers swaps the user table of a running inbound and cuts the multiplex +// sessions of everyone who just left it -- see the note in vless/users.go. +// Sessions are only cut once the table actually changed: trojan rejects a +// duplicate name and fails the whole update, and that must leave the inbound +// exactly as it was. func (h *Inbound) UpdateUsers(users []option.TrojanUser) error { - return h.service.UpdateUsers(common.Map(users, func(it option.TrojanUser) string { + err := h.service.UpdateUsers(common.Map(users, func(it option.TrojanUser) string { return it.Name }), common.Map(users, func(it option.TrojanUser) string { return it.Password })) + if err != nil { + return err + } + h.sessions().CloseUsers(usersession.KeepSet(common.Map(users, func(it option.TrojanUser) string { + return it.Name + }))) + return nil +} + +// withUserSessions installs the session registry in front of the router. Only +// the multiplex carrier is tracked: every other connection authenticates on its +// own and is already covered by ConnTracker. +// +// Deliberately not a gate: the fallback path routes unauthenticated visitors +// through this same router, and refusing there would cut off the fallback for +// whoever happens to share a muted source. +func withUserSessions(router adapter.ConnectionRouterEx) adapter.ConnectionRouterEx { + return usersession.WrapRouterEx(router, usersession.TrackMuxCarrier) +} + +func (h *Inbound) sessions() *usersession.Registry { + return h.router.(*usersession.RouterEx).Registry() } diff --git a/core/protocol/tuic/inbound.go b/core/protocol/tuic/inbound.go index 668d2e12..80c4247e 100644 --- a/core/protocol/tuic/inbound.go +++ b/core/protocol/tuic/inbound.go @@ -5,7 +5,7 @@ // tearing the listener down (see core/inbound_users.go). // // UPGRADING sing-box: re-copy this file from the new tag and re-apply the -// change below. Nothing here will fail to compile if you forget, it will just +// changes below. Nothing here will fail to compile if you forget, it will just // silently keep running the old implementation. // // Local change vs sing-box: the service is keyed by user name @@ -14,6 +14,16 @@ // UpdateUsers does rewrite it, under live sessions, and a position is not. // Deleting a user shifted every later one, which mis-attributed traffic and // could index past the end of the name slice outright (upstream #1231). +// +// Second local change: one line installs the user-session registry +// +// inbound.router = withUserSessions(inbound.router) +// +// which is what lets an already authenticated session be cut, or refused, when +// its user is removed from the inbound -- swapping the user table alone only +// decides who may start a new one. That line is the whole of it here; +// everything it reaches lives in users.go, which the copy check skips. See +// core/usersession. package tuic import ( @@ -73,6 +83,7 @@ func NewInbound(ctx context.Context, router adapter.Router, logger log.ContextLo }), tlsConfig: tlsConfig, } + inbound.router = withUserSessions(inbound.router) var udpTimeout time.Duration if options.UDPTimeout != 0 { udpTimeout = time.Duration(options.UDPTimeout) diff --git a/core/protocol/tuic/users.go b/core/protocol/tuic/users.go index 8c4624f4..f34e8581 100644 --- a/core/protocol/tuic/users.go +++ b/core/protocol/tuic/users.go @@ -1,12 +1,19 @@ package tuic import ( + "github.com/shenaba/2s-ui/core/usersession" + + "github.com/sagernet/sing-box/adapter" "github.com/sagernet/sing-box/option" E "github.com/sagernet/sing/common/exceptions" "github.com/gofrs/uuid/v5" ) +// UpdateUsers swaps the user table of a running inbound and shuts out everyone +// who just left it. Both halves are needed: the table alone only decides who +// may open a new session, while a client that authenticated before the change +// keeps opening streams on the one it already has. func (h *Inbound) UpdateUsers(users []option.TUICUser) error { userList := make([]string, 0, len(users)) userUUIDList := make([][16]byte, 0, len(users)) @@ -24,5 +31,22 @@ func (h *Inbound) UpdateUsers(users []option.TUICUser) error { userPasswordList = append(userPasswordList, user.Password) } h.server.UpdateUsers(userList, userUUIDList, userPasswordList) + h.sessions().CloseUsers(usersession.KeepSet(userList)) return nil } + +// withUserSessions installs the session registry in front of the router. TUIC +// authenticates once per QUIC session and every stream after that rides it, so +// a removed user has to be refused here -- refused rather than cut, because the +// QUIC session is not something this layer holds a handle on. See +// core/usersession for why the hook sits on the router. +func withUserSessions(router adapter.ConnectionRouterEx) adapter.ConnectionRouterEx { + return usersession.WrapRouterEx(router, usersession.GateAndBind) +} + +// sessions reaches the registry withUserSessions installed. The assertion is +// bare on purpose: reordering NewInbound so that it no longer holds would +// otherwise turn every hook here into a silent no-op. +func (h *Inbound) sessions() *usersession.Registry { + return h.router.(*usersession.RouterEx).Registry() +} diff --git a/core/protocol/vless/inbound.go b/core/protocol/vless/inbound.go index 9b9b06a4..91539eb8 100644 --- a/core/protocol/vless/inbound.go +++ b/core/protocol/vless/inbound.go @@ -5,7 +5,7 @@ // tearing the listener down (see core/inbound_users.go). // // UPGRADING sing-box: re-copy this file from the new tag and re-apply the -// change below. Nothing here will fail to compile if you forget, it will just +// changes below. Nothing here will fail to compile if you forget, it will just // silently keep running the old implementation. // // Local change vs sing-box: the service is keyed by user name @@ -14,6 +14,16 @@ // UpdateUsers does rewrite it, under live sessions, and a position is not. // Deleting a user shifted every later one, which mis-attributed traffic and // could index past the end of the name slice outright (upstream #1231). +// +// Second local change: one line installs the user-session registry +// +// inbound.router = withUserSessions(inbound.router) +// +// which is what lets an already authenticated session be cut, or refused, when +// its user is removed from the inbound -- swapping the user table alone only +// decides who may start a new one. That line is the whole of it here; +// everything it reaches lives in users.go, which the copy check skips. See +// core/usersession. package vless import ( @@ -71,6 +81,7 @@ func NewInbound(ctx context.Context, router adapter.Router, logger log.ContextLo if err != nil { return nil, err } + inbound.router = withUserSessions(inbound.router) service := vless.NewService[string](logger, adapter.NewUpstreamContextHandler(inbound.newConnectionEx, inbound.newPacketConnectionEx)) service.UpdateUsers(common.Map(options.Users, func(it option.VLESSUser) string { return it.Name diff --git a/core/protocol/vless/users.go b/core/protocol/vless/users.go index c436ee46..6042d03b 100644 --- a/core/protocol/vless/users.go +++ b/core/protocol/vless/users.go @@ -1,10 +1,18 @@ package vless import ( + "github.com/shenaba/2s-ui/core/usersession" + + "github.com/sagernet/sing-box/adapter" "github.com/sagernet/sing-box/option" "github.com/sagernet/sing/common" ) +// UpdateUsers swaps the user table of a running inbound and cuts the multiplex +// sessions of everyone who just left it. VLESS authenticates per connection, so +// the table alone already shuts a removed user out of new ones -- except on a +// multiplex session, where one authenticated carrier connection keeps serving +// every stream opened after the change. func (h *Inbound) UpdateUsers(users []option.VLESSUser) error { h.service.UpdateUsers(common.Map(users, func(it option.VLESSUser) string { return it.Name @@ -13,5 +21,22 @@ func (h *Inbound) UpdateUsers(users []option.VLESSUser) error { }), common.Map(users, func(it option.VLESSUser) string { return it.Flow })) + h.sessions().CloseUsers(usersession.KeepSet(common.Map(users, func(it option.VLESSUser) string { + return it.Name + }))) return nil } + +// withUserSessions installs the session registry in front of the router. Only +// the multiplex carrier is tracked: every other connection authenticates on its +// own and is already covered by ConnTracker. See core/usersession. +func withUserSessions(router adapter.ConnectionRouterEx) adapter.ConnectionRouterEx { + return usersession.WrapRouterEx(router, usersession.TrackMuxCarrier) +} + +// sessions reaches the registry withUserSessions installed. The assertion is +// bare on purpose: reordering NewInbound so that it no longer holds would +// otherwise turn every hook here into a silent no-op. +func (h *Inbound) sessions() *usersession.Registry { + return h.router.(*usersession.RouterEx).Registry() +} diff --git a/core/protocol/vmess/inbound.go b/core/protocol/vmess/inbound.go index a9372700..644a2206 100644 --- a/core/protocol/vmess/inbound.go +++ b/core/protocol/vmess/inbound.go @@ -5,7 +5,7 @@ // tearing the listener down (see core/inbound_users.go). // // UPGRADING sing-box: re-copy this file from the new tag and re-apply the -// change below. Nothing here will fail to compile if you forget, it will just +// changes below. Nothing here will fail to compile if you forget, it will just // silently keep running the old implementation. // // Local change vs sing-box: the service is keyed by user name @@ -16,6 +16,16 @@ // could index past the end of the name slice outright (upstream #1231). // The sing-vmess import also needs an explicit `vmess` alias, since this // file's own package is called vmess too. +// +// Second local change: one line installs the user-session registry +// +// inbound.router = withUserSessions(inbound.router) +// +// which is what lets an already authenticated session be cut, or refused, when +// its user is removed from the inbound -- swapping the user table alone only +// decides who may start a new one. That line is the whole of it here; +// everything it reaches lives in users.go, which the copy check skips. See +// core/usersession. package vmess import ( @@ -74,6 +84,7 @@ func NewInbound(ctx context.Context, router adapter.Router, logger log.ContextLo if err != nil { return nil, err } + inbound.router = withUserSessions(inbound.router) var serviceOptions []vmess.ServiceOption if timeFunc := ntp.TimeFuncFromContext(ctx); timeFunc != nil { serviceOptions = append(serviceOptions, vmess.ServiceWithTimeFunc(timeFunc)) diff --git a/core/protocol/vmess/users.go b/core/protocol/vmess/users.go index b98d3109..77090a8d 100644 --- a/core/protocol/vmess/users.go +++ b/core/protocol/vmess/users.go @@ -1,16 +1,41 @@ package vmess import ( + "github.com/shenaba/2s-ui/core/usersession" + + "github.com/sagernet/sing-box/adapter" "github.com/sagernet/sing-box/option" "github.com/sagernet/sing/common" ) +// UpdateUsers swaps the user table of a running inbound and cuts the multiplex +// sessions of everyone who just left it -- see the note in vless/users.go. +// Sessions are only cut once the table actually changed, so a rejected update +// leaves the inbound exactly as it was. func (h *Inbound) UpdateUsers(users []option.VMessUser) error { - return h.service.UpdateUsers(common.Map(users, func(it option.VMessUser) string { + err := h.service.UpdateUsers(common.Map(users, func(it option.VMessUser) string { return it.Name }), common.Map(users, func(it option.VMessUser) string { return it.UUID }), common.Map(users, func(it option.VMessUser) int { return it.AlterId })) + if err != nil { + return err + } + h.sessions().CloseUsers(usersession.KeepSet(common.Map(users, func(it option.VMessUser) string { + return it.Name + }))) + return nil +} + +// withUserSessions installs the session registry in front of the router. Only +// the multiplex carrier is tracked: every other connection authenticates on its +// own and is already covered by ConnTracker. See core/usersession. +func withUserSessions(router adapter.ConnectionRouterEx) adapter.ConnectionRouterEx { + return usersession.WrapRouterEx(router, usersession.TrackMuxCarrier) +} + +func (h *Inbound) sessions() *usersession.Registry { + return h.router.(*usersession.RouterEx).Registry() } diff --git a/core/usersession/registry.go b/core/usersession/registry.go new file mode 100644 index 00000000..fdc1f94c --- /dev/null +++ b/core/usersession/registry.go @@ -0,0 +1,318 @@ +// Package usersession tracks the client sessions of an inbound, so that the +// sessions of a user who was just removed can actually be cut. +// +// Protocols that authenticate once per session (the QUIC ones, anytls, and +// anything carried over sing-mux) keep serving an already authenticated client +// after its user is gone from the inbound: swapping the auth map only affects +// new sessions, and closing the routed connections is not enough because the +// client simply opens another stream on the session it already has. Reaching +// the session itself is what this registry is for. +// +// This lives at the inbound layer rather than in ConnTracker on purpose. A +// tracker-level gate sees a connection only after routing, so it misses the +// ones the router answers itself (hijack-dns), and refusing there still costs +// one real dial to the destination before the copy fails. Refusing here happens +// before any of that. The two layers stay separate: IP limits keep their gate +// in ConnTracker, because that policy has to outlive a core restart, while +// everything here is per-inbound and goes away with the Box. +package usersession + +import ( + "io" + "sync" + "time" + + "github.com/sagernet/sing/common" + E "github.com/sagernet/sing/common/exceptions" + N "github.com/sagernet/sing/common/network" +) + +const ( + // A source that has not opened a connection for this long is forgotten; + // its session is either gone or idle enough to be re-learned on use. + idleTimeout = 10 * time.Minute + // Backstop for blocks: a client whose session is really dead stops being + // blocked, so the address is reusable even if the user is never re-added. + blockTimeout = 10 * time.Minute + // A kicked session is muted only while it keeps trying. Once it has been + // quiet this long, the next attempt from the address is a new session and + // is let through, so a disconnect does not turn into a lockout. + kickQuietWindow = 30 * time.Second +) + +type entry struct { + user string + lastSeen time.Time + closer io.Closer +} + +// block mutes one client address. A removal keeps it muted until the backstop; +// a kick, which must not lock a still-enabled user out, lifts as soon as the +// muted session stops trying. +type block struct { + at time.Time + lastAttempt time.Time + kick bool +} + +// Registry maps a client address to the user it authenticated as. One instance +// per inbound. +// +// The key is the full source address including the port, never a normalized +// one: it identifies a single session, and two sessions from one subscriber +// must not share an entry. (ConnTracker deliberately does the opposite -- it +// masks IPv6 to a prefix -- because an IP limit counts subscribers, not +// sessions. Keying this map that way would mute an entire /64 when one client +// in it is removed.) +type Registry struct { + access sync.Mutex + sources map[string]*entry + blocked map[string]*block + lastSweep time.Time +} + +func NewRegistry() *Registry { + return &Registry{ + sources: make(map[string]*entry), + blocked: make(map[string]*block), + lastSweep: time.Now(), + } +} + +func (r *Registry) load(source string) *entry { + e, loaded := r.sources[source] + if !loaded { + e = &entry{} + r.sources[source] = e + } + e.lastSeen = time.Now() + return e +} + +// Bind records which user the session at source authenticated as. Called for +// every connection the session opens, which doubles as a liveness ping. +func (r *Registry) Bind(user string, source string) { + if source == "" { + return + } + r.access.Lock() + defer r.access.Unlock() + e := r.load(source) + if user != "" { + e.user = user + } + r.sweepLocked(e.lastSeen) +} + +// sweepLocked drops entries nothing will come back for. The QUIC inbounds only +// ever Bind -- a QUIC session ends without a callback this package could hang +// Untrack on -- so without this, sources would grow with every session the +// listener has ever seen. Riding on Bind keeps it to one walk per idleTimeout +// and needs no cron job of its own. +func (r *Registry) sweepLocked(now time.Time) { + if now.Sub(r.lastSweep) < idleTimeout { + return + } + r.lastSweep = now + for source, e := range r.sources { + if idleLost(e, now) { + delete(r.sources, source) + delete(r.blocked, source) + } + } + for source, b := range r.blocked { + if now.Sub(b.at) > blockTimeout { + delete(r.blocked, source) + } + } +} + +// idleLost reports whether an entry is one nothing will come back for. +// +// A tracked session is never that, however long it has been quiet: lastSeen +// only moves when the session opens another connection, so one carrying a +// single long-lived stream -- an ssh session, a download, a long poll -- looks +// idle here while it is perfectly alive. Dropping it would discard the closer, +// which is the only handle on it there is, and the next removal would then find +// nothing to cut and let the session run on. Tracked entries are cleaned up by +// their own Untrack instead, which the inbound defers for exactly that. +func idleLost(e *entry, now time.Time) bool { + return e.closer == nil && now.Sub(e.lastSeen) > idleTimeout +} + +// BindAndTrack records the user and the session transport in one go, for a +// carrier that arrives with both already known. +// +// The two must land under a single lock. A CloseUsers that ran in between -- +// seeing the user but not yet the closer -- would file the session as one it +// cannot close and mute it instead, and a mux carrier is never gated, so the +// mute would do nothing at all while the session kept being served. +func (r *Registry) BindAndTrack(user string, source string, closer io.Closer) { + if source == "" { + return + } + r.access.Lock() + defer r.access.Unlock() + e := r.load(source) + if user != "" { + e.user = user + } + if closer != nil { + e.closer = closer + } + r.sweepLocked(e.lastSeen) +} + +// Track stores the session transport, for protocols whose session has a closer +// of its own. Without one the session can only be muted, not closed. +func (r *Registry) Track(source string, closer io.Closer) { + if source == "" { + return + } + r.access.Lock() + defer r.access.Unlock() + r.load(source).closer = closer +} + +func (r *Registry) Untrack(source string) { + if source == "" { + return + } + r.access.Lock() + defer r.access.Unlock() + delete(r.sources, source) + delete(r.blocked, source) +} + +// Allowed reports whether connections from source may still be routed. A +// session that cannot be closed is muted here instead: nothing it opens is +// routed any more, so the removed user's traffic stops. +func (r *Registry) Allowed(source string) bool { + if source == "" { + return true + } + r.access.Lock() + defer r.access.Unlock() + b, blocked := r.blocked[source] + if !blocked { + return true + } + now := time.Now() + if now.Sub(b.at) > blockTimeout { + delete(r.blocked, source) + return true + } + // A gap this long means the muted session gave up; whatever is connecting + // now is a new one. Only for a kick: a removed user stays muted until the + // backstop, because there is nothing to let back in. + if b.kick && now.Sub(b.lastAttempt) > kickQuietWindow { + delete(r.blocked, source) + return true + } + b.lastAttempt = now + return false +} + +// CloseUsers cuts the sessions of every user not in keep and lifts the block on +// the sessions of users that are in keep, so re-enabling a user takes effect +// without waiting for their session to die. Returns the number of sessions cut. +func (r *Registry) CloseUsers(keep map[string]struct{}) int { + now := time.Now() + + r.access.Lock() + var closers []io.Closer + cut := 0 + for source, e := range r.sources { + if idleLost(e, now) { + delete(r.sources, source) + delete(r.blocked, source) + continue + } + if e.user == "" { + continue + } + if _, ok := keep[e.user]; ok { + delete(r.blocked, source) + continue + } + cut++ + if e.closer != nil { + closers = append(closers, e.closer) + delete(r.sources, source) + delete(r.blocked, source) + continue + } + r.blocked[source] = &block{at: now, lastAttempt: now} + } + for source, b := range r.blocked { + if now.Sub(b.at) > blockTimeout { + delete(r.blocked, source) + } + } + r.lastSweep = now + r.access.Unlock() + + // Outside the lock: closing a tracked session runs the inbound's own close + // handler, which comes back through Untrack. + for _, closer := range closers { + _ = closer.Close() + } + return cut +} + +// KickUserSessions disconnects a user who is still enabled. A session with a +// closer is cut outright; one without is muted, which is the only way to stop a +// QUIC session that the protocol gives us no handle on. The mute lifts as soon +// as that session stops trying, so the client reconnects on its own. +func (r *Registry) KickUserSessions(user string) int { + if user == "" { + return 0 + } + now := time.Now() + + r.access.Lock() + var closers []io.Closer + kicked := 0 + for source, e := range r.sources { + if e.user != user { + continue + } + kicked++ + if e.closer != nil { + delete(r.sources, source) + delete(r.blocked, source) + closers = append(closers, e.closer) + continue + } + r.blocked[source] = &block{at: now, lastAttempt: now, kick: true} + } + r.access.Unlock() + + for _, closer := range closers { + _ = closer.Close() + } + return kicked +} + +// KeepSet turns the user-name list an inbound just installed into the set +// CloseUsers takes. The names an in-place update is given are exactly the +// clients still enabled on that inbound, so nothing else has to be looked up. +func KeepSet(names []string) map[string]struct{} { + keep := make(map[string]struct{}, len(names)) + for _, name := range names { + keep[name] = struct{}{} + } + return keep +} + +// ErrRemoved is reported to the close handler of a connection that a muted +// session tried to open. +var ErrRemoved = E.New("user removed from inbound") + +// Reject drops a connection opened by a session whose user is gone. +func Reject(conn io.Closer, onClose N.CloseHandlerFunc) { + common.Close(conn) + if onClose != nil { + onClose(ErrRemoved) + } +} diff --git a/core/usersession/registry_test.go b/core/usersession/registry_test.go new file mode 100644 index 00000000..ab1b9fbd --- /dev/null +++ b/core/usersession/registry_test.go @@ -0,0 +1,327 @@ +package usersession + +import ( + "errors" + "testing" + "time" +) + +type fakeCloser struct { + closed bool +} + +func (c *fakeCloser) Close() error { + c.closed = true + return nil +} + +func keep(users ...string) map[string]struct{} { + set := make(map[string]struct{}, len(users)) + for _, user := range users { + set[user] = struct{}{} + } + return set +} + +func TestCloseUsersClosesTrackedSession(t *testing.T) { + r := NewRegistry() + conn := &fakeCloser{} + r.Track("1.2.3.4:1000", conn) + r.Bind("gone", "1.2.3.4:1000") + + if cut := r.CloseUsers(keep("stays")); cut != 1 { + t.Fatalf("cut = %d, want 1", cut) + } + if !conn.closed { + t.Error("a tracked session must be closed, not muted") + } + // Nothing is left muted: the session is gone, so the address is free for + // whoever connects from it next. + if !r.Allowed("1.2.3.4:1000") { + t.Error("address stayed muted after its session was closed") + } +} + +func TestCloseUsersMutesUntrackedSession(t *testing.T) { + r := NewRegistry() + // No Track: this is the QUIC shape, where the session has no closer we can + // reach. Muting is the only thing left. + r.Bind("gone", "1.2.3.4:1000") + + if cut := r.CloseUsers(keep("stays")); cut != 1 { + t.Fatalf("cut = %d, want 1", cut) + } + if r.Allowed("1.2.3.4:1000") { + t.Error("a session with no closer must be muted") + } + // An address nobody authenticated from is not affected. + if !r.Allowed("5.6.7.8:2000") { + t.Error("an unrelated address must stay allowed") + } +} + +func TestCloseUsersLeavesUnauthenticatedSourceAlone(t *testing.T) { + r := NewRegistry() + // Bound with no user name: the connection reached the handler before + // authentication produced one. It belongs to nobody, so a removal has + // nothing to say about it. + r.Bind("", "1.2.3.4:1000") + + if cut := r.CloseUsers(keep("stays")); cut != 0 { + t.Fatalf("cut = %d, want 0", cut) + } + if !r.Allowed("1.2.3.4:1000") { + t.Error("an unauthenticated source must not be muted") + } +} + +func TestCloseUsersLiftsMuteWhenUserComesBack(t *testing.T) { + r := NewRegistry() + r.Bind("flip", "1.2.3.4:1000") + r.CloseUsers(keep()) + if r.Allowed("1.2.3.4:1000") { + t.Fatal("precondition: the session should be muted") + } + + // Re-enabling has to take effect now, not when the block times out. + r.CloseUsers(keep("flip")) + if !r.Allowed("1.2.3.4:1000") { + t.Error("re-enabling a user must lift the mute immediately") + } +} + +func TestKickClosesTrackedSession(t *testing.T) { + r := NewRegistry() + conn := &fakeCloser{} + r.Track("1.2.3.4:1000", conn) + r.Bind("noisy", "1.2.3.4:1000") + + if kicked := r.KickUserSessions("noisy"); kicked != 1 { + t.Fatalf("kicked = %d, want 1", kicked) + } + if !conn.closed { + t.Error("a tracked session must be closed on kick") + } + if !r.Allowed("1.2.3.4:1000") { + t.Error("a kicked client must be free to reconnect at once") + } +} + +func TestKickMuteLiftsAfterQuietWindow(t *testing.T) { + r := NewRegistry() + r.Bind("noisy", "1.2.3.4:1000") + r.KickUserSessions("noisy") + + // The kicked session keeps trying and keeps being refused. + for i := 0; i < 3; i++ { + if r.Allowed("1.2.3.4:1000") { + t.Fatalf("attempt %d was let through while the session kept trying", i) + } + } + + // Once it has been quiet for the window, the next attempt is a new session. + r.access.Lock() + r.blocked["1.2.3.4:1000"].lastAttempt = time.Now().Add(-kickQuietWindow - time.Second) + r.access.Unlock() + + if !r.Allowed("1.2.3.4:1000") { + t.Error("a kick must not turn into a lockout") + } +} + +func TestRemovalMuteDoesNotLiftOnQuiet(t *testing.T) { + r := NewRegistry() + r.Bind("gone", "1.2.3.4:1000") + r.CloseUsers(keep()) + + // Same quiet gap that would lift a kick. A removal is not a kick: the user + // is no longer on the inbound, so there is nothing to let back in and the + // mute holds until the backstop. + r.access.Lock() + r.blocked["1.2.3.4:1000"].lastAttempt = time.Now().Add(-kickQuietWindow - time.Second) + r.access.Unlock() + + if r.Allowed("1.2.3.4:1000") { + t.Error("a removal mute must not lift just because the session went quiet") + } +} + +func TestRemovalMuteLiftsAtBackstop(t *testing.T) { + r := NewRegistry() + r.Bind("gone", "1.2.3.4:1000") + r.CloseUsers(keep()) + + r.access.Lock() + r.blocked["1.2.3.4:1000"].at = time.Now().Add(-blockTimeout - time.Second) + r.access.Unlock() + + if !r.Allowed("1.2.3.4:1000") { + t.Error("the backstop must free an address whose session is long dead") + } +} + +func TestUntrackForgetsSession(t *testing.T) { + r := NewRegistry() + conn := &fakeCloser{} + r.Track("1.2.3.4:1000", conn) + r.Bind("gone", "1.2.3.4:1000") + r.Untrack("1.2.3.4:1000") + + if cut := r.CloseUsers(keep()); cut != 0 { + t.Fatalf("cut = %d, want 0 -- the session was already gone", cut) + } + if conn.closed { + t.Error("an untracked session must not be closed again") + } +} + +// The QUIC inbounds never Untrack, so Bind is the only place that can drop what +// they leave behind. +func TestSweepDropsIdleSources(t *testing.T) { + r := NewRegistry() + r.Bind("old", "1.2.3.4:1000") + + r.access.Lock() + r.sources["1.2.3.4:1000"].lastSeen = time.Now().Add(-idleTimeout - time.Minute) + r.lastSweep = time.Now().Add(-idleTimeout - time.Minute) + r.access.Unlock() + + r.Bind("fresh", "5.6.7.8:2000") + + r.access.Lock() + _, stale := r.sources["1.2.3.4:1000"] + count := len(r.sources) + r.access.Unlock() + + if stale { + t.Error("an idle source must be swept") + } + if count != 1 { + t.Errorf("len(sources) = %d, want 1 (the fresh one)", count) + } +} + +func TestSweepIsRateLimited(t *testing.T) { + r := NewRegistry() + r.Bind("old", "1.2.3.4:1000") + + r.access.Lock() + r.sources["1.2.3.4:1000"].lastSeen = time.Now().Add(-idleTimeout - time.Minute) + r.access.Unlock() + + // lastSweep is fresh (NewRegistry set it), so this Bind must not walk the + // map -- the whole point is that the data plane does not pay per call. + r.Bind("fresh", "5.6.7.8:2000") + + r.access.Lock() + _, stale := r.sources["1.2.3.4:1000"] + r.access.Unlock() + + if !stale { + t.Error("sweep ran on a Bind inside the rate limit window") + } +} + +func TestRejectReportsClose(t *testing.T) { + conn := &fakeCloser{} + var reported error + Reject(conn, func(err error) { reported = err }) + + if !conn.closed { + t.Error("Reject must close the connection") + } + if !errors.Is(reported, ErrRemoved) { + t.Errorf("close handler got %v, want ErrRemoved", reported) + } +} + +func TestRejectWithoutCloseHandler(t *testing.T) { + conn := &fakeCloser{} + Reject(conn, nil) + if !conn.closed { + t.Error("Reject must close the connection even with no close handler") + } +} + +// A tracked session is alive however quiet it has been: lastSeen only moves +// when it opens another connection, so one carrying a single long-lived stream +// looks idle. Sweeping it would discard the closer, and the removal that came +// next would find nothing to cut -- which is issue #175 all over again. +func TestSweepKeepsTrackedSession(t *testing.T) { + r := NewRegistry() + conn := &fakeCloser{} + r.BindAndTrack("quiet", "1.2.3.4:1000", conn) + + r.access.Lock() + r.sources["1.2.3.4:1000"].lastSeen = time.Now().Add(-idleTimeout - time.Minute) + r.lastSweep = time.Now().Add(-idleTimeout - time.Minute) + r.access.Unlock() + + // Another client's traffic, which is what triggers the sweep. + r.Bind("other", "5.6.7.8:2000") + + if cut := r.CloseUsers(keep("other")); cut != 1 { + t.Fatalf("cut = %d, want 1 -- the idle session was swept away", cut) + } + if !conn.closed { + t.Error("a tracked session must survive the sweep and stay closable") + } +} + +// Same defect, second site: CloseUsers drops idle entries before it decides +// what to cut, so an idle carrier would be skipped by the very call meant to +// close it. +func TestCloseUsersCutsIdleTrackedSession(t *testing.T) { + r := NewRegistry() + conn := &fakeCloser{} + r.BindAndTrack("quiet", "1.2.3.4:1000", conn) + + r.access.Lock() + r.sources["1.2.3.4:1000"].lastSeen = time.Now().Add(-idleTimeout - time.Minute) + r.access.Unlock() + + if cut := r.CloseUsers(keep()); cut != 1 { + t.Fatalf("cut = %d, want 1 -- an idle tracked session was skipped", cut) + } + if !conn.closed { + t.Error("an idle but tracked session must still be closed") + } +} + +// This pins the outcome -- a carrier registered this way is closed, not muted. +// The atomicity it exists for cannot be asserted here: it comes from doing both +// writes under one lock, and a sequential Bind-then-Track would pass this test +// just the same. What that split would lose is the interleaving where a +// CloseUsers lands between the two, sees a user with no closer yet, files the +// session as unclosable and mutes it -- and a mux carrier is never gated, so +// the mute would do nothing. +func TestBindAndTrackClosesRatherThanMutes(t *testing.T) { + r := NewRegistry() + conn := &fakeCloser{} + r.BindAndTrack("live", "1.2.3.4:1000", conn) + + if cut := r.CloseUsers(keep()); cut != 1 { + t.Fatalf("cut = %d, want 1", cut) + } + if !conn.closed { + t.Error("the carrier must be closed, not muted") + } + if !r.Allowed("1.2.3.4:1000") { + t.Error("a closed carrier must not leave a mute behind") + } +} + +// A packet conn cannot be a mux carrier, so the router passes a nil closer. It +// must stay nil: a nil net.Conn placed in an io.Closer is not a nil interface, +// and would register as a closer that panics when the session is cut. +func TestBindAndTrackWithNilCloserMutes(t *testing.T) { + r := NewRegistry() + r.BindAndTrack("live", "1.2.3.4:1000", nil) + + if cut := r.CloseUsers(keep()); cut != 1 { + t.Fatalf("cut = %d, want 1", cut) + } + if r.Allowed("1.2.3.4:1000") { + t.Error("with no closer the session must be muted") + } +} diff --git a/core/usersession/router.go b/core/usersession/router.go new file mode 100644 index 00000000..6765ef12 --- /dev/null +++ b/core/usersession/router.go @@ -0,0 +1,219 @@ +package usersession + +import ( + "context" + "io" + "net" + + "github.com/sagernet/sing-box/adapter" + N "github.com/sagernet/sing/common/network" + + singmux "github.com/sagernet/sing-mux" +) + +// The registry is reached by wrapping an inbound's router rather than by giving +// each inbound a field and a hook in every handler. Both put the gate in the +// same place -- before anything is routed, so the connections the router +// answers itself (hijack-dns) are covered and nothing is dialed before the +// refusal -- but the copies under core/protocol/ are diffed against sing-box +// line for line by scripts/check-protocol-copies.sh, and this way each one +// carries a single added line instead of a block in every handler. +// +// Every copy assigns metadata.User before calling the router, so the user a +// connection authenticated as is already known here. + +// Mode is what a wrapped router does with the connections it sees. Which one an +// inbound wants follows from how its protocol reuses an authenticated +// connection. +type Mode int + +const ( + // GateAndBind refuses connections from a muted session and records the user + // behind every other one. For the QUIC protocols the session itself has no + // closer this package can reach, so muting is the only thing that stops a + // removed user, and the gate is what does the work. + GateAndBind Mode = iota + // TrackMuxCarrier ignores everything but a sing-mux carrier connection. + // These protocols authenticate per connection, so a removed user is already + // locked out of new ones and ConnTracker closes the routed ones -- except + // on a multiplex session, where the carrier authenticated once and every + // stream after that rides it. The carrier is a net.Conn we can close, so it + // is tracked and cut rather than muted. + TrackMuxCarrier + // BindOnly records the user and nothing else, for an inbound that reaches + // its session by another route. anytls holds its session in NewConnection, + // which never goes through the router. + BindOnly +) + +type hooks struct { + registry *Registry + mode Mode +} + +// enter applies the mode to one connection on its way to the real router. It +// reports whether the connection may proceed, and returns the cleanup to run +// once the router is done with it. +func (h hooks) enter(conn io.Closer, metadata adapter.InboundContext, onClose N.CloseHandlerFunc) (bool, func()) { + source := metadata.Source.String() + switch h.mode { + case TrackMuxCarrier: + if metadata.Destination != singmux.Destination { + return true, nil + } + // Only a stream-oriented carrier can be closed; a packet conn reaching + // here would not be a mux carrier anyway. Assigned through a nil-able + // io.Closer rather than passed directly: a nil net.Conn put into an + // interface is not a nil interface, and would register as a closer that + // panics when the session is cut. + var carrier io.Closer + if streamConn, ok := conn.(net.Conn); ok { + carrier = streamConn + } + // One call, one lock: see BindAndTrack for why these cannot be split. + h.registry.BindAndTrack(metadata.User, source, carrier) + // The router blocks for as long as the multiplex session lives, so + // untracking when it returns is not early. + return true, func() { h.registry.Untrack(source) } + case BindOnly: + h.registry.Bind(metadata.User, source) + return true, nil + default: + if !h.registry.Allowed(source) { + Reject(conn, onClose) + return false, nil + } + h.registry.Bind(metadata.User, source) + return true, nil + } +} + +// RouterEx wraps an inbound whose router field is an adapter.ConnectionRouterEx. +type RouterEx struct { + adapter.ConnectionRouterEx + hooks +} + +// WrapRouterEx returns router with the session hooks in front of it. +func WrapRouterEx(router adapter.ConnectionRouterEx, mode Mode) *RouterEx { + return &RouterEx{ + ConnectionRouterEx: router, + hooks: hooks{registry: NewRegistry(), mode: mode}, + } +} + +// Registry is how the owning inbound reaches the registry it just installed. +func (r *RouterEx) Registry() *Registry { + return r.registry +} + +func (r *RouterEx) RouteConnectionEx(ctx context.Context, conn net.Conn, metadata adapter.InboundContext, onClose N.CloseHandlerFunc) { + proceed, done := r.enter(conn, metadata, onClose) + if !proceed { + return + } + if done != nil { + defer done() + } + r.ConnectionRouterEx.RouteConnectionEx(ctx, conn, metadata, onClose) +} + +func (r *RouterEx) RoutePacketConnectionEx(ctx context.Context, conn N.PacketConn, metadata adapter.InboundContext, onClose N.CloseHandlerFunc) { + proceed, done := r.enter(conn, metadata, onClose) + if !proceed { + return + } + if done != nil { + defer done() + } + r.ConnectionRouterEx.RoutePacketConnectionEx(ctx, conn, metadata, onClose) +} + +// The deprecated pair is overridden too, not for completeness: shadowsocks' +// MultiInbound still routes through it, and leaving it to the embedded router +// would let those connections past the hooks without a word. +func (r *RouterEx) RouteConnection(ctx context.Context, conn net.Conn, metadata adapter.InboundContext) error { + proceed, done := r.enter(conn, metadata, nil) + if !proceed { + return ErrRemoved + } + if done != nil { + defer done() + } + return r.ConnectionRouterEx.RouteConnection(ctx, conn, metadata) +} + +func (r *RouterEx) RoutePacketConnection(ctx context.Context, conn N.PacketConn, metadata adapter.InboundContext) error { + proceed, done := r.enter(conn, metadata, nil) + if !proceed { + return ErrRemoved + } + if done != nil { + defer done() + } + return r.ConnectionRouterEx.RoutePacketConnection(ctx, conn, metadata) +} + +// Router wraps an inbound whose router field is the full adapter.Router, which +// hysteria and hysteria2 hold. Everything it does not override is forwarded by +// the embedded interface. +type Router struct { + adapter.Router + hooks +} + +// WrapRouter returns router with the session hooks in front of it. +func WrapRouter(router adapter.Router, mode Mode) *Router { + return &Router{ + Router: router, + hooks: hooks{registry: NewRegistry(), mode: mode}, + } +} + +func (r *Router) Registry() *Registry { + return r.registry +} + +func (r *Router) RouteConnectionEx(ctx context.Context, conn net.Conn, metadata adapter.InboundContext, onClose N.CloseHandlerFunc) { + proceed, done := r.enter(conn, metadata, onClose) + if !proceed { + return + } + if done != nil { + defer done() + } + r.Router.RouteConnectionEx(ctx, conn, metadata, onClose) +} + +func (r *Router) RoutePacketConnectionEx(ctx context.Context, conn N.PacketConn, metadata adapter.InboundContext, onClose N.CloseHandlerFunc) { + proceed, done := r.enter(conn, metadata, onClose) + if !proceed { + return + } + if done != nil { + defer done() + } + r.Router.RoutePacketConnectionEx(ctx, conn, metadata, onClose) +} + +func (r *Router) RouteConnection(ctx context.Context, conn net.Conn, metadata adapter.InboundContext) error { + proceed, done := r.enter(conn, metadata, nil) + if !proceed { + return ErrRemoved + } + if done != nil { + defer done() + } + return r.Router.RouteConnection(ctx, conn, metadata) +} + +func (r *Router) RoutePacketConnection(ctx context.Context, conn N.PacketConn, metadata adapter.InboundContext) error { + proceed, done := r.enter(conn, metadata, nil) + if !proceed { + return ErrRemoved + } + if done != nil { + defer done() + } + return r.Router.RoutePacketConnection(ctx, conn, metadata) +} diff --git a/core/usersession/router_test.go b/core/usersession/router_test.go new file mode 100644 index 00000000..48a5e4cf --- /dev/null +++ b/core/usersession/router_test.go @@ -0,0 +1,228 @@ +package usersession + +import ( + "context" + "net" + "os" + "testing" + "time" + + "github.com/sagernet/sing-box/adapter" + M "github.com/sagernet/sing/common/metadata" + N "github.com/sagernet/sing/common/network" + + singmux "github.com/sagernet/sing-mux" +) + +// fakeRouter is the next hop. It records what reached it and, while it is +// "routing", lets a test look at the registry -- which is how the carrier's +// lifetime is asserted: a mux carrier must be tracked for exactly as long as +// the router call lasts. +type fakeRouter struct { + calls int + last adapter.InboundContext + whileIn func() +} + +func (r *fakeRouter) enter(metadata adapter.InboundContext) { + r.calls++ + r.last = metadata + if r.whileIn != nil { + r.whileIn() + } +} + +func (r *fakeRouter) RouteConnection(ctx context.Context, conn net.Conn, metadata adapter.InboundContext) error { + r.enter(metadata) + return nil +} + +func (r *fakeRouter) RoutePacketConnection(ctx context.Context, conn N.PacketConn, metadata adapter.InboundContext) error { + r.enter(metadata) + return nil +} + +func (r *fakeRouter) RouteConnectionEx(ctx context.Context, conn net.Conn, metadata adapter.InboundContext, onClose N.CloseHandlerFunc) { + r.enter(metadata) +} + +func (r *fakeRouter) RoutePacketConnectionEx(ctx context.Context, conn N.PacketConn, metadata adapter.InboundContext, onClose N.CloseHandlerFunc) { + r.enter(metadata) +} + +func metadataFor(user string, source string, destination M.Socksaddr) adapter.InboundContext { + return adapter.InboundContext{ + User: user, + Source: M.ParseSocksaddr(source), + Destination: destination, + } +} + +var plainDestination = M.ParseSocksaddr("example.com:443") + +// closed reports whether one end of a pipe was closed, by reading from the +// other end. The deadline is what makes "still open" answerable at all: a pipe +// nobody closed would otherwise block this read forever. +func closed(t *testing.T, peer net.Conn) bool { + t.Helper() + // net.Pipe refuses a deadline once either end is closed, so this failing is + // itself the answer. + if err := peer.SetReadDeadline(time.Now().Add(200 * time.Millisecond)); err != nil { + return true + } + buf := make([]byte, 1) + _, err := peer.Read(buf) + if err == nil || os.IsTimeout(err) { + return false + } + return true +} + +func TestGateRefusesMutedSource(t *testing.T) { + next := &fakeRouter{} + router := WrapRouterEx(next, GateAndBind) + metadata := metadataFor("gone", "1.2.3.4:1000", plainDestination) + + // Learn the user, then remove them: with no closer, this mutes the source. + router.Registry().Bind("gone", "1.2.3.4:1000") + router.Registry().CloseUsers(KeepSet(nil)) + + conn, peer := net.Pipe() + defer peer.Close() + var reported error + router.RouteConnectionEx(context.Background(), conn, metadata, func(err error) { reported = err }) + + if next.calls != 0 { + t.Error("a muted source must not reach the router") + } + if !closed(t, peer) { + t.Error("the refused connection must be closed") + } + if reported != ErrRemoved { + t.Errorf("close handler got %v, want ErrRemoved", reported) + } +} + +func TestGateBindsAndForwards(t *testing.T) { + next := &fakeRouter{} + router := WrapRouterEx(next, GateAndBind) + + conn, peer := net.Pipe() + defer conn.Close() + defer peer.Close() + router.RouteConnectionEx(context.Background(), conn, metadataFor("live", "1.2.3.4:1000", plainDestination), nil) + + if next.calls != 1 { + t.Fatalf("router calls = %d, want 1", next.calls) + } + // The bind is what makes the next removal able to find this session. + if cut := router.Registry().CloseUsers(KeepSet(nil)); cut != 1 { + t.Errorf("cut = %d, want 1 -- the connection was not bound to its user", cut) + } +} + +// The deprecated pair is what shadowsocks' MultiInbound still routes through. +// Leaving it to the embedded router would let those connections past the hooks +// without a word, so it gets the same coverage as the Ex form. +func TestGateCoversDeprecatedRoutePath(t *testing.T) { + next := &fakeRouter{} + router := WrapRouterEx(next, GateAndBind) + router.Registry().Bind("gone", "1.2.3.4:1000") + router.Registry().CloseUsers(KeepSet(nil)) + + conn, peer := net.Pipe() + defer peer.Close() + err := router.RouteConnection(context.Background(), conn, metadataFor("gone", "1.2.3.4:1000", plainDestination)) + + if next.calls != 0 { + t.Error("a muted source must not reach the router on the deprecated path either") + } + if err != ErrRemoved { + t.Errorf("RouteConnection returned %v, want ErrRemoved", err) + } +} + +func TestMuxCarrierIsTrackedForTheRoutersLifetime(t *testing.T) { + var trackedDuring bool + next := &fakeRouter{} + router := WrapRouterEx(next, TrackMuxCarrier) + next.whileIn = func() { + // The router blocks for as long as the multiplex session lives, so the + // carrier has to be closable right here -- that is the whole point. + trackedDuring = router.Registry().KickUserSessions("live") == 1 + } + + conn, peer := net.Pipe() + defer peer.Close() + router.RouteConnectionEx(context.Background(), conn, metadataFor("live", "1.2.3.4:1000", singmux.Destination), nil) + + if !trackedDuring { + t.Error("the mux carrier was not closable while the router had it") + } + if !closed(t, peer) { + t.Error("kicking the user must have closed the carrier") + } + // And it is forgotten once the session is over. + if kicked := router.Registry().KickUserSessions("live"); kicked != 0 { + t.Errorf("kicked = %d after the session ended, want 0", kicked) + } +} + +func TestMuxModeIgnoresPlainConnections(t *testing.T) { + next := &fakeRouter{} + router := WrapRouterEx(next, TrackMuxCarrier) + + conn, peer := net.Pipe() + defer conn.Close() + defer peer.Close() + router.RouteConnectionEx(context.Background(), conn, metadataFor("live", "1.2.3.4:1000", plainDestination), nil) + + if next.calls != 1 { + t.Fatalf("router calls = %d, want 1", next.calls) + } + // A plain connection authenticates on its own and ConnTracker closes it, so + // it has no business in the registry. + if cut := router.Registry().CloseUsers(KeepSet(nil)); cut != 0 { + t.Errorf("cut = %d, want 0 -- a plain connection was registered", cut) + } +} + +// trojan routes unauthenticated fallback traffic through this same router, so +// this mode must never refuse anything. +func TestMuxModeDoesNotGate(t *testing.T) { + next := &fakeRouter{} + router := WrapRouterEx(next, TrackMuxCarrier) + router.Registry().Bind("gone", "1.2.3.4:1000") + router.Registry().CloseUsers(KeepSet(nil)) + + conn, peer := net.Pipe() + defer conn.Close() + defer peer.Close() + router.RouteConnectionEx(context.Background(), conn, metadataFor("", "1.2.3.4:1000", plainDestination), nil) + + if next.calls != 1 { + t.Error("the mux mode must not refuse a connection, fallback traffic shares this router") + } +} + +func TestBindOnlyNeitherGatesNorTracks(t *testing.T) { + next := &fakeRouter{} + router := WrapRouterEx(next, BindOnly) + + conn, peer := net.Pipe() + defer conn.Close() + defer peer.Close() + router.RouteConnectionEx(context.Background(), conn, metadataFor("live", "1.2.3.4:1000", plainDestination), nil) + + if next.calls != 1 { + t.Fatalf("router calls = %d, want 1", next.calls) + } + // Bound, so a removal finds it -- but muted rather than closed, because + // anytls registers the closable session elsewhere. + if cut := router.Registry().CloseUsers(KeepSet(nil)); cut != 1 { + t.Errorf("cut = %d, want 1", cut) + } + if closed(t, peer) { + t.Error("BindOnly must not close the connection it saw") + } +} diff --git a/go.mod b/go.mod index 476d2f45..27317995 100644 --- a/go.mod +++ b/go.mod @@ -17,6 +17,7 @@ require ( github.com/robfig/cron/v3 v3.0.1 github.com/sagernet/sing v0.9.4 github.com/sagernet/sing-box v1.14.1 + github.com/sagernet/sing-mux v0.3.6 github.com/sagernet/sing-quic v0.7.0 github.com/sagernet/sing-tun v0.9.3 github.com/sagernet/sing-vmess v0.2.8 @@ -169,7 +170,6 @@ require ( github.com/sagernet/nftables v0.3.0-mod.4 // indirect github.com/sagernet/quic-go v0.61.0-sing-box-mod.7 // indirect github.com/sagernet/sing-cloudflared v0.1.3-0.20260706062323-d9787e794aa3 // indirect - github.com/sagernet/sing-mux v0.3.6 // indirect github.com/sagernet/sing-openconnect v0.0.0-20260810065514-53aa8058f8df // indirect github.com/sagernet/sing-openvpn v0.0.0-20260729104525-103eb5fe5eb6 // indirect github.com/sagernet/sing-shadowsocks v0.2.8 // indirect diff --git a/scripts/check-protocol-copies.sh b/scripts/check-protocol-copies.sh index f204fe66..fa922a1f 100644 --- a/scripts/check-protocol-copies.sh +++ b/scripts/check-protocol-copies.sh @@ -32,28 +32,39 @@ echo "comparing core/protocol/ against sing-box $VERSION" LOCAL_ONLY="users.go" # Lines each copy is expected to differ by, beyond the shared header comment, -# keyed by "/". +# keyed by "/". These are not a tolerance: each is the exact +# size of changes we made on purpose, so a bump that alters the file anywhere +# else still shows up. Two changes make them up. # -# The six user-carrying protocols key their service by user name rather than by -# list position (Service[string], not Service[int]) -- see the header of any of -# those files. That patch is what these counts are: they are not a tolerance, -# they are the exact size of a change we made on purpose, so a bump that alters -# the file elsewhere still shows up. anytls has no user table and stays verbatim. +# 1. The six user-carrying protocols key their service by user name rather than +# by list position (Service[string], not Service[int]) -- see the header of +# any of those files. vmess also carries 2 lines of import alias, since its +# own package is `vmess`. anytls has no user table and is untouched by this. # -# vmess also carries 2 lines of import alias, since its own package is `vmess`. +# 2. Every copy installs the user-session registry by wrapping its router, which +# is one added line, identical in all seven: # -# Re-copying after a sing-box bump will change these. Re-apply the patch, then -# put the new counts here -- and read the diff first rather than just pasting -# the number the script printed, which is how a real upstream change gets -# rubber-stamped into the expected total. +# inbound.router = withUserSessions(inbound.router) +# +# Everything that line reaches lives in users.go, which is exempt below. The +# hook is a wrapper rather than a field plus a block in each handler exactly +# so that these counts stay this small -- see core/usersession. anytls costs +# 3 instead of 1: its session is held in NewConnection, which the router +# never sees, so that one call is redirected through users.go as well. +# +# Re-copying after a sing-box bump will change these. Re-apply both changes, +# then put the new counts here -- and read the diff first rather than just +# pasting the number the script printed, which is how a real upstream change +# gets rubber-stamped into the expected total. expect_diff() { case "$1" in - hysteria/inbound.go) echo 28 ;; - hysteria2/inbound.go) echo 28 ;; - trojan/inbound.go) echo 25 ;; - tuic/inbound.go) echo 26 ;; - vless/inbound.go) echo 29 ;; - vmess/inbound.go) echo 27 ;; + anytls/inbound.go) echo 3 ;; + hysteria/inbound.go) echo 29 ;; + hysteria2/inbound.go) echo 29 ;; + trojan/inbound.go) echo 26 ;; + tuic/inbound.go) echo 27 ;; + vless/inbound.go) echo 30 ;; + vmess/inbound.go) echo 28 ;; *) echo 0 ;; esac } From 351413096f7861a476c904b6e90fb01bd20cac25 Mon Sep 17 00:00:00 2001 From: ShenJiaqing <48464190+shenaba@users.noreply.github.com> Date: Wed, 16 Sep 2026 14:51:37 +0800 Subject: [PATCH 2/3] fix(core): hold a removed user's mute until their session gives up The backstop was measured from when the block was written, so a mute expired ten minutes later whatever the client was doing. But a client that keeps retrying is a session that is still alive -- and for QUIC, still authenticated, because the streams it opens afterwards never consult the user table again. A client DepleteJob had disabled therefore got its traffic back after ten minutes, on the very session the mute existed to stop. Both windows now run from the last refused attempt, so quiet is what lifts a block, not the clock. The sweep was a second road to the same place. Allowed only ever touched the block, never the entry, so a source being refused once a second looked idle and the sweep dropped its entry -- taking the block with it. Allowed now keeps the entry alive for as long as it is refusing it. A block also no longer changes kind behind the caller's back. A kick is about a user who is still enabled and lifts after thirty seconds of quiet; a removal is not, and must not be downgraded to that by kicking the same name, nor cleared by an unrelated save that happens to list the user in keep. `block.at` has no readers left and is gone. Found by re-reviewing the previous commit. The test meant to cover the backstop aged the block rather than the last attempt -- exactly the distinction that was wrong -- so it passed either way; it now ages the attempt, and a companion test pins the case it was missing. Verified on the test server across thirteen minutes of retries at one per second, spanning the ten-minute mark that used to end the mute: anytls 0 served / 695 refused hy2 0 served / 698 refused vless 0 served / 698 refused The hysteria2 inbound logged 718 arrivals in that window, so the session was alive throughout and being refused -- not quietly dead, which would have proved nothing. --- core/usersession/registry.go | 61 +++++++++++++++++------- core/usersession/registry_test.go | 78 +++++++++++++++++++++++++++++-- 2 files changed, 118 insertions(+), 21 deletions(-) diff --git a/core/usersession/registry.go b/core/usersession/registry.go index fdc1f94c..3f2cabc1 100644 --- a/core/usersession/registry.go +++ b/core/usersession/registry.go @@ -31,8 +31,10 @@ const ( // A source that has not opened a connection for this long is forgotten; // its session is either gone or idle enough to be re-learned on use. idleTimeout = 10 * time.Minute - // Backstop for blocks: a client whose session is really dead stops being - // blocked, so the address is reusable even if the user is never re-added. + // How long a removed user's session has to stay quiet before its address is + // let go. Measured from the last refused attempt, not from when the block + // was written: a client that keeps trying is a session that is still alive, + // and it stays muted for as long as that lasts. blockTimeout = 10 * time.Minute // A kicked session is muted only while it keeps trying. Once it has been // quiet this long, the next attempt from the address is a new session and @@ -46,11 +48,10 @@ type entry struct { closer io.Closer } -// block mutes one client address. A removal keeps it muted until the backstop; -// a kick, which must not lock a still-enabled user out, lifts as soon as the -// muted session stops trying. +// block mutes one client address. Both kinds lift on quiet rather than on a +// timer (see Allowed) -- a kick far sooner, because the user behind it is +// still enabled and must not be locked out. type block struct { - at time.Time lastAttempt time.Time kick bool } @@ -121,7 +122,7 @@ func (r *Registry) sweepLocked(now time.Time) { } } for source, b := range r.blocked { - if now.Sub(b.at) > blockTimeout { + if now.Sub(b.lastAttempt) > blockTimeout { delete(r.blocked, source) } } @@ -198,18 +199,30 @@ func (r *Registry) Allowed(source string) bool { return true } now := time.Now() - if now.Sub(b.at) > blockTimeout { - delete(r.blocked, source) - return true + // Both windows are measured from the last attempt, never from when the + // block was written. A client that keeps hammering is a session that is + // still alive, and a session that is still alive is still authenticated: + // letting it back in on a timer would hand a removed user their traffic + // back, because the streams it opens afterwards never consult the user + // table again. Quiet is the only evidence that a session is really gone. + // + // A kick lifts quickly because the user is still enabled and must not be + // locked out; a removal holds until the session gives up for good. + window := blockTimeout + if b.kick { + window = kickQuietWindow } - // A gap this long means the muted session gave up; whatever is connecting - // now is a new one. Only for a kick: a removed user stays muted until the - // backstop, because there is nothing to let back in. - if b.kick && now.Sub(b.lastAttempt) > kickQuietWindow { + if now.Sub(b.lastAttempt) > window { delete(r.blocked, source) return true } b.lastAttempt = now + // A source being refused is anything but idle. Without this the sweep + // would see a stale lastSeen, drop the entry and take the block with it -- + // lifting the mute by the other road. + if e, ok := r.sources[source]; ok { + e.lastSeen = now + } return false } @@ -232,7 +245,12 @@ func (r *Registry) CloseUsers(keep map[string]struct{}) int { continue } if _, ok := keep[e.user]; ok { - delete(r.blocked, source) + // Lift a removal: the user is back on the inbound. A kick is a + // separate decision an operator just made about a user who was + // never removed, so an unrelated save must not undo it. + if b, muted := r.blocked[source]; muted && !b.kick { + delete(r.blocked, source) + } continue } cut++ @@ -242,10 +260,10 @@ func (r *Registry) CloseUsers(keep map[string]struct{}) int { delete(r.blocked, source) continue } - r.blocked[source] = &block{at: now, lastAttempt: now} + r.blocked[source] = &block{lastAttempt: now} } for source, b := range r.blocked { - if now.Sub(b.at) > blockTimeout { + if now.Sub(b.lastAttempt) > blockTimeout { delete(r.blocked, source) } } @@ -284,7 +302,14 @@ func (r *Registry) KickUserSessions(user string) int { closers = append(closers, e.closer) continue } - r.blocked[source] = &block{at: now, lastAttempt: now, kick: true} + // A removal already standing is the stricter of the two and must not be + // downgraded: that user is gone from the inbound, while a kick assumes + // they are still entitled to reconnect once they stop hammering. + if existing, muted := r.blocked[source]; muted && !existing.kick { + existing.lastAttempt = now + continue + } + r.blocked[source] = &block{lastAttempt: now, kick: true} } r.access.Unlock() diff --git a/core/usersession/registry_test.go b/core/usersession/registry_test.go index ab1b9fbd..8c765437 100644 --- a/core/usersession/registry_test.go +++ b/core/usersession/registry_test.go @@ -146,17 +146,21 @@ func TestRemovalMuteDoesNotLiftOnQuiet(t *testing.T) { } } -func TestRemovalMuteLiftsAtBackstop(t *testing.T) { +// "Long dead" means the session stopped trying -- not merely that the block was +// written a while ago. Ageing the block itself was what the earlier version of +// this test did, and it let a real defect through: a client hammering away for +// ten minutes got its traffic back on the timer. +func TestRemovalMuteLiftsWhenSessionGivesUp(t *testing.T) { r := NewRegistry() r.Bind("gone", "1.2.3.4:1000") r.CloseUsers(keep()) r.access.Lock() - r.blocked["1.2.3.4:1000"].at = time.Now().Add(-blockTimeout - time.Second) + r.blocked["1.2.3.4:1000"].lastAttempt = time.Now().Add(-blockTimeout - time.Second) r.access.Unlock() if !r.Allowed("1.2.3.4:1000") { - t.Error("the backstop must free an address whose session is long dead") + t.Error("an address whose session gave up must eventually be let go") } } @@ -325,3 +329,71 @@ func TestBindAndTrackWithNilCloserMutes(t *testing.T) { t.Error("with no closer the session must be muted") } } + +// The counterpart to the one above, and the defect it was hiding: a client that +// keeps retrying is a session that is still alive -- which for QUIC means still +// authenticated, because the streams it opens never re-check the user table. +// The mute must outlast the retries, however long they go on. +// +// The sweep is the second road to the same place: Allowed has to keep the entry +// looking live, or the sweep drops it and takes the block with it. +func TestRemovalMuteSurvivesTheSweepWhileClientRetries(t *testing.T) { + r := NewRegistry() + r.Bind("gone", "1.2.3.4:1000") + r.CloseUsers(keep()) + + // Age the entry as though it had gone quiet long ago. + r.access.Lock() + r.sources["1.2.3.4:1000"].lastSeen = time.Now().Add(-idleTimeout - time.Minute) + r.lastSweep = time.Now().Add(-idleTimeout - time.Minute) + r.access.Unlock() + + // But the client is in fact still hammering: one refused attempt is enough + // to mark the source live again. + if r.Allowed("1.2.3.4:1000") { + t.Fatal("precondition: the source should still be muted") + } + + // Someone else's traffic, which is what triggers the sweep. + r.Bind("other", "5.6.7.8:2000") + + if r.Allowed("1.2.3.4:1000") { + t.Error("the sweep lifted a mute whose client is still trying") + } +} + +// A kick assumes the user may come back; a removal does not. Applying a kick on +// top of a removal must not shorten it to the kick window. +func TestKickDoesNotDowngradeARemoval(t *testing.T) { + r := NewRegistry() + r.Bind("gone", "1.2.3.4:1000") + r.CloseUsers(keep()) + r.KickUserSessions("gone") + + // Past the kick window, nowhere near the removal one. + r.access.Lock() + r.blocked["1.2.3.4:1000"].lastAttempt = time.Now().Add(-kickQuietWindow - time.Second) + r.access.Unlock() + + if r.Allowed("1.2.3.4:1000") { + t.Error("a kick downgraded a removal to the short window") + } +} + +// The reverse: a kick is a decision about a user who is still enabled, so it +// must survive an unrelated save that happens to list them in keep. +func TestUnrelatedSaveDoesNotClearAKick(t *testing.T) { + r := NewRegistry() + r.Bind("live", "1.2.3.4:1000") + r.KickUserSessions("live") + if r.Allowed("1.2.3.4:1000") { + t.Fatal("precondition: the kicked source should be muted") + } + + // Another client is saved; "live" is still enabled, so it is in keep. + r.CloseUsers(keep("live", "other")) + + if r.Allowed("1.2.3.4:1000") { + t.Error("an unrelated save cleared the operator's kick") + } +} From f9906f59c580554e38d270d07edee4bb73054111 Mon Sep 17 00:00:00 2001 From: ShenJiaqing <48464190+shenaba@users.noreply.github.com> Date: Wed, 16 Sep 2026 16:17:48 +0800 Subject: [PATCH 3/3] fix(core): rebuild the inbound when a removed user's session cannot be cut Replaces the source-address mute of the two commits before this one, which could not be made correct. anytls and the sing-mux carrier arrive as a net.Conn that lives exactly as long as the session does, so those are tracked by address and closed outright. That part stands. The QUIC protocols give this layer no handle on the session and no usable name for one either. sing-quic keeps its session list unexported and the per-stream ctx it hands the handler is the Service's own, shared by every session. All three services set quic-go's DisablePathManager, which rewrites a connection's remote address on the first decryptable packet from a new one with no path validation, so the address moves under a NAT rebind; and it is an ephemeral UDP port that is recycled to somebody else afterwards. There is also no moment at which a QUIC session can be declared gone, because quic-go keeps it alive with a 10s PING whether or not a single byte is routed -- which is what defeated both the mute's quiet window and, in review, an attempt to age a per-session entry out after ten idle minutes. Either one silently let a removed user keep an idle session. So for QUIC the registry no longer tracks sessions. It records which users have been seen on the inbound: a set keyed by user rather than address, bounded by the client count rather than the session count, and therefore needing no expiry at all. CloseUsers reports those users as Unclosable, the QUIC inbounds turn that into ErrRestartRequired, and InboundService.UpdateInboundsUsers rebuilds the inbound -- which destroys every QUIC session on it, the removed user's included. Being wrong is one-sided on purpose: a user who connected and then left for good still costs one restart that bought nothing, while the other direction is issue #175 itself. The cost is only paid when a removed user is actually connected, so adding a user or rotating a credential still updates in place. Closes #175. --- core/protocol/anytls/users.go | 24 +- core/protocol/hysteria/inbound.go | 9 +- core/protocol/hysteria/users.go | 12 +- core/protocol/hysteria2/inbound.go | 9 +- core/protocol/hysteria2/users.go | 12 +- core/protocol/trojan/inbound.go | 9 +- core/protocol/trojan/users.go | 9 +- core/protocol/tuic/inbound.go | 9 +- core/protocol/tuic/users.go | 26 +- core/protocol/vless/inbound.go | 9 +- core/protocol/vless/users.go | 5 + core/protocol/vmess/inbound.go | 9 +- core/protocol/vmess/users.go | 1 + core/usersession/registry.go | 373 +++++++++++------------- core/usersession/registry_test.go | 451 ++++++++++++----------------- core/usersession/router.go | 113 +++----- core/usersession/router_test.go | 205 ++++++++----- core/usersession/stress_test.go | 58 ++++ service/inbounds.go | 19 +- 19 files changed, 670 insertions(+), 692 deletions(-) create mode 100644 core/usersession/stress_test.go diff --git a/core/protocol/anytls/users.go b/core/protocol/anytls/users.go index 907aa8fa..07fa8b4e 100644 --- a/core/protocol/anytls/users.go +++ b/core/protocol/anytls/users.go @@ -23,11 +23,16 @@ func (h *Inbound) UpdateUsers(users []option.AnyTLSUser) error { h.service.UpdateUsers(common.Map(users, func(it option.AnyTLSUser) anytls.User { return (anytls.User)(it) })) - keep := make(map[string]struct{}, len(users)) - for _, user := range users { - keep[user.Name] = struct{}{} - } - h.sessions().CloseUsers(keep) + // Unclosable is deliberately not checked: newSessionConnection registers + // the transport of every anytls session, so a removed user's session is + // always cut outright and never costs the inbound a rebuild. The only thing + // that can be counted here is a straggler stream that reached the router + // after its own session had already ended -- not a session, and not worth + // disconnecting everyone else for. If anytls ever grows a session this + // inbound does not hold the transport of, that stops being true. + h.sessions().CloseUsers(usersession.KeepSet(common.Map(users, func(it option.AnyTLSUser) string { + return it.Name + }))) return nil } @@ -52,15 +57,10 @@ func (h *Inbound) sessions() *usersession.Registry { // here is what makes it closable; the streams it later opens reach the router // individually and cannot be used to find it. // -// Returning an error rather than closing the connection reuses the rejection -// the call site already has (N.CloseOnHandshakeFailure). +// The call blocks until the session ends, so the deferred Untrack is what keeps +// the registry to sessions that actually exist. func (h *Inbound) newSessionConnection(ctx context.Context, conn net.Conn, source M.Socksaddr, onClose N.CloseHandlerFunc) error { key := source.String() - // A session with a closer is cut outright rather than muted, so this only - // fires if closing one failed -- keeping the mute as the backstop. - if !h.sessions().Allowed(key) { - return usersession.ErrRemoved - } h.sessions().Track(key, conn) defer h.sessions().Untrack(key) return h.service.NewConnection(ctx, conn, source, onClose) diff --git a/core/protocol/hysteria/inbound.go b/core/protocol/hysteria/inbound.go index 791e3706..0a8c5ab8 100644 --- a/core/protocol/hysteria/inbound.go +++ b/core/protocol/hysteria/inbound.go @@ -19,11 +19,10 @@ // // inbound.router = withUserSessions(inbound.router) // -// which is what lets an already authenticated session be cut, or refused, when -// its user is removed from the inbound -- swapping the user table alone only -// decides who may start a new one. That line is the whole of it here; -// everything it reaches lives in users.go, which the copy check skips. See -// core/usersession. +// which is what lets an already authenticated session be reached when its user +// is removed from the inbound -- swapping the user table alone only decides who +// may start a new one. That line is the whole of it here; everything it reaches +// lives in users.go, which the copy check skips. See core/usersession. package hysteria import ( diff --git a/core/protocol/hysteria/users.go b/core/protocol/hysteria/users.go index 9eadbc9b..3a56064c 100644 --- a/core/protocol/hysteria/users.go +++ b/core/protocol/hysteria/users.go @@ -7,9 +7,10 @@ import ( "github.com/sagernet/sing-box/option" ) -// UpdateUsers swaps the user table of a running inbound and shuts out everyone -// who just left it -- see the note in tuic/users.go, hysteria reuses an -// authenticated session the same way. +// UpdateUsers swaps the user table of a running inbound and disconnects +// everyone who just left it -- see the note in tuic/users.go, hysteria reuses +// an authenticated session the same way, and a removed user who is still +// connected costs the inbound a rebuild for the same reason. func (h *Inbound) UpdateUsers(users []option.HysteriaUser) error { userList := make([]string, 0, len(users)) userPasswordList := make([]string, 0, len(users)) @@ -24,14 +25,13 @@ func (h *Inbound) UpdateUsers(users []option.HysteriaUser) error { userPasswordList = append(userPasswordList, password) } h.service.UpdateUsers(userList, userPasswordList) - h.sessions().CloseUsers(usersession.KeepSet(userList)) - return nil + return h.sessions().CloseUsers(usersession.KeepSet(userList)).RestartRequired() } // withUserSessions installs the session registry in front of the router. This // inbound holds the full adapter.Router, so it takes the Router shim. func withUserSessions(router adapter.Router) adapter.Router { - return usersession.WrapRouter(router, usersession.GateAndBind) + return usersession.WrapRouter(router, usersession.BindOnly) } func (h *Inbound) sessions() *usersession.Registry { diff --git a/core/protocol/hysteria2/inbound.go b/core/protocol/hysteria2/inbound.go index b69f13a1..d1741787 100644 --- a/core/protocol/hysteria2/inbound.go +++ b/core/protocol/hysteria2/inbound.go @@ -19,11 +19,10 @@ // // inbound.router = withUserSessions(inbound.router) // -// which is what lets an already authenticated session be cut, or refused, when -// its user is removed from the inbound -- swapping the user table alone only -// decides who may start a new one. That line is the whole of it here; -// everything it reaches lives in users.go, which the copy check skips. See -// core/usersession. +// which is what lets an already authenticated session be reached when its user +// is removed from the inbound -- swapping the user table alone only decides who +// may start a new one. That line is the whole of it here; everything it reaches +// lives in users.go, which the copy check skips. See core/usersession. package hysteria2 import ( diff --git a/core/protocol/hysteria2/users.go b/core/protocol/hysteria2/users.go index d942ac3a..6b47ea09 100644 --- a/core/protocol/hysteria2/users.go +++ b/core/protocol/hysteria2/users.go @@ -7,9 +7,10 @@ import ( "github.com/sagernet/sing-box/option" ) -// UpdateUsers swaps the user table of a running inbound and shuts out everyone -// who just left it -- see the note in tuic/users.go, hysteria2 reuses an -// authenticated session the same way. +// UpdateUsers swaps the user table of a running inbound and disconnects +// everyone who just left it -- see the note in tuic/users.go, hysteria2 reuses +// an authenticated session the same way, and a removed user who is still +// connected costs the inbound a rebuild for the same reason. func (h *Inbound) UpdateUsers(users []option.Hysteria2User) error { userList := make([]string, 0, len(users)) userPasswordList := make([]string, 0, len(users)) @@ -18,14 +19,13 @@ func (h *Inbound) UpdateUsers(users []option.Hysteria2User) error { userPasswordList = append(userPasswordList, user.Password) } h.service.UpdateUsers(userList, userPasswordList) - h.sessions().CloseUsers(usersession.KeepSet(userList)) - return nil + return h.sessions().CloseUsers(usersession.KeepSet(userList)).RestartRequired() } // withUserSessions installs the session registry in front of the router. This // inbound holds the full adapter.Router, so it takes the Router shim. func withUserSessions(router adapter.Router) adapter.Router { - return usersession.WrapRouter(router, usersession.GateAndBind) + return usersession.WrapRouter(router, usersession.BindOnly) } func (h *Inbound) sessions() *usersession.Registry { diff --git a/core/protocol/trojan/inbound.go b/core/protocol/trojan/inbound.go index b1b07223..67cad54d 100644 --- a/core/protocol/trojan/inbound.go +++ b/core/protocol/trojan/inbound.go @@ -19,11 +19,10 @@ // // inbound.router = withUserSessions(inbound.router) // -// which is what lets an already authenticated session be cut, or refused, when -// its user is removed from the inbound -- swapping the user table alone only -// decides who may start a new one. That line is the whole of it here; -// everything it reaches lives in users.go, which the copy check skips. See -// core/usersession. +// which is what lets an already authenticated session be reached when its user +// is removed from the inbound -- swapping the user table alone only decides who +// may start a new one. That line is the whole of it here; everything it reaches +// lives in users.go, which the copy check skips. See core/usersession. package trojan import ( diff --git a/core/protocol/trojan/users.go b/core/protocol/trojan/users.go index f4219b32..a2ce7d47 100644 --- a/core/protocol/trojan/users.go +++ b/core/protocol/trojan/users.go @@ -22,6 +22,7 @@ func (h *Inbound) UpdateUsers(users []option.TrojanUser) error { if err != nil { return err } + // Unclosable is not checked here; see the note in vless/users.go. h.sessions().CloseUsers(usersession.KeepSet(common.Map(users, func(it option.TrojanUser) string { return it.Name }))) @@ -30,11 +31,9 @@ func (h *Inbound) UpdateUsers(users []option.TrojanUser) error { // withUserSessions installs the session registry in front of the router. Only // the multiplex carrier is tracked: every other connection authenticates on its -// own and is already covered by ConnTracker. -// -// Deliberately not a gate: the fallback path routes unauthenticated visitors -// through this same router, and refusing there would cut off the fallback for -// whoever happens to share a muted source. +// own and is already covered by ConnTracker. The fallback path routes +// unauthenticated visitors through this same router, which is one more reason +// nothing here may refuse a connection. func withUserSessions(router adapter.ConnectionRouterEx) adapter.ConnectionRouterEx { return usersession.WrapRouterEx(router, usersession.TrackMuxCarrier) } diff --git a/core/protocol/tuic/inbound.go b/core/protocol/tuic/inbound.go index 80c4247e..d2431ef8 100644 --- a/core/protocol/tuic/inbound.go +++ b/core/protocol/tuic/inbound.go @@ -19,11 +19,10 @@ // // inbound.router = withUserSessions(inbound.router) // -// which is what lets an already authenticated session be cut, or refused, when -// its user is removed from the inbound -- swapping the user table alone only -// decides who may start a new one. That line is the whole of it here; -// everything it reaches lives in users.go, which the copy check skips. See -// core/usersession. +// which is what lets an already authenticated session be reached when its user +// is removed from the inbound -- swapping the user table alone only decides who +// may start a new one. That line is the whole of it here; everything it reaches +// lives in users.go, which the copy check skips. See core/usersession. package tuic import ( diff --git a/core/protocol/tuic/users.go b/core/protocol/tuic/users.go index f34e8581..ad6bece7 100644 --- a/core/protocol/tuic/users.go +++ b/core/protocol/tuic/users.go @@ -10,10 +10,16 @@ import ( "github.com/gofrs/uuid/v5" ) -// UpdateUsers swaps the user table of a running inbound and shuts out everyone -// who just left it. Both halves are needed: the table alone only decides who -// may open a new session, while a client that authenticated before the change -// keeps opening streams on the one it already has. +// UpdateUsers swaps the user table of a running inbound and disconnects +// everyone who just left it. Both halves are needed: the table alone only +// decides who may open a new session, while a client that authenticated before +// the change keeps opening streams on the one it already has. +// +// The second half is not something this layer can do to a QUIC session, so when +// a removed user is still connected it reports ErrRestartRequired and the +// caller rebuilds the inbound instead. That disconnects everyone on it once, +// which is why it is reported rather than done unconditionally: an update that +// removes nobody, or removes nobody who is connected, still costs nothing. func (h *Inbound) UpdateUsers(users []option.TUICUser) error { userList := make([]string, 0, len(users)) userUUIDList := make([][16]byte, 0, len(users)) @@ -31,17 +37,17 @@ func (h *Inbound) UpdateUsers(users []option.TUICUser) error { userPasswordList = append(userPasswordList, user.Password) } h.server.UpdateUsers(userList, userUUIDList, userPasswordList) - h.sessions().CloseUsers(usersession.KeepSet(userList)) - return nil + return h.sessions().CloseUsers(usersession.KeepSet(userList)).RestartRequired() } // withUserSessions installs the session registry in front of the router. TUIC // authenticates once per QUIC session and every stream after that rides it, so -// a removed user has to be refused here -- refused rather than cut, because the -// QUIC session is not something this layer holds a handle on. See -// core/usersession for why the hook sits on the router. +// the registry is here to answer one question at removal time: is the user +// being removed actually connected? It cannot do more than that -- sing-quic +// hands out neither a handle on the session nor a stable name for it. See +// core/usersession. func withUserSessions(router adapter.ConnectionRouterEx) adapter.ConnectionRouterEx { - return usersession.WrapRouterEx(router, usersession.GateAndBind) + return usersession.WrapRouterEx(router, usersession.BindOnly) } // sessions reaches the registry withUserSessions installed. The assertion is diff --git a/core/protocol/vless/inbound.go b/core/protocol/vless/inbound.go index 91539eb8..808715d0 100644 --- a/core/protocol/vless/inbound.go +++ b/core/protocol/vless/inbound.go @@ -19,11 +19,10 @@ // // inbound.router = withUserSessions(inbound.router) // -// which is what lets an already authenticated session be cut, or refused, when -// its user is removed from the inbound -- swapping the user table alone only -// decides who may start a new one. That line is the whole of it here; -// everything it reaches lives in users.go, which the copy check skips. See -// core/usersession. +// which is what lets an already authenticated session be reached when its user +// is removed from the inbound -- swapping the user table alone only decides who +// may start a new one. That line is the whole of it here; everything it reaches +// lives in users.go, which the copy check skips. See core/usersession. package vless import ( diff --git a/core/protocol/vless/users.go b/core/protocol/vless/users.go index 6042d03b..5b905f1a 100644 --- a/core/protocol/vless/users.go +++ b/core/protocol/vless/users.go @@ -21,6 +21,11 @@ func (h *Inbound) UpdateUsers(users []option.VLESSUser) error { }), common.Map(users, func(it option.VLESSUser) string { return it.Flow })) + // Unclosable is deliberately not checked: a mux carrier is a net.Conn and + // is cut outright, so a removed user never costs this inbound the rebuild a + // QUIC one does. The only thing that can be counted here is a packet conn + // addressed to the mux destination -- not a carrier, and not worth + // disconnecting everyone else for. h.sessions().CloseUsers(usersession.KeepSet(common.Map(users, func(it option.VLESSUser) string { return it.Name }))) diff --git a/core/protocol/vmess/inbound.go b/core/protocol/vmess/inbound.go index 644a2206..0c3c80c4 100644 --- a/core/protocol/vmess/inbound.go +++ b/core/protocol/vmess/inbound.go @@ -21,11 +21,10 @@ // // inbound.router = withUserSessions(inbound.router) // -// which is what lets an already authenticated session be cut, or refused, when -// its user is removed from the inbound -- swapping the user table alone only -// decides who may start a new one. That line is the whole of it here; -// everything it reaches lives in users.go, which the copy check skips. See -// core/usersession. +// which is what lets an already authenticated session be reached when its user +// is removed from the inbound -- swapping the user table alone only decides who +// may start a new one. That line is the whole of it here; everything it reaches +// lives in users.go, which the copy check skips. See core/usersession. package vmess import ( diff --git a/core/protocol/vmess/users.go b/core/protocol/vmess/users.go index 77090a8d..2f32add6 100644 --- a/core/protocol/vmess/users.go +++ b/core/protocol/vmess/users.go @@ -23,6 +23,7 @@ func (h *Inbound) UpdateUsers(users []option.VMessUser) error { if err != nil { return err } + // Unclosable is not checked here; see the note in vless/users.go. h.sessions().CloseUsers(usersession.KeepSet(common.Map(users, func(it option.VMessUser) string { return it.Name }))) diff --git a/core/usersession/registry.go b/core/usersession/registry.go index 3f2cabc1..04c87408 100644 --- a/core/usersession/registry.go +++ b/core/usersession/registry.go @@ -8,146 +8,150 @@ // client simply opens another stream on the session it already has. Reaching // the session itself is what this registry is for. // -// This lives at the inbound layer rather than in ConnTracker on purpose. A -// tracker-level gate sees a connection only after routing, so it misses the -// ones the router answers itself (hijack-dns), and refusing there still costs -// one real dial to the destination before the copy fails. Refusing here happens -// before any of that. The two layers stay separate: IP limits keep their gate -// in ConnTracker, because that policy has to outlive a core restart, while -// everything here is per-inbound and goes away with the Box. +// # Two kinds of session +// +// anytls and the sing-mux carrier arrive as a net.Conn that lives exactly as +// long as the session does. Those are tracked by source address and closed +// outright, and that is the whole story for anytls, vless, vmess and trojan: +// the key is a single TCP connection's address, it cannot move, and Untrack +// runs when the session ends. +// +// The QUIC protocols give this layer no handle on the session -- and, the part +// that decides the design, no usable name for one either: +// +// - sing-quic keeps its session list unexported, and the ctx it hands the +// handler per stream is the Service's own, shared by every session; +// - all three services set quic-go's DisablePathManager, which rewrites a +// connection's remote address the moment a decryptable packet arrives from +// a new one, with no path validation -- so the source address moves under +// one NAT rebind; +// - that address is an ephemeral UDP port, recycled to somebody else once the +// session ends, and nothing tells this layer that it has been; +// - and there is no moment at which a QUIC session can be declared gone. +// quic-go keeps one alive with a 10s PING whether or not a single byte is +// routed, so an idle session and a dead one look identical from here. +// +// So for QUIC this package does not track sessions at all. It records +// something weaker and completely reliable: **which users have been seen on +// this inbound**. That set is bounded by the client count rather than the +// session count, which is what lets it carry no timeout -- and a set with no +// timeout is the only kind that cannot be wrong about a session it cannot see. +// +// Earlier revisions tried to do better and could not. Muting the source address +// is defeated by the second and third points; ageing a per-session entry out +// after ten idle minutes is defeated by the fourth, and silently let a removed +// user keep an idle session. Both looked like they worked because a test client +// that reconnects every second never exercises either. +// +// # What a removal does +// +// CloseUsers cuts every session it holds a transport for, and reports the rest +// as Unclosable. The QUIC inbounds turn that into ErrRestartRequired and the +// caller rebuilds the inbound, which destroys every QUIC session on it, the +// removed user's included. Everyone on that inbound reconnects once. +// +// Being wrong here is one-sided on purpose: a user who connected and then left +// for good is still in the seen set, so removing them costs one restart that +// bought nothing. The other direction -- deciding a session is gone when it is +// not -- is what issue #175 is, so the cost is paid on that side. +// +// This lives at the inbound layer rather than in ConnTracker on purpose. The +// two stay separate: IP limits keep their gate in ConnTracker, because that +// policy has to outlive a core restart, while everything here is per-inbound +// and goes away with the Box. package usersession import ( "io" "sync" - "time" - "github.com/sagernet/sing/common" E "github.com/sagernet/sing/common/exceptions" - N "github.com/sagernet/sing/common/network" -) - -const ( - // A source that has not opened a connection for this long is forgotten; - // its session is either gone or idle enough to be re-learned on use. - idleTimeout = 10 * time.Minute - // How long a removed user's session has to stay quiet before its address is - // let go. Measured from the last refused attempt, not from when the block - // was written: a client that keeps trying is a session that is still alive, - // and it stays muted for as long as that lasts. - blockTimeout = 10 * time.Minute - // A kicked session is muted only while it keeps trying. Once it has been - // quiet this long, the next attempt from the address is a new session and - // is let through, so a disconnect does not turn into a lockout. - kickQuietWindow = 30 * time.Second ) +// entry is one session this layer can actually reach: a transport it can close, +// and the user that authenticated it. type entry struct { - user string - lastSeen time.Time - closer io.Closer -} - -// block mutes one client address. Both kinds lift on quiet rather than on a -// timer (see Allowed) -- a kick far sooner, because the user behind it is -// still enabled and must not be locked out. -type block struct { - lastAttempt time.Time - kick bool + user string + closer io.Closer } -// Registry maps a client address to the user it authenticated as. One instance -// per inbound. +// Registry is what an inbound knows about who is connected to it. One instance +// per inbound; it goes away with the inbound. // -// The key is the full source address including the port, never a normalized -// one: it identifies a single session, and two sessions from one subscriber -// must not share an entry. (ConnTracker deliberately does the opposite -- it -// masks IPv6 to a prefix -- because an IP limit counts subscribers, not -// sessions. Keying this map that way would mute an entire /64 when one client -// in it is removed.) +// Neither map is consulted to decide whether a connection may pass -- nothing +// in this package refuses anything. They decide whose transport to close, and +// whether a removed user was connected at all. type Registry struct { - access sync.Mutex - sources map[string]*entry - blocked map[string]*block - lastSweep time.Time + access sync.Mutex + // sources holds the sessions with a closer, keyed by the source address of + // the one connection that carries them. Bounded by live sessions: every + // entry is created by Track or BindAndTrack and removed by the Untrack its + // inbound defers. + sources map[string]*entry + // seen holds the users that turned up without a closable session -- the + // QUIC ones. Keyed by user rather than by address because no address here + // is stable, and bounded by the client count rather than by the session + // count, which is what lets it need no expiry. See the package comment. + seen map[string]struct{} } func NewRegistry() *Registry { return &Registry{ - sources: make(map[string]*entry), - blocked: make(map[string]*block), - lastSweep: time.Now(), + sources: make(map[string]*entry), + seen: make(map[string]struct{}), } } -func (r *Registry) load(source string) *entry { - e, loaded := r.sources[source] - if !loaded { - e = &entry{} - r.sources[source] = e +// Result is what a removal or a kick managed to do. +type Result struct { + // Cut counts the sessions closed outright. + Cut int + // Unclosable counts the users left connected because this layer has no + // handle on their session. For a QUIC inbound that is the signal to rebuild + // the inbound instead; see ErrRestartRequired. + Unclosable int +} + +// RestartRequired is what an inbound whose sessions it cannot close returns +// from UpdateUsers: nil when the removal was carried out in full, and +// ErrRestartRequired when a removed user is still connected by a session that +// only rebuilding the inbound will end. +// +// Only the QUIC inbounds call this. anytls and the mux protocols hold the +// transport of every session they register -- see the notes on their own +// CloseUsers calls. +func (r Result) RestartRequired() error { + if r.Unclosable > 0 { + return ErrRestartRequired } - e.lastSeen = time.Now() - return e + return nil } -// Bind records which user the session at source authenticated as. Called for -// every connection the session opens, which doubles as a liveness ping. +// Bind records that user is connected. If the connection belongs to a session +// something already tracked (anytls registers its transport in NewConnection +// before any stream is routed), the user is attached to that session so it can +// be closed by name. Otherwise there is no session to attach to and the user +// goes into the seen set -- which is the QUIC path. func (r *Registry) Bind(user string, source string) { - if source == "" { + if user == "" { return } r.access.Lock() defer r.access.Unlock() - e := r.load(source) - if user != "" { + if e, tracked := r.sources[source]; tracked { e.user = user - } - r.sweepLocked(e.lastSeen) -} - -// sweepLocked drops entries nothing will come back for. The QUIC inbounds only -// ever Bind -- a QUIC session ends without a callback this package could hang -// Untrack on -- so without this, sources would grow with every session the -// listener has ever seen. Riding on Bind keeps it to one walk per idleTimeout -// and needs no cron job of its own. -func (r *Registry) sweepLocked(now time.Time) { - if now.Sub(r.lastSweep) < idleTimeout { return } - r.lastSweep = now - for source, e := range r.sources { - if idleLost(e, now) { - delete(r.sources, source) - delete(r.blocked, source) - } - } - for source, b := range r.blocked { - if now.Sub(b.lastAttempt) > blockTimeout { - delete(r.blocked, source) - } - } -} - -// idleLost reports whether an entry is one nothing will come back for. -// -// A tracked session is never that, however long it has been quiet: lastSeen -// only moves when the session opens another connection, so one carrying a -// single long-lived stream -- an ssh session, a download, a long poll -- looks -// idle here while it is perfectly alive. Dropping it would discard the closer, -// which is the only handle on it there is, and the next removal would then find -// nothing to cut and let the session run on. Tracked entries are cleaned up by -// their own Untrack instead, which the inbound defers for exactly that. -func idleLost(e *entry, now time.Time) bool { - return e.closer == nil && now.Sub(e.lastSeen) > idleTimeout + r.seen[user] = struct{}{} } // BindAndTrack records the user and the session transport in one go, for a // carrier that arrives with both already known. // // The two must land under a single lock. A CloseUsers that ran in between -- -// seeing the user but not yet the closer -- would file the session as one it -// cannot close and mute it instead, and a mux carrier is never gated, so the -// mute would do nothing at all while the session kept being served. +// seeing the user but not yet the closer -- would put it in the seen set and +// report it Unclosable, costing a restart that the closer it was about to be +// given would have made unnecessary. func (r *Registry) BindAndTrack(user string, source string, closer io.Closer) { if source == "" { return @@ -161,11 +165,10 @@ func (r *Registry) BindAndTrack(user string, source string, closer io.Closer) { if closer != nil { e.closer = closer } - r.sweepLocked(e.lastSeen) } -// Track stores the session transport, for protocols whose session has a closer -// of its own. Without one the session can only be muted, not closed. +// Track stores the session transport, for a protocol whose session has a closer +// of its own but does not know the user yet. The Bind that follows attaches it. func (r *Registry) Track(source string, closer io.Closer) { if source == "" { return @@ -175,6 +178,9 @@ func (r *Registry) Track(source string, closer io.Closer) { r.load(source).closer = closer } +// Untrack forgets a session that has ended. This is what keeps sources bounded, +// and it is why a tracked session never needs to be aged out: its inbound +// defers this call for exactly as long as the session lives. func (r *Registry) Untrack(source string) { if source == "" { return @@ -182,92 +188,51 @@ func (r *Registry) Untrack(source string) { r.access.Lock() defer r.access.Unlock() delete(r.sources, source) - delete(r.blocked, source) } -// Allowed reports whether connections from source may still be routed. A -// session that cannot be closed is muted here instead: nothing it opens is -// routed any more, so the removed user's traffic stops. -func (r *Registry) Allowed(source string) bool { - if source == "" { - return true - } - r.access.Lock() - defer r.access.Unlock() - b, blocked := r.blocked[source] - if !blocked { - return true - } - now := time.Now() - // Both windows are measured from the last attempt, never from when the - // block was written. A client that keeps hammering is a session that is - // still alive, and a session that is still alive is still authenticated: - // letting it back in on a timer would hand a removed user their traffic - // back, because the streams it opens afterwards never consult the user - // table again. Quiet is the only evidence that a session is really gone. - // - // A kick lifts quickly because the user is still enabled and must not be - // locked out; a removal holds until the session gives up for good. - window := blockTimeout - if b.kick { - window = kickQuietWindow - } - if now.Sub(b.lastAttempt) > window { - delete(r.blocked, source) - return true - } - b.lastAttempt = now - // A source being refused is anything but idle. Without this the sweep - // would see a stale lastSeen, drop the entry and take the block with it -- - // lifting the mute by the other road. - if e, ok := r.sources[source]; ok { - e.lastSeen = now +func (r *Registry) load(source string) *entry { + e, loaded := r.sources[source] + if !loaded { + e = &entry{} + r.sources[source] = e } - return false + return e } -// CloseUsers cuts the sessions of every user not in keep and lifts the block on -// the sessions of users that are in keep, so re-enabling a user takes effect -// without waiting for their session to die. Returns the number of sessions cut. -func (r *Registry) CloseUsers(keep map[string]struct{}) int { - now := time.Now() - +// CloseUsers cuts the sessions of every user not in keep, and reports the users +// it could not reach. keep is the user table the inbound has just installed, so +// anything bound to a name outside it belongs to a user who is gone. +func (r *Registry) CloseUsers(keep map[string]struct{}) Result { r.access.Lock() var closers []io.Closer - cut := 0 + var result Result for source, e := range r.sources { - if idleLost(e, now) { - delete(r.sources, source) - delete(r.blocked, source) - continue - } if e.user == "" { continue } if _, ok := keep[e.user]; ok { - // Lift a removal: the user is back on the inbound. A kick is a - // separate decision an operator just made about a user who was - // never removed, so an unrelated save must not undo it. - if b, muted := r.blocked[source]; muted && !b.kick { - delete(r.blocked, source) - } continue } - cut++ - if e.closer != nil { - closers = append(closers, e.closer) - delete(r.sources, source) - delete(r.blocked, source) + if e.closer == nil { + result.Unclosable++ continue } - r.blocked[source] = &block{lastAttempt: now} + result.Cut++ + closers = append(closers, e.closer) + delete(r.sources, source) } - for source, b := range r.blocked { - if now.Sub(b.lastAttempt) > blockTimeout { - delete(r.blocked, source) + for user := range r.seen { + if _, ok := keep[user]; ok { + continue } + result.Unclosable++ + // Dropped because the user is off the inbound: either the caller is + // about to rebuild it, which discards this registry anyway, or it is a + // protocol that does not rebuild, where reporting the same departed + // user on every later save would be noise. A user who is re-enabled and + // connects again is recorded again by Bind. + delete(r.seen, user) } - r.lastSweep = now r.access.Unlock() // Outside the lock: closing a tracked session runs the inbound's own close @@ -275,48 +240,45 @@ func (r *Registry) CloseUsers(keep map[string]struct{}) int { for _, closer := range closers { _ = closer.Close() } - return cut + return result } -// KickUserSessions disconnects a user who is still enabled. A session with a -// closer is cut outright; one without is muted, which is the only way to stop a -// QUIC session that the protocol gives us no handle on. The mute lifts as soon -// as that session stops trying, so the client reconnects on its own. -func (r *Registry) KickUserSessions(user string) int { +// KickUserSessions disconnects a user who is still enabled -- an operator +// action, not a revocation. Only a session with a closer can be cut; a user who +// is merely known to be connected is reported instead, and it is the caller's +// business whether disconnecting one user is worth restarting the inbound +// everyone else is on. +func (r *Registry) KickUserSessions(user string) Result { if user == "" { - return 0 + return Result{} } - now := time.Now() r.access.Lock() var closers []io.Closer - kicked := 0 + var result Result for source, e := range r.sources { if e.user != user { continue } - kicked++ - if e.closer != nil { - delete(r.sources, source) - delete(r.blocked, source) - closers = append(closers, e.closer) + if e.closer == nil { + result.Unclosable++ continue } - // A removal already standing is the stricter of the two and must not be - // downgraded: that user is gone from the inbound, while a kick assumes - // they are still entitled to reconnect once they stop hammering. - if existing, muted := r.blocked[source]; muted && !existing.kick { - existing.lastAttempt = now - continue - } - r.blocked[source] = &block{lastAttempt: now, kick: true} + result.Cut++ + closers = append(closers, e.closer) + delete(r.sources, source) + } + // Left in the set: this user is still on the inbound and may still be + // connected, so a later removal has to report them again. + if _, ok := r.seen[user]; ok { + result.Unclosable++ } r.access.Unlock() for _, closer := range closers { _ = closer.Close() } - return kicked + return result } // KeepSet turns the user-name list an inbound just installed into the set @@ -330,14 +292,13 @@ func KeepSet(names []string) map[string]struct{} { return keep } -// ErrRemoved is reported to the close handler of a connection that a muted -// session tried to open. -var ErrRemoved = E.New("user removed from inbound") - -// Reject drops a connection opened by a session whose user is gone. -func Reject(conn io.Closer, onClose N.CloseHandlerFunc) { - common.Close(conn) - if onClose != nil { - onClose(ErrRemoved) - } -} +// ErrRestartRequired reports that a user who was just removed is still +// connected by a session this layer cannot close, so the only way to disconnect +// them is to tear the inbound down and build it again. +// +// The caller already has that path -- it is the same fallback taken by +// protocols with no in-place user update at all -- so returning this error is +// all an inbound has to do. It is not a failure: the user table was swapped +// successfully, and the caller tells the two apart so that this one is not +// logged as one. +var ErrRestartRequired = E.New("removed user is still connected by a session that cannot be cut") diff --git a/core/usersession/registry_test.go b/core/usersession/registry_test.go index 8c765437..cd706b1d 100644 --- a/core/usersession/registry_test.go +++ b/core/usersession/registry_test.go @@ -2,8 +2,8 @@ package usersession import ( "errors" + "strconv" "testing" - "time" ) type fakeCloser struct { @@ -23,377 +23,304 @@ func keep(users ...string) map[string]struct{} { return set } +// What the QUIC inbounds turn a CloseUsers result into. Asking for a restart +// when nothing was left connected would disconnect a whole inbound on every +// ordinary save; not asking when something was leaves the removed user online. +func TestResultRestartRequired(t *testing.T) { + for _, c := range []struct { + result Result + want error + }{ + {Result{}, nil}, + {Result{Cut: 3}, nil}, + {Result{Unclosable: 1}, ErrRestartRequired}, + {Result{Cut: 2, Unclosable: 1}, ErrRestartRequired}, + } { + if got := c.result.RestartRequired(); !errors.Is(got, c.want) { + t.Errorf("%+v.RestartRequired() = %v, want %v", c.result, got, c.want) + } + } +} + func TestCloseUsersClosesTrackedSession(t *testing.T) { r := NewRegistry() conn := &fakeCloser{} r.Track("1.2.3.4:1000", conn) r.Bind("gone", "1.2.3.4:1000") - if cut := r.CloseUsers(keep("stays")); cut != 1 { - t.Fatalf("cut = %d, want 1", cut) + got := r.CloseUsers(keep("stays")) + if got != (Result{Cut: 1}) { + t.Fatalf("CloseUsers = %+v, want {Cut:1}", got) } if !conn.closed { - t.Error("a tracked session must be closed, not muted") - } - // Nothing is left muted: the session is gone, so the address is free for - // whoever connects from it next. - if !r.Allowed("1.2.3.4:1000") { - t.Error("address stayed muted after its session was closed") + t.Error("a tracked session must be closed") } } -func TestCloseUsersMutesUntrackedSession(t *testing.T) { +// The QUIC shape: nothing was tracked, so there is no session to cut. Saying so +// is the whole contract -- it is what makes the inbound ask to be rebuilt. +func TestCloseUsersReportsUntrackedUser(t *testing.T) { r := NewRegistry() - // No Track: this is the QUIC shape, where the session has no closer we can - // reach. Muting is the only thing left. r.Bind("gone", "1.2.3.4:1000") - if cut := r.CloseUsers(keep("stays")); cut != 1 { - t.Fatalf("cut = %d, want 1", cut) - } - if r.Allowed("1.2.3.4:1000") { - t.Error("a session with no closer must be muted") - } - // An address nobody authenticated from is not affected. - if !r.Allowed("5.6.7.8:2000") { - t.Error("an unrelated address must stay allowed") + got := r.CloseUsers(keep("stays")) + if got != (Result{Unclosable: 1}) { + t.Fatalf("CloseUsers = %+v, want {Unclosable:1}", got) } } -func TestCloseUsersLeavesUnauthenticatedSourceAlone(t *testing.T) { +// THE regression this package exists for, and the one an earlier revision got +// wrong twice. A QUIC session that has routed nothing for a long time is not a +// session that ended: quic-go keeps it alive with a 10s PING, and the streams +// it opens later never re-check the user table. Nothing may therefore expire a +// user out of the seen set -- only their removal takes them out of it. +// +// There is no clock to advance here, which is the point: the fix was to delete +// the notion of an idle session rather than to tune it. What stands in for +// elapsed time is unbounded unrelated activity. +func TestIdleUserIsNeverForgotten(t *testing.T) { r := NewRegistry() - // Bound with no user name: the connection reached the handler before - // authentication produced one. It belongs to nobody, so a removal has - // nothing to say about it. - r.Bind("", "1.2.3.4:1000") + r.Bind("idle", "1.2.3.4:1000") - if cut := r.CloseUsers(keep("stays")); cut != 0 { - t.Fatalf("cut = %d, want 0", cut) + // Lots of other traffic and lots of ordinary saves, none of which concern + // "idle" -- who sends nothing at all for the whole stretch. + for i := 0; i < 500; i++ { + r.Bind("busy", "5.6.7.8:"+strconv.Itoa(2000+i)) + r.CloseUsers(keep("idle", "busy")) } - if !r.Allowed("1.2.3.4:1000") { - t.Error("an unauthenticated source must not be muted") - } -} -func TestCloseUsersLiftsMuteWhenUserComesBack(t *testing.T) { - r := NewRegistry() - r.Bind("flip", "1.2.3.4:1000") - r.CloseUsers(keep()) - if r.Allowed("1.2.3.4:1000") { - t.Fatal("precondition: the session should be muted") - } - - // Re-enabling has to take effect now, not when the block times out. - r.CloseUsers(keep("flip")) - if !r.Allowed("1.2.3.4:1000") { - t.Error("re-enabling a user must lift the mute immediately") + got := r.CloseUsers(keep("busy")) + if got != (Result{Unclosable: 1}) { + t.Fatalf("CloseUsers = %+v, want {Unclosable:1} -- an idle user was forgotten", got) } } -func TestKickClosesTrackedSession(t *testing.T) { +// The seen set is keyed by user, not by address, and that is what makes it safe +// to keep forever: it is bounded by the client count, not by how many sessions +// or source ports a client burns through. A QUIC client whose address moves +// (NAT rebind, DisablePathManager) is also still the same one entry. +func TestSeenSetIsKeyedByUser(t *testing.T) { r := NewRegistry() - conn := &fakeCloser{} - r.Track("1.2.3.4:1000", conn) - r.Bind("noisy", "1.2.3.4:1000") + for i := 0; i < 1000; i++ { + r.Bind("roamer", "1.2.3.4:"+strconv.Itoa(1000+i)) + } - if kicked := r.KickUserSessions("noisy"); kicked != 1 { - t.Fatalf("kicked = %d, want 1", kicked) + r.access.Lock() + size := len(r.seen) + sources := len(r.sources) + r.access.Unlock() + if size != 1 { + t.Errorf("len(seen) = %d after 1000 addresses, want 1", size) } - if !conn.closed { - t.Error("a tracked session must be closed on kick") + if sources != 0 { + t.Errorf("len(sources) = %d, want 0 -- an untracked user must not allocate per address", sources) } - if !r.Allowed("1.2.3.4:1000") { - t.Error("a kicked client must be free to reconnect at once") + + got := r.CloseUsers(keep()) + if got != (Result{Unclosable: 1}) { + t.Errorf("CloseUsers = %+v, want {Unclosable:1} -- one user is one report", got) } } -func TestKickMuteLiftsAfterQuietWindow(t *testing.T) { +// Re-enabling a user has to work: their next connection puts them back. +func TestRemovedUserIsRecordedAgainOnReconnect(t *testing.T) { r := NewRegistry() - r.Bind("noisy", "1.2.3.4:1000") - r.KickUserSessions("noisy") - - // The kicked session keeps trying and keeps being refused. - for i := 0; i < 3; i++ { - if r.Allowed("1.2.3.4:1000") { - t.Fatalf("attempt %d was let through while the session kept trying", i) - } + r.Bind("flip", "1.2.3.4:1000") + if got := r.CloseUsers(keep()); got != (Result{Unclosable: 1}) { + t.Fatalf("precondition: CloseUsers = %+v, want {Unclosable:1}", got) + } + // Reported once and dropped, so an unrelated later save says nothing. + if got := r.CloseUsers(keep()); got != (Result{}) { + t.Fatalf("CloseUsers = %+v on the second pass, want an empty result", got) } - // Once it has been quiet for the window, the next attempt is a new session. - r.access.Lock() - r.blocked["1.2.3.4:1000"].lastAttempt = time.Now().Add(-kickQuietWindow - time.Second) - r.access.Unlock() - - if !r.Allowed("1.2.3.4:1000") { - t.Error("a kick must not turn into a lockout") + r.Bind("flip", "1.2.3.4:2000") + if got := r.CloseUsers(keep()); got != (Result{Unclosable: 1}) { + t.Errorf("CloseUsers = %+v after reconnect, want {Unclosable:1}", got) } } -func TestRemovalMuteDoesNotLiftOnQuiet(t *testing.T) { +func TestCloseUsersLeavesUnauthenticatedSourceAlone(t *testing.T) { r := NewRegistry() - r.Bind("gone", "1.2.3.4:1000") - r.CloseUsers(keep()) - - // Same quiet gap that would lift a kick. A removal is not a kick: the user - // is no longer on the inbound, so there is nothing to let back in and the - // mute holds until the backstop. - r.access.Lock() - r.blocked["1.2.3.4:1000"].lastAttempt = time.Now().Add(-kickQuietWindow - time.Second) - r.access.Unlock() + // Bound with no user name: the connection reached the handler before + // authentication produced one. It belongs to nobody, so a removal has + // nothing to say about it. + r.Bind("", "1.2.3.4:1000") - if r.Allowed("1.2.3.4:1000") { - t.Error("a removal mute must not lift just because the session went quiet") + got := r.CloseUsers(keep("stays")) + if got != (Result{}) { + t.Fatalf("CloseUsers = %+v, want an empty result", got) } } -// "Long dead" means the session stopped trying -- not merely that the block was -// written a while ago. Ageing the block itself was what the earlier version of -// this test did, and it let a real defect through: a client hammering away for -// ten minutes got its traffic back on the timer. -func TestRemovalMuteLiftsWhenSessionGivesUp(t *testing.T) { +// A user still on the inbound is not touched, however many sessions they have. +func TestCloseUsersKeepsEnabledUsers(t *testing.T) { r := NewRegistry() - r.Bind("gone", "1.2.3.4:1000") - r.CloseUsers(keep()) - - r.access.Lock() - r.blocked["1.2.3.4:1000"].lastAttempt = time.Now().Add(-blockTimeout - time.Second) - r.access.Unlock() + first := &fakeCloser{} + second := &fakeCloser{} + r.BindAndTrack("stays", "1.2.3.4:1000", first) + r.BindAndTrack("stays", "1.2.3.4:1001", second) + r.Bind("stays", "5.6.7.8:2000") - if !r.Allowed("1.2.3.4:1000") { - t.Error("an address whose session gave up must eventually be let go") + got := r.CloseUsers(keep("stays")) + if got != (Result{}) { + t.Fatalf("CloseUsers = %+v, want an empty result", got) + } + if first.closed || second.closed { + t.Error("a user who is still enabled must keep their sessions") } } -func TestUntrackForgetsSession(t *testing.T) { +func TestKickClosesTrackedSession(t *testing.T) { r := NewRegistry() conn := &fakeCloser{} r.Track("1.2.3.4:1000", conn) - r.Bind("gone", "1.2.3.4:1000") - r.Untrack("1.2.3.4:1000") + r.Bind("noisy", "1.2.3.4:1000") - if cut := r.CloseUsers(keep()); cut != 0 { - t.Fatalf("cut = %d, want 0 -- the session was already gone", cut) + got := r.KickUserSessions("noisy") + if got != (Result{Cut: 1}) { + t.Fatalf("KickUserSessions = %+v, want {Cut:1}", got) } - if conn.closed { - t.Error("an untracked session must not be closed again") + if !conn.closed { + t.Error("a tracked session must be closed on kick") } } -// The QUIC inbounds never Untrack, so Bind is the only place that can drop what -// they leave behind. -func TestSweepDropsIdleSources(t *testing.T) { +// A kick cannot reach a QUIC session either. It says so rather than pretending, +// and leaves what to do about it to the caller. +func TestKickReportsUntrackedUser(t *testing.T) { r := NewRegistry() - r.Bind("old", "1.2.3.4:1000") - - r.access.Lock() - r.sources["1.2.3.4:1000"].lastSeen = time.Now().Add(-idleTimeout - time.Minute) - r.lastSweep = time.Now().Add(-idleTimeout - time.Minute) - r.access.Unlock() - - r.Bind("fresh", "5.6.7.8:2000") - - r.access.Lock() - _, stale := r.sources["1.2.3.4:1000"] - count := len(r.sources) - r.access.Unlock() + r.Bind("noisy", "1.2.3.4:1000") - if stale { - t.Error("an idle source must be swept") + got := r.KickUserSessions("noisy") + if got != (Result{Unclosable: 1}) { + t.Fatalf("KickUserSessions = %+v, want {Unclosable:1}", got) } - if count != 1 { - t.Errorf("len(sources) = %d, want 1 (the fresh one)", count) + // Unlike a removal, a kick leaves the user in the set: they are still on + // the inbound, so a removal later still has to report them. + if got := r.CloseUsers(keep()); got != (Result{Unclosable: 1}) { + t.Errorf("CloseUsers = %+v after a kick, want {Unclosable:1} -- the kick dropped a user who is still enabled", got) } } -func TestSweepIsRateLimited(t *testing.T) { +// The other shape a kick can fail to reach: an entry that was tracked but +// carries no closer (a packet conn addressed to the mux destination). It has to +// be reported for the same reason the seen set is. +func TestKickReportsATrackedEntryWithNoCloser(t *testing.T) { r := NewRegistry() - r.Bind("old", "1.2.3.4:1000") - - r.access.Lock() - r.sources["1.2.3.4:1000"].lastSeen = time.Now().Add(-idleTimeout - time.Minute) - r.access.Unlock() - - // lastSweep is fresh (NewRegistry set it), so this Bind must not walk the - // map -- the whole point is that the data plane does not pay per call. - r.Bind("fresh", "5.6.7.8:2000") - - r.access.Lock() - _, stale := r.sources["1.2.3.4:1000"] - r.access.Unlock() + r.BindAndTrack("live", "1.2.3.4:1000", nil) - if !stale { - t.Error("sweep ran on a Bind inside the rate limit window") + got := r.KickUserSessions("live") + if got != (Result{Unclosable: 1}) { + t.Fatalf("KickUserSessions = %+v, want {Unclosable:1}", got) } } -func TestRejectReportsClose(t *testing.T) { +func TestKickIgnoresOtherUsers(t *testing.T) { + r := NewRegistry() conn := &fakeCloser{} - var reported error - Reject(conn, func(err error) { reported = err }) + r.BindAndTrack("bystander", "1.2.3.4:1000", conn) + r.Bind("other", "5.6.7.8:2000") - if !conn.closed { - t.Error("Reject must close the connection") + got := r.KickUserSessions("noisy") + if got != (Result{}) { + t.Fatalf("KickUserSessions = %+v, want an empty result", got) } - if !errors.Is(reported, ErrRemoved) { - t.Errorf("close handler got %v, want ErrRemoved", reported) - } -} - -func TestRejectWithoutCloseHandler(t *testing.T) { - conn := &fakeCloser{} - Reject(conn, nil) - if !conn.closed { - t.Error("Reject must close the connection even with no close handler") + if conn.closed { + t.Error("a kick must not touch another user's session") } } -// A tracked session is alive however quiet it has been: lastSeen only moves -// when it opens another connection, so one carrying a single long-lived stream -// looks idle. Sweeping it would discard the closer, and the removal that came -// next would find nothing to cut -- which is issue #175 all over again. -func TestSweepKeepsTrackedSession(t *testing.T) { +func TestUntrackForgetsSession(t *testing.T) { r := NewRegistry() conn := &fakeCloser{} - r.BindAndTrack("quiet", "1.2.3.4:1000", conn) - - r.access.Lock() - r.sources["1.2.3.4:1000"].lastSeen = time.Now().Add(-idleTimeout - time.Minute) - r.lastSweep = time.Now().Add(-idleTimeout - time.Minute) - r.access.Unlock() - - // Another client's traffic, which is what triggers the sweep. - r.Bind("other", "5.6.7.8:2000") + r.Track("1.2.3.4:1000", conn) + r.Bind("gone", "1.2.3.4:1000") + r.Untrack("1.2.3.4:1000") - if cut := r.CloseUsers(keep("other")); cut != 1 { - t.Fatalf("cut = %d, want 1 -- the idle session was swept away", cut) + got := r.CloseUsers(keep()) + if got != (Result{}) { + t.Fatalf("CloseUsers = %+v, want an empty result -- the session was already gone", got) } - if !conn.closed { - t.Error("a tracked session must survive the sweep and stay closable") + if conn.closed { + t.Error("an untracked session must not be closed again") } } -// Same defect, second site: CloseUsers drops idle entries before it decides -// what to cut, so an idle carrier would be skipped by the very call meant to -// close it. -func TestCloseUsersCutsIdleTrackedSession(t *testing.T) { +// A tracked session is alive however quiet it has been -- it carries a single +// long-lived stream, an ssh session or a download, and opens nothing new. It +// must stay closable, which is why nothing ages sources out: Untrack is the +// only thing that removes an entry, and it runs when the session really ends. +func TestQuietTrackedSessionStaysClosable(t *testing.T) { r := NewRegistry() conn := &fakeCloser{} r.BindAndTrack("quiet", "1.2.3.4:1000", conn) - r.access.Lock() - r.sources["1.2.3.4:1000"].lastSeen = time.Now().Add(-idleTimeout - time.Minute) - r.access.Unlock() + // Plenty of unrelated activity while "quiet" routes nothing. + for i := 0; i < 500; i++ { + r.Bind("busy", "5.6.7.8:"+strconv.Itoa(2000+i)) + r.CloseUsers(keep("quiet", "busy")) + } - if cut := r.CloseUsers(keep()); cut != 1 { - t.Fatalf("cut = %d, want 1 -- an idle tracked session was skipped", cut) + got := r.CloseUsers(keep("busy")) + if got != (Result{Cut: 1}) { + t.Fatalf("CloseUsers = %+v, want {Cut:1} -- the quiet session was lost", got) } if !conn.closed { - t.Error("an idle but tracked session must still be closed") + t.Error("a quiet tracked session must still be closable") } } -// This pins the outcome -- a carrier registered this way is closed, not muted. +// This pins the outcome -- a carrier registered this way is cut, not reported. // The atomicity it exists for cannot be asserted here: it comes from doing both // writes under one lock, and a sequential Bind-then-Track would pass this test // just the same. What that split would lose is the interleaving where a -// CloseUsers lands between the two, sees a user with no closer yet, files the -// session as unclosable and mutes it -- and a mux carrier is never gated, so -// the mute would do nothing. -func TestBindAndTrackClosesRatherThanMutes(t *testing.T) { +// CloseUsers lands between the two, sees a user with no session yet, and puts +// them in the seen set -- costing a mux inbound a restart it never needed. +func TestBindAndTrackCutsRatherThanReports(t *testing.T) { r := NewRegistry() conn := &fakeCloser{} r.BindAndTrack("live", "1.2.3.4:1000", conn) - if cut := r.CloseUsers(keep()); cut != 1 { - t.Fatalf("cut = %d, want 1", cut) + got := r.CloseUsers(keep()) + if got != (Result{Cut: 1}) { + t.Fatalf("CloseUsers = %+v, want {Cut:1}", got) } if !conn.closed { - t.Error("the carrier must be closed, not muted") - } - if !r.Allowed("1.2.3.4:1000") { - t.Error("a closed carrier must not leave a mute behind") + t.Error("the carrier must be closed") } } // A packet conn cannot be a mux carrier, so the router passes a nil closer. It // must stay nil: a nil net.Conn placed in an io.Closer is not a nil interface, // and would register as a closer that panics when the session is cut. -func TestBindAndTrackWithNilCloserMutes(t *testing.T) { +func TestBindAndTrackWithNilCloserReportsUnclosable(t *testing.T) { r := NewRegistry() r.BindAndTrack("live", "1.2.3.4:1000", nil) - if cut := r.CloseUsers(keep()); cut != 1 { - t.Fatalf("cut = %d, want 1", cut) - } - if r.Allowed("1.2.3.4:1000") { - t.Error("with no closer the session must be muted") - } -} - -// The counterpart to the one above, and the defect it was hiding: a client that -// keeps retrying is a session that is still alive -- which for QUIC means still -// authenticated, because the streams it opens never re-check the user table. -// The mute must outlast the retries, however long they go on. -// -// The sweep is the second road to the same place: Allowed has to keep the entry -// looking live, or the sweep drops it and takes the block with it. -func TestRemovalMuteSurvivesTheSweepWhileClientRetries(t *testing.T) { - r := NewRegistry() - r.Bind("gone", "1.2.3.4:1000") - r.CloseUsers(keep()) - - // Age the entry as though it had gone quiet long ago. - r.access.Lock() - r.sources["1.2.3.4:1000"].lastSeen = time.Now().Add(-idleTimeout - time.Minute) - r.lastSweep = time.Now().Add(-idleTimeout - time.Minute) - r.access.Unlock() - - // But the client is in fact still hammering: one refused attempt is enough - // to mark the source live again. - if r.Allowed("1.2.3.4:1000") { - t.Fatal("precondition: the source should still be muted") - } - - // Someone else's traffic, which is what triggers the sweep. - r.Bind("other", "5.6.7.8:2000") - - if r.Allowed("1.2.3.4:1000") { - t.Error("the sweep lifted a mute whose client is still trying") - } -} - -// A kick assumes the user may come back; a removal does not. Applying a kick on -// top of a removal must not shorten it to the kick window. -func TestKickDoesNotDowngradeARemoval(t *testing.T) { - r := NewRegistry() - r.Bind("gone", "1.2.3.4:1000") - r.CloseUsers(keep()) - r.KickUserSessions("gone") - - // Past the kick window, nowhere near the removal one. - r.access.Lock() - r.blocked["1.2.3.4:1000"].lastAttempt = time.Now().Add(-kickQuietWindow - time.Second) - r.access.Unlock() - - if r.Allowed("1.2.3.4:1000") { - t.Error("a kick downgraded a removal to the short window") + got := r.CloseUsers(keep()) + if got != (Result{Unclosable: 1}) { + t.Fatalf("CloseUsers = %+v, want {Unclosable:1}", got) } } -// The reverse: a kick is a decision about a user who is still enabled, so it -// must survive an unrelated save that happens to list them in keep. -func TestUnrelatedSaveDoesNotClearAKick(t *testing.T) { +// Sessions are counted one by one because each is a separate transport to +// close; users without one are counted once, because one restart ends all of +// theirs at the same time. +func TestCloseUsersCountsSessionsAndUsers(t *testing.T) { r := NewRegistry() - r.Bind("live", "1.2.3.4:1000") - r.KickUserSessions("live") - if r.Allowed("1.2.3.4:1000") { - t.Fatal("precondition: the kicked source should be muted") - } - - // Another client is saved; "live" is still enabled, so it is in keep. - r.CloseUsers(keep("live", "other")) - - if r.Allowed("1.2.3.4:1000") { - t.Error("an unrelated save cleared the operator's kick") + first := &fakeCloser{} + second := &fakeCloser{} + r.BindAndTrack("gone", "1.2.3.4:1000", first) + r.BindAndTrack("gone", "1.2.3.4:1001", second) + r.Bind("gone", "1.2.3.4:1002") + r.Bind("gone", "1.2.3.4:1003") + + got := r.CloseUsers(keep()) + if got != (Result{Cut: 2, Unclosable: 1}) { + t.Fatalf("CloseUsers = %+v, want {Cut:2 Unclosable:1}", got) + } + if !first.closed || !second.closed { + t.Error("both tracked sessions must be cut") } } diff --git a/core/usersession/router.go b/core/usersession/router.go index 6765ef12..6ea98331 100644 --- a/core/usersession/router.go +++ b/core/usersession/router.go @@ -12,12 +12,14 @@ import ( ) // The registry is reached by wrapping an inbound's router rather than by giving -// each inbound a field and a hook in every handler. Both put the gate in the -// same place -- before anything is routed, so the connections the router -// answers itself (hijack-dns) are covered and nothing is dialed before the -// refusal -- but the copies under core/protocol/ are diffed against sing-box -// line for line by scripts/check-protocol-copies.sh, and this way each one -// carries a single added line instead of a block in every handler. +// each inbound a field and a hook in every handler. Both see the same +// connections at the same point, but the copies under core/protocol/ are diffed +// against sing-box line for line by scripts/check-protocol-copies.sh, and this +// way each one carries a single added line instead of a block in every handler. +// +// Nothing here refuses anything: a wrapped router records what it sees and +// passes it on. Refusing by source address is what the package used to do and +// could not be made correct -- see the package comment. // // Every copy assigns metadata.User before calling the router, so the user a // connection authenticated as is already known here. @@ -28,22 +30,24 @@ import ( type Mode int const ( - // GateAndBind refuses connections from a muted session and records the user - // behind every other one. For the QUIC protocols the session itself has no - // closer this package can reach, so muting is the only thing that stops a - // removed user, and the gate is what does the work. - GateAndBind Mode = iota + // BindOnly records which user is connected from where, and nothing else. + // + // The QUIC inbounds take this because they have no session handle to give: + // what the registry does for them is answer, at removal time, whether the + // user being removed is connected at all -- which is what decides between + // a free in-place update and rebuilding the inbound. + // + // anytls takes it too, for the opposite reason: it does have a closer, but + // it holds it in NewConnection, which the router never sees, so it + // registers the session there instead. + BindOnly Mode = iota // TrackMuxCarrier ignores everything but a sing-mux carrier connection. // These protocols authenticate per connection, so a removed user is already // locked out of new ones and ConnTracker closes the routed ones -- except // on a multiplex session, where the carrier authenticated once and every // stream after that rides it. The carrier is a net.Conn we can close, so it - // is tracked and cut rather than muted. + // is tracked and cut. TrackMuxCarrier - // BindOnly records the user and nothing else, for an inbound that reaches - // its session by another route. anytls holds its session in NewConnection, - // which never goes through the router. - BindOnly ) type hooks struct { @@ -51,16 +55,15 @@ type hooks struct { mode Mode } -// enter applies the mode to one connection on its way to the real router. It -// reports whether the connection may proceed, and returns the cleanup to run -// once the router is done with it. -func (h hooks) enter(conn io.Closer, metadata adapter.InboundContext, onClose N.CloseHandlerFunc) (bool, func()) { - source := metadata.Source.String() +// enter records one connection on its way to the real router, and returns the +// cleanup to run once the router is done with it, or nil if there is none. +func (h hooks) enter(conn io.Closer, metadata adapter.InboundContext) func() { switch h.mode { case TrackMuxCarrier: if metadata.Destination != singmux.Destination { - return true, nil + return nil } + source := metadata.Source.String() // Only a stream-oriented carrier can be closed; a packet conn reaching // here would not be a mux carrier anyway. Assigned through a nil-able // io.Closer rather than passed directly: a nil net.Conn put into an @@ -74,17 +77,10 @@ func (h hooks) enter(conn io.Closer, metadata adapter.InboundContext, onClose N. h.registry.BindAndTrack(metadata.User, source, carrier) // The router blocks for as long as the multiplex session lives, so // untracking when it returns is not early. - return true, func() { h.registry.Untrack(source) } - case BindOnly: - h.registry.Bind(metadata.User, source) - return true, nil + return func() { h.registry.Untrack(source) } default: - if !h.registry.Allowed(source) { - Reject(conn, onClose) - return false, nil - } - h.registry.Bind(metadata.User, source) - return true, nil + h.registry.Bind(metadata.User, metadata.Source.String()) + return nil } } @@ -108,47 +104,30 @@ func (r *RouterEx) Registry() *Registry { } func (r *RouterEx) RouteConnectionEx(ctx context.Context, conn net.Conn, metadata adapter.InboundContext, onClose N.CloseHandlerFunc) { - proceed, done := r.enter(conn, metadata, onClose) - if !proceed { - return - } - if done != nil { + if done := r.enter(conn, metadata); done != nil { defer done() } r.ConnectionRouterEx.RouteConnectionEx(ctx, conn, metadata, onClose) } func (r *RouterEx) RoutePacketConnectionEx(ctx context.Context, conn N.PacketConn, metadata adapter.InboundContext, onClose N.CloseHandlerFunc) { - proceed, done := r.enter(conn, metadata, onClose) - if !proceed { - return - } - if done != nil { + if done := r.enter(conn, metadata); done != nil { defer done() } r.ConnectionRouterEx.RoutePacketConnectionEx(ctx, conn, metadata, onClose) } -// The deprecated pair is overridden too, not for completeness: shadowsocks' -// MultiInbound still routes through it, and leaving it to the embedded router -// would let those connections past the hooks without a word. +// The deprecated pair is overridden too, so that a transport still routing +// through it cannot slip a session past the hooks unrecorded. func (r *RouterEx) RouteConnection(ctx context.Context, conn net.Conn, metadata adapter.InboundContext) error { - proceed, done := r.enter(conn, metadata, nil) - if !proceed { - return ErrRemoved - } - if done != nil { + if done := r.enter(conn, metadata); done != nil { defer done() } return r.ConnectionRouterEx.RouteConnection(ctx, conn, metadata) } func (r *RouterEx) RoutePacketConnection(ctx context.Context, conn N.PacketConn, metadata adapter.InboundContext) error { - proceed, done := r.enter(conn, metadata, nil) - if !proceed { - return ErrRemoved - } - if done != nil { + if done := r.enter(conn, metadata); done != nil { defer done() } return r.ConnectionRouterEx.RoutePacketConnection(ctx, conn, metadata) @@ -175,44 +154,28 @@ func (r *Router) Registry() *Registry { } func (r *Router) RouteConnectionEx(ctx context.Context, conn net.Conn, metadata adapter.InboundContext, onClose N.CloseHandlerFunc) { - proceed, done := r.enter(conn, metadata, onClose) - if !proceed { - return - } - if done != nil { + if done := r.enter(conn, metadata); done != nil { defer done() } r.Router.RouteConnectionEx(ctx, conn, metadata, onClose) } func (r *Router) RoutePacketConnectionEx(ctx context.Context, conn N.PacketConn, metadata adapter.InboundContext, onClose N.CloseHandlerFunc) { - proceed, done := r.enter(conn, metadata, onClose) - if !proceed { - return - } - if done != nil { + if done := r.enter(conn, metadata); done != nil { defer done() } r.Router.RoutePacketConnectionEx(ctx, conn, metadata, onClose) } func (r *Router) RouteConnection(ctx context.Context, conn net.Conn, metadata adapter.InboundContext) error { - proceed, done := r.enter(conn, metadata, nil) - if !proceed { - return ErrRemoved - } - if done != nil { + if done := r.enter(conn, metadata); done != nil { defer done() } return r.Router.RouteConnection(ctx, conn, metadata) } func (r *Router) RoutePacketConnection(ctx context.Context, conn N.PacketConn, metadata adapter.InboundContext) error { - proceed, done := r.enter(conn, metadata, nil) - if !proceed { - return ErrRemoved - } - if done != nil { + if done := r.enter(conn, metadata); done != nil { defer done() } return r.Router.RoutePacketConnection(ctx, conn, metadata) diff --git a/core/usersession/router_test.go b/core/usersession/router_test.go index 48a5e4cf..9ae33459 100644 --- a/core/usersession/router_test.go +++ b/core/usersession/router_test.go @@ -2,12 +2,14 @@ package usersession import ( "context" + "io" "net" "os" "testing" "time" "github.com/sagernet/sing-box/adapter" + "github.com/sagernet/sing/common/buf" M "github.com/sagernet/sing/common/metadata" N "github.com/sagernet/sing/common/network" @@ -50,6 +52,22 @@ func (r *fakeRouter) RoutePacketConnectionEx(ctx context.Context, conn N.PacketC r.enter(metadata) } +// fakePacketConn is the smallest thing that satisfies N.PacketConn. Only Close +// is ever reached; the rest exist to satisfy the interface. +type fakePacketConn struct { + closed bool +} + +func (c *fakePacketConn) ReadPacket(buffer *buf.Buffer) (M.Socksaddr, error) { + return M.Socksaddr{}, io.EOF +} +func (c *fakePacketConn) WritePacket(buffer *buf.Buffer, destination M.Socksaddr) error { return nil } +func (c *fakePacketConn) Close() error { c.closed = true; return nil } +func (c *fakePacketConn) LocalAddr() net.Addr { return M.Socksaddr{} } +func (c *fakePacketConn) SetDeadline(t time.Time) error { return nil } +func (c *fakePacketConn) SetReadDeadline(t time.Time) error { return nil } +func (c *fakePacketConn) SetWriteDeadline(t time.Time) error { return nil } + func metadataFor(user string, source string, destination M.Socksaddr) adapter.InboundContext { return adapter.InboundContext{ User: user, @@ -78,34 +96,51 @@ func closed(t *testing.T, peer net.Conn) bool { return true } -func TestGateRefusesMutedSource(t *testing.T) { - next := &fakeRouter{} - router := WrapRouterEx(next, GateAndBind) - metadata := metadataFor("gone", "1.2.3.4:1000", plainDestination) - - // Learn the user, then remove them: with no closer, this mutes the source. - router.Registry().Bind("gone", "1.2.3.4:1000") - router.Registry().CloseUsers(KeepSet(nil)) - - conn, peer := net.Pipe() - defer peer.Close() - var reported error - router.RouteConnectionEx(context.Background(), conn, metadata, func(err error) { reported = err }) - - if next.calls != 0 { - t.Error("a muted source must not reach the router") - } - if !closed(t, peer) { - t.Error("the refused connection must be closed") - } - if reported != ErrRemoved { - t.Errorf("close handler got %v, want ErrRemoved", reported) +// Nothing here refuses a connection, in any mode: refusing by source address is +// what this package used to do and could not be made correct. Every mode is +// asserted against that below -- a gate creeping back in is the regression this +// file exists to catch. +func TestNoModeEverRefuses(t *testing.T) { + for _, mode := range []struct { + name string + mode Mode + }{ + {"BindOnly", BindOnly}, + {"TrackMuxCarrier", TrackMuxCarrier}, + } { + t.Run(mode.name, func(t *testing.T) { + next := &fakeRouter{} + router := WrapRouterEx(next, mode.mode) + // Learn a user, then remove them. Whatever the registry made of + // that, the next connection from the same address still goes + // through: for TrackMuxCarrier because trojan's fallback shares + // this router, and for BindOnly because the address may by now + // belong to somebody else entirely. + router.Registry().Bind("gone", "1.2.3.4:1000") + router.Registry().CloseUsers(KeepSet(nil)) + + conn, peer := net.Pipe() + defer conn.Close() + defer peer.Close() + var reported error + router.RouteConnectionEx(context.Background(), conn, metadataFor("gone", "1.2.3.4:1000", plainDestination), func(err error) { reported = err }) + + if next.calls != 1 { + t.Errorf("router calls = %d, want 1 -- the connection was refused", next.calls) + } + if reported != nil { + t.Errorf("close handler got %v, want nothing", reported) + } + if closed(t, peer) { + t.Error("the connection was closed") + } + }) } } -func TestGateBindsAndForwards(t *testing.T) { +func TestBindOnlyRecordsTheUser(t *testing.T) { next := &fakeRouter{} - router := WrapRouterEx(next, GateAndBind) + router := WrapRouterEx(next, BindOnly) conn, peer := net.Pipe() defer conn.Close() @@ -115,30 +150,37 @@ func TestGateBindsAndForwards(t *testing.T) { if next.calls != 1 { t.Fatalf("router calls = %d, want 1", next.calls) } - // The bind is what makes the next removal able to find this session. - if cut := router.Registry().CloseUsers(KeepSet(nil)); cut != 1 { - t.Errorf("cut = %d, want 1 -- the connection was not bound to its user", cut) + // Bound, so a removal knows this user is connected -- but with no closer, + // because the session this connection belongs to is either a QUIC one + // (unreachable) or an anytls one registered elsewhere. + got := router.Registry().CloseUsers(KeepSet(nil)) + if got != (Result{Unclosable: 1}) { + t.Errorf("CloseUsers = %+v, want {Unclosable:1}", got) + } + if closed(t, peer) { + t.Error("BindOnly must not close the connection it saw") } } -// The deprecated pair is what shadowsocks' MultiInbound still routes through. -// Leaving it to the embedded router would let those connections past the hooks -// without a word, so it gets the same coverage as the Ex form. -func TestGateCoversDeprecatedRoutePath(t *testing.T) { +// The deprecated pair is overridden so a transport still routing through it +// cannot slip a session past unrecorded. +func TestDeprecatedRoutePathIsRecorded(t *testing.T) { next := &fakeRouter{} - router := WrapRouterEx(next, GateAndBind) - router.Registry().Bind("gone", "1.2.3.4:1000") - router.Registry().CloseUsers(KeepSet(nil)) + router := WrapRouterEx(next, BindOnly) conn, peer := net.Pipe() + defer conn.Close() defer peer.Close() - err := router.RouteConnection(context.Background(), conn, metadataFor("gone", "1.2.3.4:1000", plainDestination)) - - if next.calls != 0 { - t.Error("a muted source must not reach the router on the deprecated path either") + err := router.RouteConnection(context.Background(), conn, metadataFor("live", "1.2.3.4:1000", plainDestination)) + if err != nil { + t.Fatalf("RouteConnection returned %v", err) + } + if next.calls != 1 { + t.Fatalf("router calls = %d, want 1", next.calls) } - if err != ErrRemoved { - t.Errorf("RouteConnection returned %v, want ErrRemoved", err) + got := router.Registry().CloseUsers(KeepSet(nil)) + if got != (Result{Unclosable: 1}) { + t.Errorf("CloseUsers = %+v, want {Unclosable:1} -- the deprecated path did not record the user", got) } } @@ -149,7 +191,7 @@ func TestMuxCarrierIsTrackedForTheRoutersLifetime(t *testing.T) { next.whileIn = func() { // The router blocks for as long as the multiplex session lives, so the // carrier has to be closable right here -- that is the whole point. - trackedDuring = router.Registry().KickUserSessions("live") == 1 + trackedDuring = router.Registry().KickUserSessions("live") == (Result{Cut: 1}) } conn, peer := net.Pipe() @@ -162,67 +204,78 @@ func TestMuxCarrierIsTrackedForTheRoutersLifetime(t *testing.T) { if !closed(t, peer) { t.Error("kicking the user must have closed the carrier") } - // And it is forgotten once the session is over. - if kicked := router.Registry().KickUserSessions("live"); kicked != 0 { - t.Errorf("kicked = %d after the session ended, want 0", kicked) - } } -func TestMuxModeIgnoresPlainConnections(t *testing.T) { +// The carrier is forgotten once its session ends. Left behind, the entry +// outlives the session it names, and the next removal cuts a connection that +// is not there any more -- or, on an inbound that reports instead, asks for a +// restart on behalf of a client that already left. +// +// Kicking first (as the test above does) deletes the entry by itself, so this +// has to be its own case to mean anything. +func TestMuxCarrierIsUntrackedWhenTheSessionEnds(t *testing.T) { next := &fakeRouter{} router := WrapRouterEx(next, TrackMuxCarrier) conn, peer := net.Pipe() defer conn.Close() defer peer.Close() - router.RouteConnectionEx(context.Background(), conn, metadataFor("live", "1.2.3.4:1000", plainDestination), nil) + // fakeRouter returns immediately, which stands in for the session ending. + router.RouteConnectionEx(context.Background(), conn, metadataFor("live", "1.2.3.4:1000", singmux.Destination), nil) - if next.calls != 1 { - t.Fatalf("router calls = %d, want 1", next.calls) - } - // A plain connection authenticates on its own and ConnTracker closes it, so - // it has no business in the registry. - if cut := router.Registry().CloseUsers(KeepSet(nil)); cut != 0 { - t.Errorf("cut = %d, want 0 -- a plain connection was registered", cut) + got := router.Registry().CloseUsers(KeepSet(nil)) + if got != (Result{}) { + t.Errorf("CloseUsers = %+v, want an empty result -- the carrier outlived its session", got) } } -// trojan routes unauthenticated fallback traffic through this same router, so -// this mode must never refuse anything. -func TestMuxModeDoesNotGate(t *testing.T) { +func TestMuxModeIgnoresPlainConnections(t *testing.T) { next := &fakeRouter{} router := WrapRouterEx(next, TrackMuxCarrier) - router.Registry().Bind("gone", "1.2.3.4:1000") - router.Registry().CloseUsers(KeepSet(nil)) + var registeredDuring Result + next.whileIn = func() { + // Asked while the router still has the connection. Asking afterwards + // proves nothing: the deferred Untrack empties the registry either way, + // so the assertion would hold even if every connection were registered. + registeredDuring = router.Registry().CloseUsers(KeepSet(nil)) + } conn, peer := net.Pipe() defer conn.Close() defer peer.Close() - router.RouteConnectionEx(context.Background(), conn, metadataFor("", "1.2.3.4:1000", plainDestination), nil) + router.RouteConnectionEx(context.Background(), conn, metadataFor("live", "1.2.3.4:1000", plainDestination), nil) if next.calls != 1 { - t.Error("the mux mode must not refuse a connection, fallback traffic shares this router") + t.Fatalf("router calls = %d, want 1", next.calls) + } + // A plain connection authenticates on its own and ConnTracker closes it, so + // it has no business in the registry. + if registeredDuring != (Result{}) { + t.Errorf("CloseUsers = %+v mid-route, want an empty result -- a plain connection was registered", registeredDuring) + } + if closed(t, peer) { + t.Error("a plain connection must not be closed by this layer") } } -func TestBindOnlyNeitherGatesNorTracks(t *testing.T) { +// A packet conn cannot be a mux carrier. If one is registered as though it +// were, a removal closes it -- so the carrier slot has to stay empty, which is +// also what keeps a nil net.Conn out of a non-nil io.Closer. +func TestMuxModeDoesNotTakeAPacketConnAsCarrier(t *testing.T) { next := &fakeRouter{} - router := WrapRouterEx(next, BindOnly) + router := WrapRouterEx(next, TrackMuxCarrier) + conn := &fakePacketConn{} + var registeredDuring Result + next.whileIn = func() { + registeredDuring = router.Registry().CloseUsers(KeepSet(nil)) + } - conn, peer := net.Pipe() - defer conn.Close() - defer peer.Close() - router.RouteConnectionEx(context.Background(), conn, metadataFor("live", "1.2.3.4:1000", plainDestination), nil) + router.RoutePacketConnectionEx(context.Background(), conn, metadataFor("live", "1.2.3.4:1000", singmux.Destination), nil) - if next.calls != 1 { - t.Fatalf("router calls = %d, want 1", next.calls) + if registeredDuring != (Result{Unclosable: 1}) { + t.Errorf("CloseUsers = %+v mid-route, want {Unclosable:1}", registeredDuring) } - // Bound, so a removal finds it -- but muted rather than closed, because - // anytls registers the closable session elsewhere. - if cut := router.Registry().CloseUsers(KeepSet(nil)); cut != 1 { - t.Errorf("cut = %d, want 1", cut) - } - if closed(t, peer) { - t.Error("BindOnly must not close the connection it saw") + if conn.closed { + t.Error("a packet conn was closed as though it were a mux carrier") } } diff --git a/core/usersession/stress_test.go b/core/usersession/stress_test.go new file mode 100644 index 00000000..60cda6e3 --- /dev/null +++ b/core/usersession/stress_test.go @@ -0,0 +1,58 @@ +package usersession + +import ( + "strconv" + "sync" + "testing" +) + +// Every exported method driven at once from many goroutines. The point is the +// race detector, not the assertions: CloseUsers closes outside the lock and +// Untrack comes back in on the closer's own goroutine, so the two have to be +// safe against each other. Run with -race to mean anything. +func TestRegistryUnderConcurrency(t *testing.T) { + r := NewRegistry() + const workers = 8 + const rounds = 200 + + var wg sync.WaitGroup + for w := 0; w < workers; w++ { + wg.Add(1) + go func(w int) { + defer wg.Done() + user := "user" + strconv.Itoa(w%3) + for i := 0; i < rounds; i++ { + source := "10.0.0." + strconv.Itoa(w) + ":" + strconv.Itoa(1000+i%7) + switch i % 5 { + case 0: + r.Bind(user, source) + case 1: + r.BindAndTrack(user, source, &fakeCloser{}) + case 2: + r.Track(source, &fakeCloser{}) + r.Bind(user, source) + case 3: + r.CloseUsers(keep("user0")) + case 4: + r.KickUserSessions(user) + } + r.Untrack(source) + } + }(w) + } + wg.Wait() + + // Every source was untracked by its own goroutine, so nothing may be left. + r.access.Lock() + remaining := len(r.sources) + seen := len(r.seen) + r.access.Unlock() + if remaining != 0 { + t.Errorf("len(sources) = %d after every source was untracked, want 0", remaining) + } + // seen has no expiry, so what bounds it has to be the user count: three + // users across 8 goroutines and 1600 addresses. + if seen > 3 { + t.Errorf("len(seen) = %d, want at most 3 -- it is growing per address, not per user", seen) + } +} diff --git a/service/inbounds.go b/service/inbounds.go index 48ee9674..6bc94ef6 100644 --- a/service/inbounds.go +++ b/service/inbounds.go @@ -2,11 +2,13 @@ package service import ( "encoding/json" + "errors" "fmt" "os" "strconv" "strings" + "github.com/shenaba/2s-ui/core/usersession" "github.com/shenaba/2s-ui/database" "github.com/shenaba/2s-ui/database/model" "github.com/shenaba/2s-ui/logger" @@ -463,13 +465,22 @@ func (s *InboundService) UpdateInboundsUsers(tx *gorm.DB, ids []uint) error { return err } - // An in-place update that errors leaves the inbound running with its old + // Two different things arrive here as an error, and the restart below + // answers both. A real failure leaves the inbound running with its old // user table, so a removed user would stay connected and keep - // authenticating. Fall through to the restart rather than returning: - // dropping every connection on this inbound is the safe failure. + // authenticating -- dropping every connection on the inbound is the + // safe failure. ErrRestartRequired is the opposite: the table was + // swapped fine, and the inbound is asking to be rebuilt because a user + // we just removed holds a session it cannot close (a QUIC one; see + // core/usersession). Only the first is something going wrong, so only + // the first is logged as such. handled, err := corePtr.UpdateInboundUsers(inboundConfig) if err != nil { - logger.Warning("in-place user update failed for inbound ", inbound.Tag, ", restarting it: ", err) + if errors.Is(err, usersession.ErrRestartRequired) { + logger.Info("restarting inbound ", inbound.Tag, ": ", err) + } else { + logger.Warning("in-place user update failed for inbound ", inbound.Tag, ", restarting it: ", err) + } handled = false } if handled {