From b86635ee0a1ad0e72f0d580175e59542569112d8 Mon Sep 17 00:00:00 2001 From: Andrey Pleskach Date: Tue, 29 Aug 2023 21:30:25 +0200 Subject: [PATCH] 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 --- config/roles.yml | 1 + .../security/OpenSearchSecurityPlugin.java | 15 +- .../api/RestApiAdminPrivilegesEvaluator.java | 21 ++- .../rest/api/SecurityConfigApiAction.java | 60 ++++---- .../dlic/rest/api/SecurityRestApiActions.java | 2 +- ...on.java => SecuritySSLCertsApiAction.java} | 23 ++-- ...SecurityConfigApiActionValidationTest.java | 48 +++++-- .../dlic/rest/api/SecurityConfigApiTest.java | 130 +++++++++++++----- ...curitySSLCertsApiActionValidationTest.java | 69 ++++++++++ .../security/dlic/rest/api/UserApiTest.java | 6 +- .../SecurityRolesPermissionsTest.java | 27 +++- src/test/resources/restapi/internal_users.yml | 6 + src/test/resources/restapi/roles.yml | 5 + src/test/resources/restapi/roles_mapping.yml | 5 + 14 files changed, 318 insertions(+), 100 deletions(-) rename src/main/java/org/opensearch/security/dlic/rest/api/{SecuritySSLCertsAction.java => SecuritySSLCertsApiAction.java} (90%) create mode 100644 src/test/java/org/opensearch/security/dlic/rest/api/SecuritySSLCertsApiActionValidationTest.java diff --git a/config/roles.yml b/config/roles.yml index 570168fe10..77906290a0 100644 --- 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 index c34e3877f1..418c70ba34 100644 --- a/src/main/java/org/opensearch/security/OpenSearchSecurityPlugin.java +++ b/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> 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 index d05803be50..a65202796e 100644 --- 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 ENDPOINTS_WITH_PERMISSIONS = ImmutableMap.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); } 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 index 62865cf2e1..0266941ad9 100644 --- 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,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 getRoutes = addRoutesPrefix(Collections.singletonList(new Route(Method.GET, "/securityconfig/"))); - - private static final List allRoutes = new ImmutableList.Builder().addAll(getRoutes) - .addAll( - addRoutesPrefix(ImmutableList.of(new Route(Method.PUT, "/securityconfig/{name}"), new Route(Method.PATCH, "/securityconfig/"))) + private static final List 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 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 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 withConfigEntityNameOnly(final RestRequest request) { + ValidationResult 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 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 index 2eda752a82..78f9ce91df 100644 --- 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 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/src/main/java/org/opensearch/security/dlic/rest/api/SecuritySSLCertsAction.java b/src/main/java/org/opensearch/security/dlic/rest/api/SecuritySSLCertsApiAction.java similarity index 90% rename from src/main/java/org/opensearch/security/dlic/rest/api/SecuritySSLCertsAction.java rename to src/main/java/org/opensearch/security/dlic/rest/api/SecuritySSLCertsApiAction.java index 639d93c6ab..1dee3d8c84 100644 --- a/src/main/java/org/opensearch/security/dlic/rest/api/SecuritySSLCertsAction.java +++ b/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 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 withSecurityKeyStore() { + ValidationResult withSecurityKeyStore() { if (securityKeyStore == null) { return ValidationResult.error(RestStatus.OK, badRequestMessage("keystore is not initialized")); } diff --git a/src/test/java/org/opensearch/security/dlic/rest/api/SecurityConfigApiActionValidationTest.java b/src/test/java/org/opensearch/security/dlic/rest/api/SecurityConfigApiActionValidationTest.java index e5993b3698..f0de5c4418 100644 --- a/src/test/java/org/opensearch/security/dlic/rest/api/SecurityConfigApiActionValidationTest.java +++ b/src/test/java/org/opensearch/security/dlic/rest/api/SecurityConfigApiActionValidationTest.java @@ -14,7 +14,7 @@ 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; @@ -22,6 +22,9 @@ 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())); + } } diff --git a/src/test/java/org/opensearch/security/dlic/rest/api/SecurityConfigApiTest.java b/src/test/java/org/opensearch/security/dlic/rest/api/SecurityConfigApiTest.java index 8fc6ae1dd8..ff4328136d 100644 --- a/src/test/java/org/opensearch/security/dlic/rest/api/SecurityConfigApiTest.java +++ b/src/test/java/org/opensearch/security/dlic/rest/api/SecurityConfigApiTest.java @@ -36,31 +36,47 @@ public SecurityConfigApiTest() { } @Test - public void testSecurityConfigApiRead() throws Exception { + public void testSecurityConfigApiReadForSuperAdmin() throws Exception { setup(); rh.keystore = "restapi/kirk-keystore.jks"; rh.sendAdminCertificate = true; - HttpResponse response = rh.executeGetRequest(ENDPOINT + "/securityconfig", new Header[0]); + verifyResponsesWithoutPermissionOrUnsupportedFlag(); + } + + @Test + public void testSecurityConfigApiReadRestApiUser() throws Exception { + + setupWithRestRoles(); + + rh.keystore = "restapi/kirk-keystore.jks"; + rh.sendAdminCertificate = false; + + final var restApiHeader = encodeBasicHeader("test", "test"); + verifyResponsesWithoutPermissionOrUnsupportedFlag(restApiHeader); + } + + private void verifyResponsesWithoutPermissionOrUnsupportedFlag(final Header... headers) { + HttpResponse response = rh.executeGetRequest(ENDPOINT + "/securityconfig", headers); Assert.assertEquals(HttpStatus.SC_OK, response.getStatusCode()); - response = rh.executePutRequest(ENDPOINT + "/securityconfig", "{\"xxx\": 1}", new Header[0]); + response = rh.executePutRequest(ENDPOINT + "/securityconfig", "{\"xxx\": 1}", headers); Assert.assertEquals(HttpStatus.SC_METHOD_NOT_ALLOWED, response.getStatusCode()); - response = rh.executePostRequest(ENDPOINT + "/securityconfig", "{\"xxx\": 1}", new Header[0]); + response = rh.executePostRequest(ENDPOINT + "/securityconfig", "{\"xxx\": 1}", headers); Assert.assertEquals(HttpStatus.SC_METHOD_NOT_ALLOWED, response.getStatusCode()); - response = rh.executePatchRequest(ENDPOINT + "/securityconfig", "{\"xxx\": 1}", new Header[0]); - Assert.assertEquals(HttpStatus.SC_METHOD_NOT_ALLOWED, response.getStatusCode()); + response = rh.executePatchRequest(ENDPOINT + "/securityconfig", "{\"xxx\": 1}", headers); + Assert.assertEquals(HttpStatus.SC_FORBIDDEN, response.getStatusCode()); - response = rh.executeDeleteRequest(ENDPOINT + "/securityconfig", new Header[0]); + response = rh.executeDeleteRequest(ENDPOINT + "/securityconfig", headers); Assert.assertEquals(HttpStatus.SC_METHOD_NOT_ALLOWED, response.getStatusCode()); } @Test - public void testSecurityConfigApiWrite() throws Exception { + public void testSecurityConfigApiWriteWithUnsupportedFlagForSuperAdmin() throws Exception { Settings settings = Settings.builder() .put(ConfigConstants.SECURITY_UNSUPPORTED_RESTAPI_ALLOW_SECURITYCONFIG_MODIFICATION, true) @@ -70,52 +86,66 @@ public void testSecurityConfigApiWrite() throws Exception { rh.keystore = "restapi/kirk-keystore.jks"; rh.sendAdminCertificate = true; - HttpResponse response = rh.executeGetRequest(ENDPOINT + "/securityconfig", new Header[0]); + verifyWriteOperations(); + } + + @Test + public void testSecurityConfigApiWriteWithFullListOfPermissions() throws Exception { + + Settings settings = Settings.builder().put(ConfigConstants.SECURITY_RESTAPI_ADMIN_ENABLED, true).build(); + setupWithRestRoles(settings); + + rh.keystore = "restapi/kirk-keystore.jks"; + rh.sendAdminCertificate = false; + + final var restAdminFullAccess = encodeBasicHeader("rest_api_admin_user", "rest_api_admin_user"); + verifyWriteOperations(restAdminFullAccess); + } + + @Test + public void testSecurityConfigApiWriteWithOnePermission() throws Exception { + Settings settings = Settings.builder().put(ConfigConstants.SECURITY_RESTAPI_ADMIN_ENABLED, true).build(); + setupWithRestRoles(settings); + rh.keystore = "restapi/kirk-keystore.jks"; + rh.sendAdminCertificate = false; + final var updateOnlyRestApiHeader = encodeBasicHeader("rest_api_admin_config_update", "rest_api_admin_config_update"); + verifyWriteOperations(updateOnlyRestApiHeader); + } + + private void verifyWriteOperations(final Header... header) throws Exception { + HttpResponse response = rh.executeGetRequest(ENDPOINT + "/securityconfig", header); Assert.assertEquals(HttpStatus.SC_OK, response.getStatusCode()); - response = rh.executePutRequest( - ENDPOINT + "/securityconfig/xxx", - FileHelper.loadFile("restapi/securityconfig.json"), - new Header[0] - ); + response = rh.executePutRequest(ENDPOINT + "/securityconfig/xxx", FileHelper.loadFile("restapi/securityconfig.json"), header); Assert.assertEquals(HttpStatus.SC_BAD_REQUEST, response.getStatusCode()); - response = rh.executePutRequest( - ENDPOINT + "/securityconfig/config", - FileHelper.loadFile("restapi/securityconfig.json"), - new Header[0] - ); + response = rh.executePutRequest(ENDPOINT + "/securityconfig/config", FileHelper.loadFile("restapi/securityconfig.json"), header); Assert.assertEquals(HttpStatus.SC_OK, response.getStatusCode()); - response = rh.executePutRequest( - ENDPOINT + "/securityconfig/config", - FileHelper.loadFile("restapi/invalid_config.json"), - new Header[0] - ); + response = rh.executePutRequest(ENDPOINT + "/securityconfig/config", FileHelper.loadFile("restapi/invalid_config.json"), header); Assert.assertEquals(HttpStatus.SC_INTERNAL_SERVER_ERROR, response.getStatusCode()); Assert.assertTrue(response.getContentType(), response.isJsonContentType()); Assert.assertTrue(response.getBody().contains("Unrecognized field")); - response = rh.executeGetRequest(ENDPOINT + "/securityconfig", new Header[0]); + response = rh.executeGetRequest(ENDPOINT + "/securityconfig", header); Assert.assertEquals(HttpStatus.SC_OK, response.getStatusCode()); - response = rh.executePostRequest(ENDPOINT + "/securityconfig", "{\"xxx\": 1}", new Header[0]); + response = rh.executePostRequest(ENDPOINT + "/securityconfig", "{\"xxx\": 1}", header); Assert.assertEquals(HttpStatus.SC_METHOD_NOT_ALLOWED, response.getStatusCode()); response = rh.executePatchRequest( ENDPOINT + "/securityconfig", "[{\"op\": \"replace\",\"path\": \"/config/dynamic/hosts_resolver_mode\",\"value\": \"other\"}]", - new Header[0] + header ); - Assert.assertEquals(HttpStatus.SC_OK, response.getStatusCode()); + Assert.assertEquals(response.getBody(), HttpStatus.SC_OK, response.getStatusCode()); - response = rh.executeDeleteRequest(ENDPOINT + "/securityconfig", new Header[0]); + response = rh.executeDeleteRequest(ENDPOINT + "/securityconfig", header); Assert.assertEquals(HttpStatus.SC_METHOD_NOT_ALLOWED, response.getStatusCode()); - } @Test - public void testSecurityConfigForHTTPPatch() throws Exception { + public void testSecurityConfigForPatchWithUnsupportedFlag() throws Exception { Settings settings = Settings.builder() .put(ConfigConstants.SECURITY_UNSUPPORTED_RESTAPI_ALLOW_SECURITYCONFIG_MODIFICATION, true) @@ -124,28 +154,58 @@ public void testSecurityConfigForHTTPPatch() throws Exception { rh.keystore = "restapi/kirk-keystore.jks"; rh.sendAdminCertificate = true; + verifyPatch(); + } + + @Test + public void testSecurityConfigForPatchWithFullPermissions() throws Exception { + + Settings settings = Settings.builder().put(ConfigConstants.SECURITY_RESTAPI_ADMIN_ENABLED, true).build(); + setupWithRestRoles(settings); + + rh.keystore = "restapi/kirk-keystore.jks"; + rh.sendAdminCertificate = false; // non-default config + final var restAdminFullAccess = encodeBasicHeader("rest_api_admin_user", "rest_api_admin_user"); + verifyPatch(restAdminFullAccess); + } + + @Test + public void testSecurityConfigForPatchWithOnePermission() throws Exception { + + Settings settings = Settings.builder().put(ConfigConstants.SECURITY_RESTAPI_ADMIN_ENABLED, true).build(); + setupWithRestRoles(settings); + + rh.keystore = "restapi/kirk-keystore.jks"; + rh.sendAdminCertificate = false; + + // non-default config + final var updateOnlyRestApiHeader = encodeBasicHeader("rest_api_admin_config_update", "rest_api_admin_config_update"); + verifyPatch(updateOnlyRestApiHeader); + } + + private void verifyPatch(final Header... header) throws Exception { String updatedConfig = FileHelper.loadFile("restapi/securityconfig_nondefault.json"); // update config - HttpResponse response = rh.executePutRequest(ENDPOINT + "/securityconfig/config", updatedConfig, new Header[0]); + HttpResponse response = rh.executePutRequest(ENDPOINT + "/securityconfig/config", updatedConfig, header); Assert.assertEquals(HttpStatus.SC_OK, response.getStatusCode()); // make patch request response = rh.executePatchRequest( ENDPOINT + "/securityconfig", "[{\"op\": \"add\",\"path\": \"/config/dynamic/do_not_fail_on_forbidden\",\"value\": \"false\"}]", - new Header[0] + header ); Assert.assertEquals(HttpStatus.SC_OK, response.getStatusCode()); // get config - response = rh.executeGetRequest(ENDPOINT + "/securityconfig", new Header[0]); + response = rh.executeGetRequest(ENDPOINT + "/securityconfig", header); Assert.assertEquals(HttpStatus.SC_OK, response.getStatusCode()); // verify configs are same Assert.assertEquals(DefaultObjectMapper.readTree(updatedConfig), DefaultObjectMapper.readTree(response.getBody()).get("config")); - } + } diff --git a/src/test/java/org/opensearch/security/dlic/rest/api/SecuritySSLCertsApiActionValidationTest.java b/src/test/java/org/opensearch/security/dlic/rest/api/SecuritySSLCertsApiActionValidationTest.java new file mode 100644 index 0000000000..8435e521c3 --- /dev/null +++ b/src/test/java/org/opensearch/security/dlic/rest/api/SecuritySSLCertsApiActionValidationTest.java @@ -0,0 +1,69 @@ +/* + * SPDX-License-Identifier: Apache-2.0 + * + * The OpenSearch Contributors require contributions made to + * this file be licensed under the Apache-2.0 license or a + * compatible open source license. + * + * Modifications Copyright OpenSearch Contributors. See + * GitHub history for details. + */ + +package org.opensearch.security.dlic.rest.api; + +import org.junit.Test; +import org.opensearch.core.rest.RestStatus; +import org.opensearch.rest.RestRequest; +import org.opensearch.security.util.FakeRestRequest; + +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.dlic.rest.api.RestApiAdminPrivilegesEvaluator.CERTS_INFO_ACTION; +import static org.opensearch.security.dlic.rest.api.RestApiAdminPrivilegesEvaluator.RELOAD_CERTS_ACTION; + +public class SecuritySSLCertsApiActionValidationTest extends AbstractApiActionValidationTest { + + @Test + public void withSecurityKeyStore() { + final var securitySSLCertsApiAction = new SecuritySSLCertsApiAction(clusterService, threadPool, null, true, securityApiDependencies); + final var result = securitySSLCertsApiAction.withSecurityKeyStore(); + assertFalse(result.isValid()); + assertEquals(RestStatus.OK, result.status()); + } + + @Test + public void accessDenied() { + final var securitySSLCertsApiAction = + new SecuritySSLCertsApiAction(clusterService, threadPool, null, true, securityApiDependencies); + when(restApiAdminPrivilegesEvaluator.isCurrentUserAdminFor(Endpoint.SSL, CERTS_INFO_ACTION)).thenReturn(false); + when(restApiAdminPrivilegesEvaluator.isCurrentUserAdminFor(Endpoint.SSL, RELOAD_CERTS_ACTION)).thenReturn(false); + assertFalse(securitySSLCertsApiAction.accessHandler(FakeRestRequest.builder().withMethod(RestRequest.Method.GET).build())); + assertFalse(securitySSLCertsApiAction.accessHandler(FakeRestRequest.builder().withMethod(RestRequest.Method.PUT).build())); + + for (final var m : RequestHandler.RequestHandlersBuilder.SUPPORTED_METHODS) { + if (m != RestRequest.Method.GET && m != RestRequest.Method.PUT) { + assertFalse(securitySSLCertsApiAction.accessHandler(FakeRestRequest.builder().withMethod(m).build())); + } + } + } + + @Test + public void hasAccess() { + final var securitySSLCertsApiAction = + new SecuritySSLCertsApiAction(clusterService, threadPool, null, true, securityApiDependencies); + when(restApiAdminPrivilegesEvaluator.isCurrentUserAdminFor(Endpoint.SSL, CERTS_INFO_ACTION)).thenReturn(true); + when(restApiAdminPrivilegesEvaluator.isCurrentUserAdminFor(Endpoint.SSL, RELOAD_CERTS_ACTION)).thenReturn(true); + assertTrue(securitySSLCertsApiAction.accessHandler(FakeRestRequest.builder().withMethod(RestRequest.Method.GET).build())); + assertTrue(securitySSLCertsApiAction.accessHandler(FakeRestRequest.builder().withMethod(RestRequest.Method.PUT).build())); + + for (final var m : RequestHandler.RequestHandlersBuilder.SUPPORTED_METHODS) { + if (m != RestRequest.Method.GET && m != RestRequest.Method.PUT) { + assertFalse(securitySSLCertsApiAction.accessHandler(FakeRestRequest.builder().withMethod(m).build())); + } + } + } + + +} \ No newline at end of file diff --git a/src/test/java/org/opensearch/security/dlic/rest/api/UserApiTest.java b/src/test/java/org/opensearch/security/dlic/rest/api/UserApiTest.java index 754e555fdf..ef6a51cadc 100644 --- a/src/test/java/org/opensearch/security/dlic/rest/api/UserApiTest.java +++ b/src/test/java/org/opensearch/security/dlic/rest/api/UserApiTest.java @@ -51,7 +51,7 @@ protected String getEndpointPrefix() { return PLUGINS_PREFIX; } - final int USER_SETTING_SIZE = 7 * 19; // Lines per account entry * number of accounts + final int USER_SETTING_SIZE = 7 * 20; // Lines per account entry * number of accounts private static final String ENABLED_SERVICE_ACCOUNT_BODY = "{" + " \"attributes\": { \"service\": \"true\", " @@ -95,7 +95,7 @@ public void testSecurityRoles() throws Exception { HttpResponse response = rh.executeGetRequest(ENDPOINT + "/" + CType.INTERNALUSERS.toLCString()); Assert.assertEquals(response.getBody(), HttpStatus.SC_OK, response.getStatusCode()); Settings settings = Settings.builder().loadFromSource(response.getBody(), XContentType.JSON).build(); - Assert.assertEquals(133, settings.size()); + Assert.assertEquals(USER_SETTING_SIZE, settings.size()); response = rh.executePatchRequest( ENDPOINT + "/internalusers", "[{ \"op\": \"add\", \"path\": \"/newuser\", " @@ -793,7 +793,7 @@ public void testScoreBasedPasswordRules() throws Exception { HttpResponse response = rh.executeGetRequest("_plugins/_security/api/" + CType.INTERNALUSERS.toLCString()); Assert.assertEquals(response.getBody(), HttpStatus.SC_OK, response.getStatusCode()); Settings settings = Settings.builder().loadFromSource(response.getBody(), XContentType.JSON).build(); - Assert.assertEquals(133, settings.size()); + Assert.assertEquals(USER_SETTING_SIZE, settings.size()); addUserWithPassword( "admin", diff --git a/src/test/java/org/opensearch/security/securityconf/SecurityRolesPermissionsTest.java b/src/test/java/org/opensearch/security/securityconf/SecurityRolesPermissionsTest.java index 49a9be8a91..fd93259cc7 100644 --- a/src/test/java/org/opensearch/security/securityconf/SecurityRolesPermissionsTest.java +++ b/src/test/java/org/opensearch/security/securityconf/SecurityRolesPermissionsTest.java @@ -53,6 +53,7 @@ import static org.opensearch.security.dlic.rest.api.RestApiAdminPrivilegesEvaluator.CERTS_INFO_ACTION; import static org.opensearch.security.dlic.rest.api.RestApiAdminPrivilegesEvaluator.ENDPOINTS_WITH_PERMISSIONS; import static org.opensearch.security.dlic.rest.api.RestApiAdminPrivilegesEvaluator.RELOAD_CERTS_ACTION; +import static org.opensearch.security.dlic.rest.api.RestApiAdminPrivilegesEvaluator.SECURITY_CONFIG_UPDATE; public class SecurityRolesPermissionsTest { @@ -78,6 +79,10 @@ static String restAdminApiRoleName(final String endpoint) { new SimpleEntry<>(restAdminApiRoleName(CERTS_INFO_ACTION), role(pb.build(CERTS_INFO_ACTION))), new SimpleEntry<>(restAdminApiRoleName(RELOAD_CERTS_ACTION), role(pb.build(RELOAD_CERTS_ACTION))) ); + } else if (e.getKey() == Endpoint.CONFIG) { + return Stream.of( + new SimpleEntry<>(restAdminApiRoleName(SECURITY_CONFIG_UPDATE), role(pb.build(SECURITY_CONFIG_UPDATE))) + ); } else { return Stream.of(new SimpleEntry<>(restAdminApiRoleName(endpoint), role(pb.build()))); } @@ -95,6 +100,8 @@ static String[] allRestApiPermissions() { return ENDPOINTS_WITH_PERMISSIONS.entrySet().stream().flatMap(entry -> { if (entry.getKey() == Endpoint.SSL) { return Stream.of(entry.getValue().build(CERTS_INFO_ACTION), entry.getValue().build(RELOAD_CERTS_ACTION)); + } else if (entry.getKey() == Endpoint.CONFIG) { + return Stream.of(entry.getValue().build(SECURITY_CONFIG_UPDATE)); } else { return Stream.of(entry.getValue().build()); } @@ -130,6 +137,11 @@ public void hasNoExplicitClusterPermissionPermissionForRestAdmin() { endpoint.name(), securityRolesForRole.hasExplicitClusterPermissionPermission(permissionBuilder.build(RELOAD_CERTS_ACTION)) ); + } else if (endpoint == Endpoint.CONFIG) { + Assert.assertFalse( + endpoint.name(), + securityRolesForRole.hasExplicitClusterPermissionPermission(permissionBuilder.build(SECURITY_CONFIG_UPDATE)) + ); } else { Assert.assertFalse( endpoint.name(), @@ -156,6 +168,11 @@ public void hasExplicitClusterPermissionPermissionForRestAdminWitFullAccess() { endpoint.name() + "/" + CERTS_INFO_ACTION, securityRolesForRole.hasExplicitClusterPermissionPermission(permissionBuilder.build(RELOAD_CERTS_ACTION)) ); + } else if (endpoint == Endpoint.CONFIG) { + Assert.assertTrue( + endpoint.name() + "/" + SECURITY_CONFIG_UPDATE, + securityRolesForRole.hasExplicitClusterPermissionPermission(permissionBuilder.build(SECURITY_CONFIG_UPDATE)) + ); } else { Assert.assertTrue( endpoint.name(), @@ -171,7 +188,7 @@ public void hasExplicitClusterPermissionPermissionForRestAdmin() { // verify all endpoint except SSL final Collection noSslEndpoints = ENDPOINTS_WITH_PERMISSIONS.keySet() .stream() - .filter(e -> e != Endpoint.SSL) + .filter(e -> e != Endpoint.SSL && e != Endpoint.CONFIG) .collect(Collectors.toList()); for (final Endpoint endpoint : noSslEndpoints) { final String permission = ENDPOINTS_WITH_PERMISSIONS.get(endpoint).build(); @@ -190,6 +207,14 @@ public void hasExplicitClusterPermissionPermissionForRestAdmin() { ); assertHasNoPermissionsForRestApiAdminOnePermissionRole(Endpoint.SSL, sslAllowRole); } + //verify CONFIG endpoint with 1 action + final SecurityRoles securityConfigAllowRole = configModel.getSecurityRoles().filter(ImmutableSet.of(restAdminApiRoleName(SECURITY_CONFIG_UPDATE))); + final PermissionBuilder permissionBuilder = ENDPOINTS_WITH_PERMISSIONS.get(Endpoint.CONFIG); + Assert.assertTrue( + Endpoint.SSL + "/" + SECURITY_CONFIG_UPDATE, + securityConfigAllowRole.hasExplicitClusterPermissionPermission(permissionBuilder.build(SECURITY_CONFIG_UPDATE)) + ); + assertHasNoPermissionsForRestApiAdminOnePermissionRole(Endpoint.CONFIG, securityConfigAllowRole); } void assertHasNoPermissionsForRestApiAdminOnePermissionRole(final Endpoint allowEndpoint, final SecurityRoles allowOnlyRoleForRole) { diff --git a/src/test/resources/restapi/internal_users.yml b/src/test/resources/restapi/internal_users.yml index 658d3f3aa1..d5d26ef4b5 100644 --- a/src/test/resources/restapi/internal_users.yml +++ b/src/test/resources/restapi/internal_users.yml @@ -127,3 +127,9 @@ rest_api_admin_tenants: hidden: false backend_roles: [] description: "REST API Tenats admin user" +rest_api_admin_config_update: + hash: "$2y$12$capXg1HNP49Vxeb6ijzRnu5BLMUE0ZePq1l3MhF8tjnuxg614uaY6" + reserved: false + hidden: false + backend_roles: [] + description: "REST API Config update admin user" diff --git a/src/test/resources/restapi/roles.yml b/src/test/resources/restapi/roles.yml index f639a21a4e..1c3756cb4d 100644 --- a/src/test/resources/restapi/roles.yml +++ b/src/test/resources/restapi/roles.yml @@ -398,6 +398,7 @@ rest_api_admin_full_access: cluster_permissions: - 'restapi:admin/actiongroups' - 'restapi:admin/allowlist' + - 'restapi:admin/config/update' - 'restapi:admin/internalusers' - 'restapi:admin/nodesdn' - 'restapi:admin/roles' @@ -441,3 +442,7 @@ rest_api_admin_tenants_only: reserved: true cluster_permissions: - 'restapi:admin/tenants' +rest_api_admin_config_update_only: + reserved: true + cluster_permissions: + - 'restapi:admin/config/update' diff --git a/src/test/resources/restapi/roles_mapping.yml b/src/test/resources/restapi/roles_mapping.yml index a87287d5ff..8bfe826247 100644 --- a/src/test/resources/restapi/roles_mapping.yml +++ b/src/test/resources/restapi/roles_mapping.yml @@ -195,6 +195,7 @@ opendistro_security_test: - "rest_api_admin_tenants" - "rest_api_admin_ssl_info" - "rest_api_admin_ssl_reloadcerts" + - "rest_api_admin_config_update" and_backend_roles: [] description: "Migrated from v6" opendistro_security_role_starfleet_captains: @@ -260,3 +261,7 @@ rest_api_admin_tenants_only: reserved: false hidden: true users: [rest_api_admin_tenants] +rest_api_admin_config_update_only: + reserved: false + hidden: true + users: [rest_api_admin_config_update]