Closed Bug 1527480 Opened 2 years ago Closed 2 years ago

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

Categories

(Chat Core :: XMPP, defect)

defect
Not set
major

Tracking

(thunderbird66 fixed, thunderbird67 fixed)

RESOLVED FIXED
Instantbird 67
Tracking Status
thunderbird66 --- fixed
thunderbird67 --- fixed

People

(Reporter: clokep, Assigned: clokep)

References

Details

(Keywords: regression)

Attachments

(1 file, 1 obsolete file)

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 0 should have said that this worked fine in 65b4 and is broken on 66b1.

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.

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.

Blocks: 1518300
Attached patch Patch v1 including tests (obsolete) — Splinter Review
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?
Attachment #9044212 - Flags: feedback?(florian)
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.
Attachment #9044212 - Flags: feedback?(florian) → review?(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.
Attachment #9044212 - Flags: review?(florian) → review+

(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)

Attached patch Patch v2Splinter Review

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).

Attachment #9044212 - Attachment is obsolete: true
Attachment #9045217 - Flags: review?(florian)
Attachment #9045217 - Flags: review?(florian) → review+

Pushed by clokep@gmail.com:
https://hg.mozilla.org/comm-central/rev/672ec0c7864b
Fix the SCRAM-SHA-1 authentication mechanism for XMPP, r=florian

Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Instantbird 67
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.
Attachment #9045217 - Flags: approval-comm-beta?
Attachment #9045217 - Flags: approval-comm-beta? → approval-comm-beta+

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.

You need to log in before you can comment on or make changes to this bug.