vaultwarden icon indicating copy to clipboard operation
vaultwarden copied to clipboard

JWT Refresh Token

Open Timshel opened this issue 1 year ago • 6 comments

To facilitate review decided to move out the switch to a JWT refresh_token from the sso PR https://github.com/dani-garcia/vaultwarden/pull/3899.

Without the SSO logic it's not the most useful still :

  • Add an expiration on the refresh_token (work like an idle timer reset when a new access_token is generated).
  • Store the information of the AuthMethod in the token (Password ...).

Timshel avatar Feb 20 '24 22:02 Timshel

When discussed with @BlackDex we thought that 7 days might be an appropriate value for the refresh_token expiration.

Had some feedback on it, and it might be a bit short, since many actions in the application will not trigger a refresh (reading password is done as "offline"`).

Might make sense to raise it to something around 30 days as mentioned in this documentation.

Timshel avatar Apr 10 '24 08:04 Timshel

I agree, lets keep it the same as Bitwarden, and 7 days is a bit short indeed. Also, how is this linked to the refresh roll PR? Could that potentially break offline logins too?

BlackDex avatar Apr 10 '24 09:04 BlackDex

Also, how is this linked to the refresh roll PR?

This is a superset of the refresh roll PR, the same logic of changing the device token is used to prevent reusing twice the same refresh_token.

Could that potentially break offline logins too?

I'm unsure what are the offline logins ?

Timshel avatar Apr 10 '24 09:04 Timshel

Also, how is this linked to the refresh roll PR?

This is a superset of the refresh roll PR, the same logic of changing the device token is used to prevent reusing twice the same refresh_token.

Could that potentially break offline logins too?

I'm unsure what are the offline logins ?

I mean offline unlock in some way. But i doubt it.

BlackDex avatar Apr 10 '24 09:04 BlackDex

A yes when unlocking I don't believe any call is done to the refresh endpoint, so there should be no issue/change.

Timshel avatar Apr 10 '24 09:04 Timshel

Removed the rolling of the device token since it caused issues when the web client called the refresh token endpoint in parallel.

Timshel avatar Apr 15 '24 14:04 Timshel

Maybe something can be done about the logging? I think it's a little bit too verbose. (this happened when I was trying to log in after starting Vaultwarden with this PR applied)

vaultwarden  | [2024-09-01 23:46:02.959][vaultwarden::auth][ERROR] Token is invalid
vaultwarden  | [2024-09-01 23:46:02.962][vaultwarden::auth][ERROR] Impossible to read refresh_token: {"error":"","errorModel":{"message":"Token is invalid","object":"error"},"error_description":"","exceptionMessage":null,"exceptionStackTrace":null,"innerExceptionMessage":null,"message":"Token is invalid","object":"error","validationErrors":{"":["Token is invalid"]}}
vaultwarden  | [2024-09-01 23:46:02.962][vaultwarden::api::identity][ERROR] {"error":"","errorModel":{"message":"Impossible to read refresh_token: {\"error\":\"\",\"errorModel\":{\"message\":\"Token is invalid\",\"object\":\"error\"},\"error_description\":\"\",\"exceptionMessage\":null,\"exceptionStackTrace\":null,\"innerExceptionMessage\":null,\"message\":\"Token is invalid\",\"object\":\"error\",\"validationErrors\":{\"\":[\"Token is invalid\"]}}","object":"error"},"error_description":"","exceptionMessage":null,"exceptionStackTrace":null,"innerExceptionMessage":null,"message":"Impossible to read refresh_token: {\"error\":\"\",\"errorModel\":{\"message\":\"Token is invalid\",\"object\":\"error\"},\"error_description\":\"\",\"exceptionMessage\":null,\"exceptionStackTrace\":null,\"innerExceptionMessage\":null,\"message\":\"Token is invalid\",\"object\":\"error\",\"validationErrors\":{\"\":[\"Token is invalid\"]}}","object":"error","validationErrors":{"":["Impossible to read refresh_token: {\"error\":\"\",\"errorModel\":{\"message\":\"Token is invalid\",\"object\":\"error\"},\"error_description\":\"\",\"exceptionMessage\":null,\"exceptionStackTrace\":null,\"innerExceptionMessage\":null,\"message\":\"Token is invalid\",\"object\":\"error\",\"validationErrors\":{\"\":[\"Token is invalid\"]}}"]}}

dfunkt avatar Sep 02 '24 08:09 dfunkt

Made the change differently with the following logic:

  • I believe the root error should be logged so no change here.
  • I don't think it makes sense to log again an error when just rewrapping it so switched to err_silent.
  • When rewrapping an error I want to be able to keep the original error message but the current to_string is way too verbose, so added a fn message(): &str to Error.

With those change we end up with the following logs:

> curl 'http://127.0.0.1:8000/identity/connect/token' -X POST --data-raw 'grant_type=refresh_token&refresh_token=toto'

[2024-09-03 14:25:59.160][request][INFO] POST /identity/connect/token
[2024-09-03 14:25:59.161][vaultwarden::auth][ERROR] Token is invalid
[2024-09-03 14:25:59.161][vaultwarden::api::identity][ERROR] Unable to refresh login credentials: Impossible to read refresh_token: Token is invalid
[2024-09-03 14:25:59.161][response][INFO] (login) POST /identity/connect/token => 401 Unauthorized

Timshel avatar Sep 03 '24 12:09 Timshel

Cf https://github.com/dani-garcia/vaultwarden/pull/3899#issuecomment-2349876938

Timshel avatar Sep 13 '24 18:09 Timshel