Skip to content
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

Add TCP transport #490

Open
wants to merge 14 commits into
base: master
Choose a base branch
from
Open

Conversation

lukediamond
Copy link
Contributor

@lukediamond lukediamond commented May 27, 2021

Adds a transport called tcp. Connections to port 5223 are implicitly direct TLS.
Handles unencrypted, STARTTLS, and direct TLS connections. TLS renegotiation is not currently handled.

Additional transport options:

  • directTLS: Explicitly enables direct TLS connections.
  • port: Sets the TCP port to connect to. This overrides the port given in url.

Packages net and tls shim the respective node modules to allow building for web. This change also checks if a transport exists before attempting to create it, so while TCP is the preferred transport on node, it will not be attempted on web.

@lukediamond
Copy link
Contributor Author

lukediamond commented Sep 9, 2021

Tests for transports/tcp.ts are now at 75% coverage. Only STARTLS is tested in test/live-connection.ts, as anon.stanzajs.org seemingly does not like direct TLS connections on port 5223. Logging into a personal account using direct TLS passes and brings coverage up to 80% however.

@lukediamond
Copy link
Contributor Author

@legastero discoverBindings for the TCP transport only returns host:5222. XEP-0368 support is extremely rare, and I haven't found a server which has XMPP SRV records. If direct TLS detection is desirable it may necessary to use a port scanning technique instead of records/host-meta.

@legastero
Copy link
Owner

First off, you've done a good job getting TCP and TLS connections to work here.

Merging and supporting this as a feature in stanza, though, is going to take some more work to prepare for it.

I do recognize that you need this for your Bonfire client, so as you noticed I've started updating the transport code to make it at least possible to load your TCP transport here in your app so you don't have to maintain as many fork changes until stanza gains support for TCP/TLS connections. Some variation of:

const client = createClient({
  transports: { tcp: true },
  transportPreferenceOrder: ['tcp']
});
client.transports.tcp = TCP;

As for what it will take to properly integrate this, here's the shortlist of things:

  1. DNS lookups (not just SRV, but multiple possible A/AAAA records).
  2. Figuring out supporting falling back from TCP to WebSocket or BOSH. Right now, stanza does not fallback to a different transport type; it only uses the first one it discovers connection bindings for. If tcp is enabled as first preference, that blocks any websocket or bosh connection attempts, since a TCP transport will always find a binding (hostname on 5222, if nothing else).
  3. Similar to 2), but it is possible to have multiple possible address/port pairs, and there will need to be a way to cycle through connection attempts with them.
  4. Adding a way to control requiring transport encryption (in particular, that starttls gets used). Right now, everything is required to be https or wss (unless you explicitly configure the client to use a non-secure connection URL).
  5. Adding a way to control the certs and CAs used for TLS.
  6. Adding a way to validate that the server's TLS cert is authorized and matches the desired host (subject alternate names, etc).
  7. Adding a transport method to expose the TLS channel binding data so SASL can use it for SCRAM-*-PLUS.

Basically, arbitrary TLS connections require a lot to ensure they are handled securely in a library. Using just HTTPS/WSS has done a lot of heavy lifting and side stepping in that regard.

@lukediamond
Copy link
Contributor Author

That makes sense. For the time being that snippet seems like a clean way to do transports, although using TCP/TLS in its current state would surely not be as secure as it deserves to be for the reasons you've mentioned. Some thoughts:

DNS lookups

This would only be doable on Node of course, which is already handled now that discoverBindings can be done per transport. The DNS module would likely have to be shimmed as is net/tls.

Falling back from TCP to WebSocket or BOSH

This could just be a matter of connecting within the preference loop and awaiting transport connection success/failure. Success would return and failure would fall through until the No endpoints found for the requested transports error.

Multiple possible address/port pairs

Generalizing "transport candidates" as transports plus endpoint information could be a clean solution to this combined with the priority-order falling through described above. This would take one pass to generate the candidates and another to attempt/pick one. It would be interesting and very likely possible to do this in parallel and use Promise.all or similar.

This would also be very useful for cases where transport encryption is not required (although this should probably be default on). For instance ws:// should never be preferred to https:// even if websocket is higher priority.

Requiring transport encryption

This is very important and technically applies to websocket/BOSH as well. Instead of only using secure websocket/BOSH protocol schemes, you could also use raw HTTP or websocket connections unless a requireSecureTransport option is set. This would have to be a pretty strong guarantee that either it connects via TLS or fails to connect.

Adding a transport method to expose the TLS channel binding data so SASL can use it for SCRAM-*-PLUS.

Combining SASL with the transports would be interesting. Salted hash password challenges would obviously be heavily preferable if requireSecureTransport were disabled. In fact BOSH's keys combined with salted hash authentication could very well prevent an entire session from third-party interference even over unsecured channels, although obviously there would be no privacy.

@lukediamond
Copy link
Contributor Author

lukediamond commented Sep 16, 2021

requireSecureTransport now exists and works, although it likely needs more hardening. Transports are chosen in two stages where [Transport, TransportConfig] pairs are created and then attempted in priority order. Candidates are split into secure and unsecure arrays and the secure array is attempted first. If requireSecureTransport is false the unsecure array is never attempted. Transports will fall through correctly, and lower-priority but higher-security transport options will be selected. Both of these cases are included in live-connection.ts. TCP will now fail if requireSecureTransport is true, the stream is not secure, and STARTLS has not been invoked (this is irrelevant for direct TLS). This has not been tested however as I do not have access to any XMPP servers which do not offer SASL.

Additionally, a cert?: Buffer parameter is accepted and correctly blocks connections to servers with invalid public keys. Currently SPKI/DER binary buffers are expected, such as those generated by crypto, but perhaps PEM would be a preferable format. Other cert checks such as common name/issuer or a general filter function are yet to be implemented and would likely change cert to cert?: string | Buffer | CertificateFilter or something of the sort.

A major architectural change to all transports is the new promise-based structure of Transport::connect and the introduction of --transport-connect and --transport-error. All tested errors that occur within transport connection will now reject connect, including the server using an invalid public key. This was necessary to allow multiple transports to be attempted in sequence, alongside some special care to wait on disconnected events, and the triggering of the handler that fires them from new --transport-error events.

Tests exist for as much of this as possible, especially for security critical features.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants