-
Notifications
You must be signed in to change notification settings - Fork 336
[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 <[email protected]>
Signed-off-by: Darshit Chanpura <[email protected]>
Signed-off-by: Darshit Chanpura <[email protected]>
…ource access Signed-off-by: Darshit Chanpura <[email protected]>
Signed-off-by: Darshit Chanpura <[email protected]>
… framework Signed-off-by: Darshit Chanpura <[email protected]>
Signed-off-by: Darshit Chanpura <[email protected]>
Signed-off-by: Darshit Chanpura <[email protected]>
Signed-off-by: Darshit Chanpura <[email protected]>
Signed-off-by: Darshit Chanpura <[email protected]>
Signed-off-by: Darshit Chanpura <[email protected]>
Signed-off-by: Darshit Chanpura <[email protected]>
Signed-off-by: Darshit Chanpura <[email protected]>
8c7eb57
to
f636120
Compare
Signed-off-by: Darshit Chanpura <[email protected]>
Signed-off-by: Darshit Chanpura <[email protected]>
Signed-off-by: Darshit Chanpura <[email protected]>
…abled scenarios Signed-off-by: Darshit Chanpura <[email protected]>
Signed-off-by: Darshit Chanpura <[email protected]>
Signed-off-by: Darshit Chanpura <[email protected]>
Signed-off-by: Darshit Chanpura <[email protected]>
Signed-off-by: Darshit Chanpura <[email protected]>
Signed-off-by: Darshit Chanpura <[email protected]>
Signed-off-by: Darshit Chanpura <[email protected]>
Signed-off-by: Darshit Chanpura <[email protected]>
Signed-off-by: Darshit Chanpura <[email protected]>
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 <[email protected]>
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 <[email protected]>
… sec filter Signed-off-by: Darshit Chanpura <[email protected]>
src/main/java/org/opensearch/security/transport/SecurityRequestHandler.java
Outdated
Show resolved
Hide resolved
Signed-off-by: Darshit Chanpura <[email protected]>
Signed-off-by: Darshit Chanpura <[email protected]>
Signed-off-by: Darshit Chanpura <[email protected]>
Signed-off-by: Darshit Chanpura <[email protected]>
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 <[email protected]>
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:
verifyAccess
from 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.