Skip to content

Conversation

pracucci
Copy link
Contributor

What this PR does:
We had some API endpoints which allowed any HTTP method, while they were clearly only GET and/or POST. This is particularly insidious if someone misconfigure Prometheus and remote writes to / because that POST requests will succeed while they shouldn't.

In this PR I'm fixing it.

Which issue(s) this PR fixes:
Fixes #3227

Checklist

  • Tests updated
  • Documentation added
  • CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

Copy link
Contributor

@pstibrany pstibrany left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks.

I would suggest that we enforce specifying method by changing RegisterRoute signature to have one required method and fixing remaining places.

func (a *API) RegisterRoute(path string, handler http.Handler, auth bool, method string, methods ...string) {

I think the same should be done in RegisterRoutesWithPrefix (used by alertmanager only), but that requires bit more research about which methods are required there. It's not currently clear, as AlertManager acts as a mux itself.

Approving so you can decide the best course of action.

@pstibrany
Copy link
Contributor

/cc @jtlisi

@pracucci pracucci force-pushed the fix-api-endpoints-methods branch from 5e5c4b8 to 105b2d0 Compare September 23, 2020 15:09
@pull-request-size pull-request-size bot added size/L and removed size/M labels Sep 23, 2020
@pracucci
Copy link
Contributor Author

I would suggest that we enforce specifying method by changing RegisterRoute signature to have one required method and fixing remaining places.

I've enforced it in RegisterRoute() but I think shouldn't be done with RegisterRoutesWithPrefix() because the latter one registers multiple endpoints. WDYT?

@pracucci pracucci requested a review from jtlisi September 23, 2020 15:10
Copy link
Contributor

@jtlisi jtlisi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

- `GET|POST /ring`
- `GET|POST /store-gateway/ring`
- `GET|POST /compactor/ring`
- `GET|POST /ingester/flush`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps we should make /flush, /shutdown and push POST-only? It would make flush and shutdown links on index page not working from browser, which may be a good thing (no accidental click from browser, or prefetching, or recursive link-following robots).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

push POST-only

It is POST only but the CHANGELOG was wrong. Fixed, thanks!

Perhaps we should make /flush, /shutdown

I agree would be safer. If do so, we should consider:

  1. Change tools/migrate-ingester-statefulsets.sh because calls GET /shutdown, but then we would have to use something different than wget (and curl is not available in Cortex images)
  2. We would have to remove these links from the / HTML page because if you click on that they will fail (so there's no point linking them, the https://cortexmetrics.io/docs/api/ will be enough)

Thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Change tools/migrate-ingester-statefulsets.sh because calls GET /shutdown, but then we would have to use something different than wget (and curl is not available in Cortex images)

Valid point. What about we include little tool in the image to call those endpoints? We could fix another problems with included wget too -- it doesn't like 204. I'd suggest separate PR for all that.

  1. We would have to remove these links from the / HTML page because if you click on that they will fail (so there's no point linking them, the https://cortexmetrics.io/docs/api/ will be enough)

I would keep them in the index page (it's useful to know that given Cortex supports it), but add a note that they are POST-only.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Valid point. What about we include little tool in the image to call those endpoints? We could fix another problems with included wget too -- it doesn't like 204. I'd suggest separate PR for all that.

I've the feeling it's overkilled. Installing curl in our Docker images could be just easier.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

curl also doesn't like 204 and returns non-zero exit code. We could simplify our script if we had this.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 to making them POST only!
+1 to showing them in index page but mention that they only support POST.

Not too familiar with the script though.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would you agree if I do it in a separate PR? We need to find a solution for the wget replacement and I would like to avoid blocking this PR because of that.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would you agree if I do it in a separate PR? We need to find a solution for the wget replacement and I would like to avoid blocking this PR because of that.

Absolutely

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Opened an issue for /flush and /shutdown:
#3243

a.indexPage.AddLink(SectionAdminEndpoints, "/multitenant_alertmanager/status", "Alertmanager Status")
// Ensure this route is registered before the prefixed AM route
a.RegisterRoute("/multitenant_alertmanager/status", am.GetStatusHandler(), false)
a.RegisterRoute("/multitenant_alertmanager/status", am.GetStatusHandler(), false, "GET")
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gotjosh Do you see problem with this?

Signed-off-by: Marco Pracucci <[email protected]>
@pracucci pracucci merged commit e3ca9e3 into cortexproject:master Sep 25, 2020
@pracucci pracucci deleted the fix-api-endpoints-methods branch September 25, 2020 15:38
simonswine pushed a commit to grafana/e2e that referenced this pull request Jan 13, 2022
…tex#3228)

* Enforce HTTP method required by most API endpoints

Signed-off-by: Marco Pracucci <[email protected]>

* Enforced method to api.RegisterRoute()

Signed-off-by: Marco Pracucci <[email protected]>

* Small fixes

Signed-off-by: Marco Pracucci <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Index page should not return 200 on POST requests

4 participants