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

#71 Skip basic auth for OPTIONS http method #79

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

stokito
Copy link

@stokito stokito commented Jan 28, 2022

The OPTIONS needed for CORS requests

The OPTIONS needed for CORS requests

Signed-off-by: Sergey Ponomarev <stokito@gmail.com>
@roidelapluie
Copy link
Member

While I understand the need, I need to assess the security impact, as said in the issue. Not only for Prometheus but for generic exporters as well.

@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. This fix would then disable basic auth for metrics endpoints if OPTIONS is used as method.

@stokito
Copy link
Author

stokito commented Feb 2, 2022

I recently switched to Go and when I first time saw the HTTP handler API my first thought was "hey, here is not forced to check a method like in Java servlet api". Also all code examples even from go tour didn't handled this :(

@SuperQ
Copy link
Member

SuperQ commented Feb 2, 2022

@stokito I agree, we should be much more careful about request method handling here and in other parts of the http handler(s).

@stokito
Copy link
Author

stokito commented Jun 6, 2022

Hi, if you need my assistance I may try to contribute a little bit and fix parts. I can spent about 4-5 hours, just let me know where to change

web/handler.go Show resolved Hide resolved
@roidelapluie
Copy link
Member

This is not safe. Instead we should try to reply ourselves when there is an OPTIONS query.

It would mean moving CORS config into the toolkit.

@roidelapluie
Copy link
Member

I will try to detail what I have in mind if it's not clear.

@gagbo
Copy link

gagbo commented Aug 16, 2023

Hello,

I will try to detail what I have in mind if it's not clear.

Don't you think that checking for OPTIONS should be the responsibility of the exporter that's being wrapped by the toolkit? The fact that prometheus has its own logic to set CORS headers makes me think so.

When you say that you want to move CORS config in the toolkit, that would mean moving all this code, and the CORS origin regexp config value, into the toolkit? If so, that'd be a breaking change I assume, do you have a timeline for this change, or could some help make such a change come faster and available in prometheus API?

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

Successfully merging this pull request may close these issues.

4 participants