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 monitor and view_index_metadata to built-in kibana_system role #57755

Merged
merged 3 commits into from
Jun 15, 2020
Merged

Add monitor and view_index_metadata to built-in kibana_system role #57755

merged 3 commits into from
Jun 15, 2020

Conversation

afharo
Copy link
Member

@afharo afharo commented Jun 5, 2020

Required for the reimplementation planned of elastic/kibana#64935. Allows the kibana user to collect Data telemetry in a background task by giving the kibana_system reserved role the view_index_metadata and monitoring privileges on *.

@afharo afharo added :Security/Authorization Roles, Privileges, DLS/FLS, RBAC/ABAC >enhancement v7.9.0 v8.0.0 labels Jun 5, 2020
@afharo afharo marked this pull request as ready for review June 5, 2020 18:32
@elasticmachine
Copy link
Collaborator

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

@elasticmachine elasticmachine added the Team:Security Meta label for security team label Jun 5, 2020
@afharo afharo requested review from ywangd, ogupte, albertzaharovits, a team, legrego and kobelb and removed request for ogupte, ywangd and albertzaharovits June 8, 2020 08:51
Copy link
Member

@ywangd ywangd left a comment

Choose a reason for hiding this comment

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

IIUC, privileges for 3 actions are required:

  • GetIndexAction
  • GetMappingsAction
  • IndicesStatsAction

view_index_metadata and monitor grant many more privileges than required. Other than the 3 required actions, they also allow following actions:

  • GetFieldMappingsAction
  • ClusterSearchShardsAction
  • GetSettingsAction
  • ExplainLifecycleAction
  • ValidateQueryAction
  • RecoveryAction
  • IndicesSegmentsAction
  • GetSettingsAction
  • IndicesShardStoresAction
  • UpgradeStatusAction

We could potentially make new named privileges to get better granularity for the actions. But I am not sure whether it is premature and/or superficial, unless any above actions are expensive or can be easily abused.

Alternatively, I wonder if we could defer this to be a runtime configuration instead of code level. Sorry if this has been discussed already. I may not have the right context here, so I'll let other stack security team members to chime in.

@albertzaharovits
Copy link
Contributor

Thanks for raising the PR, @afharo .

I think the proposed change is relatively benign given that it roughly grants access to index settings and metadata, for all indices, which are already available in the response for GET cluster state, which is permitted under the existing monitor cluster privilege (which kibana-system already has). I also believe we're better off granting the "whole" privileges, as you're proposing in this PR, instead of piece-wise as @ywangd is suggesting, because the named privileges are consistent in the sense that the same information is accessible by different APIs (eg GetFieldMappingsAction and GetMappingsAction).

Since our team meeting is scheduled in a couple of hours, let us have a quick sync on this one, and I'll report back.

@ywangd
Copy link
Member

ywangd commented Jun 10, 2020

I also believe we're better off granting the "whole" privileges, as you're proposing in this PR, instead of piece-wise as @ywangd is suggesting, because the named privileges are consistent in the sense that the same information is accessible by different APIs (eg GetFieldMappingsAction and GetMappingsAction).

Just to clarify that I was not suggesting granting access for actions directly because it would be against our recommendation. I was wondering whether we could make a new named privilege (e.g. telemetry) or a new role (e.g. telemetry_user) to group relevant actions or privileges, and grant the new privilege or role to kibana.

@afharo
Copy link
Member Author

afharo commented Jun 11, 2020

Thank you both on the updates! I'm sure you'll find the best approach to this. I'll let you discuss it thouroughly. Just let me know if you want me to apply any changes or go on with this one.
Thank you again!

@albertzaharovits
Copy link
Contributor

Just to clarify that I was not suggesting granting access for actions directly because it would be against our recommendation. I was wondering whether we could make a new named privilege (e.g. telemetry) or a new role (e.g. telemetry_user) to group relevant actions or privileges, and grant the new privilege or role to kibana.

Apologies Yang for disregarding your suggestion in the above.
My opinion is that view_index_metadata and monitor grant more privileges than necessary, as is usually the case with asks like these, but that it's not an issue because the bundled actions expose the same information in a different form (eg GetFieldMappingsAction and GetMappingsAction), a subset of the information presented by other actions (eg IndicesStatsAction and IndicesSegmentsAction), or just information that semantically belongs together and there's no practical risk justifying breaking a privilege into slight variations of itself (generally my bias in this case is against a long list of privileges because that increases the burden on the administrator, which might be tempted to "add privileges until it finally works" instead of taking the time to understand each and every privilege).

Of course discussions to reform the privileges structure doesn't have to hold this PR, so I would say we can merge this and discuss breaking the monitor and view_index_metadata privileges with another occasion.

@ywangd would you agree with this course of action?

@albertzaharovits albertzaharovits self-requested a review June 12, 2020 10:48
@ywangd
Copy link
Member

ywangd commented Jun 14, 2020

@albertzaharovits Your analysis sounds right to me. Thanks for taking time to go through your reasoning.

@albertzaharovits
Copy link
Contributor

@elasticmachine update branch

@albertzaharovits albertzaharovits changed the title Add GET */_mappings and */_stats indices privileges to kibana_system role Add monitor and view_index_metadata to built-in kibana_system role Jun 15, 2020
@albertzaharovits albertzaharovits self-assigned this Jun 15, 2020
@albertzaharovits albertzaharovits merged commit 1b4ccef into elastic:master Jun 15, 2020
@afharo afharo deleted the roles/kibana_system/add-view-metadata-and-monitoring branch June 15, 2020 11:34
albertzaharovits pushed a commit that referenced this pull request Jun 15, 2020
…ole (#57755)

Allows the kibana user to collect data telemetry in a background
task by giving the kibana_system built-in role the view_index_metadata
and monitoring privileges over all indices (*).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>enhancement :Security/Authorization Roles, Privileges, DLS/FLS, RBAC/ABAC Team:Security Meta label for security team v7.9.0 v8.0.0-alpha1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants