Support security config updates on the REST API using permission (ope…

…nsearch-project#3264) 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]>
willyborankin · Oct 4, 2023 · e4a1b77 · e4a1b77
1 parent 7924da1
commit e4a1b77
Show file tree

Hide file tree

Showing 14 changed files with 360 additions and 140 deletions.
diff --git a/config/roles.yml b/config/roles.yml
@@ -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'

diff --git a/src/main/java/org/opensearch/security/OpenSearchSecurityPlugin.java b/src/main/java/org/opensearch/security/OpenSearchSecurityPlugin.java
@@ -27,47 +27,18 @@
 package org.opensearch.security;
 
 // CS-SUPPRESS-SINGLE: RegexpSingleline Extensions manager used to allow/disallow TLS connections to extensions
-import java.io.IOException;
-import java.nio.file.Files;
-import java.nio.file.LinkOption;
-import java.nio.file.Path;
-import java.nio.file.attribute.PosixFilePermission;
-import java.security.AccessController;
-import java.security.MessageDigest;
-import java.security.PrivilegedAction;
-import java.security.Security;
-import java.util.ArrayList;
-import java.util.Arrays;
-import java.util.Collection;
-import java.util.Collections;
-import java.util.HashMap;
-import java.util.HashSet;
-import java.util.List;
-import java.util.Map;
-import java.util.Objects;
-import java.util.Set;
-import java.util.concurrent.atomic.AtomicReference;
-import java.util.function.BiFunction;
-import java.util.function.Function;
-import java.util.function.Predicate;
-import java.util.function.Supplier;
-import java.util.function.UnaryOperator;
-import java.util.stream.Collectors;
-import java.util.stream.Stream;
 
 import com.google.common.collect.Lists;
 import org.apache.logging.log4j.LogManager;
 import org.apache.logging.log4j.Logger;
 import org.apache.lucene.search.QueryCachingPolicy;
 import org.apache.lucene.search.Weight;
 import org.bouncycastle.jce.provider.BouncyCastleProvider;
-
 import org.opensearch.OpenSearchException;
 import org.opensearch.OpenSearchSecurityException;
 import org.opensearch.SpecialPermission;
 import org.opensearch.Version;
 import org.opensearch.action.ActionRequest;
-import org.opensearch.core.action.ActionResponse;
 import org.opensearch.action.search.PitService;
 import org.opensearch.action.search.SearchScrollAction;
 import org.opensearch.action.support.ActionFilter;
@@ -80,7 +51,6 @@
 import org.opensearch.common.lifecycle.Lifecycle;
 import org.opensearch.common.lifecycle.LifecycleComponent;
 import org.opensearch.common.lifecycle.LifecycleListener;
-import org.opensearch.core.common.io.stream.NamedWriteableRegistry;
 import org.opensearch.common.logging.DeprecationLogger;
 import org.opensearch.common.network.NetworkModule;
 import org.opensearch.common.network.NetworkService;
@@ -93,14 +63,18 @@
 import org.opensearch.common.util.BigArrays;
 import org.opensearch.common.util.PageCacheRecycler;
 import org.opensearch.common.util.concurrent.ThreadContext;
+import org.opensearch.core.action.ActionResponse;
+import org.opensearch.core.common.io.stream.NamedWriteableRegistry;
+import org.opensearch.core.index.Index;
 import org.opensearch.core.indices.breaker.CircuitBreakerService;
+import org.opensearch.core.rest.RestStatus;
+import org.opensearch.core.transport.TransportResponse;
 import org.opensearch.core.xcontent.NamedXContentRegistry;
 import org.opensearch.env.Environment;
 import org.opensearch.env.NodeEnvironment;
 import org.opensearch.extensions.ExtensionsManager;
 import org.opensearch.http.HttpServerTransport;
 import org.opensearch.http.HttpServerTransport.Dispatcher;
-import org.opensearch.core.index.Index;
 import org.opensearch.index.IndexModule;
 import org.opensearch.index.cache.query.QueryCache;
 import org.opensearch.indices.IndicesService;
@@ -111,7 +85,6 @@
 import org.opensearch.repositories.RepositoriesService;
 import org.opensearch.rest.RestController;
 import org.opensearch.rest.RestHandler;
-import org.opensearch.core.rest.RestStatus;
 import org.opensearch.script.ScriptService;
 import org.opensearch.search.internal.InternalScrollSearchRequest;
 import org.opensearch.search.internal.ReaderContext;
@@ -140,6 +113,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;
@@ -190,10 +164,42 @@
 import org.opensearch.transport.TransportRequest;
 import org.opensearch.transport.TransportRequestHandler;
 import org.opensearch.transport.TransportRequestOptions;
-import org.opensearch.core.transport.TransportResponse;
 import org.opensearch.transport.TransportResponseHandler;
 import org.opensearch.transport.TransportService;
 import org.opensearch.watcher.ResourceWatcherService;
+
+import java.io.IOException;
+import java.nio.file.Files;
+import java.nio.file.LinkOption;
+import java.nio.file.Path;
+import java.nio.file.attribute.PosixFilePermission;
+import java.security.AccessController;
+import java.security.MessageDigest;
+import java.security.PrivilegedAction;
+import java.security.Security;
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.Collection;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Map;
+import java.util.Objects;
+import java.util.Set;
+import java.util.concurrent.atomic.AtomicReference;
+import java.util.function.BiFunction;
+import java.util.function.Function;
+import java.util.function.Predicate;
+import java.util.function.Supplier;
+import java.util.function.UnaryOperator;
+import java.util.stream.Collectors;
+import java.util.stream.Stream;
+
+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;
 // CS-ENFORCE-SINGLE
 
 public final class OpenSearchSecurityPlugin extends OpenSearchSecuritySSLPlugin
@@ -332,21 +338,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<Void>) () -> {
+            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()) {
@@ -1788,7 +1797,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

diff --git a/src/main/java/org/opensearch/security/dlic/rest/api/RestApiAdminPrivilegesEvaluator.java b/src/main/java/org/opensearch/security/dlic/rest/api/RestApiAdminPrivilegesEvaluator.java
@@ -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";
 
@@ -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;

diff --git a/src/main/java/org/opensearch/security/dlic/rest/api/SecurityConfigApiAction.java b/src/main/java/org/opensearch/security/dlic/rest/api/SecurityConfigApiAction.java
@@ -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,
@@ -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
@@ -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

diff --git a/src/main/java/org/opensearch/security/dlic/rest/api/SecurityRestApiActions.java b/src/main/java/org/opensearch/security/dlic/rest/api/SecurityRestApiActions.java
@@ -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)
         );
     }
 

diff --git a/...dlic/rest/api/SecuritySSLCertsAction.java → ...c/rest/api/SecuritySSLCertsApiAction.java b/...dlic/rest/api/SecuritySSLCertsAction.java → ...c/rest/api/SecuritySSLCertsApiAction.java
@@ -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;
 
 /**
@@ -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/"))
     );
@@ -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,
@@ -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"));
         }