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

RESOLVED FIXED in Instantbird 67

Status

defect
--
major
RESOLVED FIXED
2 months ago
2 months ago

People

(Reporter: clokep, Assigned: clokep)

Tracking

({regression})

Instantbird 67

Thunderbird Tracking Flags

(thunderbird66 fixed, thunderbird67 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

2 months ago

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.

(Assignee)

Comment 1

2 months ago

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

(Assignee)

Comment 2

2 months ago

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.

(Assignee)

Comment 3

2 months ago

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

Comment 4

2 months ago
Posted patch Patch v1 including tests (obsolete) — Splinter Review
(Assignee)

Comment 5

2 months ago
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)
(Assignee)

Comment 6

2 months ago
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+
(Assignee)

Comment 8

2 months ago

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

(Assignee)

Comment 9

2 months ago
Posted 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+

Comment 10

2 months ago

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
Last Resolved: 2 months ago
Resolution: --- → FIXED
(Assignee)

Updated

2 months ago
Target Milestone: --- → Instantbird 67
(Assignee)

Comment 11

2 months ago
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?

Updated

2 months ago
Attachment #9045217 - Flags: approval-comm-beta? → approval-comm-beta+

Comment 12

2 months ago

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.