prometheus / common

Go libraries shared across Prometheus components and libraries.
Apache License 2.0
261 stars 320 forks source link

Empty auth-scheme support #349

Open sepich opened 2 years ago

sepich commented 2 years ago

What did you do? I want to scrape target secured via Authorization header, curl example:

curl -H "Authorization: $token" ...

Per docs i should be able to not use Bearer https://prometheus.io/docs/prometheus/latest/configuration/configuration/#scrape_config

authorization:
  # Sets the authentication type of the request.
  [ type: <string> | default: Bearer ]

So i've set:

authorization:
  type: ''

But in tcpdump i still see that Bearer prefix is added before token. Validation code: https://github.com/prometheus/common/blob/f57586dab602b7ae2607c552438a23a551f38773/config/http_config.go#L232

What did you expect to see? I expect to being able to choose any auth-scheme, including nil one.

What did you see instead? Under which circumstances? I see that Bearer auth-scheme is forced, i.e.:

curl -H "Authorization: Bearer $token" ...

instead of:

curl -H "Authorization: $token" ...

Environment

prometheus, version 2.31.1 (branch: HEAD, revision: 411021ada9ab41095923b8d2df9365b632fd40c3)
  build user:       root@9419c9c2d4e0
  build date:       20211105-20:35:02
  go version:       go1.17.3
  platform:         linux/amd64

This could be fixed by verifying that type is explicitly set to empty string before callingTrimSpace. I can send PR if such a solution is fine.

LeviHarrison commented 2 years ago

I agree that we should support an empty auth scheme. Ideally, we can go with the solution you proposed, although I'm not sure if bearer token configurations expect no string to be "Bearer", because if so we would have to come up with something else in order to avoid breaking backward compatibility.

cc @roidelapluie

roidelapluie commented 2 years ago

I do think the opposite. The type is mandatory. Accepting something else would be confusing. Here is the page I used when designing this: https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Authorization

sepich commented 2 years ago

Unfortunately reality is more complicated than expectations. As we know already, there are many systems avoiding log4shell by not updating from v1 ;) Also there are many vendor systems that expect raw token in Authorization header, and nobody here to update them. Standard allows to use only auth-scheme in the header: https://datatracker.ietf.org/doc/html/rfc7235#appendix-C

Authorization = credentials credentials = auth-scheme [ 1SP ( token68 / [ ( "," / auth-param ) ( OWS "," [ OWS auth-param ] ) ] ) ]

And it is possible to workaround this via prometheus config:

    authorization:
      type: SECRET_TOKEN
      credentials: ' '

But in this case token in plain text is displayed in /config web-ui. So is better to fix it via "hiding" authorization.type in web-ui?

roidelapluie commented 2 years ago

the intersection between vendors not following rfc's and supporting prometheus is too small at the moment to consider exceptions like this upstream.

nuved commented 1 year ago

please let's set an empty value for the type right now I have to set a proxy up and add a header there and ask Prometheus for scrapping that endpoint.

roidelapluie commented 1 year ago

For proxies you need to use proxy headers which dont have that limitation.

Le jeu. 5 janv. 2023, 11:31, nuved @.***> a écrit :

please let's set an empty value for the type right now I have to set a proxy up and add a header there and ask Prometheus for scrapping that endpoint.

— Reply to this email directly, view it on GitHub https://github.com/prometheus/common/issues/349#issuecomment-1372041677, or unsubscribe https://github.com/notifications/unsubscribe-auth/AACHHJQHXZWRLTMGTDJ35X3WQ2PHPANCNFSM5KO26M6A . You are receiving this because you were mentioned.Message ID: @.***>

nuved commented 1 year ago

@roidelapluie BTW, we have to set up a proxy and add the header of Authorization there because Prometheus can't add a simple header like this one . Authorization ${TOKEN} and add

timlynch2k commented 1 year ago

@roidelapluie please reconsider. authorization stanza could be full general for Authorization request header value, with basic_auth the convenience alternative.

Relax this validation: https://github.com/prometheus/common/blob/ccc2474a155b447845821ac257550a127d14b30e/config/http_config.go#L346

Already authorization.credentials_file allows a path that could be a mounted Secret. If authorization.type validation is relaxed, this solves a case where the remote requires Basic auth with a secret username and no password.

Almost all bytes on Authorization request header are user controlled. Please allow full control.

shurkus commented 7 months ago

Another case when authorization.type is unnecessary and does not allow data to be collected normally https://grafana.com/docs/oncall/latest/oncall-api-reference/