-
Notifications
You must be signed in to change notification settings - Fork 1k
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 random session tokens #4661
Conversation
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 |
what's the status on this one? |
Still missing the login protocol part |
I can understand from a logging pov that you would want to save those session tokens into database. 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. |
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 |
It's ready but I can't really test since there are no working clients for Linux 🤷 |
@@ -53,7 +53,7 @@ void ProtocolLogin::getCharacterList(const std::string& accountName, const std:: | |||
} | |||
|
|||
// Generate and add session key | |||
static std::independent_bits_engine<std::default_random_engine, CHAR_BIT, unsigned char> rbe; | |||
static std::independent_bits_engine<std::default_random_engine, CHAR_BIT, unsigned short> rbe; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note: the standard says "the effect is undefined unless the parameter is cv-unqualified and is one of unsigned short
, unsigned int
, unsigned long
, or unsigned long long
."
GCC and Clang accept unsigned char
, but MSVC refuses to compile with an error message.
output->addString(accountName + "\n" + password + "\n" + token + "\n" + std::to_string(ticks)); | ||
output->addString({sessionKey.data(), sessionKey.size()}); | ||
|
||
if (Database& db = Database::getInstance(); !db.executeQuery(fmt::format( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could be async query?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
then there is no guarantee when it'll be saved, could possible lead to players not beeing able to login because the session key was not saved (only case I could think of and probably very unlikely)
Pull Request Prelude
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.