-
Notifications
You must be signed in to change notification settings - Fork 24.9k
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_snapshot' cluster privilege #50489
Add 'monitor_snapshot' cluster privilege #50489
Conversation
Pinging @elastic/es-security (:Security/Authorization) |
@elasticmachine ok to test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you again for the contribution @J-Bean !
I see you've added a new cluster privilege monitor_snapshot
as well as a new built-in role monitor_snapshot_user
. But I don't think adding a built-in role is appropriate. We have too many of those and the addition of a new one, without a reasonable motivation, hurts the discoverability of the others. When we add a new built-in role we think of privileges that justifiably should go together, such as the privileges required to use a distinct Kibana feature (transform) or an Elastic Stack integration (Beats).
The case for the snapshot_user
role, on which you've based your new role, is Curator. Curator requires that it can snapshot as well as list all the indices. In addition to supporting Curator, it would be incongruous for any other users to be able to snapshot indices that they cannot list. Given that at the moment we cannot limit the indices a user can snapshot, the next best alternative was to create a built-in role that grants snapshot and list permissions for all indices.
Given the above, I don't think there is a case for a new snapshot monitoring built-in role.
Other than that, the changes look good to me!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @J-Bean !
LGTM
We should wait for Tim to have his say before merging.
I think this change highlights a gap in our testing. Separate to this PR, we need a non-integration |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
@elasticmachine update branch |
This adds a new cluster privilege `monitor_snapshot` which is a restricted version of `create_snapshot`, granting the same privileges to view snapshot and repository info and status but not granting the actual privilege to create a snapshot. Co-authored-by: j-bean <[email protected]>
This adds a new cluster privilege `monitor_snapshot` which is a restricted version of `create_snapshot`, granting the same privileges to view snapshot and repository info and status but not granting the actual privilege to create a snapshot. Co-authored-by: Anton Shuvaev <[email protected]>
PR adds 'monitor_snapshot' cluster privilege.
Closes #50210