diff --git a/MobileGL/MG_Remote/Server/PreAuthGate.h b/MobileGL/MG_Remote/Server/PreAuthGate.h index d2ef9fdd..b7dce40e 100644 --- a/MobileGL/MG_Remote/Server/PreAuthGate.h +++ b/MobileGL/MG_Remote/Server/PreAuthGate.h @@ -32,11 +32,23 @@ // which that address is refused at accept, before a byte of it is read. // // What a failure IS, for the backoff: a Hello whose token did not match, a first frame that is not -// a well-formed control frame, and a connection that let the deadline pass. What it is NOT: a -// connection that closed before sending a byte (a port probe costs the server an accept and -// nothing else), a DataBind that names no live session (a stale data connection of a session that -// just ended is a legitimate client's), and a refusal the backoff itself issued (counting those -// would let anybody behind a shared address keep that address locked out for ever). +// a well-formed control frame, a connection that let the deadline pass, and a connection that HELD +// its pending slot for kPreAuthProbeGraceMs or longer and then closed (or was displaced, below) +// without completing a frame. What it is NOT: a connection that closed within kPreAuthProbeGraceMs +// of its accept without sending a byte (a port probe costs the server an accept and nothing else), +// a DataBind that names no live session (a stale data connection of a session that just ended is a +// legitimate client's), and a refusal the backoff itself issued (counting those would let anybody +// behind a shared address keep that address locked out for ever). +// +// Fix round (the f2-auth critic's reproduction). The first cut counted no silent close at all, and +// the pending slots were one pool for everybody: one address could open MOBILEGL_IPC_PREAUTH_MAX +// silent connections, close them just before the deadline, open them again, and every Hello from +// anywhere was refused Busy for as long as it liked - no failure counted, no backoff ever. Two +// rules close that: a silent connection that held its slot past the grace is a failure (so the +// holder is backed off after MOBILEGL_IPC_AUTH_BACKOFF_AFTER of them), and a full queue is SHARED +// (ChoosePendingToDisplace): a newcomer from an address that holds fewer slots than the busiest +// address displaces the busiest address's oldest pending connection, so no one address can keep +// another out. #pragma once @@ -73,6 +85,10 @@ namespace MobileGL::MG_Remote::Server { // A failure this old no longer counts: the operator who mistyped a token yesterday starts // from zero today. inline constexpr std::uint32_t kAuthBackoffForgetMs = 10 * 60 * 1000; + // A connection that closes (or is displaced) without a byte this soon after its accept is a + // port probe; one that held its slot longer is a failure. A real client writes its first frame + // the moment it connects, so it is never on the wrong side of this. + inline constexpr std::uint32_t kPreAuthProbeGraceMs = 250; // The refusal details a peer (and the log) sees. Named once so the supervisor and the unit test // cannot drift on the words; the TCP lane's scripts mirror them rather than parse them, so a @@ -85,6 +101,7 @@ namespace MobileGL::MG_Remote::Server { inline constexpr const char* kFirstFrameBadDataBind = "first frame is a DataBind without a 16-byte nonce"; inline constexpr const char* kPreAuthDeadline = "no authenticated first frame within the pre-auth deadline"; inline constexpr const char* kPreAuthFull = "too many connections are awaiting authentication"; + inline constexpr const char* kPreAuthDisplaced = "displaced from the pre-auth queue by a connection from another address"; inline constexpr const char* kAuthBackoffRefusal = "too many failed authentications from this address; retry later"; struct PreAuthKnobs { @@ -164,6 +181,28 @@ namespace MobileGL::MG_Remote::Server { return kFirstFrameNotAHello; } + // THE PENDING QUEUE IS SHARED BETWEEN ADDRESSES. Called when every pending slot is taken and a + // connection from `newcomer` has just been accepted. `pending` is the addresses of the pending + // connections, oldest first. The newcomer displaces the OLDEST pending connection of the + // address that holds the most slots, unless its own address already holds as many as that - + // then the newcomer is the one refused. Returns the index to displace, or pending.size() to + // refuse the newcomer. + // + // So an address holding nothing is always let in, whoever fills the queue; an address can only + // keep every slot while nobody else wants one; and among addresses with equal shares, the + // connection that has waited longest - the one furthest from being a client that writes its + // Hello on connect - goes first. + inline std::size_t ChoosePendingToDisplace(const std::vector& pending, const std::string& newcomer) { + std::unordered_map held; + std::size_t most = 0; + for (const auto& address : pending) most = std::max(most, ++held[address]); + const auto own = held.find(newcomer); + if (own != held.end() && own->second >= most) return pending.size(); + for (std::size_t index = 0; index < pending.size(); ++index) + if (held[pending[index]] == most) return index; + return pending.size(); + } + // Failures per peer address and the window each address is refused for. class AuthBackoff { public: diff --git a/MobileGL/MG_Remote/Server/ServerMain.cpp b/MobileGL/MG_Remote/Server/ServerMain.cpp index b778a4e7..6ea7665f 100644 --- a/MobileGL/MG_Remote/Server/ServerMain.cpp +++ b/MobileGL/MG_Remote/Server/ServerMain.cpp @@ -627,12 +627,15 @@ void LogSupervisorSummary(unsigned long sessionsFaulted) { // itself, a few bytes per wakeup, from all pending connections at once (one poll loop - no // peer can make it wait for another), and never a byte past the frame; // - a pending connection has MOBILEGL_IPC_PREAUTH_MS (default 2000) to complete it, and at most -// MOBILEGL_IPC_PREAUTH_MAX (default 8) are pending at once - one more is refused Busy at accept; +// MOBILEGL_IPC_PREAUTH_MAX (default 8) are pending at once. The queue is SHARED between +// addresses: when it is full a newcomer displaces the oldest connection of the address holding +// the most slots, and is itself refused Busy only when its own address already holds as many; // - a Hello is AUTHENTICATED HERE, token first; an unauthenticated one is answered // Refuse{Authentication} and nothing else. Only an authenticated Hello is forked for, and the // child is handed the frame it would otherwise have read; -// - a failure (wrong token, malformed first frame, deadline) is counted against the peer's -// address; after MOBILEGL_IPC_AUTH_BACKOFF_AFTER (default 5) of them that address is refused +// - a failure (wrong token, malformed first frame, deadline, a silent connection that held its +// slot past kPreAuthProbeGraceMs before it closed or was displaced) is counted against the +// peer's address; after MOBILEGL_IPC_AUTH_BACKOFF_AFTER (default 5) of them that address is refused // at accept, before a byte is read, for MOBILEGL_IPC_AUTH_BACKOFF_MS doubling per further // failure up to a minute. An authenticated Hello from the address clears it; // - a DataBind is routed exactly as PH-7 (4) routed it: to the live session's child over the @@ -690,8 +693,14 @@ public: NoteFailure(peer.address, "not a control frame"); continue; case Step::EndedEmpty: - // A port probe: it cost an accept and nothing else. Not a failure. + // A port probe - gone within the grace, having sent nothing - cost an + // accept and nothing else, and is not a failure. A connection that HELD + // its slot past the grace and then left without a byte is how one + // address kept every slot taken without ever reaching the deadline, + // so that one counts (fix round; PreAuthGate.h). ::close(peer.fd); + if (now - peer.accepted >= std::chrono::milliseconds(Server::kPreAuthProbeGraceMs)) + NoteFailure(peer.address, "held a pre-auth slot and closed without a byte"); continue; case Step::EndedPartial: RefuseAndClose(peer.fd, Protocol::RefuseCode::MalformedHello, Server::kFirstFrameTruncated); @@ -722,6 +731,7 @@ private: struct PendingPeer { int fd = -1; std::string address; + Clock::time_point accepted{}; Clock::time_point deadline{}; Server::FirstFrameAssembler frame; }; @@ -766,14 +776,34 @@ private: continue; } if (m_pending.size() >= m_knobs.maxPending) { - WireLogError("MG_Remote server: connection from %s refused at accept: %zu connections are " - "already awaiting authentication", address.c_str(), m_pending.size()); - RefuseAndClose(fd, Protocol::RefuseCode::Busy, Server::kPreAuthFull); - continue; + // Full. The queue is shared between addresses (PreAuthGate.h + // ChoosePendingToDisplace): the newcomer takes the busiest address's oldest slot, + // unless its own address is the busiest - then it is the one refused. + std::vector holders; + holders.reserve(m_pending.size()); + for (const auto& peer : m_pending) holders.push_back(peer.address); + const std::size_t victim = Server::ChoosePendingToDisplace(holders, address); + if (victim >= m_pending.size()) { + WireLogError("MG_Remote server: connection from %s refused at accept: %zu connections are " + "already awaiting authentication", address.c_str(), m_pending.size()); + RefuseAndClose(fd, Protocol::RefuseCode::Busy, Server::kPreAuthFull); + continue; + } + PendingPeer displaced = std::move(m_pending[victim]); + m_pending.erase(m_pending.begin() + static_cast(victim)); + WireLogError("MG_Remote server: pending connection from %s displaced by one from %s: the pre-auth " + "queue is full and %s holds the most of it", displaced.address.c_str(), address.c_str(), + displaced.address.c_str()); + RefuseAndClose(displaced.fd, Protocol::RefuseCode::Busy, Server::kPreAuthDisplaced); + // A displaced connection that had already held its slot past the grace is a silent + // holder like one that closed; otherwise displacement would be a free way out. + if (now - displaced.accepted >= std::chrono::milliseconds(Server::kPreAuthProbeGraceMs)) + NoteFailure(displaced.address, "held a pre-auth slot until displaced"); } PendingPeer peer; peer.fd = fd; peer.address = std::move(address); + peer.accepted = now; peer.deadline = now + std::chrono::milliseconds(m_knobs.deadlineMs); m_pending.push_back(std::move(peer)); } diff --git a/MobileGL/MG_Test/Wire/PreAuthGateTest.cpp b/MobileGL/MG_Test/Wire/PreAuthGateTest.cpp index 3ce8a07b..ddfc5e0d 100644 --- a/MobileGL/MG_Test/Wire/PreAuthGateTest.cpp +++ b/MobileGL/MG_Test/Wire/PreAuthGateTest.cpp @@ -15,7 +15,7 @@ // TcpLane.ControlFrameFuzz (scripts/ci/ph_fuzz_control_frames.py). What is pinned here is what // those scripts cannot see from outside: that each malformation lands on the shape it is named // for (and not merely on SOME refusal), that the assembler never reads a byte past the first -// frame, and the backoff's arithmetic. +// frame, the backoff's arithmetic, and which pending connection a full queue gives up. #include #include @@ -246,6 +246,34 @@ TEST(PreAuthGateTest, TheBackoffWindowStartsAtTheThresholdDoublesAndIsCapped) { EXPECT_LE(backoff.Tracked(), Server::kAuthBackoffTrackedPeers); } +// Fix round: the full pending queue is shared between addresses. One address that holds every slot +// keeps out nobody but itself; the busiest address's OLDEST connection is the one displaced. Red +// once by making ChoosePendingToDisplace always refuse the newcomer (the first cut's single pool): +// the first EXPECT_EQ names it. +TEST(PreAuthGateTest, AFullQueueIsSharedBetweenAddresses) { + const std::string a = "192.0.2.1", b = "192.0.2.2", c = "192.0.2.3", d = "192.0.2.4"; + // One address holds all four slots: anybody else displaces its oldest; it cannot add a fifth. + const std::vector oneHolder = {a, a, a, a}; + EXPECT_EQ(Server::ChoosePendingToDisplace(oneHolder, b), 0u); + EXPECT_EQ(Server::ChoosePendingToDisplace(oneHolder, a), oneHolder.size()); + // The busiest address loses its oldest connection, wherever that sits in the queue. + const std::vector mixed = {b, a, c, a}; + EXPECT_EQ(Server::ChoosePendingToDisplace(mixed, d), 1u); + // An address with fewer slots than the busiest still gets one; one with as many does not. + EXPECT_EQ(Server::ChoosePendingToDisplace(mixed, b), 1u); + EXPECT_EQ(Server::ChoosePendingToDisplace(mixed, a), mixed.size()); + // Equal shares: the connection that has waited longest goes first. + const std::vector even = {b, a, a, b}; + EXPECT_EQ(Server::ChoosePendingToDisplace(even, c), 0u); + EXPECT_EQ(Server::ChoosePendingToDisplace(even, a), even.size()); + // One slot each: a new address still gets in, by displacing the oldest. + const std::vector spread = {d, c, b, a}; + EXPECT_EQ(Server::ChoosePendingToDisplace(spread, "192.0.2.5"), 0u); + EXPECT_EQ(Server::ChoosePendingToDisplace(spread, a), spread.size()); + // Nothing pending: nothing to displace. + EXPECT_EQ(Server::ChoosePendingToDisplace({}, a), 0u); +} + // The knobs: defaults, values, and the one knob where 0 means something. TEST(PreAuthGateTest, TheKnobsReadTheirEnvironmentAndKeepTheirDefaultsOtherwise) { { diff --git a/scripts/ci/tcp_supervisor_smoke.py b/scripts/ci/tcp_supervisor_smoke.py index 56134bfb..c802868c 100644 --- a/scripts/ci/tcp_supervisor_smoke.py +++ b/scripts/ci/tcp_supervisor_smoke.py @@ -18,8 +18,10 @@ the controls here assert that an unauthenticated peer learns nothing of ours (`unauthenticated_wrong_layout`), that a silent one costs no fork and is answered at the pre-auth deadline (`silent_peer`), that Busy is the answer to an AUTHENTICATED second client (`busy`), the pending cap and the backoff (`pending_cap`, `auth_backoff`), and ID-P7-44's queued-DataBind -refusal (`data_bind_queued_before_bound`). The five malformed-frame shapes are fuzz arm 1's own -entry, scripts/ci/ph_fuzz_control_frames.py. +refusal (`data_bind_queued_before_bound`). The f2-auth fix round added that the pending queue is +shared between addresses (`pending_queue_is_shared`) and that an address holding slots with silent +connections is backed off (`silent_slot_holder_backed_off`). The five malformed-frame shapes are +fuzz arm 1's own entry, scripts/ci/ph_fuzz_control_frames.py. It needs flatc, which is deliberately absent from the default build graph (gen_protocol.py's header block says why). Resolution order is gen_protocol.py's, minus the build-it-for-you arm: @@ -255,14 +257,17 @@ def supervisor(server, log, same_build=False, serve=True, extra_env=None): child.wait(timeout=10) -def connect_control(port, seconds=5): +def connect_control(port, seconds=5, source=None): """A connection to the supervisor's port, retried on ECONNREFUSED for `seconds`: a supervisor just started may not have reached listen() yet, and a refused connect before it has is - startup, not a finding.""" + startup, not a finding. `source` binds the local end to another loopback address (127.0.0.2, + ...), which is how the pre-auth controls stand in for a second host: the supervisor keys its + backoff and its share of the pending queue by peer address.""" deadline = time.monotonic() + seconds while True: try: - return socket.create_connection(('127.0.0.1', port), timeout=3) + return socket.create_connection(('127.0.0.1', port), timeout=3, + source_address=(source, 0) if source else None) except ConnectionRefusedError: if time.monotonic() >= deadline: raise @@ -361,6 +366,7 @@ kDetailUnverifiable = 'first frame did not verify as a CtrlEnvelope' kDetailNotAHello = 'first control frame is not a verifiable Hello' kDetailDeadline = 'no authenticated first frame within the pre-auth deadline' kDetailPendingFull = 'too many connections are awaiting authentication' +kDetailDisplaced = 'displaced from the pre-auth queue by a connection from another address' kDetailBackoff = 'too many failed authentications from this address; retry later' kDetailHandoff = 'data connection could not be handed to the live session' @@ -815,19 +821,20 @@ def main(): # # A supervisor with small knobs, so both limits are reached in a few connections: at most # two connections pending authentication, and an address backed off after two failures, - # for 1500 ms. + # for 1500 ms. Each control below speaks from its OWN loopback address (connect_control's + # `source`), because the failures one control provokes are counted against the address it + # used, and a silent connection held past the probe grace is one of them. knobs = {'MOBILEGL_IPC_PREAUTH_MAX': '2', 'MOBILEGL_IPC_PREAUTH_MS': '3000', 'MOBILEGL_IPC_AUTH_BACKOFF_AFTER': '2', 'MOBILEGL_IPC_AUTH_BACKOFF_MS': '1500'} with supervisor(args.server, args.out / 'preauth-knobs.log', extra_env=knobs) as (port, process, serverLog): - # (a) THE CAP. Two silent connections fill it; the third is refused Busy AT ACCEPT, at - # once, instead of joining them. The two are then closed before their deadline - - # a close before any byte is a probe, not an authentication failure, so (b) below - # starts from zero. RED with the cap disabled: the third is not answered until its - # own deadline, and then as Authentication. - first, second = connect_control(port), connect_control(port) + # (a) THE CAP. Two silent connections from 127.0.0.3 fill it; a third from the SAME + # address is refused Busy AT ACCEPT, at once, instead of joining them - its address + # already holds the whole queue. RED with the cap disabled: the third is not + # answered until its own deadline, and then as Authentication. + first, second = connect_control(port, source='127.0.0.3'), connect_control(port, source='127.0.0.3') try: time.sleep(.3) # both accepted and pending - third = connect_control(port) + third = connect_control(port, source='127.0.0.3') try: third.settimeout(10) began = time.monotonic() @@ -843,6 +850,40 @@ def main(): evidence['pending_cap'] = {'refusal': full, 'answeredAfterSeconds': round(waited, 3)} time.sleep(.3) + # (a2) THE QUEUE IS SHARED (fix round). The first cut had one pool: one address that + # held every slot kept every other address out, for as long as it cared to. Now two + # silent connections from 127.0.0.2 fill the queue and a Hello from 127.0.0.1 is + # still JUDGED - it displaces 127.0.0.2's oldest connection, which is told so by + # name - and reaches a session child (an authenticated Hello with fingerprint 0 is + # answered Refuse{WireFingerprint} by the child, i.e. it was forked for). RED with a + # single pool: the Hello is refused Busy "too many connections are awaiting + # authentication". + holders = [connect_control(port, source='127.0.0.2') for _ in range(2)] + try: + time.sleep(.3) # both accepted and pending + # Not `exchange`: its Busy retry would turn the red shape into a timeout. No + # session is live on this supervisor, so a Busy here can only be the queue's. + with pair(port) as newcomer: + newcomer.settimeout(10) + newcomer.sendall(hello(schema, flatbuffers)) + shared = receive(newcomer, schema) + holders[0].settimeout(5) + try: + displaced = receive(holders[0], schema) + except (RuntimeError, OSError) as error: + displaced = {'error': repr(error)} + finally: + for holder in holders: + holder.close() + assert shared.get('code') == 2 and shared.get('expected', 0), ( + f'a Hello from 127.0.0.1 while 127.0.0.2 held the whole pre-auth queue was answered {shared}') + assert displaced.get('code') == 7 and displaced.get('detail') == kDetailDisplaced, displaced + named = wait_for_log(serverLog, 'pending connection from 127.0.0.2 displaced by one from 127.0.0.1') + assert named is not None, f'the displacement was not named in {serverLog}' + evidence['pending_queue_is_shared'] = {'newcomer': shared, 'displaced': displaced, 'line': next( + l.strip() for l in named.splitlines() if 'displaced by one from 127.0.0.1' in l)} + time.sleep(.3) + # (b) THE BACKOFF. Two wrong tokens from this address, and the THIRD connection - with # the right token - is refused at accept, before its Hello is read, with the backoff's # own words; once the window has passed the right token is welcomed again (and that @@ -862,6 +903,34 @@ def main(): evidence['auth_backoff'] = {'refusal': backed, 'line': next( l.strip() for l in named.splitlines() if 'refused at accept: authentication backoff' in l), 'afterWindow': after} + + # (c) A SILENT SLOT HOLDER IS BACKED OFF (fix round). The critic's reproduction, with + # this supervisor's small knobs: 127.0.0.4 holds the whole queue with connections + # that never send a byte and closes them just before the deadline would have + # counted them. The first cut counted no close without a byte, so that address + # could repeat this for ever. A close after the probe grace (250 ms) is a failure + # now: after two, 127.0.0.4 is refused at accept - even with the right token. + # RED without the rule: that Hello is judged and forked for (Refuse{WireFingerprint} + # from the child, code 2). + holders = [connect_control(port, source='127.0.0.4') for _ in range(2)] + time.sleep(.6) # past the grace, well inside the 3000 ms deadline + for holder in holders: + holder.close() + time.sleep(.2) # the supervisor sees both closes before the next connection + with socket.create_connection(('127.0.0.1', port), timeout=3, source_address=('127.0.0.4', 0)) as held: + held.settimeout(10) + held.sendall(hello(schema, flatbuffers)) + try: + silent = receive(held, schema) + except (RuntimeError, OSError) as error: + silent = {'error': repr(error)} + assert silent.get('code') == 4 and silent.get('detail') == kDetailBackoff, ( + f'127.0.0.4 held the pre-auth queue twice without a byte and its next Hello was answered {silent}') + reason = '127.0.0.4 failed pre-auth 2 times (last: held a pre-auth slot and closed without a byte)' + named = wait_for_log(serverLog, reason) + assert named is not None, f'the silent holder\'s failures were not named ("{reason}") in {serverLog}' + evidence['silent_slot_holder_backed_off'] = {'refusal': silent, 'line': next( + l.strip() for l in named.splitlines() if reason in l)} assert process.poll() is None, 'the pre-auth limits killed the supervisor' # ---- F fix round: THE SINGLE-SESSION SERVER CLOSES ITS LISTENER ONCE THE SESSION IS BOUND