Fix multitency config update #2758

willyborankin · 2023-05-10T11:12:16Z

Description

The aim of this PR is to fix 2 issues with muti-tenancy:

Fixed for the multi-tenancy configuratiuon update. The current implementation uses "yet another" approach, it updates the configuration on each node and since the security index has a dedicated listener which sends new configuration on each node such action updates configuration on each node multiple times, which leads to timeouts.
Fixed who can change multi-tanency configuratuion. The current implementation gives access to update configuration for all users full cluster persmissions, while it is possible to do via REST API only if user is a "super-admin or it was assigned to the security_rest_api_access role.

Issues Resolved

Closes #2769.

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.

codecov-commenter · 2023-05-10T11:41:36Z

Codecov Report

Merging #2758 (b7ed864) into main (7c4e06d) will increase coverage by 0.03%.
The diff coverage is 89.33%.

@@             Coverage Diff              @@
##               main    #2758      +/-   ##
============================================
+ Coverage     61.31%   61.34%   +0.03%     
+ Complexity     3408     3386      -22     
============================================
  Files           272      265       -7     
  Lines         18846    18789      -57     
  Branches       3295     3296       +1     
============================================
- Hits          11555    11527      -28     
+ Misses         5690     5664      -26     
+ Partials       1601     1598       -3

Impacted Files	Coverage Δ
.../opensearch/security/OpenSearchSecurityPlugin.java	`80.03% <ø> (-0.12%)`	⬇️
...g/opensearch/security/support/ConfigConstants.java	`94.44% <ø> (ø)`
...ity/dlic/rest/api/MultiTenancyConfigApiAction.java	`88.23% <88.23%> (ø)`
...security/dlic/rest/api/SecurityRestApiActions.java	`95.23% <100.00%> (+0.23%)`	⬆️
...c/rest/validation/MultiTenancyConfigValidator.java	`100.00% <100.00%> (ø)`

... and 4 files with indirect coverage changes

RyanL1997

Hi @willyborankin , thank you for putting this together. This PR looks good, and especially for the testing coverage! I left some minor questions/reviews just for my understanding.

RyanL1997 · 2023-05-10T18:47:52Z

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

+
+    private final static Set<String> ACCEPTABLE_DEFAULT_TENANTS = ImmutableSet.of(
+            ConfigConstants.TENANCY_GLOBAL_TENANT_DEFAULT_NAME,
+            ConfigConstants.TENANCY_GLOBAL_TENANT_NAME,


Are the L63 and L64 are the 2 case sensitive name for global tenants? Like 'global' vs 'Global'?

AFAIU they are not not, will change and add equalsIgnoreCase

RyanL1997 · 2023-05-10T20:43:06Z

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

-    private final Header asUser = encodeBasicHeader("kirk", "kirk");
-    private final Header onUserTenant = new BasicHeader("securitytenant", "__user__");
+    private static final Header AS_REST_API_USER = encodeBasicHeader("rest_api_access", "rest_api_access");
+    private static final Header AS_ADMIN_USER = encodeBasicHeader("admin", "admin");


Just for my information, is there any reason for renaming this? Cuz I saw the asUser still remains the same.

Yes since configuration can be changed only by user which has access to REST API (I just want to highlight it)

RyanL1997 · 2023-05-10T20:52:19Z

src/test/resources/multitenancy/internal_users.yml

@@ -156,3 +156,9 @@ user_tenant_parameters_substitution:
  attributes:
    attribute1: "tenant_parameters_substitution"
  description: "PR# 819 / Issue#817"
+rest_api_access:


Nit: probably user_rest_api_access?

RyanL1997 · 2023-05-10T21:05:44Z

src/test/java/org/opensearch/security/dlic/rest/api/MultiTenancyConfigApiTest.java

+public class MultiTenancyConfigApiTest extends AbstractRestApiUnitTest {
+
+    private static final Header ADMIN_FULL_ACCESS_USER = encodeBasicHeader("admin_all_access", "admin_all_access");
+    private static final Header USER_NO_REST_API_ACCESS = encodeBasicHeader("admin", "admin");


Is this means that it is the admin user but without the all_access or only the rest_api_access?

Yes by default admin has full access to the cluster and indices but not to the REST API.

cwperks

Thank you for the contribution @willyborankin! This approach brings this API in line with other security APIs and reduces the amount of code for this feature.

cwperks · 2023-05-11T12:56:36Z

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

+import static org.opensearch.security.dlic.rest.support.Utils.addRoutesPrefix;
+
+
+public class MultiTenancyConfigApiAction extends AbstractApiAction {


This greatly simplifies this down to a single class that extends AbstractApiAction. Great work!

cwperks · 2023-05-11T12:57:35Z

src/test/java/org/opensearch/security/dlic/rest/api/MultiTenancyConfigApiTest.java

+    }
+
+    @Test
+    public void testForbiddenAccess() throws Exception {


Thank you for adding this test. I agree that this should behave like all of the other security admin APIs.

cwperks · 2023-05-11T13:01:39Z

src/test/resources/multitenancy/roles.yml

@@ -2,6 +2,8 @@
 _meta:
  type: "roles"
  config_version: 2
+security_rest_api_access:


For anyone wondering what's special about this role and how it gets restapi access, its one of the roles specified in the demo configuration for restapi access here: https://github.com/opensearch-project/security/blob/main/tools/install_demo_configuration.sh#L384

A small addition from my side.
There are 3 types of admins in the plugin:

super admin - has access to everything e.g. can change configuration like SSL reload, update config on each node, CRUD operations for nodes ds etc. This is leftovers from the past idea to have a dedicated node admin

admin - has full access to cluster and indices

REST admin API - "admin" which has access to change some plugin configuration like roles, roles mapping, internal users etc.

compare to other security frameworks the plugin idea is very very confusing and it is hard to support such config.

DarshitChanpura

Nicely done @willyborankin! This is a great improvement, readability and maintenance wise!

DarshitChanpura · 2023-05-11T17:23:13Z

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

-    private final Header asAdminUser = encodeBasicHeader("admin", "admin");
-    private final Header asUser = encodeBasicHeader("kirk", "kirk");
-    private final Header onUserTenant = new BasicHeader("securitytenant", "__user__");
+    private static final Header AS_REST_API_USER = encodeBasicHeader("user_rest_api_access", "user_rest_api_access");


nicely done!

DarshitChanpura · 2023-05-11T17:27:48Z

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

        assertThat(searchInUserTenantWithPrivateTenantEnabled.getStatusCode(), equalTo(HttpStatus.SC_OK));
        assertThat(searchInUserTenantWithPrivateTenantEnabled.findValueInJson("hits.hits[0]._source.index-pattern.title"), equalTo("userIndex"));

-        final HttpResponse disablePrivateTenantResponse = nonSslRestHelper().executePutRequest("/_plugins/_security/api/tenancy/config", "{\"private_tenant_enabled\": \"false\"}", asAdminUser);
+        final HttpResponse disablePrivateTenantResponse = nonSslRestHelper().executePutRequest("/_plugins/_security/api/tenancy/config", "{\"private_tenant_enabled\": \"false\"}", AS_REST_API_USER);
        assertThat(disablePrivateTenantResponse.getStatusCode(), equalTo(HttpStatus.SC_OK));
        assertThat(disablePrivateTenantResponse.findValueInJson("private_tenant_enabled"), equalTo("false"));



Should this test case also be added here:

final HttpResponse updateSettingResponse = nonSslRestHelper().executePutRequest("/_plugins/_security/api/tenancy/config", "{\"private_tenant_enabled\":false}", AS_USER); assertThat(updateSettingResponse.getStatusCode(), equalTo(HttpStatus.SC_BAD_REQUEST)); assertThat(updateSettingResponse.findValueInJson("error.reason"), containsString("Private tenant can not be disabled if it is the default tenant."));

It is end to end test and it tests what index dashboard should use for the auth use.

Moved multi-tenancy to REST API implementation Signed-off-by: Andrey Pleskach <ples@aiven.io>

willyborankin · 2023-05-15T12:07:47Z

all tests now passed :-)

stephen-crawford

Thank you for taking the time to complete this. Looks good!

opensearch-trigger-bot · 2023-05-15T20:05:48Z

The backport to 2.7 failed:

The process '/usr/bin/git' failed with exit code 1

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-2.7 2.7
# Navigate to the new working tree
cd .worktrees/backport-2.7
# Create a new branch
git switch --create backport/backport-2758-to-2.7
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 94db5dbb0e2351b6d2b31b2a0969863acbad9ebe
# Push it to GitHub
git push --set-upstream origin backport/backport-2758-to-2.7
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-2.7

Then, create a pull request where the base branch is 2.7 and the compare/head branch is backport/backport-2758-to-2.7.

opensearch-trigger-bot · 2023-05-15T20:05:50Z

The backport to 2.x failed:

The process '/usr/bin/git' failed with exit code 1

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-2.x 2.x
# Navigate to the new working tree
cd .worktrees/backport-2.x
# Create a new branch
git switch --create backport/backport-2758-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 94db5dbb0e2351b6d2b31b2a0969863acbad9ebe
# Push it to GitHub
git push --set-upstream origin backport/backport-2758-to-2.x
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-2.x

Then, create a pull request where the base branch is 2.x and the compare/head branch is backport/backport-2758-to-2.x.

RyanL1997 · 2023-05-15T20:29:12Z

Since the auto bacporting failed, I will manually backport this.

stephen-crawford · 2023-05-16T16:26:51Z

@RyanL1997, did you get a chance to manually backport this to 2.x yet? If not I can take care of it.

Moved multi-tenancy to REST API implementation Signed-off-by: Andrey Pleskach <ples@aiven.io>

…s, and multi tenancy changes (#2737) * [Extensions] Generate auth tokens for service accounts (#2716) * Generate auth tokens for service accounts Signed-off-by: Stephen Crawford <steecraw@amazon.com> Signed-off-by: Stephen Crawford <65832608+scrawfor99@users.noreply.github.com> * Security User Refactor (#2594) --------- Signed-off-by: Stephen Crawford <steecraw@amazon.com> Signed-off-by: Stephen Crawford <65832608+scrawfor99@users.noreply.github.com> * Backport service account changes Signed-off-by: Stephen Crawford <steecraw@amazon.com> * Update test Signed-off-by: Stephen Crawford <steecraw@amazon.com> * Optimize imports Signed-off-by: Stephen Crawford <steecraw@amazon.com> * Spotless Signed-off-by: Stephen Crawford <steecraw@amazon.com> * fix plugin Signed-off-by: Stephen Crawford <steecraw@amazon.com> * fix whitespace Signed-off-by: Stephen Crawford <steecraw@amazon.com> * Fix multitency config update (#2758) Moved multi-tenancy to REST API implementation Signed-off-by: Andrey Pleskach <ples@aiven.io> * Remove SSLCertsAction Signed-off-by: Stephen Crawford <steecraw@amazon.com> * Fix dependency Signed-off-by: Stephen Crawford <steecraw@amazon.com> * fix tenancy tests Signed-off-by: Stephen Crawford <steecraw@amazon.com> --------- Signed-off-by: Stephen Crawford <steecraw@amazon.com> Signed-off-by: Stephen Crawford <65832608+scrawfor99@users.noreply.github.com> Signed-off-by: Andrey Pleskach <ples@aiven.io> Co-authored-by: Andrey Pleskach <ples@aiven.io>

…s, and multi tenancy changes (#2737) * [Extensions] Generate auth tokens for service accounts (#2716) * Generate auth tokens for service accounts Signed-off-by: Stephen Crawford <steecraw@amazon.com> Signed-off-by: Stephen Crawford <65832608+scrawfor99@users.noreply.github.com> * Security User Refactor (#2594) --------- Signed-off-by: Stephen Crawford <steecraw@amazon.com> Signed-off-by: Stephen Crawford <65832608+scrawfor99@users.noreply.github.com> * Backport service account changes Signed-off-by: Stephen Crawford <steecraw@amazon.com> * Update test Signed-off-by: Stephen Crawford <steecraw@amazon.com> * Optimize imports Signed-off-by: Stephen Crawford <steecraw@amazon.com> * Spotless Signed-off-by: Stephen Crawford <steecraw@amazon.com> * fix plugin Signed-off-by: Stephen Crawford <steecraw@amazon.com> * fix whitespace Signed-off-by: Stephen Crawford <steecraw@amazon.com> * Fix multitency config update (#2758) Moved multi-tenancy to REST API implementation Signed-off-by: Andrey Pleskach <ples@aiven.io> * Remove SSLCertsAction Signed-off-by: Stephen Crawford <steecraw@amazon.com> * Fix dependency Signed-off-by: Stephen Crawford <steecraw@amazon.com> * fix tenancy tests Signed-off-by: Stephen Crawford <steecraw@amazon.com> --------- Signed-off-by: Stephen Crawford <steecraw@amazon.com> Signed-off-by: Stephen Crawford <65832608+scrawfor99@users.noreply.github.com> Signed-off-by: Andrey Pleskach <ples@aiven.io> Co-authored-by: Andrey Pleskach <ples@aiven.io> (cherry picked from commit fa33fc5)

…s, and multi tenancy changes (#2737) (#2777) * [Extensions] Generate auth tokens for service accounts (#2716) * Generate auth tokens for service accounts Signed-off-by: Stephen Crawford <steecraw@amazon.com> Signed-off-by: Stephen Crawford <65832608+scrawfor99@users.noreply.github.com> * Security User Refactor (#2594) --------- Signed-off-by: Stephen Crawford <steecraw@amazon.com> Signed-off-by: Stephen Crawford <65832608+scrawfor99@users.noreply.github.com> * Backport service account changes Signed-off-by: Stephen Crawford <steecraw@amazon.com> * Update test Signed-off-by: Stephen Crawford <steecraw@amazon.com> * Optimize imports Signed-off-by: Stephen Crawford <steecraw@amazon.com> * Spotless Signed-off-by: Stephen Crawford <steecraw@amazon.com> * fix plugin Signed-off-by: Stephen Crawford <steecraw@amazon.com> * fix whitespace Signed-off-by: Stephen Crawford <steecraw@amazon.com> * Fix multitency config update (#2758) Moved multi-tenancy to REST API implementation Signed-off-by: Andrey Pleskach <ples@aiven.io> * Remove SSLCertsAction Signed-off-by: Stephen Crawford <steecraw@amazon.com> * Fix dependency Signed-off-by: Stephen Crawford <steecraw@amazon.com> * fix tenancy tests Signed-off-by: Stephen Crawford <steecraw@amazon.com> --------- Signed-off-by: Stephen Crawford <steecraw@amazon.com> Signed-off-by: Stephen Crawford <65832608+scrawfor99@users.noreply.github.com> Signed-off-by: Andrey Pleskach <ples@aiven.io> Co-authored-by: Andrey Pleskach <ples@aiven.io> (cherry picked from commit fa33fc5) Co-authored-by: Stephen Crawford <65832608+scrawfor99@users.noreply.github.com>

Moved multi-tenancy to REST API implementation Signed-off-by: Andrey Pleskach <ples@aiven.io>

Moved multi-tenancy to REST API implementation Signed-off-by: Andrey Pleskach <ples@aiven.io> Signed-off-by: Maciej Mierzwa <dev.maciej.mierzwa@gmail.com>

Moved multi-tenancy to REST API implementation Signed-off-by: Andrey Pleskach <ples@aiven.io>

Moved multi-tenancy to REST API implementation Signed-off-by: Andrey Pleskach <ples@aiven.io> Signed-off-by: Sam <samuel.costa@eliatra.com>

willyborankin requested review from cliu123, cwperks, DarshitChanpura, davidlago, peternied, RyanL1997, stephen-crawford and reta as code owners May 10, 2023 11:12

RyanL1997 reviewed May 10, 2023

View reviewed changes

cwperks previously approved these changes May 11, 2023

View reviewed changes

willyborankin dismissed cwperks’s stale review via 6c1f4f9 May 11, 2023 15:06

willyborankin force-pushed the fix-timeout-issue branch 5 times, most recently from 0b7c9b2 to 59e7a35 Compare May 11, 2023 17:14

DarshitChanpura previously approved these changes May 11, 2023

View reviewed changes

cwperks previously approved these changes May 11, 2023

View reviewed changes

willyborankin dismissed stale reviews from cwperks and DarshitChanpura via fac40fb May 12, 2023 07:39

willyborankin force-pushed the fix-timeout-issue branch 4 times, most recently from 4b55714 to 6337959 Compare May 12, 2023 09:39

Fix multitency config update

b7ed864

Moved multi-tenancy to REST API implementation Signed-off-by: Andrey Pleskach <ples@aiven.io>

willyborankin force-pushed the fix-timeout-issue branch from 6337959 to b7ed864 Compare May 15, 2023 09:20

cwperks approved these changes May 15, 2023

View reviewed changes

stephen-crawford approved these changes May 15, 2023

View reviewed changes

stephen-crawford merged commit 94db5db into opensearch-project:main May 15, 2023

willyborankin mentioned this pull request May 15, 2023

[Backport 2.x] deserializeSafeFromHeader uses context.getHeader(headerName) instead of context.getHeaders() #2771

Merged

RyanL1997 added backport 2.x backport to 2.x branch backport 2.7 backport to 2.7 branch labels May 15, 2023

willyborankin mentioned this pull request May 16, 2023

[Manual Backport 2.x] Manually backports auth tokens, service accounts, and multi tenancy changes #2737

Merged

3 tasks

stephen-crawford pushed a commit to stephen-crawford/security that referenced this pull request May 16, 2023

Fix multitency config update (opensearch-project#2758)

c57e3ca

Moved multi-tenancy to REST API implementation Signed-off-by: Andrey Pleskach <ples@aiven.io>

sebastianmichalski pushed a commit to sebastianmichalski/security that referenced this pull request May 19, 2023

Fix multitency config update (opensearch-project#2758)

c58442e

Moved multi-tenancy to REST API implementation Signed-off-by: Andrey Pleskach <ples@aiven.io>

willyborankin deleted the fix-timeout-issue branch May 22, 2023 15:18

peternied mentioned this pull request Jun 16, 2023

[Flaky Test] Configuration is not available after an update for more than 10 seconds #2742

Closed

samuelcostae pushed a commit to samuelcostae/security that referenced this pull request Jun 19, 2023

Fix multitency config update (opensearch-project#2758)

4826455

Moved multi-tenancy to REST API implementation Signed-off-by: Andrey Pleskach <ples@aiven.io>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix multitency config update #2758

Fix multitency config update #2758

willyborankin commented May 10, 2023 •

edited by dblock

Loading

codecov-commenter commented May 10, 2023 •

edited by codecov bot

Loading

RyanL1997 left a comment

RyanL1997 May 10, 2023

willyborankin May 11, 2023

RyanL1997 May 10, 2023

willyborankin May 11, 2023 •

edited

Loading

RyanL1997 May 10, 2023

willyborankin May 11, 2023

RyanL1997 May 10, 2023

willyborankin May 11, 2023

cwperks left a comment

cwperks May 11, 2023

cwperks May 11, 2023

cwperks May 11, 2023

willyborankin May 11, 2023

DarshitChanpura left a comment

DarshitChanpura May 11, 2023

DarshitChanpura May 11, 2023

willyborankin May 12, 2023

willyborankin commented May 15, 2023

stephen-crawford left a comment

opensearch-trigger-bot bot commented May 15, 2023

opensearch-trigger-bot bot commented May 15, 2023

RyanL1997 commented May 15, 2023

stephen-crawford commented May 16, 2023

		import static org.opensearch.security.dlic.rest.support.Utils.addRoutesPrefix;


		public class MultiTenancyConfigApiAction extends AbstractApiAction {

Fix multitency config update #2758

Fix multitency config update #2758

Conversation

willyborankin commented May 10, 2023 • edited by dblock Loading

Description

Issues Resolved

Check List

codecov-commenter commented May 10, 2023 • edited by codecov bot Loading

Codecov Report

RyanL1997 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

willyborankin May 11, 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

cwperks 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

willyborankin commented May 15, 2023

stephen-crawford left a comment

Choose a reason for hiding this comment

opensearch-trigger-bot bot commented May 15, 2023

opensearch-trigger-bot bot commented May 15, 2023

RyanL1997 commented May 15, 2023

stephen-crawford commented May 16, 2023

willyborankin commented May 10, 2023 •

edited by dblock

Loading

codecov-commenter commented May 10, 2023 •

edited by codecov bot

Loading

willyborankin May 11, 2023 •

edited

Loading