hummingbird-me / kitsu-tools

:hammer: The tools we use to build Kitsu, the coolest platform for anime and manga
https://kitsu.io
Apache License 2.0
2.09k stars 264 forks source link

Improve Session Management #247

Closed NuckChorris closed 9 years ago

NuckChorris commented 9 years ago

The session management right now is rather poor, though not quite "insecure":

Only one authentication_token exists per user, and it never changes and can't be revoked. Logging out, changing password, other standard procedures to "secure" an account are futile. If the attacker has your authentication_token, you're fucked for all eternity.

I propose the following changes:

We don't need a complex "Issued tokens" page like GitHub has, the password change implicitly secures the account. It'd also be nice in future (once we have an Apps table) to bind the tokens to a given App. We should also finish enabling force_ssl.

I should add that I currently have no idea how to do this in Devise (quite frankly, Devise seems like a disaster of not-really-OOP)

nashby commented 9 years ago

@NuckChorris just wanted to mention that Devise doesn't have token authentication module anymore. Also there's some replacement for that removed module here (didn't use it though)

NuckChorris commented 9 years ago

I have basically no idea what magic is going on but I think we're not using the token auth module that's built in. It looks like it's a custom thing shoved in an initializer (why is this in an initializer!?)

NuckChorris commented 9 years ago

Okay after some research I think what needs to happen is the following:

Optionally: also change new tokens to be of the form id:token to remove the timing attack vector which was presented in that link @nashby gave?

infrequent commented 9 years ago

Could look into 2factor authentication and auto revoke session cookies on logout

NuckChorris commented 9 years ago

2FA and other authentication upgrades would be nice, but more important is upgrades to the token system.

infrequent commented 9 years ago

Quick fix atm would be to just revoke all user cookies, thou depends on how the system is setup also you might piss off some users. (I'm assuming accounts are still so called compromised)

NuckChorris commented 9 years ago

Nah, there's no compromise. Cookies are safe unless they were already stolen through other means (ie 2cool, who we should probably revoke the token for)

This is mostly about mitigation procedures for future.

infrequent commented 9 years ago

Ah gotcha

vikhyat commented 9 years ago

Issue a new auth token per login, stored in their own table and joined with user

We can't do this without making extensive changes to how Discourse authentication works.

Remove the token generation from app/models/user.rb and move it to a new model. The current system is a friggin' joke

I don't see what the "friggin' joke" is.

Make the log-out action revoke the current token

Since we're only going to have one auth_token per user we can give them a "log out everywhere" option on sign out that will log them out of all sessions.

NuckChorris commented 9 years ago

If the idea of a single per-user token that's never changed doesn't terrify you then you shouldn't write authentication code. Even changing it when they click "log out all" or change their password, it's still giving everybody the same token which lasts way too long. Tokens are identifiers of the session, not the person! Making them per-person means phishing is far more tragic and breaks pretty much every sane standard regarding session identifiers.

Every other token system on the Internet issues a new one for each session, except ours. Plain and simple.

We really don't have a robust toolbox for if users get phished, much less if (god forbid) they find a security hole. Per-session would allow us to track the last login IP, the last visit time, the initial visit time, etc. — so if things go south we'd have the tools to identify the real user's session. That's what I'm most worried about: when 2cool got hacked/phished/whatever, we couldn't figure out basically any info or take any action, we were flying blind with the controls broken.

I'd also like to add that the Devise friendly_token has a 2x bias towards a certain four characters, making it slightly easier to predict (it's no longer got an even distribution). Not life-threatening just mildly irksome.

The discourse stuff is certainly a problem though. Can't we have it hit up an endpoint on the main server with the user's HB token, then assign its own token?

NuckChorris commented 9 years ago

Bumping this issue back to the fore because we need to be able to issue multiple revokable tokens per user if we want to implement an OAuth2 provider: otherwise we've totally defeated the purpose for OAuth2 in the first place.

So, new proposal: use JWT (JSON Web Tokens) since there's already libs for that. Revocation can be handled easily by storing the revoked tokens on the server until they expire naturally.

The Discourse problem can be solved by sharing the server secret between the two servers, so there doesn't need to be a connection between the two servers for auth even.

Thoughts @vikhyat?


Side note: doesn't Rails actually have a built-in HMAC-based session store?