Fixes

Signed-off-by: Nils Bandener <[email protected]>

src/integrationTest/java/org/opensearch/security/legacy/LegacyConfigV6AutoConversionTest.java

@@ -10,70 +10,98 @@
 package org.opensearch.security.legacy;
 
 import com.carrotsearch.randomizedtesting.annotations.ThreadLeakScope;
+import org.junit.Assert;
 import org.junit.ClassRule;
+import org.junit.FixMethodOrder;
 import org.junit.Test;
 import org.junit.runner.RunWith;
 
+import org.junit.runners.MethodSorters;
 import org.opensearch.test.framework.TestSecurityConfig;
 import org.opensearch.test.framework.cluster.ClusterManager;
 import org.opensearch.test.framework.cluster.LocalCluster;
 import org.opensearch.test.framework.cluster.TestRestClient;
 
+import java.util.Map;
+
+@FixMethodOrder( MethodSorters.NAME_ASCENDING )
 @RunWith(com.carrotsearch.randomizedtesting.RandomizedRunner.class)
 @ThreadLeakScope(ThreadLeakScope.Scope.NONE)
 public class LegacyConfigV6AutoConversionTest {
  static final TestSecurityConfig LEGACY_CONFIG = new TestSecurityConfig()//
- .rawConfigurationDocumentYaml("config", """
- opendistro_security:
- dynamic:
- authc:
- basic_internal_auth_domain:
- http_enabled: true
- order: 4
- http_authenticator:
- type: basic
- challenge: true
- authentication_backend:
- type: intern
- """)//
- .rawConfigurationDocumentYaml("internalusers", """
- admin:
- readonly: true
- hash: $2a$12$VcCDgh2NDk07JGN0rjGbM.Ad41qVR/YFJcgHp0UGns5JDymv..TOG
- roles:
- - admin
- attributes:
- attribute1: value1
- """)//
- .rawConfigurationDocumentYaml("roles", """
- all_access:
- readonly: true
- cluster:
- - UNLIMITED
- indices:
- '*':
- '*':
- - UNLIMITED
- tenants:
- admin_tenant: RW
- """)//
- .rawConfigurationDocumentYaml("rolesmapping", """
- all_access:
- readonly: true
- backendroles:
- - admin
- """);
+ .rawConfigurationDocumentYaml("config", "opendistro_security:\n" +
+ " dynamic:\n" +
+ " authc:\n" +
+ " basic_internal_auth_domain:\n" +
+ " http_enabled: true\n" +
+ " order: 4\n" +
+ " http_authenticator:\n" +
+ " type: basic\n" +
+ " challenge: true\n" +
+ " authentication_backend:\n" +
+ " type: intern\n")//
+ .rawConfigurationDocumentYaml("internalusers", "admin:\n" +
+ " readonly: true\n" +
+ " hash: $2a$12$VcCDgh2NDk07JGN0rjGbM.Ad41qVR/YFJcgHp0UGns5JDymv..TOG\n" +
+ " roles:\n" +
+ " - admin\n" +
+ " attributes:\n" +
+ " attribute1: value1\n")//
+ .rawConfigurationDocumentYaml("roles", "all_access_role:\n" +
+ " readonly: true\n" +
+ " cluster:\n" +
+ " - UNLIMITED\n" +
+ " indices:\n" +
+ " '*':\n" +
+ " '*':\n" +
+ " - UNLIMITED\n" +
+ " tenants:\n" +
+ " admin_tenant: RW\n")//
+ .rawConfigurationDocumentYaml("rolesmapping", "all_access_role:\n" +
+ " readonly: true\n" +
+ " backendroles:\n" +
+ " - admin")//
+ .rawConfigurationDocumentYaml("actiongroups", "dummy:\n" +
+ " permissions: []")
+ ;
 
  @ClassRule
  public static LocalCluster cluster = new LocalCluster.Builder().clusterManager(ClusterManager.THREE_CLUSTER_MANAGERS)
  .config(LEGACY_CONFIG)
+ .nodeSettings(Map.of("plugins.security.restapi.roles_enabled.0", "all_access_role"))
  .build();
 
  @Test
- public void checkAuthc() {
+ public void authc() {
  try (TestRestClient client = cluster.getRestClient("admin", "admin")) {
  TestRestClient.HttpResponse response = client.get("_opendistro/_security/authinfo");
- System.out.println(response.getBody());
+ Assert.assertEquals(response.getBody(), 200, response.getStatusCode());
+ Assert.assertEquals(response.getBody(), true, response.getBooleanFromJsonBody("/tenants/admin"));
+ }
+ }
+
+ @Test
+ public void configRestApiReturnsV6Config() {
+ try (TestRestClient client = cluster.getRestClient("admin", "admin")) {
+ TestRestClient.HttpResponse response = client.get("_opendistro/_security/api/roles/all_access_role");
+ Assert.assertEquals(response.getBody(), 200, response.getStatusCode());
+ Assert.assertEquals("Expected v6 format", "{\"all_access_role\":{\"readonly\":true,\"hidden\":false,\"cluster\":[\"UNLIMITED\"],\"tenants\":{\"admin_tenant\":\"RW\"},\"indices\":{\"*\":{\"*\":[\"UNLIMITED\"]}}}}", response.getBody());
  }
  }
+
+ /**
+ * This must be the last test executed, as it changes the config index
+ */
+ @Test
+ public void zzz_migrateApi() {
+ try (TestRestClient client = cluster.getRestClient("admin", "admin")) {
+ TestRestClient.HttpResponse response = client.post("_opendistro/_security/api/migrate");
+ Assert.assertEquals(response.getBody(), 200, response.getStatusCode());
+ Assert.assertEquals(response.getBody(), "Migration completed.", response.getTextFromJsonBody("/message"));
+ response = client.get("_opendistro/_security/api/roles/all_access_role");
+ Assert.assertEquals(response.getBody(), 200, response.getStatusCode());
+ Assert.assertEquals("Expected v7 format", "Migrated from v6 (all types mapped)", response.getTextFromJsonBody("/all_access_role/description"));
+ }
+ }
+
 }

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

@@ -59,6 +59,7 @@
 import org.opensearch.security.securityconf.impl.CType;
 import org.opensearch.security.securityconf.impl.SecurityDynamicConfiguration;
 import org.opensearch.security.securityconf.impl.v6.RoleV6;
+import org.opensearch.security.securityconf.impl.v7.RoleV7;
 import org.opensearch.security.securityconf.impl.v7.TenantV7;
 import org.opensearch.security.support.ConfigConstants;
 import org.opensearch.security.support.ConfigHelper;
@@ -122,17 +123,6 @@ public void success(SecurityDynamicConfiguration<?> dConf) {
 
  result.with(dConf);
 
- if (dConf.getCType() == CType.ROLES && dConf.getAutoConvertedFrom() != null) {
- // Special case for configuration that was auto-converted from v6:
- // We need to generate tenant config from role config.
- // Having such a special case here is not optimal, but IMHO acceptable, as this
- // should be only a temporary measure until V6 configuration is completely discontinued.
- @SuppressWarnings("unchecked")
- SecurityDynamicConfiguration<RoleV6> roleV6config = (SecurityDynamicConfiguration<RoleV6>) dConf.getAutoConvertedFrom();
- SecurityDynamicConfiguration<TenantV7> tenants = Migration.createTenants(roleV6config);
- result.with(tenants);
- }
-
  latch.countDown();
  if (isDebugEnabled) {
  log.debug(
@@ -158,28 +148,11 @@ public void singleFailure(Failure failure) {
  public void noData(String id) {
  CType<?> cType = CType.fromString(id);
 
- // when index was created with ES 6 there are no separate tenants. So we load just empty ones.
- // when index was created with ES 7 and type not "security" (ES 6 type) there are no rolemappings anymore.
- if (cs.state().metadata().index(securityIndex).getCreationVersion().before(LegacyESVersion.V_7_0_0)) {
- // created with SG 6
- // skip tenants
-
- if (isDebugEnabled) {
- log.debug("Skip tenants because we not yet migrated to ES 7 (index was created with ES 6)");
- }
-
- if (cType == CType.TENANTS) {
- rs.put(cType, SecurityDynamicConfiguration.empty());
- latch.countDown();
- return;
- }
- }
-
  // Since NODESDN is newly introduced data-type applying for existing clusters as well, we make it backward compatible by
  // returning valid empty
  // SecurityDynamicConfiguration.
  // Same idea for new setting WHITELIST/ALLOWLIST
- if (cType == CType.NODESDN || cType == CType.WHITELIST || cType == CType.ALLOWLIST) {
+ if (cType == CType.NODESDN || cType == CType.WHITELIST || cType == CType.ALLOWLIST || cType == CType.TENANTS) {
  try {
  SecurityDynamicConfiguration<?> empty = ConfigHelper.createEmptySdc(
  cType,
@@ -235,6 +208,20 @@ public void failure(Throwable t) {
  );
  }
 
+ SecurityDynamicConfiguration<RoleV7> roleConfig = result.get(CType.ROLES);
+ SecurityDynamicConfiguration<TenantV7> tenantConfig = result.get(CType.TENANTS);
+
+ if (roleConfig != null && roleConfig.getAutoConvertedFrom() != null && (tenantConfig == null || tenantConfig.getCEntries().isEmpty())) {
+ // Special case for configuration that was auto-converted from v6:
+ // We need to generate tenant config from role config.
+ // Having such a special case here is not optimal, but IMHO acceptable, as this
+ // should be only a temporary measure until V6 configuration is completely discontinued.
+ @SuppressWarnings("unchecked")
+ SecurityDynamicConfiguration<RoleV6> roleV6config = (SecurityDynamicConfiguration<RoleV6>) roleConfig.getAutoConvertedFrom();
+ SecurityDynamicConfiguration<TenantV7> tenants = Migration.createTenants(roleV6config);
+ result.with(tenants);
+ }
+
  return result.build();
  }
 

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

@@ -11,6 +11,8 @@
 
 package org.opensearch.security.configuration;
 
+import java.util.HashMap;
+import java.util.Map;
 import java.util.Set;
 
 import com.google.common.collect.ImmutableMap;
@@ -72,7 +74,7 @@ public static ConfigurationMap of(SecurityDynamicConfiguration<?>... configs) {
  }
 
  public static class Builder {
- private ImmutableMap.Builder<CType<?>, SecurityDynamicConfiguration<?>> map = new ImmutableMap.Builder<>();
+ private Map<CType<?>, SecurityDynamicConfiguration<?>> map = new HashMap<>();
 
  public Builder() {}
 
@@ -86,8 +88,23 @@ public Builder with(ConfigurationMap configurationMap) {
  return this;
  }
 
+ public <T> SecurityDynamicConfiguration<T> get(CType<T> ctype) {
+ @SuppressWarnings("unchecked")
+ SecurityDynamicConfiguration<T> config = (SecurityDynamicConfiguration<T>) map.get(ctype);
+
+ if (config == null) {
+ return null;
+ }
+
+ if (!config.getCType().equals(ctype)) {
+ throw new RuntimeException("Stored configuration does not match type: " + ctype + "; " + config);
+ }
+
+ return config;
+ }
+
  public ConfigurationMap build() {
- return new ConfigurationMap(this.map.build());
+ return new ConfigurationMap(ImmutableMap.copyOf(this.map));
  }
  }
 }

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

@@ -635,6 +635,16 @@ public ConfigurationMap getConfigurationsFromIndex(
  return result;
  }
 
+ public SecurityDynamicConfiguration<?> getUnconvertedConfigurationFromIndex(CType<?> configType, boolean logComplianceEvent) {
+ ConfigurationMap map = this.getConfigurationsFromIndex(List.of(configType), logComplianceEvent, this.acceptInvalid);
+ SecurityDynamicConfiguration<?> result = map.get(configType);
+ if (result != null && result.getAutoConvertedFrom() != null) {
+ result = result.getAutoConvertedFrom();
+ }
+
+ return result;
+ }
+
  private ConfigurationMap validate(ConfigurationMap conf, int expectedSize) throws InvalidConfigException {
 
  if (conf == null || conf.size() < expectedSize) {

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

@@ -455,16 +455,14 @@ public RequestContentValidator createRequestContentValidator(Object... params) {
 
  protected final SecurityDynamicConfiguration<?> load(final CType<?> config, boolean logComplianceEvent) {
  SecurityDynamicConfiguration<?> loaded = securityApiDependencies.configurationRepository()
- .getConfigurationsFromIndex(List.of(config), logComplianceEvent)
- .get(config)
+ .getUnconvertedConfigurationFromIndex(config, logComplianceEvent)
  .deepClone();
  return DynamicConfigFactory.addStatics(loaded);
  }
 
  protected final SecurityDynamicConfiguration<?> loadAndRedact(final CType<?> config, boolean logComplianceEvent) {
  SecurityDynamicConfiguration<?> loaded = securityApiDependencies.configurationRepository()
- .getConfigurationsFromIndex(List.of(config), logComplianceEvent)
- .get(config)
+ .getUnconvertedConfigurationFromIndex(config, logComplianceEvent)
  .deepCloneWithRedaction();
  return DynamicConfigFactory.addStatics(loaded);
  }

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

@@ -116,7 +116,7 @@ public static SecurityDynamicConfiguration<TenantV7> createTenants(SecurityDynam
  }
  }
 
- SecurityDynamicConfiguration<TenantV7> tenantConfig = SecurityDynamicConfiguration.empty(CType.TENANTS);
+ SecurityDynamicConfiguration<TenantV7> tenantConfig = SecurityDynamicConfiguration.empty(CType.TENANTS, 2);
  tenantConfig.set_meta(new Meta());
  tenantConfig.get_meta().setConfig_version(2);
  tenantConfig.get_meta().setType("tenants");

src/main/java/org/opensearch/security/securityconf/impl/CType.java

@@ -297,6 +297,7 @@ public SecurityDynamicConfiguration<NewType> parseJson(CType<NewType> ctype, Str
  public SecurityDynamicConfiguration<NewType> convert(SecurityDynamicConfiguration<OldType> oldConfig, CType<NewType> ctype) {
  SecurityDynamicConfiguration<NewType> newConfig = SecurityDynamicConfiguration.empty(ctype);
  newConfig.setAutoConvertedFrom(oldConfig);
+// newConfig.setSeqNoPrimaryTerm();
 
  for (Map.Entry<String, OldType> oldEntry : oldConfig.getCEntries().entrySet()) {
  newConfig.putCEntry(mapKeyConversionFunction.apply(oldEntry.getKey()), entryConversionFunction.apply(oldEntry.getValue()));