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.

Sign up for GitHub

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

Jump to bottom

Add permissions to kibana_system for TI package transforms to support IOC expiration #94506

Merged
merged 11 commits into from
Apr 19, 2023
Merged

Add permissions to kibana_system for TI package transforms to support IOC expiration #94506

merged 11 commits into from
Apr 19, 2023

Conversation

kcreddy
Copy link
Contributor

@kcreddy kcreddy commented Mar 14, 2023

Closes #94505 by adding required permissions to kibana_system to support IOC expiration of Threat Intel indices.

@elasticsearchmachine elasticsearchmachine added v8.8.0 external-contributor Pull request authored by a developer outside the Elasticsearch team labels Mar 14, 2023
@kcreddy kcreddy changed the title Add permissions to kibana_system for TI package transforms to support… Add permissions to kibana_system for TI package transforms to support IOC expiration Mar 23, 2023
@kcreddy kcreddy marked this pull request as ready for review March 23, 2023 15:53
@kcreddy kcreddy self-assigned this Mar 23, 2023
@elasticsearchmachine elasticsearchmachine added the needs:triage Requires assignment of a team area label label Mar 23, 2023
@kcreddy kcreddy added the :Security/Authorization Roles, Privileges, DLS/FLS, RBAC/ABAC label Mar 23, 2023
@elasticsearchmachine elasticsearchmachine added Team:Security Meta label for security team and removed needs:triage Requires assignment of a team area label labels Mar 23, 2023
@elasticsearchmachine
Copy link
Collaborator

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

@kcreddy kcreddy requested review from andrewkroh and P1llus March 23, 2023 16:10
@ywangd
Copy link
Member

ywangd commented Mar 24, 2023

Ping @elastic/kibana-security for awareness

@azasypkin azasypkin requested a review from kc13greiner March 24, 2023 10:01
kc13greiner
kc13greiner previously approved these changes Mar 24, 2023
View reviewed changes
Copy link
Contributor

@kc13greiner kc13greiner left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM Discussing with KB Security team!

@kc13greiner
Copy link
Contributor

Ping @elastic/kibana-security for awareness

ACK! Thanks for the update!

@kc13greiner kc13greiner dismissed their stale review March 24, 2023 12:17

Discussing internally with KB Sec

@P1llus
Copy link
Member

P1llus commented Mar 30, 2023 •
edited
Loading

Just before we merge, @andrewkroh, would it be wise to have a pattern outside of logs-*? If we want to place all our TI transforms there in the future, and any future logs-* templates and global index patterns might complicate things if it covers the transform indices as well?

kc13greiner
kc13greiner requested changes Mar 30, 2023
View reviewed changes
Copy link
Contributor

@kc13greiner kc13greiner left a comment

Choose a reason for hiding this comment

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

Would it be possible to change the name of the indices being used so that they have a . preceding? This would be more in line with other system indices. (like the indices listed at the bottom of this comment

As is, this change may allow kibana_system to access user defined indices that follow a similar pattern.

Generally we would like to prevent the kibana_system user from getting too powerful.

cc: @azasypkin @legrego

@kcreddy
Copy link
Contributor Author

kcreddy commented Apr 3, 2023

Hey @andrewkroh,
As per @kc13greiner review above, I changed names of the indices with a preceding .. Could you please confirm if this also addresses @P1llus concern above? Or does it make sense to move into a different pattern altogether? If so, may I have a suggestion please? Thanks!

@andrewkroh
Copy link
Member

andrewkroh commented Apr 3, 2023 •
edited
Loading

would it be wise to have a pattern outside of logs-? If we want to place all our TI transforms there in the future, and any future logs- templates and global index patterns might complicate things if it covers the transform indices as well?

@P1llus What specifically is the concern that could cause a complication? And by "to place all our TI transforms there" do you mean the destination index name for the installed transforms? What naming conventions are you thinking?

In elastic/integrations#5582 the convention used is logs-ti_<name>_latest.<data_stream> for transform destination and the integration data streams are logs-ti_<name>.<data_stream>-*.

IIUC Fleet should create an index template for the transform's target index. This is described at https://github.com/elastic/package-spec/blob/29cc306237b541e7c32841548c09e25dcfd19333/spec/integration/elasticsearch/transform/spec.yml#L7-L10. That template should have a priority of 200 according to this. So as long as the transforms destination index names don't overlap with the integration data stream naming it shouldn't be an issue. I think the inclusion of _latest should work to prevent collisions.

I'm not opposed to a completely non-overlapping naming convention, but there is no prior art within Fleet that I'm aware of.

andrewkroh
andrewkroh reviewed Apr 3, 2023
View reviewed changes
Copy link
Member

@andrewkroh andrewkroh left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Would it be possible to change the name of the indices being used so that they have a . preceding? This would be more in line with other system indices.

@kc13greiner It's not really a "system index" as I have seen the term defined in various Elasticsearch issues. It is going to contain data that users directly access. Users will utilize it through the Security Indicator Match Rules, the Security Intelligence page, and possibly Discover. Therefore I think it would be better to not use a . to indicate this is a system or hidden.

I view this as a workaround until a proper mechanism exists for authorizing the transform setup based on the user installing the Fleet integration. That feature is described in elastic/kibana#137278, but AFAIK is not started yet. I would hope that we could remove the privileges directly related to transform setup after that task is completed.

These other changes have added similar workarounds:

But if a dot is required and users will still be able to directly read the data through the aforementioned apps then a dot prefix can work.

...core/src/main/java/org/elasticsearch/xpack/core/security/authz/store/ReservedRolesStore.java Outdated
@@ -871,7 +871,17 @@ public static RoleDescriptor kibanaSystemRoleDescriptor(String name) {
UpdateSettingsAction.NAME
)
.build(),
RoleDescriptor.IndicesPrivileges.builder().indices(".ds-logs-ti*").privileges("read", "view_index_metadata").build(),
RoleDescriptor.IndicesPrivileges.builder()
.indices(".ds-logs-ti*")
Copy link
Member

Choose a reason for hiding this comment

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

Should this be on the data stream itself rather than the backing indices? It looks like the prior art mostly uses the data stream name (e.g. logs-ti_*.*-*).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated as suggested

...core/src/main/java/org/elasticsearch/xpack/core/security/authz/store/ReservedRolesStore.java Outdated
RoleDescriptor.IndicesPrivileges.builder()
.indices(".ds-logs-ti*")
.privileges(
UpdateSettingsAction.NAME,
Copy link
Member

Choose a reason for hiding this comment

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

These first three (update settings, put mapping, and rollover) are already granted to logs-* on L798. So I think they can be omitted.

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 update settings, put mapping, and rollover as suggested

...core/src/main/java/org/elasticsearch/xpack/core/security/authz/store/ReservedRolesStore.java
UpdateSettingsAction.NAME,
PutMappingAction.NAME,
RolloverAction.NAME,
DeleteIndexAction.NAME,
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 there should be a comment similar to L829 that states that the ILM policies for Threat Intelligence packages may include delete actions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added separate comments for ILM and transform permissions

...core/src/main/java/org/elasticsearch/xpack/core/security/authz/store/ReservedRolesStore.java Outdated
UpdateSettingsAction.NAME
)
.build(),
// For source indices of the Threat Intel (ti*) packages that ships a transform for supporting IOC expiration
Copy link
Member

@andrewkroh andrewkroh Apr 3, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Perhaps this comment should be directly above the "read" priv, and indicate that read and view_index_metadata are required for transforms to read the source data. I say this because it is mixing privileges related to ILM and transforms.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added separate comments for ILM and transform permissions

...core/src/main/java/org/elasticsearch/xpack/core/security/authz/store/ReservedRolesStore.java Outdated
@@ -858,6 +858,31 @@ public static RoleDescriptor kibanaSystemRoleDescriptor(String name) {
)
.privileges("create_index", "delete_index", "read", "index", IndicesAliasesAction.NAME, UpdateSettingsAction.NAME)
.build(),
// For destination indices of the Threat Intel (ti*) packages that ships a transform for supporting IOC expiration
Copy link
Member

Choose a reason for hiding this comment

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

The TI packages all share the common prefix of ti_ so I would include the understand.

Suggested change
// For destination indices of the Threat Intel (ti*) packages that ships a transform for supporting IOC expiration
// For destination indices of the Threat Intel (ti_*) packages that ships a transform for supporting IOC expiration

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated as suggested

...core/src/main/java/org/elasticsearch/xpack/core/security/authz/store/ReservedRolesStore.java Outdated
@@ -858,6 +858,31 @@ public static RoleDescriptor kibanaSystemRoleDescriptor(String name) {
)
.privileges("create_index", "delete_index", "read", "index", IndicesAliasesAction.NAME, UpdateSettingsAction.NAME)
.build(),
// For destination indices of the Threat Intel (ti*) packages that ships a transform for supporting IOC expiration
RoleDescriptor.IndicesPrivileges.builder()
.indices(".logs-ti_*latest*")
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
.indices(".logs-ti_*latest*")
.indices(".logs-ti_*_latest.*")

I'm trying to be as specific as possible.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated as suggested

@P1llus
Copy link
Member

P1llus commented Apr 4, 2023

would it be wise to have a pattern outside of logs-? If we want to place all our TI transforms there in the future, and any future logs- templates and global index patterns might complicate things if it covers the transform indices as well?

@P1llus What specifically is the concern that could cause a complication? And by "to place all our TI transforms there" do you mean the destination index name for the installed transforms? What naming conventions are you thinking?

In elastic/integrations#5582 the convention used is logs-ti_<name>_latest.<data_stream> for transform destination and the integration data streams are logs-ti_<name>.<data_stream>-*.

IIUC Fleet should create an index template for the transform's target index. This is described at https://github.com/elastic/package-spec/blob/29cc306237b541e7c32841548c09e25dcfd19333/spec/integration/elasticsearch/transform/spec.yml#L7-L10. That template should have a priority of 200 according to this. So as long as the transforms destination index names don't overlap with the integration data stream naming it shouldn't be an issue. I think the inclusion of _latest should work to prevent collisions.

I'm not opposed to a completely non-overlapping naming convention, but there is no prior art within Fleet that I'm aware of.

@andrewkroh I was just concerned that any future ILM, index templates, dynamic templates or other settings that might be added to logs-* as a global pattern could affect the destination index for the transforms, outside of that I do not have any other concerns.

@kcreddy
Copy link
Contributor Author

kcreddy commented Apr 6, 2023

Hey @kc13greiner

@kc13greiner It's not really a "system index" as I have seen the term defined in various Elasticsearch issues. It is going to contain data that users directly access. Users will utilize it through the Security Indicator Match Rules, the Security Intelligence page, and possibly Discover. Therefore I think it would be better to not use a . to indicate this is a system or hidden.

The current code has the . prefix added. Based on Andrew's suggestion above, could you give your confirmation that its required or not? Could you please review and let me know if thats the only change required?

@kc13greiner
Copy link
Contributor

Hey @kc13greiner

@kc13greiner It's not really a "system index" as I have seen the term defined in various Elasticsearch issues. It is going to contain data that users directly access. Users will utilize it through the Security Indicator Match Rules, the Security Intelligence page, and possibly Discover. Therefore I think it would be better to not use a . to indicate this is a system or hidden.

The current code has the . prefix added. Based on Andrew's suggestion above, could you give your confirmation that its required or not? Could you please review and let me know if thats the only change required?

@kcreddy @andrewkroh

I have discussed with the security team and we are ok to approve without the . prefix on the indices. The docs do warn users against using these log- patterns to avoid collisions.

Sorry for the confusion.

@kcreddy
Copy link
Contributor Author

kcreddy commented Apr 19, 2023

@kc13greiner,
Sorry for the delay. I removed the dot . prefix. Could you please approve/merge if everything looks good?

kc13greiner
kc13greiner approved these changes Apr 19, 2023
View reviewed changes
Copy link
Contributor

@kc13greiner kc13greiner left a comment

Choose a reason for hiding this comment

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

LGTM!

@kcreddy kcreddy merged commit b77fc67 into elastic:main Apr 19, 2023
@kcreddy kcreddy mentioned this pull request Apr 21, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@andrewkroh andrewkroh andrewkroh approved these changes

@kc13greiner kc13greiner kc13greiner approved these changes

@P1llus P1llus Awaiting requested review from P1llus

Assignees

@kcreddy kcreddy

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 v8.8.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Need kibana_system privileges for Threat Intel (TI) package transforms to support IOC expiration
6 participants
@kcreddy @elasticsearchmachine @ywangd @kc13greiner @P1llus @andrewkroh