Skip to content

Fix TURN failover reusing the previous server's realm and nonce - #446

Open
PerryLink wants to merge 1 commit into
ValveSoftware:masterfrom
PerryLink:fix/turn-failover-stale-challenge
Open

PerryLink wants to merge 1 commit into
ValveSoftware:masterfrom
PerryLink:fix/turn-failover-stale-challenge

Conversation

@PerryLink

Copy link
Copy Markdown

Fixes #445.

The report is accurate, and the state that goes stale is the pair m_strTURNRealm / m_strTURNNonce (and the key derived from them) on the ICE session interface. Every step of the failure is in steamnetworkingsockets_ice_client.cpp:

step where
the 401 from the first server caches its realm, nonce and key :2068-:2071
on timeout the session moves to the next server :2091-:2103
the Allocate sent to that server carries the cached credentials, because QueueTURNRequest only skips authentication while the realm is empty :1032, then :1067-:1076
the second server's own 401 is ignored, because the handler requires an empty realm :2045
control falls through to "mark the relay failed" :2086-:2087

So the request to the second server has the right username and the first server's realm and key, and its challenge is read as a wrong-credentials answer. Configuring a backup only survives the case where the primary never answers, which is what the report describes.

Change

    // Try the next TURN server.  The realm and nonce came from the server that issued them, so they
    // have to be dropped before the next server is asked.  ...
    pIntf->m_strTURNRealm.clear();
    pIntf->m_strTURNNonce.clear();

    pIntf->QueueAllocateRequest( m_vecTURNServers[nNextTURNServerIdx], &CSteamNetworkingICESession::STUNRequestCallback_AllocateRelay, m_nEncoding );

This is the fix tintinhamans reports testing.

m_arrTURNKey is deliberately left alone: QueueTURNRequest returns before touching it whenever m_strTURNRealm is empty (:1032), and it is rewritten from the new realm in the challenge handler (:2071), so it cannot be used once the realm is cleared. Clearing it would need memset in a function that does not currently include <string.h>.

The 401 retry path (:2078) re-queues to the same server immediately after setting that state, so it must and does keep it.

Not in this change

Two things from the report and the surrounding code that are separate decisions, so I left them out:

  • The second half of the report: a server that answers 401 and then keeps rejecting us does not fall through to the next server, only a timeout does. That changes which error responses are retryable, and I would rather not fold it into this fix.
  • Think_DiscoverRelayCandidate (:2000) always restarts from the first TURN server, and it is also the re-allocation path after a refresh timeout (:2140-:2143). If an allocation had previously failed over to the second server, that path re-uses the same stale realm and nonce against the first one. Same stale state, different entry point; the fix above does not cover it.

Verification

Source level, not execution level: I followed the state through the five sites above and each step matches the report, and tintinhamans states that clearing these two members in this function fixed the failure for them.

I could not build or run this here. tests/stun_server.py does implement TURN long-term credentials with a realm and nonce, and test_p2p.py can already give a client its own TURN server, so this is testable end to end: start two servers with different credentials where the first answers the challenge and then drops the authenticated Allocate, and assert that the relay still comes up through the second. That needs the GNS test binaries built, which I do not have in this environment, so I did not attempt it rather than guess at a test I cannot run.

STUNRequestCallback_AllocateRelay walks to the next TURN server once the current one times out, but
the realm, nonce and key left over from the server that issued the 401 challenge are kept.
QueueTURNRequest only skips authentication while m_strTURNRealm is empty, so the Allocate sent to the
next server carries the previous server's realm and key. That server answers 401, the challenge
handler ignores it because m_strTURNRealm is no longer empty, and the relay is marked failed. A
second TURN server therefore only helps when the first one goes silent, not when it challenges and
then goes away.

Drop the cached realm and nonce when moving on to the next server so that it gets to issue its own
challenge. The key does not need clearing: it is only used while m_strTURNRealm is set, and it is
recomputed from whatever realm the next server sends.

Reported in ValveSoftware#445.
@sashakosti

Copy link
Copy Markdown

Verified this change builds cleanly on Linux x86_64 with GCC 16 and Ninja without warnings or regressions. The class invariant m_strTURNRealm is empty until receipt of a 401 challenge documented in steamnetworkingsockets_ice_client.h is properly restored before attempting the fallback TURN server in m_vecTURNServers.

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.

Native ICE TURN failover fails when the servers use different credentials

2 participants