From 4af36fece290263c4fd86f0e06d3e12bdb05f81b Mon Sep 17 00:00:00 2001 From: Xavier Mouligneau Date: Thu, 4 Jan 2024 16:41:30 -0500 Subject: [PATCH] [SECURITY_SOLUTIONS] Only query security alerts with current user (#174216) ## Summary We just got an [SDH#814](https://github.com/elastic/sdh-security-team/issues/814) that tell us that some feature like `KPIs` and `grouping` are not acting as they should be. @PhilippeOberti is doing an investigation to check which feature has been impacted by this bug. This bug has been introduced in this https://github.com/elastic/kibana/pull/112113 since 8.0.0 I think this simple solution should not impact any features. --- .../signals/query_signals_route.test.ts | 27 ++-- .../routes/signals/query_signals_route.ts | 7 +- .../default_license/alerts/index.ts | 1 + .../default_license/alerts/query_alerts.ts | 153 ++++++++++++++++++ 4 files changed, 176 insertions(+), 12 deletions(-) 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/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 7c88d5c9192eee..8ae230d65506bf 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/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 673b6c2d858186..3bd52ebdaacdae 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 } 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 85e2e602a89292..81923173bf50c3 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 00000000000000..6e8e312f9075af --- /dev/null +++ b/x-pack/test/security_solution_api_integration/test_suites/detections_response/default_license/alerts/query_alerts.ts @@ -0,0 +1,153 @@ +/* + * 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. + */ + +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: { + elasticsearch: { + indices: [ + { + names: ['.alerts-security.alerts-default'], + privileges: ['all'], + }, + ], + }, + kibana: [ + { + feature: { + siem: ['all'], + }, + spaces: ['*'], + }, + ], + }, +}; +const roleToAccessSecuritySolutionWithDls = { + name: 'sec_all_spaces_with_dls', + 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 userAllSecWithDls = { + username: 'user_all_sec_with_dls', + password: 'user_all_sec_with_dls', + full_name: 'userAllSecWithDls', + email: 'userAllSecWithDls@elastic.co', + roles: [roleToAccessSecuritySolutionWithDls.name], +}; + +export default ({ getService }: FtrProviderContext) => { + const supertestWithoutAuth = getService('supertestWithoutAuth'); + const esArchiver = getService('esArchiver'); + const security = getService('security'); + + 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, + } + ); + }); + + 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: {} }], + }, + }, + }; + 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(userAllSecWithDls.username, userAllSecWithDls.password) + .set('kbn-xsrf', 'true') + .send(query) + .expect(200); + expect(body.hits.total.value).to.eql(0); + }); + }); +};