-
-
Notifications
You must be signed in to change notification settings - Fork 6
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
Authentication overhaul and replace MongoDB Driver with mongoose. #48
Conversation
Co-authored-by: [email protected]
bug(45): Use secure password and sessions feat(dynamo): Replace the mongodb driver with mongoose.
@VelocityDesign I don't have that much time right now, but it looks mostly good (skimming the code). I don't have the chance right now to see how the refresh token is generated, so do you want to tell me? I'll look at it fully later. |
It follows the same code that you created, just kinda restructured |
Yeah, I can tell. I notice you used a secret but I didn't see a mention of a JTI (JWT ID) anywhere, are you using it? |
The JWT ID is created during the authenticator.signIn() process |
Looks good! I would still use 12 for the salt strength, but if the server can't handle it I guess 10 will have to do. |
Is the JWT ID meant to be sent to the user or the encrypted token? |
In my repo, the Access Token and Refresh Token are both JWTs but the RT is stored in the database. (the AT is too but that doesn't work for you.) The JWT ID and secret are supposed to be in the Refresh Token JWT. |
Oh, I meant to add that. Let me fix that in a bit, as I have a few bugs |
I caved and let the AT be stored in the DB. I also added a feature that invalidates an RT if it is requested with the wrong username (ex: hacker tries to use user A's token with user B's username whilst scanning for the right user. |
Cool. That's not strong defense, did you still implement refresh token rotation with theft detection? Has this also been tested? |
I'm currently running tests on this. Also what do you mean by refresh token rotation? |
"Getting a new refresh token allows us to detect refresh token theft and makes for much better UX. Since the refresh token expires after 100 days, if the user login on the 100th day, the session will be extended another day. We can detect refresh token theft if the attacker and victim are online at the same time. The user has Refresh Token 0 and Access Token 0. We'll call these RT0 and AT0. The attacker steals RT0. The victim or attacker makes a request to the refresh API. If the attacker makes a request first, the attacker gets AT1 and RT1. The the victim uses RT0, which the server knows is the old token. The session is then revoked since token theft has happened. A similar scenario would happen if the victim made a request first." |
I do have the RT set to expire after 100 days. |
Huh? You should be using different tokens per browser... It was in my repo. |
I am... |
If a JWT is valid, the secret is correct, but the JTI is is nowhere to be found in the DB, then refresh token theft has happened. |
Oh, okay! I'll implement that while I'm working on this later (my laptop is dead currently and I'm out at a track meet). |
@VelocityDesign You also have to make sure to change the secret after theft has occurred. Here's an explanation on how this works: Since a JWT is just a bunch of JSON values encoded as Base64 with a signature, you cannot forge data inside the JWT. (You can read the contents of a JWT if you steal it, so your username protection thing is useless) If the JWT is valid but the JWT ID is incorrect, then theft must have occurred. This is because the attacker has a valid JWT that could not have been manipulated. However, it was an JWT with a JTI that doesn't currently exist, indicating theft. The problem with this is that if an attacker steals a token once, they can keep logging the user out by keep sending the refresh token. Since the refresh token expires after 100 days, this is less than ideal. To combat this, the JWT now contains a 100 character random string which is a secret. It is generated on signup and is used in the JWT. Theft now only occurs if the JWT is valid, the JWT is invalid, and the secret is correct. If the secret is incorrect, either the RSA key has been compromised (extremely unlikely) or an attacker is using an old refresh token to logout the user. If the secret is correct, then theft has occurred and the secret is changed to a new 100 character value. |
Another way to do this is have a regular old session token with a secret. So to do false alarms, you only need the secret (100 character, so unlikely to be bruteforced). With the JWT, you either need to have the RSA key or have a JWT and the secret. Either works well enough. Speaking of which, if the access token is stored in the DB, why have it as a JWT? Why not just have it be a regular old session token and get data from the database? (since you're querying to it) |
I'm invoking rule #1 of programming in this case: it works, so I'm not touching it for now. |
I mean, that's the problem. For this to be secure, you need to go all the way. |
This system is secure, isn't it? |
Ah, nevermind. I just saw your comment before that (internet === bad) |
@iamperry294 How do you verify ATs in the case that they aren't stored in the DB? |
Wait, how are you verifying the refresh token JWT then? The same way... |
So access tokens should be JWTs? Your example had them being random base64 lengths. |
I said since you're storing it in the DB. |
Deploying with Cloudflare Pages
|
@iamperry294 check this out. |
What am I supposed to check? |
Never mind, Should I merge? Access tokens expire every fifteen minutes and aren't stored in the DB. |
Theft detection? |
Implemented. |
Secret? |
Ah, I forgot to implement that. One more commit to go! |
Ready to go! I'll merge this into main tommorrow. |
This pull request:
Fixes #45 by replacing SHA512 with bcrypt and recreating the Session system.
Replaces the mongoDB Node.JS driver with mongoose.