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

RFC: Feature: Public key company authorization #8391

Draft
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

Milek7
Copy link
Contributor

@Milek7 Milek7 commented Dec 16, 2020

This PR serves more as RFC, as there are many things left to consider which could change implementation significantly

Libhydrogen is included in-tree as cryptography library.
As we don't need full-blown complex TLS crypto library, Libsodium is usually good choice. However, it is rather big library, which would add another external dependency, grow code size unnecessarily (especially important eg. with Emscripten), etc. Thus libhydrogen is smaller alternative, which could be included directly in-tree avoiding any extra dependencies. Monocypher was also considered but I decided against it as it requires random bytes to be supplied by application, which would require additional platform-specific code on our side.

How this is implemented now:
For now in this PR only single pubkey is stored for a company. It works as alternative access method to password, so you can join with matching pubkey OR password. As I think storing multiple pubkeys should be ultimately implemented, GUI changes for current solution are rudimentary.

In NetworkStartUp, keypair is read from BASE_DIR/keypair in format of publickey,secretkey (keys are hex-encoded). If file cannot be parsed or keypair is invalid, new keypair is generated and saved to this file.
Always during joining to server, after NewGRF check and possible game password check server sends PACKET_SERVER_NEED_KEYAUTH packet containing challenge with 16 random bytes. Client then replies with PACKET_CLIENT_KEYAUTH containing its own public key and signature for given challenge. Server verifies challenge and stores client pubkey in ClientInfo, or disconnects client when signature is invalid.
Now, if client tries to join company with matching pubkey, access is allowed without any passwords.
Client can protect company with pubkey sending PACKET_CLIENT_PROTECT_COMPANY, containing bool indicating whether to protect or unprotect. As for now only single pubkey is stored, it always stores pubkey of client who sent the packet.
Autoprotecting on company creation can be enabled by network.company_autoprotect setting (enabled by default).
As company is indicated to clients as 'protected' whenever there is any protection on the company, and there is no way to know whether pubkey will succeed, clients now always try to join after clicking on Password button without password. This is harmless, except of short password prompt flicker in case when pubkey auth succeeds.
Protecting/unprotecting company with pubkeys manually is done in password setting GUI through two additional buttons.

Implications and further changes
Next step would be adding ability to store multiple allowed public keys for a company, and add necessary GUI for managing that. Keys could be also now stored in savegames, as there is no risk that user entered sensitive data there. (contrary to passwords)

Now players can be uniquely identified by servers. Whether this is desired, or exactly the opposite is up to debate. While I think this is nice feature, I guess it might raise privacy concerns. Currently in this PR pubkeys are not shared to other clients, but if we allow multiple keys in company it would be useful, as client could store some sort of "friends" list, consisting of pubkey and last known readable name. You could then grant access to your company to a friend, even if he's offline at the moment.
In future, masterserver could be also extended to share list of connected clients pubkeys, so we could have an icon on server list indicating "some friends are playing there".
If pubkeys won't be shared with other clients, then these two features I mentioned won't be possible, so I think sharing keys is preferable even if it makes privacy concerns even bigger, as clients could be also uniquely tracked by other clients. To alleviate that, I propose adding optional 'privacy' mode, which won't store one keypair, but random secret material from which ephemeral pubkeys are generated based on some server characteristics (server address, or something like that, TBD). That would break friends list features, but basic pubkey authorization will still work.

Copy link
Member

@LordAro LordAro left a comment

Choose a reason for hiding this comment

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

Excellent!

TBH, I'd be in favour of removing "passwords" as they are currently entirely and only using this system.

Oh, and since I actually thought to look - libhydrogen (ISC licenced) is GPL compatible - https://www.gnu.org/licenses/license-list.en.html#ISC

src/network/network.cpp Outdated Show resolved Hide resolved
@@ -1749,6 +1749,9 @@ STR_CONFIG_SETTING_REVERSE_AT_SIGNALS_HELPTEXT :Allow trains to

STR_CONFIG_SETTING_QUERY_CAPTION :{WHITE}Change setting value

STR_CONFIG_SETTING_COMPANY_AUTOPROTECT :Automatically protect new companies with pubkey
STR_CONFIG_SETTING_COMPANY_AUTOPROTECT_HELPTEXT :Automatically protect new companies with cryptographic public key, allowing you to enter your companies without password.
Copy link
Member

Choose a reason for hiding this comment

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

We need to ... "soften" the language here for sure :D But that will come when we do a proper review :)

@Milek7 Milek7 force-pushed the pubkey_auth branch 7 times, most recently from cd5c500 to 81e15ce Compare December 17, 2020 00:47
@YJSoft
Copy link

YJSoft commented Dec 17, 2020

Pubkey might used for server auth(like OpenRCT2).
If that is possible, it can be used for server access whitelist.
(in this case, of course, pubkeys should saved like global config, not savefile)

@TrueBrain
Copy link
Member

Now players can be uniquely identified by servers. Whether this is desired, or exactly the opposite is up to debate. While I think this is nice feature, I guess it might raise privacy concerns. Currently in this PR pubkeys are not shared to other clients, but if we allow multiple keys in company it would be useful, as client could store some sort of "friends" list, consisting of pubkey and last known readable name. You could then grant access to your company to a friend, even if he's offline at the moment.
In future, masterserver could be also extended to share list of connected clients pubkeys, so we could have an icon on server list indicating "some friends are playing there".
If pubkeys won't be shared with other clients, then these two features I mentioned won't be possible, so I think sharing keys is preferable even if it makes privacy concerns even bigger, as clients could be also uniquely tracked by other clients. To alleviate that, I propose adding optional 'privacy' mode, which won't store one keypair, but random secret material from which ephemeral pubkeys are generated based on some server characteristics (server address, or something like that, TBD). That would break friends list features, but basic pubkey authorization will still work.

Basically, we need a plan where we want to head with this. This is a bit unusual for OpenTTD, as mostly we just do stuff and see where we end up :P But as we are touching network protocol and the way servers operate, this requires some planning ahead. As many of the things you mention are valid, but are also point-solutions, which currently don't fit in a bigger picture (as that bigger picture is lacking).

Making a plan does not mean we have to stick to it, of course, but it gives a direction to aim and to evaluate extensions to this idea. For now I think this PR should remain as small as possible, with just some basic proof-of-value code to show what this can do. The plan can extend on that, and later PRs can start implementing that plan.

As example, whether we need an "anonymous mode" strongly depends on how we want to use this. Maybe we find out that, because of the plan, a client should always generate an unique public/secret key pair for every unique server it connects to. Or maybe per cluster. But to evaluate this, we need a bigger vision :)

If you feel up for it, please draft up a plan (in a gist or something; and I mean text here, not code). It should loosely define an end-state, how we see the multiplayer eco-system work. This allows server-operators (and players) to contribute to the plan, before we make choices :) Let me know if you feel up for it!

@TrueBrain TrueBrain added candidate: yes This Pull Request is a candidate for being merged size: large This Pull Request is large in size; review will take a while labels Dec 21, 2020
@TrueBrain
Copy link
Member

The above request of mine has been done and resulted in a Discussion page: #8420

@TrueBrain
Copy link
Member

Assuming this PR will represent Phase 1 of the above discussion (and this is not a black&white thing of course), already a few requests for this PR (of course Phase 1 can change based on feedback, but I doubt it will change heavily):

  • Could you put in a single commit the adding of the 3rdparty library? That makes it easier to see what code in OpenTTD itself changed .. lot of scrolling now :D (maybe including the CMake changes .. dunno .. your call)
  • Can you take a look at the warnings it is throwing? Maybe we can upstream fixes, maybe we can fix it locally.
  • I think it would be good if we put crypto-related functions in their own file, so later phases can more easily manage them. network_crypto.cpp or something. I dunno .. not even sure it is a good idea. But it is now mixed in with the rest, which is a bit hard. Maybe we can mix it with the function that will be created for access-list? Dunno .. something to think about at least :)

And of course it might be good to scan if we can make changes already that make other phases easier to implement.

Owh, and in case this wasn't clear yet: I really like what you have done here :D Now we all have to do is bring it home ;)

Cheers!

@Milek7
Copy link
Contributor Author

Milek7 commented Jan 7, 2021

Some commit splitting, pubkey based rcon, pubkey based admin port, keymaterial stored in .cfg, moved KEYAUTH step before game password, keypairs generated during connecting in preparation for privacy features, libhydrogen compiles as .c to avoid warnings.
Quick example admin port implementation there: Milek7/libottdadmin2@4590a89
More work to follow later (ie. identity manager, company ACLs, game ACLs, privacy modes, split crypto into separate file).

@Milek7
Copy link
Contributor Author

Milek7 commented Jan 10, 2021

While there's still some work in this PR, I meanwhile realized that security model achieved by NEED_KEYAUTH/KEYAUTH packets on unencrypted connection is pretty weak. Discussion here: https://gist.github.com/Milek7/8482c4e87947f4748aea5d96b3e89b80

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
candidate: yes This Pull Request is a candidate for being merged size: large This Pull Request is large in size; review will take a while
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants