Skip to content

Commit

Permalink
Support security config updates on the REST API using permission (#3264)
Browse files Browse the repository at this point in the history 
Instead of setting
`SECURITY_UNSUPPORTED_RESTAPI_ALLOW_SECURITYCONFIG_MODIFICATION`
settings to update security configuration using `PATCH` or `PUT` a new
permission was added: `restapi:admin/config/update`.

So far I decided to keep this flag as it is due to a backward
compatibility and log a deprecation message that these settings will be
removed in the future. Maybe it is better to remove it completely.

Besides, added the missed test for `SecurityConfigApiAction`

Signed-off-by: Andrey Pleskach <[email protected]>
(cherry picked from commit e4a1b77)
Signed-off-by: Andrey Pleskach <[email protected]>
  • Loading branch information
@willyborankin
willyborankin committed Oct 4, 2023
1 parent 59e0e82 commit 9480a22
Show file tree
Hide file tree
Showing 14 changed files with 326 additions and 106 deletions.
1 change: 1 addition & 0 deletions config/roles.yml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ security_rest_api_full_access:
cluster_permissions:
- 'restapi:admin/actiongroups'
- 'restapi:admin/allowlist'
- 'restapi:admin/config/update'
- 'restapi:admin/internalusers'
- 'restapi:admin/nodesdn'
- 'restapi:admin/roles'
Expand Down
25 changes: 17 additions & 8 deletions src/main/java/org/opensearch/security/OpenSearchSecurityPlugin.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -137,6 +137,7 @@
import org.opensearch.security.configuration.PrivilegesInterceptorImpl;
import org.opensearch.security.configuration.Salt;
import org.opensearch.security.configuration.SecurityFlsDlsIndexSearcherWrapper;
import org.opensearch.security.dlic.rest.api.Endpoint;
import org.opensearch.security.dlic.rest.api.SecurityRestApiActions;
import org.opensearch.security.dlic.rest.validation.PasswordValidator;
import org.opensearch.security.filter.SecurityFilter;
Expand Down Expand Up @@ -192,6 +193,11 @@
import org.opensearch.transport.TransportService;
import org.opensearch.watcher.ResourceWatcherService;

import static org.opensearch.security.dlic.rest.api.RestApiAdminPrivilegesEvaluator.ENDPOINTS_WITH_PERMISSIONS;
import static org.opensearch.security.dlic.rest.api.RestApiAdminPrivilegesEvaluator.SECURITY_CONFIG_UPDATE;
import static org.opensearch.security.setting.DeprecatedSettings.checkForDeprecatedSetting;
import static org.opensearch.security.support.ConfigConstants.SECURITY_UNSUPPORTED_RESTAPI_ALLOW_SECURITYCONFIG_MODIFICATION;

public final class OpenSearchSecurityPlugin extends OpenSearchSecuritySSLPlugin implements ClusterPlugin, MapperPlugin {

private static final String KEYWORD = ".keyword";
Expand Down Expand Up @@ -319,21 +325,24 @@ public OpenSearchSecurityPlugin(final Settings settings, final Path configPath)
sm.checkPermission(new SpecialPermission());
}

AccessController.doPrivileged(new PrivilegedAction<Object>() {
@Override
public Object run() {
if (Security.getProvider("BC") == null) {
Security.addProvider(new BouncyCastleProvider());
}
return null;
AccessController.doPrivileged((PrivilegedAction<Object>) () -> {
if (Security.getProvider("BC") == null) {
Security.addProvider(new BouncyCastleProvider());
}
return null;
});

final String advancedModulesEnabledKey = ConfigConstants.SECURITY_ADVANCED_MODULES_ENABLED;
if (settings.hasValue(advancedModulesEnabledKey)) {
deprecationLogger.deprecate("Setting {} is ignored.", advancedModulesEnabledKey);
}

checkForDeprecatedSetting(
settings,
SECURITY_UNSUPPORTED_RESTAPI_ALLOW_SECURITYCONFIG_MODIFICATION,
ENDPOINTS_WITH_PERMISSIONS.get(Endpoint.CONFIG).build(SECURITY_CONFIG_UPDATE) + " permission"
);

log.info("Clustername: {}", settings.get("cluster.name", "opensearch"));

if (!transportSSLEnabled && !SSLConfig.isSslOnlyMode()) {
Expand Down Expand Up @@ -1773,7 +1782,7 @@ public List<Setting<?>> getSettings() {
);
settings.add(
Setting.boolSetting(
ConfigConstants.SECURITY_UNSUPPORTED_RESTAPI_ALLOW_SECURITYCONFIG_MODIFICATION,
SECURITY_UNSUPPORTED_RESTAPI_ALLOW_SECURITYCONFIG_MODIFICATION,
false,
Property.NodeScope,
Property.Filtered
Expand Down
18 changes: 6 additions & 12 deletions src/main/java/org/opensearch/security/dlic/rest/api/RestApiAdminPrivilegesEvaluator.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -35,9 +35,11 @@ public class RestApiAdminPrivilegesEvaluator {

protected final Logger logger = LogManager.getLogger(RestApiAdminPrivilegesEvaluator.class);

public final static String CERTS_INFO_ACTION = "certs";
public final static String CERTS_INFO_ACTION = "certs/info";

public final static String RELOAD_CERTS_ACTION = "reloadcerts";
public final static String RELOAD_CERTS_ACTION = "certs/reload";

public final static String SECURITY_CONFIG_UPDATE = "update";

private final static String REST_API_PERMISSION_PREFIX = "restapi:admin";

Expand All @@ -61,21 +63,13 @@ default String build() {
public final static Map<Endpoint, PermissionBuilder> ENDPOINTS_WITH_PERMISSIONS = ImmutableMap.<Endpoint, PermissionBuilder>builder()
.put(Endpoint.ACTIONGROUPS, action -> buildEndpointPermission(Endpoint.ACTIONGROUPS))
.put(Endpoint.ALLOWLIST, action -> buildEndpointPermission(Endpoint.ALLOWLIST))
.put(Endpoint.CONFIG, action -> buildEndpointActionPermission(Endpoint.CONFIG, action))
.put(Endpoint.INTERNALUSERS, action -> buildEndpointPermission(Endpoint.INTERNALUSERS))
.put(Endpoint.NODESDN, action -> buildEndpointPermission(Endpoint.NODESDN))
.put(Endpoint.ROLES, action -> buildEndpointPermission(Endpoint.ROLES))
.put(Endpoint.ROLESMAPPING, action -> buildEndpointPermission(Endpoint.ROLESMAPPING))
.put(Endpoint.TENANTS, action -> buildEndpointPermission(Endpoint.TENANTS))
.put(Endpoint.SSL, action -> {
switch (action) {
case CERTS_INFO_ACTION:
return buildEndpointActionPermission(Endpoint.SSL, "certs/info");
case RELOAD_CERTS_ACTION:
return buildEndpointActionPermission(Endpoint.SSL, "certs/reload");
default:
return null;
}
})
.put(Endpoint.SSL, action -> buildEndpointActionPermission(Endpoint.SSL, action))
.build();

private final ThreadContext threadContext;
Expand Down
49 changes: 28 additions & 21 deletions src/main/java/org/opensearch/security/dlic/rest/api/SecurityConfigApiAction.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,42 +11,41 @@

package org.opensearch.security.dlic.rest.api;

import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
import org.opensearch.cluster.service.ClusterService;
import org.opensearch.common.inject.Inject;
import org.opensearch.common.settings.Settings;
import org.opensearch.core.rest.RestStatus;
import org.opensearch.rest.RestRequest;
import org.opensearch.rest.RestRequest.Method;
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.ValidationResult;
import org.opensearch.security.securityconf.impl.CType;
import org.opensearch.security.support.ConfigConstants;
import org.opensearch.threadpool.ThreadPool;

import java.util.Collections;
import java.util.List;
import java.util.Map;

import static org.opensearch.security.dlic.rest.api.RequestHandler.methodNotImplementedHandler;
import static org.opensearch.security.dlic.rest.api.Responses.methodNotImplementedMessage;
import static org.opensearch.security.dlic.rest.api.RestApiAdminPrivilegesEvaluator.SECURITY_CONFIG_UPDATE;
import static org.opensearch.security.dlic.rest.support.Utils.addRoutesPrefix;
import static org.opensearch.security.support.ConfigConstants.SECURITY_RESTAPI_ADMIN_ENABLED;

public class SecurityConfigApiAction extends AbstractApiAction {

private static final List<Route> getRoutes = addRoutesPrefix(Collections.singletonList(new Route(Method.GET, "/securityconfig/")));

private static final List<Route> allRoutes = new ImmutableList.Builder<Route>().addAll(getRoutes)
.addAll(
addRoutesPrefix(ImmutableList.of(new Route(Method.PUT, "/securityconfig/config"), new Route(Method.PATCH, "/securityconfig/")))
private static final List<Route> routes = addRoutesPrefix(
List.of(
new Route(Method.GET, "/securityconfig"),
new Route(Method.PATCH, "/securityconfig"),
new Route(Method.PUT, "/securityconfig/config")
)
.build();
);

private final boolean allowPutOrPatch;

private final boolean restApiAdminEnabled;

@Inject
public SecurityConfigApiAction(
final ClusterService clusterService,
Expand All @@ -57,12 +56,13 @@ public SecurityConfigApiAction(
super(Endpoint.CONFIG, clusterService, threadPool, securityApiDependencies);
allowPutOrPatch = securityApiDependencies.settings()
.getAsBoolean(ConfigConstants.SECURITY_UNSUPPORTED_RESTAPI_ALLOW_SECURITYCONFIG_MODIFICATION, false);
this.restApiAdminEnabled = securityApiDependencies.settings().getAsBoolean(SECURITY_RESTAPI_ADMIN_ENABLED, false);
this.requestHandlersBuilder.configureRequestHandlers(this::securityConfigApiActionRequestHandlers);
}

@Override
public List<Route> routes() {
return allowPutOrPatch ? allRoutes : getRoutes;
return routes;
}

@Override
Expand All @@ -74,20 +74,27 @@ protected CType getConfigType() {
protected void consumeParameters(RestRequest request) {}

private void securityConfigApiActionRequestHandlers(RequestHandler.RequestHandlersBuilder requestHandlersBuilder) {
requestHandlersBuilder.onChangeRequest(
Method.PUT,
request -> withAllowedEndpoint(request).map(ignore -> processPutRequest("config", request))
)
.onChangeRequest(Method.PATCH, request -> withAllowedEndpoint(request).map(this::processPatchRequest))
requestHandlersBuilder.withAccessHandler(this::accessHandler)
.verifyAccessForAllMethods()
.onChangeRequest(Method.PUT, request -> processPutRequest("config", request))
.onChangeRequest(Method.PATCH, this::processPatchRequest)
.override(Method.DELETE, methodNotImplementedHandler)
.override(Method.POST, methodNotImplementedHandler);
}

ValidationResult<RestRequest> withAllowedEndpoint(final RestRequest request) {
if (!allowPutOrPatch) {
return ValidationResult.error(RestStatus.NOT_IMPLEMENTED, methodNotImplementedMessage(request.method()));
boolean accessHandler(final RestRequest request) {
switch (request.method()) {
case PATCH:
case PUT:
if (!restApiAdminEnabled) {
return allowPutOrPatch;
} else {
return securityApiDependencies.restApiAdminPrivilegesEvaluator()
.isCurrentUserAdminFor(endpoint, SECURITY_CONFIG_UPDATE);
}
default:
return true;
}
return ValidationResult.success(request);
}

@Override
Expand Down
2 changes: 1 addition & 1 deletion src/main/java/org/opensearch/security/dlic/rest/api/SecurityRestApiActions.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,7 @@ public static Collection<RestHandler> getHandler(
new AllowlistApiAction(Endpoint.ALLOWLIST, clusterService, threadPool, securityApiDependencies),
new AuditApiAction(clusterService, threadPool, securityApiDependencies),
new MultiTenancyConfigApiAction(clusterService, threadPool, securityApiDependencies),
new SecuritySSLCertsAction(clusterService, threadPool, securityKeyStore, certificatesReloadEnabled, securityApiDependencies)
new SecuritySSLCertsApiAction(clusterService, threadPool, securityKeyStore, certificatesReloadEnabled, securityApiDependencies)
);
}

Expand Down
23 changes: 12 additions & 11 deletions ...dlic/rest/api/SecuritySSLCertsAction.java → ...c/rest/api/SecuritySSLCertsApiAction.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,8 @@
import static org.opensearch.security.dlic.rest.api.Responses.badRequestMessage;
import static org.opensearch.security.dlic.rest.api.Responses.ok;
import static org.opensearch.security.dlic.rest.api.Responses.response;
import static org.opensearch.security.dlic.rest.api.RestApiAdminPrivilegesEvaluator.CERTS_INFO_ACTION;
import static org.opensearch.security.dlic.rest.api.RestApiAdminPrivilegesEvaluator.RELOAD_CERTS_ACTION;
import static org.opensearch.security.dlic.rest.support.Utils.addRoutesPrefix;

/**
Expand All @@ -45,7 +47,7 @@
* This action serves GET request for _plugins/_security/api/ssl/certs endpoint and
* PUT _plugins/_security/api/ssl/{certType}/reloadcerts
*/
public class SecuritySSLCertsAction extends AbstractApiAction {
public class SecuritySSLCertsApiAction extends AbstractApiAction {
private static final List<Route> ROUTES = addRoutesPrefix(
ImmutableList.of(new Route(Method.GET, "/ssl/certs"), new Route(Method.PUT, "/ssl/{certType}/reloadcerts/"))
);
Expand All @@ -56,7 +58,7 @@ public class SecuritySSLCertsAction extends AbstractApiAction {

private final boolean httpsEnabled;

public SecuritySSLCertsAction(
public SecuritySSLCertsApiAction(
final ClusterService clusterService,
final ThreadPool threadPool,
final SecurityKeyStore securityKeyStore,
Expand Down Expand Up @@ -116,18 +118,17 @@ private void securitySSLCertsRequestHandlers(RequestHandler.RequestHandlersBuild
}).error((status, toXContent) -> response(channel, status, toXContent)));
}

private boolean accessHandler(final RestRequest request) {
switch (request.method()) {
case GET:
return securityApiDependencies.restApiAdminPrivilegesEvaluator().isCurrentUserAdminFor(endpoint, "certs");
case PUT:
return securityApiDependencies.restApiAdminPrivilegesEvaluator().isCurrentUserAdminFor(endpoint, "reloadcerts");
default:
return false;
boolean accessHandler(final RestRequest request) {
if (request.method() == Method.GET) {
return securityApiDependencies.restApiAdminPrivilegesEvaluator().isCurrentUserAdminFor(endpoint, CERTS_INFO_ACTION);
} else if (request.method() == Method.PUT) {
return securityApiDependencies.restApiAdminPrivilegesEvaluator().isCurrentUserAdminFor(endpoint, RELOAD_CERTS_ACTION);
} else {
return false;
}
}

private ValidationResult<SecurityKeyStore> withSecurityKeyStore() {
ValidationResult<SecurityKeyStore> withSecurityKeyStore() {
if (securityKeyStore == null) {
return ValidationResult.error(RestStatus.OK, badRequestMessage("keystore is not initialized"));
}
Expand Down
Loading

0 comments on commit 9480a22

Please sign in to comment.