Skip to content
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

[DRAFT] [Feature]Introduces ability to control access to and share resources #16030

Draft
wants to merge 27 commits into
base: main
Choose a base branch
from

Conversation

DarshitChanpura
Copy link
Member

@DarshitChanpura DarshitChanpura commented Sep 22, 2024

Work in Progress.

companion PR: opensearch-project/security#4746

Description

This PR introduces a new capability to enable access-control and sharing of resources. This PR introduces:

  1. Interfaces to be extended by security plugin for concrete implementation, and to be used by plugins when authorizing the requested resources.
  2. Adds a No-op implementation when security plugin is not enabled.

At present, plugins have implemented in-house authorization mechanisms to control access to their resources. This framework enables capability to have a centralized resource-authorization framework.

Please review feature proposal here that discusses the problem-statement and design approach. opensearch-project/security#4500

Plugins will leverage the APIs introduced here to check user access to resources.

To-do items:

  • Add integration tests
  • Add end-to-end tests

Documentation website will follow.

Related Issues

Check List

  • Functionality includes testing.
  • API changes companion pull request created, if applicable.
  • Public documentation issue/PR created, if applicable.

By 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.

Signed-off-by: Darshit Chanpura <[email protected]>
Signed-off-by: Darshit Chanpura <[email protected]>
Signed-off-by: Darshit Chanpura <[email protected]>

/**
* This class contains information about whom a resource is shared with.
* It could be a user-name, a role or a backend_role.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it really necessary to support all three criteria (user name, roles, backend roles)? Especially back end roles can vary between auth backend (e.g. users authenticated via LDAP might have different backend roles than users authenticated via OIDC).

Such disparities can lead to confusion.

Who is supposed to specify these criteria when?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately, backend roles is already in place amongst plugins that implement custom resource authz.

Its already in place in:

  1. ML Commons
  2. Flow Framework
  3. Anomaly Detection
  4. Alerting

I think there may be others as well (possibly Reporting?).

Backend roles would need to be supported in order for those plugins to adopt the mechanism provided by security.

Copy link

@nibix nibix Oct 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, so this has the goal of a seamless transition from the older methods used in these different plugins?

Still, from a UX point of view, having too many options for a single thing is not optimal. It requires users to make a choice which is the optimal option. Proper information on how to make the right choice might be hard to find or might even not exist.

This is actually demonstrated by the broad range of artifacts which require backend roles in their configuration.

Thus, maybe the backend role option should be marked as deprecated? Or, maybe the docs should be clearer on how to use the roles?

Signed-off-by: Darshit Chanpura <[email protected]>
Signed-off-by: Darshit Chanpura <[email protected]>
Signed-off-by: Darshit Chanpura <[email protected]>
Copy link
Contributor

github-actions bot commented Oct 2, 2024

❌ Gradle check result for fba48ab: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

Copy link
Contributor

github-actions bot commented Oct 4, 2024

❌ Gradle check result for 6a6e6f7: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

Copy link
Contributor

github-actions bot commented Oct 4, 2024

✅ Gradle check result for 566913a: SUCCESS

Copy link

codecov bot commented Oct 4, 2024

Codecov Report

Attention: Patch coverage is 8.46154% with 119 lines in your changes missing coverage. Please review.

Project coverage is 71.94%. Comparing base (691f725) to head (e313071).
Report is 112 commits behind head on main.

Files with missing lines Patch % Lines
...earch/accesscontrol/resources/SharedWithScope.java 0.00% 43 Missing ⚠️
...earch/accesscontrol/resources/ResourceSharing.java 0.00% 33 Missing ⚠️
.../opensearch/accesscontrol/resources/ShareWith.java 0.00% 13 Missing ⚠️
.../opensearch/accesscontrol/resources/CreatedBy.java 0.00% 12 Missing ⚠️
...earch/accesscontrol/resources/ResourceService.java 42.85% 7 Missing and 1 partial ⚠️
...earch/plugins/NoOpResourceAccessControlPlugin.java 14.28% 6 Missing ⚠️
...opensearch/accesscontrol/resources/EntityType.java 0.00% 4 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##               main   #16030      +/-   ##
============================================
- Coverage     72.06%   71.94%   -0.13%     
+ Complexity    64822    64771      -51     
============================================
  Files          5308     5315       +7     
  Lines        302574   302710     +136     
  Branches      43710    43720      +10     
============================================
- Hits         218048   217780     -268     
- Misses        66648    67074     +426     
+ Partials      17878    17856      -22     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

❌ Gradle check result for 0eb47ac: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

Copy link
Contributor

✅ Gradle check result for e313071: SUCCESS

@opensearch-trigger-bot
Copy link
Contributor

This PR is stalled because it has been open for 30 days with no activity.

@opensearch-trigger-bot opensearch-trigger-bot bot added stalled Issues that have stalled and removed stalled Issues that have stalled labels Nov 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants