Skip to content

Commit 45e541d

Browse files
nibixnibix
authored
Optimized tenant privilege evaluation (#5350)
Signed-off-by: Nils Bandener <nils.bandener@eliatra.com>
1 parent 19a41b8 commit 45e541d

File tree

14 files changed

+782
-260
lines changed

14 files changed

+782
-260
lines changed

‎CHANGELOG.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
1212
### Changed
1313
- Use extendedPlugins in integrationTest framework for sample resource plugin testing ([#5322](https://github.com/opensearch-project/security/pull/5322))
1414
- Refactor ResourcePermissions to refer to action groups as access levels ([#5335](https://github.com/opensearch-project/security/pull/5335))
15+
- Introduced new, performance-optimized implementation for tenant privileges ([#5339](https://github.com/opensearch-project/security/pull/5339))
1516

1617
- Performance improvements: Immutable user object ([#5212])
1718

‎src/integrationTest/java/org/opensearch/security/privileges/TenantPrivilegesTest.java‎

Lines changed: 416 additions & 0 deletions
Large diffs are not rendered by default.

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

Lines changed: 24 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -46,7 +46,9 @@
4646
import org.opensearch.cluster.metadata.IndexNameExpressionResolver;
4747
import org.opensearch.cluster.service.ClusterService;
4848
import org.opensearch.security.privileges.DocumentAllowList;
49+
import org.opensearch.security.privileges.PrivilegesEvaluationContext;
4950
import org.opensearch.security.privileges.PrivilegesInterceptor;
51+
import org.opensearch.security.privileges.TenantPrivileges;
5052
import org.opensearch.security.resolver.IndexResolverReplacer.Resolved;
5153
import org.opensearch.security.securityconf.DynamicConfigModel;
5254
import org.opensearch.security.user.User;
@@ -84,32 +86,6 @@ public PrivilegesInterceptorImpl(
8486
super(resolver, clusterService, client, threadPool);
8587
}
8688

87-
private boolean isTenantAllowed(
88-
final ActionRequest request,
89-
final String action,
90-
final User user,
91-
final Map<String, Boolean> tenants,
92-
final String requestedTenant
93-
) {
94-
95-
if (!tenants.keySet().contains(requestedTenant)) {
96-
log.warn("Tenant {} is not allowed for user {}", requestedTenant, user.getName());
97-
return false;
98-
} else {
99-
100-
if (log.isDebugEnabled()) {
101-
log.debug("request " + request.getClass());
102-
}
103-
104-
if (tenants.get(requestedTenant) == Boolean.FALSE && !READ_ONLY_ALLOWED_ACTIONS.contains(action)) {
105-
log.warn("Tenant {} is not allowed to write (user: {})", requestedTenant, user.getName());
106-
return false;
107-
}
108-
}
109-
110-
return true;
111-
}
112-
11389
/**
11490
* return Boolean.TRUE to prematurely deny request
11591
* return Boolean.FALSE to prematurely allow request
@@ -123,7 +99,8 @@ public ReplaceResult replaceDashboardsIndex(
12399
final User user,
124100
final DynamicConfigModel config,
125101
final Resolved requestedResolved,
126-
final Map<String, Boolean> tenants
102+
final PrivilegesEvaluationContext context,
103+
final TenantPrivileges tenantPrivileges
127104
) {
128105

129106
final boolean enabled = config.isDashboardsMultitenancyEnabled();// config.dynamic.kibana.multitenancy_enabled;
@@ -154,20 +131,28 @@ public ReplaceResult replaceDashboardsIndex(
154131
final boolean dashboardsIndexOnly = !user.getName().equals(dashboardsServerUsername)
155132
&& resolveToDashboardsIndexOrAlias(requestedResolved, dashboardsIndexName);
156133
final boolean isTraceEnabled = log.isTraceEnabled();
134+
135+
TenantPrivileges.ActionType actionType = getActionTypeForAction(action);
136+
157137
if (requestedTenant == null || requestedTenant.length() == 0) {
158138
if (isTraceEnabled) {
159139
log.trace("No tenant, will resolve to " + dashboardsIndexName);
160140
}
161141

162-
if (dashboardsIndexOnly && !isTenantAllowed(request, action, user, tenants, "global_tenant")) {
142+
if (dashboardsIndexOnly && !tenantPrivileges.hasTenantPrivilege(context, "global_tenant", actionType)) {
163143
return ACCESS_DENIED_REPLACE_RESULT;
164144
}
165145

166146
return CONTINUE_EVALUATION_REPLACE_RESULT;
167147
}
168148

169-
if (USER_TENANT.equals(requestedTenant)) {
149+
boolean isPrivateTenant;
150+
151+
if (USER_TENANT.equals(requestedTenant) || user.getName().equals(requestedTenant)) {
170152
requestedTenant = user.getName();
153+
isPrivateTenant = true;
154+
} else {
155+
isPrivateTenant = false;
171156
}
172157

173158
if (isDebugEnabled && !user.getName().equals(dashboardsServerUsername)) {
@@ -181,7 +166,7 @@ public ReplaceResult replaceDashboardsIndex(
181166
final String tenantIndexName = toUserIndexName(dashboardsIndexName, requestedTenant);
182167
if (indices.size() == 1
183168
&& indices.iterator().next().startsWith(tenantIndexName)
184-
&& isTenantAllowed(request, action, user, tenants, requestedTenant)) {
169+
&& (isPrivateTenant || tenantPrivileges.hasTenantPrivilege(context, requestedTenant, actionType))) {
185170
return ACCESS_GRANTED_REPLACE_RESULT;
186171
}
187172
}
@@ -195,7 +180,7 @@ && isTenantAllowed(request, action, user, tenants, requestedTenant)) {
195180
log.debug("is user tenant: " + requestedTenant.equals(user.getName()));
196181
}
197182

198-
if (!isTenantAllowed(request, action, user, tenants, requestedTenant)) {
183+
if (!isPrivateTenant && !tenantPrivileges.hasTenantPrivilege(context, requestedTenant, actionType)) {
199184
return ACCESS_DENIED_REPLACE_RESULT;
200185
}
201186

@@ -238,6 +223,14 @@ private void applyDocumentAllowList(String indexName) {
238223
documentAllowList.applyTo(threadPool.getThreadContext());
239224
}
240225

226+
static TenantPrivileges.ActionType getActionTypeForAction(String action) {
227+
if (READ_ONLY_ALLOWED_ACTIONS.contains(action)) {
228+
return TenantPrivileges.ActionType.READ;
229+
} else {
230+
return TenantPrivileges.ActionType.WRITE;
231+
}
232+
}
233+
241234
private String getConcreteIndexName(String name, Map<String, IndexAbstraction> indicesLookup) {
242235
for (int i = 1; i < Integer.MAX_VALUE; i++) {
243236
String concreteName = name.concat("_" + i);

‎src/main/java/org/opensearch/security/dlic/rest/api/AccountApiAction.java‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,7 @@
3232
import org.opensearch.security.dlic.rest.validation.RequestContentValidator.DataType;
3333
import org.opensearch.security.dlic.rest.validation.ValidationResult;
3434
import org.opensearch.security.hasher.PasswordHasher;
35+
import org.opensearch.security.privileges.PrivilegesEvaluationContext;
3536
import org.opensearch.security.securityconf.Hashed;
3637
import org.opensearch.security.securityconf.impl.CType;
3738
import org.opensearch.security.securityconf.impl.SecurityDynamicConfiguration;
@@ -127,8 +128,7 @@ private void userAccount(
127128
final TransportAddress remoteAddress,
128129
final SecurityDynamicConfiguration<?> configuration
129130
) {
130-
final var securityRoles = securityApiDependencies.privilegesEvaluator().mapRoles(user, remoteAddress);
131-
131+
PrivilegesEvaluationContext context = securityApiDependencies.privilegesEvaluator().createContext(user, null);
132132
ok(
133133
channel,
134134
(builder, params) -> builder.startObject()
@@ -139,8 +139,8 @@ private void userAccount(
139139
.field("user_requested_tenant", user.getRequestedTenant())
140140
.field("backend_roles", user.getRoles())
141141
.field("custom_attribute_names", user.getCustomAttributesMap().keySet())
142-
.field("tenants", securityApiDependencies.privilegesEvaluator().mapTenants(user, securityRoles))
143-
.field("roles", securityRoles)
142+
.field("tenants", securityApiDependencies.privilegesEvaluator().tenantPrivileges().tenantMap(context))
143+
.field("roles", context.getMappedRoles())
144144
.endObject()
145145
);
146146
}

‎src/main/java/org/opensearch/security/privileges/PrivilegesEvaluator.java‎

Lines changed: 30 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -93,14 +93,14 @@
9393
import org.opensearch.security.resolver.IndexResolverReplacer;
9494
import org.opensearch.security.resolver.IndexResolverReplacer.Resolved;
9595
import org.opensearch.security.securityconf.ConfigModel;
96-
import org.opensearch.security.securityconf.DynamicConfigFactory;
9796
import org.opensearch.security.securityconf.DynamicConfigModel;
9897
import org.opensearch.security.securityconf.FlattenedActionGroups;
9998
import org.opensearch.security.securityconf.impl.CType;
10099
import org.opensearch.security.securityconf.impl.DashboardSignInOption;
101100
import org.opensearch.security.securityconf.impl.SecurityDynamicConfiguration;
102101
import org.opensearch.security.securityconf.impl.v7.ActionGroupsV7;
103102
import org.opensearch.security.securityconf.impl.v7.RoleV7;
103+
import org.opensearch.security.securityconf.impl.v7.TenantV7;
104104
import org.opensearch.security.support.ConfigConstants;
105105
import org.opensearch.security.support.WildcardMatcher;
106106
import org.opensearch.security.user.User;
@@ -157,6 +157,7 @@ public class PrivilegesEvaluator {
157157
private final Settings settings;
158158
private final Map<String, Set<String>> pluginToClusterActions;
159159
private final AtomicReference<ActionPrivileges> actionPrivileges = new AtomicReference<>();
160+
private final AtomicReference<TenantPrivileges> tenantPrivileges = new AtomicReference<>();
160161

161162
public PrivilegesEvaluator(
162163
final ClusterService clusterService,
@@ -200,16 +201,13 @@ public PrivilegesEvaluator(
200201

201202
if (configurationRepository != null) {
202203
configurationRepository.subscribeOnChange(configMap -> {
203-
try {
204-
SecurityDynamicConfiguration<ActionGroupsV7> actionGroupsConfiguration = configurationRepository.getConfiguration(
205-
CType.ACTIONGROUPS
206-
);
207-
SecurityDynamicConfiguration<RoleV7> rolesConfiguration = configurationRepository.getConfiguration(CType.ROLES);
208-
209-
this.updateConfiguration(actionGroupsConfiguration, rolesConfiguration);
210-
} catch (Exception e) {
211-
log.error("Error while updating ActionPrivileges object with {}", configMap, e);
212-
}
204+
SecurityDynamicConfiguration<ActionGroupsV7> actionGroupsConfiguration = configurationRepository.getConfiguration(
205+
CType.ACTIONGROUPS
206+
);
207+
SecurityDynamicConfiguration<RoleV7> rolesConfiguration = configurationRepository.getConfiguration(CType.ROLES);
208+
SecurityDynamicConfiguration<TenantV7> tenantConfiguration = configurationRepository.getConfiguration(CType.TENANTS);
209+
210+
this.updateConfiguration(actionGroupsConfiguration, rolesConfiguration, tenantConfiguration);
213211
});
214212
}
215213

@@ -226,15 +224,15 @@ public PrivilegesEvaluator(
226224

227225
void updateConfiguration(
228226
SecurityDynamicConfiguration<ActionGroupsV7> actionGroupsConfiguration,
229-
SecurityDynamicConfiguration<RoleV7> rolesConfiguration
227+
SecurityDynamicConfiguration<RoleV7> rolesConfiguration,
228+
SecurityDynamicConfiguration<TenantV7> tenantConfiguration
230229
) {
231-
if (rolesConfiguration != null) {
232-
SecurityDynamicConfiguration<ActionGroupsV7> actionGroupsWithStatics = actionGroupsConfiguration != null
233-
? DynamicConfigFactory.addStatics(actionGroupsConfiguration.clone())
234-
: DynamicConfigFactory.addStatics(SecurityDynamicConfiguration.empty(CType.ACTIONGROUPS));
235-
FlattenedActionGroups flattenedActionGroups = new FlattenedActionGroups(actionGroupsWithStatics);
230+
FlattenedActionGroups flattenedActionGroups = new FlattenedActionGroups(actionGroupsConfiguration.withStaticConfig());
231+
rolesConfiguration = rolesConfiguration.withStaticConfig();
232+
tenantConfiguration = tenantConfiguration.withStaticConfig();
233+
try {
236234
ActionPrivileges actionPrivileges = new ActionPrivileges(
237-
DynamicConfigFactory.addStatics(rolesConfiguration.clone()),
235+
rolesConfiguration,
238236
flattenedActionGroups,
239237
() -> clusterStateSupplier.get().metadata().getIndicesLookup(),
240238
settings,
@@ -247,6 +245,14 @@ void updateConfiguration(
247245
if (oldInstance != null) {
248246
oldInstance.shutdown();
249247
}
248+
} catch (Exception e) {
249+
log.error("Error while updating ActionPrivileges", e);
250+
}
251+
252+
try {
253+
this.tenantPrivileges.set(new TenantPrivileges(rolesConfiguration, tenantConfiguration, flattenedActionGroups));
254+
} catch (Exception e) {
255+
log.error("Error while updating TenantPrivileges", e);
250256
}
251257
}
252258

@@ -455,7 +461,8 @@ public PrivilegesEvaluatorResponse evaluate(PrivilegesEvaluationContext context)
455461
user,
456462
dcm,
457463
requestedResolved,
458-
mapTenants(context)
464+
context,
465+
this.tenantPrivileges.get()
459466
);
460467

461468
if (isDebugEnabled) {
@@ -517,7 +524,8 @@ public PrivilegesEvaluatorResponse evaluate(PrivilegesEvaluationContext context)
517524
user,
518525
dcm,
519526
requestedResolved,
520-
mapTenants(context)
527+
context,
528+
this.tenantPrivileges.get()
521529
);
522530

523531
if (isDebugEnabled) {
@@ -594,19 +602,8 @@ public Set<String> mapRoles(final User user, final TransportAddress caller) {
594602
return this.configModel.mapSecurityRoles(user, caller);
595603
}
596604

597-
public Map<String, Boolean> mapTenants(PrivilegesEvaluationContext privilegesEvaluationContext) {
598-
return this.configModel.mapTenants(privilegesEvaluationContext);
599-
}
600-
601-
public Map<String, Boolean> mapTenants(User user, Set<String> mappedRoles) {
602-
return this.configModel.mapTenants(
603-
new PrivilegesEvaluationContext(user, ImmutableSet.copyOf(mappedRoles), null, null, null, irr, resolver, clusterStateSupplier)
604-
);
605-
}
606-
607-
public Set<String> getAllConfiguredTenantNames() {
608-
609-
return configModel.getAllConfiguredTenantNames();
605+
public TenantPrivileges tenantPrivileges() {
606+
return this.tenantPrivileges.get();
610607
}
611608

612609
public boolean multitenancyEnabled() {

‎src/main/java/org/opensearch/security/privileges/PrivilegesInterceptor.java‎

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -26,8 +26,6 @@
2626

2727
package org.opensearch.security.privileges;
2828

29-
import java.util.Map;
30-
3129
import org.opensearch.action.ActionRequest;
3230
import org.opensearch.action.admin.indices.create.CreateIndexRequestBuilder;
3331
import org.opensearch.cluster.metadata.IndexNameExpressionResolver;
@@ -84,7 +82,8 @@ public ReplaceResult replaceDashboardsIndex(
8482
final User user,
8583
final DynamicConfigModel config,
8684
final Resolved requestedResolved,
87-
final Map<String, Boolean> tenants
85+
final PrivilegesEvaluationContext context,
86+
final TenantPrivileges tenantPrivileges
8887
) {
8988
throw new RuntimeException("not implemented");
9089
}

0 commit comments

Comments
 (0)