search

xmppjs / xmpp.js

XMPP for JavaScript
ISC License
2.18k stars 372 forks source link

Improve TLSv1.3 compatibility #912

Closed netmikey closed 2 years ago

netmikey commented 2 years ago

As discussed in https://github.com/xmppjs/xmpp.js/issues/889

sonnyp commented 2 years ago

Don't worry about CI, I will fix the tests

sonnyp commented 2 years ago

This PR breaks prosody with xmpp:, xmpps: and ws:

Screenshot from 2021-08-21 23-16-28

If you want to upstream this you'll have to do the following

  1. CI should pass (you can test locally with make test-ci after installing prosody)
  2. The fix shouldn't break other servers (at least prosody)
  3. Add a test see https://github.com/xmppjs/xmpp.js/issues/889#issuecomment-902786001
sonnyp commented 2 years ago

Indeed, whitespaces are not allowed before opening the stream

. MAY send whitespace as separators between XML stanzas or between any other first-level elements sent over the stream

I guess you'll have to find an other workaround or fix.

Have you tried to write an empty string?

netmikey commented 2 years ago

You're right, I was reading that exact same part of the spec while you were replying 😊

I tried with an empty message and it also seems to fix the issue with Openfire's TLSv1.3. This should be even less impactful than the white space. I don't have prosody installed, so let's see if that fixes CI...

sonnyp commented 2 years ago

For the record all you need to do is brew install prosody. make test-ci takes care of configuring, starting, ... I have not tried on macOS though so no guarantees there.

These workaround also worth trying

await Promise.resolve();
await new Promise(resolve => setTimeout(resolve, 500));
netmikey commented 2 years ago

Looks like even the empty network packet broke CI tests. Your ideas were great though, while await Promise.resolve(); didn't work (I guess the runtime short-circuits this), the second variant await new Promise(resolve => setTimeout(resolve, 1)); worked, even with only 1ms of delay. I guess it suffices to wait for the next tick of the runloop before sending the first packet. So weird...

Now that we don't send anything over the network anymore, this should reeeeealy not have any externally visible impact anymore.

sonnyp commented 2 years ago

I have found a flaw in xmpp.js TLS handling that may solve this without the need for a workaround.

I will fix it first to see if it solves your issue. :crossed_fingers:

If not, we can get this in.

sonnyp commented 2 years ago

Could you try with a (fresh) install of xmpp.js 0.12.1 ?

I'm hoping that listening for secureConnect rather than connect might solve your issue. Altough that's what starttls did so I'm not convinced it will...

https://github.com/xmppjs/xmpp.js/releases/tag/v0.12.1

netmikey commented 2 years ago

I tested it with 0.12.1, unfortunately it behaves the same with regard to the TLSv1.3 connection hanging.

"Fortunately", the one-liner proposed in the latest version of this PR still seems to fix it ;-)

sonnyp commented 2 years ago

Couldn't push to this branch for some reason - closing in favor of https://github.com/xmppjs/xmpp.js/pull/914