vaultwarden
vaultwarden copied to clipboard
JWT Refresh Token
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
...).
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.
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?
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 ?
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.
A yes when unlocking I don't believe any call is done to the refresh
endpoint, so there should be no issue/change.
Removed the rolling of the device token since it caused issues when the web client called the refresh token endpoint in parallel.
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\"]}}"]}}
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 afn message(): &str
toError
.
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
Cf https://github.com/dani-garcia/vaultwarden/pull/3899#issuecomment-2349876938