Support security config updates on the REST API

Instead of, SECURITY_UNSUPPORTED_RESTAPI_ALLOW_SECURITYCONFIG_MODIFICATION settings which we use to update security configuration a new permission was added: `restapi:admin/config/update`.

Signed-off-by: Andrey Pleskach <[email protected]>

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'

src/main/java/org/opensearch/security/OpenSearchSecurityPlugin.java

@@ -193,6 +193,8 @@
 import org.opensearch.transport.TransportResponseHandler;
 import org.opensearch.transport.TransportService;
 import org.opensearch.watcher.ResourceWatcherService;
+
+import static org.opensearch.security.support.ConfigConstants.SECURITY_UNSUPPORTED_RESTAPI_ALLOW_SECURITYCONFIG_MODIFICATION;
 // CS-ENFORCE-SINGLE
 
 public final class OpenSearchSecurityPlugin extends OpenSearchSecuritySSLPlugin
@@ -339,6 +341,17 @@ public Object run() {
             deprecationLogger.deprecate("Setting {} is ignored.", advancedModulesEnabledKey);
         }
 
+        final boolean unsupportedRestapiAllowSecurityConfigModification = settings.getAsBoolean(
+            SECURITY_UNSUPPORTED_RESTAPI_ALLOW_SECURITYCONFIG_MODIFICATION,
+            false
+        );
+        if (unsupportedRestapiAllowSecurityConfigModification) {
+            deprecationLogger.deprecate(
+                "Setting {} is deprecated. Please use permissions instead.",
+                SECURITY_UNSUPPORTED_RESTAPI_ALLOW_SECURITYCONFIG_MODIFICATION
+            );
+        }
+
         log.info("Clustername: {}", settings.get("cluster.name", "opensearch"));
 
         if (!transportSSLEnabled && !SSLConfig.isSslOnlyMode()) {
@@ -1776,7 +1789,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

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;
@@ -159,6 +153,9 @@ public boolean isCurrentUserAdminFor(final Endpoint endpoint) {
     }
 
     private static String buildEndpointActionPermission(final Endpoint endpoint, final String action) {
+        if (action == null) {
+            return "";
+        }
         return String.format(REST_ENDPOINT_ACTION_PERMISSION_PATTERN, endpoint.name().toLowerCase(Locale.ROOT), action);
     }
 

src/main/java/org/opensearch/security/dlic/rest/api/SecurityConfigApiAction.java

@@ -11,12 +11,6 @@
 
 package org.opensearch.security.dlic.rest.api;
 
-import java.util.Collections;
-import java.util.List;
-import java.util.Map;
-
-import com.google.common.collect.ImmutableList;
-
 import com.google.common.collect.ImmutableMap;
 import org.opensearch.cluster.service.ClusterService;
 import org.opensearch.common.inject.Inject;
@@ -32,23 +26,29 @@
 import org.opensearch.security.support.ConfigConstants;
 import org.opensearch.threadpool.ThreadPool;
 
+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.badRequestMessage;
-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/{name}"), 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/{name}")
         )
-        .build();
+    );
 
     private final boolean allowPutOrPatch;
 
+    private final boolean restApiAdminEnabled;
+
     @Inject
     public SecurityConfigApiAction(
         final ClusterService clusterService,
@@ -59,12 +59,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
@@ -73,28 +74,37 @@ protected CType getConfigType() {
     }
 
     private void securityConfigApiActionRequestHandlers(RequestHandler.RequestHandlersBuilder requestHandlersBuilder) {
-        requestHandlersBuilder.onChangeRequest(
-            Method.PUT,
-            request -> withAllowedEndpoint(request).map(this::withConfigEntityNameOnly).map(ignore -> processPutRequest(request))
-        )
-            .onChangeRequest(Method.PATCH, request -> withAllowedEndpoint(request).map(this::processPatchRequest))
+        requestHandlersBuilder.withAccessHandler(this::accessHandler)
+            .verifyAccessForAllMethods()
+            .onChangeRequest(Method.PUT, request -> withConfigEntityNameOnly(request).map(this::processPutRequest))
+            .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 (!allowPutOrPatch && !restApiAdminEnabled) {
+                    return false;
+                } else if (allowPutOrPatch && !restApiAdminEnabled) {
+                    return true;
+                } else {
+                    return securityApiDependencies.restApiAdminPrivilegesEvaluator()
+                        .isCurrentUserAdminFor(endpoint, SECURITY_CONFIG_UPDATE);
+                }
+            default:
+                return true;
         }
-        return ValidationResult.success(request);
     }
 
-    ValidationResult<String> withConfigEntityNameOnly(final RestRequest request) {
+    ValidationResult<RestRequest> withConfigEntityNameOnly(final RestRequest request) {
         final var name = nameParam(request);
         if (!"config".equals(name)) {
             return ValidationResult.error(RestStatus.BAD_REQUEST, badRequestMessage("name must be config"));
         }
-        return ValidationResult.success(name);
+        return ValidationResult.success(request);
     }
 
     @Override

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)
         );
     }
 

src/main/java/org/opensearch/security/dlic/rest/api/SecuritySSLCertsAction.java → src/main/java/org/opensearch/security/dlic/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"));
         }

src/test/java/org/opensearch/security/dlic/rest/api/SecurityConfigApiActionValidationTest.java

@@ -14,14 +14,17 @@
 import org.junit.Test;
 import org.opensearch.common.settings.Settings;
 import org.opensearch.core.rest.RestStatus;
-import org.opensearch.security.support.ConfigConstants;
+import org.opensearch.rest.RestRequest;
 import org.opensearch.security.util.FakeRestRequest;
 
 import java.util.Map;
 
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertTrue;
+import static org.mockito.Mockito.when;
+import static org.opensearch.security.support.ConfigConstants.SECURITY_RESTAPI_ADMIN_ENABLED;
+import static org.opensearch.security.support.ConfigConstants.SECURITY_UNSUPPORTED_RESTAPI_ALLOW_SECURITYCONFIG_MODIFICATION;
 
 public class SecurityConfigApiActionValidationTest extends AbstractApiActionValidationTest {
 
@@ -39,18 +42,20 @@ public void configEntityNameOnly() {
     }
 
     @Test
-    public void withAllowedEndpoint() {
-        var securityConfigApiAction = new SecurityConfigApiAction(
+    public void accessHandlerForDefaultSettings() {
+        final var securityConfigApiAction = new SecurityConfigApiAction(
             clusterService,
             threadPool,
             new SecurityApiDependencies(null, configurationRepository, null, null, restApiAdminPrivilegesEvaluator, null, Settings.EMPTY)
         );
+        assertTrue(securityConfigApiAction.accessHandler(FakeRestRequest.builder().withMethod(RestRequest.Method.GET).build()));
+        assertFalse(securityConfigApiAction.accessHandler(FakeRestRequest.builder().withMethod(RestRequest.Method.PUT).build()));
+        assertFalse(securityConfigApiAction.accessHandler(FakeRestRequest.builder().withMethod(RestRequest.Method.PATCH).build()));
+    }
 
-        var result = securityConfigApiAction.withAllowedEndpoint(FakeRestRequest.builder().build());
-        assertFalse(result.isValid());
-        assertEquals(RestStatus.NOT_IMPLEMENTED, result.status());
-
-        securityConfigApiAction = new SecurityConfigApiAction(
+    @Test
+    public void accessHandlerForUnsupportedSetting() {
+        final var securityConfigApiAction = new SecurityConfigApiAction(
             clusterService,
             threadPool,
             new SecurityApiDependencies(
@@ -60,11 +65,32 @@ public void withAllowedEndpoint() {
                 null,
                 restApiAdminPrivilegesEvaluator,
                 null,
-                Settings.builder().put(ConfigConstants.SECURITY_UNSUPPORTED_RESTAPI_ALLOW_SECURITYCONFIG_MODIFICATION, true).build()
+                Settings.builder().put(SECURITY_UNSUPPORTED_RESTAPI_ALLOW_SECURITYCONFIG_MODIFICATION, true).build()
             )
         );
-        result = securityConfigApiAction.withAllowedEndpoint(FakeRestRequest.builder().build());
-        assertTrue(result.isValid());
+        assertTrue(securityConfigApiAction.accessHandler(FakeRestRequest.builder().withMethod(RestRequest.Method.GET).build()));
+        assertTrue(securityConfigApiAction.accessHandler(FakeRestRequest.builder().withMethod(RestRequest.Method.PUT).build()));
+        assertTrue(securityConfigApiAction.accessHandler(FakeRestRequest.builder().withMethod(RestRequest.Method.PATCH).build()));
     }
 
+    @Test
+    public void accessHandlerForRestAdmin() {
+        when(restApiAdminPrivilegesEvaluator.isCurrentUserAdminFor(Endpoint.CONFIG, RestApiAdminPrivilegesEvaluator.SECURITY_CONFIG_UPDATE)).thenReturn(true);
+        final var securityConfigApiAction = new SecurityConfigApiAction(
+                clusterService,
+                threadPool,
+                new SecurityApiDependencies(
+                        null,
+                        configurationRepository,
+                        null,
+                        null,
+                        restApiAdminPrivilegesEvaluator,
+                        null,
+                        Settings.builder().put(SECURITY_RESTAPI_ADMIN_ENABLED, true).build()
+                )
+        );
+        assertTrue(securityConfigApiAction.accessHandler(FakeRestRequest.builder().withMethod(RestRequest.Method.GET).build()));
+        assertTrue(securityConfigApiAction.accessHandler(FakeRestRequest.builder().withMethod(RestRequest.Method.PUT).build()));
+        assertTrue(securityConfigApiAction.accessHandler(FakeRestRequest.builder().withMethod(RestRequest.Method.PATCH).build()));
+    }
 }