-
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
Introduce asynchronous RBACEngine #36245
Conversation
Pinging @elastic/es-security |
run gradle build tests 2 |
The xpack security user is a system user and the changes in this branch make bulk requests audit a lot more. Without this change, the bulk request can overwhelm the index audit trail and lead to an OOM.
run gradle build tests 2 |
if (indexAccessControl == null || indexAccessControl.isGranted() == false) { | ||
item.abort(resolvedIndex, denial(requestId, authentication, itemAction, request, authzInfo)); | ||
} else if (audit.get()) { | ||
auditTrail.accessGranted(requestId, authentication, itemAction, request, authzInfo); |
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.
this is important to note, we previously did not audit access granted for individual bulk items, but now we do. I think the fact that we didn't was wrong but it does greatly increase the amount of audit logging that we will do when bulk requests are used and we already do a ton :|
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.
I think logically you are correct.
When we added in the checking of bulk items, I made a conscious decision to make the change as minimal as possible and not audit access granted.
I think we should add this change for 7.0, but not backport it to 6.x.
The new behaviour is certainly more correct, but it seems a bit unexpected to customers to do it in a very late minor without any flag to turn it off.
run gradle build tests 2 |
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.
I got part way through this, but it's been updated so Github won't let me add more comments.
So I'm going to submit what I wrote so far and then try again.
|
||
boolean checkSameUserPermissions(String action, TransportRequest request, Authentication authentication); | ||
|
||
boolean shouldAuthorizeIndexActionNameOnly(String action, TransportRequest request); |
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.
Nit: The argument order for checkSameUserPermissions
and shouldAuthorizeIndexActionNameOnly
is inconsistent with the other methods.
...lugin/security/src/main/java/org/elasticsearch/xpack/security/authz/AuthorizationEngine.java
Outdated
Show resolved
Hide resolved
|
||
void buildIndicesAccessControl(Authentication authentication, TransportRequest request, String action, | ||
AuthorizationInfo authorizationInfo, Set<String> indices, | ||
SortedMap<String, AliasOrIndex> aliasAndIndexLookup, ActionListener<IndexAuthorizationResult> listener); |
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.
I still need to read further and see how this is used, but it feels like it leaks too much in the way of RBAC implementation details.
We have a variety of methods for loading a list of authorized indices, and checking action names, but the core of the problem being solved is "should this user be allowed to perform this action using this request."
We could provide convenience utilities for when this request is an indices request (and pull out the list of indices) but the interface right now very much feels like it was extracted from the implementation (as it was).
|
||
// first, we'll check if the action is a cluster action. If it is, we'll only check it against the cluster permissions | ||
private void authorizePostRunAs(final Authentication authentication, final String action, final String requestId, |
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.
This method does a lot, but is named based on the sequence in which it runs "after run as".
That makes it hard to follow exactly what's going on.
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.
I've done the best I can given the nature of this PR. I assume most of the code has been moved around through IDE refactoring, and passes the tests, so there's no a lot of value I can add in reviewing it.
I've mostly tried to look at the API and the resulting flow control.
// need to validate that the action is allowed and then move on | ||
authorizeIndexActionName(authentication, action, requestId, unwrappedRequest, authzInfo, authzEngine, listener); | ||
} else if (authzEngine.shouldAuthorizeIndexActionNameOnly(action, unwrappedRequest)) { | ||
authorizeIndexActionName(authentication, action, requestId, unwrappedRequest, authzInfo, authzEngine, listener); |
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.
I'm not fundamentally opposed to this approach, but it seems strange, and I assume it's being done to try and optimise performance, but it creates an API that feels a bit weird.
Why ask the engine whether it wants to authorize by name? Why not just let it authorize however it wants - why does the service care?
I take it that we do this so we can avoid doing the index name resolution if it's not going to be used, but I think the API would be improved if we did by passing a Supplier
or something like that to a single authorizeIndexAction
method, and let the engine call it if/when it needed to.
Although I see from your comments below that some of the flow here is based on the desire to keep BWC in responses. If that's the case here, then perhaps we can come back to this as a master only change later, but I have a gut feel that if we push more of this flow control into the engine, then the BWC could be handled by the RBAC engine rather than here.
if (indexAccessControl == null || indexAccessControl.isGranted() == false) { | ||
item.abort(resolvedIndex, denial(requestId, authentication, itemAction, request, authzInfo)); | ||
} else if (audit.get()) { | ||
auditTrail.accessGranted(requestId, authentication, itemAction, request, authzInfo); |
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.
I think logically you are correct.
When we added in the checking of bulk items, I made a conscious decision to make the change as minimal as possible and not audit access granted.
I think we should add this change for 7.0, but not backport it to 6.x.
The new behaviour is certainly more correct, but it seems a bit unexpected to customers to do it in a very late minor without any flag to turn it off.
This reverts commit 54c33bd.
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.
I like where this ended up.
Given the extent of the refactoring, I haven't tried to review every line, but I think the splt between AuthzService and RBAC engine has ended up about right.
If the implementors of other engines want to reuse some of the RBAC code we can move it to AuthzUtils or a base class when needed.
}), threadContext); | ||
authorizeRunAs(authentication, unwrappedRequest, action, requestId, authzInfo, runAsListener); | ||
} else { | ||
authorizeAction(authentication, action, requestId, unwrappedRequest, authzInfo, listener); |
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.
Very minor nit: It would be nice if authorizeRunAs
and authorizeAction
had the same parameter order.
commit 05b3a571658312fa4259b652d9b8478ecec278e7 Author: jaymode <[email protected]> Date: Thu Jan 31 15:48:48 2019 -0700 Move request interceptors to AuthorizationService This change moves the RequestInterceptor iteration from the action filter to the AuthorizationService. This is done to remove the need for the use of a role within the request interceptors and replace it with the AuthorizationEngine. The AuthorizationEngine interface was also enhanced with a new method that is used to determine if a users permission on one index is a subset of their permissions on a list of indices or aliases. Additionally, this change addresses some leftover cleanups. commit 0e1c191 Merge: 3280607 b7de8e1 Author: jaymode <[email protected]> Date: Thu Jan 31 08:56:45 2019 -0700 Merge branch 'master' into security_authz_engine commit 3280607 Author: Jay Modi <[email protected]> Date: Tue Jan 29 14:17:37 2019 -0700 Allow authorization engines as an extension (elastic#37785) Authorization engines can now be registered by implementing a plugin, which also has a service implementation of a security extension. Only one extension may register an authorization engine and this engine will be used for all users except reserved realm users and internal users. commit d628008 Author: jaymode <[email protected]> Date: Tue Jan 29 10:06:09 2019 -0700 fix RBACEngine after restricted indices changes commit 5074683 Merge: 74f2e99 3c9f703 Author: jaymode <[email protected]> Date: Tue Jan 29 08:09:39 2019 -0700 Merge branch 'master' into security_authz_engine commit 74f2e99 Merge: 7846ee8 899dfc3 Author: jaymode <[email protected]> Date: Fri Jan 25 15:02:07 2019 -0700 Merge branch 'master' into security_authz_engine commit 7846ee8 Merge: b9a2c81 a81931b Author: jaymode <[email protected]> Date: Thu Jan 24 07:52:08 2019 -0700 Merge branch 'master' into security_authz_engine commit b9a2c81 Author: jaymode <[email protected]> Date: Tue Jan 22 09:48:11 2019 -0700 Fix resolving restricted indices after merging commit d98a77a Merge: 83cde40 5c1a1f7 Author: jaymode <[email protected]> Date: Tue Jan 22 09:09:23 2019 -0700 Merge branch 'master' into security_authz_engine commit 83cde40 Author: Jay Modi <[email protected]> Date: Tue Jan 22 08:03:19 2019 -0700 Add javadoc to the AuthorizationEngine interface (elastic#37620) This commit adds javadocs to the AuthorizationEngine interface aimed at developers of an authorization engine. Additionally, some classes were also moved to the core project so that they are ready to be exposed once we allow authorization engines to be plugged in. commit 9a240c6 Author: Jay Modi <[email protected]> Date: Thu Jan 17 19:33:35 2019 -0700 Encapsulate request, auth, and action name (elastic#37495) This change introduces a new class called RequestInfo that encapsulates the common objects that are passed to the authorization engine methods. By doing so, we give ourselves a way of adding additional data without breaking the interface. Additionally, this also reduces the need to ensure we pass these three parameters in the same order everywhere for consistency. commit 6278eab Merge: c555a44 4351a5e Author: jaymode <[email protected]> Date: Thu Jan 17 07:51:32 2019 -0700 Merge branch 'master' into security_authz_engine commit c555a44 Merge: 1362ab6 ecf0de3 Author: jaymode <[email protected]> Date: Wed Jan 16 10:24:33 2019 -0700 Merge branch 'master' into security_authz_engine commit 1362ab6 Author: Jay Modi <[email protected]> Date: Wed Jan 16 10:23:45 2019 -0700 Replace AuthorizedIndices class with a List (elastic#37328) This change replaces the AuthorizedIndices class with a simple list. The change to a simple list does remove the lazy loading of the authorized indices in favor of simpler code as the loading of this list is now an asynchronous operation that is delegated to the authorization engine. commit 0246442 Merge: 8ccdc19 a2a40c5 Author: jaymode <[email protected]> Date: Tue Jan 15 10:49:12 2019 -0700 Merge branch 'master' into security_authz_engine commit 8ccdc19 Author: Jay Modi <[email protected]> Date: Mon Jan 7 13:43:22 2019 -0700 Introduce asynchronous RBACEngine (elastic#36245) In order to support the concept of different authorization engines, this change begins the refactoring of the AuthorizationService to support this. Previously, the asynchronous work for authorization was performed by the AsyncAuthorizer class, but this tied the authorization service to a role based implementation. In this change, the authorize method become asynchronous and delegates much of the actual permission checking to an AuthorizationEngine. The pre-existing RBAC permission checking has been abstracted into the RBACEngine. The majority of calls to AuthorizationEngine instances are asynchronous as the underlying implementation may need to make network calls that should not block the current thread, which are often network threads. This change is meant to be built upon. The basic concepts are introduced without proper documentation, plumbing to enable other AuthorizationEngine types, and some items we may want to refactor. For example, the AuthorizedIndices class is lazily loaded but this might actually be something we want to make asynchronous. We pass a lot of the same arguments to the various methods and it would be prudent to wrap these in a class; this class would provide a way for us to pass additional items needed by future enhancements without breaking the interface and requiring updates to all implementations. See elastic#32435
commit 3e60a91 Author: jaymode <[email protected]> Date: Mon Feb 4 12:34:23 2019 -0700 add licensing for authorization engine commit 1c9a8e1 Author: jaymode <[email protected]> Date: Mon Feb 4 12:17:54 2019 -0700 fix inconsistency in parameter name/type commit 34aa55a Author: Jay Modi <[email protected]> Date: Mon Feb 4 11:45:01 2019 -0700 Authorization engines evaluate privileges for APIs (elastic#38219) This commit moves the evaluation of privileges from a few transport actions into the authorization engine. The APIs are used by other applications for making decisions and if a different authorization engine is used that is not role based, we should still allow these APIs to work. By moving this evaluation out of the transport action, the transport actions no longer have a dependency on roles. commit 54d7b4c Merge: e5615d2 715e581 Author: jaymode <[email protected]> Date: Mon Feb 4 08:06:14 2019 -0700 Merge branch 'master' into security_authz_engine commit e5615d2 Author: Jay Modi <[email protected]> Date: Mon Feb 4 07:53:37 2019 -0700 Move request interceptors to AuthorizationService (elastic#38137) This change moves the RequestInterceptor iteration from the action filter to the AuthorizationService. This is done to remove the need for the use of a role within the request interceptors and replace it with the AuthorizationEngine. The AuthorizationEngine interface was also enhanced with a new method that is used to determine if a users permission on one index is a subset of their permissions on a list of indices or aliases. Additionally, this change addresses some leftover cleanups. commit 0e1c191 Merge: 3280607 b7de8e1 Author: jaymode <[email protected]> Date: Thu Jan 31 08:56:45 2019 -0700 Merge branch 'master' into security_authz_engine commit 3280607 Author: Jay Modi <[email protected]> Date: Tue Jan 29 14:17:37 2019 -0700 Allow authorization engines as an extension (elastic#37785) Authorization engines can now be registered by implementing a plugin, which also has a service implementation of a security extension. Only one extension may register an authorization engine and this engine will be used for all users except reserved realm users and internal users. commit d628008 Author: jaymode <[email protected]> Date: Tue Jan 29 10:06:09 2019 -0700 fix RBACEngine after restricted indices changes commit 5074683 Merge: 74f2e99 3c9f703 Author: jaymode <[email protected]> Date: Tue Jan 29 08:09:39 2019 -0700 Merge branch 'master' into security_authz_engine commit 74f2e99 Merge: 7846ee8 899dfc3 Author: jaymode <[email protected]> Date: Fri Jan 25 15:02:07 2019 -0700 Merge branch 'master' into security_authz_engine commit 7846ee8 Merge: b9a2c81 a81931b Author: jaymode <[email protected]> Date: Thu Jan 24 07:52:08 2019 -0700 Merge branch 'master' into security_authz_engine commit b9a2c81 Author: jaymode <[email protected]> Date: Tue Jan 22 09:48:11 2019 -0700 Fix resolving restricted indices after merging commit d98a77a Merge: 83cde40 5c1a1f7 Author: jaymode <[email protected]> Date: Tue Jan 22 09:09:23 2019 -0700 Merge branch 'master' into security_authz_engine commit 83cde40 Author: Jay Modi <[email protected]> Date: Tue Jan 22 08:03:19 2019 -0700 Add javadoc to the AuthorizationEngine interface (elastic#37620) This commit adds javadocs to the AuthorizationEngine interface aimed at developers of an authorization engine. Additionally, some classes were also moved to the core project so that they are ready to be exposed once we allow authorization engines to be plugged in. commit 9a240c6 Author: Jay Modi <[email protected]> Date: Thu Jan 17 19:33:35 2019 -0700 Encapsulate request, auth, and action name (elastic#37495) This change introduces a new class called RequestInfo that encapsulates the common objects that are passed to the authorization engine methods. By doing so, we give ourselves a way of adding additional data without breaking the interface. Additionally, this also reduces the need to ensure we pass these three parameters in the same order everywhere for consistency. commit 6278eab Merge: c555a44 4351a5e Author: jaymode <[email protected]> Date: Thu Jan 17 07:51:32 2019 -0700 Merge branch 'master' into security_authz_engine commit c555a44 Merge: 1362ab6 ecf0de3 Author: jaymode <[email protected]> Date: Wed Jan 16 10:24:33 2019 -0700 Merge branch 'master' into security_authz_engine commit 1362ab6 Author: Jay Modi <[email protected]> Date: Wed Jan 16 10:23:45 2019 -0700 Replace AuthorizedIndices class with a List (elastic#37328) This change replaces the AuthorizedIndices class with a simple list. The change to a simple list does remove the lazy loading of the authorized indices in favor of simpler code as the loading of this list is now an asynchronous operation that is delegated to the authorization engine. commit 0246442 Merge: 8ccdc19 a2a40c5 Author: jaymode <[email protected]> Date: Tue Jan 15 10:49:12 2019 -0700 Merge branch 'master' into security_authz_engine commit 8ccdc19 Author: Jay Modi <[email protected]> Date: Mon Jan 7 13:43:22 2019 -0700 Introduce asynchronous RBACEngine (elastic#36245) In order to support the concept of different authorization engines, this change begins the refactoring of the AuthorizationService to support this. Previously, the asynchronous work for authorization was performed by the AsyncAuthorizer class, but this tied the authorization service to a role based implementation. In this change, the authorize method become asynchronous and delegates much of the actual permission checking to an AuthorizationEngine. The pre-existing RBAC permission checking has been abstracted into the RBACEngine. The majority of calls to AuthorizationEngine instances are asynchronous as the underlying implementation may need to make network calls that should not block the current thread, which are often network threads. This change is meant to be built upon. The basic concepts are introduced without proper documentation, plumbing to enable other AuthorizationEngine types, and some items we may want to refactor. For example, the AuthorizedIndices class is lazily loaded but this might actually be something we want to make asynchronous. We pass a lot of the same arguments to the various methods and it would be prudent to wrap these in a class; this class would provide a way for us to pass additional items needed by future enhancements without breaking the interface and requiring updates to all implementations. See elastic#32435
For some users, the built in authorization mechanism does not fit their needs and no feature that we offer would allow them to control the authorization process to meet their needs. In order to support this, a concept of an AuthorizationEngine is being introduced, which can be provided using the security extension mechanism. An AuthorizationEngine is responsible for making the authorization decisions about a request. The engine is responsible for knowing how to authorize and can be backed by whatever mechanism a user wants. The default mechanism is one backed by roles to provide the authorization decisions. The AuthorizationEngine will be called by the AuthorizationService, which handles more of the internal workings that apply in general to authorization within Elasticsearch. In order to support external authorization services that would back an authorization engine, the entire authorization process has become asynchronous, which also includes all calls to the AuthorizationEngine. The use of roles also leaked out of the AuthorizationService in our existing code that is not specifically related to roles so this also needed to be addressed. RequestInterceptor instances sometimes used a role to ensure a user was not attempting to escalate their privileges. Addressing this leakage of roles meant that the RequestInterceptor execution needed to move within the AuthorizationService and that AuthorizationEngines needed to support detection of whether a user has more privileges on a name than another. The second area where roles leaked to the user is in the handling of a few privilege APIs that could be used to retrieve the user's privileges or ask if a user has privileges to perform an action. To remove the leakage of roles from these actions, the AuthorizationService and AuthorizationEngine gained methods that enabled an AuthorizationEngine to return the response for these APIs. Ultimately this feature is the work included in: #37785 #37495 #37328 #36245 #38137 #38219 Closes #32435
For some users, the built in authorization mechanism does not fit their needs and no feature that we offer would allow them to control the authorization process to meet their needs. In order to support this, a concept of an AuthorizationEngine is being introduced, which can be provided using the security extension mechanism. An AuthorizationEngine is responsible for making the authorization decisions about a request. The engine is responsible for knowing how to authorize and can be backed by whatever mechanism a user wants. The default mechanism is one backed by roles to provide the authorization decisions. The AuthorizationEngine will be called by the AuthorizationService, which handles more of the internal workings that apply in general to authorization within Elasticsearch. In order to support external authorization services that would back an authorization engine, the entire authorization process has become asynchronous, which also includes all calls to the AuthorizationEngine. The use of roles also leaked out of the AuthorizationService in our existing code that is not specifically related to roles so this also needed to be addressed. RequestInterceptor instances sometimes used a role to ensure a user was not attempting to escalate their privileges. Addressing this leakage of roles meant that the RequestInterceptor execution needed to move within the AuthorizationService and that AuthorizationEngines needed to support detection of whether a user has more privileges on a name than another. The second area where roles leaked to the user is in the handling of a few privilege APIs that could be used to retrieve the user's privileges or ask if a user has privileges to perform an action. To remove the leakage of roles from these actions, the AuthorizationService and AuthorizationEngine gained methods that enabled an AuthorizationEngine to return the response for these APIs. Ultimately this feature is the work included in: elastic#37785, elastic#37495, elastic#37328, elastic#36245, elastic#38137, elastic#38219 Closes elastic#32435
For some users, the built in authorization mechanism does not fit their needs and no feature that we offer would allow them to control the authorization process to meet their needs. In order to support this, a concept of an AuthorizationEngine is being introduced, which can be provided using the security extension mechanism. An AuthorizationEngine is responsible for making the authorization decisions about a request. The engine is responsible for knowing how to authorize and can be backed by whatever mechanism a user wants. The default mechanism is one backed by roles to provide the authorization decisions. The AuthorizationEngine will be called by the AuthorizationService, which handles more of the internal workings that apply in general to authorization within Elasticsearch. In order to support external authorization services that would back an authorization engine, the entire authorization process has become asynchronous, which also includes all calls to the AuthorizationEngine. The use of roles also leaked out of the AuthorizationService in our existing code that is not specifically related to roles so this also needed to be addressed. RequestInterceptor instances sometimes used a role to ensure a user was not attempting to escalate their privileges. Addressing this leakage of roles meant that the RequestInterceptor execution needed to move within the AuthorizationService and that AuthorizationEngines needed to support detection of whether a user has more privileges on a name than another. The second area where roles leaked to the user is in the handling of a few privilege APIs that could be used to retrieve the user's privileges or ask if a user has privileges to perform an action. To remove the leakage of roles from these actions, the AuthorizationService and AuthorizationEngine gained methods that enabled an AuthorizationEngine to return the response for these APIs. Ultimately this feature is the work included in: #37785, #37495, #37328, #36245, #38137, #38219 Closes #32435
In order to support the concept of different authorization engines, this
change begins the refactoring of the
AuthorizationService
to supportthis. Previously, the asynchronous work for authorization was performed
by the
AsyncAuthorizer
class, but this tied the authorization serviceto a role based implementation. In this change, the authorize method
become asynchronous and delegates much of the actual permission checking
to an
AuthorizationEngine
. The pre-existing RBAC permission checkinghas been abstracted into the
RBACEngine
. The majority of calls toAuthorizationEngine
instances are asynchronous as the underlyingimplementation may need to make network calls that should not block
the current thread, which are often network threads.
This change is meant to be built upon. The basic concepts are introduced
without proper documentation, plumbing to enable other
AuthorizationEngine
types, and some items we may want to refactor.For example, the AuthorizedIndices class is lazily loaded but this might
actually be something we want to make asynchronous. We pass a lot of the
same arguments to the various methods and it would be prudent to wrap
these in a class; this class would provide a way for us to pass
additional items needed by future enhancements without breaking the
interface and requiring updates to all implementations.
See #32435