Support TCP for protocol messages

[softins]

Short description of changes

Support fallback to TCP for protocol messages, in order to overcome potential loss of large messages due to UDP fragmentation. Currently an incomplete draft, for comment as development continues.

CHANGELOG: Client/Server: Support TCP fallback for protocol messages.

Context: Fixes an issue?

Discussed in issue #3242.

Does this change need documentation? What needs to be documented and how?

It will need documentation once design and development are complete. Particularly need to explain the firewall requirements for a server or directory.

Status of this Pull Request

Incomplete, still under development. Main server side complete and working. Client side development in progress. Complete and ready for review and testing. Still marked draft as it needs some of the debug messages to be commented out before merging.

What is missing until this pull request can be merged?

A lot of testing of both server and client. Intended for Jamulus 4.0.0.

Checklist

• I've verified that this Pull Request follows the general code principles
• I tested my code and it does what I want
• My code follows the style guide
• I waited some time after this Pull Request was opened and all GitHub checks completed without errors.
• I've filled all the content above

[softins]

So far, this implements the server side of the design described here and here

[softins]

So the next stage of implementation has been achieved: client-side support in the Connect dialog.

1. If the server list has not been received via UDP when the associated message indicating TCP support has arrived, the client will retry fetching the server list over TCP.
2. If the client list for a server has not been received via UDP when the associated message indicating TCP support has arrived, the client will retry fetching the client list over TCP, and will continue to use TCP for that server while the Connect dialog is open.
3. A directory or server that does not have TCP support will not send the TCP supported message, and will continue to be handled as in current versions.
4. If the server list or client list is successfully received over UDP, there is no need for the client to try TCP.

It has been tested by using nft to drop outbound Jamulus UDP messages with a specific message ID, to simulate loss due to fragmentation.

Examples for a directory-enabled server running on port 22120:

• drop UDP server list: nft add rule inet filter output udp sport 22120 @ih,16,16 0xee03 drop
• drop UDP client list: nft add rule inet filter output udp sport 22120 @ih,16,16 0xf503 drop
• drop UDP "TCP supported" msg: nft add rule inet filter output udp sport 22120 @ih,16,16 0xfb03 drop

Note that nft rules require network byte order (big-endian), but Jamulus IDs are little-endian:

• CLM_SERVER_LIST = 1006 = 0x03ee => 0xee03 (LE byte order)
• CLM_RED_SERVER_LIST = 1018 = 0x03fa => 0xfa03 (LE byte order)
• CLM_CONN_CLIENTS_LIST = 1013 = 0x03f5 => 0xf503 (LE byte order)
• CLM_TCP_SUPPORTED = 1019 = 0x03fb => 0xfb03 (LE byte order)

[softins]

The next step is to try implementing the connected-mode TCP described here

[ann0see]

Since we'll have a long time for the 4.0 release, I'd enable it by default soon (of course once we've tested that the basics work)

[softins]

No, I disagree. It's a server-only option, and most servers operators will not need to enable TCP support. Only those running large directories or large servers will need to, and they also need to understand and configure their firewall requirements.

TCP support in the client will indeed be enabled by default, but will only take effect when talking to a directory or server that has enabled it.

If a server operator enables TCP without having configured their firewall correctly, client users could have problems as the server would advertise TCP support to the client, but the client could be unable to connect.

[ann0see]

Can we not give an error message or fallback procedure in case the TCP connection timed out?

[softins]

Yes, I'm sure we can. I haven't yet tested that scenario.

But it doesn't negate my view that server-side TCP support needs to be an explicit option.

[ann0see]

I assume we have the timer now?
https://github.com/jamulussoftware/jamulus/pull/3636/changes#diff-7bc46552432fa03e009f9f455e4729312853d92775935ef20849797f24ac0c60R292-R300

[pljones]

Since when do we have a long time?

There's absolutely no benefit to anyone involved in the project - developers or users - in having long release cycles.

A version cut takes very little time - apart from the translation process. Even then, getting used to more, small changes is likely to speed things up generally.

[softins]

Well I've finished implementing everything I intended to, for directory, server and client, so it's ready for reviewing and trying out, as and when time permits (post 3.12.0).

I have a private directory and server built and running with TCP support, at newjam.softins.co.uk on the standard port 22124.

In order to demonstrate the use of TCP in a new client's connect dialog, it will be necessary to use custom firewall filters on the client end to temporarily drop incoming UDP Jamulus protocol messages containing a server list or connected clients list.

There is full forward and backward compatibility between clients and servers built with TCP support and older versions.

[softins]

Keeping as draft, because it will need quite a few debug messages removed before merging.

[mcfnord]

Note

📡 STAND BY FOR AN LLM-AUTHORED MESSAGE.

@softins — two things on the pasted review, one of which saves you work.

Its first merge-blocker is already done. CServer::OnCLClientIDReceived rejects on InetAddr.InetAddr != pChannel->GetAddress().InetAddr, after a range and IsConnected() check. The review never fetched server.cpp — it isn't in its citation list — so it reasoned about the association from the prose in #3242. Worth skipping that item.

The part it raised in one clause and then dropped is the one worth sizing: an IP match can't separate two clients at the same address. We operate a fleet of Jamulus servers and a directory-sampling census, so we can put numbers on it rather than argue from intuition.

What the data says

Authoritative client-IP observations at our own servers, 15.9-day window, operator addresses excluded — 592 distinct clients across 626 distinct addresses:

addresses carrying more than one client	83
clients sharing an address with another client	185 — 31.2%
most clients seen behind one address	10

Joining that against per-minute presence data, so this is genuine simultaneity rather than "sometime that fortnight":

• of 35,001 server-minutes with ≥2 known-address clients present, 1,608 (4.6%) had two co-present clients sharing a public address
• 81 distinct (clientA, clientB, server) pairs across 53 distinct servers — about 5 new same-address pairings per day

A separate correlation engine we run reaches the same conclusion independently: its concurrent multi-client-per-address detector fired on 7 distinct addresses in the same window, and one address appears in both datasets. Caveat on the sample: these are players who touched our servers, not a random draw from all Jamulus users.

The direction that I think actually decides it

The check isn't only too weak. It is also too strict.

Our correlation engine's single largest source of failed attributions is carrier NAT re-binding: the mapping expires during a pause and the client reappears on a different egress address, not merely a different port. In the same window, 75 of 592 clients (12.7%) were observed on more than one address.

So a legitimate client whose egress address changes between establishing the UDP session and opening the TCP connection hits rejected mismatched IP address and silently loses the fallback — and carrier-NAT users are exactly the population most exposed to the MTU and fragmentation problems this PR exists to solve. That failure is invisible to the user: they simply don't get large lists, with no error surfaced.

The suggestion

Have the server issue an unguessable handle and have the client echo it on TCP, instead of sending the channel ID. It removes both failures at once, because it stops requiring the address to be stable in either direction.

Two things make this cheap specifically now:

• CLM_CLIENT_ID is new in this PR and deployed nowhere, so widening a 1-byte field costs one commit today and a compatibility shim forever after 4.0.0.
• The channel ID isn't secret in any case — CreateConClientListMes carries chan ID per entry, so every client on a server is already told every other client's ID. There's no guessing involved for a same-address peer.

Keep the address comparison as defence in depth if you like, but as a hint rather than the authenticator — that also lets you log a mismatch instead of dropping a client who did nothing wrong.

If you'd rather not change the wire format at this stage, the minimum floor is to refuse to replace a live association rather than overwrite it: SetTcpConnection is currently a bare setter with no incumbent check, so a second successful CLM_CLIENT_ID silently displaces the first, and the old connection's teardown guard means the displacement survives the victim's socket closing.

Evidence I can produce if it would help

Happy to run any of these — they're queries against data we already hold, not new instrumentation:

1. Overlap duration — how long same-address client pairs are concurrently on one server, so you can judge whether the window is seconds or hours.
2. Mid-session egress change rate — how often a client's address changes while connected, which is the direct predictor of false rejections by the current check.
3. Expected rejection rate — the fraction of TCP associations that would fail the IP test today, as a deployment-risk number for 4.0.0.
4. Same-household vs unrelated — splitting same-address pairs by self-reported city, to separate "two musicians in one house" from carrier NAT sharing strangers.
5. A longer window — the above is 15.9 days because of our retention policy; I can extend it if the sample size looks thin to you.

Also glad to exercise the association path under forced loss with a netns fault-injection harness we built for this PR, if you want the failure modes demonstrated rather than argued.

Happy to be told the risk is acceptable for a first version — that's a reasonable call, and documenting the limitation as the review suggests would be a fine outcome. I'd just rather you make it with the numbers in hand, since the too-strict direction affects working users and not just hostile ones.

[mcfnord]

Regarding the above, it's reasonable to take the numeric statistics with a grain of salt, because IP:identity matching is only about 80% sound. I see a risk of making a wall of text, but I decided to post that comment in response to your comment because I draw from my own statistics (some even reliable) and from a pretty full repo discussion history.

[softins]

But it's a valid suggestion though, to use an un-guessable token to relate the TCP connection to the existing UDP session. I'll see what I can do.

[mcfnord]

MY LLM WROTE:

One more on the TCP path, in the mirror image of a question @ann0see already asked here.

On 2 July, reviewing src/protocol.cpp, ann0see asked "Any security implications if the client sends a wrong id?" and the answer was "Not that I can think of, because I made sure the server will only accept it if it comes from the correct IP address for that channel." That is correct, and CServer::OnCLClientIDReceived does exactly that. The same check is missing in the other direction, on the client.

CClient::OnCLTcpSupportedReceived, case PROTMESSID_CLM_CLIENT_ID: calls

Q_ASSERT ( InetAddr == Channel.GetAddress() );
ConnLessProtocol.CreateCLClientIDMes ( InetAddr, iClientID, PROTO_TCP_LONG );

using InetAddr, the source address of the inbound UDP datagram, where the sibling site CClient::OnClientIDReceived correctly uses Channel.GetAddress(). The Q_ASSERT is not present in shipped builds: qmake on Jamulus.pro writes a wrapper Makefile whose default target is release, Makefile.Release carries -DQT_NO_DEBUG, and linux/debian/rules calls plain qmake with no debug/release override, so the packaged build takes that default.

Nothing filters the source. CSocket::ProcessPacket hands any well-formed connectionless frame to ProtocolCLMessageReceived unconditionally, and EvaluateCLTcpSupportedMes checks only that the body is 2 bytes before emitting. So a single UDP datagram carrying CLM_TCP_SUPPORTED with body 1024 (PROTMESSID_CLM_CLIENT_ID) makes any client in an active session call connectToHost on the sender's address and port. Three consequences, in order of severity:

1. An outbound TCP connection the user never initiated, to an address of the sender's choosing.
2. The client's channel ID is sent to that address.
3. Channel.SetTcpConnection installs the sender's socket as the channel's session connection. Because bIsSession is true for PROTO_TCP_LONG, OnCLConnClientsListMesReceived then takes its IsSession() branch, so that peer can inject an arbitrary connected-clients list into the victim's mixer board.

The third point is the one I would not want to ship. The first two are noise by comparison.

On how reachable this is, being precise: no address spoofing is involved, because the sender needs its real address to receive the connection, so this is reachable off-path rather than only on-path. A client's UDP port is bound in 22124–22224 (DEFAULT_PORT_NUMBER plus a randomised start within NUM_SOCKET_PORTS_TO_TRY), so the port is not a meaningful obstacle. NAT is the real limit: this reaches clients that are not behind NAT, are port-forwarded, or sit behind a full-cone NAT, and does not reach a client behind a port-restricted NAT. It is also reachable by any directory the connect dialog has queried, since those learn the client's address and port.

Suggested fix, 9 insertions and 2 deletions:

    case PROTMESSID_CLM_CLIENT_ID:
        // Only the server we are connected to may ask us to open a session
        // connection. Connectionless messages are not source filtered, and
        // Q_ASSERT is compiled out of release builds, so check this at runtime.
        if ( !( InetAddr == Channel.GetAddress() ) )
        {
            qWarning() << "Ignoring unexpected CLM_TCP_SUPPORTED for client ID from" << InetAddr.toString();
            break;
        }

        // if client ID already received, make TCP connection to server
        bTcpSupported = true;

        if ( iClientID != INVALID_INDEX )
        {
            // *** Make TCP connection
            qDebug() << Q_FUNC_INFO << "need to make TCP connection for client ID" << iClientID;
            ConnLessProtocol.CreateCLClientIDMes ( Channel.GetAddress(), iClientID, PROTO_TCP_LONG );
        }
        break;

Three notes on that shape:

• The check has to sit before bTcpSupported = true. That flag is sticky, cleared only in CClient::Start(), so if an unsolicited message can set it, the next legitimate CLM_CLIENT_ID opens a session connection to a server that never advertised TCP support and stalls for TCP_CONNECT_TIMEOUT_MS against a closed port.
• It is written !( a == b ) rather than a != b because CHostAddress defines operator== but not operator!=, and at C++11 the latter is not synthesised from the former. a != b does not compile here. I assume this is also why the server-side check compares the .InetAddr members instead of the objects.
• Comparing the full address including the port is right on this side, even though excluding the port is right on the server side. The reason the server cannot use the port is that a client's UDP and TCP NAT mappings are independent. There is no equivalent asymmetry client-side: the server sends CLM_TCP_SUPPORTED from the same socket the client is already exchanging audio with, so Channel.GetAddress() matches on both fields.

Verified that the patched file compiles with -fsyntax-only under release flags, and that clang-format leaves it unchanged. I have not opened a PR, per the design-first request. Happy to send it as one, or leave it for you to fold into the branch, whichever you prefer.

Separately, the frame-length bounds check in CTcpConnection::OnReadyRead from my 21 July comments is fixed at 11dbca56 and I am not re-raising it. The general pattern is worth one sentence though: Q_ASSERT on a value that arrived over the network documents an invariant while compiling its enforcement out of every shipped build. Both instances in this PR were of that kind. It may be worth grepping the diff for any remaining Q_ASSERT whose argument derives from a datagram.

[softins]

Cool, I have plenty to be looking at now, and some suggested changes already in progress. Probably best to wait until my WIP is stable and review again then.

[softins]

I have updated the branch with a number of changes based on the review feedback.

The main changes are:

• Exposed the success of creating the IPv4 and IPv6 listeners, using the booleans bTCPv4Available and bTCPv6Available, only offering TCP support to clients on the relevant protocol if really available.

• Added a random 32-bit token to each channel when starting a new session. Pass the token to the client when sending CLM_TCP_SUPPORTED for a client session, and have the client pass it back to the server in CLM_CLIENT_ID over TCP. Check the token matches, instead of checking IP addresses. Checking the IP address matches could, in different situations, be either too loose or too strict, so the token is used as the only authenticator.

• When a client receives a CLM_TCP_SUPPORTED inviting to use TCP for a session, it now verifies that the source of that message matches the server it had opened the session with. This guards against rogue such messages originating from elsewhere.

• When associating a CTcpConnection with a CChannel, handle the unlikely situation that the channel already has an associated connection. Close the old one and use the new instead.

• Log socket errors in CTcpConnection::OnReadyRead(), and also test for and log unexpected EOF (bytes == 0). These situations are expected to be rare.

• Updated docs/TCP.md to mention the channel token.

• Added clearing of pendingClientList and pendingServerList when the connect dialog opens. This was mentioned by a review from Claude via another contributor.

Other responses I have to the review comments are:

• I am satisfied by close inspection that OnReadyRead() correctly handles Jamulus protocol headers or bodies that arrive in multiple small chunks. It knows whether it is receiving a header or a body and only requests the number of bytes required in that state. It only handles the whole body and CRC when it has received a complete message. It will continue reading if there is more data containing a subsequent protocol message. Testing every possible division of a protocol message into multiple small TCP packets seems hard to achieve in practice.

• I need to investigate further the suggestion on checking the TCP connection lifecycle.

• I haven't yet provided any rate-limiting DoS mitigation for TCP connections. Still trying to decide what level of defence is appropriate.

• I don't yet have the server re-offer a TCP session if an existing one drops while the UDP session is still active. This situation should be rare, but I will try and cater for it.

• I have yet to remove the verbose debug messages.

[ann0see]

Great! Thanks.

I still think that we should rename CLM_TCP_SUPPORTED to something more descriptive. CLM_TCP_SUGGESTED ?

[softins]

I still think that we should rename CLM_TCP_SUPPORTED to something more descriptive. CLM_TCP_SUGGESTED ?

I don't think so. We're not suggesting they use it, we are saying it is available to be used if they need to. Maybe CLM_TCP_AVAILABLE? Although I don't feel there's anything wrong with the existing name.

[ann0see]

But SUPPORTED is not the semantics - If it was only used to tell clients that the server supports TCP, it would be enough to have the capability bit set in the newly introduced protocol message.

[mcfnord]

MY LLM WROTE:

@softins Re-reviewed at f1c51f8b. The five items I raised on 21 and 27 July are all addressed — frame-length guard (11dbca56), client-side source check placed before bTcpSupported = true with operator!= added (34f05af9, 9b373653), pending hashes cleared (7519f9f1), existing-connection replacement (a2d2c3b6), and OnDisconnected already guards GetTcpConnection() == this, so the replacement path doesn't unlink the new connection. One new finding, on the token itself.

The channel token is generated by a non-cryptographic PRNG, and the server hands out its raw output on request.

InitChannel uses QRandomGenerator::global() (server.cpp:1568). Qt documents system() as "cryptographically safe"; global() is a securely seeded std::mersenne_twister_engine<quint32, 32, 624, 397, ...> (qrandom.h:189) — plain MT19937, whose tempering is invertible and whose full state follows from 624 consecutive outputs. generate() returns those outputs directly.

That would be a minor nit if the token were defence in depth, but 9bbe8650 removed the IP comparison and made the token the sole authenticator. And the server is the oracle: every new channel consumes exactly one output (the only QRandomGenerator call site in the tree) and sends it to that channel's client. So an ordinary connect/disconnect cycle yields one raw generator output on demand, and channels can be occupied in parallel. Victim channel IDs come from the connected-clients list.

Measured, Qt 5.15.13: 700 values from QRandomGenerator::global()->generate(), untempered the first 624 to recover the state, predicted the remaining 76 — 76/76 exact. Same script against system(): 0/76. Script and both runs available if useful.

With a predicted token, OnCLClientIDReceived links the attacker's socket to the victim's channel, and since a2d2c3b6 that also disconnects the victim's real one. Bounded impact — client-list traffic redirected, victim's TCP session dropped, no audio path — but it is the exact thing the token exists to prevent.

Fix is one word:

vecChannels[iNewChanID].SetChannelToken ( QRandomGenerator::system()->generate() );

Once per channel init, so the cost doesn't matter here. I agree with dropping the IP check — with separate v4/v6 listeners a client's UDP and TCP addresses can legitimately differ — which is precisely why the token has to carry the whole load.

Two small notes:

• iFeatures |= ( bEnableTcp << FS_TCP_ENABLED ) (server.cpp:545) still derives from the flag rather than from bTCPv4Available || bTCPv6Available, so a server whose listeners both failed to bind still advertises TCP. Nothing reads that bit yet, so it's harmless today.
• The CFM_TCP_REQUEST → "switching back to UDP" branch is unchanged, and TCP_CONNECT_TIMEOUT_MS (3000) still exceeds both re-request cadences that reach it: the client-list path at PING_UPDATE_TIME_SERVER_LIST_MS (2500) and the server-list path at SERV_LIST_REQ_UPDATE_TIME_MS (2000). I said "~2.5 s" in July; 2000 is the tighter of the two. Noting it as still open, not re-arguing it.

@ann0see on the name — no opinion on which word wins, but one fact that may bear on it: the capability advertisement already exists separately as FS_TCP_ENABLED (bit 13 of CLM_SERVER_FEATURES, util.h:637), currently set by no reader. Since 9bbe8650 this message is per-message-ID and carries a per-session token, so it is closer to an offer addressed to one client than to a statement about the server.

[softins]

But SUPPORTED is not the semantics - If it was only used to tell clients that the server supports TCP, it would be enough to have the capability bit set in the newly introduced protocol message.

The semantics are that it is an offer, not a suggestion or a requirement. The client decides whether it needs to take up the offer. So I think the closest we would get might be either CLM_TCP_AVAILABLE or CLM_TCP_OFFERED or CLM_TCP_OFFER.

[pljones]

* `iFeatures |= ( bEnableTcp << FS_TCP_ENABLED )` (`server.cpp:545`) still derives from the flag rather than from `bTCPv4Available || bTCPv6Available`, so a server whose listeners both failed to bind still advertises TCP. Nothing reads that bit yet, so it's harmless today.

Just on this, we have two booleans, bTCPv4Available and bTCPv6Available -- moving to using these would argue for two capabilities (or features). But that's why it's "features" rather than "capabilities". It's not what the Server is offering one specific client, it's stating what features it has enabled.

[softins]

* `iFeatures |= ( bEnableTcp << FS_TCP_ENABLED )` (`server.cpp:545`) still derives from the flag rather than from `bTCPv4Available || bTCPv6Available`, so a server whose listeners both failed to bind still advertises TCP. Nothing reads that bit yet, so it's harmless today.

Just on this, we have two booleans, bTCPv4Available and bTCPv6Available -- moving to using these would argue for two capabilities (or features). But that's why it's "features" rather than "capabilities". It's not what the Server is offering one specific client, it's stating what features it has enabled.

Those two booleans are only set on startup to confirm that the TCP listeners were successfully started. It should be vanishingly rare that they wouldn't, but it does allow the server not to offer TCP support if it doesn't have a listener on the corresponding protocol. Each boolean is global to the server, not specific to any client. I agree with AI saying it's better that FS_TCP_ENABLED means the listener(s) were successfully started (which they almost always will be), rather than just that --enabletcp was specified. And I don't think it helps to split them separately for v4 and v6, as they are still one feature, really.

[ann0see]

CLM_TCP_OFFERED

Is probably better then.

[pljones]

Those two booleans are only set on startup to confirm that the TCP listeners were successfully started.

Ah, my misunderstanding.

And I don't think it helps to split them separately for v4 and v6, as they are still one feature, really.

I'm not certain, even, whether TCP is any more a "feature" than IPv4/IPv6. In the same way that you might --enabletcp yet not have a firewall that allows connections (Jamulus wouldn't recognise this), you might use the default of having IPv4 and IPv6 both running -- but you might not be reachable on IPv4 or alternatively on IPv6 (Jamulus wouldn't recognise this, either).

It's easier to probe actively to determine whether a UDP IPv4 / IPv6 port exists by sending a CLM feature req; similarly, the reliably way to find out if a TCP connection is available is to probe with a similar pair of CLM feature req messages. If the feature response comes back, you know what's there. (Obviously this is from a Server's perspective and I'm not suggesting any random IP could send such a message and expect to get a response.)