1267649 - Support SASL SCRAM authentication mechanism

Status: RESOLVED FIXED

(Chat Core :: XMPP, defect)

Description

[aleth [:aleth]]

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.

Comment 1

[aleth [:aleth]]

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.

Comment 2

[aleth [:aleth]]

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

Comment 3

[Abdelrahman Ahmed [:abdelrahman]]

Attachment: v1 - scram

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

Comment 4

[aleth [:aleth]]

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

Comment 5

[Abdelrahman Ahmed [:abdelrahman]]

Attachment: v1 - remove DIGEST-MD5

Comment 6

[Abdelrahman Ahmed [:abdelrahman]]

Attachment: v2 - scram

Comment 7

[aleth [:aleth]]

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.

Comment 8

[aleth [:aleth]]

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?

Comment 9

[Abdelrahman Ahmed [:abdelrahman]]

Attachment: v2 - remove DIGEST-MD5

Comment 10

[Abdelrahman Ahmed [:abdelrahman]]

Attachment: 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]

> 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

Comment 11

[aleth [:aleth]]

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

Comment 12

[aleth [:aleth]]

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

Comment 13

[aleth [:aleth]]

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.

Comment 14

[Abdelrahman Ahmed [:abdelrahman]]

Attachment: v4 - scram

Comment 15

[aleth [:aleth]]

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.

Comment 16

[aleth [:aleth]]

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?

Comment 17

[Abdelrahman Ahmed [:abdelrahman]]

Attachment: v5 - scram

(In reply to aleth [:aleth] from comment #16)
> In what cases does bad-auth happen?

When user enters a wrong password.

Comment 18

[aleth [:aleth]]

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? ;)

Comment 19

[Patrick Cloke [:clokep]]

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.

Comment 20

[aleth [:aleth]]

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.

Comment 21

[aleth [:aleth]]

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

Comment 22

[Abdelrahman Ahmed [:abdelrahman]]

Attachment: v6 - scram

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

Comment 23

[aleth [:aleth]]

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?

Comment 24

[Abdelrahman Ahmed [:abdelrahman]]

Attachment: v7 - scram

Sorry for the previous nits.

Comment 25

[aleth [:aleth]]

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.

Comment 26

[aleth [:aleth]]

https://hg.mozilla.org/comm-central/rev/71291aefc43aa28f26e6b37e76e142ee232ebb31
Bug 1267649 - Remove DIGEST-MD5. r=aleth

https://hg.mozilla.org/comm-central/rev/7ca98d3ce77c62a79a4442b8dc6d3e4641ffa52d
Bug 1267649 - Support SASL SCRAM authentication mechanism. r=aleth,clokep

Comment 27

[aleth [:aleth]]

Landed with last nits fixed.

Comment 28

[Neustradamus]

SCRAM-SHA-1-PLUS and SCRAM-SHA-256-PLUS are missing because https://bugzilla.mozilla.org/show_bug.cgi?id=563276
People can look?

Tickets:

• For IMAP: https://bugzilla.mozilla.org/show_bug.cgi?id=1503382
• For POP: https://bugzilla.mozilla.org/show_bug.cgi?id=1597102
• For SMTP: https://bugzilla.mozilla.org/show_bug.cgi?id=1597103
• For LDAP: https://bugzilla.mozilla.org/show_bug.cgi?id=1597106
  People can look?

RFCs:

• RFC5802: Salted Challenge Response Authentication Mechanism (SCRAM) SASL and GSS-API Mechanisms: https://tools.ietf.org/html/rfc5802
• RFC7677: SCRAM-SHA-256 and SCRAM-SHA-256-PLUS Simple Authentication and Security Layer (SASL) Mechanisms: https://tools.ietf.org/html/rfc7677 - since 2015-11-02
• RFC5056: On the Use of Channel Bindings to Secure Channels: https://tools.ietf.org/html/rfc5056
• RFC5929: Channel Bindings for TLS: https://tools.ietf.org/html/rfc5929
• RFC5803: Lightweight Directory Access Protocol (LDAP) Schema for Storing Salted: Challenge Response Authentication Mechanism (SCRAM) Secrets: https://tools.ietf.org/html/rfc5803
• RFC7804: Salted Challenge Response HTTP Authentication Mechanism: https://tools.ietf.org/html/rfc7804

IANA:

• Simple Authentication and Security Layer (SASL) Mechanisms: https://www.iana.org/assignments/sasl-mechanisms/sasl-mechanisms.xhtml
• Channel-Binding Types: https://www.iana.org/assignments/channel-binding-types/channel-binding-types.xhtml

Cyrus SASL supports:

• SCRAM-SHA-1
• SCRAM-SHA-1-PLUS
• SCRAM-SHA-224
• SCRAM-SHA-224-PLUS
• SCRAM-SHA-256
• SCRAM-SHA-256-PLUS
• SCRAM-SHA-384
• SCRAM-SHA-384-PLUS
• SCRAM-SHA-512
• SCRAM-SHA-512-PLUS
  -> https://cyrusimap.org/sasl/sasl/authentication_mechanisms.html
  -> https://github.com/cyrusimap/cyrus-sasl/commits/master

Dovecot SASL supports:

• SCRAM-SHA-1
  -> https://doc.dovecot.org/configuration_manual/authentication/password_schemes/

GNU SASL supports:

• SCRAM-SHA-1
• SCRAM-SHA-1-PLUS
  -> http://www.gnu.org/software/gsasl/

CRAM-MD5 to Historic:

• https://tools.ietf.org/html/draft-ietf-sasl-crammd5-to-historic-00 // 20 November 2008

RFC6331: Moving DIGEST-MD5 to Historic

• https://tools.ietf.org/html/rfc6331 since July 2011

More informations:

• https://github.com/scram-xmpp/info/issues/1

Comment 29

[Kai Engert (:KaiE:)]

Neustradamus, please don't paste duplicate contents in a large number of bugs.
In particular, if you're suggesting a new feature, please don't add comments to already resolved bugs. It's sufficient to create new bug reports in the appropriate bugzilla component.