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

[Ingest] Enforce security required and superuser permissions #65315

Merged
merged 6 commits into from
May 5, 2020

Conversation

jen-huang
Copy link
Contributor

@jen-huang jen-huang commented May 5, 2020

Resolves #64236
Resolves #64242

Summary

This PR makes it so that accessing Ingest Manager UI requires that 1) security is enabled and 2) user has superuser role privileges. This is done by adding a new route /api/ingest_manager/check-permissions and sending a request to that from the UI.

On the server side, routes will only be registered if security is enabled. Due to additional complexity involved with checking for superuser role for each route, this PR does not cover that.

To test security disabled, set xpack.security.enabled=false in Kibana and ES. You should see this UI:

image

To test superuser role privileges, create a new user that does not have superuser role and log in with that user. You should see this UI:

image

@jen-huang jen-huang added v8.0.0 release_note:skip Skip the PR/issue when compiling release notes v7.8.0 Team:Fleet Team label for Observability Data Collection Fleet team labels May 5, 2020
@jen-huang jen-huang requested a review from a team May 5, 2020 17:50
@jen-huang jen-huang self-assigned this May 5, 2020
@elasticmachine
Copy link
Contributor

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

Comment on lines +177 to +178
<PackageInstallProvider notifications={notifications}>
<FleetStatusProvider>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These two providers were moved here from IngestManagerApp so that they are loaded only after permissions & setup checks are done

Comment on lines +76 to +77
if (permissionsResponse.success) {
const { isInitialized: success } = await core.http.post(setupRouteService.getSetupPath());
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Only send setup if permissions check passed

const permissionsResponse = await sendGetPermissionsCheck();
setIsPermissionsLoading(false);
if (permissionsResponse.data?.success) {
try {
Copy link
Member

Choose a reason for hiding this comment

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

nitpick: maybe put this in a new function for readability, the level of indentation start to be hard to read

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.

Tested locally it worked as expected

@jen-huang
Copy link
Contributor Author

Pushed up a change that hides the global settings link if there is a permission/security issue. As a bonus, this also kicks in if there were any setup issues:

image

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

History

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

@jen-huang jen-huang merged commit ea19374 into elastic:master May 5, 2020
@jen-huang jen-huang deleted the ingest/superuser-permissions branch May 5, 2020 20:44
jen-huang added a commit to jen-huang/kibana that referenced this pull request May 5, 2020
…#65315)

* Add check permissions route and request hook

* Conditionally register routes

* Add security not enabled and permissions missing warning UIs

* Hide global settings when there is an error
# Conflicts:
#	x-pack/plugins/ingest_manager/server/plugin.ts
jen-huang added a commit that referenced this pull request May 6, 2020
…#65368)

* Add check permissions route and request hook

* Conditionally register routes

* Add security not enabled and permissions missing warning UIs

* Hide global settings when there is an error
# Conflicts:
#	x-pack/plugins/ingest_manager/server/plugin.ts

Co-authored-by: Elastic Machine <[email protected]>
@paul-tavares
Copy link
Contributor

@jen-huang the route /api/ingest_manager/check-permissions will be registered even if no security, correct?

Since endpoint has a dependency on Ingest (APIs), we will also need to implement something like this on our side for the views that interact directly with Ingest APIs. We also have one of our API that uses the ingest service to enrich our host metadata with an agent status value, so that might be impacted as well.

cc: @kevinlog , @nnamdifrankie

@nnamdifrankie
Copy link
Contributor

@paul-tavares thank you for the ping. We currently use the security of the data client from the core. Does this change that?

@paul-tavares
Copy link
Contributor

@nnamdifrankie not sure. If that client uses the login user's security profile, then yes it would be impacted (I assume). Although - I think @jen-huang indicated above that its not being enforced at the API route level, but what about at the ingest API service level?

@jen-huang
Copy link
Contributor Author

@jen-huang the route /api/ingest_manager/check-permissions will be registered even if no security, correct?

Yes that's correct.

its not being enforced at the API route level, but what about at the ingest API service level

No enforcement (neither security enabled nor permissions) is done at API service level atm.

@jfsiii
Copy link
Contributor

jfsiii commented May 6, 2020

I added this to the agenda for the next sync so we can discuss it more.

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.8.0 v8.0.0
Projects
None yet
7 participants