Closed
Bug 1267649
Opened 9 years ago
Closed 8 years ago
Support SASL SCRAM authentication mechanism
Categories
(Chat Core :: XMPP, defect)
Chat Core
XMPP
Tracking
(Not tracked)
RESOLVED
FIXED
Instantbird 50
People
(Reporter: aleth, Assigned: abdelrahman)
References
Details
Attachments
(2 files, 7 obsolete files)
(deleted),
patch
|
aleth
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
aleth
:
review+
clokep
:
review+
|
Details | Diff | Splinter Review |
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 | ||
Comment 1•9 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•9 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
Reporter | ||
Updated•8 years ago
|
Assignee: nobody → ab
Status: NEW → ASSIGNED
Assignee | ||
Comment 3•8 years ago
|
||
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•8 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•8 years ago
|
||
Attachment #8757691 -
Flags: review?(aleth)
Assignee | ||
Comment 6•8 years ago
|
||
Attachment #8757651 -
Attachment is obsolete: true
Attachment #8757651 -
Flags: review?(aleth)
Attachment #8757692 -
Flags: review?(aleth)
Reporter | ||
Comment 7•8 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•8 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•8 years ago
|
||
Attachment #8757691 -
Attachment is obsolete: true
Attachment #8757968 -
Flags: review?(aleth)
Reporter | ||
Updated•8 years ago
|
Attachment #8757968 -
Flags: review?(aleth) → review+
Assignee | ||
Comment 10•8 years ago
|
||
(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•8 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•8 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•8 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•8 years ago
|
||
Attachment #8757972 -
Attachment is obsolete: true
Attachment #8758064 -
Flags: review?(aleth)
Reporter | ||
Comment 15•8 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•8 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•8 years ago
|
||
(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•8 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•8 years ago
|
Attachment #8759275 -
Flags: review?(aleth) → feedback+
Comment 19•8 years ago
|
||
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•8 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•8 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•8 years ago
|
||
(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•8 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•8 years ago
|
||
Sorry for the previous nits.
Attachment #8759964 -
Attachment is obsolete: true
Attachment #8759964 -
Flags: review?(aleth)
Attachment #8759970 -
Flags: review?(aleth)
Reporter | ||
Comment 25•8 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+
Updated•8 years ago
|
Attachment #8759970 -
Flags: review?(clokep) → review+
Reporter | ||
Comment 26•8 years ago
|
||
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
Reporter | ||
Comment 27•8 years ago
|
||
Landed with last nits fixed.
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Instantbird 50
Comment 28•5 years ago
|
||
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:
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:
Comment 29•5 years ago
|
||
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.
You need to log in
before you can comment on or make changes to this bug.
Description
•