axieum / authme

:lock: Authenticate yourself in Minecraft and re-validate your session
https://www.curseforge.com/minecraft/mc-mods/auth-me
MIT License
85 stars 32 forks source link

Save login session and add auto login on game launch #98

Closed Enjoys-1 closed 2 weeks ago

Enjoys-1 commented 1 year ago

After logging in, the newly generated session is saved and can be used again to automatically log in when starting the game. Both of these features are disabled by default, but can be enabled in the configuration. Great for people who frequently restart the game and need to log in every time.

axieum commented 1 year ago

Welcome back @Enjoys-1 😂

Initial code review looks very well done. I'll need to check over the GUI changes once I get around to checking out the branch.

The main concern I have though, is that we need to think about how we store this access token (you can get up to speed by reading #4).

We should really try to avoid saving the plaintext access token to disk. Initial thoughts is that a malicious mod can just as easily write mixins, etc. to grab the tokens from memory - nothing we can really do about that. But, once they exit memory and are saved to disk, that's where I'm mainly concerned. If we can easily introduce encryption, let's do it.

In both scenarios, encrypted or plaintext, we should add a big disclaimer to that option stating that the user is enabling it at their own risk.

Thanks,

Enjoys-1 commented 1 year ago

I added a warning screen that will be shown when trying to login and having session saving enabled.

The access token is now encrypted and the key is randomly generated on every login. Note: No amount of encryption will prevent malware from decrypting and stealing access tokens. If this mod can decrypt it, any software can. Especially when the encryption algorithm is publicly visible. We can only make it difficult for someone to socialy engineer the user to share their credentials. I was thinking about saving the key in a separate file or using CryptUnprotectData/CryptProtectData (iirc it's Windows only) but I'd like to hear your thoughts on that, since you would like to keep the mod simple and small.

axieum commented 1 year ago

Agreed. This should at least make it harder to scan the disk for plaintext tokens - unless they are specifically targeting this mod.

I think the warnings are sufficient enough, and I'd only expect advanced users to use this new feature.

I'll check out the branch over the weekend and inspect the UI changes.

Thanks again,

Enjoys-1 commented 2 weeks ago

I updated my fork of the mod to 1.21.1, but it looks like I also accidentally closed this pull request. It wouldn't probably get merged anyway, but if someone wants it, you can build my fork.