Feature activation for multi-tenancy #2568

peternied · 2023-03-19T19:24:01Z

Description

Reusing saveAnUpdateConfigs to perform specific updates on the multitenancy_enabled configuration flag.

New APIs

GET /_plugins/_security/config/tenancy/multitenancy_enabled
PUT /_plugins/_security/config/tenancy/multitenancy_enabled

New Permissions

securityconfig:admin/config/tenancy/multitenancy_enabled/read
securityconfig:admin/config/tenancy/multitenancy_enabled/update

Issues Resolved

Related [FEATURE] Enable/Disable Multi-Tenancy, Private Tenant and set Default Tenant in OS Dashboards. security-dashboards-plugin#1302
Related Dynamically Enable/Disable multi-tenancy, private tenant and set Defa… #2444

Testing

New integration tests have been included that exercise the new actions functionality.

Check List

New functionality includes testing
New functionality has been documented
Commits are signed per the DCO using --signoff

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.

Reusing saveAnUpdateConfigs to perform specific updates on the `multitenancy_enabled` configuration flag. New APIs - GET /_plugins/_security/config/tenancy/multitenancy_enabled - PUT /_plugins/_security/config/tenancy/multitenancy_enabled New Permissions - securityconfig:admin/config/tenancy/multitenancy_enabled/read - securityconfig:admin/config/tenancy/multitenancy_enabled/update Signed-off-by: Peter Nied <[email protected]>

peternied

Added some inline comments if we look to move forward with this as dynamic configuration via a 'feature activate' pattern.

peternied · 2023-03-19T19:25:36Z

src/main/java/org/opensearch/security/action/tenancy/BooleanSettingRetrieveResponse.java

+import org.opensearch.core.xcontent.ToXContentObject;
+import org.opensearch.core.xcontent.XContentBuilder;
+
+public class BooleanSettingRetrieveResponse extends ActionResponse implements ToXContentObject {


If there are additional actions that reuse this pattern { "value": {VALUE} } these names might need to be made more generic and moved outside of the tenancy package. The same is true for the Request types as well.

src/main/java/org/opensearch/security/dlic/rest/api/AbstractApiAction.java

peternied · 2023-03-19T19:27:15Z

src/test/java/org/opensearch/security/test/helper/rest/RestHelper.java

@@ -92,8 +92,6 @@
 import org.opensearch.security.test.helper.cluster.ClusterInfo;
 import org.opensearch.security.test.helper.file.FileHelper;

-import static org.junit.jupiter.api.Assertions.fail;


I was getting missing class exception when this assertion was triggered at runtime.

Interesting. I checked out your branch and restored the original code and it worked fine for me. ./gradlew test --tests TenancyActionsTests

Package Tests Failures Ignored Duration Success rate

org.opensearch.security.multitenancy.test 2 0 0 12.698s 100%

src/main/java/org/opensearch/security/action/tenancy/MultiTenancyUpdateTransportAction.java

Signed-off-by: Peter Nied <[email protected]>

DarshitChanpura

Nice work @peternied !

src/main/java/org/opensearch/security/OpenSearchSecurityPlugin.java

src/main/java/org/opensearch/security/action/tenancy/EmptyRequest.java

DarshitChanpura · 2023-03-20T15:14:44Z

src/main/java/org/opensearch/security/action/tenancy/MultiTenancyRetrieveAction.java

+public class MultiTenancyRetrieveAction extends ActionType<BooleanSettingRetrieveResponse> {
+
+    public static final MultiTenancyRetrieveAction INSTANCE = new MultiTenancyRetrieveAction();
+    public static final String NAME = "securityconfig:admin/config/tenancy/multitenancy_enabled/read";


What do you think about using cluster:admin/config/tenancy/multitenancy_enabled/read?
This is currently being used in ConfigUpdateAction.java

security/src/main/java/org/opensearch/security/action/configupdate/ConfigUpdateAction.java

Line 34 in 575c2bc

public static final String NAME = "cluster:admin/opendistro_security/config/update";

I was thinking about making a new prefix instead of 'cluster'. What would you think about feature:cluster/tenancy/multitenancy_enabled/(read|update) instead?

I'd like it to be unassociated from where the setting is stored, it should be abstract enough it could move to another plugin, it should indicate the scope (e.g. cluster wide!), and then the local namespace tenancy followed by feature name.

What do you think, other recommendations?

What do you think about swapping the order of feature:cluster to be cluster:feature? Unless there is a reason to put feature first, I am generally in favor of broadest-->granlur(est) ordering for the route. What do you think?

I like the idea of cluster:feature/... This should be clear in reading and will identify the scopes in the decreasing order.

I've updated it to cluster:feature/tenancy/multitenancy_enabled/update - but it seems like we should also update the rest request url to be inline with this, otherwise the coupling is still in place.

src/main/java/org/opensearch/security/action/tenancy/MultiTenancyRetrieveTransportAction.java

DarshitChanpura · 2023-03-20T15:18:29Z

src/main/java/org/opensearch/security/action/tenancy/MultiTenancyUpdateAction.java

+public class MultiTenancyUpdateAction extends ActionType<BooleanSettingRetrieveResponse> {
+
+    public static final MultiTenancyUpdateAction INSTANCE = new MultiTenancyUpdateAction();
+    public static final String NAME = "securityconfig:admin/config/tenancy/multitenancy_enabled/update";


What do you think about using cluster:admin/config/tenancy/multitenancy_enabled/update?
This is currently being used in ConfigUpdateAction.java

security/src/main/java/org/opensearch/security/action/configupdate/ConfigUpdateAction.java

Line 34 in 575c2bc

public static final String NAME = "cluster:admin/opendistro_security/config/update";

The point of these new actions is they can be permissioned separately from the security config, if I make them the same this removes its benefit, see the related issue for details

[FEATURE] Create a way to add action oriented APIs to update configuration values #2559

src/main/java/org/opensearch/security/dlic/rest/api/AbstractApiAction.java

src/test/java/org/opensearch/security/multitenancy/test/TenancyActionsTests.java

DarshitChanpura · 2023-03-20T15:55:34Z

src/test/java/org/opensearch/security/test/helper/rest/RestHelper.java

@@ -92,8 +92,6 @@
 import org.opensearch.security.test.helper.cluster.ClusterInfo;
 import org.opensearch.security.test.helper.file.FileHelper;

-import static org.junit.jupiter.api.Assertions.fail;


Interesting. I checked out your branch and restored the original code and it worked fine for me. ./gradlew test --tests TenancyActionsTests

Package Tests Failures Ignored Duration Success rate

org.opensearch.security.multitenancy.test 2 0 0 12.698s 100%

stephen-crawford

Overall I think this looks good.

stephen-crawford · 2023-03-20T17:50:44Z

src/main/java/org/opensearch/security/action/tenancy/MultiTenancyRetrieveAction.java

+public class MultiTenancyRetrieveAction extends ActionType<BooleanSettingRetrieveResponse> {
+
+    public static final MultiTenancyRetrieveAction INSTANCE = new MultiTenancyRetrieveAction();
+    public static final String NAME = "securityconfig:admin/config/tenancy/multitenancy_enabled/read";


What do you think about swapping the order of feature:cluster to be cluster:feature? Unless there is a reason to put feature first, I am generally in favor of broadest-->granlur(est) ordering for the route. What do you think?

src/main/java/org/opensearch/security/action/tenancy/MultiTenancyRetrieveTransportAction.java

cwperks · 2023-03-20T20:19:54Z

@peternied IMO An action like securityconfig:admin/config/tenancy/multitenancy_enabled/update is overly narrow and won't scale well for all of the possible values that could be placed within config.yml.

What do you think about going one step up to say securityconfig:admin/config/opensearch_dashboards/edit which would allow the user of the API to PATCH the whole dynamic OSD config or PATCH a specific value using the same pattern as the PATCH configuration endpoint: https://opensearch.org/docs/2.6/security/access-control/api/#patch-configuration

Something like

PATCH /_plugins/_security/api/securityconfig/opensearch_dashboards
[
  {
    "op": "replace", "path": "/config/dynamic/kibana/multitenancy_enabled", "value": "true"
  }
]

And then this endpoint can be supported without having plugins.security.unsupported.restapi.allow_securityconfig_modification: true setting in opensearch.yml

Signed-off-by: Peter Nied <[email protected]>

codecov-commenter · 2023-03-20T22:35:56Z

Codecov Report

Merging #2568 (d861441) into main (ca4d752) will increase coverage by 0.04%.
The diff coverage is 73.33%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@             Coverage Diff              @@
##               main    #2568      +/-   ##
============================================
+ Coverage     61.28%   61.32%   +0.04%     
- Complexity     3333     3360      +27     
============================================
  Files           260      268       +8     
  Lines         18509    18598      +89     
  Branches       3269     3269              
============================================
+ Hits          11344    11406      +62     
- Misses         5573     5594      +21     
- Partials       1592     1598       +6

Impacted Files	Coverage Δ
...earch/security/dlic/rest/api/MigrateApiAction.java	`4.30% <0.00%> (ø)`
...action/tenancy/BooleanSettingRetrieveResponse.java	`57.14% <57.14%> (ø)`
...ensearch/security/action/tenancy/EmptyRequest.java	`60.00% <60.00%> (ø)`
...ty/action/tenancy/BooleanSettingUpdateRequest.java	`61.11% <61.11%> (ø)`
...ion/tenancy/MultiTenancyUpdateTransportAction.java	`76.00% <76.00%> (ø)`
...curity/action/tenancy/MutliTenancyRestHandler.java	`77.77% <77.77%> (ø)`
.../opensearch/security/OpenSearchSecurityPlugin.java	`80.00% <100.00%> (+0.12%)`	⬆️
...ity/action/tenancy/MultiTenancyRetrieveAction.java	`100.00% <100.00%> (ø)`
...n/tenancy/MultiTenancyRetrieveTransportAction.java	`100.00% <100.00%> (ø)`
...urity/action/tenancy/MultiTenancyUpdateAction.java	`100.00% <100.00%> (ø)`
... and 6 more

... and 7 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

peternied · 2023-03-21T16:07:47Z

IMO An action like securityconfig:admin/config/tenancy/multitenancy_enabled/update is overly narrow and won't scale well for all of the possible values that could be placed within config.yml.

Thanks for the comment @cwperks - I do not think every configuration setting should be exposed this way; however, this granularity allows for control over a subset of the values around the tenancy feature settings.

Circling back to the original problem from #2444 the OpenSearch Managed Service does not provide access to modify the security configuration by its users. So when there is a value that is controlled by that configuration that could be allowed there is no way to expose access to it. In PR #2444 the multitenancy_enabled value is duplicated and synchronized.

This draft pull request was created to granularize the access to that setting so it could be modified by users that are not running as super admin.

What do you think about ... PATCH!

I think this is another approach that could be taken on to support the user scenarios for #2444. However, I didn't create a POC around this scenario for a couple of reasons:

Lack of Abstraction, There is a strong desire to decouple tenancy setup/configuration from the security plugin. Reusing security API features will effectively need to be deprecated and migration will need to be undertaken vs a new abstracted API for enabling/disabling the feature could avoid this user migration even if the underlying APIs shift to a new plugin/extension.
Security Risks, Limiting the write/read surface area of a tenancy-focused patch request needs solid error checking. Path traversal or other kinds of actions could result in a CVE - a pretty urgent one. While this is likely very doable I didn't want to risk it for a feature that is targeting the next release.
No Authorization in SecurityConfigAction, the existing action is built off of AbstractApiAction which is only in REST Layer. We would need to build a new AuthZ system/connector to use the same kinds of permissions that are assignable via role/role mapping."

peternied · 2023-03-23T18:03:35Z

@abhivka7 I am not planning on iterating on this pull request any more. There are a couple of outstanding items - I think you could pull this draft PR in to your change in #2444 to get unblocked or maybe pivot with some of he ideas here. Feel free to @mention me if you have questions.

peternied · 2023-04-01T14:02:41Z

Closing out this reference pull request, thanks everyone for the discussion and review!

peternied commented Mar 19, 2023

View reviewed changes

This was referenced Mar 19, 2023

[FEATURE] Create a way to add action oriented APIs to update configuration values #2559

Closed

Dynamically Enable/Disable multi-tenancy, private tenant and set Defa… #2444

Closed

CI Fixes

79fac2f

Signed-off-by: Peter Nied <[email protected]>

DarshitChanpura reviewed Mar 20, 2023

View reviewed changes

stephen-crawford previously approved these changes Mar 20, 2023

View reviewed changes

PR Feedback

d861441

Signed-off-by: Peter Nied <[email protected]>

peternied dismissed stephen-crawford’s stale review via d861441 March 20, 2023 21:57

peternied closed this Apr 1, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature activation for multi-tenancy #2568

Feature activation for multi-tenancy #2568

peternied commented Mar 19, 2023

peternied left a comment

peternied Mar 19, 2023

peternied Mar 19, 2023

DarshitChanpura Mar 20, 2023

DarshitChanpura left a comment

DarshitChanpura Mar 20, 2023

peternied Mar 20, 2023 •

edited

Loading

stephen-crawford Mar 20, 2023

DarshitChanpura Mar 22, 2023

peternied Mar 22, 2023

DarshitChanpura Mar 20, 2023

peternied Mar 20, 2023

DarshitChanpura Mar 20, 2023

stephen-crawford left a comment

stephen-crawford Mar 20, 2023

cwperks commented Mar 20, 2023

codecov-commenter commented Mar 20, 2023 •

edited

Loading

peternied commented Mar 21, 2023

peternied commented Mar 23, 2023

peternied commented Apr 1, 2023

Feature activation for multi-tenancy #2568

Feature activation for multi-tenancy #2568

Conversation

peternied commented Mar 19, 2023

Description

Issues Resolved

Testing

Check List

peternied left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DarshitChanpura left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

peternied Mar 20, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stephen-crawford left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cwperks commented Mar 20, 2023

codecov-commenter commented Mar 20, 2023 • edited Loading

Codecov Report

peternied commented Mar 21, 2023

peternied commented Mar 23, 2023

peternied commented Apr 1, 2023

peternied Mar 20, 2023 •

edited

Loading

codecov-commenter commented Mar 20, 2023 •

edited

Loading