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

CORS requests are failed if Basic Auth is enabled #71

Open
stokito opened this issue Oct 27, 2021 · 9 comments
Open

CORS requests are failed if Basic Auth is enabled #71

stokito opened this issue Oct 27, 2021 · 9 comments
Labels
enhancement New feature or request

Comments

@stokito
Copy link

stokito commented Oct 27, 2021

I enabled CORS headers and basic auth but a browser still can't fetch information from Prometheus.
Before sending a GET request an OPTIONS request is performed first and it doesn't contains the Authorization header.
So Prometheus must allow the OPTIONS request without the Authorization header.
I can do that with some Nginx configuration but I don't want to use just for this and also I want to keep configuration clear.

@roidelapluie roidelapluie transferred this issue from prometheus/prometheus Nov 5, 2021
@roidelapluie
Copy link
Member

You are correct, we should allow OPTIONS without basic auth, I will move this to the exporter-toolkit repository. We probably do not want this by default, but as an option in web.yml.

@roidelapluie
Copy link
Member

I guess we will also need to understand how Prometheus itself handles OPTIONS requests (without basic auth).

@roidelapluie roidelapluie added the enhancement New feature or request label Nov 5, 2021
stokito added a commit to stokito/exporter-toolkit that referenced this issue Jan 28, 2022
The OPTIONS needed for CORS requests
@stokito
Copy link
Author

stokito commented Jan 28, 2022

I created a PR. Honestly I don't see benefits of having this configurable. In theory, that may lead to problem when someone tested an API endpoint with the OPTIONS covered by a basic auth and then they may be wondered that someone disabled basic auth and now sensitive data is exposed.
Th OPTIONS was created for as a replacement for Flash crossdomain.xml but originally the method was introduced in WebDAV. For a security reason it must not be a resource specific but rather show features of the server itself.
As a possible risk that I see it may be in some API endpoints be a check if r.Method == GET {} else { POST } i.e. that it always assume that there is only two possible http methods and the 405 error is not returned properly.
There is no that many api endpoints to check

stokito added a commit to stokito/exporter-toolkit that referenced this issue Jan 28, 2022
The OPTIONS needed for CORS requests

Signed-off-by: Sergey Ponomarev <[email protected]>
@roidelapluie
Copy link
Member

curl -s -X OPTIONS https://node.demo.do.prometheus.io/metrics |head 
# HELP go_gc_duration_seconds A summary of the pause duration of garbage collection cycles.
# TYPE go_gc_duration_seconds summary
go_gc_duration_seconds{quantile="0"} 4.141e-05
go_gc_duration_seconds{quantile="0.25"} 5.7685e-05
go_gc_duration_seconds{quantile="0.5"} 6.1472e-05
go_gc_duration_seconds{quantile="0.75"} 6.9851e-05
go_gc_duration_seconds{quantile="1"} 0.000358368
go_gc_duration_seconds_sum 297.390302862
go_gc_duration_seconds_count 4.113281e+06
# HELP go_goroutines Number of goroutines that currently exist.
curl -s -X WHATEVER https://node.demo.do.prometheus.io/metrics |head 
# HELP go_gc_duration_seconds A summary of the pause duration of garbage collection cycles.
# TYPE go_gc_duration_seconds summary
go_gc_duration_seconds{quantile="0"} 4.141e-05
go_gc_duration_seconds{quantile="0.25"} 5.7685e-05
go_gc_duration_seconds{quantile="0.5"} 6.1472e-05
go_gc_duration_seconds{quantile="0.75"} 6.9851e-05
go_gc_duration_seconds{quantile="1"} 0.000358368
go_gc_duration_seconds_sum 297.390426157
go_gc_duration_seconds_count 4.113283e+06
# HELP go_goroutines Number of goroutines that currently exist.

It looks like the Prometheus golang API does not check the method. Enabling this would then disable basic auth for metrics endpoints if OPTIONS is used as method.

How could we move this forward? What would be an alternative?

@stokito
Copy link
Author

stokito commented Feb 2, 2022

The only valid way is to fix api and check for api method

@SuperQ
Copy link
Member

SuperQ commented Feb 2, 2022

Yes, we should probably fix client_golang promhttp to allow enforcing the HTTP method.

@stokito
Copy link
Author

stokito commented Jul 2, 2022

Can we do something to force the HTTP method checking?

@stokito
Copy link
Author

stokito commented Dec 24, 2022

Hi again, could you please add the task to a some roadmap. Or maybe we should just close it

@stokito
Copy link
Author

stokito commented Feb 27, 2023

There is also another related problem that needs to be fixed in scope of the task.
Since I using a basic authorization I need to ether pass the Authorization header myself or add
withCredentials option and then my browser will ask me for a password.
The raw Authorization works but withCredentials requires and additional security and the Access-Control-Allow-Origin: * won't work and it should be specified explicit to origin e.g. Access-Control-Allow-Origin: example.com.
See https://developer.mozilla.org/en-US/docs/Web/HTTP/CORS#requests_with_credentials

But Prometheus doesn't do that because the --web.cors.origin has default value .*.
The regexp has a special meaning and will just return wildcard.
https://github.com/prometheus/prometheus/blob/8f3b2fc9aeaf1bd3f839e820e7f9ffc571006e89/util/httputil/cors.go#L30

I've set the regexp to https.* and only then it started to return an Origin instead of wildcard.
I'm not sure if this is a sane default or maybe it would be totally fine to set the Origin by default instead of wildcard. But this was not trivial for me to understand and configure.
Anyway, maybe some tutorials will explain this.

Another problem is that the withCredentials mode also requires the Access-Control-Allow-Credentials: true to be set.
So just for one this header I need to keep the Nginx proxy behind.
It would be great to set the header by default. It basically should be safe. Or maybe to add an option to web config http_server_config.headers

Here is a workaround Nginx conf that I made
https://gist.github.com/stokito/b1e7189ed85d970f20e3e5dd4bfad998

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants