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

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

Merged
merged 4 commits into from Jul 2, 2021

Conversation

@TrueBrain
Copy link
Member

@TrueBrain TrueBrain commented May 27, 2021

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 force-pushed the secrets-settings branch from ecd283d to fb72fa1 May 28, 2021
@TrueBrain TrueBrain marked this pull request as ready for review May 28, 2021
Copy link
Contributor

@rubidium42 rubidium42 left a comment

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.

Loading

src/settings.cpp Outdated Show resolved Hide resolved
Loading
src/settings.cpp Outdated Show resolved Hide resolved
Loading
src/table/secrets_settings.ini Outdated Show resolved Hide resolved
Loading
src/table/secrets_settings.ini Outdated Show resolved Hide resolved
Loading
@TrueBrain
Copy link
Member Author

@TrueBrain TrueBrain commented May 28, 2021

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 :)

Loading

@TrueBrain TrueBrain marked this pull request as draft May 28, 2021
@TrueBrain TrueBrain force-pushed the secrets-settings branch from fb72fa1 to cb42fc3 May 28, 2021
@TrueBrain
Copy link
Member Author

@TrueBrain TrueBrain commented May 28, 2021

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.

Loading

src/settings.cpp Outdated Show resolved Hide resolved
Loading
@TrueBrain TrueBrain force-pushed the secrets-settings branch from cb42fc3 to b5e32b6 Jun 28, 2021
@TrueBrain
Copy link
Member Author

@TrueBrain TrueBrain commented Jun 28, 2021

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.

Loading

@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 Jun 28, 2021
@TrueBrain TrueBrain force-pushed the secrets-settings branch from b5e32b6 to e43a776 Jun 29, 2021
src/settings.cpp Outdated Show resolved Hide resolved
Loading
src/settings.cpp Outdated Show resolved Hide resolved
Loading
src/settings.cpp Outdated Show resolved Hide resolved
Loading
src/settings.cpp Outdated Show resolved Hide resolved
Loading
src/settings.cpp Outdated Show resolved Hide resolved
Loading
src/settings.cpp Outdated Show resolved Hide resolved
Loading
This to prepare the code to split up network-related settings
into private / secrets / generic.
@nielsmh
Copy link
Contributor

@nielsmh 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.

Loading

@TrueBrain TrueBrain force-pushed the secrets-settings branch from e43a776 to b9f7f6a Jul 1, 2021
@TrueBrain
Copy link
Member Author

@TrueBrain TrueBrain 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.

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.

Loading

src/settings.cpp Outdated Show resolved Hide resolved
Loading
@TrueBrain TrueBrain force-pushed the secrets-settings branch from b9f7f6a to 1121612 Jul 1, 2021
TrueBrain added 3 commits Jul 1, 2021
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.
Clearly someone really wanted to generalize the function, but
in reality it makes it a lot longer than needed. Let's keep it
simple.
…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 force-pushed the secrets-settings branch from 1121612 to 905dd7b Jul 1, 2021
@TrueBrain TrueBrain merged commit 75b6051 into OpenTTD:master Jul 2, 2021
15 checks passed
Loading
@TrueBrain TrueBrain deleted the secrets-settings branch Jul 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants