Skip to content

Return 405 when receiving POST request on the route path#16750

Merged
lucasmrod merged 4 commits intomainfrom
16182-fail-post-to-root
Feb 14, 2024
Merged

Return 405 when receiving POST request on the route path#16750
lucasmrod merged 4 commits intomainfrom
16182-fail-post-to-root

Conversation

@lucasmrod
Copy link
Copy Markdown
Member

#16182

  • Changes file added for user-visible changes in changes/ or orbit/changes/.
    See Changes files for more information.
  • Added/updated tests
  • Manual QA for all new/changed functionality

@codecov
Copy link
Copy Markdown

codecov Bot commented Feb 12, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (f8f93e5) 65.86% compared to head (426a355) 66.04%.
Report is 15 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #16750      +/-   ##
==========================================
+ Coverage   65.86%   66.04%   +0.17%     
==========================================
  Files        1139     1143       +4     
  Lines      100116   101063     +947     
  Branches     2463     2463              
==========================================
+ Hits        65940    66743     +803     
- Misses      29277    29367      +90     
- Partials     4899     4953      +54     
Flag Coverage Δ
backend 67.12% <100.00%> (+0.18%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Comment thread server/service/frontend.go
sharon-fdm
sharon-fdm previously approved these changes Feb 13, 2024
Copy link
Copy Markdown
Collaborator

@sharon-fdm sharon-fdm left a comment

Choose a reason for hiding this comment

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

This code and approach looks fine if we know that osqury will drop and will not retry to send it.
Approving tentatively, but if we are not sure let's chat about it.

@lucasmrod
Copy link
Copy Markdown
Member Author

This code and approach looks fine if we know that osqury will drop and will not retry to send it. Approving tentatively, but if we are not sure let's chat about it.

If POST / returns a 200 (which is currently the case) then there could be data loss (because a misconfigured osquery thinks it delivered logs successfully). Also it's probably a good idea to return a non-200 so that administrators know there's some misconfiguration going on.

@sharon-fdm
Copy link
Copy Markdown
Collaborator

sharon-fdm commented Feb 13, 2024

@lucasmrod the problem is we don't know how wide this phenomena is.
If we think it's very minor, then NP. let the admins know.

But what if it's wide? In extreme cases we could cause some damage (I assume it's a bit over panic, but want to raise the question.)

TMWYT

@lucasmrod
Copy link
Copy Markdown
Member Author

But what if it's wide? In extreme cases we could cause some damage (I assume it's a bit over panic, but want to raise the question.)

What damage are you thinking?
(If osquery hosts are already sending its result logs to / then there's damage already, this will shed some light on it)

@sharon-fdm
Copy link
Copy Markdown
Collaborator

@lucasmrod my main concern is that those osquery agents will keep sending this msg repeatedly in a way that will inflate network activity.
Again, if we think it's rare then there is no problem.

@lucasmrod
Copy link
Copy Markdown
Member Author

my main concern is that those osquery agents will keep sending this msg repeatedly in a way that will inflate network activity.

Sorry, I don't follow.

@lucasmrod
Copy link
Copy Markdown
Member Author

We met with @sharon-fdm and agreed that we should add a warning on the release notes (in case this misconfiguration is a common issue).

@lucasmrod lucasmrod requested a review from sharon-fdm February 13, 2024 18:56
WARNING:
We found that misconfigured (empty `logger_tls_endpoint`) osquery instances were sending log results (`POST` requests) to the root path and Fleet was incorrectly returning HTTP 200 responses on such root path.
This version will now return HTTP 405 (Method Not Allowed) when receiving `POST` requests on the root path so that this misconfiguration can be detected by administrators.
If you deploy this version of Fleet and there's log traffic on the root path it could cause increased network usage on your infrastructure because osquery will retry sending the logs and these will accumulate (up to a limit configured by logger flags). Thus, before upgrading, make sure there's no osquery traffic (`POST` requests) to Fleet's root path.
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

@ksatter let me know if this warning makes sense.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Makes sense to me.

@lucasmrod lucasmrod merged commit a5a7df4 into main Feb 14, 2024
@lucasmrod lucasmrod deleted the 16182-fail-post-to-root branch February 14, 2024 15:40
roperzh added a commit that referenced this pull request Feb 23, 2024
In #16750 we introduced logic to
prevent POST requests to frontend endpoints.

The redirect for SSO was using `http.StatusTemporaryRedirect` as the
status code, which preserves the original request method (`POST` in this
case).

This changes the method to be `http.StatusSeeOther`, [per MDN][1]:

> This response code is often sent back as a result of PUT or POST. The
> method used to display this redirected page is always GET.

[1]: https://developer.mozilla.org/en-US/docs/Web/HTTP/Status/303
@roperzh roperzh mentioned this pull request Feb 23, 2024
3 tasks
roperzh pushed a commit that referenced this pull request Feb 23, 2024
In #16750 we introduced logic to
prevent POST requests to frontend endpoints.

The redirect for SSO was using `http.StatusTemporaryRedirect` as the
status code, which preserves the original request method (`POST` in this
case).

This changes the method to be `http.StatusSeeOther`, [per MDN][1]:

> This response code is often sent back as a result of PUT or POST. The
> method used to display this redirected page is always GET.

[1]: https://developer.mozilla.org/en-US/docs/Web/HTTP/Status/303

# Checklist for submitter

If some of the following don't apply, delete the relevant line.

<!-- Note that API documentation changes are now addressed by the
product design team. -->

- [x] Changes file added for user-visible changes in `changes/` or
`orbit/changes/`.
See [Changes
files](https://fleetdm.com/docs/contributing/committing-changes#changes-files)
for more information.
- [x] Added/updated tests
- [x] Manual QA for all new/changed functionality
georgekarrv pushed a commit that referenced this pull request Feb 23, 2024
In #16750 we introduced logic to
prevent POST requests to frontend endpoints.

The redirect for SSO was using `http.StatusTemporaryRedirect` as the
status code, which preserves the original request method (`POST` in this
case).

This changes the method to be `http.StatusSeeOther`, [per MDN][1]:

> This response code is often sent back as a result of PUT or POST. The
> method used to display this redirected page is always GET.

[1]: https://developer.mozilla.org/en-US/docs/Web/HTTP/Status/303

# Checklist for submitter

If some of the following don't apply, delete the relevant line.

<!-- Note that API documentation changes are now addressed by the
product design team. -->

- [x] Changes file added for user-visible changes in `changes/` or
`orbit/changes/`.
See [Changes
files](https://fleetdm.com/docs/contributing/committing-changes#changes-files)
for more information.
- [x] Added/updated tests
- [x] Manual QA for all new/changed functionality
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