-
Notifications
You must be signed in to change notification settings - Fork 363
Support SASL2 FAST authentication #840
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
| } | ||
| ); | ||
|
|
||
| return elem; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is meant to glue into the old non-event-based SASL1 login system
| */ | ||
| // eslint-disable-next-line class-methods-use-this | ||
| async clientChallenge(connection, test_cnonce) { | ||
| // from https://github.com/xmppjs/xmpp.js/blob/d01b2f1dcb81c7d2880d1021ca352256675873a4/packages/sasl-ht-sha-256-none/index.js#L12 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TODO: check licensing
|
|
||
| authenticate.nodeTree.querySelector("fast")?.setAttribute("invalidate", "true") | ||
|
|
||
| this.conn.send(authenticate.tree()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This never gets sent?? maybe because it's sent during disconnection?
Could someone tell me where is a better place to hook into the logout process?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know you're probably busy @jcbrand (me too!). Perhaps you can just give me a pointer on this logout part. You don't need to review the whole PR yet.
I'm trying to send a logout to the server, which in FAST means sending a stanza using your token to invalidate itself:
<authenticate xmlns='urn:xmpp:sasl:2' mechanism='HT-SHA-256-NONE'>
<initial-response>[base64 encoded SASL data]</initial-response>
<fast xmlns='urn:xmpp:fast:0' count='123' invalidate='true'/>
</authenticate>
but this code happens too late, after the xmpp socket is already closed. Maybe it's racey and succeeds some small percentage of the time, I'm not super sure. I hooked it in with this sketchy code:
Lines 116 to 122 in 3ee9a13
| // **MONKEY-PATCH** connection to catch the logout event | |
| let reset = this.conn.reset.bind(this.conn) | |
| this.conn.reset = () => { | |
| this.logout.bind(this)().then(() => { | |
| reset() | |
| }) | |
| } |
Is there a better hook I could use?
Do I to teach ConverseJS to call this logout() function before actually disconnecting? I hoped not, I hoped to contain most of the FAST stuff in one place. I'll do it if you think that's the best way though.
Thanks :)
| return $build('open', { | ||
| 'xmlns': NS.FRAMING, | ||
| 'to': this._conn.domain, | ||
| ...((this._conn.service.startsWith("wss") || this._conn.service.startsWith("ws://localhost")) ? { 'from': this._conn.jid } : {}), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
from is mandatory for FAST, but is bad for privacy to send over an unencrypted channel. Split the difference by sending only when over TLS.
https://xmpp.org/extensions/xep-0484.html#rules-clients
Clients wishing to use FAST authentication MUST provide the authenticating JID in the secure stream's 'from' attribute. They MUST also provide the a SASL2 element with an 'id' attribute (both of these values are discussed in more detail in XEP-0388).
|
|
||
| const body = this._buildBody().attrs({ | ||
| 'to': this._conn.domain, | ||
| ...(this._conn.service.startsWith("https://") ? { 'from': this._conn.jid } : {}), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| if (data[i] === 'restart') { | ||
| body.attrs({ | ||
| 'to': this._conn.domain, | ||
| ...(this._conn.service.startsWith("https://") ? { 'from': this._conn.jid } : {}), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dda0ad6 to
1cb9581
Compare
FAST is a cookie-style authentication method that lets clients store and auth with an unguesseable token. It enables clients to forget the user's full password, which is especially important for web-based clients, that are prone to data leaks. Leaked tokens can be invalidated. - https://xmpp.org/extensions/xep-0484.html - https://xmpp.org/extensions/xep-0388.html Intended to fix conversejs/converse.js#3144 Some aside changes I needed for this: - I let handlers listen to the *opening* stanza - Set 'from' on the opening <stream> tag. (ref: https://github.com/xmppjs/xmpp.js/pull/1006/files#r1893267922) - Create a type of handler that can search *nested data*. This made setting up listeners a lot more convenient. - During connection, replace has_features with the direct XML <stream:features> more direct and defensive. - Moved Status.AUTHENTICATING before FAST/SASL Still TODO: - support the other HT- methods from the spec - rewrite the SASL code into sasl.js to look like sasl2.js ? - allow fallback from SASL2 to SASL (currently assumes only ONE login method will be tried per connect(), which could block login if one is failing) - pull SASL2 into sasl2.js and make it a plugin - Disentangle the circular dependency between index.js loading sasl2.js/sasl2_fast.js but them needing to talk to Strophe - Invalidate token on logout (and in the corresponding Converse.js branch, actually forget the token on logout)
| test: function (connection, hashName, hashBits) { | ||
| return true; // XXX debug | ||
| return connection.authcid !== null | ||
| && ( | ||
| (typeof connection.pass === 'string' || connection.pass instanceof String) | ||
| || (connection.pass?.name === hashName) | ||
| ); | ||
| }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is here to make sure that we don't accidentally try to SCRAM-SHA-1 when we only have a HT-SHA-256-NONE token available.
| logout: async function () { | ||
| // Invalidate the FAST token on log out | ||
| // XXX this does not seem to actually get sent, | ||
| // and Converse does not forget the token from its IndexedDB | ||
| // if you edit Local Storage using the web debugger to re-add conversejs-session-jid: '[email protected]' | ||
| // and reload then FAST will happily log you back in | ||
|
|
||
| if (this.credential.token) { | ||
| let authenticate = await this.conn.sasl2.authenticateStanza(this.credential.mechanism) | ||
|
|
||
| // XXX copy-pasta | ||
| const response = await this.conn.mechanisms[this.credential.mechanism].clientChallenge(this.conn); | ||
| authenticate | ||
| .c('initial-response', | ||
| null, | ||
| btoa(/** @type {string} */(response))) | ||
| .up(); | ||
|
|
||
| authenticate.nodeTree.querySelector("fast")?.setAttribute("invalidate", "true") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This whole thing doesn't work. In conversejs/converse.js#3693 I made sure the token gets forgotten on logout, so it's not the worst, but it would be ideal to invalidate tokens we're not using.
|
Hi @kousu Thanks a lot for all your effort in making this PR. |
|
Thank you for taking a look. |
FAST is a cookie-style authentication method that lets clients store and auth with an unguesseable token. It enables clients to forget the user's full password, which is especially important for web-based clients, that are prone to data leaks. Leaked tokens can be invalidated.
This my second attempt, and supersedes #839 .
Intended to fix conversejs/converse.js#3144
Some aside changes I needed for this:
Testing
On a prosody server, set these
modules_enabled:Make or pick a test account on your server to test with.
Then run the client with:
Edit converse.js/dev.html to change the prefilled username to match your server (or just be ready to type it in)
TODO:
Potential follow ups:
rewrite the SASL code into an event-based
src/sasl.jsto make it look likesrc/sasl2.jsallow fallback from SASL2 to SASL and between SASL methods
(currently assumes only ONE login method will be tried per connect(), which could block login if one is failing)