Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Subset of permissions check on creation #5012

Open
wants to merge 24 commits into
base: feature/api-tokens
Choose a base branch
from
Open

Subset of permissions check on creation #5012

Show file tree
Hide file tree
Changes from 22 commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
9f695fa
Naive cluster permission authz and authc based on token validity
derek-ho Dec 24, 2024
d3fcc4a
Crude index permissions authz
derek-ho Dec 24, 2024
6904317
Fix tests
derek-ho Dec 26, 2024
17bca93
Revert mis-merge in abstractauditlog
derek-ho Dec 26, 2024
92d4e60
Add allowlist for authc, add basic test showing it works
derek-ho Dec 27, 2024
22cfbe8
Add more extensive tests for authenticator, switch to list of indexPe…
derek-ho Dec 27, 2024
665b9e9
Directly store permissions in the cache
derek-ho Dec 30, 2024
e39df0d
Remove permissions from jti
derek-ho Dec 30, 2024
ad63974
Onboard onto clusterPrivileges
derek-ho Dec 30, 2024
73eb2ab
Add index permissions api token eval
derek-ho Dec 31, 2024
6418226
Add testing for cluster and index priv
derek-ho Dec 31, 2024
bc8aacf
Use transport action
derek-ho Jan 6, 2025
b90bae9
Cleanup tests and constants
derek-ho Jan 6, 2025
552aeda
Fix test
derek-ho Jan 6, 2025
aa506e7
Remove unecessary id to jti map since we are reloading every time and…
derek-ho Jan 6, 2025
8299645
Add permission checks around creation
derek-ho Jan 7, 2025
eebe5fb
Add tests
derek-ho Jan 8, 2025
c079d09
Clean up TODOs
derek-ho Jan 8, 2025
5b48fb9
Fix logic
derek-ho Jan 9, 2025
e13c055
Fix test and clean up code
derek-ho Jan 9, 2025
a1057ce
Merge branch 'feature/api-tokens' of github.com:opensearch-project/se…
derek-ho Feb 4, 2025
b6aa196
Remove unecessary file changes
derek-ho Feb 4, 2025
bf31791
Add alias support
derek-ho Feb 6, 2025
c95d256
remove permission validation
derek-ho Feb 11, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 2 additions & 4 deletions src/integrationTest/java/org/opensearch/security/privileges/ActionPrivilegesTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,6 @@
import org.opensearch.core.common.unit.ByteSizeUnit;
import org.opensearch.core.common.unit.ByteSizeValue;
import org.opensearch.security.action.apitokens.ApiToken;
import org.opensearch.security.action.apitokens.ApiTokenRepository;
import org.opensearch.security.action.apitokens.Permissions;
import org.opensearch.security.resolver.IndexResolverReplacer;
import org.opensearch.security.securityconf.FlattenedActionGroups;
Expand All @@ -50,9 +49,8 @@
import org.opensearch.security.user.User;
import org.opensearch.security.util.MockIndexMetadataBuilder;

import org.mockito.Mockito;

import static org.hamcrest.MatcherAssert.assertThat;
import static org.opensearch.security.privileges.ActionPrivilegesTest.IndexPrivileges.IndicesAndAliases.resolved;
import static org.opensearch.security.privileges.PrivilegeEvaluatorResponseMatcher.isAllowed;
import static org.opensearch.security.privileges.PrivilegeEvaluatorResponseMatcher.isForbidden;
import static org.opensearch.security.privileges.PrivilegeEvaluatorResponseMatcher.isPartiallyOk;
Expand Down Expand Up @@ -342,6 +340,7 @@ public void apiToken_succeedsWithActionGroupsExapnded() throws Exception {
"apitoken:" + token,
new Permissions(List.of("CLUSTER_ALL"), List.of())
);

// Explicit succeeds
assertThat(subject.hasExplicitClusterPrivilege(context, "cluster:whatever"), isAllowed());
// Not explicit succeeds
Expand Down Expand Up @@ -1152,7 +1151,6 @@ static RoleBasedPrivilegesEvaluationContext ctx(String... roles) {
static RoleBasedPrivilegesEvaluationContext ctxWithUserName(String userName, String... roles) {
User user = new User(userName);
user.addAttributes(ImmutableMap.of("attrs.dept_no", "a11"));
ApiTokenRepository mockRepository = Mockito.mock(ApiTokenRepository.class);
return new RoleBasedPrivilegesEvaluationContext(
user,
ImmutableSet.copyOf(roles),
Expand Down
30 changes: 23 additions & 7 deletions src/main/java/org/opensearch/security/OpenSearchSecurityPlugin.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -259,7 +259,7 @@ public final class OpenSearchSecurityPlugin extends OpenSearchSecuritySSLPlugin
private volatile UserService userService;
private volatile RestLayerPrivilegesEvaluator restLayerEvaluator;
private volatile ConfigurationRepository cr;
private volatile ApiTokenRepository ar;
private volatile ApiTokenRepository apiTokenRepository;
private volatile AdminDNs adminDns;
private volatile ClusterService cs;
private volatile AtomicReference<DiscoveryNode> localNode = new AtomicReference<>();
Expand Down Expand Up @@ -649,7 +649,19 @@ public List<RestHandler> getRestHandlers(
)
);
handlers.add(new CreateOnBehalfOfTokenAction(tokenManager));
handlers.add(new ApiTokenAction(ar));
handlers.add(
new ApiTokenAction(
Objects.requireNonNull(threadPool),
cr,
evaluator,
settings,
adminDns,
auditLog,
configPath,
principalExtractor,
apiTokenRepository
)
);
handlers.addAll(
SecurityRestApiActions.getHandler(
settings,
Expand Down Expand Up @@ -1111,7 +1123,7 @@ public Collection<Object> createComponents(
final XFFResolver xffResolver = new XFFResolver(threadPool);
backendRegistry = new BackendRegistry(settings, adminDns, xffResolver, auditLog, threadPool);
tokenManager = new SecurityTokenManager(cs, threadPool, userService);
ar = new ApiTokenRepository(localClient, clusterService, tokenManager);
apiTokenRepository = new ApiTokenRepository(localClient, clusterService, tokenManager);

final CompatConfig compatConfig = new CompatConfig(environment, transportPassiveAuthSetting);

Expand All @@ -1128,7 +1140,7 @@ public Collection<Object> createComponents(
cih,
irr,
namedXContentRegistry.get(),
ar
apiTokenRepository
);

dlsFlsBaseContext = new DlsFlsBaseContext(evaluator, threadPool.getThreadContext(), adminDns);
Expand Down Expand Up @@ -1170,7 +1182,7 @@ public Collection<Object> createComponents(
configPath,
compatConfig
);
dcf = new DynamicConfigFactory(cr, settings, configPath, localClient, threadPool, cih, passwordHasher, ar);
dcf = new DynamicConfigFactory(cr, settings, configPath, localClient, threadPool, cih, passwordHasher, apiTokenRepository);
dcf.registerDCFListener(backendRegistry);
dcf.registerDCFListener(compatConfig);
dcf.registerDCFListener(irr);
Expand Down Expand Up @@ -1220,7 +1232,7 @@ public Collection<Object> createComponents(
components.add(dcf);
components.add(userService);
components.add(passwordHasher);
components.add(ar);
components.add(apiTokenRepository);

components.add(sslSettingsManager);
if (isSslCertReloadEnabled(settings) && sslCertificatesHotReloadEnabled(settings)) {
Expand Down Expand Up @@ -2143,7 +2155,11 @@ public Collection<SystemIndexDescriptor> getSystemIndexDescriptors(Settings sett
ConfigConstants.OPENDISTRO_SECURITY_DEFAULT_CONFIG_INDEX
);
final SystemIndexDescriptor systemIndexDescriptor = new SystemIndexDescriptor(indexPattern, "Security index");
return Collections.singletonList(systemIndexDescriptor);
final SystemIndexDescriptor apiTokenSystemIndexDescriptor = new SystemIndexDescriptor(
ConfigConstants.OPENSEARCH_API_TOKENS_INDEX,
"Security API token index"
);
return List.of(systemIndexDescriptor, apiTokenSystemIndexDescriptor);
}

@Override
Expand Down
202 changes: 178 additions & 24 deletions src/main/java/org/opensearch/security/action/apitokens/ApiTokenAction.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,26 +12,53 @@
package org.opensearch.security.action.apitokens;

import java.io.IOException;
import java.nio.file.Path;
import java.time.Instant;
import java.util.Collections;
import java.util.HashSet;
import java.util.List;
import java.util.Map;
import java.util.Set;
import java.util.concurrent.TimeUnit;
import java.util.stream.Collectors;

import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableSet;
import org.apache.logging.log4j.LogManager;
import org.apache.logging.log4j.Logger;

import org.opensearch.OpenSearchException;
import org.opensearch.client.node.NodeClient;
import org.opensearch.common.settings.Settings;
import org.opensearch.common.util.concurrent.ThreadContext;
import org.opensearch.core.action.ActionListener;
import org.opensearch.core.common.transport.TransportAddress;
import org.opensearch.core.rest.RestStatus;
import org.opensearch.core.xcontent.XContentBuilder;
import org.opensearch.rest.BaseRestHandler;
import org.opensearch.rest.BytesRestResponse;
import org.opensearch.rest.RestChannel;
import org.opensearch.rest.RestHandler;
import org.opensearch.rest.RestRequest;
import org.opensearch.security.auditlog.AuditLog;
import org.opensearch.security.configuration.AdminDNs;
import org.opensearch.security.configuration.ConfigurationRepository;
import org.opensearch.security.dlic.rest.api.Endpoint;
import org.opensearch.security.dlic.rest.api.RestApiAdminPrivilegesEvaluator;
import org.opensearch.security.dlic.rest.api.RestApiPrivilegesEvaluator;
import org.opensearch.security.dlic.rest.api.SecurityApiDependencies;
import org.opensearch.security.dlic.rest.support.Utils;
import org.opensearch.security.privileges.PrivilegesEvaluator;
import org.opensearch.security.securityconf.DynamicConfigFactory;
import org.opensearch.security.securityconf.FlattenedActionGroups;
import org.opensearch.security.securityconf.impl.CType;
import org.opensearch.security.securityconf.impl.SecurityDynamicConfiguration;
import org.opensearch.security.securityconf.impl.v7.ActionGroupsV7;
import org.opensearch.security.securityconf.impl.v7.RoleV7;
import org.opensearch.security.ssl.transport.PrincipalExtractor;
import org.opensearch.security.support.ConfigConstants;
import org.opensearch.security.support.WildcardMatcher;
import org.opensearch.security.user.User;
import org.opensearch.threadpool.ThreadPool;

import static org.opensearch.rest.RestRequest.Method.DELETE;
import static org.opensearch.rest.RestRequest.Method.GET;
Expand All @@ -44,23 +71,51 @@
import static org.opensearch.security.action.apitokens.ApiToken.INDEX_PERMISSIONS_FIELD;
import static org.opensearch.security.action.apitokens.ApiToken.NAME_FIELD;
import static org.opensearch.security.dlic.rest.support.Utils.addRoutesPrefix;
import static org.opensearch.security.support.ConfigConstants.SECURITY_RESTAPI_ADMIN_ENABLED;
import static org.opensearch.security.util.ParsingUtils.safeMapList;
import static org.opensearch.security.util.ParsingUtils.safeStringList;

public class ApiTokenAction extends BaseRestHandler {
private ApiTokenRepository apiTokenRepository;
private final ApiTokenRepository apiTokenRepository;
public Logger log = LogManager.getLogger(this.getClass());
private final ThreadPool threadPool;
private final ConfigurationRepository configurationRepository;
private final PrivilegesEvaluator privilegesEvaluator;
private final SecurityApiDependencies securityApiDependencies;

private static final List<RestHandler.Route> ROUTES = addRoutesPrefix(
ImmutableList.of(
new RestHandler.Route(POST, "/apitokens"),
new RestHandler.Route(DELETE, "/apitokens"),
new RestHandler.Route(GET, "/apitokens")
)
private static final List<Route> ROUTES = addRoutesPrefix(
ImmutableList.of(new Route(POST, "/apitokens"), new Route(DELETE, "/apitokens"), new Route(GET, "/apitokens"))
);

public ApiTokenAction(ApiTokenRepository apiTokenRepository) {
public ApiTokenAction(
ThreadPool threadpool,
ConfigurationRepository configurationRepository,
PrivilegesEvaluator privilegesEvaluator,
Settings settings,
AdminDNs adminDns,
AuditLog auditLog,
Path configPath,
PrincipalExtractor principalExtractor,
ApiTokenRepository apiTokenRepository
) {
this.apiTokenRepository = apiTokenRepository;
this.threadPool = threadpool;
this.configurationRepository = configurationRepository;
this.privilegesEvaluator = privilegesEvaluator;
this.securityApiDependencies = new SecurityApiDependencies(
adminDns,
configurationRepository,
privilegesEvaluator,
new RestApiPrivilegesEvaluator(settings, adminDns, privilegesEvaluator, principalExtractor, configPath, threadPool),
new RestApiAdminPrivilegesEvaluator(
threadPool.getThreadContext(),
privilegesEvaluator,
adminDns,
settings.getAsBoolean(SECURITY_RESTAPI_ADMIN_ENABLED, false)
),
auditLog,
settings
);
}

@Override
Expand All @@ -69,22 +124,28 @@
}

@Override
public List<RestHandler.Route> routes() {
public List<Route> routes() {
return ROUTES;
}

@Override
protected RestChannelConsumer prepareRequest(final RestRequest request, final NodeClient client) throws IOException {
// TODO: Authorize this API properly
switch (request.method()) {
case POST:
return handlePost(request, client);
case DELETE:
return handleDelete(request, client);
case GET:
return handleGet(request, client);
default:
throw new IllegalArgumentException(request.method() + " not supported");
authorizeSecurityAccess(request);
return doPrepareRequest(request, client);

Check warning on line 134 in src/main/java/org/opensearch/security/action/apitokens/ApiTokenAction.java

View check run for this annotation

Codecov / codecov/patch

src/main/java/org/opensearch/security/action/apitokens/ApiTokenAction.java#L133-L134 

Added lines #L133 - L134 were not covered by tests
}

RestChannelConsumer doPrepareRequest(RestRequest request, NodeClient client) {
final var originalUserAndRemoteAddress = Utils.userAndRemoteAddressFrom(client.threadPool().getThreadContext());
try (final ThreadContext.StoredContext ctx = client.threadPool().getThreadContext().stashContext()) {
client.threadPool()
.getThreadContext()
.putTransient(ConfigConstants.OPENDISTRO_SECURITY_USER, originalUserAndRemoteAddress.getLeft());

Check warning on line 142 in src/main/java/org/opensearch/security/action/apitokens/ApiTokenAction.java

View check run for this annotation

Codecov / codecov/patch

src/main/java/org/opensearch/security/action/apitokens/ApiTokenAction.java#L138-L142 

Added lines #L138 - L142 were not covered by tests
return switch (request.method()) {
case POST -> handlePost(request, client);
case DELETE -> handleDelete(request, client);
case GET -> handleGet(request, client);
default -> throw new IllegalArgumentException(request.method() + " not supported");

Check warning on line 147 in src/main/java/org/opensearch/security/action/apitokens/ApiTokenAction.java

View check run for this annotation

Codecov / codecov/patch

src/main/java/org/opensearch/security/action/apitokens/ApiTokenAction.java#L144-L147 

Added lines #L144 - L147 were not covered by tests
};
}
}

Expand Down Expand Up @@ -119,15 +180,15 @@

private RestChannelConsumer handlePost(RestRequest request, NodeClient client) {
return channel -> {
final XContentBuilder builder = channel.newBuilder();
BytesRestResponse response;
try {
final Map<String, Object> requestBody = request.contentOrSourceParamParser().map();
validateRequestParameters(requestBody);

List<String> clusterPermissions = extractClusterPermissions(requestBody);
List<ApiToken.IndexPermission> indexPermissions = extractIndexPermissions(requestBody);

validateUserPermissions(clusterPermissions, indexPermissions);

Check warning on line 190 in src/main/java/org/opensearch/security/action/apitokens/ApiTokenAction.java

View check run for this annotation

Codecov / codecov/patch

src/main/java/org/opensearch/security/action/apitokens/ApiTokenAction.java#L190 

Added line #L190 was not covered by tests

String token = apiTokenRepository.createApiToken(
(String) requestBody.get(NAME_FIELD),
clusterPermissions,
Expand Down Expand Up @@ -245,8 +306,6 @@

private RestChannelConsumer handleDelete(RestRequest request, NodeClient client) {
return channel -> {
final XContentBuilder builder = channel.newBuilder();
BytesRestResponse response;
try {
final Map<String, Object> requestBody = request.contentOrSourceParamParser().map();

Expand Down Expand Up @@ -295,4 +354,99 @@
}
}

/**
* Validates that the user has the required permissions to create an API token (must be a subset of their own permissions)
* */
@SuppressWarnings("unchecked")
void validateUserPermissions(List<String> clusterPermissions, List<ApiToken.IndexPermission> indexPermissions) {
derek-ho marked this conversation as resolved.
Show resolved Hide resolved
final User user = threadPool.getThreadContext().getTransient(ConfigConstants.OPENDISTRO_SECURITY_USER);
final TransportAddress caller = threadPool.getThreadContext().getTransient(ConfigConstants.OPENDISTRO_SECURITY_REMOTE_ADDRESS);
final Set<String> roles = privilegesEvaluator.mapRoles(user, caller);

// Early return conditions
if (roles.isEmpty()) {
throw new OpenSearchException("User does not have any roles");

Check warning on line 368 in src/main/java/org/opensearch/security/action/apitokens/ApiTokenAction.java

View check run for this annotation

Codecov / codecov/patch

src/main/java/org/opensearch/security/action/apitokens/ApiTokenAction.java#L368 

Added line #L368 was not covered by tests
} else if (roles.contains("all_access")) {
derek-ho marked this conversation as resolved.
Show resolved Hide resolved
// all_access == *
return;

Check warning on line 371 in src/main/java/org/opensearch/security/action/apitokens/ApiTokenAction.java

View check run for this annotation

Codecov / codecov/patch

src/main/java/org/opensearch/security/action/apitokens/ApiTokenAction.java#L371 

Added line #L371 was not covered by tests
}

// Verify user has all requested cluster permissions
final SecurityDynamicConfiguration<ActionGroupsV7> actionGroupsConfiguraiton = (SecurityDynamicConfiguration<ActionGroupsV7>) load(
CType.ACTIONGROUPS
);
FlattenedActionGroups flattenedActionGroups = new FlattenedActionGroups(actionGroupsConfiguraiton);
final SecurityDynamicConfiguration<?> rolesConfiguration = load(CType.ROLES);
ImmutableSet<String> resolvedClusterPermissions = flattenedActionGroups.resolve(clusterPermissions);
Set<String> clusterPermissionsWithoutActionGroups = resolvedClusterPermissions.stream()
.filter(permission -> !actionGroupsConfiguraiton.getCEntries().containsKey(permission))
.collect(Collectors.toSet());

// Load all roles the user has access to, remove permissions that
for (String role : roles) {
RoleV7 roleV7 = (RoleV7) rolesConfiguration.getCEntry(role);
ImmutableSet<String> expandedRoleClusterPermissions = flattenedActionGroups.resolve(roleV7.getCluster_permissions());
for (String clusterPermission : expandedRoleClusterPermissions) {
WildcardMatcher matcher = WildcardMatcher.from(clusterPermission);
clusterPermissionsWithoutActionGroups.removeIf(matcher);
}
}

if (!clusterPermissionsWithoutActionGroups.isEmpty()) {
throw new OpenSearchException("User does not have all requested cluster permissions");
}

// Verify user has all requested index permissions
for (ApiToken.IndexPermission requestedPermission : indexPermissions) {
// First, flatten/resolve any action groups into the underlying actions and remove action group names (which may not exact
// match)
Set<String> resolvedActions = new HashSet<>(flattenedActionGroups.resolve(requestedPermission.getAllowedActions()));
resolvedActions.removeIf(permission -> actionGroupsConfiguraiton.getCEntries().containsKey(permission));

// For each index pattern in the requested permission
for (String requestedPattern : requestedPermission.getIndexPatterns()) {

Set<String> actionsForIndexPattern = new HashSet<>(resolvedActions);

// Check each role the user has
for (String roleName : roles) {
RoleV7 role = (RoleV7) rolesConfiguration.getCEntry(roleName);
if (role == null || role.getIndex_permissions() == null) continue;

// Check each index permission block in the role
for (RoleV7.Index indexPermission : role.getIndex_permissions()) {
List<String> rolePatterns = indexPermission.getIndex_patterns();
List<String> roleIndexPerms = indexPermission.getAllowed_actions();

// Check if this role's pattern covers the requested pattern
if (WildcardMatcher.from(rolePatterns).test(requestedPattern)) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Assume that I have a role with the index_pattern /index_[0-9]+/. How can a requestedPattern look like that replicates this pattern?

At the moment, I have doubts that simple pattern matching can be used to support the full pattern semantics.

Copy link
Collaborator

Choose a reason for hiding this comment

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

This also does not cover alias semantics.

Copy link
Member

@cwperks cwperks Feb 5, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Assume that I have a role with the index_pattern /index_[0-9]+/. How can a requestedPattern look like that replicates this pattern?

Do role definitions support this? I have only seen this in a context of masked fields

Edit: Found an example: https://github.com/opensearch-project/security/blob/main/src/test/resources/roles.yml#L20-L33

Edit2: Is that the purpose of the renderedMatcher?

Looks like this is handled by WildcardMatcher itself.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added alias support in this commit: bf31791. I also shimmed in a way to not pass in a privilegesEvaluationContext into the indexpattern class, as long as you pass in the used components out of the context. What do y'all think of this change?

Copy link
Member

Choose a reason for hiding this comment

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

This also does not cover alias semantics.

How would this not cover aliases? i.e. if a user has a role mapping with read access to logs and requests a token with read access to logs (assuming logs is an alias) won't this work?

Agree on regexp support here, @derek-ho can we throw an IllegalArgumentException if the requested permission is for a regexp?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think it doesn't cover alias support for underlying indices, i.e. Users with access to logs alias would not be able to request api tokens for a subset of any of the underlying indices, even when they have permission to do so.

Copy link
Collaborator

@nibix nibix Feb 10, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Regarding the regexp issue:

I think, one of the fundamental issues here is the following:

Normal users generally do not have access to the roles.yml configuration. Thus, they do not know how they gain access to the indices - they just know that they have access to the indices they can observe.

Thus, they need to resort to trial-and-error process to see whether they can gain API tokens to an index or whether the roles.yml definition uses a construct that is not supported.

Obviously, this is sub-optimal from a UX point of view.

Ways around that issue - just from the top of my head:

  • Just support API token creation for admin users only. These have full access anyway, and all the checks for intersection become unnecessary (as described in Subset of permissions check on creation #5012 (comment)).
  • Just support API token creation for concrete index names, not for patterns. Then you do not have to match regexes against regexes.
  • Move the process of determining the intersection of requested privileges and available privileges from the token creation to the privilege evaluation phase. Thus, one would change the check "does roles.yml provide privileges" into "does roles.yml provide privileges AND does the api token request the privileges". This would also solve - to a certain extent - the issue of changes to roles.yml.

Copy link
Member

Choose a reason for hiding this comment

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

Obviously, this is sub-optimal from a UX point of view.

Can't this be handled another way though? i.e. prevent using tokens entirely if any role uses regex in index_permissions or skip the subset check for any role using regexes?

One other way this could be handled is creating an API that a user can call that helps them comprehend what actions and index patterns they are able to select. This API could also be used to power UX for the feature.

Just support API token creation for concrete index names, not for patterns. Then you do not have to match regexes against regexes.

What about glob-like patterns?

Copy link
Collaborator

@nibix nibix Feb 12, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

What about glob-like patterns?

It is quite a brain-twister to think about matching patterns against patterns 😅 .. but I thought this through again, and there might be another issue.

Consider:

  • I have in roles.yml privileges for the pattern index_1?. This gives me privileges on index_10, index_11, etc., but not on index_1.

  • I now try to request an API token for the index pattern index_1*. The pattern index_1? will positively match index_1*. The * character will be bound to the ?. However, the pattern index_1* is broader than index_1?. It will also match index_1, which was not matched by index_1?. Thus, we would have a privilege escalation vulnerability.

Of course, this might be quite a theoretical issue where its real world applicability is questionable. Still, IMHO, security software should be also safe of such scenarios.

Thus, I would strongly recommend against matching any patterns against patterns. It is just too fragile.

I would still recommend the approach sketched out above:

Move the process of determining the intersection of requested privileges and available privileges from the token creation to the privilege evaluation phase. Thus, one would change the check "does roles.yml provide privileges" into "does roles.yml provide privileges AND does the api token request the privileges". This would also solve - to a certain extent - the issue of changes to roles.yml.

// Get resolved actions for this role's index permissions
Set<String> roleActions = flattenedActionGroups.resolve(roleIndexPerms);
WildcardMatcher matcher = WildcardMatcher.from(roleActions);

actionsForIndexPattern.removeIf(matcher);
}
}
}

// After checking all roles, verify if all requested actions were covered
if (!actionsForIndexPattern.isEmpty()) {
throw new OpenSearchException("User does not have sufficient permissions for index pattern: " + requestedPattern);
}
}
}
}

private SecurityDynamicConfiguration<?> load(final CType<?> config) {
SecurityDynamicConfiguration<?> loaded = configurationRepository.getConfiguration(config);
return DynamicConfigFactory.addStatics(loaded);
}

protected void authorizeSecurityAccess(RestRequest request) throws IOException {
Copy link
Collaborator

Choose a reason for hiding this comment

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

What do you think about having a dedicated transport action backing the REST action? Then, one would get access control out of the box without needing explicit checks within the action code.

Copy link
Member

Choose a reason for hiding this comment

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

Agree with this though it depends on the action.

If the security admin is going to a page in the security dashboards plugin to manage the existing tokens (listing them and performing actions such as revoke) then these should follow the authorization model of other security APIs.

If regular users are allowed to request a token (with a subset of their own permissions) then that could be authorized like other cluster actions in OpenSearch.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think (for now) my operating assumption would be that this is an admin-level action to issue api tokens to connect with other services programmatically. Since API tokens (currently) do not follow the new optimized privileges evaluation, and thus have higher performance penalty than other means of authc/z, and thus we should restrict this as much as possible. However, if there is community feedback/requests for this feature, then we can consider it at a later time. Since we want to authorize these APIs like the others, I added this logic.

Copy link
Collaborator

Choose a reason for hiding this comment

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

To be honest, it is a bit difficult for me to review this without a proper scope definition. During review, I learned from you about a couple of assumptions and assumed limitations (such as not using the optimized privilege evaluation code paths, not supporting regexes, etc). IMHO, there should be some definitions in the issue #1504 what's the initial goal to be achieved and what not. That will be then an information to guide the code review process. Otherwise, this review process will get quite inefficient.

If the assumption is indeed that issuing tokens is an admin-level action, I do wonder why we need the matching to existing privileges at all. Won't an admin-level action just be similar to granting new privileges?

Copy link
Member

@cwperks cwperks Feb 10, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

If its rolled out to admin users initially at first than I agree, but if the longer term plan is for other users to be able to request tokens then the logic to determine whether the requested permissions are a subset of the user's permissions would be needed. I was thinking to keep the logic simple and only consider: 1) cluster_permissions and 2) index_permissions. For index permissions, would it be sufficient to limit the checks to concrete indices and glob-like patterns and prohibit regexes?

In practice, I have not come across many roles definitions where cluster administrators use regex when defining the index patterns in a role. I'm sure its used, but not prevalent from my experience.

// Check if user has security API access
if (!(securityApiDependencies.restApiAdminPrivilegesEvaluator().isCurrentUserAdminFor(Endpoint.APITOKENS)
|| securityApiDependencies.restApiPrivilegesEvaluator().checkAccessPermissions(request, Endpoint.APITOKENS) == null)) {
throw new SecurityException("User does not have required security API access");

Check warning on line 449 in src/main/java/org/opensearch/security/action/apitokens/ApiTokenAction.java

View check run for this annotation

Codecov / codecov/patch

src/main/java/org/opensearch/security/action/apitokens/ApiTokenAction.java#L449 

Added line #L449 was not covered by tests
}
}

Check warning on line 451 in src/main/java/org/opensearch/security/action/apitokens/ApiTokenAction.java

View check run for this annotation

Codecov / codecov/patch

src/main/java/org/opensearch/security/action/apitokens/ApiTokenAction.java#L451 

Added line #L451 was not covered by tests
}
Loading
Loading