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. |
…tering cap_new() and cap_del() skipped every connection that was not yet IsUser. A client that sent CAP LS 302 while its server was still bursting with the network saw no sasl in the listing, the path to the SASL server completed before it registered, and the NEW was dropped: it registered without ever learning that sasl exists. This is what clients of a restarted leaf hit when they reconnect while the leaf links. cap-notify is enabled by negotiation (implicitly by CAP LS 302), not by registration, and the spec lets NEW be "sent at any time" with "*" as the target when no nick is available yet. Notify any local user connection, registered or not, that has cap-notify active. Tests: a registering client receives NEW, can REQ the capability before CAP END and gets a single DEL on split; plus real hub<->leaf relink coverage (either side initiating) for a registered client on the leaf.
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.Also: CAP NEW/DEL for cap-notify clients that are still registering (3306538, folded in from #124)
cap_new()andcap_del()skipped every connection that was not yetIsUser. A client that sentCAP LS 302while its server was still linking saw nosaslin the listing, the SASL server became reachable before it registered, and theCAP NEW :sasl=...was dropped — it registered without ever learning that sasl exists. This is what the clients of a restarted leaf hit when they reconnect while the leaf links.capability-negotiation: NEW "may be sent at any time"; cap-notify "MUST be implicitly enabled if the client requests
CAP LSwith a version of 302 or newer" and, when enabled, "the server MUST notify the client about all new capabilities and about existing capabilities that are no longer available"; "Replies from the server must contain the client identifier name or asterisk if one is not yet available." cap-notify is enabled by negotiation, not registration, so theIsUser()filter was a deviation.Fix: a
cap_notify_target()helper used by both functions — any local user connection, registered or unregistered on a user/websocket port (the same testcap_ls()/cap_req()use for "registration not complete"), with cap-notify active.%Calready yields*without a nick.Tests:
test_sasl_cap_burst_split.py::test_cap_new_reaches_client_still_registering—CAP LS 302+ NICK/USER withoutCAP END; NEW arrives at the uplink's EB,CAP REQ :saslis ACKed before registration, single DEL on split (fails without them_cap.cchange).test_sasl_cap_relink.py(new,multi_server) — a real leaf1 is split from the hub, a fake SASL server links fully on the hub side, a cap-notify client registers on the lone leaf, then the link is re-established by the hub and by the leaf respectively: exactly oneCAP NEW :sasl=PLAINat the hub's EB on the leaf. Coverage for the real ircd↔ircd path that the stub tests cannot show.Run of
pr66_capsasl/ cap/with this commit: 53 passed, 0 failed.