xmppo / node-xmpp-bosh

An XMPP BOSH & WebSocket server (connection manager) written on node.js using Javascript
https://github.com/xmppo/node-xmpp-bosh
263 stars 85 forks source link

WebSockets don't send <stream:stream> restart after SASL auth #16

Closed astro closed 12 years ago

astro commented 12 years ago

Specified as a direct translation of the TCP protocol, my WebSocket implementation relies on that detail. :-(

Here's some sample output of my client:

>> <stream:stream xmlns="jabber:client" version="1.0" to="example.com">
<< <stream:stream xmlns="jabber:client" xmlns:stream="http://etherx.jabber.org/streams" version="1.0" xml:lang="en" from="example.com">
<< <stream:features xmlns="jabber:client" xmlns:stream="http://etherx.jabber.org/streams" version="1.0"><mechanisms xmlns="urn:ietf:params:xml:ns:xmpp-sasl"><mechanism>PLAIN</mechanism><mechanism>DIGEST-MD5</mechanism><mechanism>SCRAM-SHA-1</mechanism></mechanisms><c xmlns="http://jabber.org/protocol/caps" hash="sha-1" node="http://www.process-one.net/en/ejabberd/" ver="o8zQAtrb2wELMmZizvbnpvqp5cE="/><register xmlns="http://jabber.org/features/iq-register"/></stream:features>
>> <auth xmlns="urn:ietf:params:xml:ns:xmpp-sasl" mechanism="DIGEST-MD5"></auth>
<< <challenge xmlns="urn:ietf:params:xml:ns:xmpp-sasl" xmlns:stream="http://etherx.jabber.org/streams" version="1.0">bm9uY2U9IjI3NzU0MDYxMjciLHFvcD0iYXV0aCIsY2hhcnNldD11dGYtOCxhbGdvcml0aG09bWQ1LXNlc3M=</challenge>
>> <response xmlns="urn:ietf:params:xml:ns:xmpp-sasl">dXNlcm5hbWU9ImNvbGxlY3RvciIscmVhbG09ImphYmJlci5jY2MuZGUiLG5vbmNlPSIyNzc1NDA2MTI3Iixjbm9uY2U9Ijc3MTM6NTk5IixuYz0iMDAwMDAwMDEiLHFvcD0iYXV0aCIsZGlnZXN0LXVyaT0ieG1wcC9qYWJiZXIuY2NjLmRlIixyZXNwb25zZT0iZGY3MWM3ZDM4OTJmODAwZGU3MTIxNjEzMTBiNDUyYTAiLGF1dGh6aWQ9ImNvbGxlY3RvckBqYWJiZXIuY2NjLmRlIixjaGFyc2V0PSJ1dGYtOCI=</response>
<< <challenge xmlns="urn:ietf:params:xml:ns:xmpp-sasl" xmlns:stream="http://etherx.jabber.org/streams" version="1.0">cnNwYXV0aD1lYWQ1NDU4MTAzNDc5NjUwNGE2MjA0OGVlOGNlOTliZA==</challenge>
>> <response xmlns="urn:ietf:params:xml:ns:xmpp-sasl"></response>
<< <success xmlns="urn:ietf:params:xml:ns:xmpp-sasl" xmlns:stream="http://etherx.jabber.org/streams" version="1.0"/>
# At this point I reset my parser, does node-xmpp-bosh do the same?
>> <stream:stream xmlns="jabber:client" version="1.0" to="example.com">
<< <stream:features xmlns="jabber:client" xmlns:stream="http://etherx.jabber.org/streams" version="1.0"><bind xmlns="urn:ietf:params:xml:ns:xmpp-bind"/><session xmlns="urn:ietf:params:xml:ns:xmpp-session"/><c xmlns="http://jabber.org/protocol/caps" hash="sha-1" node="http://www.process-one.net/en/ejabberd/" ver="o8zQAtrb2wELMmZizvbnpvqp5cE="/><register xmlns="http://jabber.org/features/iq-register"/></stream:features>
>> <iq type="set" id="bind"><bind xmlns="urn:ietf:params:xml:ns:xmpp-bind"/></iq>
<< <iq id="bind" type="result" xmlns="jabber:client" xmlns:stream="http://etherx.jabber.org/streams" version="1.0"><bind xmlns="urn:ietf:params:xml:ns:xmpp-bind"><jid>collector@example.com/18674533551332549886610980</jid></bind></iq>

Error: junk after document element
dhruvbird commented 12 years ago

I see the problem. The xmpp-proxy is currently wired to not emit that stanza. Basically, websocket.js is just another front-end, so it will probably (incorrectly) share a lot of behaviour with bosh.

dhruvbird commented 12 years ago

@astro Yes, I believe the parser is reset - not seeing a stream:stream element from the jabber server post sasl auth though... (using jabber.org). Can you point me to the server you are using (with creds - email me).

dhruvbird commented 12 years ago

@astro Pushed something to master - can't be sure if it is correct - let me know if it works.

astro commented 12 years ago

I'm sorry, but this didn't do it:

<< <success xmlns="urn:ietf:params:xml:ns:xmpp-sasl" xmlns:stream="http://etherx.jabber.org/streams" version="1.0"/>
>> <stream:stream xmlns="jabber:client" version="1.0" to="jabber.ccc.de">
<< <stream:features xmlns="jabber:client" xmlns:stream="http://etherx.jabber.org/streams" version="1.0"><bind xmlns="urn:ietf:params:xml:ns:xmpp-bind"/><session xmlns="urn:ietf:params:xml:ns:xmpp-session"/><c xmlns="http://jabber.org/protocol/caps" hash="sha-1" node="http://www.process-one.net/en/ejabberd/" ver="o8zQAtrb2wELMmZizvbnpvqp5cE="/><register xmlns="http://jabber.org/features/iq-register"/></stream:features>
dhruvbird commented 12 years ago

@astro I think it should be fixed now. If not, then you'll have to re-state what the problem is cause I might have misunderstood.

astro commented 12 years ago

Plain awesome, thank you again!

<< <success xmlns="urn:ietf:params:xml:ns:xmpp-sasl" xmlns:stream="http://etherx.jabber.org/streams" version="1.0"/>
>> <stream:stream xmlns="jabber:client" version="1.0" to="jabber.ccc.de">
# Strrream rrrestarrrt!
<< <stream:stream xmlns="jabber:client" xmlns:stream="http://etherx.jabber.org/streams" id="104369161" from="jabber.ccc.de" version="1.0" xml:lang="en">
<< <stream:features xmlns="jabber:client" xmlns:stream="http://etherx.jabber.org/streams" version="1.0"><bind xmlns="urn:ietf:params:xml:ns:xmpp-bind"/><session xmlns="urn:ietf:params:xml:ns:xmpp-session"/><c xmlns="http://jabber.org/protocol/caps" hash="sha-1" node="http://www.process-one.net/en/ejabberd/" ver="o8zQAtrb2wELMmZizvbnpvqp5cE="/><register xmlns="http://jabber.org/features/iq-register"/></stream:features>

When do you plan to release?

dhruvbird commented 12 years ago

@astro Welcome! fyi, (not entirely sure, but) the stream:stream tag is also sent when we do a tls handshake (I think - is that okay?) - since the xml doc. is re-created.

I'll have to ask the release manager @satyamshekhar ! Not sure if we are stable.

satyamshekhar commented 12 years ago

We aren't. Need to test more before we can release. We should release sometime this week. Is that alright?

dhruvbird commented 12 years ago

@satyamshekhar I'm in no hurry as such - anything is fine by me. We can do it at your convenience - once you are reasonably confident of the stability.

astro commented 12 years ago

@dhruvbird <stream:stream> restart after <starttls/> is right on a TCP stream. But do you really want me to try on a Websocket?

@satyamshekhar This week is soon enough for me.

dhruvbird commented 12 years ago

@astro umm... I meant that node-xmpp-bosh will do TLS negotiation on behalf of the client (much like bosh), and the client won't really know of it except that it will suddenly see a stream:stream tag open.

astro commented 12 years ago

Ah. My code will probably be able to handle that, but in general it would be unexpected without the starttls. The same holds true for any duplicate <stream:features/>.

This is how I would do it: if node-xmpp-bosh reacts on <stream:stream> <stream:features>… <starttls/> </stream:features>, it should not relay them to the client. Relay them only later or when missing SSL support.

dhruvbird commented 12 years ago

@astro It's good that the code can handle them.

The reason why the stream:features tag is being sent is because the original client should now know newly updated (and possibly reduced) capabilities of the new stream. The current websocket draft specifically disallows TLS negotiation from the client http://tools.ietf.org/html/draft-moffitt-xmpp-over-websocket-00#section-3.9

I didn't get what you meant to say when you said "Relay them only later or when missing SSL support."

astro commented 12 years ago

When receiving <stream:features/>, a client will react.

Scenario one

Supporting & seeing TLS, the client will attempt TLS negotiation. That will fail with an error.

Scenario two

Not supporting TLS, the client will initiate authantication as next step. Then comes the second <stream:features/> with equivalent content. Now a client must keep track that it is already authenticating.

I am not talking about retaining the second <stream:stream><stream:features/> but the first one that node-xmpp-bosh acts upon.

PS: What are you testing with? Are there any other XMPP/Websockets client-side implementations other than Strophe.js+patches? Can node-xmpp be of help to you?

dhruvbird commented 12 years ago

okay so you mean to say that the first stream:features need not be relayed back to the client per-se? I'll see what can be done about it.

Currently using strophe.js (self patched) for bosh and strophe.js (patch by superfeedr) for websockets. There is a project that we worked on (node-xmpp-via-bosh) which is supposed to be API compatible with node-xmpp - except that it uses the BOSH transport & supports multiple streams. We need to move to that project to test NXB.

Doesn't node-xmpp support only TCP socket transports? It would be great if you could add support for other transports since that would then be the one xmpp client on node.js that supports all transports.

astro commented 12 years ago

Actually, it's not only the first <stream:features/> but the first <stream:stream> too. It will be unexpected by implementations.

The node-xmpp experiments branch does now support BOSH, Websockets, and runs in the browser thanks to browserify. It's all still very rough, but I'm working towards a 0.4.0 release.

dhruvbird commented 12 years ago

@astro Makes sense - will look into preventing the stream:stream that goes out due to a TLS success.

nice! (about the bosh transport) - I can probably just use your tests for nxb!

dhruvbird commented 12 years ago

The stream:stream fix should be on master now. Let me know if you are still having problems with this.