TShock
TShock copied to clipboard
Pryaxis
Rewriting the ban system
The TShock ban system needs improvements.
The aim of this issue is to discuss what these improvements will encompass. Feel free to chime in with drawbacks to the current system, improvements you'd like to see, what's good, what's bad, etc.
Requesting input from @Zaicon @bartico6 @hakusaro @Ijwu @ivanbiljan @Pryaxis/tshock
My quick 2c on what a new system needs:
- Relational persistence model - the current persistence model is, like all the TShock db tables, a mess. However TShock should manage the bare minimum required to function in order to achieve the below.
- Extensibility - The TShock side should just fire hooks and deal with storing basic information. Plugins should be able to customise the rest
- Enough of a framework that plugins can extend the ban system easily
There are a thousand crappy ways to ban someone, and none of them are good. Here are the problems with the current ban system:
- Right now, we have a table that requires you to have IP as a primary key. This shouldn't be a limit.
- You can't "ban accounts" but you can ban character names. But if a user is offline, then the system bans their account name is if it were a character name. But you can log into a user account from a different character name on some servers, so this makes banning that user ineffective. An aside: it would also be a good idea to just be able to deactivate accounts (like disable them in the db).
- Players can join with their name as an IP address, but then banning them becomes ambiguous because it could be an IP or an IP name. Note: this can be solved at the command level but I didn't want to get into the woods of what would or wouldn't happen with it.
- There's ambiguity in basically everything. Are you referring to an online player? An offline player? An offline player while a different online player has that name?
- Utils has a ban method and so does the BanManager and they do different things and one calls the other.
- Every stupid field in the database is a string. Well, shoutouts to the IP field because it's an actual MySQL string that's char capped at 16 characters. So if Terraria ever goes to IPv6 we're hard coded to not support banning v6 addresses.
- Right now, if you ban a thing, the ban system bans anything it can find on that thing. We don't have the ability to just say "ban only this character name" or "ban only this IP" without also getting UUID or other fields filled in for us.
- You can't ban on IP ranges or DNS resolutions or hostnames.
- You can't ban on details about a character, which may uniquely identify them (e.g., costume, inventory item position and stack counts, etc. Basically all of the SSC data we know).
There are two primary approaches to bans, and I think we should stick to one or the other, not co-mingle both:
- Bans are applied to a [person] and that [person] is represented by IP, UUID, whatever. Any time anything trips a flag that might link back to [person] (like, join from a different IP and /login to a banned account) the ban system should keep them out for good.
- Bans are applied to data fields about someone. Instead of banning a [person] you ban their [IP] or their [character name] or their [inventory contents] or their [character uniform] or their [account name].
From a usability perspective, both systems can become a nightmare. I'm personally more of a fan of the first option, but there are big reasons for IP bans. Things like IP range banning and other things aren't currently supported by our system and that needs to change. But I almost feel like IP bans are the "nuclear" option and tend to shy away from them myself.
From reading the above, I'd say our primary problem is figuring out what the most important field of a ban is - i.e., for any given player, what data identifies them the most consistently.
Account name is great, but players without account names become unbannable. And it's easy enough to make a new account. IP addresses are unique, but easy to change, proxy, etc. Character names can be changed at will. UUIDs can also be changed at will.
It's a real shame Terraria doesn't send Steam IDs or some unique identifier that isn't easily modifiable.
How about the lightest bridge possible:
User joins server Server sends request to intermediary Server tells user to go to intermediary and enter six digit pin User goes to intermediary and enters six digit pin If user is cookied, server is notified instantly and login completes If user is not cookied, user completes steam OpenID authentication and then login completes Server now has SteamID representation of user on server
imo that's still too much overhead for the use case that most people have. This kind of thing needs to be done with as little removal from the game as possible
¯\(ツ)/¯ it's an option. It's the fastest/easiest bridge I can do with minimal overhead. If a server owner is concerned about bans sticking, imho, they could flip a toggle and have it on.
More details:
- Would require no server enrollment to get features.
- Off by default
- One flag to turn on
- One flag to reject non-owners of Terraria
- One flag to reject vac bans
- Could also be a one-time thing during the registration flow (requires no more authenticating other than that)
The primary key in the ban database should be the ban id, and there should be a ban id. You'd ideally want to be able to bring up a ban history...
Other than that, you should also be able to lock an account within the ban command, and, hopefully, be able to choose if you're banning their account, ip, or both.
Bans that expired should not be removed, they should instead be marked as expired..
or you can just fetch bans that expire later than UNIX_TIMESTAMP() and that don't have a negative expiry timestamp (which are perms)...
You should also make unbanning, as mentioned above, set a flag that it was unbanned along with a reason, who did it, and when was it done.
I'm just scratching the surface here. Out of all the features, blacklisting access to your service (in this case tShock servers) should be robust and powerful, but also usable.
A ban ID doesn't uniquely identify anything except a ban record. A properly structured table should not need arbitrary identity columns - you should be able to uniquely identify any given record by an attribute or set of attributes.
I would argue that the above pictures display a table that is trying to do far more than a DB table should.
In particular, having a column for unbantype implies, to me, that there should be a secondary table for different types of bans/unbans, and the main table should only hold generic data that is common to all types
BanID is for identifying a specific ban.
You can have more than one ban per player, such as a history - ideally you'd like to be able to hand-pick one of them, and a primary key IP address will stop that. You really want to keep track of their previous bans, this is "selecting punishment 101" -> see their history on the server. In its current state tShock doesn't enable that.
You need an auto-increment ID to allow duplicate entries like that.
Unban*** works as part of the specific ban object (which also means, part of the single entry) - information on when and how that one ban was revoked.
What about a DB table with 3 fields:
- autoincrement ID
- Type (int, enum)
- Data (varchar 255)
And then types can be added and data can be added and hooks on login and join check for whatever they want?
You can ban as many different sources as you want. Unban can just target any types of data or an offline player and remove all known attributes?
I wanted to update this with a discussion I had with @QuiCM yesterday:
- Use a primary key derived from hashing unique factors about the original ban to create a "per ban" primary key.
- Store the details about said ban in a separate database table using that PK and with additional metadata.
I believe I noticed that we would probably need to rewrite a lot of the TShock database infra to get this to change. I would much rather move to a database ORM like Shaolinq as opposed to trying to patch the SqlColumn + SqlTable + SqlValue nonsense we have sitting around.
@bartico6 to answer your question:
- A Steam64 ID
- An IP address
- A character name
- A player name
Where the type field would identify what the data is.
I'm curious as to how you plan to pull a SteamID64 in the current game version in a normal server not running through steam. If you're saying "Steam64 in the future" then yeah, I know what you're on about. I'd also like to mention that you might want to log all relevant data about the banned user (but only block by specific factors).
For example, if you ban me with IP 127.0.0.1, character name "_q" and username "quake1337", all of those three should be shown in the ban data (for the purpose of even the most basic stuff like showing ban info (and easily identifying WHO is the victim of the ban as well as easier catching of evaders).
Sorry, I don't understand how your question furthers the discussion. It seems like you're being argumentative for sport. It was an idea not a god given assurance.
It occurred to me that one of the biggest issues with using an arbitrary data field w/ a character limit is that you can create a situation where a potential new use case can't be accounted for. In that solution, it might be worth considering a larger data pool (say, if you wanted to ban someone based on their inventory).
I think, right now, my inclination is to go with a meld of @QuiCM's system and an arbitrary data system.
Table 1 (Metadata):
| Ban Identifier | Idealized Target | Reason | Expiration |
|---|---|---|---|
| sha2(original target) | Ash | Hacking & cheating | Nov 29, 2018 |
Table 2 (Match/filter data):
| Ban Identifier | Data Type | Data Identifier |
|---|---|---|
| sha2(original target) | IP | 172.284.193.294 |
| sha2(original target) | Character name | Ashes |
| sha2(original target) | Account name | Cynthia |
| sha2(original target) | Steam64 | 76561197999258201 |
Where sha2(original target) is like sha2(target selector + time). The target selector would just be the information that was used to construct the original ban (like, time and IP or time and character name). This might be 100% of @QuiCM's system actually. The ID could just be an autoincrement or a UUID too, but I don't know which route is best to go on.
The benefit to using arbitrary data is that an enum can track types and the system can expand without needing table structure modification. The code still has to change to check for new types, but this would at least work in that way. Obviously, when you run a ban, we would want to add as many identifiers as possible, but removal of the ban would only require the idealized target or a specific detail of information that could then be checked in the lookup table and the entire ban expunged from one direction. Arbitrary data would also let you collapse both of these tables together if we wanted to use the existing TShock database system.
A potential rewrite should include all of the following matchers:
- Character name
- Account name
- IP address
Ideal bonus types that could be added:
- IP range
- Steam64
Ideally, if a target joined with only one set of metadata added, we would just update their ban information to match all of the new stuff. So an original ban that has the following characteristics:
- IP
- Character name
- User name
Could be updated if a user joins with a new IP and character name, but logs into their account. We could then add another IP and character field (optionally) or update the filter to track them. That's more about ban evasion detection, though, and I don't know 100% of how likely it is that people do or do not need this type of tracking.
"Ok, Google. Remind me to comment on issue #1511 tomorrow at 4PM."
Give me some time to think about this. :)
@hakusaro Your proposed table structure with the metadata + match tables looks good. I actually hit up one of our DBAs and posed this as a question and he sees it as a good, standard, solution. 👍
idk how to do a pretty table but I was thinking more along the lines of
SCHEMA(Time [CK, datetime], Target [CK, varchar], Expires [datetime?]) = Ban table
SCHEMA(Time [FK, datetime], Target [FK, varchar], Type [varchar/enum/whatever], Data) = Ban details
I don't see any reason to hash stuff together, especially not the ban time - you're losing data that way (e.g., what date/time was the ban created?)
Time and Target are a composite primary key in the Ban table. No need for a surrogate key because no two bans should share the same time and target, meaning you could feasibly enter multiple bans for the same user. In Ban details, Time and Target are a composite foreign key to the Ban table.
The Ban table stores the minimum required data to identify a ban. The Ban details table stores whatever someone's ban implementation needs to store. The ban system should be abstract enough that people can introduce their own tables if they need different data stored, and all they need is to FK the same properties
Commands need to support specifying only IP or only character, etc. ban. The whole system needs to be rebuilt to this should be trivial.
Ideally there shouldn't be a "delip" or "del" variation -- just one add, delete, etc, with unix style command line flags to skip things around.