Dynamically Enable/Disable multi-tenancy, private tenant and set Defa… by abhivka7 · Pull Request #2444 · opensearch-project/security

abhivka7 · 2023-02-17T16:16:24Z

…ult Tenant

Description

Category : Enhancement
Why these changes are required?
These changes are required to dynamically enable/disable multi-tenancy, private tenant and set a default tenant.
What is the old behavior before changes and new behavior after changes?
Earlier users had to change YAMl files in each node and restart node to enable/disable multi-tenancy and private tenant. With our changes, users will be able to change these settings dynamically. We will also give admins the option to set a default tenant for all users for first time log in.
More details can be found here:
[RFC] Dynamically Configurable Tenancy Feature OpenSearch#5853
[FEATURE] Enable/Disable Multi-Tenancy, Private Tenant and set Default Tenant in OS Dashboards. security-dashboards-plugin#1302

Issues Resolved

opensearch-project/security-dashboards-plugin#1302

Is this a backport? If so, please add backport PR # and/or commits #
Nope

Testing

We have done manual testing for all changes on our local server. We have also written some unit tests for our api changes. We will write more unit tests and integration tests based on reviewer feedbacks soon.

Check List

[ ✅ ] New functionality includes testing
[] New functionality has been documented. : Will document in further commits.
[ ✅ ] 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.

…ult Tenant Signed-off-by: Abhi Kalra <abhivka@amazon.com>

stephen-crawford

Looks good, just a couple comments and general questions. I left a note for cleaning the style stuff. Thank you for you contribution.

stephen-crawford · 2023-02-17T16:56:34Z

config/tenancy_config.yml

+tenancy_config:
+  multitenancy_enabled: true
+  private_tenant_enabled: true
+  default_tenant: ""


You can use ./gradlew spotlessApply to automatically correct style

Thanks for the sugestion @scrawfor99 . I used ./gradlew spotlessApply as per your suggestion. It corrected style for some files but not tenancy_config.yml. Do you need any specific change in this file that you want me to do manually?

stephen-crawford · 2023-02-17T17:00:16Z

src/main/java/org/opensearch/security/configuration/ConfigurationRepository.java

                                    ConfigHelper.uploadFile(client, cd+"roles_mapping.yml", securityIndex, CType.ROLESMAPPING, DEFAULT_CONFIG_VERSION);
                                    ConfigHelper.uploadFile(client, cd+"internal_users.yml", securityIndex, CType.INTERNALUSERS, DEFAULT_CONFIG_VERSION);
                                    ConfigHelper.uploadFile(client, cd+"action_groups.yml", securityIndex, CType.ACTIONGROUPS, DEFAULT_CONFIG_VERSION);
+                                    final boolean populateEmptyIfFileMissing = true;


Should this be moved to a config constants file?

Sure will do in next commit. Thanks for the suggestion.

With this, would populateEmptyIfFileMissing always be true? When would it be false?

It was already always true. We just moved the line of declaring this variable. From next commit onwards, we can change this from config constants file.

stephen-crawford · 2023-02-17T17:07:36Z

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

+
+        final SecurityDynamicConfiguration<?> configuration = load(getConfigName(), true);
+        filter(configuration);
+        successResponse(channel,configuration);


If you want to do anything special for responses you will need to add onResponse logic here.

@scrawfor99 We do not want to do anything special with our responses. I don't think we need that logic here.

expani · 2023-02-17T18:13:45Z

src/main/java/org/opensearch/security/configuration/PrivilegesInterceptorImpl.java

                                                final Resolved requestedResolved, final Map<String, Boolean> tenants) {

-        final boolean enabled = config.isDashboardsMultitenancyEnabled();//config.dynamic.kibana.multitenancy_enabled;
+        final boolean enabled = tenancyconfig.isDashboardsMultitenancyEnabled();


nit: Camel Case

Thanks @anijain-Amazon for finding this miss. Will update in next commit.

With this change, would config.dynamic.kibana.multitenancy_enabled be ignored? Would this work in mixed clusters? Based on my understanding, security index is node-scoped. If there're 2 settings(the old and the new), could there be 2 different values(false and true) of multitenancy_enabled setting in a mixed cluster?

@cliu123 going forward we will only use config.dynamic.kibana.multitenancy_enabled and this will get priority over config.dynamic.kibana.multitenancy_enabled.

If there're 2 settings(the old and the new), could there be 2 different values(false and true) of multitenancy_enabled setting in a mixed cluster?

In this case we will give priority to new settings.

expani · 2023-02-17T18:22:57Z

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

 		try {
 			final XContentBuilder builder = channel.newBuilder();
 			builder.startObject();
+			builder.endObject();


Were we not calling builder.endObject(); before ?

No we were not doing it. That was a miss I guess.

expani · 2023-02-17T18:24:25Z

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

 			// validate additional settings, if any
 			AbstractConfigurationValidator validator = getValidator(request, request.content());
-			if (!validator.validate()) {
+			if (validator!= null && !validator.validate()) {


If we change the condition to validator == null || !validator.validate() then the use of checks in the lines below is not required.

This was added while testing and I forgot to remove it. Thanks @anijain-Amazon for pointing this out. Will remove this check condition in next commit.

expani · 2023-02-17T18:26:10Z

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

+
+    @Override
+    protected void handlePut(RestChannel channel, final RestRequest request, final Client client, final JsonNode content) throws IOException{
+        successResponse(channel);


If this is not doing anything, shouldn't it be notImplemented as well ?

We want to enable PUT request for tenancyConfig. That's why we are not using notImplemented .

For my understanding, why is it returning successResponse directly?

cwperks

@abhivka7 Thank you for the PR! Overall the PR looks good, but I wanted to run an idea by you to ensure that introducing a new tenantconfig.yml file is the right way to go.

I see that config.yml already provides:

@Override
public boolean isDashboardsMultitenancyEnabled() {
    return config.dynamic.kibana.multitenancy_enabled;
}

was any consideration given to also adding private_tenant_enabled and default_tenant to this section of config.yml? I think that could potentially simplify this PR. The beginning of config.yml could look like:

config:
  dynamic:
    # Set filtered_alias_mode to 'disallow' to forbid more than 2 filtered aliases per index
    # Set filtered_alias_mode to 'warn' to allow more than 2 filtered aliases per index but warns about it (default)
    # Set filtered_alias_mode to 'nowarn' to allow more than 2 filtered aliases per index silently
    #filtered_alias_mode: warn
    #do_not_fail_on_forbidden: false
    multitenancy:
      enabled: true
      private_tenant_enabled: true
      default_tenant: ''

then to get these into SecurityInfoAction you could follow a pattern similar to https://github.com/opensearch-project/security/blob/main/src/main/java/org/opensearch/security/auth/BackendRegistry.java#L152-L169 to subscribe to changes in this config.

cwperks · 2023-02-17T21:04:17Z

src/main/java/org/opensearch/security/tools/SecurityAdmin.java

 	private static String[] getTypes(boolean legacy) {
 		if (legacy) {
-			return new String[]{"config", "roles", "rolesmapping", "internalusers", "actiongroups", "nodesdn", "audit"};
+			return new String[]{"config", "roles", "rolesmapping", "internalusers", "actiongroups", "nodesdn", "audit","tenancyconfig"};


This legacy block is only for CType for ES6. This can be removed from this block.

Thanks for pointing this out @cwperks. Will remove this in next commit.

cwperks · 2023-02-17T21:07:38Z

src/main/java/org/opensearch/security/configuration/ConfigurationRepository.java

+                                    final boolean populateEmptyIfFileMissing = true;
                                    if(DEFAULT_CONFIG_VERSION == 2) {
                                        ConfigHelper.uploadFile(client, cd+"tenants.yml", securityIndex, CType.TENANTS, DEFAULT_CONFIG_VERSION);
+                                        ConfigHelper.uploadFile(client, cd+"tenancy_config.yml", securityIndex, CType.TENANCYCONFIG, DEFAULT_CONFIG_VERSION, populateEmptyIfFileMissing);


Should this only be populated if the file exists? I see tenants.yml does not create this config entry if no tenants.yml file is supplied.

cwperks · 2023-02-17T21:09:53Z

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

 			// validate additional settings, if any
 			AbstractConfigurationValidator validator = getValidator(request, request.content());
-			if (!validator.validate()) {
+			if (validator!= null && !validator.validate()) {


I see that this PR introduces a validator. When will the validator ever be null? Are the changes in this file needed?

This was added while testing and I forgot to remove it. Thanks @cwperks for pointing this out. Will remove this check condition in next commit.

shikharj05

Thank you for the work, added few comments.

shikharj05 · 2023-02-20T04:49:48Z

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

+                            new Route(Method.GET, "/tenancyconfig/"),
+                            new Route(Method.PUT, "/tenancyconfig/"),
+                            new Route(Method.PATCH, "/tenancyconfig/")
+                    )


Is DELETE not required?

I don't think there will be a use case where we require DELETE. We should not delete tenancy_config and we can replace existing config using PATCH.
Correct me if I am wrong @devardee

shikharj05 · 2023-02-20T04:51:42Z

src/main/java/org/opensearch/security/securityconf/TenancyConfigModel.java

+
+    public boolean isDashboardsMultitenancyEnabled() { return this.tenancyConfig.multitenancy_enabled; };
+    public boolean isDashboardsPrivateTenantEnabled() { return this.tenancyConfig.private_tenant_enabled; };
+    public String dashboardsDefaultTenant() { return this.tenancyConfig.default_tenant; };


nit: rename to getDashboardsDefaultTenant

Ok. Will change in next commit.

shikharj05 · 2023-02-20T04:53:13Z

src/main/java/org/opensearch/security/securityconf/DynamicConfigFactory.java

        final WhitelistingSettings whitelist = (WhitelistingSettings) cr.getConfiguration(CType.WHITELIST).getCEntry("config");
        final AllowlistingSettings allowlist = (AllowlistingSettings) cr.getConfiguration(CType.ALLOWLIST).getCEntry("config");
        final AuditConfig audit = (AuditConfig)cr.getConfiguration(CType.AUDIT).getCEntry("config");
+        final TenancyConfigModel tcm ;


nit: extra space

Thanks @shikharj05 for pointing this out. Will fix in next commit.

shikharj05 · 2023-02-20T04:56:02Z

src/main/java/org/opensearch/security/securityconf/impl/v7/TenancyConfigV7.java

+
+import com.fasterxml.jackson.annotation.JsonInclude;
+
+public class TenancyConfigV7 {


implement a toString() method?

Sure. Will do in next commit.

shikharj05 · 2023-02-20T04:58:06Z

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

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


these can be split into different tests for each property

Thanks for the suggestion @shikharj05 . I will split these and update in next commit.

I would suggest to keep it in one test, as the test sets up a cluster before executing the test and tears down after. This will impact on the performance of the tests.

cwperks · 2023-02-20T14:35:33Z

@abhivka7 @shikharj05 See comment above. Have you considered doing this inside of config.yml under config.dynamic instead of introducing a new config file?

i.e.

config:
  dynamic:
    multitenancy:
      enabled: true
      private_tenant_enabled: true
      default_tenant: ''

abhivka7 · 2023-02-21T05:34:39Z

@abhivka7 Thank you for the PR! Overall the PR looks good, but I wanted to run an idea by you to ensure that introducing a new tenantconfig.yml file is the right way to go.

I see that config.yml already provides:
@Override
public boolean isDashboardsMultitenancyEnabled() {
    return config.dynamic.kibana.multitenancy_enabled;
}
was any consideration given to also adding private_tenant_enabled and default_tenant to this section of config.yml? I think that could potentially simplify this PR. The beginning of config.yml could look like:
config:
  dynamic:
    # Set filtered_alias_mode to 'disallow' to forbid more than 2 filtered aliases per index
    # Set filtered_alias_mode to 'warn' to allow more than 2 filtered aliases per index but warns about it (default)
    # Set filtered_alias_mode to 'nowarn' to allow more than 2 filtered aliases per index silently
    #filtered_alias_mode: warn
    #do_not_fail_on_forbidden: false
    multitenancy:
      enabled: true
      private_tenant_enabled: true
      default_tenant: ''
then to get these into SecurityInfoAction you could follow a pattern similar to https://github.com/opensearch-project/security/blob/main/src/main/java/org/opensearch/security/auth/BackendRegistry.java#L152-L169 to subscribe to changes in this config.

@cwperks We were initially working on the same approach that you suggested. But going forward, we plan on introducing some other fields and features for multi-tenancy. Therefore we plan on separating this file and introducing a new one.

cliu123 · 2023-02-21T06:39:57Z

src/main/java/org/opensearch/security/configuration/ConfigurationRepository.java

                                    ConfigHelper.uploadFile(client, cd+"roles_mapping.yml", securityIndex, CType.ROLESMAPPING, DEFAULT_CONFIG_VERSION);
                                    ConfigHelper.uploadFile(client, cd+"internal_users.yml", securityIndex, CType.INTERNALUSERS, DEFAULT_CONFIG_VERSION);
                                    ConfigHelper.uploadFile(client, cd+"action_groups.yml", securityIndex, CType.ACTIONGROUPS, DEFAULT_CONFIG_VERSION);
+                                    final boolean populateEmptyIfFileMissing = true;


With this, would populateEmptyIfFileMissing always be true? When would it be false?

cliu123 · 2023-02-21T06:42:47Z

src/main/java/org/opensearch/security/configuration/PrivilegesInterceptorImpl.java

        //intercept when requests are not made by the kibana server and if the kibana index/alias (.kibana) is the only index/alias involved
        final boolean dashboardsIndexOnly = !user.getName().equals(dashboardsServerUsername) && resolveToDashboardsIndexOrAlias(requestedResolved, dashboardsIndexName);
        final boolean isTraceEnabled = log.isTraceEnabled();
+


Please remove the empty line and avoid format-only changes.

cliu123 · 2023-02-21T07:02:21Z

src/main/java/org/opensearch/security/configuration/PrivilegesInterceptorImpl.java

                                                final Resolved requestedResolved, final Map<String, Boolean> tenants) {

-        final boolean enabled = config.isDashboardsMultitenancyEnabled();//config.dynamic.kibana.multitenancy_enabled;
+        final boolean enabled = tenancyconfig.isDashboardsMultitenancyEnabled();


With this change, would config.dynamic.kibana.multitenancy_enabled be ignored? Would this work in mixed clusters? Based on my understanding, security index is node-scoped. If there're 2 settings(the old and the new), could there be 2 different values(false and true) of multitenancy_enabled setting in a mixed cluster?

cliu123 · 2023-02-21T07:03:35Z

src/main/java/org/opensearch/security/rest/TenantInfoAction.java


                    final User user = (User)threadContext.getTransient(ConfigConstants.OPENDISTRO_SECURITY_USER);
-                    
+
