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.

Sign up for GitHub

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

Jump to bottom

Change: move sensitive information to secrets.cfg and private information to private.cfg #9298

Merged
merged 4 commits into from
Jul 2, 2021
Merged

Change: move sensitive information to secrets.cfg and private information to private.cfg #9298

merged 4 commits into from
Jul 2, 2021
+697 −416

Conversation

TrueBrain
Copy link
Member

@TrueBrain TrueBrain commented May 27, 2021 •
edited
Loading

Motivation / Problem

While working on the STUN patch (#9017), I wanted to store a secret on the client. The only place we currently have to do this, is openttd.cfg. This has a huge draw-back for several reasons:

  • We ask for openttd.cfg in bug-reports, basically telling the world about the secret.
  • People tend to share their openttd.cfg pretty easily with others, sharing their secret too.

In my case, this would mean that getting your hands on an openttd.cfg meant you could claim the invite-code, and do a hostile take-over of their server.

A closer look made us realise that storing secrets in openttd.cfg is already an issue: default_company_pass, rcon_password, etc. In general it is not wise to store this information in the same file as all other configuration, especially as it is shared this easily.

Description

In result, I set out to find a way to create a secrets.cfg which contains secrets people should not be sharing. The name should make that obvious enough.

Additionally, as suggested by @rubidium42 , I also created a private.cfg to store things like:

  • server-name
  • client-name
  • last-joined
  • server-bind-addresses
  • servers
  • bans

Basically, everything with identifiable information. This information people shouldn't be sharing freely either, but aren't exactly secrets.

When you start the game the first time with/after this PR, while already having a settings file from before, it will automatically migrate the settings to the new files and remove it from openttd.cfg. This is a one time migration.

Limitations

  • These secrets cannot be sync'd over the network, or stored in the savegame. I think that goes without saying that they also shouldn't be sync'd over the network or stored in the savegame .. that would defeat the purpose of calling it a secrets/private ;) But the code also really doesn't allow it.
  • I am unsure if adding a IniFileVersion is really needed. I could also just add a flag or indicator. Not sure if this would ever be used again in the future. But I need at least something to know you have been past this PR with your configuration .. so alternatives for this are more than welcome.

Checklist for review

Some things are not automated, and forgotten often. This list is a reminder for the reviewers.

  • The bug fix is important enough to be backported? (label: 'backport requested')
  • This PR affects the save game format? (label 'savegame upgrade')
  • This PR affects the GS/AI API? (label 'needs review: Script API')
    • ai_changelog.hpp, gs_changelog.hpp need updating.
    • The compatibility wrappers (compat_*.nut) need updating.
  • This PR affects the NewGRF API? (label 'needs review: NewGRF')

@TrueBrain TrueBrain marked this pull request as ready for review May 28, 2021 08:15
rubidium42
rubidium42 reviewed May 28, 2021
View reviewed changes
Copy link
Contributor

@rubidium42 rubidium42 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally I think that the ini not removing settings that are not known to it is not a bad thing. This means switching between development/main/beta/rc and release won't remove all your development/main/beta/rc settings.

However, here I would propose implementing something that does remove them from the settings but only under certain circumstances. For me that would be IsReleasedVersion() and _openttd_newgrf_version > 1 << 28 | 12 << 24, or any release version 1.12.0 or later. By then it is relatively unlikely that you go back to an earlier version regularly, so it would not burden people switching between development and release builds.

Similarly I think, for the average Joe, it would be better to migrate the settings. Or rather, when it is not set in secrets.ini, take the value from settings.ini (you already have the ini file loaded). The removal of the settings should of course come after this. Both of these are not necessarily part of this PR.

The server-name and client-name are not really secret, it's mostly not useful to have them in a settings file that is used by others, though I'm not really sure that warrants them being secrets. Currently last-joined and servers do not really matter, though if those get join keys that might become slightly more problematic. The server-bind-addresses are not really a problem with respect to secrecy, and other people cannot use that information to fake a server. After all, they do not own that IP, or it is a local IP.
The bans are a bit tricky, though maybe this is the time to consider the separate banlist ini file.
So maybe have a private_secrets.cfg and private_settings.cfg, where passwords and unique IDs go in the former file and other things that should not be distributed in settings.cfg would go in the latter file such as the (last joined) servers. Yes, private secrets are a bit double in the naming, but I hope that brings over the point that they are really private and secrets so people are less likely to distribute them.

@TrueBrain
Copy link
Member Author

Similarly I think, for the average Joe, it would be better to migrate the settings. Or rather, when it is not set in secrets.ini, take the value from settings.ini (you already have the ini file loaded). The removal of the settings should of course come after this. Both of these are not necessarily part of this PR.

I really would like to have the migration in this PR too, but my earlier attempts failed. But I now have an idea how I possibly can pull it off :) Tnx to some rewrites someone did :P

So maybe have a private_secrets.cfg and private_settings.cfg, where passwords and unique IDs go in the former file and other things that should not be distributed in settings.cfg would go in the latter file such as the (last joined) servers. Yes, private secrets are a bit double in the naming, but I hope that brings over the point that they are really private and secrets so people are less likely to distribute them.

I like this approach. I will make it so :)

@TrueBrain TrueBrain marked this pull request as draft May 28, 2021 20:02
@TrueBrain
Copy link
Member Author

This current version doesn't work as I want it .. our ini-loader is rather annoying :P

I tried to load first from openttd.cfg next from secrets.cfg. But .. that doesn't do anything, as every entry in the _secrets_settings not found is cleared. So .. yeah .. not done yet :) I have some ideas, but it involves rewriting some more code.

glx22
glx22 reviewed May 28, 2021
View reviewed changes
@TrueBrain
Copy link
Member Author

TrueBrain commented Jun 28, 2021 •
edited
Loading

Finally had a good idea how to do the migration ... by introducing an ini-file version :D I initially tried the newgrf-version, but that would mean people using the nightlies wouldn't get the migration. So in the end, I considered it is best to just have a version to indicate if we should do the migration or not.

That does mean that if you go back to before this PR, those fields reset. But if you go past this PR again, as ini_version is not changed, it will not load (or remove) those fields from openttd.cfg again. It really is a one-time action.
I think this is the best solution for users, but I am open to other suggestions.

However, I am unsure if adding a IniFileVersion is really needed. I could also just add a flag or indicator. Not sure if this would ever be used again in the future. But I need at least something to know you have been past this PR with your configuration .. so alternatives for this are more than welcome.

Also, there is now private.cfg to store private information, like name, servers, etc.

@TrueBrain TrueBrain changed the title Change: move secrets out of "openttd.cfg" into "secrets.cfg" Change: move sensitive information to secrets.cfg and private information to private.cfg Jun 28, 2021
@TrueBrain TrueBrain marked this pull request as ready for review June 28, 2021 15:05
@TrueBrain TrueBrain mentioned this pull request Jun 29, 2021
Closed
rubidium42
rubidium42 reviewed Jul 1, 2021
View reviewed changes
@TrueBrain
Codechange: move network-related settings out of settings.ini
77151ad 
This to prepare the code to split up network-related settings
into private / secrets / generic.
@nielsmh
Copy link
Contributor

nielsmh commented Jul 1, 2021

Can I suggest writing a short comment at the top of secrets.cfg when it gets created, explaining that these are secrets belonging to the specific installation, and isn't something supposed to be secret from the user. I think some might misunderstand the filename as "these are the secrets of the OpenTTD project and now I can go hack everyone". I don't have any good suggestions for phrasing right now.

@TrueBrain
Copy link
Member Author

TrueBrain commented Jul 1, 2021 •
edited
Loading

Can I suggest writing a short comment at the top of secrets.cfg when it gets created, explaining that these are secrets belonging to the specific installation, and isn't something supposed to be secret from the user. I think some might misunderstand the filename as "these are the secrets of the OpenTTD project and now I can go hack everyone". I don't have any good suggestions for phrasing right now.

To put IRC here:

I am not too worried that people confuse this secrets file with one that would allow access to anything owned by OpenTTD. I give our users a tiny bit more credit ;) But there is validity in your suggestion, as we should make a lot more clear that people should not be sharing secrets.cfg with anyone.

Sadly, IniFile doesn't really allow that (don't be fouled by the comment field .. it is just "the junk between this group and the last", including \ns). So what I cooked up is this:

When writing secrets.cfg (and private.cfg) I add a group first called secrets (or private), which has a comment before it starts stating what the file is about. This means that as long as the file was empty when we first create it, this group will be on top, and so the comment will be at the top of the file.

The small downside of this approach is, that it will always overwrite this comment, so if people manually alter it, it will be overwritten on next save. This shouldn't be a problem at all, but before this change, ini-files always kept what-ever spacing/comments was between groups.
Edit: I changed my mind, and it now only writes this when the secrets group doesn't exist yet. That should prevent changing anything if the user manually makes alterations.

rubidium42
rubidium42 reviewed Jul 1, 2021
View reviewed changes
TrueBrain added 3 commits July 1, 2021 20:41
@TrueBrain
Codechange: C++-ify the usage of IniFile in settings.cpp
1cabd5d 
Instead of creating the object on heap and use a pointer, create
the object on stack and use a guaranteed-not-null pointer.
The size of IniFile doesn't warrent the forcing to heap.

Additionally, use a subclass instead of a function to do some
initial bookkeeping on an IniFile meant to read a configuration.
@TrueBrain
Codechange: simplify SaveVersionInConfig()
8b5b665 
Clearly someone really wanted to generalize the function, but
in reality it makes it a lot longer than needed. Let's keep it
simple.
@TrueBrain
Change: move sensitive information to secrets.cfg and private informa…
905dd7b 
…tion to private.cfg

We often ask people for their openttd.cfg, which now includes their
passwords, usernames, etc. It is easy for people to overlook this,
unwillingly sharing information they shouldn't.

By splitting this information over either private.cfg or secrets.cfg,
we make it more obvious they shouldn't be sharing those files, and
hint to what is inside them.
@TrueBrain TrueBrain merged commit 75b6051 into OpenTTD:master Jul 2, 2021
@TrueBrain TrueBrain deleted the secrets-settings branch July 2, 2021 12:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@glx22 glx22

@rubidium42 rubidium42

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@TrueBrain @nielsmh @glx22 @rubidium42