From 48b7acfd2427d9927d4a62ca658592c4b7a5bbb2 Mon Sep 17 00:00:00 2001 From: Vitalii Dmyterko <92328789+vitaliidm@users.noreply.github.com> Date: Thu, 10 Aug 2023 10:37:29 +0100 Subject: [PATCH] [Security Solution][Detection engine] skips geo_point non-ecs validation (#163487) ## Summary While reviewing https://github.com/elastic/kibana/pull/163414, I have noticed, that `geo_point` type still gets validated in `computeIsEcsCompliant`, that could lead to removing some of the complex `geo_point` type representations, notably object like ones, for example ```JSON { "type": "Point", "coordinates": [-88.34, 20.12], } ``` In this PR, I completely removing validation for this field type (we even have functional test to verify it) With changes introduced in https://github.com/elastic/kibana/pull/163414, alert will be created even with not valid `geo_point` fields, instead of failing (changed e2e test name) ### Checklist Delete any items that are not applicable to this PR. - [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios --- .../utils/strip_non_ecs_fields.test.ts | 68 +++++++++++++++++++ .../factories/utils/strip_non_ecs_fields.ts | 8 ++- .../rule_execution_logic/non_ecs_fields.ts | 4 +- 3 files changed, 77 insertions(+), 3 deletions(-) diff --git a/x-pack/plugins/security_solution/server/lib/detection_engine/rule_types/factories/utils/strip_non_ecs_fields.test.ts b/x-pack/plugins/security_solution/server/lib/detection_engine/rule_types/factories/utils/strip_non_ecs_fields.test.ts index 1d9d349dd0b5a..de4e9982c852d 100644 --- a/x-pack/plugins/security_solution/server/lib/detection_engine/rule_types/factories/utils/strip_non_ecs_fields.test.ts +++ b/x-pack/plugins/security_solution/server/lib/detection_engine/rule_types/factories/utils/strip_non_ecs_fields.test.ts @@ -551,4 +551,72 @@ describe('stripNonEcsFields', () => { expect(removed).toEqual([]); }); }); + + // geo_point is too complex so we going to skip its validation + describe('geo_point field', () => { + it('should not strip invalid geo_point field', () => { + const { result, removed } = stripNonEcsFields({ + 'client.location.geo': 'invalid geo_point', + }); + + expect(result).toEqual({ + 'client.location.geo': 'invalid geo_point', + }); + expect(removed).toEqual([]); + }); + + it('should not strip valid geo_point fields', () => { + expect( + stripNonEcsFields({ + 'client.geo.location': [0, 90], + }).result + ).toEqual({ + 'client.geo.location': [0, 90], + }); + + expect( + stripNonEcsFields({ + 'client.geo.location': { + type: 'Point', + coordinates: [-88.34, 20.12], + }, + }).result + ).toEqual({ + 'client.geo.location': { + type: 'Point', + coordinates: [-88.34, 20.12], + }, + }); + + expect( + stripNonEcsFields({ + 'client.geo.location': 'POINT (-71.34 41.12)', + }).result + ).toEqual({ + 'client.geo.location': 'POINT (-71.34 41.12)', + }); + + expect( + stripNonEcsFields({ + client: { + geo: { + location: { + lat: 41.12, + lon: -71.34, + }, + }, + }, + }).result + ).toEqual({ + client: { + geo: { + location: { + lat: 41.12, + lon: -71.34, + }, + }, + }, + }); + }); + }); }); diff --git a/x-pack/plugins/security_solution/server/lib/detection_engine/rule_types/factories/utils/strip_non_ecs_fields.ts b/x-pack/plugins/security_solution/server/lib/detection_engine/rule_types/factories/utils/strip_non_ecs_fields.ts index 975b2b643a4e7..62e5c9211c1be 100644 --- a/x-pack/plugins/security_solution/server/lib/detection_engine/rule_types/factories/utils/strip_non_ecs_fields.ts +++ b/x-pack/plugins/security_solution/server/lib/detection_engine/rule_types/factories/utils/strip_non_ecs_fields.ts @@ -57,10 +57,11 @@ const ecsObjectFields = getEcsObjectFields(); /** * checks if path is a valid Ecs object type (object or flattened) + * geo_point also can be object */ const getIsEcsFieldObject = (path: string) => { const ecsField = ecsFieldMap[path as keyof typeof ecsFieldMap]; - return ['object', 'flattened'].includes(ecsField?.type) || ecsObjectFields[path]; + return ['object', 'flattened', 'geo_point'].includes(ecsField?.type) || ecsObjectFields[path]; }; /** @@ -117,6 +118,11 @@ const computeIsEcsCompliant = (value: SourceField, path: string) => { const ecsField = ecsFieldMap[path as keyof typeof ecsFieldMap]; const isEcsFieldObject = getIsEcsFieldObject(path); + // do not validate geo_point, since it's very complex type that can be string/array/object + if (ecsField?.type === 'geo_point') { + return true; + } + // validate if value is a long type if (ecsField?.type === 'long') { return isValidLongType(value); diff --git a/x-pack/test/detection_engine_api_integration/security_and_spaces/rule_execution_logic/non_ecs_fields.ts b/x-pack/test/detection_engine_api_integration/security_and_spaces/rule_execution_logic/non_ecs_fields.ts index f315dfabb4d86..2f2115333d5c2 100644 --- a/x-pack/test/detection_engine_api_integration/security_and_spaces/rule_execution_logic/non_ecs_fields.ts +++ b/x-pack/test/detection_engine_api_integration/security_and_spaces/rule_execution_logic/non_ecs_fields.ts @@ -258,8 +258,8 @@ export default ({ getService }: FtrProviderContext) => { // we don't validate it because geo_point is very complex type with many various representations: array, different object, string with few valid patterns // more on geo_point type https://www.elastic.co/guide/en/elasticsearch/reference/current/geo-point.html - // since .alerts-* indices allow _ignore_malformed option, alert will be indexed for this document - it('should fail creating alert when ECS field mapping is geo_point', async () => { + // since .alerts-* indices allow _ignore_malformed option, alert will be created for this document + it('should not fail creating alert when ECS field mapping is geo_point', async () => { const document = { client: { geo: {