Skip to content
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
### Enhancements
- Make security plugin aware of FIPS build param (-Pcrypto.standard=FIPS-140-3) ([#5952](https://github.com/opensearch-project/security/pull/5952))
- Hardens input validation for resource sharing APIs ([#5831](https://github.com/opensearch-project/security/pull/5831)
- Add `preferred_tenants` to config and dashboard info APIs ([#5969](https://github.com/opensearch-project/security/issues/5969))
- Optimize getFieldFilter to only return a predicate when index has FLS restrictions for user ([#5777](https://github.com/opensearch-project/security/pull/5777))
- Performance optimizations for building internal authorization data structures upon config updates ([#5988](https://github.com/opensearch-project/security/pull/5988))
- Make encryption_key optional for obo token authenticator ([#6017](https://github.com/opensearch-project/security/pull/6017)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@
import org.opensearch.security.dlic.rest.validation.EndpointValidator;
import org.opensearch.security.dlic.rest.validation.RequestContentValidator;
import org.opensearch.security.dlic.rest.validation.RequestContentValidator.DataType;
import org.opensearch.security.dlic.rest.validation.RequestContentValidator.FieldConfiguration;
import org.opensearch.security.securityconf.impl.CType;
import org.opensearch.security.securityconf.impl.DashboardSignInOption;
import org.opensearch.security.securityconf.impl.SecurityDynamicConfiguration;
Expand All @@ -49,12 +50,15 @@
import static org.opensearch.security.dlic.rest.support.Utils.OPENDISTRO_API_DEPRECATION_MESSAGE;
import static org.opensearch.security.dlic.rest.support.Utils.addLegacyRoutesPrefix;
import static org.opensearch.security.dlic.rest.support.Utils.addRoutesPrefix;
import static org.opensearch.security.dlic.rest.validation.RequestContentValidator.ARRAY_OF_STRINGS_VALIDATOR;
import static org.opensearch.security.dlic.rest.validation.RequestContentValidator.arraySizeValidator;

public class MultiTenancyConfigApiAction extends AbstractApiAction {

public static final String DEFAULT_TENANT_JSON_PROPERTY = "default_tenant";
public static final String PRIVATE_TENANT_ENABLED_JSON_PROPERTY = "private_tenant_enabled";
public static final String MULTITENANCY_ENABLED_JSON_PROPERTY = "multitenancy_enabled";
public static final String PREFERRED_TENANTS = "preferred_tenants";
public static final String SIGN_IN_OPTIONS = "sign_in_options";

private static final List<Route> ROUTES = addRoutesPrefix(
Expand All @@ -74,6 +78,8 @@ public class MultiTenancyConfigApiAction extends AbstractApiAction {
ConfigConstants.TENANCY_PRIVATE_TENANT_NAME
);

private static final int PREFERRED_TENANTS_MAX_SIZE = 100;

@Override
public String getName() {
return "Multi Tenancy actions to Retrieve / Update configs.";
Expand Down Expand Up @@ -132,6 +138,7 @@ public Settings settings() {

@Override
public Map<String, DataType> allowedKeys() {
// Provide basic type information for backward compatibility
return ImmutableMap.of(
DEFAULT_TENANT_JSON_PROPERTY,
DataType.STRING,
Expand All @@ -140,9 +147,32 @@ public Map<String, DataType> allowedKeys() {
MULTITENANCY_ENABLED_JSON_PROPERTY,
DataType.BOOLEAN,
SIGN_IN_OPTIONS,
DataType.ARRAY,
PREFERRED_TENANTS,
DataType.ARRAY
);
}

@Override
public Map<String, FieldConfiguration> allowedKeysWithConfig() {
return ImmutableMap.<String, FieldConfiguration>builder()
.put(
DEFAULT_TENANT_JSON_PROPERTY,
FieldConfiguration.of(
DataType.STRING,
RequestContentValidator.MAX_STRING_LENGTH,
RequestContentValidator.principalValidator(false)
)
)
.put(PRIVATE_TENANT_ENABLED_JSON_PROPERTY, FieldConfiguration.of(DataType.BOOLEAN))
.put(MULTITENANCY_ENABLED_JSON_PROPERTY, FieldConfiguration.of(DataType.BOOLEAN))
.put(SIGN_IN_OPTIONS, FieldConfiguration.of(DataType.ARRAY))
.put(PREFERRED_TENANTS, FieldConfiguration.of(DataType.ARRAY, (fieldName, value) -> {
arraySizeValidator(PREFERRED_TENANTS_MAX_SIZE).validate(fieldName, value);
ARRAY_OF_STRINGS_VALIDATOR.validate(fieldName, value);
}))
.build();
}
});
}
};
Expand All @@ -154,6 +184,7 @@ private ToXContent multitenancyContent(final ConfigV7 config) {
.field(PRIVATE_TENANT_ENABLED_JSON_PROPERTY, config.dynamic.kibana.private_tenant_enabled)
.field(MULTITENANCY_ENABLED_JSON_PROPERTY, config.dynamic.kibana.multitenancy_enabled)
.field(SIGN_IN_OPTIONS, config.dynamic.kibana.sign_in_options)
.field(PREFERRED_TENANTS, config.dynamic.kibana.preferred_tenants)
.endObject();
}

Expand Down Expand Up @@ -204,6 +235,10 @@ private void updateAndValidatesValues(final ConfigV7 config, final JsonNode json
List<DashboardSignInOption> options = getNewSignInOptions(newOptions, config.dynamic.authc);
config.dynamic.kibana.sign_in_options = options;
}
if (jsonContent.hasNonNull(PREFERRED_TENANTS)) {
List<String> preferredTenants = getPreferredTenants(jsonContent.findValue(PREFERRED_TENANTS));
config.dynamic.kibana.preferred_tenants = preferredTenants;
}

final String defaultTenant = Optional.ofNullable(config.dynamic.kibana.default_tenant).map(String::toLowerCase).orElse("");

Expand All @@ -222,6 +257,7 @@ private void updateAndValidatesValues(final ConfigV7 config, final JsonNode json
.stream()
.map(String::toLowerCase)
.collect(Collectors.toSet());

if (!availableTenants.contains(defaultTenant)) {
throw new IllegalArgumentException(
config.dynamic.kibana.default_tenant
Expand All @@ -246,4 +282,11 @@ private List<DashboardSignInOption> getNewSignInOptions(JsonNode newOptions, Aut
}
}).map(DashboardSignInOption::valueOf).collect(Collectors.toList());
}

private List<String> getPreferredTenants(JsonNode preferredTenants) {
return IntStream.range(0, preferredTenants.size())
.mapToObj(preferredTenants::get)
.map(tenant -> { return tenant.asText(); })
.collect(Collectors.toList());
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
import java.util.Optional;
import java.util.Set;
import java.util.regex.Pattern;
import java.util.stream.IntStream;

import com.fasterxml.jackson.core.JsonParser;
import com.fasterxml.jackson.core.JsonToken;
Expand Down Expand Up @@ -379,7 +380,15 @@ private ValidationResult<JsonNode> validateDataType(final JsonNode jsonContent)
}

private ValidationResult<JsonNode> nullValuesInArrayValidator(final JsonNode jsonContent) {
for (final Map.Entry<String, DataType> allowedKey : validationContext.allowedKeys().entrySet()) {
Copy link
Member

Choose a reason for hiding this comment

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

nit: I think key name is all that matters here so idk if we need this change. That being said, is it enforced that allowedKeyWithConfig is strictly a subset of allowedKeys?

Copy link
Author

Choose a reason for hiding this comment

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

I understand, we don’t need the DataType; just the key name is sufficient, and I’ll update the changes.

And allowedKeysWithConfig isn’t required to be a strict subset of allowedKeys. If allowedKeysWithConfig is present, it takes precedence; otherwise, we fallback to allowedKeys. Going forward, for any new API we define, we can start using allowedKeysWithConfig directly, which will make it easier when we eventually remove allowedKeys completely.

// Use allowedKeysWithConfig if provided, otherwise fall back to allowedKeys
final Map<String, FieldConfiguration> fieldConfigs = validationContext.allowedKeysWithConfig();
final Map<String, DataType> allowedKeys = new HashMap<>();
if (fieldConfigs != null) {
fieldConfigs.forEach((fieldName, fieldConfig) -> { allowedKeys.put(fieldName, fieldConfig.dataType); });
} else {
allowedKeys.putAll(validationContext.allowedKeys());
}
for (final Map.Entry<String, DataType> allowedKey : allowedKeys.entrySet()) {
JsonNode value = jsonContent.get(allowedKey.getKey());
if (value != null) {
if (hasNullOrBlankArrayElement(value)) {
Expand Down Expand Up @@ -702,19 +711,40 @@ public static FieldValidator principalValidator(boolean allowWildcard) {
};

/**
* Validator for array entry counts (works with JsonNode arrays)
* Validator for array entry counts with default MAX_ARRAY_SIZE (works with JsonNode arrays)
*/
public static final FieldValidator ARRAY_SIZE_VALIDATOR = (fieldName, value) -> {
validateArraySize(fieldName, value, MAX_ARRAY_SIZE);
};

/**
* Validator for array entry counts with custom maxSize (works with JsonNode arrays)
*/
public static FieldValidator arraySizeValidator(int maxSize) {
return (fieldName, value) -> { validateArraySize(fieldName, value, maxSize); };
}

private static void validateArraySize(String fieldName, Object value, int maxSize) {
if (value instanceof JsonNode node) {
if (node.isArray() && node.size() > MAX_ARRAY_SIZE) {
throw new IllegalArgumentException("Array field [" + fieldName + "] exceeds maximum size of " + MAX_ARRAY_SIZE);
if (node.isArray() && node.size() > maxSize) {
throw new IllegalArgumentException("Array field [" + fieldName + "] exceeds maximum size of " + maxSize);
}
} else if (value instanceof Integer) {
int count = (Integer) value;
if (count > MAX_ARRAY_SIZE) {
throw new IllegalArgumentException("Array field [" + fieldName + "] exceeds maximum size of " + MAX_ARRAY_SIZE);
if (count > maxSize) {
throw new IllegalArgumentException("Array field [" + fieldName + "] exceeds maximum size of " + maxSize);
}
}
}

public static final FieldValidator ARRAY_OF_STRINGS_VALIDATOR = (fieldName, value) -> {
if (value instanceof JsonNode node) {
IntStream.range(0, node.size()).mapToObj(node::get).forEach(element -> {
if (!element.isTextual()) {
throw new IllegalArgumentException(fieldName + " should only contain string values.");
}
});
}
};

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,8 @@

package org.opensearch.security.privileges;

import java.util.List;

import org.opensearch.security.securityconf.impl.v7.ConfigV7;

/**
Expand All @@ -24,6 +26,7 @@ public class DashboardsMultiTenancyConfiguration {
private final boolean multitenancyEnabled;
private final boolean privateTenantEnabled;
private final String defaultTenant;
private final List<String> preferredTenants;
private final String index;
private final String serverUsername;
private final String role;
Expand All @@ -32,6 +35,7 @@ public DashboardsMultiTenancyConfiguration(ConfigV7.Kibana dashboardsConfig) {
this.multitenancyEnabled = dashboardsConfig.multitenancy_enabled;
this.privateTenantEnabled = dashboardsConfig.private_tenant_enabled;
this.defaultTenant = dashboardsConfig.default_tenant;
this.preferredTenants = dashboardsConfig.preferred_tenants;
this.index = dashboardsConfig.index;
this.serverUsername = dashboardsConfig.server_username;
this.role = dashboardsConfig.opendistro_role;
Expand Down Expand Up @@ -65,6 +69,10 @@ public String dashboardsOpenSearchRole() {
return role;
}

public List<String> preferredTenants() {
return preferredTenants;
}

private static ConfigV7.Kibana dashboardsConfig(ConfigV7 generalConfig) {
if (generalConfig != null && generalConfig.dynamic != null && generalConfig.dynamic.kibana != null) {
return generalConfig.dynamic.kibana;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -143,6 +143,7 @@ public void accept(RestChannel channel) throws Exception {
builder.field("opensearch_dashboards_index", multiTenancyConfiguration.dashboardsIndex());
builder.field("opensearch_dashboards_server_user", multiTenancyConfiguration.dashboardsServerUsername());
builder.field("multitenancy_enabled", multiTenancyConfiguration.multitenancyEnabled());
builder.field("preferred_tenants", multiTenancyConfiguration.preferredTenants());
builder.field("private_tenant_enabled", multiTenancyConfiguration.privateTenantEnabled());
builder.field("default_tenant", multiTenancyConfiguration.dashboardsDefaultTenant());
builder.field("sign_in_options", getSignInOptions());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -113,6 +113,7 @@ public static class Kibana {
public boolean private_tenant_enabled = true;
@JsonInclude(JsonInclude.Include.NON_NULL)
public String default_tenant = "";
public List<String> preferred_tenants = Collections.emptyList();
public String server_username = "kibanaserver";
public String opendistro_role = null;
public String index = ".kibana";
Expand All @@ -127,6 +128,8 @@ public String toString() {
+ private_tenant_enabled
+ ", default_tenant="
+ default_tenant
+ ", preferred_tenants="
+ preferred_tenants
+ ", server_username="
+ server_username
+ ", opendistro_role="
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ private void verifyTenantUpdate(final Header... header) throws Exception {
getDashboardsinfoResponse.findValueInJson("default_tenant"),
equalTo(ConfigConstants.TENANCY_GLOBAL_TENANT_DEFAULT_NAME)
);
assertThat(getDashboardsinfoResponse.findArrayInJson("preferred_tenants").size(), equalTo(0));

final HttpResponse setPrivateTenantAsDefaultResponse = rh.executePutRequest(
"/_plugins/_security/api/tenancy/config",
Expand All @@ -68,12 +69,31 @@ private void verifyTenantUpdate(final Header... header) throws Exception {
);
assertThat(updateDashboardSignInOptions.getBody(), updateDashboardSignInOptions.getStatusCode(), equalTo(HttpStatus.SC_OK));

final HttpResponse updatePreferredTenants = rh.executePutRequest(
"/_plugins/_security/api/tenancy/config",
"{\"preferred_tenants\": [\"Private\", \"Global\"]}",
header
);
assertThat(updatePreferredTenants.getBody(), updatePreferredTenants.getStatusCode(), equalTo(HttpStatus.SC_OK));

getDashboardsinfoResponse = rh.executeGetRequest("/_plugins/_security/dashboardsinfo", ADMIN_FULL_ACCESS_USER);
assertThat(getDashboardsinfoResponse.getStatusCode(), equalTo(HttpStatus.SC_OK));
assertThat(getDashboardsinfoResponse.findValueInJson("default_tenant"), equalTo("Private"));

assertThat(getDashboardsinfoResponse.findArrayInJson("sign_in_options"), hasItem((DashboardSignInOption.BASIC.toString())));
assertThat(getDashboardsinfoResponse.findArrayInJson("sign_in_options"), hasItem((DashboardSignInOption.OPENID.toString())));
assertThat(getDashboardsinfoResponse.findArrayInJson("preferred_tenants"), hasItem("Private"));
assertThat(getDashboardsinfoResponse.findArrayInJson("preferred_tenants"), hasItem("Global"));

final HttpResponse clearPreferredTenants = rh.executePutRequest(
"/_plugins/_security/api/tenancy/config",
"{\"preferred_tenants\": []}",
header
);
assertThat(clearPreferredTenants.getBody(), clearPreferredTenants.getStatusCode(), equalTo(HttpStatus.SC_OK));

getDashboardsinfoResponse = rh.executeGetRequest("/_plugins/_security/dashboardsinfo", ADMIN_FULL_ACCESS_USER);
assertThat(getDashboardsinfoResponse.findArrayInJson("preferred_tenants").size(), equalTo(0));

final HttpResponse updateUnavailableSignInOption = rh.executePutRequest(
"/_plugins/_security/api/tenancy/config",
Expand Down Expand Up @@ -209,6 +229,30 @@ private void verifyTenantUpdateFailed(final Header... header) throws Exception {
invalidSignInOption.findValueInJson("error.reason"),
containsString("authentication provider is not available for this cluster")
);

final HttpResponse preferredTenantsNonArrayValue = rh.executePutRequest(
"/_plugins/_security/api/tenancy/config",
"{\"preferred_tenants\": \"Private\"}",
header
);
assertThat(preferredTenantsNonArrayValue.getStatusCode(), equalTo(HttpStatus.SC_BAD_REQUEST));
assertThat(
preferredTenantsNonArrayValue.getBody(),
preferredTenantsNonArrayValue.findValueInJson("reason"),
containsString("Wrong datatype")
);

final HttpResponse preferredTenantsContainInvalidType = rh.executePutRequest(
"/_plugins/_security/api/tenancy/config",
"{\"preferred_tenants\": [\"Private\", 1]}",
header
);
assertThat(preferredTenantsContainInvalidType.getStatusCode(), equalTo(HttpStatus.SC_BAD_REQUEST));
assertThat(
preferredTenantsContainInvalidType.getBody(),
preferredTenantsContainInvalidType.findValueInJson("preferred_tenants"),
containsString("preferred_tenants should only contain string values")
);
}

@Test
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -646,6 +646,31 @@ public void testArraySizeValidatorRejectsLargeCount() {
);
}

@Test
public void testArraySizeValidatorFactoryUsesCustomLimit() {
final RequestContentValidator.FieldValidator validator = RequestContentValidator.arraySizeValidator(2);
final ObjectNode node = DefaultObjectMapper.objectMapper.createObjectNode();
node.putArray("arr").add("1").add("2").add("3");
expectThrows(IllegalArgumentException.class, () -> validator.validate("arr", node.get("arr")));
}

@Test
public void testArrayOfStringsValidatorAcceptsStringArray() {
final ObjectNode node = DefaultObjectMapper.objectMapper.createObjectNode();
node.putArray("arr").add("one").add("two");
RequestContentValidator.ARRAY_OF_STRINGS_VALIDATOR.validate("arr", node.get("arr"));
}

@Test
public void testArrayOfStringsValidatorRejectsNonStringElement() {
final ObjectNode node = DefaultObjectMapper.objectMapper.createObjectNode();
node.putArray("arr").add("one").add(2);
expectThrows(
IllegalArgumentException.class,
() -> RequestContentValidator.ARRAY_OF_STRINGS_VALIDATOR.validate("arr", node.get("arr"))
);
}

@Test
public void testAllowedValuesValidator() {
final Set<String> allowed = Set.of("read", "write");
Expand Down
Loading
Loading