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

add privileges for kIbana_system user to serve cloud security posture… #84941

Merged
merged 2 commits into from
Mar 29, 2022

Conversation

CohenIdo
Copy link
Contributor

Add privileges for kibana_system user to allow support in Cloud Security Posture's feature.

@CohenIdo CohenIdo added :Security/Authorization Roles, Privileges, DLS/FLS, RBAC/ABAC Team:Security Meta label for security team auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) labels Mar 14, 2022
@CohenIdo CohenIdo requested review from joshdover, mark-vieira and a team March 14, 2022 10:04
@elasticmachine
Copy link
Collaborator

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

@elasticsearchmachine elasticsearchmachine added v8.2.0 external-contributor Pull request authored by a developer outside the Elasticsearch team labels Mar 14, 2022
"cloud_security_posture-findings_latest",
"cloud_security_posture-benchmark_scores"
)
.privileges("create_index", "delete_index", "read", "index", "delete" ,"write" ,"all")
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 need delete? I don't see it as a requirement for the transform APIs nor do I expect Kibana to be deleting documents from these indices

Copy link
Contributor Author

@CohenIdo CohenIdo Mar 16, 2022

Choose a reason for hiding this comment

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

in the doc, it mentioned the required privileges are:
source indices: read, view_index_metadata
destination index: read, create_index, index

while when the Transform is initiated with those privileges we are getting the next error message:

task encountered irrecoverable failure: org.elasticsearch.ElasticsearchSecurityException: action [indices:data/write/delete/byquery] is unauthorized for user [kibana_system] with roles [kibana_system] on indices [cloud_security_posture-findings_latest], this action is granted by the index privileges [delete,write,all]

As a solution, I added the following privileges: [delete,write,all]

Copy link
Contributor

Choose a reason for hiding this comment

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

@elastic/ml-data (I hope this is the right team) are our docs out of date here? I'm surprised that installing a transform is attempting a delete by query on the destination index and that the docs don't indicate this. I'm also surprised this issue doesn't affect our other transforms installed by Fleet?

task encountered irrecoverable failure: org.elasticsearch.ElasticsearchSecurityException: action [indices:data/write/delete/byquery] is unauthorized for user [kibana_system] with roles [kibana_system] on indices [cloud_security_posture-findings_latest], this action is granted by the index privileges [delete,write,all]

That error seems to indicate that they need one of these [delete,write,all] privileges, but not all of them. Can we test what the minimum required here is?

Copy link

@Winterflower Winterflower Mar 28, 2022

Choose a reason for hiding this comment

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

Hi @joshdover - I think @elastic/ml-core would help here

Copy link
Member

Choose a reason for hiding this comment

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

@joshdover we need delete by query. This is for the new data retention policies that transforms can create. Our docs our out of date if delete is not included.

Copy link
Member

Choose a reason for hiding this comment

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

I will create an ES issue to capture this inaccuracy and adding a check early on to verify security (like we do for write/read).

Copy link
Member

Choose a reason for hiding this comment

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

#85409 for tracking our missing validation and documentation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you all, I added the delete privilege.

@CohenIdo CohenIdo removed the auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) label Mar 14, 2022
@CohenIdo CohenIdo requested a review from joshdover March 16, 2022 14:05
.privileges("read", "view_index_metadata")
.build(),
RoleDescriptor.IndicesPrivileges.builder()
.indices("cloud_security_posture-findings_latest", "cloud_security_posture-benchmark_scores")
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 still need these?

@ywangd
Copy link
Member

ywangd commented Mar 23, 2022

Ping @elastic/kibana-security for review

@CohenIdo CohenIdo force-pushed the addPrivilegesForKibana_system branch from 23bb91a to 31a3f83 Compare March 24, 2022 16:33
@CohenIdo CohenIdo requested review from kfirpeled and joshdover March 24, 2022 16:35
@CohenIdo CohenIdo force-pushed the addPrivilegesForKibana_system branch from 31a3f83 to 4e327b2 Compare March 24, 2022 16:52
Copy link

@kfirpeled kfirpeled left a comment

Choose a reason for hiding this comment

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

namings lgtm

@jportner jportner self-requested a review March 24, 2022 18:38
Copy link

@jportner jportner left a comment

Choose a reason for hiding this comment

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

Kibana Security Review --

Thanks for tagging us, LGTM for posterity

@CohenIdo CohenIdo force-pushed the addPrivilegesForKibana_system branch from 1525089 to 090c876 Compare March 29, 2022 07:59
Copy link
Contributor

@joshdover joshdover left a comment

Choose a reason for hiding this comment

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

LGTM from a Fleet perspective, still need approval from @elastic/es-security

@sophiec20
Copy link
Contributor

For full disclosure - We are just starting to work on 2 more Security packages (host risk score and beaconing) which contain multiple transforms, and we anticipate more in the near future.

Are we sure that extending the privileges of kibana_system is how we want to achieve this? In this case, we are using kibana_system as a type of service account. This is just about acceptable when transforming internal data (as in the security posture use case) but if the use case requires source data to be a data stream and as the number of transforms grows, then we should consider a longer term solution. Due to the split nature of the elasticsearch and kibana security model there are no easy options here, but I wanted to raise awareness that this is not a one-off request on kibana_system.

Copy link
Contributor

@albertzaharovits albertzaharovits left a comment

Choose a reason for hiding this comment

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

LGTM
My personal preference, not a requirement, is that system generated data go into index names that start with ..

@CohenIdo CohenIdo merged commit c10a277 into elastic:master Mar 29, 2022
@joshdover
Copy link
Contributor

joshdover commented Mar 29, 2022

Are we sure that extending the privileges of kibana_system is how we want to achieve this? In this case, we are using kibana_system as a type of service account. This is just about acceptable when transforming internal data (as in the security posture use case) but if the use case requires source data to be a data stream and as the number of transforms grows, then we should consider a longer term solution. Due to the split nature of the elasticsearch and kibana security model there are no easy options here, but I wanted to raise awareness that this is not a one-off request on kibana_system.

@sophiec20 Agreed, this is not a long-term solution we can keep sustaining. Please see elastic/package-spec#293 for discussion of other options. AFAIK no option has been proposed that wouldn't involve having Elasticsearch implement a package install API. If transforms shipped in packages is going to be a growing feature we need to start having more discussions with Elasticsearch about implementing this feature.

cc @ruflin

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
external-contributor Pull request authored by a developer outside the Elasticsearch team >non-issue :Security/Authorization Roles, Privileges, DLS/FLS, RBAC/ABAC Team:Security Meta label for security team v8.2.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.