Add db table for login tokens which allows for invalidation #3818

Nutomic · 2023-08-04T11:30:08Z

Adds a new database table where login tokens are stored. These are validated on every api call. The login endpoint now sets a cookie directly, so clients dont have to handle it explicitly.There is also a new endpoint /api/v3/user/logout which deletes the given token from the db, so that it cant be used for any further api actions. Password resets also invalidate all tokens for that user account.

This replaces #3636 with a simpler solution.

0xAnansi · 2023-08-04T11:38:40Z

Wouldn´t revalidating the token everytime make token authentication completely useless? This could then just be done with a session ID and server side session management right?

Nutomic · 2023-08-04T13:04:53Z

The token is still necessary because we need a cryptographically secure way to determine which user is making the request.

0xAnansi · 2023-08-04T13:51:27Z

The token doesn't do that tho.

The only cryptographical proof of anything is that the server signed the token, and that the token's payload didn't change.

By revalidating the information at every request, the second point becomes redundant, and the first point is negated by the fact that if you trust the token you emit, you can trust the info you locally store on the instance concerning the user's session.

At no point in this you check that the actual user is the one sending you the request, which was supposed to be mitigated by catching refresh token reuse that was highlighted in previous bug reports.

Nutomic · 2023-08-04T13:53:28Z

crates/api/src/local_user/save_settings.rs

+
+  LocalUser::update(&mut context.pool(), local_user_id, &local_user_form).await?;
+
+  Ok(Json(()))


Did some cleanup in this file. It doesnt try to parse postgres error strings anymore, and doesnt generate a new jwt token unnecessarily.

Nutomic · 2023-08-04T13:59:53Z

@0xAnansi Yes it does because the token stores the user id. We dont store any session information on the server, so the user id from jwt token is the only way to find out who is making the current request.

I dont see any way to check that the request is actually coming from the user it claims to. Even with the access token thats impossible, because an attacker can steal the token and use it himself.

Nutomic · 2023-08-04T14:02:52Z

migrations/2023-08-04-100313_login-token/up.sql

@@ -0,0 +1,10 @@
+CREATE TABLE login_token (
+    id serial PRIMARY KEY,
+    token text NOT NULL,


Instead of storing the full token string, it should be enough to store the iat (token creation timestamp). Or put a short random string in jti and store that.

https://datatracker.ietf.org/doc/html/rfc7519#section-4.1

0xAnansi · 2023-08-04T14:56:46Z

Yes it does because the token stores the user id.

Yes that's what I mean.

Since there is 0 use of the token based logic, this logic could be done entirely server side with just user sessions stored on the client side as a random GUID in a cookie, that would be matched in the backend to the actual user session.
This is how it is/was done for the majority of the time user sessions were handled on the internet.

I'm not sure the fundamentals on the token based authentication/authorization, the why, when and where it should be used are understood here.

sunaurus · 2023-08-05T10:56:59Z

Without separate auth/refresh tokens, we are adding an extra DB query to every single API request - it seems like going in the wrong direction in terms of perfromance and optimization.

Sorry, I've been quite busy with IRL stuff for the past few weeks, so my original PR has become a bit stale, but I am planning ot get back to it this week. @Nutomic would you be OK with holding off on this PR for a bit so I can address the discussions on my original PR?

Nutomic · 2023-08-07T12:36:51Z

@0xAnansi I see that makes sense. Do you have a good link where I can read more about session handling?

@sunaurus Okay I will wait. But I really dont think that these minor db reads will affect performance. My PR has the advantage that clients only have to make very minor adjustments. I would rather not have all the extra complexity from your PR with auth and refresh tokens. Its much more complicated and the benefit is questionable, because an attacker can steal the token and immediately perform malicious actions via api.

Nutomic · 2023-09-21T13:34:11Z

Fixed all the conflicts and changed the auth header to Authorization. For auth via cookie it is still named simply auth. Please give it a review.

phiresky · 2023-09-21T13:43:07Z

crates/db_schema/src/source/login_token.rs

+  pub id: i32,
+  pub token: String,
+  pub user_id: LocalUserId,
+}


usually i think you would want to add a few other fields here:

created timestamp

ip address of the request that created the token

user agent of the request that created the token

invalidated timestamp (instead of deleting rows, use invalidated is null to filter validity)

this is all info you can (a) use to display a list of known sessions to the user and (b) so you can invalidate a session in some cases (e.g. geoip country of the session changes, user agent changes too much) (c) invalidate other sessions remotely

I don't think we'd want to store IP addresses for privacy reasons.

@phiresky Good suggestions, I added them. Though Im doubtful about using invalidated timestamp, it means the table will grow forever and I dont see the benefit of keeping years old login info around. Adding another scheduled task for cleanup also doesnt seem worth the trouble.

Its true that storing IPs could cause some problems with privacy legislation. But I think its definitely worth it to fight abuse. We just need to be careful that IPs are not leaked to other users.

instead of IP you could also store derived info ( city+country, ASIN) that's a bit less private and closer to what you'd want to use for session invalidation/display. But that would need an external service for the geolocation which is a pain.

or you can store it truncated (e.g. the /24 for ipv4 and the /52 for ipv6)

Yes geoip seems to complicated and error prone. Truncating would require the ip first. Im going to leave it like this and we can make changes in the future if necessary.

phiresky · 2023-09-22T14:19:46Z

with oauth there probably needs to be a table with very similar structure like this. ideally it will be possible to migrate this table to oauth compatible

Nutomic · 2023-09-22T14:20:07Z

Fixed the api tests, should be ready to merge now. Note that auth in ts has to be set like alpha.setHeaders({ Authorization: "Bearer " + res[0].jwt ?? "" }); which is pretty ugly. Would be much nicer to have an explicit method setAuth which sets the header.

@dessalines Oauth would be entirely separate from this, I dont see how it would conflict. If anything it might reuse some of the same code.

dessalines · 2023-09-27T02:58:34Z

Cool.. Need some conflicts fixed.

Nutomic · 2023-09-27T10:12:54Z

Conflicts resolved.

dessalines

Check this PR I've made to this one, with a few of my suggestions: https://github.com/LemmyNet/lemmy/pull/3991/files

crates/api/src/lib.rs

dessalines · 2023-09-27T15:24:43Z

crates/api/src/local_user/logout.rs

+  _local_user_view: LocalUserView,
+  context: Data<LemmyContext>,
+) -> LemmyResult<HttpResponse> {
+  let jwt = read_auth_token(&req)?.expect("user is logged in");


We should avoid expects in these API, because they panic and don't propagate any errors. I'll turn this into a NotLoggedIn error.

migrations/2023-09-18-141700_login-token/up.sql

* A few suggestions. * Fixing SQL format.

Nutomic · 2023-10-02T11:40:53Z

@phiresky Please review this PR and merge it if you dont have any more comments.

crates/api_common/src/utils.rs

crates/api/src/lib.rs

phiresky · 2023-10-02T12:06:01Z

crates/api/src/local_user/save_settings.rs

+    // if email was changed, check that it is not taken and send verification mail
+    if &previous_email != email {
+      if LocalUser::is_email_taken(&mut context.pool(), email).await? {
+        return Err(LemmyErrorType::EmailAlreadyExists)?;


In general it's not good to tell a user this information because it allows email enumeration attacks (finding out which users have an account in the system which then allows easier brue-forcing of accounts based on public password leak lists). The signup flow also has a leak but that might have more barriers (e.g. captcha, rate limits) so I'm still mentioning it as good practice.

more info on what better responses would be e.g. here https://cloud.google.com/identity-platform/docs/admin/email-enumeration-protection :

Email enumeration protection offers the following features:

Removes error responses for email verification flows. If the email address exists, a verification email is sent. If it does not exist, a verification email is not sent. We recommend that you do not allow users to sign up without an email verification flow.

Disables the ability for users to change their email address without first verifying the new address.

Disables listing of sign-in methods for a specified email address when calling createAuthUri.

Thats true, but we need to return some kind of error in case of duplicate email. We could change it to a generic error message, but that would be extremely confusing for users.

The "clean" way I guess would be to just return "ok" always with a message of "an email has been sent to the new email to verify it) and to send two different emails, one saying "could not change email since another account already exists" and one saying "click here to verify your email".

I guess that might be too much effort for this change though. Email enumeration will happen though when lemmy gets popular enough and cause account takeovers for people that like to reuse passwords.

Makes sense, but that will require updating translation so Id rather do it in another PR. Opened #4021

crates/api_crud/src/user/create.rs

migrations/2023-09-18-141700_login-token/up.sql

phiresky · 2023-10-04T13:35:21Z

crates/utils/src/error.rs

@@ -212,6 +212,7 @@ pub enum LemmyErrorType {
  CouldntSendWebmention,
  ContradictingFilters,
  InstanceBlockAlreadyExists,
+  #[serde(rename = "`jwt` cookie must be marked secure and httponly")]


same as the other PR: i don't think using "rename" is the correct way to add messages to this enum. (see #4007 (comment) )

Changed this to a comment instead.

dessalines · 2023-10-04T21:55:51Z

crates/api_common/src/utils.rs

@@ -38,6 +44,8 @@ use rosetta_i18n::{Language, LanguageId};
 use tracing::warn;
 use url::{ParseError, Url};

+pub static AUTH_COOKIE_NAME: &str = "jwt";


Why change the cookie name back to jwt, just to make it more backward compatible?

To be honest its up to you and @SleeplessOne1917 which way its easier to handle the cookie. So let me know if you prefer to use the same name (jwt, means you need to mark the existing cookie as secure and httponly), or rename it (then you can set a new cookie and dont worry about the old one). Though if you use header for auth it doesnt matter at all.

Doesn't matter to me. Thoughts @SleeplessOne1917 ?

imo the hasJwtCookie and setJwtCookie functions should be removed from lemmy-ui, and with the cookie being named jwt here it shouldn't require any further changes there (I think). lemmy-ui shouldn't need to worry about the cookie at all - lemmy_server now sets it and removes it, and with it being HttpOnly it will also make session exfiltration via XSS impossible

I'm with phiresky on this one.

Actually the breaking changes post says that the cookie is called auth. So I renamed it back to that.

https://lemmy.ml/post/5711722

i guess it doesn't matter, lemmy-ui should still be able to remove their cookie logic regardless of what the new cookie is named

Nutomic · 2023-10-09T09:42:26Z

Can we merge this?

Nutomic mentioned this pull request Aug 4, 2023

Auth overhaul (access tokens, refresh tokens, api tokens) #3636

Closed

Nutomic commented Aug 4, 2023

View reviewed changes

Nutomic added 2 commits September 18, 2023 17:14

wip

8da32fb

stuff

20e38c7

Nutomic force-pushed the login_token_table branch from 805e84d to 20e38c7 Compare September 19, 2023 11:35

Nutomic added 5 commits September 19, 2023 13:37

fmt

71f0a59

fmt 2

8bfc202

fmt 3

ea42469

fix default feature

37c2803

Merge branch 'main' into login_token_table

246ea92

Nutomic marked this pull request as ready for review September 21, 2023 13:00

Nutomic requested review from dessalines and phiresky as code owners September 21, 2023 13:00

Merge branch 'main' into login_token_table

989bae9

Nutomic force-pushed the login_token_table branch from eac67fd to 1d8bef7 Compare September 21, 2023 13:33

use Authorization header

4381bb8

phiresky reviewed Sep 21, 2023

View reviewed changes

Nutomic added 2 commits September 22, 2023 11:27

store ip and user agent for each login

53a1177

Merge branch 'main' into login_token_table

f591ee1

Nutomic force-pushed the login_token_table branch from 1d8bef7 to f591ee1 Compare September 22, 2023 09:28

add list_logins endpoint

053dff9

Nutomic mentioned this pull request Sep 25, 2023

feat: Replace ad hoc auth header with internet standard bearer token auth header #3982

Merged

Merge branch 'main' into login_token_table

aa405fa

dessalines reviewed Sep 27, 2023

View reviewed changes

This was referenced Sep 28, 2023

[Bug]: POST /pictrs/image not possible with CORS #3567

Closed

Invalid format for "sub" claim in JWT #3417

Closed

[Security]: No token/session invalidation after logout/reset password #3364

Closed

A few suggestions for login_token (#3991)

45570a6

* A few suggestions. * Fixing SQL format.

dessalines approved these changes Sep 28, 2023

View reviewed changes

dessalines requested a review from phiresky September 28, 2023 13:59

phiresky reviewed Oct 2, 2023

View reviewed changes

crates/api_common/src/utils.rs Show resolved Hide resolved

phiresky reviewed Oct 2, 2023

View reviewed changes

crates/api/src/lib.rs Outdated Show resolved Hide resolved

phiresky reviewed Oct 2, 2023

View reviewed changes

crates/api_crud/src/user/create.rs Show resolved Hide resolved

phiresky reviewed Oct 2, 2023

View reviewed changes

migrations/2023-09-18-141700_login-token/up.sql Show resolved Hide resolved

review

2ea45be

phiresky reviewed Oct 4, 2023

View reviewed changes

dessalines reviewed Oct 4, 2023

View reviewed changes

Nutomic added 2 commits October 5, 2023 10:45

review

414376f

Merge branch 'main' into login_token_table

3d0317e

dessalines approved these changes Oct 5, 2023

View reviewed changes

Nutomic added 2 commits October 6, 2023 10:35

rename cookie

c60b001

Merge branch 'main' into login_token_table

ed85328

dessalines requested a review from phiresky October 6, 2023 12:58

phiresky merged commit dc32765 into main Oct 9, 2023
2 checks passed


		LocalUser::update(&mut context.pool(), local_user_id, &local_user_form).await?;

		Ok(Json(()))

Add db table for login tokens which allows for invalidation #3818

Add db table for login tokens which allows for invalidation #3818

Conversation

Nutomic commented Aug 4, 2023

0xAnansi commented Aug 4, 2023

Nutomic commented Aug 4, 2023

0xAnansi commented Aug 4, 2023

Choose a reason for hiding this comment

Nutomic commented Aug 4, 2023 • edited Loading

Choose a reason for hiding this comment

0xAnansi commented Aug 4, 2023

sunaurus commented Aug 5, 2023

Nutomic commented Aug 7, 2023

Nutomic commented Sep 21, 2023

phiresky Sep 21, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

phiresky commented Sep 22, 2023

Nutomic commented Sep 22, 2023

dessalines commented Sep 27, 2023

Nutomic commented Sep 27, 2023

dessalines left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Nutomic commented Oct 2, 2023

phiresky Oct 2, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

phiresky Oct 5, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Nutomic commented Oct 9, 2023

Nutomic commented Aug 4, 2023 •

edited

Loading

phiresky Sep 21, 2023 •

edited

Loading

dessalines left a comment •

edited

Loading

phiresky Oct 2, 2023 •

edited

Loading

phiresky Oct 5, 2023 •

edited

Loading