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

Fix fleet route protections #85626

Merged
merged 4 commits into from
Dec 13, 2020
Merged

Conversation

legrego
Copy link
Member

@legrego legrego commented Dec 10, 2020

#85136 Introduced additional route checks for Fleet's endpoints.

These checks did not account for the fact that security might be disabled in Elasticsearch. When that happens, these routes were incorrectly returning a 401 response to the UI, which triggered a logout response.

cc @elastic/kibana-qa

@legrego legrego added v8.0.0 release_note:skip Skip the PR/issue when compiling release notes v7.11.0 labels Dec 10, 2020
@legrego legrego requested a review from a team December 10, 2020 19:21
@botelastic botelastic bot added the Team:Fleet Team label for Observability Data Collection Fleet team label Dec 10, 2020
@elasticmachine
Copy link
Contributor

Pinging @elastic/ingest-management (Team:Ingest Management)

@legrego legrego requested a review from nchaulet December 10, 2020 19:33
@ph
Copy link
Contributor

ph commented Dec 10, 2020

The above linked PR is also in 7.10

@jfsiii
Copy link
Contributor

jfsiii commented Dec 10, 2020

@legrego thanks for the PR! We definitely want to fix the logout regression.

What happens if security is disabled and the user requests one of these routes? Will it succeed? I'd try myself but I'm not near a computer right now.

cc @ph @nchaulet @ruflin @mostlyjason who have greater context

@legrego
Copy link
Member Author

legrego commented Dec 10, 2020

What happens if security is disabled and the user requests one of these routes? Will it succeed? I'd try myself but I'm not near a computer right now.

Before or after this change?

Prior to this change, accessing any page that leveraged a Fleet API would cause the user to be redirected a few times, and eventually end up on the home page. The sample data screen, for example, makes a call to /api/fleet/settings. When this returns a 401, Kibana thinks that the user is not authenticated, and attempts to redirect the logout flow so that the user can reauthenticate. Since security is not enabled, the user will end up on the Kibana home page following the "logout". The result is that any page that uses a Fleet API will be inaccessible to the end user when security is disabled.

After this change, the call will succeed. Since security isn't enabled, there's no need to check for superuser status, since we don't have the concept of users or roles at that point.

@nchaulet
Copy link
Member

nchaulet commented Dec 10, 2020

@legrego Thanks for looking at this I think we do not want Fleet API to succeed if security is not enabled, so we should probably change this to return a 403 instead no?
The superuser role is enforced in the UI so Kibana will not make any request to Fleet API if the user do not have the superuser role.

@legrego
Copy link
Member Author

legrego commented Dec 10, 2020

Thanks for looking at this I think we do not want Fleet API to succeed if security is not enabled, so we should probably change this to return a 403 instead no?

Fleet currently has an optional dependency on the security plugin, so that lead me to believe that Fleet should continue to work if security wasn't enabled. If that's not the case then I'm happy to return a 403 instead, but we should probably update your dependencies to reflect that as well.

The superuser role is enforced in the UI so Kibana will not make any request to Fleet API if the user do not have the superuser role.

That's good to know. If that's the case, then shouldn't the sample data screen not even attempt the call to /api/fleet/settings when security is disabled?

@nchaulet
Copy link
Member

@legrego We do not enable API routes if the security plugin is not here, and we display a message to the user that it should have security enabled and the superuser role, in the fleet plugin.

Looks like we missed a check in the sample data screen

@jfsiii
Copy link
Contributor

jfsiii commented Dec 10, 2020

@legrego I meant after. I was asking for the reason @nchaulet mentioned.

I agree with the 403 failure if it's disabled.

I understand the assumption/confusion about the optional security dependency. There was some confusion internally about when superuser was required (e.g. only agent or all routes) and @ruflin recently clarified that we want all our routes protected.

I'm not sure if that means we should make security required, or if it can remain optional and we reject if it's not available.

@legrego
Copy link
Member Author

legrego commented Dec 10, 2020

We do not enable API routes if the security plugin is not here, and we display a message to the user that it should have security enabled and the superuser role, in the fleet plugin.

Ah gotcha. For what it's worth, enabling Kibana's security plugin isn't enough to enable the stack security features. Elasticsearch's security plugin must also be enabled, and Kibana doesn't know if it's enabled until it's had a chance to connect and potentially authenticate to the cluster.

I meant after. I was asking for the reason @nchaulet mentioned.

I agree with the 403 failure if it's disabled.

I understand the assumption/confusion about the optional security dependency. There was some confusion internally about when superuser was required (e.g. only agent or all routes) and @ruflin recently clarified that we want all our routes protected.

I'm not sure if that means we should make security required, or if it can remain optional and we reject if it's not available.

Thanks for clarifying. I'll go ahead and change the response to 403 instead of allowing the request to continue.

Copy link
Member

@nchaulet nchaulet left a comment

Choose a reason for hiding this comment

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

🚀 LGTM I will create a follow up issue to avoid to send a request from the sample data if the user do not have permissions or if security is not enabled

@legrego
Copy link
Member Author

legrego commented Dec 13, 2020

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Distributable file count

id before after diff
default 47129 47889 +760

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@legrego legrego added v7.10.2 and removed v7.10.0 labels Dec 13, 2020
@legrego legrego merged commit 96bb72f into elastic:master Dec 13, 2020
@legrego legrego deleted the fleet/fix-superuser-check branch December 13, 2020 17:40
legrego added a commit to legrego/kibana that referenced this pull request Dec 13, 2020
legrego added a commit to legrego/kibana that referenced this pull request Dec 13, 2020
legrego added a commit that referenced this pull request Dec 13, 2020
Co-authored-by: Kibana Machine <[email protected]>

Co-authored-by: Kibana Machine <[email protected]>
legrego added a commit that referenced this pull request Dec 13, 2020
Co-authored-by: Kibana Machine <[email protected]>

Co-authored-by: Kibana Machine <[email protected]>
gmmorris added a commit to gmmorris/kibana that referenced this pull request Dec 14, 2020
* master: (116 commits)
  Fix UX E2E tests (elastic#85722)
  Increasing default api key removalDelay to 1h (elastic#85576)
  align cors settings names with elasticsearch (elastic#85738)
  unskip tests and make sure submit is not triggered too quickly (elastic#85567)
  Row trigger 2 (elastic#83167)
  Add session id to audit log (elastic#85451)
  [TSVB] Fields lists do not populate all the times (elastic#85530)
  [Visualize] Removes the external link icon from OSS badges (elastic#85580)
  fixes EQL tests (elastic#85712)
  [APM] enable 'log_level' for Go (elastic#85511)
  ini `1.3.5` -> `1.3.7` (elastic#85707)
  Fix fleet route protections (elastic#85626)
  [Monitoring] Some progress on making alerts better in the UI (elastic#81569)
  [Security Solution] Refactor Timeline Notes to use EuiCommentList (elastic#85256)
  [Security Solution][Detections][Threshold Rules] Threshold rule exceptions (elastic#85103)
  [Security Solution] Alerts details (elastic#83963)
  skip flaky suite (elastic#62060)
  skip flaky suite (elastic#85098)
  skip flaky suite (elastic#84020)
  skip flaky suite (elastic#85671)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release_note:skip Skip the PR/issue when compiling release notes Team:Fleet Team label for Observability Data Collection Fleet team v7.10.2 v7.11.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants