Add random session tokens

otland / forgottenserver

A free and open-source MMORPG server emulator written in C++

https://otland.net

GNU General Public License v2.0

1.57k stars 1.05k forks source link

Add random session tokens #4661

Closed ranisalt closed 3 months ago

ranisalt commented 4 months ago

Pull Request Prelude

[x] I have followed proper The Forgotten Server code styling.
[x] I have read and understood the contribution guidelines before making this PR.
[x] I am aware that this PR may be closed if the above-mentioned criteria are not fulfilled.

Changes Proposed

Currently we use a fixed session token that is insecure if sniffed, since it contains plaintext username and password, and vulnerable to replay attacks, although extremely unlikely.

Changing this to a random session token, without any account information, is both safer and integrates better with external tools, while also (slightly) improving game login performance, since there's no verification needed - 16 random bytes is enough that it can't be guessed.

This is not ready yet, I can't test it since I'm not on my PC and there are two disconnection messages that need fixing.

Zbizu commented 4 months ago

will this be compatible with both legacy character list and weblogin? (both can be tested with otc)

ranisalt commented 4 months ago

will this be compatible with both legacy character list and weblogin? (both can be tested with otc)

I won't touch the current protocol login (it's time to let go) but yes it should be with no worries

EvilHero90 commented 4 months ago

what's the status on this one?

ranisalt commented 4 months ago

what's the status on this one?

Still missing the login protocol part

EvilHero90 commented 4 months ago

I can understand from a logging pov that you would want to save those session tokens into database. Wouldn't it make more sense tho instead of fetching them from database on each authentication to rather have them stored in a std::unordered_list<ip, token> and just check from there if it's valid? and if it's expired just remove it out of the list. They're not really meant to be persistent data since they expire at some point, that's why I'm making that suggestion.

If you save them for the purpose that if the server would suffer from a crash, that they could just login back again without re authenticating then the same could be achieved by fetching all the valid tokens from database into the list again at server startup.

We could get rid of database transactions from authentication then, which I kinda consider the purpose of a token system.

ranisalt commented 4 months ago

They are persisted to work with external authentication (i.e. separate login server). If we don't persist, then we forbid all login servers (while ours doesn't exist)

EvilHero90 commented 4 months ago

They are persisted to work with external authentication (i.e. separate login server). If we don't persist, then we forbid all login servers (while ours doesn't exist)

makes sense, completly forgot that a web service or any other software keeps track of them aswell anything else you want to change or is it up for review?

ranisalt commented 4 months ago

It's ready but I can't really test since there are no working clients for Linux :shrug: