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

Redirect loop following login after visiting /grafana, but not for /grafana/ #22227

Closed
consideRatio opened this issue Feb 15, 2020 · 11 comments · Fixed by #22265 or #22285
Closed

Redirect loop following login after visiting /grafana, but not for /grafana/ #22227

consideRatio opened this issue Feb 15, 2020 · 11 comments · Fixed by #22265 or #22285

Comments

@consideRatio
Copy link
Contributor

What happened:
I ended up in a redirect loop. The loop occur between https://example.local/grafana and https://example.local/grafana/login, and occur after an initial visit to /grafana and the initial login to /grafana/login.

What you expected to happen:
To continue from a successful login at /grafana or /grafana/ that renders to the Grafana home screen.

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

  1. Deploy Grafana using the charts/stable Grafana Helm chart.

  2. Configure the Helm chart like this, but with a valid domain name

adminUser: admin
adminPassword: tmp-password

image:
  # This contains a recent fix not yet released from PR #21652 regarding use of sub paths
  repository: grafana/grafana-dev
  tag: master-0e2d874ecf9277dcc17d562e05271917efc8b595

grafana.ini:
  server:
    domain: example.local
    root_url: 'http://%(domain)s/grafana'
    # also tried: 'http://%(domain)s/grafana/'
    serve_from_sub_path: true
    enforce_domain: false
    enable_gzip: true
    router_logging: true
  1. Visit http://example.local/grafana, then login after redirect to /grafana/login and login.

Anything else we need to know?:
My investigation and thoughts:

  • The redirect_to cookie is set to /grafana (%2Fgrafana) or /grafana/ (%2Fgrafana%2F) on a visit to http://example.local/grafana (no trailing /) or http://example.local/grafana/ (with trailing /). This is probably fine as it is.
  • The real issue and where to fix it as I see it, is that for a logged in user, it should not matter if I visit /grafana or /grafana/, both requests should be treated by the Webservers home screen handler for the authenticated user.
  • I used Okta authentication (OpenID Connect / OAuth2) when I logged in, and I had HTTPS instead of HTTP, with TLS termination managed by a nginx-ingress-controller in kubernetes, but I don't think this matters.

Environment:

  • Grafana version: unreleased, pre 6.6.2
  • OS Grafana is installed on: an official docker image of grafana/grafana-dev
  • User OS & Browser: chrome
  • Grafana plugins: none
  • Others:
@torkelo
Copy link
Member

torkelo commented Feb 15, 2020

Can you try latest nightly build?

Exactly what version are you running? Pre 6.6.2 is not a version we have released, looks strange

@consideRatio
Copy link
Contributor Author

consideRatio commented Feb 15, 2020

I'm using this Docker image with grafana installed, it is the image that was built following the merge of #21652, so it is something in between 6.6.1 (released) and 6.6.2 (unreleased).

  repository: grafana/grafana-dev
  tag: master-0e2d874ecf9277dcc17d562e05271917efc8b595

I can try the current master build, grafana/grafana:master on Docker hub. Done! I experience the same issue still.

@papagian papagian added this to the 6.6.2 milestone Feb 17, 2020
@papagian papagian self-assigned this Feb 17, 2020
@consideRatio
Copy link
Contributor Author

consideRatio commented Feb 17, 2020

The issue I experience relates to grafana_session cookies path that is /grafana/.

I recorded a GIF showing that if i modify my cookie to be sent along with web requests of path /grafana instead of /grafana/, I stop experiencing the redirect loop.

grafana-debugging


PR Idea

We avoid a trailing slash on the cookie path. Is that plausible? I'm not used to working with cookies yet, and doesn't fully understand how the path works except that it is some filter to say when to pass along the cookie with the web request.

@consideRatio
Copy link
Contributor Author

consideRatio commented Feb 17, 2020

This is the code that sets the cookie path to /grafana/ instead of just /grafana.

func newCookieOptions() CookieOptions {
return CookieOptions{
Path: setting.AppSubUrl + "/",
Secure: setting.CookieSecure,
SameSiteDisabled: setting.CookieSameSiteDisabled,
SameSiteMode: setting.CookieSameSiteMode,
}
}

It may make sense to append / to the path here, but then it would be good if a visit to /grafana redirects to /grafana/ for anonymous users no matter what instead of forcing a visit to /grafana/login to set a cookie for /grafana/ only.

Conclusion

I'm not sure how to proceed.

Ideas:

  • One could remove the trailing slash to the cookie path, but is this safe, or will this break stuff?
  • One could redirect anonymous users from /sub-path to /sub-path/
  • One could ensure that the redirect_to cookie get a trailing slash, then the worst outcome is a visit to /sub-path leads to /sub-path/login and then a quick bounce to /sub-path/
  • Something else?

@marefr
Copy link
Member

marefr commented Feb 18, 2020

According to https://stackoverflow.com/a/53784228 seems like RFC 6265, April 2011 suggest to include / if there's no other path info, but not include trailing slash / when there are path info. Seems to me it's safe to remove the trailing slash when there's path info like /grafana.

@consideRatio
Copy link
Contributor Author

Hej @marefr =)

It would be fun for me to make my first code contribution to grafana. What do you think about updating the code below to set the cookie path like setting.AppSubUrl or "/" instead of setting.AppSubUrl + "/"?

func newCookieOptions() CookieOptions {
return CookieOptions{
Path: setting.AppSubUrl + "/",
Secure: setting.CookieSecure,
SameSiteDisabled: setting.CookieSameSiteDisabled,
SameSiteMode: setting.CookieSameSiteMode,
}
}

@marefr
Copy link
Member

marefr commented Feb 18, 2020

@consideRatio thanks. @papagian will review your PR

@papagian papagian mentioned this issue Feb 18, 2020
21 tasks
@marefr
Copy link
Member

marefr commented Feb 20, 2020

@consideRatio just noticed you're using both serve_from_sub_path and an nginx-ingress-controller. serve_from_sub_path is quite new feature and was added to help users run grafana with a sub path without using a reverse proxy like Nginx and similar. So you may have stumbled upon an edge case. I may be wrong but I think if you handle the sub path in nginx-ingress-controller and disable the serve_from_sub_path you would have much greater control whether to include trailing slash or not and/or redirect to (non-)trailing slash url. Worth a shot trying?

@consideRatio
Copy link
Contributor Author

consideRatio commented Feb 20, 2020

@marefr I'd love to make a lasting contribution rather than working around the issue ;)


I do use a nginx-ingress-controller, but I'd like to make it remain very transparent in the sense that it isn't given any more responsibility than than encrypt/decrypt traffic and direct traffic to servers based on the Host header such as https://example.org/grafana.

I understand there are tricks to manipulate the request and response in a reverse proxy so it looks to the user its original request was served under a subpath but the serving server was fooled to think it served a request on a root path. Is this kind of trick needed if I disable serve_from_sub_path?

Off topic:
From a configuration design perspective, I wonder if one could deduce the serve_from_sub_path setting from the root_url value.

grafana.ini:
server:
    domain: example.org
    root_url: 'https://%(domain)s/grafana'
    # does serve_from_sub_path=false, ever make sense given the root_url above?
    serve_from_sub_path: true

@papagian papagian modified the milestones: 6.6.2, 6.6.3 Feb 20, 2020
@marefr
Copy link
Member

marefr commented Feb 20, 2020

I'd love to make a lasting contribution rather than working around the issue ;)

I understand that, but given we had a lot of other redirect related issues tried to provide a workaround so that you can have it working before we're able to review/accept your contribution which could take a while.

From a configuration design perspective, I wonder if one could deduce the serve_from_sub_path setting from the root_url value.

Yes, if you use a reverse proxy in front of Grafana. The root url is Grafana's public facing URL, like http://local.domain/grafana. So you can keep Grafana's default config and only change the root url and then configure your reverse proxy to proxy everything under a certain subpath to Grafana (localhost:3000).

@consideRatio
Copy link
Contributor Author

Ah thanks i understand things better now! Grafana will always need the public facing url to help the user find its way back after a redirect to oauth etc, but it may still want to listen on its root path if a reverse proxy did some stuff in between

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