Support SASL SCRAM authentication mechanism

RESOLVED FIXED in Instantbird 50

Status

defect
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: aleth, Assigned: abdelrahman)

Tracking

trunk
Instantbird 50
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 7 obsolete attachments)

(Reporter)

Description

3 years ago
As discussed in bug 1267410, DIGEST-MD5 is obsolete, and should be replaced with SCRAM.

Once SCRAM is added, bug 1193494 could be landed again.
(Reporter)

Updated

3 years ago
Blocks: 1193494
(Reporter)

Updated

3 years ago
Blocks: 955019
(Reporter)

Comment 1

3 years ago
RFC 6120 13.8. Mandatory-to-Implement TLS and SASL Technologies
13.8.1 For authentication only, servers and clients MUST support the SASL Salted Challenge Response Authentication Mechanism [SCRAM] -- in particular, the SCRAM-SHA-1 and SCRAM-SHA-1-PLUS variants.
(Reporter)

Comment 2

3 years ago
Apart from the links to the spec in RFC 6120, this might be helpful: https://stackoverflow.com/questions/29298346/xmpp-sasl-scram-sha1-authentication

Updated

3 years ago
Blocks: 1268081
(Reporter)

Updated

3 years ago
Assignee: nobody → ab
Status: NEW → ASSIGNED
(Assignee)

Comment 3

3 years ago
Posted patch v1 - scram (obsolete) — Splinter Review
There are changes required to handle last message (e.g. we need to call |next| to check server signature in success message) [1], I'm not sure if they are should be done here or in bug 1193494?

[1] https://dxr.mozilla.org/comm-central/rev/52ec8593c427bb0ba02713f2f060bbd0dd2c0551/chat/protocols/xmpp/xmpp-session.jsm#449
Attachment #8757651 - Flags: review?(aleth)
(Reporter)

Comment 4

3 years ago
(In reply to Abdelrhman Ahmed [:abdelrhman] from comment #3)

Looks like good progress!

> There are changes required to handle last message (e.g. we need to call
> |next| to check server signature in success message) [1], I'm not sure if
> they are should be done here or in bug 1193494?

Bug 1193494 just means we don't use PLAIN if we can avoid it. I don't think that will require changes.

Add a second patch here, that will land before the SCRAM patch, that removes DIGEST-MD5 completely, together with the hack from bug 787046 you pointed to in [1] that was added for Openfire servers. (We know from bug 1267410 that DIGEST-MD5 doesn't work properly with Openfire any more anyway.) Hopefully that will simplify things.
(Assignee)

Comment 5

3 years ago
Posted patch v1 - remove DIGEST-MD5 (obsolete) — Splinter Review
Attachment #8757691 - Flags: review?(aleth)
(Assignee)

Comment 6

3 years ago
Posted patch v2 - scram (obsolete) — Splinter Review
Attachment #8757651 - Attachment is obsolete: true
Attachment #8757651 - Flags: review?(aleth)
Attachment #8757692 - Flags: review?(aleth)
(Reporter)

Comment 7

3 years ago
Comment on attachment 8757691 [details] [diff] [review]
v1 - remove DIGEST-MD5

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

::: chat/protocols/xmpp/xmpp-session.jsm
@@ +442,2 @@
>            errorMsg = "notAuthorized";
> +        }

Please move this to the other patch.

@@ +466,5 @@
> +          // on success stanza then pass this stanza to authResult as
> +          // we do not expect to receive more stanzas.
> +          this.stanzaListeners.authResult.call(this, aStanza);
> +        }
> +      }

And this.
Attachment #8757691 - Flags: review?(aleth) → review-
(Reporter)

Comment 8

3 years ago
Comment on attachment 8757692 [details] [diff] [review]
v2 - scram

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

::: chat/protocols/xmpp/xmpp-authmechs.jsm
@@ +14,5 @@
>  
>  var {classes: Cc, interfaces: Ci, utils: Cu} = Components;
>  
>  Cu.import("resource:///modules/xmpp-xml.jsm");
> +Cu.import("resource://services-crypto/utils.js");

You'll have to package this component, like this
https://dxr.mozilla.org/comm-central/source/mozilla/browser/installer/package-manifest.in#295

@@ +40,5 @@
> +// Generates a random nonce and returns a base64 encoded string.
> +// aLength in bytes.
> +function createNonce(aLength) {
> +  let buffer = new Uint8Array(aLength);
> +  crypto.getRandomValues(buffer);

As you're using CryptoUtils everywhere else, you can save one import here and use generateRandomBytes

@@ +47,5 @@
> +  // We guarantee a vaild nonce value using base64 encoding.
> +  return btoa(buffer);
> +}
> +
> +// Parses the string of server's repsonse (aChallenge) into an object.

Typo (response)

@@ +51,5 @@
> +// Parses the string of server's repsonse (aChallenge) into an object.
> +function parseChallenge(aChallenge) {
> +  let values = aChallenge.split(",");
> +  let retVal = {};
> +  values.forEach(value => {

You can inline aChallenge.split

@@ +61,5 @@
> +}
> +
> +// RFC 4013 and RFC 3454: Stringprep Profile for User Names and Passwords.
> +function saslPrep(aString) {
> +  // RFC 3454 (section 2): Preparation Overview.

RFC 3454/4013 has been replaced by RFC 7564/7613, but it looks like SCRAM-SHA1 still uses stringprep, so that's OK.

@@ +82,5 @@
> +function saslName(aName) {
> +  // RFC 5802 (5.1): 1*(value-safe-char / "=2C" / "=3D")
> +  // value-safe-char is a UTF8-char except NUL, "=", and ",".
> +  if (!aName)
> +    throw "Name is not valid";

Shouldn't you move this check to just before the return?

@@ +93,5 @@
> +}
> +
> +function normalize(aName) {
> +  return saslPrep(aName);
> +}

Just use saslPrep() where you call normalize(). If this was intended as documentation, just add e.g. "// normalize using saslPrep" where it is called.

@@ +100,5 @@
> +  let hasher = Cc["@mozilla.org/security/hash;1"]
> +             .createInstance(Ci.nsICryptoHash);
> +  hasher.init(hasher.SHA1);
> +
> +  return CryptoUtils.digestBytes(aMessage, hasher);

Are you sure you want digestBytes and not digestUTF8? Why?
Otherwise, this looks a lot likt CryptoUtils.UTF8AndSHA1

@@ +139,5 @@
> +
> +    // Expected to contain the user’s iteration count (i) and the user’s
> +    // salt (s), and the server appends its own nonce to the client-specified
> +    // one (r).
> +    let attributes = parseChallenge(decodedResponse);

Better check r,s, and i exist in attributes.

@@ +161,5 @@
> +    // ClientKey := HMAC(SaltedPassword, "Client Key")
> +    let hasher =
> +      CryptoUtils.makeHMACHasher(Ci.nsICryptoHMAC.SHA1,
> +                                 CryptoUtils.makeHMACKey(saltedPassword));
> +    let clientKey = CryptoUtils.digestUTF8("Client Key", hasher);

same question as above, why digestUTF8 and not digestBytes?

@@ +182,5 @@
> +    // Calculate ServerSignature.
> +
> +    // ServerKey := HMAC(SaltedPassword, "Server Key")
> +    hasher = CryptoUtils.makeHMACHasher(Ci.nsICryptoHMAC.SHA1,
> +                                        CryptoUtils.makeHMACKey(saltedPassword));

We already got this hasher earlier. Is it required to use a fresh one?
Attachment #8757692 - Flags: review?(aleth) → feedback+
(Assignee)

Comment 9

3 years ago
Attachment #8757691 - Attachment is obsolete: true
Attachment #8757968 - Flags: review?(aleth)
(Reporter)

Updated

3 years ago
Attachment #8757968 - Flags: review?(aleth) → review+
(Assignee)

Comment 10

3 years ago
Posted patch v3 - scram (obsolete) — Splinter Review
(In reply to aleth [:aleth] from comment #8) 
> You'll have to package this component, like this
> https://dxr.mozilla.org/comm-central/source/mozilla/browser/installer/
> package-manifest.in#295
 
I think it's already packaged according to links [1], [2], [3] and [4]

> Shouldn't you move this check to just before the return?

Yes, done in this patch.
 
> Are you sure you want digestBytes and not digestUTF8? Why?
> Otherwise, this looks a lot likt CryptoUtils.UTF8AndSHA1
> same question as above, why digestUTF8 and not digestBytes?

We should use |digestBytes| for all calls as |digestUTF8| will map some characters and may change the length of string according to this mapping (UTF-8 conversion), but this will not work with hashing as we need to keep result as is (bits level). |digestBytes| does not change the result, it only changes way result is represented (array of bytes), so it keeps result as is.
Luckily, |digestUTF8| gave the same results as strings like "Client Key" will be the same after UTF-8 conversion then it's converted to array of bytes, so it worked in the previous patch.

> We already got this hasher earlier. Is it required to use a fresh one?

No, we can reuse only with the same key it as |digestBytes| and |digestUTF8| reset it after using it.

[1] https://dxr.mozilla.org/comm-central/source/mozilla/services/crypto/moz.build
[2] https://dxr.mozilla.org/comm-central/source/im/installer/package-manifest.in#406
[3] https://dxr.mozilla.org/comm-central/source/suite/installer/package-manifest.in#392
[4] https://dxr.mozilla.org/comm-central/source/mail/installer/package-manifest.in#654
Attachment #8757692 - Attachment is obsolete: true
Attachment #8757972 - Flags: review?(aleth)
(Reporter)

Comment 11

3 years ago
(In reply to Abdelrhman Ahmed [:abdelrhman] from comment #10)
> Created attachment 8757972 [details] [diff] [review]
> v3 - scram
> 
> (In reply to aleth [:aleth] from comment #8) 
> > You'll have to package this component, like this
> > https://dxr.mozilla.org/comm-central/source/mozilla/browser/installer/
> > package-manifest.in#295
>  
> I think it's already packaged according to links [1], [2], [3] and [4]

That's packaging the cryptoComponent, but not services/crypto/component which is a dependency. But I think you're right that we can probably do without it as I don't think utils.js uses JPAKE, so let's leave it as it is for now.

> > Are you sure you want digestBytes and not digestUTF8? Why?
> > Otherwise, this looks a lot likt CryptoUtils.UTF8AndSHA1
> > same question as above, why digestUTF8 and not digestBytes?
> 
> We should use |digestBytes| for all calls as |digestUTF8| will map some
> characters and may change the length of string according to this mapping
> (UTF-8 conversion), but this will not work with hashing as we need to keep
> result as is (bits level). |digestBytes| does not change the result, it only
> changes way result is represented (array of bytes), so it keeps result as is.
> Luckily, |digestUTF8| gave the same results as strings like "Client Key"
> will be the same after UTF-8 conversion then it's converted to array of
> bytes, so it worked in the previous patch.

OK, now it is consistent. It's important to check because unicode usernames and passwords are allowed.
(Reporter)

Comment 12

3 years ago
Comment on attachment 8757972 [details] [diff] [review]
v3 - scram

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

::: chat/protocols/xmpp/xmpp-authmechs.jsm
@@ +45,5 @@
> +}
> +
> +// Parses the string of server's response (aChallenge) into an object.
> +function parseChallenge(aChallenge) {
> +  let retVal = {};

A more descriptive name would be better. let attributes = ... or something like that?

@@ +81,5 @@
> +  let retVal = saslPrep(aName).replace(/\=/g,"=3D").replace(/,/g, "=2C");
> +
> +  // RFC 5802 (5.1): 1*(value-safe-char / "=2C" / "=3D")
> +  // value-safe-char is a UTF8-char except NUL, "=", and ",".
> +  if (!aName)

!retVal

Better to use "saslName" instead of "retVal", it's clearer.

@@ +89,5 @@
> +}
> +
> +function bytesAndSHA1(aMessage) {
> +  let hasher = Cc["@mozilla.org/security/hash;1"]
> +             .createInstance(Ci.nsICryptoHash);

indentation
(Reporter)

Comment 13

3 years ago
Comment on attachment 8757972 [details] [diff] [review]
v3 - scram

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

::: chat/protocols/xmpp/xmpp-session.jsm
@@ +465,5 @@
> +          // Used for mechanisms (e.g. SCRAM) that require to do checks
> +          // on success stanza then pass this stanza to authResult as
> +          // we do not expect to receive more stanzas.
> +          this.stanzaListeners.authResult.call(this, aStanza);
> +        }

This is really confusing. 

Let's instead assume that when done=true, we're actually done, and so immediately include whatever authResult does here.

For this, the success stanza has to be handled in the auth mech, and when it's handled you set done, which is what you want for SCRAM. So you have to add a success check to PLAIN.
Attachment #8757972 - Flags: review?(aleth) → review-
(Assignee)

Comment 14

3 years ago
Posted patch v4 - scram (obsolete) — Splinter Review
Attachment #8757972 - Attachment is obsolete: true
Attachment #8758064 - Flags: review?(aleth)
(Reporter)

Comment 15

3 years ago
Comment on attachment 8758064 [details] [diff] [review]
v4 - scram

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

::: chat/protocols/xmpp/xmpp-authmechs.jsm
@@ +26,3 @@
>  }
>  PlainAuth.prototype = {
> +  _init: function(aStanza) {

this.next = ...

@@ +33,5 @@
>        log: '<auth mechanism:="PLAIN"/> (base64 encoded username and password not logged)'
>      };
> +  },
> +
> +  // Handling only success stanza.

I think it is clear without this comment.

@@ +36,5 @@
> +
> +  // Handling only success stanza.
> +  _finish: function(aStanza) {
> +    if (aStanza.localName != "success")
> +      throw "Unhandled received stanza: " + aStanza.convertToString();

No need to include the stanza, it gets logged automatically.

throw "Didn't receive the expected auth success stanza."

@@ +70,5 @@
> +function saslPrep(aString) {
> +  // RFC 3454 (section 2): Preparation Overview.
> +
> +  // 1. Map: Appendix B
> +  // B.1: Commonly mapped to nothing.

Combine these three comments
// RFC 4013 2.1: RFC 3454 3.1, B.1: Map certain codepoints to nothing.

@@ +74,5 @@
> +  // B.1: Commonly mapped to nothing.
> +  let retVal =
> +    aString.replace(/[\u00ad\u034f\u1806\u180b-\u180d\u200b-\u200d\u2060\ufe00-\ufe0f\ufeff]/g,
> +                    "");
> +

RFC 4013 2.1 also asks for "non-ASCII space characters [StringPrep, C.1.2] mapped to SPACE (U+0020)"

@@ +75,5 @@
> +  let retVal =
> +    aString.replace(/[\u00ad\u034f\u1806\u180b-\u180d\u200b-\u200d\u2060\ufe00-\ufe0f\ufeff]/g,
> +                    "");
> +
> +  // B.2: Mapping for case-folding used with NFKC

// RFC 4013 2.2 asks for Unicode normalization form KC, which corresponds to RFC 3454 B.2.

Formulating the comment this way makes it easier to understand later why those particular bits of spec are relevant.

@@ +79,5 @@
> +  // B.2: Mapping for case-folding used with NFKC
> +  retVal = retVal.normalize("NFKC");
> +
> +  // TODO: parts 3 and 4 in this section will handle or detect Prohibited cases
> +  // before sending.

RFC 4013 2.3 should be added here.

@@ +91,5 @@
> +  // ’=3D’ respectively.
> +  let saslName = saslPrep(aName).replace(/\=/g,"=3D").replace(/,/g, "=2C");
> +
> +  // RFC 5802 (5.1): 1*(value-safe-char / "=2C" / "=3D")
> +  // value-safe-char is a UTF8-char except NUL, "=", and ",".

Just add the 5.1 to the previous comment and you can do without these two comment lines.

@@ +228,5 @@
> +      if (this._serverSignature != ServerSignature)
> +        throw "Server signature does not match";
> +    }
> +    else
> +      throw "Unhandled received stanza: " + aStanza.convertToString();

same as for PLAIN

also, turn it around to avoid the long indented block:

if (!success)
  throw
do stuff.
Attachment #8758064 - Flags: review?(aleth) → review-
(Reporter)

Comment 16

3 years ago
Comment on attachment 8758064 [details] [diff] [review]
v4 - scram

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

::: chat/protocols/xmpp/xmpp-session.jsm
@@ +437,5 @@
>      authDialog: function(aAuthMec, aStanza) {
>        if (aStanza && aStanza.localName == "failure") {
>          let errorMsg = "authenticationFailure";
> +        if (aStanza.getElement(["not-authorized"]) ||
> +            aStanza.getElement(["bad-auth"])) {

In what cases does bad-auth happen?
(Assignee)

Comment 17

3 years ago
Posted patch v5 - scram (obsolete) — Splinter Review
(In reply to aleth [:aleth] from comment #16)
> In what cases does bad-auth happen?

When user enters a wrong password.
Attachment #8758064 - Attachment is obsolete: true
Attachment #8759275 - Flags: review?(aleth)
(Reporter)

Comment 18

3 years ago
Comment on attachment 8759275 [details] [diff] [review]
v5 - scram

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

Looks good otherwise. Adding a r?clokep for a second look at the encoding stuff.

::: chat/protocols/xmpp/xmpp-authmechs.jsm
@@ +81,5 @@
> +  // RFC 4013 2.2 asks for Unicode normalization form KC, which corresponds to
> +  // RFC 3454 B.2.
> +  retVal = retVal.normalize("NFKC");
> +
> +  // TODO: RFC 4013 2.3: Prohibited Output.

Why not just do it? ;)
Attachment #8759275 - Flags: review?(clokep)
(Reporter)

Updated

3 years ago
Attachment #8759275 - Flags: review?(aleth) → feedback+
Comment on attachment 8759275 [details] [diff] [review]
v5 - scram

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

Looks pretty good overall. I haven't been following the conversation much, so I'm sorry if these were answered already:
* How hard would it be to also support SHA-256? Is that even a thing?
* Has this been tried with a password with Unicode in it?
* How hard is it to write tests for this?

::: chat/protocols/xmpp/xmpp-authmechs.jsm
@@ +14,5 @@
>  
>  var {classes: Cc, interfaces: Ci, utils: Cu} = Components;
>  
>  Cu.import("resource:///modules/xmpp-xml.jsm");
> +Cu.import("resource://services-crypto/utils.js");

I think we usually put these above /// moduules.

@@ +58,5 @@
> +// Parses the string of server's response (aChallenge) into an object.
> +function parseChallenge(aChallenge) {
> +  let attributes = {};
> +  aChallenge.split(",").forEach(value => {
> +    let match =  /^(\w)=([^]+)$/.exec(value);

What is this matching? Specifically the [^]+ part. (Add a comment.) Aren't you really just trying to split on the first '=' character? I suspect there's a clearer way to write this.

@@ +86,5 @@
> +  return retVal;
> +}
> +
> +// Converts aName to saslname.
> +function saslName(aName) {

This (and all the other functions that are only related to scramSHA1Auth) should probably be methods of scramSHA1Auth.

@@ +90,5 @@
> +function saslName(aName) {
> +  // RFC 5802 (5.1): the client SHOULD prepare the username using the "SASLprep".
> +  // The characters ’,’ or ’=’ in usernames are sent as ’=2C’ and
> +  // ’=3D’ respectively.
> +  let saslName = saslPrep(aName).replace(/\=/g,"=3D").replace(/,/g, "=2C");

Why is this escaped? = isn't a special character that I know of.

@@ +97,5 @@
> +
> +  return saslName;
> +}
> +
> +function bytesAndSHA1(aMessage) {

Please provide a comment for what this is doing.

@@ +99,5 @@
> +}
> +
> +function bytesAndSHA1(aMessage) {
> +  let hasher = Cc["@mozilla.org/security/hash;1"]
> +               .createInstance(Ci.nsICryptoHash);

Nit: Line up the . and the [.

@@ +111,5 @@
> +  this._password = password;
> +  this.next = this._init;
> +}
> +
> +scramSHA1Auth.prototype = {

Nit: Remove the blank line above this.

@@ +114,5 @@
> +
> +scramSHA1Auth.prototype = {
> +  _init: function(aStanza) {
> +    // RFC 5802 (5): SCRAM Authentication Exchange.
> +    this._gs2Header = "n,,";

If this is constant, just declare it in the prototype (as a constant: GS2_HEADER).

@@ +120,5 @@
> +    let username = "n=" + saslName(this._username);
> +    this._cNonce = createNonce(32);
> +    let nonce = "r=" +  this._cNonce;
> +
> +    this._clientFirstMessageBare =  username + "," + nonce;

You can probably just inline the `username` variable here, I don't think it adds much above.

@@ +152,5 @@
> +    }
> +    if (!attributes.r.startsWith(this._cNonce))
> +      throw "Nonce is not correct";
> +
> +

Nit: Two blank lines, expected 1!

@@ +223,5 @@
> +      throw "Unexpected response: " + decodedResponse;
> +
> +    // Compare ServerSignature with our ServerSignature which we calculated in
> +    // _generateResponse.
> +    let ServerSignature = atob(attributes.v);

Nit: lowercase 's' to start the variable name.
Attachment #8759275 - Flags: review?(clokep) → review-
(Reporter)

Comment 20

3 years ago
How about converting the two auth mechanism objects (collections of auth step functions) to one generator function each? Whenever the current step functions return, you just yield instead.
(Reporter)

Comment 21

3 years ago
(In reply to Patrick Cloke [:clokep] from comment #19)
> * How hard would it be to also support SHA-256? Is that even a thing?

There's a (proposed) RFC 7677, would be straightforward to add, I'm not sure any servers support it though so it doesn't seem worth it.
(Assignee)

Comment 22

3 years ago
Posted patch v6 - scram (obsolete) — Splinter Review
(In reply to Patrick Cloke [:clokep] from comment #19)
> * Has this been tried with a password with Unicode in it?

Yes, I tested that one time.

> * How hard is it to write tests for this?

Done in this patch.
Attachment #8759275 - Attachment is obsolete: true
Attachment #8759964 - Flags: review?(aleth)
(Reporter)

Comment 23

3 years ago
Comment on attachment 8759964 [details] [diff] [review]
v6 - scram

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

Nice simplification :-)

::: chat/protocols/xmpp/xmpp-authmechs.jsm
@@ +18,4 @@
>  Cu.import("resource:///modules/xmpp-xml.jsm");
>  
> +// Handle PLAIN authorization mechanism.
> +function* PlainAuth(username, password, domain) {

Nit: aUsername, aPassword, ...

@@ +273,5 @@
> +  // RFC 4013 2.3: Prohibited Output and 2.5: Unassigned Code Points.
> +  let matchStr =
> +    RFC3454.C12 + "|" + RFC3454.C21 + "|" + RFC3454.C22 + "|" + RFC3454.C3 +
> +    "|" + RFC3454.C4 + "|" + RFC3454.C5 + "|" + RFC3454.C6 + "|" + RFC3454.C7 +
> +    "|" + RFC3454.C8 + "|" + RFC3454.C9 + "|" + RFC3454.A1;

Wow, I didn't expect you to do A and D because they are such long lists ;)

@@ +276,5 @@
> +    "|" + RFC3454.C4 + "|" + RFC3454.C5 + "|" + RFC3454.C6 + "|" + RFC3454.C7 +
> +    "|" + RFC3454.C8 + "|" + RFC3454.C9 + "|" + RFC3454.A1;
> +  let match = new RegExp(matchStr, "u").test(retVal);
> +  if (match)
> +    throw "string contains prohibited characters";

Nit: capitalize the first letter

@@ +282,5 @@
> +  // RFC 4013 2.4: Bidirectional Characters.
> +  let r = new RegExp(RFC3454.D1, "u").test(retVal);
> +  let l = new RegExp(RFC3454.D2, "u").test(retVal);
> +  if (l && r)
> +    throw "string must not contain";

Incomplete sentence?

@@ +287,5 @@
> +  else if (r) {
> +    let matchFirst = new RegExp("^(" + RFC3454.D1 + ")", "u").test(retVal);
> +    let matchLast = new RegExp("(" + RFC3454.D1 + ")$", "u").test(retVal);
> +    if (!matchFirst || !matchLast)
> +      throw "a RandALCat character must be the first and the last character";

Nit as above

@@ +314,5 @@
> +
> +  return CryptoUtils.digestBytes(aMessage, hasher);
> +}
> +
> +function* scramSHA1Auth(username, password, domain) {

Nit

@@ +316,5 @@
> +}
> +
> +function* scramSHA1Auth(username, password, domain) {
> +  // RFC 5802 (5): SCRAM Authentication Exchange.
> +  let gs2Header = "n,,";

const gs2Header...

::: chat/protocols/xmpp/xmpp-session.jsm
@@ +407,5 @@
>            canUsePlain = true;
>          }
>          else if (authMechanisms.hasOwnProperty(mech)) {
>            selectedMech = mech;
> +          //break;

Looks like this was for testing?
(Assignee)

Comment 24

3 years ago
Posted patch v7 - scramSplinter Review
Sorry for the previous nits.
Attachment #8759964 - Attachment is obsolete: true
Attachment #8759964 - Flags: review?(aleth)
Attachment #8759970 - Flags: review?(aleth)
(Reporter)

Comment 25

3 years ago
Comment on attachment 8759970 [details] [diff] [review]
v7 - scram

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

::: chat/protocols/xmpp/xmpp-authmechs.jsm
@@ +32,5 @@
> +
> +  if (stanza.localName != "success")
> +      throw "Didn't receive the expected auth success stanza.";
> +
> +  return;

Not needed.

@@ +418,5 @@
> +  let serverSignatureResponse = atob(attributes.v);
> +  if (serverSignature != serverSignatureResponse)
> +    throw "Server signature does not match";
> +
> +  return;

Not needed.
Attachment #8759970 - Flags: review?(clokep)
Attachment #8759970 - Flags: review?(aleth)
Attachment #8759970 - Flags: review+
Attachment #8759970 - Flags: review?(clokep) → review+
(Reporter)

Comment 27

3 years ago
Landed with last nits fixed.
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Instantbird 50
Depends on: 1281782
You need to log in before you can comment on or make changes to this bug.