-
Couldn't load subscription status.
- Fork 337
[Resource Sharing] Adds a Resource Access Evaluator for standalone Resource access authorization #5408
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
[Resource Sharing] Adds a Resource Access Evaluator for standalone Resource access authorization #5408
Conversation
Signed-off-by: Darshit Chanpura <dchanp@amazon.com>
Signed-off-by: Darshit Chanpura <dchanp@amazon.com>
Signed-off-by: Darshit Chanpura <dchanp@amazon.com>
…ource access Signed-off-by: Darshit Chanpura <dchanp@amazon.com>
Signed-off-by: Darshit Chanpura <dchanp@amazon.com>
… framework Signed-off-by: Darshit Chanpura <dchanp@amazon.com>
Signed-off-by: Darshit Chanpura <dchanp@amazon.com>
Signed-off-by: Darshit Chanpura <dchanp@amazon.com>
Signed-off-by: Darshit Chanpura <dchanp@amazon.com>
Signed-off-by: Darshit Chanpura <dchanp@amazon.com>
Signed-off-by: Darshit Chanpura <dchanp@amazon.com>
Signed-off-by: Darshit Chanpura <dchanp@amazon.com>
Signed-off-by: Darshit Chanpura <dchanp@amazon.com>
8c7eb57 to
f636120
Compare
Signed-off-by: Darshit Chanpura <dchanp@amazon.com>
Signed-off-by: Darshit Chanpura <dchanp@amazon.com>
Signed-off-by: Darshit Chanpura <dchanp@amazon.com>
…abled scenarios Signed-off-by: Darshit Chanpura <dchanp@amazon.com>
Signed-off-by: Darshit Chanpura <dchanp@amazon.com>
Signed-off-by: Darshit Chanpura <dchanp@amazon.com>
Signed-off-by: Darshit Chanpura <dchanp@amazon.com>
Signed-off-by: Darshit Chanpura <dchanp@amazon.com>
Signed-off-by: Darshit Chanpura <dchanp@amazon.com>
Signed-off-by: Darshit Chanpura <dchanp@amazon.com>
Signed-off-by: Darshit Chanpura <dchanp@amazon.com>
Signed-off-by: Darshit Chanpura <dchanp@amazon.com>
70f09f1 to
bc312cc
Compare
src/main/java/org/opensearch/security/privileges/ResourceAccessEvaluator.java
Outdated
Show resolved
Hide resolved
src/main/java/org/opensearch/security/privileges/ResourceAccessEvaluator.java
Show resolved
Hide resolved
src/main/java/org/opensearch/security/resources/ResourceSharingIndexHandler.java
Outdated
Show resolved
Hide resolved
src/main/java/org/opensearch/security/resources/ResourceSharingIndexHandler.java
Outdated
Show resolved
Hide resolved
src/main/java/org/opensearch/security/resources/ResourceAccessHandler.java
Show resolved
Hide resolved
Signed-off-by: Darshit Chanpura <dchanp@amazon.com>
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.
The async handling for resource privilege evaluation looks good! 👍
I am a bit surprised by the additional ThreadContext header introduced in this PR. It feels a bit unrelated. This will be enabled independent of the feature flag, correct? As this will significantly increase the request size for each request, this is a bit critical, IMHO.
...ce-plugin/src/integrationTest/java/org/opensearch/sample/resource/SecurityDisabledTests.java
Outdated
Show resolved
Hide resolved
...ain/java/org/opensearch/sample/resource/actions/rest/revoke/RevokeResourceAccessRequest.java
Show resolved
Hide resolved
src/main/java/org/opensearch/security/resources/ResourceAccessHandler.java
Show resolved
Hide resolved
src/main/java/org/opensearch/security/transport/SecurityInterceptor.java
Show resolved
Hide resolved
… from user header Signed-off-by: Darshit Chanpura <dchanp@amazon.com>
… sec filter Signed-off-by: Darshit Chanpura <dchanp@amazon.com>
src/main/java/org/opensearch/security/transport/SecurityRequestHandler.java
Outdated
Show resolved
Hide resolved
Signed-off-by: Darshit Chanpura <dchanp@amazon.com>
Signed-off-by: Darshit Chanpura <dchanp@amazon.com>
Signed-off-by: Darshit Chanpura <dchanp@amazon.com>
Signed-off-by: Darshit Chanpura <dchanp@amazon.com>
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!
Approving. Still, I think we need to put a bit more thought into the serialization handling of the UserSubject; IMHO, it has quite a bit potential for inconsistencies at the moment:
security/src/main/java/org/opensearch/security/transport/SecurityRequestHandler.java
Lines 179 to 239 in 5d41f95
| String authUsrHdr = getThreadContext().getHeader(ConfigConstants.OPENDISTRO_SECURITY_AUTHENTICATED_USER_HEADER); | |
| String shouldUseUserHeader = getThreadContext().getHeader(ConfigConstants.OPENDISTRO_SECURITY_USER_SAME_AS_SUBJECT_HEADER); | |
| String userHeader = getThreadContext().getHeader(ConfigConstants.OPENDISTRO_SECURITY_USER_HEADER); | |
| User user = null; | |
| // restore a persistent user-subject from subject header | |
| if (getThreadContext().getPersistent(ConfigConstants.OPENDISTRO_SECURITY_AUTHENTICATED_USER) == null) { | |
| // when auth subject user is same request user. | |
| if (Boolean.parseBoolean(shouldUseUserHeader) && userHeader != null) { | |
| user = this.userFactory.fromSerializedBase64(userHeader); | |
| getThreadContext().putPersistent( | |
| ConfigConstants.OPENDISTRO_SECURITY_AUTHENTICATED_USER, | |
| new UserSubjectImpl(getThreadPool(), user) | |
| ); | |
| } else if (authUsrHdr != null) { | |
| User authUser = this.userFactory.fromSerializedBase64(authUsrHdr); | |
| getThreadContext().putPersistent( | |
| ConfigConstants.OPENDISTRO_SECURITY_AUTHENTICATED_USER, | |
| new UserSubjectImpl(getThreadPool(), authUser) | |
| ); | |
| } | |
| } | |
| final String injectedRolesHeader = getThreadContext().getHeader(ConfigConstants.OPENDISTRO_SECURITY_INJECTED_ROLES_HEADER); | |
| final String injectedUserHeader = getThreadContext().getHeader(ConfigConstants.OPENDISTRO_SECURITY_INJECTED_USER_HEADER); | |
| if (Strings.isNullOrEmpty(userHeader)) { | |
| // Keeping role injection with higher priority as plugins under OpenSearch will be using this | |
| // on transport layer | |
| if (!Strings.isNullOrEmpty(injectedRolesHeader)) { | |
| getThreadContext().putTransient(ConfigConstants.OPENDISTRO_SECURITY_INJECTED_ROLES, injectedRolesHeader); | |
| } else if (!Strings.isNullOrEmpty(injectedUserHeader)) { | |
| getThreadContext().putTransient(ConfigConstants.OPENDISTRO_SECURITY_INJECTED_USER, injectedUserHeader); | |
| } | |
| } else { | |
| user = user != null ? user : this.userFactory.fromSerializedBase64(userHeader); | |
| getThreadContext().putTransient( | |
| ConfigConstants.OPENDISTRO_SECURITY_USER, | |
| user | |
| ); | |
| } | |
| String originalRemoteAddress = getThreadContext().getHeader(ConfigConstants.OPENDISTRO_SECURITY_REMOTE_ADDRESS_HEADER); | |
| if (!Strings.isNullOrEmpty(originalRemoteAddress)) { | |
| getThreadContext().putTransient( | |
| ConfigConstants.OPENDISTRO_SECURITY_REMOTE_ADDRESS, | |
| new TransportAddress((InetSocketAddress) Base64Helper.deserializeObject(originalRemoteAddress)) | |
| ); | |
| } else { | |
| getThreadContext().putTransient(ConfigConstants.OPENDISTRO_SECURITY_REMOTE_ADDRESS, request.remoteAddress()); | |
| } | |
| final String rolesValidation = getThreadContext().getHeader( | |
| ConfigConstants.OPENDISTRO_SECURITY_INJECTED_ROLES_VALIDATION_HEADER | |
| ); | |
| if (!Strings.isNullOrEmpty(rolesValidation)) { | |
| getThreadContext().putTransient(ConfigConstants.OPENDISTRO_SECURITY_INJECTED_ROLES_VALIDATION, rolesValidation); | |
| } |
But as this is not actually the central point of this PR, this can and should be handled separately.
Signed-off-by: Darshit Chanpura <dchanp@amazon.com>
Description
This PR adds a new privilege evaluator for evaluating access to a resource. #5281 introduced a way for plugin offload sharing and access evaluation to security plugin but that was done by requiring plugins to call verifyAccess method on their end. This leaves room for error. This new evaluator will filter all resource access requests through SecurityFilter class without requiring plugins to explicitly call verifyAccess method. It also adds support for access-levels instead of just the default one declared in the previous PR.
Notes:
verifyAccessfrom the client as plugin no longer have to explicitly call the method to check user access.Issues Resolved
#5442
Testing
Check List
- [ ] New Roles/Permissions have a corresponding security dashboards plugin PR- [ ] API changes companion pull request createdBy submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.