Skip to content

Match session takeover on transport identifier only#73

Closed
fireb0rngg wants to merge 1 commit into
Extremelyd1:mainfrom
fireb0rngg:fix/session-replacement-authkey
Closed

Match session takeover on transport identifier only#73
fireb0rngg wants to merge 1 commit into
Extremelyd1:mainfrom
fireb0rngg:fix/session-replacement-authkey

Conversation

@fireb0rngg
Copy link
Copy Markdown
Contributor

@fireb0rngg fireb0rngg commented May 26, 2026

Because AuthKey is shared across all clients launched from the same install, two local clients connecting to the same server caused the second connection to disconnect the first. This makes locally testing dev builds difficult. My mistake from a previous PR.

ReplaceExistingSessionIfPresent matched on UniqueClientIdentifier OR AuthKey. Because AuthKey is persisted in ModSettings and shared across all clients launched from the same install, two local clients connecting to the same server caused the second connection to displace the first. Restrict the match to UniqueClientIdentifier, leaving the legitimate same-transport reconnect path intact.
@Liparakis
Copy link
Copy Markdown
Contributor

Thank you for putting this pull request together. While I understand this was introduced to solve a specific local-testing convenience issue -- specifically, allowing multiple local client instances launched from the exact same installation folder to connect without displacing one another; we cannot merge this change in its current form.

Removing the AuthKey from the session replacement validation introduces critical functional bugs and security vulnerabilities for production environments:

  1. Breaking Legitimate Player Reconnections (Critical UX Bug)
    In standard UDP/DTLS networking, GetUniqueIdentifier() tracks the client socket endpoint (IP:Port).
    Whenever a production player experiences a transient network disconnect (e.g., brief packet loss, router DHCP renewals, or switching from Wi-Fi to cellular), their local network stack binds to a new random ephemeral port. This changes their UniqueClientIdentifier (e.g., from X.X.X.X:49152 to X.X.X.X:49153).
    Because the AuthKey check is removed, the server will fail to match the reconnecting client to their active stale session. Instead of displacing the old connection, the server will reject the player with the error "Username already in use", locking them out until the original session times out.

  2. Security Risks & Session Vulnerabilities
    No Cryptographic Verification for Takeovers: The 56-character AuthKey is the only cryptographically sound way the server can verify that a reconnecting client is actually the owner of the active session. Relying strictly on transport-level metrics like IP/port is extremely fragile.
    Shared-IP Disconnect Collisions: In CGNAT, university campus, or shared residential LAN environments, multiple players share the exact same external public IP. Depending on the transport setup, removing cryptographic token validation leaves the session-replacement path vulnerable to accidental disconnections or spoofing.

@fireb0rngg
Copy link
Copy Markdown
Contributor Author

Agreed, thanks for reviewing.

@fireb0rngg fireb0rngg closed this May 28, 2026
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.

2 participants