Closed Bug 1193494 Opened 5 years ago Closed 5 years ago

Never prefer SASL PLAIN for JS-XMPP auth even over encrypted connections

Categories

(Chat Core :: XMPP, defect)

defect
Not set
normal

Tracking

(thunderbird47+ fixed, thunderbird48 fixed, thunderbird49 fixed, thunderbird_esr45 fixed)

RESOLVED FIXED
Instantbird 43
Tracking Status
thunderbird47 + fixed
thunderbird48 --- fixed
thunderbird49 --- fixed
thunderbird_esr45 --- fixed

People

(Reporter: aleth, Assigned: aleth)

References

(Depends on 1 open bug)

Details

Attachments

(2 files)

Currently if the server sends the supported auth mechanisms in an order such that the first one we support is PLAIN, we use that even if there are better mechanisms available, as long as the connection is encrypted.

This isn't really a problem, but we might as well pick a better method.
Attached patch neverplain.diffSplinter Review
Attachment #8646552 - Flags: review?(clokep)
Comment on attachment 8646552 [details] [diff] [review]
neverplain.diff

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

How difficult would it be to have tests that just respond with the mechanisms and ensure that we choose the proper one with different sets of options?
Attachment #8646552 - Flags: review?(clokep) → review+
Keywords: checkin-needed
(In reply to Patrick Cloke [:clokep] from comment #2)
> How difficult would it be to have tests that just respond with the
> mechanisms and ensure that we choose the proper one with different sets of
> options?

Filed as a separate bug as it will require separating out some of this code into a helper function that can be tested without a fakeserver.
Depends on: 1195097
https://hg.mozilla.org/comm-central/rev/c37d201e97fa9f2baa9fe0f8edbbb41bf59c6498
Bug 1193494 - Never prefer SASL PLAIN for JS-XMPP auth even over encrypted connections. r=clokep
Assignee: nobody → aleth
Status: NEW → RESOLVED
Closed: 5 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Instantbird 43
Depends on: 1267410
https://hg.mozilla.org/comm-central/rev/5c96590fcd6cd2fb3d6e64a1a249c06e41aaac8f
Backed out bug 1193494 (changeset c37d201e97fa) as DIGEST-MD5 caused a regression for some users. rs=bustage-fix
This backout should be uplifted to esr45. (As stated in the bug description, this patch is not essential, so there is no reason to keep it while it is causing regressions.)
Flags: needinfo?(rkent)
Depends on: 1267649
Blocks: 1268081
Backout done for TB 45
Flags: needinfo?(rkent)
Reminder for Kent.
Attachment #8746727 - Flags: approval-comm-beta?
Flags: needinfo?(rkent)
The approval-comm-beta? is sufficient to get my attention when I do landings for beta.
Flags: needinfo?(rkent)
Comment on attachment 8746727 [details]
KENT DONT FORGET THE BACKOUT on Beta

Interesting use of a patch to send a message. But it works!

http://hg.mozilla.org/releases/comm-beta/rev/5ea3c40618e7
Attachment #8746727 - Flags: approval-comm-beta? → approval-comm-beta+
BTW I am not currently monitoring tracking flags other than for major releases. I'm not saying that is the right thing to do, but it is the current practice.
You need to log in before you can comment on or make changes to this bug.