Skip to content
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

V6 Authentication can expire session before client is updated #16757

Closed
briangann opened this issue Apr 25, 2019 · 9 comments · Fixed by #16838
Closed

V6 Authentication can expire session before client is updated #16757

briangann opened this issue Apr 25, 2019 · 9 comments · Fixed by #16838

Comments

@briangann
Copy link
Contributor

What happened:

Kiosk user session automatically logs out on auto-refresh

What you expected to happen:

Kiosk user stays logged in for max duration (30 days default)

How to reproduce it (as minimally and precisely as possible):

This is an edge case that is hard to reproduce. A client logged in can have its cookie/auth rotated but not receive the new session.

Environment:

  • Grafana version:
    6.1.3

  • Data source type & version:

Any datasource, problem reported with a Zabbix datasource. Test datasource can also do this as long as the dashboard has an auto-refresh.

  • OS Grafana is installed on:

linux/docker

  • User OS & Browser:

linux/chrome

DETAILS:

This edge case occurs when the auth_token is rotated but the client does not receive the updated token.

Looks like it can happen at

rotated, err := authTokenService.TryRotateToken(token, ctx.RemoteAddr(), ctx.Req.UserAgent())

the function TryRotateToken can succeed, updating the database with the new token, but fail to send the new session cookie at line 200

There should be error handling to detect when WriteSessionCookie fails, and to roll back the auth token to previous so it can be refreshed again by the client.

When this happens, the client reconnects with the old token, and is logged out since it is no longer valid.

@torkelo
Copy link
Member

torkelo commented Apr 25, 2019

What error can WriteSessionCookie encounter?

@torkelo
Copy link
Member

torkelo commented Apr 25, 2019

Maybe should add some error handling / logging there, can SetCookie fail?

@marefr
Copy link
Member

marefr commented Apr 25, 2019

http.SetCookie doesn't return any error so "can't fail" that way. @briangann are you meaning that the response still fails to include the written cookie?

@marefr
Copy link
Member

marefr commented Apr 25, 2019

Guess this is a duplicate of #15316?

@marefr
Copy link
Member

marefr commented Apr 25, 2019

Hmm I think that this code could be problematic. The idea is to not allow the proxied response to set cookies but could probably remove Grafana set-cookie here?

proxy.ctx.Resp.Header().Del("Set-Cookie")

@briangann
Copy link
Contributor Author

http.SetCookie doesn't return any error so "can't fail" that way. @briangann are you meaning that the response still fails to include the written cookie?

It looks like the response never reaches the client, and the cookie that stays on the client is no longer valid.

The bug does appear similar to #15316

Looking at the lifecycle of the auth_token made this area looks like a possible failure point.

@marefr marefr added this to the 6.2-beta1 milestone May 1, 2019
marefr added a commit that referenced this issue May 1, 2019
If Grafana rotates the user's auth token during a request to the data 
source proxy it will set the Set-Cookie header with new auth token in 
response before proxying the request to the datasource.
Before this fix the Set-Cookie response header was cleared after the 
proxied request was finished to make sure that proxied datasources 
cannot affect cookies in users browsers. This had the consequence 
of accidentally also clearing the new auth token set in Set-Cookie 
header.
With this fix the original Set-Cookie value in response header is now 
restored after the proxied datasource request is finished. The existing
logic of clearing Set-Cookie response header from proxied request 
have been left intact.

Fixes #16757
@mirronelli
Copy link

mirronelli commented Jun 30, 2019

I still have this problem in 6.2.5. The session cookie gets set to empty value after a few minutes. Thus forcing a logout. I can provide you with browser logs as well as server logs if needed. I don't see any regularity in this. Sometimes the session lasts for more than 10 minutes, sometimes it only lasts as few as 3 minutes. I have already changed the short lived token timeout to 300 minutes but that didn't help.

The first request that fails is: /api/annotations?dashboardId=1&from=1561925014400&to=1561925874916. It is marked as cancelled in the browser dev tools. This one does not yet reset the cookie. The one immediately after this one however does reset it: /api/tsdb/query resp 401. Set-Cookie: grafana_session=; Path=/; Max-Age=0; HttpOnly; SameSite=Lax.

the log has lots of errors like this:
t=2019-06-30T22:29:38+0200 lvl=eror msg="failed to look up user based on cookie" logger=context error="database is locked"
t=2019-06-30T22:29:38+0200 lvl=eror msg="failed to look up user based on cookie" logger=context error="database is locked"

the database is locked error is shown on many different lines. that may be the culprit.

@marefr
Copy link
Member

marefr commented Jul 1, 2019

@mirronelli please refer to #16638 for further discussions.

@r3econ
Copy link

r3econ commented Nov 8, 2021

I have the same problem. When will there be a fix? I'm on v8.2.2

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants