From caf640ad524ac845e79e2f2169cefc21c07dffbe Mon Sep 17 00:00:00 2001 From: Stevan Buzejic Date: Mon, 12 Dec 2022 19:24:53 +0100 Subject: [PATCH] Added dummy search when creating detector on the given indicies Signed-off-by: Stevan Buzejic --- .../TransportIndexDetectorAction.java | 26 ++- .../SecurityAnalyticsRestTestCase.java | 62 +++++++ .../resthandler/SecureDetectorRestApiIT.java | 166 +++++++++++++++++- 3 files changed, 247 insertions(+), 7 deletions(-) diff --git a/src/main/java/org/opensearch/securityanalytics/transport/TransportIndexDetectorAction.java b/src/main/java/org/opensearch/securityanalytics/transport/TransportIndexDetectorAction.java index 057033904..bc60fe8ea 100644 --- a/src/main/java/org/opensearch/securityanalytics/transport/TransportIndexDetectorAction.java +++ b/src/main/java/org/opensearch/securityanalytics/transport/TransportIndexDetectorAction.java @@ -173,8 +173,31 @@ protected void doExecute(Task task, IndexDetectorRequest request, ActionListener return; } + checkIndicesAndExecute(task, request, listener, user); + } + + // Checks if user can access the indices and executes detector creation + private void checkIndicesAndExecute( + Task task, + IndexDetectorRequest request, + ActionListener listener, + User user + ) { + String [] detectorIndices = request.getDetector().getInputs().stream().flatMap(detectorInput -> detectorInput.getIndices().stream()).toArray(String[]::new); + SearchRequest searchRequest = new SearchRequest(detectorIndices).source(SearchSourceBuilder.searchSource().size(1).query(QueryBuilders.matchAllQuery()));; + StepListener checkIndexAccessStep = new StepListener(); + client.search(searchRequest, checkIndexAccessStep); AsyncIndexDetectorsAction asyncAction = new AsyncIndexDetectorsAction(user, task, request, listener); - asyncAction.start(); + // Check and execute as a step if the check was successful + checkIndexAccessStep.whenComplete(searchResponse -> asyncAction.start(), e -> { + if(e instanceof OpenSearchStatusException) { + listener.onFailure(SecurityAnalyticsException.wrap( + new OpenSearchStatusException(String.format(Locale.getDefault(), "User doesn't have read permissions for one or more configured index %s", detectorIndices), RestStatus.FORBIDDEN) + )); + } else { + listener.onFailure(e); + } + }); } private void createMonitorFromQueries(String index, List> rulesById, Detector detector, ActionListener> listener, WriteRequest.RefreshPolicy refreshPolicy) throws SigmaError, IOException { @@ -595,7 +618,6 @@ class AsyncIndexDetectorsAction { void start() { try { - TransportIndexDetectorAction.this.threadPool.getThreadContext().stashContext(); if (!detectorIndices.detectorIndexExists()) { diff --git a/src/test/java/org/opensearch/securityanalytics/SecurityAnalyticsRestTestCase.java b/src/test/java/org/opensearch/securityanalytics/SecurityAnalyticsRestTestCase.java index 086457225..7cb512963 100644 --- a/src/test/java/org/opensearch/securityanalytics/SecurityAnalyticsRestTestCase.java +++ b/src/test/java/org/opensearch/securityanalytics/SecurityAnalyticsRestTestCase.java @@ -18,6 +18,7 @@ import org.junit.After; import org.opensearch.action.admin.indices.mapping.get.GetMappingsResponse; import org.opensearch.action.search.SearchResponse; +import org.opensearch.client.Client; import org.opensearch.client.Request; import org.opensearch.client.RequestOptions; import org.opensearch.client.Response; @@ -375,6 +376,20 @@ public static SearchResponse executeSearchRequest(String indexName, String query return SearchResponse.fromXContent(parser); } + public static SearchResponse executeSearchRequest(RestClient client, String indexName, String queryJson) throws IOException { + + Request request = new Request("GET", indexName + "/_search"); + request.setJsonEntity(queryJson); + Response response = client.performRequest(request); + + XContentParser parser = JsonXContent.jsonXContent.createParser( + new NamedXContentRegistry(ClusterModule.getNamedXWriteables()), + DeprecationHandler.THROW_UNSUPPORTED_OPERATION, + response.getEntity().getContent() + ); + return SearchResponse.fromXContent(parser); + } + protected HttpEntity toHttpEntity(Detector detector) throws IOException { return new StringEntity(toJsonString(detector), ContentType.APPLICATION_JSON); } @@ -997,6 +1012,41 @@ protected RestClient buildClient(Settings settings, HttpHost[] hosts) throws IOE } + protected void createCustomRoleWithIndexPermissions(String name, List clusterPermissions, List indexPermission, List indexPatterns) throws IOException { + Response response; + try { + response = client().performRequest(new Request("GET", String.format(Locale.getDefault(), "/_plugins/_security/api/roles/%s", name))); + } catch (ResponseException ex) { + response = ex.getResponse(); + } + // Role already exists + if(response.getStatusLine().getStatusCode() == RestStatus.OK.getStatus()) { + return; + } + + Request request = new Request("PUT", String.format(Locale.getDefault(), "/_plugins/_security/api/roles/%s", name)); + String clusterPermissionsStr = clusterPermissions.stream().map(p -> "\"" + p + "\"").collect(Collectors.joining(",")); + String indexPermissionsStr = indexPermission.stream().map(p -> "\"" + p + "\"").collect(Collectors.joining(",")); + String indexPatternsStr = indexPatterns.stream().map(p -> "\"" + p + "\"").collect(Collectors.joining(",")); + + String entity = "{\n" + + "\"cluster_permissions\": [\n" + + "" + clusterPermissionsStr + "\n" + + "], \n" + + "\"index_permissions\": [\n" + + "{" + + "\"fls\": [], " + + "\"masked_fields\": [], " + + "\"allowed_actions\": [" + indexPermissionsStr + "], " + + "\"index_patterns\": [" + indexPatternsStr + "]" + + "}" + + "], " + + "\"tenant_permissions\": []" + + "}"; + + request.setJsonEntity(entity); + client().performRequest(request); + } protected void createCustomRole(String name, String clusterPermissions) throws IOException { Request request = new Request("PUT", String.format(Locale.getDefault(), "/_plugins/_security/api/roles/%s", name)); @@ -1048,6 +1098,13 @@ protected void createUserWithDataAndCustomRole(String userName, String userPass createUserRolesMapping(roleName, users); } + protected void createUserWithDataAndCustomRole(String userName, String userPasswd, String roleName, String[] backendRoles, List clusterPermissions, List indexPermissions, List indexPatterns) throws IOException { + String[] users = {userName}; + createUser(userName, userPasswd, backendRoles); + createCustomRoleWithIndexPermissions(roleName, clusterPermissions, indexPermissions, indexPatterns); + createUserRolesMapping(roleName, users); + } + protected void createUserWithData(String userName, String userPasswd, String roleName, String[] backendRoles ) throws IOException { String[] users = {userName}; createUser(userName, userPasswd, backendRoles); @@ -1061,6 +1118,11 @@ protected void deleteUser(String name) throws IOException { client().performRequest(request); } + protected void deleteRole(String name) throws IOException{ + Request request = new Request("DELETE", String.format(Locale.getDefault(), "/_plugins/_security/api/roles/%s", name)); + client().performRequest(request); + } + @Override protected boolean preserveIndicesUponCompletion() { return true; diff --git a/src/test/java/org/opensearch/securityanalytics/resthandler/SecureDetectorRestApiIT.java b/src/test/java/org/opensearch/securityanalytics/resthandler/SecureDetectorRestApiIT.java index e4f760f93..185afbff1 100644 --- a/src/test/java/org/opensearch/securityanalytics/resthandler/SecureDetectorRestApiIT.java +++ b/src/test/java/org/opensearch/securityanalytics/resthandler/SecureDetectorRestApiIT.java @@ -38,17 +38,27 @@ public class SecureDetectorRestApiIT extends SecurityAnalyticsRestTestCase { static String SECURITY_ANALYTICS_FULL_ACCESS_ROLE = "security_analytics_full_access"; static String SECURITY_ANALYTICS_READ_ACCESS_ROLE = "security_analytics_read_access"; static String TEST_HR_BACKEND_ROLE = "HR"; - static String CUSTOM_HR_ROLE = "HR"; - static String TEST_IT_BACKEND_ROLE = "IT"; - - static Map roleToPermissionsMap = Map.ofEntries( Map.entry(SECURITY_ANALYTICS_FULL_ACCESS_ROLE, "cluster:admin/opendistro/securityanalytics/detector/*"), Map.entry(SECURITY_ANALYTICS_READ_ACCESS_ROLE, "cluster:admin/opendistro/securityanalytics/detector/read") ); + private final List clusterPermissions = List.of( + "cluster:admin/opensearch/securityanalytics/detector/*", + "cluster:admin/opendistro/alerting/alerts/*", + "cluster:admin/opendistro/alerting/findings/*", + "cluster:admin/opensearch/securityanalytics/mapping/*", + "cluster:admin/opensearch/securityanalytics/rule/*" + ); + + private final List indexPermissions = List.of( + "indices:admin/mappings/get", + "indices:admin/mapping/put", + "indices:data/read/search" + ); + private RestClient userClient; private final String user = "userDetector"; @@ -70,7 +80,6 @@ public void cleanup() throws IOException { @SuppressWarnings("unchecked") public void testCreateDetectorWithFullAccess() throws IOException { - String[] users = {user}; //createUserRolesMapping("alerting_full_access", users); String index = createTestIndex(client(), randomIndex(), windowsIndexMapping(), Settings.EMPTY); @@ -170,4 +179,151 @@ public void testCreateDetectorWithFullAccess() throws IOException { userReadOnlyClient.close(); deleteUser(userRead); } + + /** + * 1. Tries to create a detector as a user without access to the given index pattern and verifies that it is forbidden + * 2. Creates a detector as a user with access to a given index + * @throws IOException + */ + public void testCreateDetectorIndexAccess() throws IOException { + String[] backendRoles = { TEST_IT_BACKEND_ROLE }; + + String userWithoutAccess = "user"; + String roleNameWithoutIndexPatternAccess = "test-role"; + String testIndexPattern = "test*"; + createUserWithDataAndCustomRole(userWithoutAccess, userWithoutAccess, roleNameWithoutIndexPatternAccess, backendRoles, clusterPermissions, indexPermissions, List.of(testIndexPattern)); + RestClient clientWithoutAccess = null; + + String userWithAccess = "user1"; + String roleNameWithIndexPatternAccess = "test-role-1"; + String windowsIndexPattern = "windows*"; + createUserWithDataAndCustomRole(userWithAccess, userWithAccess, roleNameWithIndexPatternAccess, backendRoles, clusterPermissions, indexPermissions, List.of(windowsIndexPattern)); + RestClient clientWithAccess = null; + try { + clientWithoutAccess = new SecureRestClientBuilder(getClusterHosts().toArray(new HttpHost[]{}), isHttps(), userWithoutAccess, userWithoutAccess).setSocketTimeout(60000).build(); + clientWithAccess = new SecureRestClientBuilder(getClusterHosts().toArray(new HttpHost[]{}), isHttps(), userWithAccess, userWithAccess).setSocketTimeout(60000).build(); + //createUserRolesMapping("alerting_full_access", users); + String index = createTestIndex(client(), randomIndex(), windowsIndexMapping(), Settings.EMPTY); + + // Execute CreateMappingsAction to add alias mapping for index + Request createMappingRequest = new Request("POST", SecurityAnalyticsPlugin.MAPPER_BASE_URI); + // both req params and req body are supported + createMappingRequest.setJsonEntity( + "{ \"index_name\":\"" + index + "\"," + + " \"rule_topic\":\"" + randomDetectorType() + "\", " + + " \"partial\":true" + + "}" + ); + Response response = userClient.performRequest(createMappingRequest); + assertEquals(HttpStatus.SC_OK, response.getStatusLine().getStatusCode()); + + Detector detector = randomDetector(getRandomPrePackagedRules()); + + try { + makeRequest(clientWithoutAccess, "POST", SecurityAnalyticsPlugin.DETECTOR_BASE_URI, Collections.emptyMap(), toHttpEntity(detector)); + } catch (ResponseException e) { + assertEquals("Create detector error status", RestStatus.FORBIDDEN, restStatus(e.getResponse())); + } + + Response createResponse = makeRequest(clientWithAccess, "POST", SecurityAnalyticsPlugin.DETECTOR_BASE_URI, Collections.emptyMap(), toHttpEntity(detector)); + assertEquals("Create detector failed", RestStatus.CREATED, restStatus(createResponse)); + + Map responseBody = asMap(createResponse); + + String createdId = responseBody.get("_id").toString(); + int createdVersion = Integer.parseInt(responseBody.get("_version").toString()); + + assertNotEquals("response is missing Id", Detector.NO_ID, createdId); + assertTrue("incorrect version", createdVersion > 0); + assertEquals("Incorrect Location header", String.format(Locale.getDefault(), "%s/%s", SecurityAnalyticsPlugin.DETECTOR_BASE_URI, createdId), createResponse.getHeader("Location")); + assertFalse(((Map) responseBody.get("detector")).containsKey("rule_topic_index")); + assertFalse(((Map) responseBody.get("detector")).containsKey("findings_index")); + assertFalse(((Map) responseBody.get("detector")).containsKey("alert_index")); + } finally { + if(clientWithoutAccess!= null) clientWithoutAccess.close(); + deleteUser(userWithoutAccess); + deleteRole(roleNameWithoutIndexPatternAccess); + + if (clientWithAccess != null) clientWithAccess.close(); + deleteUser(userWithAccess); + deleteRole(roleNameWithIndexPatternAccess); + } + } + + /** + * 1. Tries to update a detector as a user without access to the given index pattern and verifies that it is forbidden + * 2. Updates a detector as a user with access to a given index + * @throws IOException + */ + public void testUpdateDetectorIndexAccess() throws IOException { + String[] backendRoles = { TEST_IT_BACKEND_ROLE }; + + String userWithoutAccess = "user"; + String roleNameWithoutIndexPatternAccess = "test-role"; + String testIndexPattern = "test*"; + createUserWithDataAndCustomRole(userWithoutAccess, userWithoutAccess, roleNameWithoutIndexPatternAccess, backendRoles, clusterPermissions, indexPermissions, List.of(testIndexPattern)); + RestClient clientWithoutAccess = null; + + + String userWithAccess = "user1"; + String roleNameWithIndexPatternAccess = "test-role-1"; + String windowsIndexPattern = "windows*"; + createUserWithDataAndCustomRole(userWithAccess, userWithAccess, roleNameWithIndexPatternAccess, backendRoles, clusterPermissions, indexPermissions, List.of(windowsIndexPattern)); + RestClient clientWithAccess = null; + try { + clientWithoutAccess = new SecureRestClientBuilder(getClusterHosts().toArray(new HttpHost[]{}), isHttps(), userWithoutAccess, userWithoutAccess).setSocketTimeout(60000).build(); + clientWithAccess = new SecureRestClientBuilder(getClusterHosts().toArray(new HttpHost[]{}), isHttps(), userWithAccess, userWithAccess).setSocketTimeout(60000).build(); + //createUserRolesMapping("alerting_full_access", users); + String index = createTestIndex(client(), randomIndex(), windowsIndexMapping(), Settings.EMPTY); + + // Execute CreateMappingsAction to add alias mapping for index + Request createMappingRequest = new Request("POST", SecurityAnalyticsPlugin.MAPPER_BASE_URI); + // both req params and req body are supported + createMappingRequest.setJsonEntity( + "{ \"index_name\":\"" + index + "\"," + + " \"rule_topic\":\"" + randomDetectorType() + "\", " + + " \"partial\":true" + + "}" + ); + Response response = clientWithAccess.performRequest(createMappingRequest); + assertEquals(HttpStatus.SC_OK, response.getStatusLine().getStatusCode()); + + Detector detector = randomDetector(getRandomPrePackagedRules()); + + Response createResponse = makeRequest(clientWithAccess, "POST", SecurityAnalyticsPlugin.DETECTOR_BASE_URI, Collections.emptyMap(), toHttpEntity(detector)); + assertEquals("Create detector failed", RestStatus.CREATED, restStatus(createResponse)); + + Map responseBody = asMap(createResponse); + + String createdId = responseBody.get("_id").toString(); + int createdVersion = Integer.parseInt(responseBody.get("_version").toString()); + + assertNotEquals("response is missing Id", Detector.NO_ID, createdId); + assertTrue("incorrect version", createdVersion > 0); + assertEquals("Incorrect Location header", String.format(Locale.getDefault(), "%s/%s", SecurityAnalyticsPlugin.DETECTOR_BASE_URI, createdId), createResponse.getHeader("Location")); + assertFalse(((Map) responseBody.get("detector")).containsKey("rule_topic_index")); + assertFalse(((Map) responseBody.get("detector")).containsKey("findings_index")); + assertFalse(((Map) responseBody.get("detector")).containsKey("alert_index")); + + String detectorId = responseBody.get("_id").toString(); + + try { + makeRequest(clientWithoutAccess, "PUT", SecurityAnalyticsPlugin.DETECTOR_BASE_URI + "/" + detectorId, Collections.emptyMap(), toHttpEntity(detector)); + } catch (ResponseException e) { + assertEquals("Update detector error status", RestStatus.FORBIDDEN, restStatus(e.getResponse())); + } + + Response updateResponse = makeRequest(clientWithAccess, "PUT", SecurityAnalyticsPlugin.DETECTOR_BASE_URI + "/" + detectorId, Collections.emptyMap(), toHttpEntity(detector)); + assertEquals("Update detector failed", RestStatus.OK, restStatus(updateResponse)); + } finally { + if (clientWithoutAccess != null) clientWithoutAccess.close(); + deleteUser(userWithoutAccess); + deleteRole(roleNameWithoutIndexPatternAccess); + + if (clientWithAccess != null) clientWithAccess.close(); + deleteUser(userWithAccess); + deleteRole(roleNameWithIndexPatternAccess); + } + } + } \ No newline at end of file