Refactor cluster privileges and cluster permission #45265
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…44729) This commit renames `ConditionalClusterPrivilege` to `ConfigurableClusterPrivilege`. `ConfigurableClusterPrivilege` is a renderable cluster privilege exposed as a `global` field in role descriptor.
The current implementations make it difficult for adding new privileges (example: a cluster privilege which is more than cluster action-based and not exposed to the security administrator). On the high level, we would like our cluster privilege either: - a named cluster privilege This corresponds to `cluster` field from the role descriptor - or a configurable cluster privilege This corresponds to the `global` field from the role-descriptor and allows a security administrator to configure them. Some of the responsibilities like the merging of action based cluster privileges are now pushed at cluster permission level. How to implement the predicate (using Automaton) is being now enforced by cluster permission. `ClusterPermission` helps in enforcing the cluster level access either by performing checks against cluster action and optionally against a request. It is a collection of one or more permission checks where if any of the checks allow access then the permission allows access to a cluster action. Implementations of cluster privilege must be able to provide information regarding the predicates to the cluster permission so that can be enforced. This is enforced by making implementations of cluster privilege aware of cluster permission builder and provide a way to specify how the permission is to be built for a given privilege. Other than this there is a requirement where we would want to know if a cluster permission is implied by another cluster-permission (`has-privileges`). This is helpful in addressing queries related to privileges for a user. This is not just simply checking of cluster permissions since we do not have access to runtime information (like request object). This refactoring does not try to address those scenarios. Relates #44048
bizybot
added
>refactoring
:Security/Authorization
|
Pinging @elastic/es-security |
|
@elasticmachine run elasticsearch-ci/packaging-sample |
|
Shouldn't #45267 be batched on this PR as well? |
Hi @albertzaharovits, |
|
Hi @albertzaharovits and @tvernum, As discussed I have closed the other PR so this can be merged so I can raise next PRs related to manage own API key privilege. Please review when you get some time. Thank you. |
albertzaharovits
approved these changes
Aug 8, 2019
.../src/main/java/org/elasticsearch/xpack/core/security/authz/permission/ClusterPermission.java
Outdated
Show resolved
Hide resolved
| if (permissionCheck instanceof AutomatonPermissionCheck) { | ||
| return Operations.subsetOf(((AutomatonPermissionCheck) permissionCheck).automaton, this.automaton); | ||
| } | ||
| return false; |
There was a problem hiding this comment.
I worry we might lose track of the has_privileges scenarios that do not work because of this, e.g. all does not imply application privileges.
After this is merged to master, can you please raise an issue about it, so we don't have to fake surprise when that happens? 😛 (It will require some work to clearly scope out the scenarios that don't work).
| * @param builder {@link ClusterPermission.Builder} | ||
| * @return an instance of {@link ClusterPermission.Builder} | ||
| */ | ||
| ClusterPermission.Builder buildPermission(ClusterPermission.Builder builder); |
There was a problem hiding this comment.
We had a bunch of discussions about "implies", "privileges" and "permissions". Given this interface, it is impossible for a PermissionCheck to say if a ClusterPrivilege is implied or not.
I suggest we enrich this interface and have an impliedBy(ClusterPermission clusterPermission) or impliedBy(PermissionCheck check) method.
This is a suggestion for our future selves, not to be treated in this PR, ofc!
...main/java/org/elasticsearch/xpack/security/rest/action/user/RestGetUserPrivilegesAction.java
Show resolved
Hide resolved
|
@elasticmachine update branch |
|
|
tvernum
approved these changes
Aug 12, 2019
bizybot
added a commit
to bizybot/elasticsearch
that referenced
this pull request
Aug 12, 2019
The current implementations make it difficult for adding new privileges (example: a cluster privilege which is more than cluster action-based and not exposed to the security administrator). On the high level, we would like our cluster privilege either: - a named cluster privilege This corresponds to `cluster` field from the role descriptor - or a configurable cluster privilege This corresponds to the `global` field from the role-descriptor and allows a security administrator to configure them. Some of the responsibilities like the merging of action based cluster privileges are now pushed at cluster permission level. How to implement the predicate (using Automaton) is being now enforced by cluster permission. `ClusterPermission` helps in enforcing the cluster level access either by performing checks against cluster action and optionally against a request. It is a collection of one or more permission checks where if any of the checks allow access then the permission allows access to a cluster action. Implementations of cluster privilege must be able to provide information regarding the predicates to the cluster permission so that can be enforced. This is enforced by making implementations of cluster privilege aware of cluster permission builder and provide a way to specify how the permission is to be built for a given privilege. This commit renames `ConditionalClusterPrivilege` to `ConfigurableClusterPrivilege`. `ConfigurableClusterPrivilege` is a renderable cluster privilege exposed as a `global` field in role descriptor. Other than this there is a requirement where we would want to know if a cluster permission is implied by another cluster-permission (`has-privileges`). This is helpful in addressing queries related to privileges for a user. This is not just simply checking of cluster permissions since we do not have access to runtime information (like request object). This refactoring does not try to address those scenarios. Relates elastic#44048
bizybot
added a commit
that referenced
this pull request
Aug 12, 2019
The current implementations make it difficult for adding new privileges (example: a cluster privilege which is more than cluster action-based and not exposed to the security administrator). On the high level, we would like our cluster privilege either: - a named cluster privilege This corresponds to `cluster` field from the role descriptor - or a configurable cluster privilege This corresponds to the `global` field from the role-descriptor and allows a security administrator to configure them. Some of the responsibilities like the merging of action based cluster privileges are now pushed at cluster permission level. How to implement the predicate (using Automaton) is being now enforced by cluster permission. `ClusterPermission` helps in enforcing the cluster level access either by performing checks against cluster action and optionally against a request. It is a collection of one or more permission checks where if any of the checks allow access then the permission allows access to a cluster action. Implementations of cluster privilege must be able to provide information regarding the predicates to the cluster permission so that can be enforced. This is enforced by making implementations of cluster privilege aware of cluster permission builder and provide a way to specify how the permission is to be built for a given privilege. This commit renames `ConditionalClusterPrivilege` to `ConfigurableClusterPrivilege`. `ConfigurableClusterPrivilege` is a renderable cluster privilege exposed as a `global` field in role descriptor. Other than this there is a requirement where we would want to know if a cluster permission is implied by another cluster-permission (`has-privileges`). This is helpful in addressing queries related to privileges for a user. This is not just simply checking of cluster permissions since we do not have access to runtime information (like request object). This refactoring does not try to address those scenarios. Relates #44048
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The current implementations make it difficult for
adding new privileges (example: a cluster privilege which is
more than cluster action-based and not exposed to the security
administrator). On the high level, we would like our cluster privilege
either:
This corresponds to
clusterfield from the role descriptorThis corresponds to the
globalfield from the role-descriptor andallows a security administrator to configure them.
Some of the responsibilities like the merging of action based cluster privileges
are now pushed at cluster permission level. How to implement the predicate
(using Automaton) is being now enforced by cluster permission.
ClusterPermissionhelps in enforcing the cluster level access either byperforming checks against cluster action and optionally against a request.
It is a collection of one or more permission checks where if any of the checks
allow access then the permission allows access to a cluster action.
Implementations of cluster privilege must be able to provide information
regarding the predicates to the cluster permission so that can be enforced.
This is enforced by making implementations of cluster privilege aware of
cluster permission builder and provide a way to specify how the permission is
to be built for a given privilege.
This commit renames
ConditionalClusterPrivilegetoConfigurableClusterPrivilege.ConfigurableClusterPrivilegeis a renderable cluster privilege exposedas a
globalfield in role descriptor.Other than this there is a requirement where we would want to know if a cluster
permission is implied by another cluster-permission (
has-privileges).This is helpful in addressing queries related to privileges for a user.
This is not just simply checking of cluster permissions since we do not
have access to runtime information (like request object).
This refactoring does not try to address those scenarios.
Relates #44048