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

[Cloud Security][CDR] Update Wiz vuln privileges #112192

Merged
merged 14 commits into from
Sep 3, 2024

Conversation

CohenIdo
Copy link
Contributor

@CohenIdo CohenIdo commented Aug 25, 2024

Solves:

Summary

Update kibana_system privileges:

  • Include the privilege to create the latest Transform index.
  • Include the privilege to read from the Transform's source index.

Integration PR:

Please note: We considered using the logs prefix for the destination index, which is already mentioned in the documentation as an index that users should avoid writing to due to pattern collisions. The reasons we decided not to use the logs index are summarized here.

@CohenIdo CohenIdo requested a review from a team as a code owner August 25, 2024 13:20
@elasticsearchmachine elasticsearchmachine added v8.16.0 external-contributor Pull request authored by a developer outside the Elasticsearch team needs:triage Requires assignment of a team area label labels Aug 25, 2024
@CohenIdo CohenIdo added the Team:Cloud Security Meta label for Cloud Security team label Aug 25, 2024
@elasticsearchmachine elasticsearchmachine removed the needs:triage Requires assignment of a team area label label Aug 25, 2024
@CohenIdo CohenIdo added >non-issue needs:triage Requires assignment of a team area label labels Aug 25, 2024
@elasticsearchmachine elasticsearchmachine removed the needs:triage Requires assignment of a team area label label Aug 25, 2024
@CohenIdo CohenIdo added the :ml/Transform Transform label Aug 25, 2024
@CohenIdo CohenIdo marked this pull request as draft August 25, 2024 13:29
@CohenIdo CohenIdo marked this pull request as ready for review August 27, 2024 07:26
@elasticsearchmachine elasticsearchmachine added the Team:ML Meta label for the ML team label Aug 27, 2024
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/ml-core (Team:ML)

@CohenIdo CohenIdo changed the title [Cloud Security][CDR] update Wiz vuln privileges [Cloud Security][CDR] Update Wiz vuln privileges Aug 27, 2024
@@ -30,7 +30,8 @@
import static org.elasticsearch.xpack.core.security.authz.store.ReservedRolesStore.getRemoteIndicesReadPrivileges;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Linting has resulted in changes to the comments:
./gradlew :x-pack:plugin:core:spotlessApply

@davidkyle davidkyle added the :Security/Authorization Roles, Privileges, DLS/FLS, RBAC/ABAC label Aug 28, 2024
@elasticsearchmachine elasticsearchmachine added the Team:Security Meta label for security team label Aug 28, 2024
@elasticsearchmachine
Copy link
Collaborator

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

@CohenIdo CohenIdo removed the external-contributor Pull request authored by a developer outside the Elasticsearch team label Aug 28, 2024
@davidkyle davidkyle removed the :ml/Transform Transform label Aug 28, 2024
@elasticsearchmachine elasticsearchmachine removed the Team:ML Meta label for the ML team label Aug 28, 2024
Comment on lines +418 to +421
RoleDescriptor.IndicesPrivileges.builder()
.indices("logs-wiz.vulnerability-*")
.privileges("read", "view_index_metadata")
.build(),

Choose a reason for hiding this comment

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

Looks good - since logs-* is a known collision pattern https://www.elastic.co/guide/en/elasticsearch/reference/current/index-templates.html

Comment on lines 422 to 431
RoleDescriptor.IndicesPrivileges.builder()
.indices("security_solution-wiz.vulnerability_latest-*")
.privileges(
"create_index",
"read",
"index",
"delete",
TransportIndicesAliasesAction.NAME,
TransportUpdateSettingsAction.TYPE.name()
)
Copy link

@SiddharthMantri SiddharthMantri Aug 28, 2024

Choose a reason for hiding this comment

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

For indices not owned by Kibana, the kibana_system user should only have read privileges. security_solution-* falls under that pattern.

Can you please provide any additional information on why the elevated privileges are needed? I can then raise it with the KB security team.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The Transform we're adding in the corresponding PR reads from the logs-wiz.vulnerability-default index and writes the result to the security_solution-wiz.vulnerability_latest-v1 index. This index is created in Kibana after the integration installation, which is why these privileges are necessary.

We've implemented the same logic for our native integration for the same reason—creating an index via the Transform, as shown here.

@slobodanadamovic
Copy link
Contributor

slobodanadamovic commented Aug 28, 2024

@CohenIdo Could you please add corresponding test for security_solution-wiz.vulnerability_latest-* and logs-wiz.vulnerability-*" in ReservedRolesStoreTests#testKibanaSystemRole method?

Arrays.asList(
"logs-cloud_security_posture.findings_latest-default",
"logs-cloud_security_posture.scores-default",
"logs-cloud_security_posture.vulnerabilities_latest-default",
"logs-cloud_security_posture.findings_latest-default-" + Version.CURRENT,
"logs-cloud_security_posture.scores-default-" + Version.CURRENT,
"logs-cloud_security_posture.vulnerabilities_latest-default" + Version.CURRENT
"logs-cloud_security_posture.vulnerabilities_latest-default" + Version.CURRENT,
"security_solution-wiz.vulnerability_latest-" + Version.CURRENT
Copy link
Contributor Author

@CohenIdo CohenIdo Aug 28, 2024

Choose a reason for hiding this comment

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

This new index I added, security_solution-wiz.vulnerability_latest, is used by the cloud security solution, this is the reason it was added under the same test case.

@CohenIdo
Copy link
Contributor Author

@CohenIdo Could you please add corresponding test for security_solution-wiz.vulnerability_latest-* and logs-wiz.vulnerability-*" in ReservedRolesStoreTests#testKibanaSystemRole method?

Done:)

Copy link
Contributor

@slobodanadamovic slobodanadamovic left a comment

Choose a reason for hiding this comment

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

Looks good from es-security side 👍

@SiddharthMantri
Copy link

Hey @CohenIdo

After having discussed this with the KB team, we've got a few questions -

  • Given that the result needs to be written to security_solution...., can this be granted just index and create_index? If it does require the other two privileges, can you clarify why and add that to the PR description?
  • Do we have any user facing docs that specify that users should not be writing to this index due to pattern collisions like we have here: https://www.elastic.co/guide/en/elasticsearch/reference/current/index-templates.html?

We try to be quite restrictive for kibana_system privileges to prevent any exploits should credentials or configs be compromised.

@CohenIdo
Copy link
Contributor Author

CohenIdo commented Sep 2, 2024

Given that the result needs to be written to security_solution...., can this be granted just index and create_index? If it does require the other two privileges, can you clarify why and add that to the PR description?

It's a good point, I will test that with those privilege, I guess you right and we can use just index and create_index

Do we have any user facing docs that specify that users should not be writing to this index due to pattern collisions like we have here: https://www.elastic.co/guide/en/elasticsearch/reference/current/index-templates.html?

Our PMs probably will be able to help here @smriti0321 @tinnytintin10

@CohenIdo
Copy link
Contributor Author

CohenIdo commented Sep 2, 2024

Given that the result needs to be written to security_solution...., can this be granted just index and create_index? If it does require the other two privileges, can you clarify why and add that to the PR description?

@SiddharthMantri, from the transform docs:

Elasticsearch API user
To manage transforms, you must meet all of the following requirements:
- create_index, index, manage, and read index privileges on destination indices. If a retention_policy is configured, delete index privilege is also required on the destination index.

@smriti0321
Copy link
Contributor

Thanks for the catch on doc update @SiddharthMantri and @CohenIdo , I will work on this along with our documentation team. @benironside do you know who will be able to help with the updates on this page- https://www.elastic.co/guide/en/elasticsearch/reference/current/index-templates.html?

Also, we will introduce the change closer to GA of this feature. @CohenIdo

@CohenIdo
Copy link
Contributor Author

CohenIdo commented Sep 2, 2024

./ci

@SiddharthMantri
Copy link

Hey @CohenIdo ! So after consulting with the team, I think we are good now. It's not super apparent from the PR, but we'd like to know why the source and destination index can't be the same one? Something like this:

// For destination indices of the Threat Intel (ti_*) packages that ships a
// transform for supporting IOC expiration
RoleDescriptor.IndicesPrivileges.builder()
.indices("logs-ti_*_latest.*")
.privileges(
"create_index",
"delete_index",
"read",
"index",
"delete",
"manage",
TransportIndicesAliasesAction.NAME,
TransportUpdateSettingsAction.TYPE.name()
)
.build(),
// For source indices of the Threat Intel (ti_*) packages that ships a transform
// for supporting IOC expiration
RoleDescriptor.IndicesPrivileges.builder()
.indices("logs-ti_*.*-*")
.privileges(
// Require "delete_index" to perform ILM policy actions
TransportDeleteIndexAction.TYPE.name(),
// Require "read" and "view_index_metadata" for transform
"read",
"view_index_metadata"
)
.build(),

@CohenIdo
Copy link
Contributor Author

CohenIdo commented Sep 2, 2024

Hey @CohenIdo ! So after consulting with the team, I think we are good now. It's not super apparent from the PR, but we'd like to know why the source and destination index can't be the same one? Something like this:

Good Idea @SiddharthMantri , I changed the index pattern to match any future integration we are going to add.
Instead security_solution-wiz.vulnerability_latest-* I am using now security_solution-*.vulnerability_latest-*

Copy link

@SiddharthMantri SiddharthMantri left a comment

Choose a reason for hiding this comment

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

LGTM! After discussing with @CohenIdo, we now know why the name pattern was added. Thank you for your patience! Can we add the summary from the discussion behind the naming to the PR description?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>non-issue :Security/Authorization Roles, Privileges, DLS/FLS, RBAC/ABAC Team:Cloud Security Meta label for Cloud Security team Team:Security Meta label for security team v8.16.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants