From 8e63d89cc9d71825dc09185fcc18a2ef474b4390 Mon Sep 17 00:00:00 2001 From: Joanne Wang Date: Thu, 7 Mar 2024 09:40:44 -0800 Subject: [PATCH] Fix duplicate ecs mappings which returns incorrect log index field in mapping view API (#786) (#788) * field mapping changes Signed-off-by: Joanne Wang * add integ test Signed-off-by: Joanne Wang * turn unmappedfieldaliases as set and add integ test Signed-off-by: Joanne Wang * add comments Signed-off-by: Joanne Wang * fix integ tests Signed-off-by: Joanne Wang * moved logic to method for better readability Signed-off-by: Joanne Wang --------- Signed-off-by: Joanne Wang --- .../mapper/MapperService.java | 83 +++++++-- .../mapper/MapperRestApiIT.java | 171 ++++++++++++++++++ .../resthandler/OCSFDetectorRestApiIT.java | 4 +- 3 files changed, 238 insertions(+), 20 deletions(-) diff --git a/src/main/java/org/opensearch/securityanalytics/mapper/MapperService.java b/src/main/java/org/opensearch/securityanalytics/mapper/MapperService.java index e50af189a..c86a36535 100644 --- a/src/main/java/org/opensearch/securityanalytics/mapper/MapperService.java +++ b/src/main/java/org/opensearch/securityanalytics/mapper/MapperService.java @@ -33,6 +33,8 @@ import org.opensearch.cluster.metadata.MappingMetadata; import org.opensearch.cluster.service.ClusterService; import org.opensearch.common.xcontent.XContentFactory; +import org.opensearch.core.action.ActionListener; +import org.opensearch.core.rest.RestStatus; import org.opensearch.core.xcontent.XContentBuilder; import org.opensearch.core.rest.RestStatus; import org.opensearch.securityanalytics.action.GetIndexMappingsResponse; @@ -43,6 +45,16 @@ import org.opensearch.securityanalytics.util.IndexUtils; import org.opensearch.securityanalytics.util.SecurityAnalyticsException; +import java.io.IOException; +import java.util.ArrayList; +import java.util.Collection; +import java.util.HashMap; +import java.util.HashSet; +import java.util.List; +import java.util.Map; +import java.util.Optional; +import java.util.Set; +import java.util.stream.Collectors; import static org.opensearch.securityanalytics.mapper.MapperUtils.PATH; import static org.opensearch.securityanalytics.mapper.MapperUtils.PROPERTIES; @@ -80,6 +92,7 @@ public void createMappingAction(String indexName, String logType, String aliasMa if (IndexUtils.isDataStream(indexName, this.clusterService.state())) { String writeIndex = IndexUtils.getWriteIndex(indexName, this.clusterService.state()); if (writeIndex != null) { + log.debug("Write index for {} is {}", indexName, writeIndex); index = writeIndex; } } @@ -91,6 +104,7 @@ public void onResponse(GetMappingsResponse getMappingsResponse) { applyAliasMappings(getMappingsResponse.getMappings(), logType, aliasMappings, partial, new ActionListener<>() { @Override public void onResponse(Collection createMappingResponse) { + log.debug("Completed create mappings for {}", indexName); // We will return ack==false if one of the requests returned that // else return ack==true Optional notAckd = createMappingResponse.stream() @@ -109,6 +123,7 @@ public void onResponse(Collection createMappingResponse) { @Override public void onFailure(Exception e) { + log.debug("Failed to create mappings for {}", indexName ); actionListener.onFailure(e); } }); @@ -122,7 +137,7 @@ public void onFailure(Exception e) { } private void applyAliasMappings(Map indexMappings, String logType, String aliasMappings, boolean partial, ActionListener> actionListener) { - int numOfIndices = indexMappings.size(); + int numOfIndices = indexMappings.size(); GroupedActionListener doCreateMappingActionsListener = new GroupedActionListener(new ActionListener>() { @Override @@ -150,12 +165,13 @@ public void onFailure(Exception e) { /** * Applies alias mappings to index. - * @param indexName Index name + * + * @param indexName Index name * @param mappingMetadata Index mappings - * @param logType Rule topic spcifying specific alias templates - * @param aliasMappings User-supplied alias mappings - * @param partial Partial flag indicating if we should apply mappings partially, in case source index doesn't have all paths specified in alias mappings - * @param actionListener actionListener used to return response/error + * @param logType Rule topic spcifying specific alias templates + * @param aliasMappings User-supplied alias mappings + * @param partial Partial flag indicating if we should apply mappings partially, in case source index doesn't have all paths specified in alias mappings + * @param actionListener actionListener used to return response/error */ private void doCreateMapping( String indexName, @@ -450,9 +466,10 @@ public void onFailure(Exception e) { /** * Constructs Mappings View of index - * @param logType Log Type + * + * @param logType Log Type * @param actionListener Action Listener - * @param concreteIndex Concrete Index name for which we're computing Mappings View + * @param concreteIndex Concrete Index name for which we're computing Mappings View */ private void doGetMappingsView(String logType, ActionListener actionListener, String concreteIndex) { GetMappingsRequest getMappingsRequest = new GetMappingsRequest().indices(concreteIndex); @@ -477,17 +494,20 @@ public void onResponse(GetMappingsResponse getMappingsResponse) { String rawPath = requiredField.getRawField(); String ocsfPath = requiredField.getOcsf(); if (allFieldsFromIndex.contains(rawPath)) { - if (alias != null) { - // Maintain list of found paths in index - applyableAliases.add(alias); - } else { - applyableAliases.add(rawPath); + // if the alias was already added into applyable aliases, then skip to avoid duplicates + if (!applyableAliases.contains(alias) && !applyableAliases.contains(rawPath)) { + if (alias != null) { + // Maintain list of found paths in index + applyableAliases.add(alias); + } else { + applyableAliases.add(rawPath); + } + pathsOfApplyableAliases.add(rawPath); } - pathsOfApplyableAliases.add(rawPath); } else if (allFieldsFromIndex.contains(ocsfPath)) { applyableAliases.add(alias); pathsOfApplyableAliases.add(ocsfPath); - } else if ((alias == null && allFieldsFromIndex.contains(rawPath) == false) || allFieldsFromIndex.contains(alias) == false) { + } else if ((alias == null && allFieldsFromIndex.contains(rawPath) == false) || allFieldsFromIndex.contains(alias) == false) { if (alias != null) { // we don't want to send back aliases which have same name as existing field in index unmappedFieldAliases.add(alias); @@ -497,13 +517,21 @@ public void onResponse(GetMappingsResponse getMappingsResponse) { } } + // turn unmappedFieldAliases into a set to remove duplicates + Set setOfUnmappedFieldAliases = new HashSet<>(unmappedFieldAliases); + + // filter out aliases that were included in applyableAliases already + List filteredUnmappedFieldAliases = setOfUnmappedFieldAliases.stream() + .filter(e -> false == applyableAliases.contains(e)) + .collect(Collectors.toList()); + Map> aliasMappingFields = new HashMap<>(); XContentBuilder aliasMappingsObj = XContentFactory.jsonBuilder().startObject(); for (LogType.Mapping mapping: requiredFields) { if (allFieldsFromIndex.contains(mapping.getOcsf())) { aliasMappingFields.put(mapping.getEcs(), Map.of("type", "alias", "path", mapping.getOcsf())); } else if (mapping.getEcs() != null) { - aliasMappingFields.put(mapping.getEcs(), Map.of("type", "alias", "path", mapping.getRawField())); + shouldUpdateEcsMappingAndMaybeUpdates(mapping, aliasMappingFields, pathsOfApplyableAliases); } else if (mapping.getEcs() == null) { aliasMappingFields.put(mapping.getRawField(), Map.of("type", "alias", "path", mapping.getRawField())); } @@ -518,9 +546,8 @@ public void onResponse(GetMappingsResponse getMappingsResponse) { .stream() .filter(e -> pathsOfApplyableAliases.contains(e) == false) .collect(Collectors.toList()); - actionListener.onResponse( - new GetMappingsViewResponse(aliasMappings, unmappedIndexFields, unmappedFieldAliases) + new GetMappingsViewResponse(aliasMappings, unmappedIndexFields, filteredUnmappedFieldAliases) ); } catch (Exception e) { actionListener.onFailure(e); @@ -534,6 +561,26 @@ public void onFailure(Exception e) { }); } + /** + * Only updates the alias mapping fields if the ecs key has not been mapped yet + * or if pathOfApplyableAliases contains the raw field + * + * @param mapping + * @param aliasMappingFields + * @param pathsOfApplyableAliases + */ + private static void shouldUpdateEcsMappingAndMaybeUpdates(LogType.Mapping mapping, Map> aliasMappingFields, List pathsOfApplyableAliases) { + // check if aliasMappingFields already contains a key + if (aliasMappingFields.containsKey(mapping.getEcs())) { + // if the pathOfApplyableAliases contains the raw field, then override the existing map + if (pathsOfApplyableAliases.contains(mapping.getRawField())) { + aliasMappingFields.put(mapping.getEcs(), Map.of("type", "alias", "path", mapping.getRawField())); + } + } else { + aliasMappingFields.put(mapping.getEcs(), Map.of("type", "alias", "path", mapping.getRawField())); + } + } + /** * Given index name, resolves it to single concrete index, depending on what initial indexName is. * In case of Datastream or Alias, WriteIndex would be returned. In case of index pattern, newest index by creation date would be returned. diff --git a/src/test/java/org/opensearch/securityanalytics/mapper/MapperRestApiIT.java b/src/test/java/org/opensearch/securityanalytics/mapper/MapperRestApiIT.java index 91057069f..b3f16baf0 100644 --- a/src/test/java/org/opensearch/securityanalytics/mapper/MapperRestApiIT.java +++ b/src/test/java/org/opensearch/securityanalytics/mapper/MapperRestApiIT.java @@ -390,6 +390,114 @@ public void testGetMappingsViewLinuxSuccess() throws IOException { assertEquals(HttpStatus.SC_OK, response.getStatusLine().getStatusCode()); } + // Tests mappings where multiple raw fields correspond to one ecs value + public void testGetMappingsViewWindowsSuccess() throws IOException { + + String testIndexName = "get_mappings_view_index"; + + createSampleWindex(testIndexName); + + // Execute GetMappingsViewAction to add alias mapping for index + Request request = new Request("GET", SecurityAnalyticsPlugin.MAPPINGS_VIEW_BASE_URI); + // both req params and req body are supported + request.addParameter("index_name", testIndexName); + request.addParameter("rule_topic", "windows"); + Response response = client().performRequest(request); + assertEquals(HttpStatus.SC_OK, response.getStatusLine().getStatusCode()); + Map respMap = responseAsMap(response); + + // Verify alias mappings + Map props = (Map) respMap.get("properties"); + assertEquals(3, props.size()); + assertTrue(props.containsKey("winlog.event_data.LogonType")); + assertTrue(props.containsKey("winlog.provider_name")); + assertTrue(props.containsKey("host.hostname")); + + // Verify unmapped index fields + List unmappedIndexFields = (List) respMap.get("unmapped_index_fields"); + assertEquals(3, unmappedIndexFields.size()); + assert(unmappedIndexFields.contains("plain1")); + assert(unmappedIndexFields.contains("ParentUser.first")); + assert(unmappedIndexFields.contains("ParentUser.last")); + + // Verify unmapped field aliases + List filteredUnmappedFieldAliases = (List) respMap.get("unmapped_field_aliases"); + assertEquals(191, filteredUnmappedFieldAliases.size()); + assert(!filteredUnmappedFieldAliases.contains("winlog.event_data.LogonType")); + assert(!filteredUnmappedFieldAliases.contains("winlog.provider_name")); + assert(!filteredUnmappedFieldAliases.contains("host.hostname")); + List> iocFieldsList = (List>) respMap.get(GetMappingsViewResponse.THREAT_INTEL_FIELD_ALIASES); + assertEquals(iocFieldsList.size(), 1); + + // Index a doc for a field with multiple raw fields corresponding to one ecs field + indexDoc(testIndexName, "1", "{ \"EventID\": 1 }"); + // Execute GetMappingsViewAction to add alias mapping for index + request = new Request("GET", SecurityAnalyticsPlugin.MAPPINGS_VIEW_BASE_URI); + // both req params and req body are supported + request.addParameter("index_name", testIndexName); + request.addParameter("rule_topic", "windows"); + response = client().performRequest(request); + assertEquals(HttpStatus.SC_OK, response.getStatusLine().getStatusCode()); + respMap = responseAsMap(response); + + // Verify alias mappings + props = (Map) respMap.get("properties"); + assertEquals(4, props.size()); + assertTrue(props.containsKey("winlog.event_id")); + + // verify unmapped index fields + unmappedIndexFields = (List) respMap.get("unmapped_index_fields"); + assertEquals(3, unmappedIndexFields.size()); + + // verify unmapped field aliases + filteredUnmappedFieldAliases = (List) respMap.get("unmapped_field_aliases"); + assertEquals(190, filteredUnmappedFieldAliases.size()); + assert(!filteredUnmappedFieldAliases.contains("winlog.event_id")); + } + + // Tests mappings where multiple raw fields correspond to one ecs value and all fields are present in the index + public void testGetMappingsViewMulitpleRawFieldsSuccess() throws IOException { + + String testIndexName = "get_mappings_view_index"; + + createSampleWindex(testIndexName); + String sampleDoc = "{" + + " \"EventID\": 1," + + " \"EventId\": 2," + + " \"event_uid\": 3" + + "}"; + indexDoc(testIndexName, "1", sampleDoc); + + // Execute GetMappingsViewAction to add alias mapping for index + Request request = new Request("GET", SecurityAnalyticsPlugin.MAPPINGS_VIEW_BASE_URI); + // both req params and req body are supported + request.addParameter("index_name", testIndexName); + request.addParameter("rule_topic", "windows"); + Response response = client().performRequest(request); + assertEquals(HttpStatus.SC_OK, response.getStatusLine().getStatusCode()); + Map respMap = responseAsMap(response); + + // Verify alias mappings + Map props = (Map) respMap.get("properties"); + assertEquals(4, props.size()); + assertTrue(props.containsKey("winlog.event_data.LogonType")); + assertTrue(props.containsKey("winlog.provider_name")); + assertTrue(props.containsKey("host.hostname")); + assertTrue(props.containsKey("winlog.event_id")); + + // Verify unmapped index fields + List unmappedIndexFields = (List) respMap.get("unmapped_index_fields"); + assertEquals(5, unmappedIndexFields.size()); + + // Verify unmapped field aliases + List filteredUnmappedFieldAliases = (List) respMap.get("unmapped_field_aliases"); + assertEquals(190, filteredUnmappedFieldAliases.size()); + assert(!filteredUnmappedFieldAliases.contains("winlog.event_data.LogonType")); + assert(!filteredUnmappedFieldAliases.contains("winlog.provider_name")); + assert(!filteredUnmappedFieldAliases.contains("host.hostname")); + assert(!filteredUnmappedFieldAliases.contains("winlog.event_id")); + } + public void testCreateMappings_withDatastream_success() throws IOException { String datastream = "test_datastream"; @@ -1273,6 +1381,69 @@ private void createSampleIndex(String indexName, Settings settings, String alias assertEquals(HttpStatus.SC_OK, response.getStatusLine().getStatusCode()); } + private void createSampleWindex(String indexName) throws IOException { + createSampleWindex(indexName, Settings.EMPTY, null); + } + + private void createSampleWindex(String indexName, Settings settings, String aliases) throws IOException { + String indexMapping = + " \"properties\": {" + + " \"LogonType\": {" + + " \"type\": \"integer\"" + + " }," + + " \"Provider\": {" + + " \"type\": \"text\"" + + " }," + + " \"hostname\": {" + + " \"type\": \"text\"" + + " }," + + " \"plain1\": {" + + " \"type\": \"integer\"" + + " }," + + " \"ParentUser\":{" + + " \"type\":\"nested\"," + + " \"properties\":{" + + " \"first\":{" + + " \"type\":\"text\"," + + " \"fields\":{" + + " \"keyword\":{" + + " \"type\":\"keyword\"," + + " \"ignore_above\":256" + + "}" + + "}" + + "}," + + " \"last\":{" + + "\"type\":\"text\"," + + "\"fields\":{" + + " \"keyword\":{" + + " \"type\":\"keyword\"," + + " \"ignore_above\":256" + + "}" + + "}" + + "}" + + "}" + + "}" + + " }"; + + createIndex(indexName, settings, indexMapping, aliases); + + // Insert sample doc with event_uid not explicitly mapped + String sampleDoc = "{" + + " \"LogonType\":1," + + " \"Provider\":\"Microsoft-Windows-Security-Auditing\"," + + " \"hostname\":\"FLUXCAPACITOR\"" + + "}"; + + // Index doc + Request indexRequest = new Request("POST", indexName + "/_doc?refresh=wait_for"); + indexRequest.setJsonEntity(sampleDoc); + Response response = client().performRequest(indexRequest); + assertEquals(HttpStatus.SC_CREATED, response.getStatusLine().getStatusCode()); + // Refresh everything + response = client().performRequest(new Request("POST", "_refresh")); + assertEquals(HttpStatus.SC_OK, response.getStatusLine().getStatusCode()); + } + private void createSampleDatastream(String datastreamName) throws IOException { String indexMapping = " \"properties\": {" + diff --git a/src/test/java/org/opensearch/securityanalytics/resthandler/OCSFDetectorRestApiIT.java b/src/test/java/org/opensearch/securityanalytics/resthandler/OCSFDetectorRestApiIT.java index 248bb8798..07ab7164d 100644 --- a/src/test/java/org/opensearch/securityanalytics/resthandler/OCSFDetectorRestApiIT.java +++ b/src/test/java/org/opensearch/securityanalytics/resthandler/OCSFDetectorRestApiIT.java @@ -436,7 +436,7 @@ public void testOCSFCloudtrailGetMappingsViewApi() throws IOException { assertEquals(20, unmappedIndexFields.size()); // Verify unmapped field aliases List unmappedFieldAliases = (List) respMap.get("unmapped_field_aliases"); - assertEquals(25, unmappedFieldAliases.size()); + assertEquals(24, unmappedFieldAliases.size()); } @SuppressWarnings("unchecked") @@ -502,7 +502,7 @@ public void testRawCloudtrailGetMappingsViewApi() throws IOException { assertEquals(17, unmappedIndexFields.size()); // Verify unmapped field aliases List unmappedFieldAliases = (List) respMap.get("unmapped_field_aliases"); - assertEquals(26, unmappedFieldAliases.size()); + assertEquals(25, unmappedFieldAliases.size()); } @SuppressWarnings("unchecked")