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

[Fleet] Allow kibana_system to put datastream lifecycle #97732

Merged
merged 9 commits into from Jul 19, 2023

Conversation

nchaulet
Copy link
Member

@nchaulet nchaulet commented Jul 17, 2023

Description

In Fleet we need to be able to update datastream lifecycle when we install/update a package, that PR fix that by
allowingkibana_system to put datastream lifecycle.

Related kibana PR elastic/kibana#162078

@nchaulet nchaulet requested a review from a team as a code owner July 17, 2023 18:48
@elasticsearchmachine
Copy link
Collaborator

@nchaulet please enable the option "Allow edits and access to secrets by maintainers" on your PR. For more information, see the documentation.

@elasticsearchmachine elasticsearchmachine added v8.10.0 external-contributor Pull request authored by a developer outside the Elasticsearch team labels Jul 17, 2023
@nchaulet nchaulet added the Team:Security Meta label for security team label Jul 17, 2023
@elasticsearchmachine elasticsearchmachine removed the Team:Security Meta label for security team label Jul 17, 2023
@azasypkin azasypkin requested a review from a team July 18, 2023 12:09
Copy link
Member

@azasypkin azasypkin left a comment

Choose a reason for hiding this comment

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

Related kibana PR elastic/kibana#162078

Unfortunately the only description this PR has is Work in progress. Just for the record and context, could you please explain in a bit more detail why In Fleet we need to be able to update datastream lifecycle when we install/update a package (considering that it wasn't necessary before)?

Otherwise, LGTM from Kibana point of view.

UpdateSettingsAction.NAME,
PutMappingAction.NAME,
RolloverAction.NAME,
"indices:admin/data_stream/lifecycle/put"
Copy link
Member

Choose a reason for hiding this comment

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

nit: Cannot you use PutDataStreamLifecycleAction.NAME to not hardcode the string?

Copy link
Member Author

Choose a reason for hiding this comment

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

I tried to import PutDataStreamLifecycleAction here but without sucess java was not compiling

Copy link
Member

Choose a reason for hiding this comment

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

Got it, maybe ES team can guide on that then. In any case, it's not a big deal, just an optional nit.

Copy link
Contributor

Choose a reason for hiding this comment

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

The xpack core module/plugin doesn't have direct visibility to the dlm module/plugin which is why you can't reference the name directly. I would have a minor preference to put the action name in a variable in IndexPrivilege.java so that it is more centralized.

The core the change here (adding the priv) seems fine to me but @n1v0lg - can you take a look too, i don't remember the details of the this feature you assisted with.

Copy link
Contributor

@n1v0lg n1v0lg Jul 18, 2023

Choose a reason for hiding this comment

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

If .logs-endpoint.action.responses-* and the other indices with the leading dot are not system indices/system data streams, this change is fine as is. Otherwise, we'd have to update the internal user privileges here as well: https://github.com/elastic/elasticsearch/blob/main/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/user/InternalUsers.java#L133

@nchaulet could you confirm those are not system indices/system data streams? I don't think they are since the privilege definition does not include allowRestrictedIndices but just double-checking.

As far the privilege itself goes, "indices:admin/data_stream/lifecycle/put" is fine -- there is also a named privilege manage_data_stream_lifecycle which grants access to the PUT, GET, and DELETE data stream lifecycle APIs. If only updating is required though, going with the specific action as you do works and is good in terms of least privilege.

Copy link
Member Author

Choose a reason for hiding this comment

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

the .logs-* are not system indices just some hidden datastreams used by elastic agent and defined in integrations.

It seems we only need the PUT currently

@nchaulet
Copy link
Member Author

Unfortunately the only description this PR has is Work in progress. Just for the record and context, could you please explain in a bit more detail why In Fleet we need to be able to update datastream lifecycle when we install/update a package (considering that it wasn't necessary before)?

@azasypkin just updated the PR with a litle more than "Work in progress", to resume here Fleet was not installing DLM before 8.10 so it's why it was not necessary before.

Copy link
Contributor

@n1v0lg n1v0lg left a comment

Choose a reason for hiding this comment

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

LGTM

@n1v0lg n1v0lg added :Security/Authorization Roles, Privileges, DLS/FLS, RBAC/ABAC Team:Security Meta label for security team labels Jul 18, 2023
@elasticsearchmachine
Copy link
Collaborator

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

@n1v0lg
Copy link
Contributor

n1v0lg commented Jul 18, 2023

@elasticmachine update branch

@elasticsearchmachine
Copy link
Collaborator

Hi @nchaulet, I've created a changelog YAML for you.

@n1v0lg n1v0lg removed the Team:Fleet label Jul 18, 2023
@n1v0lg
Copy link
Contributor

n1v0lg commented Jul 18, 2023

@nchaulet just a heads up: I added the missing labels (including >enhancement) -- this also automatically generated a changelog.

@nchaulet nchaulet merged commit a34ff55 into main Jul 19, 2023
16 checks passed
@nchaulet nchaulet deleted the feature-fleet-put-datastream-lifecycle branch July 19, 2023 12:53
felixbarny pushed a commit to felixbarny/elasticsearch that referenced this pull request Aug 3, 2023
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:Fleet Team:Security Meta label for security team v8.10.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants