Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add a flag to read banned users from a defined file instead of from openttd.cfg #7957

Open
duckfullstop opened this issue Jan 24, 2020 · 5 comments
Labels
enhancement good first issue

Comments

@duckfullstop
Copy link
Contributor

@duckfullstop duckfullstop commented Jan 24, 2020

The problem

Right now, OpenTTD writes banned users to the [bans] section of openttd.cfg. This works fine for simple dedicated servers and listen servers, but I'm looking to implement more advanced scenarios, like a Kubernetes deployment.

In Kubernetes, configurations can be loaded in with the configMap functionality, which mounts a defined file in the container's filesystem read-only. This works fine, up until the server wants to write stuff to it, at which point we're SOL.

The OpenTTD binary already has the -x flag, which prevents "automatically sav(ing) to config file on exit", which gets us part way to the intended functionality but still means bans don't get saved.

The proposed solution

Add a new flag, such as -B (-b is taken), which reads a list of banned IP addresses (users) from a given file, such as bans.cfg, and adds them to the list defined in openttd.cfg. This will allow the main configuration file to retain static definitions and be mounted read-only, while moving the ban list to a dynamic file which can be loaded separately.

Default functionality should remain loading the bans list exclusively from openttd.cfg to leave existing installations unaffected.

@nielsmh
Copy link
Contributor

@nielsmh nielsmh commented Jan 25, 2020

Alternative idea: Have a pair of console commands, banlist_read and banlist_write that take a filename. Reading replaces the entire banlist in memory, writing stores the banlist to a file in the appropriate format. That could be executed by a startup script, and by an admin port tool. It will also allow you to specify when the banlist should be re-read, which could be useful in case you have an external system that might determine (additional) bans, or want to share ban list between multiple server instances.

@LordAro LordAro added the good first issue label Jan 31, 2020
@Berbe
Copy link
Contributor

@Berbe Berbe commented Feb 22, 2020

The problem is just being moved from the openttd.cfg to another, unspecified, file.

Your base problem is having the configuration mounted RO.
If you write to another file containing bans, and writing to it, how will you save it?
If you manage to write to another file and to save its content, why not mounting openttd.cfg in lieu of this other file?

The way I would build that would be:
1°) Ensure banned users appear in log files, correlated to any useful information (timestamp, username, etc.)
2°) Use log parsing tools (fail2ban? logstash? fluent*? ...? You name it!) to send them to a robust (HA, write-safe) location for storage
3°) Generate openttd.cfg on start based from a template populated with said banned list

Changing the openttd.cfg file for another one does not help this specific problem. Configuration files should be considered trashable in a trashable envrionnements such as containers.

@TrueBrain TrueBrain added the enhancement label Jan 1, 2021
@rpwjanzen
Copy link

@rpwjanzen rpwjanzen commented Mar 26, 2021

@LordAro What is an acceptable solution to this issue? Is it outputting ban information to log files? Is this card still a "good first issue"?

@TrueBrain
Copy link
Member

@TrueBrain TrueBrain commented Mar 27, 2021

There are so many ways to approach this :D If we pick the kubernetes use-case, there are many ways you can manage banlists. For example, I can imagine a CRD which states which IPs are banned, which is being fed into OpenTTD (via a controller connecting to the Admin part). This does mean we need a dynamic way to ban/unban (which we already have via console/admin). The main issue would be that you would have to list all bans, compare it with your "known lists for bans", and delta (add/remove) entries in OpenTTD. This is rather annoying and so far from an atomic operation, that I can imagine this being a pita to implement.
In other words, what the request basically: allow me to do mass operations on bans.
I mostly see this as needed if you host a cluster of OpenTTD servers, where you ban people at once from your whole cluster (synchronize ban-list, basically).

Another use-case I can imagine is with Docker, where you minimize what files are readable/writable. I wouldn't want my openttd.cfg to be writable, but I would like to mount my banlist.cfg on a writeable volume, so it survives Docker restarts.

Personally, I wouldn't go for the route of -B, as that means parts of our configuration are read-only, parts are read-write. This is not really clear to the user, and I can imagine that given a lot of debugging facepalms.

What @nielsm suggests is a bit of all worlds: let the user control it himself via a console/admin command, and we have startup/shutdown scripts that can automate this for the user. For example, this would be excellent to add in https://github.com/OpenTTD/OpenTTD/blob/master/bin/scripts/pre_dedicated.scr.example as example how to use such commands.

Long story short, for me an acceptable solution would also be two new console commands (I would name them save/load, not read/write, but that is bikeshedding):

  • banlist_save, which accepts a filename as parameter and stores the current banlist in a similar format as openttd.cfg
  • banlist_load, which accepts a filename as parameter, which reads the banlist as stored by banlist_save. This will modify openttd.cfg too if OpenTTD is not started with -x, but that, for me at least, is more than fine. You could consider a second parameter that states the merge strategy: "replace" or "extend". With this I mean, if an IP is banned on the server but is not in the list that banlist_load is reading, should it remove the IP or not. But this might be way overthinking this, and you can ignore if you are like: dude, no :)

To retrieve the current list, banlist already exists, so that needs no further work.

This means it is a bit more effort for the server operator to get this to work, but as this is an advanced scenario anyway, I don't expect many issues from that.

In other words: a long post to say: what @nielsmh suggests should work fine. @duckfullstop : do you agree?

And yes, this is a "good first issue". It is fully isolated in the console command system, only adds functionality, and is closely scoped. So that should be a fantastic way to learn a bit about the console system, ini-files, etc :)

@rpwjanzen
Copy link

@rpwjanzen rpwjanzen commented Mar 28, 2021

In that case, I'll pick this up. Hope to have a PR open by Wednesday of this week.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement good first issue
Projects
None yet
Development

No branches or pull requests

6 participants