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

[Security Solution] Add kibana system permissions for Endpoint action indices #81953

Conversation

kevinlog
Copy link
Contributor

@kevinlog kevinlog commented Dec 20, 2021

The PR adds proper permissions for the kibana_system user to two indices specific to Endpoint actions.

The two indices are .logs-endpoint.actions-* and .logs-endpoint.action.responses-* .

The kibana_system user will create, write, and read from the .logs-endpoint-actions-* index to log new actions and visualize them for users.

The kibana_system user will read from .logs-endpoint.action.responses-*` to visualize responses from the Endpoint for the user.

This changes is needed as Endpoint Security currently requires the superuser role. As we move away from superuser and as superuser become less permissive with system indices, we need to ensure kibana_system can properly interact with these indices. We have Kibana PRs which convert our API calls to ES from reading these system indices as asCurrentUser to asInternalUser .

Relevant Kibana PRs:
elastic/kibana#121642
elastic/kibana#121681

Checked out the Kibana PRs and ran it against a locally running ES and verified that the permissions are now properly set and we can display the proper UI:

image

@elasticsearchmachine elasticsearchmachine added v8.1.0 external-contributor Pull request authored by a developer outside the Elasticsearch team labels Dec 20, 2021
@kevinlog kevinlog added v7.17.0 v8.0.0 :Security/Authorization Roles, Privileges, DLS/FLS, RBAC/ABAC Team:Security Meta label for security team auto-backport-and-merge labels Dec 20, 2021
@kevinlog kevinlog marked this pull request as ready for review December 20, 2021 20:49
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-security (Team:Security)

Copy link
Member

@ywangd ywangd 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'd appreciate if you could address the comments before merging. Thanks!

@@ -683,6 +683,13 @@ public static RoleDescriptor kibanaSystemRoleDescriptor(String name) {
// Fleet Server indices. Kibana create this indice before Fleet Server use them.
// Fleet Server indices. Kibana read and write to this indice to manage Elastic Agents
RoleDescriptor.IndicesPrivileges.builder().indices(".fleet*").allowRestrictedIndices(true).privileges("all").build(),
// Endpoint specific action responses. Kibana reads from these to display responses to the user.
RoleDescriptor.IndicesPrivileges.builder().indices(".logs-endpoint.action.responses-*").privileges("read").build(),
Copy link
Member

Choose a reason for hiding this comment

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

I think we should merge this statement with the one from line 682:

RoleDescriptor.IndicesPrivileges.builder().indices(".logs-endpoint.diagnostic.collection-*").privileges("read").build(),

Read through the existing code, I'd prefer to put the merged statement under the block starting at line 709 so that related grants stay together:

RoleDescriptor.IndicesPrivileges.builder()
.indices(
"logs-*",
"synthetics-*",
"traces-*",
"/metrics-.*&~(metrics-endpoint\\.metadata_current_default)/",
".logs-endpoint.action.responses-*",
".logs-endpoint.diagnostic.collection-*",
".logs-endpoint.actions-*"
)
.privileges(UpdateSettingsAction.NAME, PutMappingAction.NAME, RolloverAction.NAME)
.build(),

Copy link
Contributor Author

Choose a reason for hiding this comment

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

statement is moved: f703c23

Comment on lines 689 to 692
RoleDescriptor.IndicesPrivileges.builder()
.indices(".logs-endpoint.actions-*")
.privileges("create_index", "auto_configure", "read", "write")
.build(),
Copy link
Member

Choose a reason for hiding this comment

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

We can remove auto_configure since it is covered by create_index and write.

Similar to the above comment, I'd prefer we relocate this statement along with the above one to under line 709-720.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed auto_configure and moved the statement: f703c23

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updating this, based on feedback further down, I'm using auto_configure as opposed to create_index

@ywangd
Copy link
Member

ywangd commented Dec 20, 2021

Btw, the term "system indices" is mentioned a few times in the PR description. But neither .logs-endpoint.actions-* or .logs-endpoint.action.responses-* is currently a system index. Are there plans from the endpoint or observability side to change it?

@kevinlog
Copy link
Contributor Author

@ywangd

Btw, the term "system indices" is mentioned a few times in the PR description.

This is me getting terms confused. These datastreams are marked as hidden, not system indices. Thanks for verifying.

My understanding is the superuser access is being limited for any . indices, so we are being proactive with this change. Let me know if I'm missing something.

// Endpoint specific actions. Kibana reads and writes to this index to track new actions and display them.
RoleDescriptor.IndicesPrivileges.builder()
.indices(".logs-endpoint.actions-*")
.privileges("create_index", "read", "write")
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we actually need create_index or could we use auto_configure instead and rely on an index template in the Endpoint package?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, auto_configure should work here. privileges-list-indices

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just made the change and tested again

@ywangd
Copy link
Member

ywangd commented Dec 21, 2021

My understanding is the superuser access is being limited for any . indices, so we are being proactive with this change. Let me know if I'm missing something.

The plan is to not allow superuser to "write" to system indices. Hidden indices are not part of the restriction. But it is always better to move away from using superuser since it is still a very powerful (and unnecessary for most use cases) user even with the restriction.

@kevinlog kevinlog merged commit c541150 into elastic:master Dec 21, 2021
ywangd pushed a commit to ywangd/elasticsearch that referenced this pull request Dec 21, 2021
… indices (elastic#81953)

* [Security Solution] Add kibana system permissions for Endpoint action indices
elasticsearchmachine pushed a commit that referenced this pull request Dec 22, 2021
… indices (#81953) (#82015)

* [Security Solution] Add kibana system permissions for Endpoint action indices

Co-authored-by: Kevin Logan <56395104+kevinlog@users.noreply.github.com>
Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>enhancement external-contributor Pull request authored by a developer outside the Elasticsearch team :Security/Authorization Roles, Privileges, DLS/FLS, RBAC/ABAC Team:Security Meta label for security team v7.17.0 v8.0.0-rc1 v8.1.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants