From 6db785beb15f1d43abb663f60148a14dbd9bade1 Mon Sep 17 00:00:00 2001 From: Xavier Mouligneau Date: Wed, 3 Jan 2024 14:43:07 -0500 Subject: [PATCH 1/5] only use current user --- .../detection_engine/routes/signals/query_signals_route.ts | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/x-pack/plugins/security_solution/server/lib/detection_engine/routes/signals/query_signals_route.ts b/x-pack/plugins/security_solution/server/lib/detection_engine/routes/signals/query_signals_route.ts index 673b6c2d85818..3bd52ebdaacda 100644 --- a/x-pack/plugins/security_solution/server/lib/detection_engine/routes/signals/query_signals_route.ts +++ b/x-pack/plugins/security_solution/server/lib/detection_engine/routes/signals/query_signals_route.ts @@ -40,6 +40,8 @@ export const querySignalsRoute = ( }, }, async (context, request, response) => { + const esClient = (await context.core).elasticsearch.client.asCurrentUser; + // eslint-disable-next-line @typescript-eslint/naming-convention const { query, aggs, _source, fields, track_total_hits, size, runtime_mappings, sort } = request.body; @@ -58,10 +60,11 @@ export const querySignalsRoute = ( body: '"value" must have at least 1 children', }); } - try { const spaceId = (await context.securitySolution).getSpaceId(); - const result = await ruleDataClient?.getReader({ namespace: spaceId }).search({ + const indexPattern = ruleDataClient?.indexNameWithNamespace(spaceId); + const result = await esClient.search({ + index: indexPattern, body: { query, // Note: I use a spread operator to please TypeScript with aggs: { ...aggs } From 5488066e27acf0d456dc9ea8e193685de713275a Mon Sep 17 00:00:00 2001 From: Xavier Mouligneau Date: Thu, 4 Jan 2024 13:59:05 -0500 Subject: [PATCH 2/5] add test to show how DLS works --- .../default_license/alerts/index.ts | 1 + .../default_license/alerts/query_alerts.ts | 144 ++++++++++++++++++ 2 files changed, 145 insertions(+) create mode 100644 x-pack/test/security_solution_api_integration/test_suites/detections_response/default_license/alerts/query_alerts.ts diff --git a/x-pack/test/security_solution_api_integration/test_suites/detections_response/default_license/alerts/index.ts b/x-pack/test/security_solution_api_integration/test_suites/detections_response/default_license/alerts/index.ts index 85e2e602a8929..81923173bf50c 100644 --- a/x-pack/test/security_solution_api_integration/test_suites/detections_response/default_license/alerts/index.ts +++ b/x-pack/test/security_solution_api_integration/test_suites/detections_response/default_license/alerts/index.ts @@ -15,5 +15,6 @@ export default function ({ loadTestFile }: FtrProviderContext) { loadTestFile(require.resolve('./open_close_alerts')); loadTestFile(require.resolve('./set_alert_tags')); loadTestFile(require.resolve('./assignments')); + loadTestFile(require.resolve('./query_alerts')); }); } diff --git a/x-pack/test/security_solution_api_integration/test_suites/detections_response/default_license/alerts/query_alerts.ts b/x-pack/test/security_solution_api_integration/test_suites/detections_response/default_license/alerts/query_alerts.ts new file mode 100644 index 0000000000000..cd0cc49505a78 --- /dev/null +++ b/x-pack/test/security_solution_api_integration/test_suites/detections_response/default_license/alerts/query_alerts.ts @@ -0,0 +1,144 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License + * 2.0; you may not use this file except in compliance with the Elastic License + * 2.0. + */ + +const roleToAccessSecuritySolution = { + name: 'sec_all_spaces', + privileges: { + elasticsearch: { + indices: [ + { + names: ['.alerts-security.alerts-default'], + privileges: ['all'], + }, + ], + }, + kibana: [ + { + feature: { + siem: ['all'], + }, + spaces: ['*'], + }, + ], + }, +}; +const roleToAccessSecuritySolutionWithDsl = { + name: 'sec_all_spaces_with_dsl', + privileges: { + elasticsearch: { + indices: [ + { + names: ['.alerts-security.alerts-default'], + privileges: ['read'], + query: + '{"wildcard" : { "kibana.alert.ancestors.index" : { "value": ".ds-kibana_does_not_exist" } } }', + }, + ], + }, + kibana: [ + { + feature: { + siem: ['all'], + }, + spaces: ['*'], + }, + ], + }, +}; +const userAllSec = { + username: 'user_all_sec', + password: 'user_all_sec', + full_name: 'userAllSec', + email: 'userAllSec@elastic.co', + roles: [roleToAccessSecuritySolution.name], +}; +const userAllSecWithDsl = { + username: 'user_all_sec_with_dsl', + password: 'user_all_sec_with_dsl', + full_name: 'userAllSecWithDsl', + email: 'userAllSecWithDsl@elastic.co', + roles: [roleToAccessSecuritySolutionWithDsl.name], +}; + +describe('find alert with/without doc level security', () => { + const supertest = getService('supertest'); + const supertestWithoutAuth = getService('supertestWithoutAuth'); + const esArchiver = getService('esArchiver'); + const security = getService('security'); + + before(async () => { + await security.role.create( + roleToAccessSecuritySolution.name, + roleToAccessSecuritySolution.privileges + ); + await security.role.create( + roleToAccessSecuritySolutionWithDsl.name, + roleToAccessSecuritySolutionWithDsl.privileges + ); + await security.user.create(userAllSec.username, { + password: userAllSec.password, + roles: userAllSec.roles, + full_name: userAllSec.full_name, + email: userAllSec.email, + }); + await security.user.create(userAllSecWithDsl.username, { + password: userAllSecWithDsl.password, + roles: userAllSecWithDsl.roles, + full_name: userAllSecWithDsl.full_name, + email: userAllSecWithDsl.email, + }); + + await esArchiver.load( + 'x-pack/test/functional/es_archives/security_solution/alerts/8.8.0_multiple_docs', + { + useCreate: true, + docsOnly: true, + } + ); + }); + after(async () => { + await security.user.delete(userAllSec.username); + await security.user.delete(userAllSecWithDsl.username); + await security.role.delete(roleToAccessSecuritySolution.name); + await security.role.delete(roleToAccessSecuritySolutionWithDsl.name); + await esArchiver.unload( + 'x-pack/test/functional/es_archives/security_solution/alerts/8.8.0_multiple_docs' + ); + }); + it('should return alerts with user who has access to security solution privileges', async () => { + const query = { + query: { + bool: { + should: [{ match_all: {} }], + }, + }, + }; + const { body } = await supertestWithoutAuth + .post(DETECTION_ENGINE_QUERY_SIGNALS_URL) + .auth(userAllSec.username, userAllSec.password) + .set('kbn-xsrf', 'true') + .send(query) + .expect(200); + expect(body.hits.total.value).to.eql(3); + }); + it('should filter out alerts with user who has access to security solution privileges and document level security', async () => { + const query = { + query: { + bool: { + should: [{ match_all: {} }], + }, + }, + }; + const { body } = await supertestWithoutAuth + .post(DETECTION_ENGINE_QUERY_SIGNALS_URL) + .auth(userAllSecWithDsl.username, userAllSecWithDsl.password) + .set('kbn-xsrf', 'true') + .send(query) + .expect(200); + expect(body.hits.total.value).to.eql(0); + }); +}); From a3d77fbeeeb4857d3e9026656f2278bf5db3fe67 Mon Sep 17 00:00:00 2001 From: Xavier Mouligneau Date: Thu, 4 Jan 2024 14:05:39 -0500 Subject: [PATCH 3/5] formatting --- .../detections_response/default_license/alerts/query_alerts.ts | 3 +++ 1 file changed, 3 insertions(+) diff --git a/x-pack/test/security_solution_api_integration/test_suites/detections_response/default_license/alerts/query_alerts.ts b/x-pack/test/security_solution_api_integration/test_suites/detections_response/default_license/alerts/query_alerts.ts index cd0cc49505a78..30e3f6e47859d 100644 --- a/x-pack/test/security_solution_api_integration/test_suites/detections_response/default_license/alerts/query_alerts.ts +++ b/x-pack/test/security_solution_api_integration/test_suites/detections_response/default_license/alerts/query_alerts.ts @@ -100,6 +100,7 @@ describe('find alert with/without doc level security', () => { } ); }); + after(async () => { await security.user.delete(userAllSec.username); await security.user.delete(userAllSecWithDsl.username); @@ -109,6 +110,7 @@ describe('find alert with/without doc level security', () => { 'x-pack/test/functional/es_archives/security_solution/alerts/8.8.0_multiple_docs' ); }); + it('should return alerts with user who has access to security solution privileges', async () => { const query = { query: { @@ -125,6 +127,7 @@ describe('find alert with/without doc level security', () => { .expect(200); expect(body.hits.total.value).to.eql(3); }); + it('should filter out alerts with user who has access to security solution privileges and document level security', async () => { const query = { query: { From 97caa8a1fce99b73c54202563a4ab722041e4b82 Mon Sep 17 00:00:00 2001 From: Xavier Mouligneau Date: Thu, 4 Jan 2024 14:34:55 -0500 Subject: [PATCH 4/5] review --- .../default_license/alerts/query_alerts.ts | 36 +++++++++---------- 1 file changed, 18 insertions(+), 18 deletions(-) diff --git a/x-pack/test/security_solution_api_integration/test_suites/detections_response/default_license/alerts/query_alerts.ts b/x-pack/test/security_solution_api_integration/test_suites/detections_response/default_license/alerts/query_alerts.ts index 30e3f6e47859d..ef769e81f5d1e 100644 --- a/x-pack/test/security_solution_api_integration/test_suites/detections_response/default_license/alerts/query_alerts.ts +++ b/x-pack/test/security_solution_api_integration/test_suites/detections_response/default_license/alerts/query_alerts.ts @@ -26,8 +26,8 @@ const roleToAccessSecuritySolution = { ], }, }; -const roleToAccessSecuritySolutionWithDsl = { - name: 'sec_all_spaces_with_dsl', +const roleToAccessSecuritySolutionWithDls = { + name: 'sec_all_spaces_with_dls', privileges: { elasticsearch: { indices: [ @@ -56,12 +56,12 @@ const userAllSec = { email: 'userAllSec@elastic.co', roles: [roleToAccessSecuritySolution.name], }; -const userAllSecWithDsl = { - username: 'user_all_sec_with_dsl', - password: 'user_all_sec_with_dsl', - full_name: 'userAllSecWithDsl', - email: 'userAllSecWithDsl@elastic.co', - roles: [roleToAccessSecuritySolutionWithDsl.name], +const userAllSecWithDls = { + username: 'user_all_sec_with_dls', + password: 'user_all_sec_with_dls', + full_name: 'userAllSecWithDls', + email: 'userAllSecWithDls@elastic.co', + roles: [roleToAccessSecuritySolutionWithDls.name], }; describe('find alert with/without doc level security', () => { @@ -76,8 +76,8 @@ describe('find alert with/without doc level security', () => { roleToAccessSecuritySolution.privileges ); await security.role.create( - roleToAccessSecuritySolutionWithDsl.name, - roleToAccessSecuritySolutionWithDsl.privileges + roleToAccessSecuritySolutionWithDls.name, + roleToAccessSecuritySolutionWithDls.privileges ); await security.user.create(userAllSec.username, { password: userAllSec.password, @@ -85,11 +85,11 @@ describe('find alert with/without doc level security', () => { full_name: userAllSec.full_name, email: userAllSec.email, }); - await security.user.create(userAllSecWithDsl.username, { - password: userAllSecWithDsl.password, - roles: userAllSecWithDsl.roles, - full_name: userAllSecWithDsl.full_name, - email: userAllSecWithDsl.email, + await security.user.create(userAllSecWithDls.username, { + password: userAllSecWithDls.password, + roles: userAllSecWithDls.roles, + full_name: userAllSecWithDls.full_name, + email: userAllSecWithDls.email, }); await esArchiver.load( @@ -103,9 +103,9 @@ describe('find alert with/without doc level security', () => { after(async () => { await security.user.delete(userAllSec.username); - await security.user.delete(userAllSecWithDsl.username); + await security.user.delete(userAllSecWithDls.username); await security.role.delete(roleToAccessSecuritySolution.name); - await security.role.delete(roleToAccessSecuritySolutionWithDsl.name); + await security.role.delete(roleToAccessSecuritySolutionWithDls.name); await esArchiver.unload( 'x-pack/test/functional/es_archives/security_solution/alerts/8.8.0_multiple_docs' ); @@ -138,7 +138,7 @@ describe('find alert with/without doc level security', () => { }; const { body } = await supertestWithoutAuth .post(DETECTION_ENGINE_QUERY_SIGNALS_URL) - .auth(userAllSecWithDsl.username, userAllSecWithDsl.password) + .auth(userAllSecWithDls.username, userAllSecWithDls.password) .set('kbn-xsrf', 'true') .send(query) .expect(200); From f54224a12810a2dca22806c851b93de3034b9ba5 Mon Sep 17 00:00:00 2001 From: Xavier Mouligneau Date: Thu, 4 Jan 2024 15:34:35 -0500 Subject: [PATCH 5/5] fix tests --- .../signals/query_signals_route.test.ts | 27 ++-- .../default_license/alerts/query_alerts.ts | 146 +++++++++--------- 2 files changed, 93 insertions(+), 80 deletions(-) diff --git a/x-pack/plugins/security_solution/server/lib/detection_engine/routes/signals/query_signals_route.test.ts b/x-pack/plugins/security_solution/server/lib/detection_engine/routes/signals/query_signals_route.test.ts index 7c88d5c9192ee..8ae230d65506b 100644 --- a/x-pack/plugins/security_solution/server/lib/detection_engine/routes/signals/query_signals_route.test.ts +++ b/x-pack/plugins/security_solution/server/lib/detection_engine/routes/signals/query_signals_route.test.ts @@ -17,18 +17,23 @@ import { import { requestContextMock, serverMock, requestMock } from '../__mocks__'; import { querySignalsRoute } from './query_signals_route'; import { ruleRegistryMocks } from '@kbn/rule-registry-plugin/server/mocks'; +import { elasticsearchClientMock } from '@kbn/core-elasticsearch-client-server-mocks'; describe('query for signal', () => { let server: ReturnType; let { context } = requestContextMock.createTools(); - const ruleDataClient = ruleRegistryMocks.createRuleDataClient('.alerts-security.alerts'); + context.core.elasticsearch.client.asCurrentUser.search.mockResolvedValue( + elasticsearchClientMock.createSuccessTransportRequestPromise(getEmptySignalsResponse()) + ); + const ruleDataClient = ruleRegistryMocks.createRuleDataClient('.alerts-security.alerts-'); beforeEach(() => { server = serverMock.create(); ({ context } = requestContextMock.createTools()); - - // eslint-disable-next-line @typescript-eslint/no-explicit-any - ruleDataClient.getReader().search.mockResolvedValue(getEmptySignalsResponse() as any); + context.core.elasticsearch.client.asCurrentUser.search.mockResolvedValue( + // eslint-disable-next-line @typescript-eslint/no-explicit-any + getEmptySignalsResponse() as any + ); querySignalsRoute(server.router, ruleDataClient); }); @@ -41,7 +46,7 @@ describe('query for signal', () => { ); expect(response.status).toEqual(200); - expect(ruleDataClient.getReader().search).toHaveBeenCalledWith( + expect(context.core.elasticsearch.client.asCurrentUser.search).toHaveBeenCalledWith( expect.objectContaining({ body: typicalSignalsQuery(), }) @@ -55,9 +60,9 @@ describe('query for signal', () => { ); expect(response.status).toEqual(200); - expect(ruleDataClient.getReader).toHaveBeenCalledWith( + expect(context.core.elasticsearch.client.asCurrentUser.search).toHaveBeenCalledWith( expect.objectContaining({ - namespace: 'default', + index: '.alerts-security.alerts-default', }) ); }); @@ -69,7 +74,7 @@ describe('query for signal', () => { ); expect(response.status).toEqual(200); - expect(ruleDataClient.getReader().search).toHaveBeenCalledWith( + expect(context.core.elasticsearch.client.asCurrentUser.search).toHaveBeenCalledWith( expect.objectContaining({ body: typicalSignalsQueryAggs(), ignore_unavailable: true }) ); }); @@ -81,7 +86,7 @@ describe('query for signal', () => { ); expect(response.status).toEqual(200); - expect(ruleDataClient.getReader().search).toHaveBeenCalledWith( + expect(context.core.elasticsearch.client.asCurrentUser.search).toHaveBeenCalledWith( expect.objectContaining({ body: { ...typicalSignalsQuery(), @@ -92,7 +97,9 @@ describe('query for signal', () => { }); test('catches error if query throws error', async () => { - ruleDataClient.getReader().search.mockRejectedValue(new Error('Test error')); + context.core.elasticsearch.client.asCurrentUser.search.mockRejectedValue( + new Error('Test error') + ); const response = await server.inject( getSignalsAggsQueryRequest(), requestContextMock.convertContext(context) diff --git a/x-pack/test/security_solution_api_integration/test_suites/detections_response/default_license/alerts/query_alerts.ts b/x-pack/test/security_solution_api_integration/test_suites/detections_response/default_license/alerts/query_alerts.ts index ef769e81f5d1e..6e8e312f9075a 100644 --- a/x-pack/test/security_solution_api_integration/test_suites/detections_response/default_license/alerts/query_alerts.ts +++ b/x-pack/test/security_solution_api_integration/test_suites/detections_response/default_license/alerts/query_alerts.ts @@ -5,6 +5,11 @@ * 2.0. */ +import expect from '@kbn/expect'; + +import { DETECTION_ENGINE_QUERY_SIGNALS_URL } from '@kbn/security-solution-plugin/common/constants'; +import { FtrProviderContext } from '../../../../ftr_provider_context'; + const roleToAccessSecuritySolution = { name: 'sec_all_spaces', privileges: { @@ -64,84 +69,85 @@ const userAllSecWithDls = { roles: [roleToAccessSecuritySolutionWithDls.name], }; -describe('find alert with/without doc level security', () => { - const supertest = getService('supertest'); +export default ({ getService }: FtrProviderContext) => { const supertestWithoutAuth = getService('supertestWithoutAuth'); const esArchiver = getService('esArchiver'); const security = getService('security'); - before(async () => { - await security.role.create( - roleToAccessSecuritySolution.name, - roleToAccessSecuritySolution.privileges - ); - await security.role.create( - roleToAccessSecuritySolutionWithDls.name, - roleToAccessSecuritySolutionWithDls.privileges - ); - await security.user.create(userAllSec.username, { - password: userAllSec.password, - roles: userAllSec.roles, - full_name: userAllSec.full_name, - email: userAllSec.email, - }); - await security.user.create(userAllSecWithDls.username, { - password: userAllSecWithDls.password, - roles: userAllSecWithDls.roles, - full_name: userAllSecWithDls.full_name, - email: userAllSecWithDls.email, - }); + describe('find alert with/without doc level security', () => { + before(async () => { + await security.role.create( + roleToAccessSecuritySolution.name, + roleToAccessSecuritySolution.privileges + ); + await security.role.create( + roleToAccessSecuritySolutionWithDls.name, + roleToAccessSecuritySolutionWithDls.privileges + ); + await security.user.create(userAllSec.username, { + password: userAllSec.password, + roles: userAllSec.roles, + full_name: userAllSec.full_name, + email: userAllSec.email, + }); + await security.user.create(userAllSecWithDls.username, { + password: userAllSecWithDls.password, + roles: userAllSecWithDls.roles, + full_name: userAllSecWithDls.full_name, + email: userAllSecWithDls.email, + }); - await esArchiver.load( - 'x-pack/test/functional/es_archives/security_solution/alerts/8.8.0_multiple_docs', - { - useCreate: true, - docsOnly: true, - } - ); - }); + await esArchiver.load( + 'x-pack/test/functional/es_archives/security_solution/alerts/8.8.0_multiple_docs', + { + useCreate: true, + docsOnly: true, + } + ); + }); - after(async () => { - await security.user.delete(userAllSec.username); - await security.user.delete(userAllSecWithDls.username); - await security.role.delete(roleToAccessSecuritySolution.name); - await security.role.delete(roleToAccessSecuritySolutionWithDls.name); - await esArchiver.unload( - 'x-pack/test/functional/es_archives/security_solution/alerts/8.8.0_multiple_docs' - ); - }); + after(async () => { + await security.user.delete(userAllSec.username); + await security.user.delete(userAllSecWithDls.username); + await security.role.delete(roleToAccessSecuritySolution.name); + await security.role.delete(roleToAccessSecuritySolutionWithDls.name); + await esArchiver.unload( + 'x-pack/test/functional/es_archives/security_solution/alerts/8.8.0_multiple_docs' + ); + }); - it('should return alerts with user who has access to security solution privileges', async () => { - const query = { - query: { - bool: { - should: [{ match_all: {} }], + it('should return alerts with user who has access to security solution privileges', async () => { + const query = { + query: { + bool: { + should: [{ match_all: {} }], + }, }, - }, - }; - const { body } = await supertestWithoutAuth - .post(DETECTION_ENGINE_QUERY_SIGNALS_URL) - .auth(userAllSec.username, userAllSec.password) - .set('kbn-xsrf', 'true') - .send(query) - .expect(200); - expect(body.hits.total.value).to.eql(3); - }); + }; + const { body } = await supertestWithoutAuth + .post(DETECTION_ENGINE_QUERY_SIGNALS_URL) + .auth(userAllSec.username, userAllSec.password) + .set('kbn-xsrf', 'true') + .send(query) + .expect(200); + expect(body.hits.total.value).to.eql(3); + }); - it('should filter out alerts with user who has access to security solution privileges and document level security', async () => { - const query = { - query: { - bool: { - should: [{ match_all: {} }], + it('should filter out alerts with user who has access to security solution privileges and document level security', async () => { + const query = { + query: { + bool: { + should: [{ match_all: {} }], + }, }, - }, - }; - const { body } = await supertestWithoutAuth - .post(DETECTION_ENGINE_QUERY_SIGNALS_URL) - .auth(userAllSecWithDls.username, userAllSecWithDls.password) - .set('kbn-xsrf', 'true') - .send(query) - .expect(200); - expect(body.hits.total.value).to.eql(0); + }; + const { body } = await supertestWithoutAuth + .post(DETECTION_ENGINE_QUERY_SIGNALS_URL) + .auth(userAllSecWithDls.username, userAllSecWithDls.password) + .set('kbn-xsrf', 'true') + .send(query) + .expect(200); + expect(body.hits.total.value).to.eql(0); + }); }); -}); +};