Adjustments to work better with cloudflare and reverse proxies #110
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
While it's perfectly understandable to not show IPs to mods or players, it's bog standard practice to log the IP address on connections. Every webserver does this. Since client.ip was actually a getter for ipid, replace every instance of client.ip with ipid. Will note that ipid is a very misleading name. It's a numeric, unique ID of each client and in principle it has nothing to do with IP.
Rename TransportWrapper to WSTransport and make it a proper subclass. Implement comprehensive checking of X-Forwarded-For using proxy manager. Make get_extra_info more robust by using super().
Tsuserver already has a lot in its class, and checking for valid client connections is probably more suited in the clientmanager class. Valid client connections includes checking for bogus proxies, ip range bans and some other things.
OmniTroid
changed the title
|
It looks like WS and TCP connections are working correctly now, as well as rangebans. |
|
review plz |
Crystalwarrior
approved these changes
Jul 16, 2024
There was a problem hiding this comment.
Not my area of expertise, I'll mark it as approved but live testing is absolutely needed @OmniTroid
OmniTroid
changed the title
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Rationale
The current setup assumes that when an incoming connection is established with X-Forwarded-For, the reverse proxy is always hosted on the same server (127.0.0.1). However, this may not always be the case. Cloudflare's normal proxying setup connects from their servers, so we should trust those as well. To facilitate this, we implement a new class and refactor some of the connection handling code.
IPIDs
While it's perfectly understandable to not show IPs to mods or players, it's bog standard practice to log the IP address on connections. Every webserver does this, so implement it here too.
Since client.ip was actually a getter for ipid, replace every instance of client.ip with ipid.
Will note that ipid is a very misleading name. It's a numeric, unique ID of each client and in principle it has nothing to do with IP.
It also seems that IPs are completely absent from the sqlite bans table? This is actually baffling.
ProxyManager
In order to get the "true" IP of a client connecting through a (reverse) proxy using websockets, we need to check the X-Forwarded-For header. There is, however, a catch. The X-Forwarded-For header can be set arbitrarily, so if it exists on the connection, the server needs to check that the request originates from a trusted proxy (eg. cloudflare). If it does not, the connection is considered untrustworthy and should be rejected. In order to facilitate this, I made a new class to handle setting up this proxy whitelist and check if a connection is valid or not.
Move client connect logic into client manager
Conceptually, there is a number of checks a connection must pass before being promoted to a client (not being banned, not claiming to be an unauthorized proxy). I think this is the client manager's job, so to speak, so these checks have been moved there. It is somewhat odd that we check for rangebanned IPs, but not for specific IP bans at this stage.
Changes in TransportWrapper
TransportWrapper has been renamed to WSTransport and made a proper subclass of asyncio.Transport. Comments suggest this was first implemented as a wrapper, but has clear signs of overriding features. I think there's a better way to design this. Also removed the get_extra_info overload and replaced it with get_client_ip in client manager.
Things that need to be tested before merging
Connections, bans and rangebans from TCP and WS connections. (could be covered by autotests. the complexity of this project more than warrants a handful of them).