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

Merged
merged 26 commits into from
Feb 21, 2025
Merged

Subset of permissions check on creation #5012

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
26 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
1314851
Revert refactor
derek-ho Feb 20, 2025
280e00c
Unecessary stubbings
derek-ho Feb 21, 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
32 changes: 25 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,21 @@ 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,
cs,
indexNameExpressionResolver
)
);
handlers.addAll(
SecurityRestApiActions.getHandler(
settings,
Expand Down Expand Up @@ -1111,7 +1125,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 +1142,7 @@ public Collection<Object> createComponents(
cih,
irr,
namedXContentRegistry.get(),
ar
apiTokenRepository
);

dlsFlsBaseContext = new DlsFlsBaseContext(evaluator, threadPool.getThreadContext(), adminDns);
Expand Down Expand Up @@ -1170,7 +1184,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 +1234,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 +2157,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);
Copy link
Member

Choose a reason for hiding this comment

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

nit: we can start creating these inline.

}

@Override
Expand Down
109 changes: 84 additions & 25 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,6 +12,7 @@
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.List;
Expand All @@ -24,14 +25,29 @@
import org.apache.logging.log4j.Logger;

import org.opensearch.client.node.NodeClient;
import org.opensearch.cluster.metadata.IndexNameExpressionResolver;
import org.opensearch.cluster.service.ClusterService;
import org.opensearch.common.settings.Settings;
import org.opensearch.common.util.concurrent.ThreadContext;
import org.opensearch.core.action.ActionListener;
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.ssl.transport.PrincipalExtractor;
import org.opensearch.security.support.ConfigConstants;
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 +60,57 @@
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 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 final ThreadPool threadPool;
private final ConfigurationRepository configurationRepository;
private final PrivilegesEvaluator privilegesEvaluator;
private final SecurityApiDependencies securityApiDependencies;
private final ClusterService clusterService;
private final IndexNameExpressionResolver indexNameExpressionResolver;

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,
ClusterService clusterService,
IndexNameExpressionResolver indexNameExpressionResolver
) {
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
);
this.clusterService = clusterService;
this.indexNameExpressionResolver = indexNameExpressionResolver;
}

@Override
Expand All @@ -69,22 +119,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 129 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#L128-L129 

Added lines #L128 - L129 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 137 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-L137 

Added lines #L133 - L137 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 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#L139-L142 

Added lines #L139 - L142 were not covered by tests
};
}
}

Expand Down Expand Up @@ -119,8 +175,6 @@

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);
Expand Down Expand Up @@ -245,8 +299,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 +347,11 @@
}
}

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.

Copy link
Member

Choose a reason for hiding this comment

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

Should we consider following a similar model to AbstractApiAction here?

I know this action is not a subset of AbstractApiAction (since it doesn't alter anything in the security index), but maybe it can be modeled similarly? Part of that block also registers an updateHandler to synchronize across the cluster.

// 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 354 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#L354 

Added line #L354 was not covered by tests
}
}

Check warning on line 356 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#L356 

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