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

Expand beats_system role privileges #40876

Merged

Conversation

ycombinator
Copy link
Contributor

@ycombinator ycombinator commented Apr 4, 2019

Traditionally we have recommended that Beats send their monitoring data to the production Elasticsearch cluster. Beats do this by calling the POST _monitoring/bulk API. When Security is enabled this API call requires the cluster:admin/xpack/monitoring/bulk privilege. The built-in beats_system role has this privilege.

Going forward, Beats will be able to send their monitoring data directly to the monitoring Elasticsearch cluster. Beats will do this by calling the regular POST _bulk API. When Security is enabled this API call requires the indices:data/write/bulk privilege. Further, the call has to be able to create any indices that don't exist.

This PR expands the built-in beats_system role's privileges. Specifically, it adds index-level write and create_index privileges for .monitoring-beats-* indices.

This will allow Beats users to continue using the beats_system role for the new direct monitoring route when Security is enabled.

@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-security

@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-features

@urso
Copy link

urso commented Apr 5, 2019

They way monitoring works and is configured in Beats, this change makes sense. It creates the least user-disruption.

What about the template/index mapping? Is it setup correctly if Beats uses the bulk API?

@ycombinator
Copy link
Contributor Author

What about the template/index mapping? Is it setup correctly if Beats uses the bulk API?

The monitoring plugin in Elasticsarch sets up the monitoring index templates. So as long as the cluster that Beats is shipping to has xpack.monitoring.enabled: true (which is the default), the correct monitoring index templates will get created when the cluster is created.

new String[] { "monitor", MonitoringBulkAction.NAME},
new RoleDescriptor.IndicesPrivileges[]{
RoleDescriptor.IndicesPrivileges.builder()
.indices(".monitoring-beats-*").privileges("create_index", "write").build()
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we make this create instead of write ?
That gives the ability to index documents (including overwriting existing documents), but not call the update or delete APIs.

I believe that's sufficient for your purposes, and would get us closer to having least privilege users & roles.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, indeed create instead of write is sufficient here as the .monitoring-beats-* indices are meant to be append-only and the create privilege allows for bulk indexing as well.

Fixed in 6bf4d8b4756.

is(false));
assertThat(beatsSystemRole.indices().allowedIndicesMatcher(CreateIndexAction.NAME).test(index), is(true));
assertThat(beatsSystemRole.indices().allowedIndicesMatcher(IndexAction.NAME).test(index), is(true));
assertThat(beatsSystemRole.indices().allowedIndicesMatcher(DeleteAction.NAME).test(index), is(true));
Copy link
Contributor

Choose a reason for hiding this comment

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

If you're happy with my suggestion above, then this will need to be false.

@ycombinator ycombinator force-pushed the security/beats-system-role-expand branch from 348390b to 6bf4d8b Compare April 15, 2019 12:23
@ycombinator ycombinator force-pushed the security/beats-system-role-expand branch from 811b163 to d9e4cb0 Compare April 15, 2019 17:27
@ycombinator
Copy link
Contributor Author

@tvernum I made the change you suggested. This is ready for your review again. Thanks!

Copy link
Contributor

@tvernum tvernum left a comment

Choose a reason for hiding this comment

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

LGTM

@ycombinator ycombinator merged commit 64768bf into elastic:master Apr 16, 2019
@ycombinator ycombinator deleted the security/beats-system-role-expand branch April 16, 2019 01:25
ycombinator added a commit to ycombinator/elasticsearch that referenced this pull request Apr 16, 2019
Traditionally we have [recommended](https://www.elastic.co/guide/en/beats/filebeat/current/monitoring.html) that Beats send their monitoring data to the **production** Elasticsearch cluster. Beats do this by calling the `POST _monitoring/bulk` API. When Security is enabled this API call requires the `cluster:admin/xpack/monitoring/bulk` privilege. The built-in `beats_system` role has this privilege.

[Going forward](elastic/beats#9260), Beats will be able to send their monitoring data directly to the **monitoring** Elasticsearch cluster. Beats will do this by calling the regular `POST _bulk` API. When Security is enabled this API call requires the `indices:data/write/bulk` privilege. Further, the call has to be able to create any indices that don't exist.

This PR expands the built-in `beats_system` role's privileges. Specifically, it adds index-level `write` and `create_index` privileges for `.monitoring-beats-*` indices. 

This will allow Beats users to continue using the `beats_system` role for the new direct monitoring route when Security is enabled.
ycombinator added a commit that referenced this pull request Apr 16, 2019
Traditionally we have [recommended](https://www.elastic.co/guide/en/beats/filebeat/current/monitoring.html) that Beats send their monitoring data to the **production** Elasticsearch cluster. Beats do this by calling the `POST _monitoring/bulk` API. When Security is enabled this API call requires the `cluster:admin/xpack/monitoring/bulk` privilege. The built-in `beats_system` role has this privilege.

[Going forward](elastic/beats#9260), Beats will be able to send their monitoring data directly to the **monitoring** Elasticsearch cluster. Beats will do this by calling the regular `POST _bulk` API. When Security is enabled this API call requires the `indices:data/write/bulk` privilege. Further, the call has to be able to create any indices that don't exist.

This PR expands the built-in `beats_system` role's privileges. Specifically, it adds index-level `write` and `create_index` privileges for `.monitoring-beats-*` indices. 

This will allow Beats users to continue using the `beats_system` role for the new direct monitoring route when Security is enabled.
gurkankaymak pushed a commit to gurkankaymak/elasticsearch that referenced this pull request May 27, 2019
Traditionally we have [recommended](https://www.elastic.co/guide/en/beats/filebeat/current/monitoring.html) that Beats send their monitoring data to the **production** Elasticsearch cluster. Beats do this by calling the `POST _monitoring/bulk` API. When Security is enabled this API call requires the `cluster:admin/xpack/monitoring/bulk` privilege. The built-in `beats_system` role has this privilege.

[Going forward](elastic/beats#9260), Beats will be able to send their monitoring data directly to the **monitoring** Elasticsearch cluster. Beats will do this by calling the regular `POST _bulk` API. When Security is enabled this API call requires the `indices:data/write/bulk` privilege. Further, the call has to be able to create any indices that don't exist.

This PR expands the built-in `beats_system` role's privileges. Specifically, it adds index-level `write` and `create_index` privileges for `.monitoring-beats-*` indices. 

This will allow Beats users to continue using the `beats_system` role for the new direct monitoring route when Security is enabled.
@jakelandis jakelandis added v7.3.0 and removed v7.2.0 labels Jun 17, 2019
@jpountz
Copy link
Contributor

jpountz commented Jul 5, 2019

@ycombinator This change is on 7.3 already so I'm assuming there is nothing left to backport and removed the backport pending label.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants