1527480 - No longer able to login into jabber.org: authorization failure

Status: RESOLVED FIXED

(Chat Core :: XMPP, defect)

Description

[Patrick Cloke [:clokep]]

In Thunderbird 64b4 I'm able to log into jabber.org. On 66b1 I'm no longer able to. Looking at protocol logs it seems like everything is "the same".

Potential culprits might be:
https://hg.mozilla.org/comm-central/diff/4dbbf88b2d37/chat/protocols/xmpp/xmpp-authmechs.jsm
https://hg.mozilla.org/comm-central/diff/985a367b6423/chat/protocols/xmpp/xmpp-authmechs.jsm

Both point to bug 1520643.

Comment 1

[Patrick Cloke [:clokep]]

Comment 0 should have said that this worked fine in 65b4 and is broken on 66b1.

Comment 2

[Patrick Cloke [:clokep]]

After looking into this more, I believe this is a regression from bug 1518300 (specifically https://hg.mozilla.org/mozilla-central/rev/7befa4aa3c7f). This modified pbkdf2Generate to return a Promise (instead of synchronously returning the value). I'm not 100% sure how to modify our current code to handle this, however.

Comment 3

[Patrick Cloke [:clokep]]

I have a "working" version (well the unit tests pass :-D) I need to test this, but Daily seems busted right now.

There was two major changes from bug 1518300:

1. The pbkdf2Generate returns a Promise.
2. pbkdf2Generate was changed to use SHA-256 instead of SHA-1.

To handle the first one, I made it so the XMPP auth mechanisms can either directly return an object with data to send or can return a Promise that resolves into the object to send.

To handle the second one I copied a version of pbkdf2Generate and changed the 256 to a 1.

Comment 4

[Patrick Cloke [:clokep]]

Attachment: Patch v1 including tests

Comment 5

[Patrick Cloke [:clokep]]

Comment on attachment 9044212 [details] [diff] [review]
Patch v1 including tests

This needs more testing (aka in an actual build of Thunderbird), but what do you think of this approach, Florian?

Comment 6

[Patrick Cloke [:clokep]]

Comment on attachment 9044212 [details] [diff] [review]
Patch v1 including tests

I was able to test this out now that Daily is no longer busted. This fixed my login for jabber.org, thus requesting a real review.

Comment 7

[Florian Quèze [:florian]]

Comment on attachment 9044212 [details] [diff] [review]
Patch v1 including tests

Review of attachment 9044212 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for including a test! Would be good to also have the test cover the 'PLAIN' method, especially as the patch touches it too with the change in xmpp-session.jsm.

::: chat/protocols/xmpp/xmpp-authmechs.jsm
@@ +389,3 @@
>  
> +  // Once the promise resolves, continue calculating the stuff.
> +  let result = passwordPromise.then((saltedPassword) => {

nit: The 'result' variable doesn't seem useful here, as you could just do:

  receivedStanza = yield passwordPromise.then(...

Not something introduced by your patch, but 'receivedStanza" is a strange name for a variable containing something we are computing.

Comment 8

[Patrick Cloke [:clokep]]

(In reply to Florian Quèze [:florian] from comment #7)

Thanks for including a test! Would be good to also have the test cover the
'PLAIN' method, especially as the patch touches it too with the change in
xmpp-session.jsm.

I can attempt to do that, sure. You know how much I like reading RFCs.

::: chat/protocols/xmpp/xmpp-authmechs.jsm
@@ +389,3 @@

• // Once the promise resolves, continue calculating the stuff.
• let result = passwordPromise.then((saltedPassword) => {

nit: The 'result' variable doesn't seem useful here, as you could just do:

receivedStanza = yield passwordPromise.then(...

That's a bit cleaner, good call!

Not something introduced by your patch, but 'receivedStanza" is a strange
name for a variable containing something we are computing.

The receivedStanza variable is what gets passed in each time the generator is "pumped" (e.g. in xmpp-session.jsm: https://searchfox.org/comm-central/rev/b2c0c977fa22d1217b0bd98ee0f3d1b0eab9109b/chat/protocols/xmpp/xmpp-session.jsm#478)

Comment 9

[Patrick Cloke [:clokep]]

Attachment: Patch v2

Patch that makes the minor change suggested in comment 7 + adds a test for PLAIN authentication. I tweaked a few comments too.

We also have another authentication mechanism in gtalk.js (PlainFullBindAuth).

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/672ec0c7864b
Fix the SCRAM-SHA-1 authentication mechanism for XMPP, r=florian

Comment 11

[Patrick Cloke [:clokep]]

Comment on attachment 9045217 [details] [diff] [review]
Patch v2

[Approval Request Comment]
Regression caused by (bug #): bug 1527480
User impact if declined: Unable to log into (most) XMPP servers
Testing completed (on c-c, etc.): Tested by logging into jabber.org on a custom built Daily.
Risk to taking this patch (and alternatives if risky): XMPP authentication might still be broken, but doesn't seem worse that it is now.

Comment 12

[Jorg K (CEST = GMT+2)]

TB 66 beta 3:
https://hg.mozilla.org/releases/comm-beta/rev/2960e4a6cec05f9fcd3061719f64772a112167ec

Hmm, this doesn't look right for mozilla66 since ChromeUtils.import() was different there.
https://hg.mozilla.org/releases/comm-beta/rev/2960e4a6cec05f9fcd3061719f64772a112167ec#l1.8

+var {XMPPAuthMechanisms} = ChromeUtils.import("resource:///modules/xmpp-authmechs.jsm");
+var {Stanza} = ChromeUtils.import("resource:///modules/xmpp-xml.jsm");

But since there was no test failure, I guess it worked.