m_sasl: Remove implicit abort on registration

This doesn't make sense in a world where post-registration SASL is
allowed, and should fix one case of an annoying login desync that's seen
in the real world.

Specifically, when a client sends its final AUTHENTICATE and Atheme
receives it, it sends an SVSLOGIN for that client. If the client sends
us its CAP END *before* we see the SVSLOGIN, the implicit abort will try
to abort the SASL session that's already succeeded.

Atheme interprets this as an instruction to forget about the successful
SASL session; you'll connect unidentified. But it's already sent
SVSLOGIN, which will log the client in ircd-side, causing ircd and
services views to differ until the user authenticates again manually.

I think allowing a SASL session to be aborted when it has already
succeeded is an Atheme bug, and it can still be triggered without this
change. But our behaviour here seems silly anyway.
This commit is contained in:
Ed Kellett 2021-10-17 00:00:55 +01:00 committed by Doug Freed
parent 687f290a6c
commit 06c5309534
3 changed files with 11 additions and 26 deletions

View file

@ -14,6 +14,8 @@ bolded warnings in the full release notes below.
- Add `--with-asan` to configure to produce an ASan instrumented build - Add `--with-asan` to configure to produce an ASan instrumented build
### server protocol ### server protocol
- **Breaking:** Don't implicitly abort SASL when connection registration handshake completes;
requires updating atheme to include https://github.com/atheme/atheme/pull/833.
- OPER is now propagated globally, as :operator OPER opername privset - OPER is now propagated globally, as :operator OPER opername privset
### user ### user

View file

@ -72,7 +72,6 @@ mapi_clist_av1 sasl_clist[] = {
&authenticate_msgtab, &sasl_msgtab, &mechlist_msgtab, NULL &authenticate_msgtab, &sasl_msgtab, &mechlist_msgtab, NULL
}; };
mapi_hfn_list_av1 sasl_hfnlist[] = { mapi_hfn_list_av1 sasl_hfnlist[] = {
{ "new_local_user", abort_sasl },
{ "client_exit", abort_sasl_exit }, { "client_exit", abort_sasl_exit },
{ NULL, NULL } { NULL, NULL }
}; };
@ -300,9 +299,6 @@ me_mechlist(struct MsgBuf *msgbuf_p, struct Client *client_p, struct Client *sou
rb_strlcpy(mechlist_buf, parv[1], sizeof mechlist_buf); rb_strlcpy(mechlist_buf, parv[1], sizeof mechlist_buf);
} }
/* If the client never finished authenticating but is
* registering anyway, abort the exchange.
*/
static void static void
abort_sasl(void *data_) abort_sasl(void *data_)
{ {

View file

@ -34,9 +34,9 @@
#include "s_newconf.h" #include "s_newconf.h"
#include "hash.h" #include "hash.h"
#define MSG "%s:%d (%s; aborted=%d, by_user=%d)", __FILE__, __LINE__, __FUNCTION__, aborted, by_user #define MSG "%s:%d (%s; aborted=%d)", __FILE__, __LINE__, __FUNCTION__, aborted
static void common_sasl_test(bool aborted, bool by_user) static void common_sasl_test(bool aborted)
{ {
ircd_util_init(__FILE__); ircd_util_init(__FILE__);
client_util_init(); client_util_init();
@ -78,19 +78,12 @@ static void common_sasl_test(bool aborted, bool by_user)
is_client_sendq_empty(server, MSG); is_client_sendq_empty(server, MSG);
if (aborted) { if (aborted) {
if (by_user) {
// Explicit abort by user // Explicit abort by user
client_util_parse(user, "AUTHENTICATE *" CRLF); client_util_parse(user, "AUTHENTICATE *" CRLF);
is_client_sendq(":" TEST_ME_NAME " 906 " TEST_NICK " :SASL authentication aborted" CRLF, user, MSG); is_client_sendq(":" TEST_ME_NAME " 906 " TEST_NICK " :SASL authentication aborted" CRLF, user, MSG);
client_util_parse(user, "CAP END" CRLF); client_util_parse(user, "CAP END" CRLF);
ok(IsClient(user), MSG); ok(IsClient(user), MSG);
} else {
// Implicit abort by completing registration
client_util_parse(user, "CAP END" CRLF);
ok(IsClient(user), MSG);
is_client_sendq_one(":" TEST_ME_NAME " 906 " TEST_NICK " :SASL authentication aborted" CRLF, user, MSG);
}
is_client_sendq_one(":" TEST_ME_ID " ENCAP " TEST_SERVER_NAME " SASL " TEST_ME_ID "AAAAAB " TEST_REMOTE_ID " D A" CRLF, server, MSG); is_client_sendq_one(":" TEST_ME_ID " ENCAP " TEST_SERVER_NAME " SASL " TEST_ME_ID "AAAAAB " TEST_REMOTE_ID " D A" CRLF, server, MSG);
is_client_sendq(":" TEST_ME_ID " UID " TEST_NICK " 1 42 +i ~" TEST_USERNAME " " TEST_HOSTNAME " " TEST_IP " " TEST_ME_ID "AAAAAB :" TEST_REALNAME CRLF, server, MSG); is_client_sendq(":" TEST_ME_ID " UID " TEST_NICK " 1 42 +i ~" TEST_USERNAME " " TEST_HOSTNAME " " TEST_IP " " TEST_ME_ID "AAAAAB :" TEST_REALNAME CRLF, server, MSG);
@ -128,17 +121,12 @@ static void common_sasl_test(bool aborted, bool by_user)
static void successful_login(void) static void successful_login(void)
{ {
common_sasl_test(false, false); common_sasl_test(false);
}
static void successful_login_after_aborted_by_registration(void)
{
common_sasl_test(true, false);
} }
static void successful_login_after_aborted_by_user(void) static void successful_login_after_aborted_by_user(void)
{ {
common_sasl_test(true, true); common_sasl_test(true);
} }
int main(int argc, char *argv[]) int main(int argc, char *argv[])
@ -146,7 +134,6 @@ int main(int argc, char *argv[])
plan_lazy(); plan_lazy();
successful_login(); successful_login();
successful_login_after_aborted_by_registration();
successful_login_after_aborted_by_user(); successful_login_after_aborted_by_user();
return 0; return 0;