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 read permissions for apm_user role to APM fleet indices #68749

Merged
merged 10 commits into from
Mar 31, 2021

Conversation

sorenlouv
Copy link
Member

@sorenlouv sorenlouv commented Feb 9, 2021

Related: elastic/kibana#87501

Permissions to apm_user
Starting in 7.12 APM Server will be available under fleet. In this mode data will no longer be ingested to apm-* indices but to logs-apm*, metrics-apm*, traces-apm*.

This PR ensures that users upgrading to APM Server under fleet can still access APM data.

Permissions to kibana_user
In Kibana there is a background task that collects telemetry. We want ensure that this background task can access APM indices and upload telemetry based on this.

Note to self:

# run tests
./gradlew :x-pack:plugin:core:test --tests 'org.elasticsearch.xpack.core.security.authz.store.*'

cc @elastic/apm-ui

@sorenlouv sorenlouv added the :Security/Authorization Roles, Privileges, DLS/FLS, RBAC/ABAC label Feb 9, 2021
@elasticmachine elasticmachine added the Team:Security Meta label for security team label Feb 9, 2021
@elasticmachine
Copy link
Collaborator

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

@sorenlouv
Copy link
Member Author

jenkins test this please

@sorenlouv sorenlouv removed the request for review from tvernum February 15, 2021 09:58
@sorenlouv sorenlouv force-pushed the add-default-access-to-fleet-indices branch from a45309d to c94086d Compare February 15, 2021 11:28
@sorenlouv sorenlouv force-pushed the add-default-access-to-fleet-indices branch from c94086d to aac7957 Compare February 16, 2021 19:02
@kobelb
Copy link
Contributor

kobelb commented Feb 16, 2021

There are some risks to this PR, even though it only changes the end-user's apm_user role to have access to the logs-apm*, metrics-apm* and traces-apm* indices. If users happen to be using these indices for different purposes than we anticipate, we would be granting users with the apm_user role privileges that they shouldn't have. For example, a user might be using the traces-apma index to trace something for https://www.apma.org/.

Do we need this change to go into 7.12? Is it possible to delay it until 8.0?

@sorenlouv
Copy link
Member Author

Do we need this change to go into 7.12? Is it possible to delay it until 8.0?

Unfortunately, APM Server will be released under Fleet as experimental in 7.12 and GA (ish) in 7.13. So waiting until 8.0 is not possible afaict.
We could wait until 7.13 but I don't think that'll give us anything.

@sorenlouv
Copy link
Member Author

@kobelb Discussing this with people on the fleet side I was made aware that the indices I added (logs-apm*, metrics-apm*, traces-apm*) are already "reserved" to elastic data streams, and using them for other purposes is recommended against. There's obviously still a risk that users might be using those but I think it's less likely.

@bytebilly
Copy link
Contributor

I understand @kobelb concerns about security implications for this change, but on the other side the user experience would be very impacted if they will move to the new approach and a user with a role called apm_user will not be able to be a user of APM anymore.

This is probably a problem we have to face in the near future when we'll introduce predefined roles and Solutions may need to modify their index permissions in a minor, for example because of a new feature.

I'm wondering if we can mitigate the security risk, for example using logs-apm.* (or whatever is the less greedy pattern that identifies the new APM indices) instead of logs-apm*. The more we make it specific, the more we avoid conflicts.

Another option could be to send warnings (or even prevent upgrades) if you have indices that match the pattern and are not "because of APM". Not sure how we can make them visible enough, but it could be a compromise.

If you already have the conflict with existing indices, I suppose it would be a wider problem as Fleet installs templates and will start ingesting data to those indices regardless of the permissions of apm_user.

What do you think?

@kobelb
Copy link
Contributor

kobelb commented Mar 18, 2021

@kobelb Discussing this with people on the fleet side I was made aware that the indices I added (logs-apm*, metrics-apm*, traces-apm*) are already "reserved" to elastic data streams, and using them for other purposes is recommended against. There's obviously still a risk that users might be using those but I think it's less likely.

Can you or the fleet people elaborate on this? The only official documentation that I've found about the logs-*-*, metrics-*-* and traces-*-* indices have been the detailed 7.9.0 breaking changes they aren't even mentioned in the 7.9.0 release note's breaking changes. This breaking change specifies that the index templates are created for use by the Elastic Agent, but it doesn't go into the details about how these indices will be used. Are all logs-*-*, metrics-*-* and traces-*-* now officially reserved for the Elastic Agent to use however they see fit? Is this documented anywhere?

Additionally, the creation of these index-templates has caused issues for customers and they can currently be disabled. This confirms my suspicion that actual users are using these indices for purposes other than how we anticipated. Even if the user has turned off the stack installed index templates, we're still going to be changing the definition of the apm_user role to now have access to these indices.

I understand @kobelb concerns about security implications for this change, but on the other side the user experience would be very impacted if they will move to the new approach and a user with a role called apm_user will not be able to be a user of APM anymore.

Agreed. That's why I was originally asking the reasoning for making this change in 7.x and whether we could wait until 8.0. Otherwise, I think we need to be honest with ourselves and our users and document this as a new breaking change that has security implications.

@sorenlouv
Copy link
Member Author

Are all logs--, metrics-- and traces-- now officially reserved for the Elastic Agent to use however they see fit? Is this documented anywhere?

This is my understanding but I will track this down.

Otherwise, I think we need to be honest with ourselves and our users and document this as a new breaking change that has security implications.

Agree that this change should be communicated clearly.

@sorenlouv
Copy link
Member Author

@kobelb The datastreams are mentioned here: https://www.elastic.co/guide/en/elasticsearch/reference/7.11/set-up-a-data-stream.html#create-a-data-stream-template

I don't know if that's enough. We don't clearly state that they are reserved and should not be used for custom purposes.

@kobelb
Copy link
Contributor

kobelb commented Mar 25, 2021

@sqren and I had a quick synchronous chat about this, please keep me honest on the following.

Given the fact that the APM team planned to deprecate the apm_user role in the near future, we decided to do the following:

  • grant the apm_user role additional privileges to read from the logs-apm*, metrics-apm*, traces-apm* indices
  • document the increased privileges as a breaking change, so users are aware of the ramifications
  • deprecate the apm_user

Long-term, we should figure out what changes are necessary to allow people to more easily only grant users to access to APM. This required further thought about how it'd work with the new indexing strategy.

@sorenlouv
Copy link
Member Author

jenkins test this please

@sorenlouv sorenlouv force-pushed the add-default-access-to-fleet-indices branch from 498e26a to 6bc0696 Compare March 29, 2021 09:32
@sorenlouv
Copy link
Member Author

jenkins test this please

@sorenlouv sorenlouv force-pushed the add-default-access-to-fleet-indices branch from ffc8ea1 to f83be80 Compare March 31, 2021 12:45
@sorenlouv sorenlouv merged commit 8998045 into master Mar 31, 2021
@sorenlouv sorenlouv deleted the add-default-access-to-fleet-indices branch March 31, 2021 15:29
@sorenlouv sorenlouv mentioned this pull request May 31, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants