processone / ejabberd

Robust, Ubiquitous and Massively Scalable Messaging Platform (XMPP, MQTT, SIP Server)
https://www.process-one.net/en/ejabberd/
Other
6k stars 1.5k forks source link

Authentication failure by websocket and BOSH with converse.js #4142

Closed crasoanirina closed 5 months ago

crasoanirina commented 6 months ago

ejabberd version: 23.10.38

When upgrading ejabberd 23.10 to ejabberd 23.10.38, converse.js web client authentication fails. Before the update it worked correctly.

ejabberd.log

[warning] <0.5095.1>@ejabberd_c2s:process_auth_result/3:288 (websocket|<0.5094.1>) Failed c2s SCRAM-SHA-1 authentication from

licaon-kter commented 6 months ago

Browser console log for Converse debug mode?

badlop commented 6 months ago

I can reproduce using Conversations Converse 10.1.2 and the latest 10.1.6 When using 23.10 tag:

2024-01-05 00:22:47.836 [info] (websocket|<0.5315.0>)
 Accepted c2s SCRAM-SHA-512 authentication for admin@localhost by mnesia backend from ::1
2024-01-05 00:22:47.842 [info] (websocket|<0.5315.0>)
 Opened c2s session for admin@localhost/converse.js-83353802

when using master branch:

2024-01-05 00:25:22.945 [warning] (websocket|<0.5320.0>)
 Failed c2s SCRAM-SHA-512 authentication from ::1: Response decoding failed

Using git bisect, it seems the commit that introduced the problem is one of b556fae08fc025f5f5228b6e74984678b6c31874 b5ce53c90710e54c3fa749d5d85a878c04043f48

How to reproduce:

Apply this configuration change: ```diff diff --git a/ejabberd.yml.example b/ejabberd.yml.example index df52c85be..a2484b190 100644 --- a/ejabberd.yml.example +++ b/ejabberd.yml.example @@ -66,6 +66,8 @@ listen: request_handlers: /admin: ejabberd_web_admin /.well-known/acme-challenge: ejabberd_acme + /conversejs: mod_conversejs + /ws: ejabberd_http_ws - port: 3478 ip: "::" @@ -155,6 +157,7 @@ shaper_rules: s2s_shaper: fast modules: + mod_conversejs: {} mod_adhoc: {} mod_admin_extra: {} mod_announce: ``` Compile ejabberd: ```bash ./autogen.sh ./configure --with-rebar=`which rebar3` make make relive ``` then in another window register an account ``` ejabberdctl register admin localhost pass ``` Finally, go to the Conversations page and try to login: http://localhost:5280/conversejs/
Neustradamus commented 6 months ago

@badlop: Attention, it is converse.js not Conversations.

prefiks commented 6 months ago

This looks issue with xmpp lib used here, seems that it doesn't like d= attribute in sasl payload that downgrade protection commits adds, and makes it return base64 encoded "false" in response to sasl challenge. Will probably need to add option to at least disable offering those.

prefiks commented 6 months ago

Looks like it's from this: https://github.com/strophe/strophejs/blob/master/src/scram.js#L52

prefiks commented 6 months ago

I created pull request in strophejs that fixes this: https://github.com/strophe/strophejs/pull/698

badlop commented 6 months ago

That PR in the client library is great for the long-term :)

But looking at strophe and converse.js git repositories activity, that may take a while. In the short term, existing Converse.js clients will not work with the next ejabberd release until the client is fixed.

Just in case the next ejabberd release gets published before a fix is ready in that client: is there anything admins with newer ejabberd can do to get old buggy Converse.js working?

crasoanirina commented 6 months ago

Temporarily, locate the following text in converse.min.js: n=Wr.base64ToArrayBuf(r[2]);break;case"i":s=parseInt(r[2],10);break;default:return}}if

Delete the text “default:”, leaving it as follows: n=Wr.base64ToArrayBuf(r[2]);break;case"i":s=parseInt(r[2],10);break;return}}if

It works for me now without problems

prefiks commented 6 months ago

I will introduce option to disable this, probably even two options to configure sasl and sasl2 separately, we probably can enable it for sasl2 by default and disable for sasl (but as mitm downgrade protection separating those two don't make that much sense mitm box could just downgrade to old sasl then, so maybe not worth doing this separately for both?)

Neustradamus commented 5 months ago

@crasoanirina: It is possible to rename the title of this issue? It is BOSH ^^

prefiks commented 5 months ago

Commit 29ec5bff606da07b03cc3c7a9d5f963008996474 adds option disable_sasl_scram_downgrade_protection: false that can be used to disable sending data that causes problem here.

Should we add that at least temporary to default config, or just add mention about it to release notes? I guess we could add it to next release, at least until patched clients will be available, what do you think?

nosnilmot commented 5 months ago

Adding to default config will be a disadvantage to new installations in the long run, and provide no relief to existing installations, so that doesn't seem like a great idea

Neustradamus commented 5 months ago

Good news, the @prefiks PR has been merged in strophejs by @jcbrand.

Now we wait a new strophejs release build and conversejs release build too.

Neustradamus commented 4 months ago

Currently, no new ConverseJS version with the StropheJS fix...