-
Notifications
You must be signed in to change notification settings - Fork 11.9k
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
Live: Rely on app url for origin check #35983
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Was just thinking would it be simpler to pass the AppURL
as part of the WebsocketConfig
instead? But maybe that's mixing concerns - don't have enough context to fully judge
One of 2 WebsocketConfig is part of Centrifuge lib - it accepts |
@@ -277,6 +290,23 @@ func (g *GrafanaLive) Init() error { | |||
return nil | |||
} | |||
|
|||
func checkOrigin(r *http.Request, appURL *url.URL) bool { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My only concern is when appURL = "localhost:3000"
This usually means that the appURL was not actually configured. In the frontend, we decided to just use the browser URL rather than having an error that everyone needs to fix. Can we do the same here? essentially skip verification if it is not specified?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
localhost:3000
will work since if root_url
not configured then appURL is http://localhost:3000/
.
Approach in this pr won't work if root_url
set to sth like https://mygrafana.example.com
but user tries to open http://localhost:3000/
which is an actual upstream behind https://mygrafana.example.com
.
In theory we can skip check if Origin starts with http://localhost:
- in this case I can only imagine a case where a user can hijack WS connection for it's own account only. But this won't help a lot with cases where Grafana instance is accessed over ip: like 127.0.0.1:3000
or 10.0.0.1:3000
etc.
(cherry picked from commit 5bbf455)
@FZambia this and the backport PR lacks a milestone which is important to keep track of which changes went into which release, Please assign milestone. This should probably have "add to changelog" label as well since a bug fix and it should be included in the release notes/changelog. |
What this PR does / why we need it:
At this moment we follow same origin strategy for WS connections, in current implementation this requires request Host to be properly set. Unfortunately this means that HTTP Host header should be properly passed on load balancer level (keeping port part, for example using
$http_host
Nginx variable). Here we change the check function to look at AppURL (root_url
in configuration) for origin check - thus all requests from browsers using public URL should be authorized by Live.Which issue(s) this PR fixes:
Fixes #34537