Fix spurious CAP NEW sasl on netsplit of a half-linked SASL server - #100
Fix spurious CAP NEW sasl on netsplit of a half-linked SASL server#100MrIron-no wants to merge 3 commits into
Conversation
A SASL server (channels.*) behind a hub that linked half-way -- introduced its downlinks but never completed its burst -- and then pinged out caused cap-notify clients to see "CAP NEW sasl" immediately followed by "CAP DEL sasl" on the split. Three defects combined: * SASL availability was only re-evaluated on END_OF_BURST_ACK, so the capability flag went stale relative to find_match_server() while the SASL server was linked through a bursting hop. * exit_one_client() re-checked availability for every server torn down in the split. A sibling of the SASL server exited first, the check still found the SASL server in server_list[], and emitted CAP NEW one message before the real CAP DEL. * cap_new()/cap_del() iterated i < HighestFd instead of <=, so the local client holding the highest fd never received CAP NEW/DEL at all. Fixes: * sasl_available() walks the uplink chain from the SASL server to &me and reports unavailable if any hop IsBurst(). This gates both advertising and m_sasl's runtime routing check. * Re-check on END_OF_BURST (the moment IsBurst clears) instead of on END_OF_BURST_ACK, which a services package may never send. * Re-check once at the end of exit_client() after the whole subtree is gone, instead of per server inside exit_one_client(). * cap_new()/cap_del() loop to <= HighestFd. * CAP NEW now prefixes the trailing parameter with ':' like CAP DEL. Tests: tests/pr66_capsasl/test_sasl_cap_burst_split.py reproduces the incident topology with a fake P10 hub introducing the SASL server plus a sibling, dropping mid-burst (fails on the old code with exactly [NEW sasl=PLAIN, DEL sasl]), and covers NEW-on-EB, single DEL on split, and a J10-introduced SASL server staying hidden until its own EB. P10Server.handshake() is split into begin_handshake()/send_end_of_burst()/ complete_handshake(), and send_downstream_server() gains bursting= (P10 vs J10) plus send_end_of_burst_for().
Covers the direct-link case where sasl.server already points at the bursting link: no CAP NEW while bursting, NEW exactly at EB, nothing on EA, a single DEL on disconnect.
|
@MrIron-no a few findings:
sasl_available() now walks the burst-path of whichever server find_match_server() returns — which is the lowest-numnick match, not necessarily the established one. With a wildcard mask like channels.*, if a second matching server with a lower numnick relinks and is bursting, availability flips to 0 even though a fully-linked SASL server exists. The next trigger then sends every cap-notify client a spurious CAP DEL :sasl / CAP NEW — the exact churn this PR is meant to fix — and AUTHENTICATE is rejected during the window. The old code only checked existence, so it didn't care which match came back. Fix: walk all matches and prefer a non-bursting one.
A server matching sasl.server introduced with protocol P (non-bursting) over an established link sends no END_OF_BURST, so none of the PR's triggers fire: availability becomes 1 but clients never get CAP NEW :sasl until an unrelated netjoin/split. Pure ircu emits J+EB, but ms_server accepts P from any peer, and services packages introducing virtual servers (and the PR's own test helper with bursting=False) produce exactly this shape. Fix: trigger the capability re-check from server introduction too.
m_sasl.c does its own find_match_server() lookup while sasl_available() repeats the scan internally — the routed-to server is only coincidentally the validated one. This is a trap: fixing finding #1 (prefer a non-bursting match) would silently route AUTHENTICATE to a server whose path was never checked. Fix: add a sasl_server() accessor returning the validated struct Client * and use it at both call sites; it also drops a linear scan per AUTHENTICATE.
The P10Server connect + handshake + config setup appears three times (the helper plus inline copies in tests 3 and 4), differing only in sasl.server value, bursting flag, and extra downstreams. Fix: parameterize _half_link_with_sasl_server(...). Caveat from verification: do not replace _capnotify_client with cap_helpers.make_cap_client — cap-notify is CAPFL_HIDDEN_302, so that helper would pytest.skip these tests.
max(remaining, 0.1) grants a fabricated 100 ms window when begin_handshake() has already consumed the whole timeout, so the failure surfaces 100 ms late as a confusing "Timed out waiting for EA" instead of "handshake timed out" at the declared deadline. Fix: pass remaining straight through — complete_handshake() already raises when it's ≤ 0. |
…duction hook Review findings on UndernetIRC#100: 1. With a wildcard sasl.server mask, find_match_server() returns the lowest-numnick match, so a matching server that is re-linking and still bursting could hide an established one and cause DEL/NEW churn. Add find_match_server_next() to enumerate all matches; sasl_server() walks them and returns the first whose path to us is not bursting. 2. A server matching sasl.server introduced as already past its burst (P) over an established link never sends END_OF_BURST, so nothing re-checked availability. Call sasl_check_capability() at the end of ms_server(); it is a no-op for J introductions and for servers behind a bursting hop, so nothing is advertised during a burst. 3. m_sasl() did its own find_match_server() lookup, only coincidentally routing AUTHENTICATE to the server sasl_available() validated. Both now use sasl_server(), which is the single source of truth and returns the validated struct Client *. This also stops collapse()ing the netconf value in place: the mask is copied first. 4. Tests: _half_link_with_sasl_server() is parameterized on the sasl.server value and the downstream servers to introduce, replacing the inline copies. New tests cover the P-introduction-over-established- link case, and the wildcard case with a bursting lower-numnick sibling including that AUTHENTICATE is routed to the validated server (XQ target numeric). 5. P10Server.handshake() passes the remaining budget straight to complete_handshake() instead of clamping to 100 ms.
|
Thanks for the review — all five addressed in 7e4b5a7. 1. Wildcard 2. No re-check on server introduction (HIGH) — 3. AUTHENTICATE routing (MEDIUM) — 4. Test setup duplication (LOW) — 5. Timeout clamp (LOW) — New tests:
Both fail without the fixes and pass with them. |
2 participants
Problem
A SASL server (
channels.*) sat behind a hub (shub) that linked half-way — it introduced its downlinks but never finished its burst — and then pinged out. cap-notify clients on a remote hub saw, on the split:Root causes
END_OF_BURST_ACK. Correct that the half-linked server was never advertised, but the capability flag went stale relative tofind_match_server(), which could already see the SASL server.exit_one_client()). A sibling of the SASL server was torn down first; the check still found the SASL server inserver_list[]and emitted the spuriousCAP NEWone message before the realCAP DEL.cap_new()/cap_del():i < HighestFdinstead of<=, so the local client holding the highest fd never received CAP NEW/DEL at all.Fix
sasl.c:sasl_available()walks the uplink chain from the SASL server to&meand returns unavailable if any hopIsBurst(). Gates both advertising andm_sasl's runtime routing check.m_endburst.c: re-check onEND_OF_BURST(the momentIsBurstclears) instead ofEND_OF_BURST_ACK, which a services package may never send.s_misc.c: one re-check at the end ofexit_client()after the whole subtree is gone.m_cap.c:<= HighestFd;CAP NEWnow prefixes the trailing parameter with:likeCAP DEL.No CAP NEW is emitted while any hop on the path to the SASL server is still bursting.
Tests
tests/pr66_capsasl/test_sasl_cap_burst_split.py(4 tests, fake P10 hub viaP10Server):[NEW sasl=PLAIN, DEL sasl])P10) → NEW exactly at the uplink's EB, nothing on EA, single DEL on splitJ10→ stays hidden after the uplink's EB, NEW only at its own EBsasl.serveralready set → NEW exactly at its EB, single DEL on disconnectP10Server.handshake()is split intobegin_handshake()/send_end_of_burst()/complete_handshake()(the compositehandshake()is unchanged for existing tests);send_downstream_server()gainsbursting=(P10 vs J10) andsend_end_of_burst_for().Full run of
pr66_capsasl/ pr_iauthverify/ pr64_netconf/ pr_msgtags_compat/test_s2s_service_rpc_untagged.py: 49 passed, 0 failed.