search

elastic / apm-server

https://www.elastic.co/guide/en/apm/guide/current/index.html
Other
1.22k stars 523 forks source link

Authentication on root handler potentially too lenient #8191

Open Mpdreamz opened 2 years ago

Mpdreamz commented 2 years ago

The root handler currently is too lenient in accepting requests

https://github.com/elastic/apm-server/blob/2a88e25a60b4adc6f460b76d844c047dafb7e20f/beater/api/mux.go#L284

This was done to make it easy for health checks to always succeed but at the same time this leniency can be trappy.

It will accept badly formatted authentication headers and return 200.

curl -H 'Authorization: ApiKy badformat' -i https:/***.apm.us-**.gcp.cloud.es.io

It will accept expired authentication keys and return 200.

curl -H 'Authorization: ApiKey <expired>' -i https:/***.apm.us-**.gcp.cloud.es.io

While this makes health checks easier as it lessens the requirement to provide authentication it will also return erroneous results to health checks that are explicitly set up to include authentication.

axw commented 2 years ago

Agreed, I think we should return 401 if an invalid auth token is provided.

Mpdreamz commented 2 years ago

Should we also return a 401 if no authentication is provided and APM is explicitly configured to not accept anonymous requests?

axw commented 2 years ago

No, I don't think so. Anonymous auth is only auto-enabled when RUM is enabled. We shouldn't break health checks just because RUM is disabled.

Mpdreamz commented 2 years ago

I'm splitting hairs but

Anonymous auth is only auto-enabled when RUM is enabled.

... and anonymous auth is not explicitly set correct?

What if anonymous auth is explicitly set to false (with or without RUM being enabled).

Separate issue but today in the integration configuration the default anonymous setting being dependent on rum is not reflected e.g flipping it on:

image

Doesn't enable anonymous access.

image

Which presumably means its unset and thus the server will actually default to it being enabled.

Should we look in to the integration never leaving this setting unset but always explicitly providing a true|false value for anonymous access?

axw commented 2 years ago

... and anonymous auth is not explicitly set correct?

What if anonymous auth is explicitly set to false (with or without RUM being enabled).

True. Still doesn't seem very useful to reject unauthenticated health check requests.

Separate issue but today in the integration configuration the default anonymous setting being dependent on rum is not reflected e.g flipping it on: ... Should we look in to the integration never leaving this setting unset but always explicitly providing a true|false value for anonymous access?

My earlier comment about implicitly enabling anonymous access is actually only relevant to standalone/legacy. Fleet will always render this option in the policy: https://github.com/elastic/apm-server/blob/c00efb988b99c9cc455ac764d5811d0975860e3f/apmpackage/apm/agent/input/template.yml.hbs#L12. If anonymous access is disabled, apm-server.auth.anonymous.enabled: false will be set in the policy.