Skip to content

Commit

Permalink
[8.11] [SECURITY_SOLUTIONS] Only query security alerts with current u…
Browse files Browse the repository at this point in the history
…ser (#174216) (#174306)

# Backport

This will backport the following commits from `main` to `8.11`:
- [[SECURITY_SOLUTIONS] Only query security alerts with current user
(#174216)](#174216)

<!--- Backport version: 9.4.3 -->

### Questions ?
Please refer to the [Backport tool
documentation](https://github.com/sqren/backport)

<!--BACKPORT [{"author":{"name":"Xavier
Mouligneau","email":"[email protected]"},"sourceCommit":{"committedDate":"2024-01-04T21:41:30Z","message":"[SECURITY_SOLUTIONS]
Only query security alerts with current user (#174216)\n\n##
Summary\r\n\r\nWe just got
an\r\n[SDH#814](elastic/sdh-security-team#814)
that\r\ntell us that some feature like `KPIs` and `grouping` are not
acting as\r\nthey should be.\r\n\r\n@PhilippeOberti is doing an
investigation to check which feature has\r\nbeen impacted by this bug.
This bug has been introduced in
this\r\nhttps://github.com//pull/112113 since
8.0.0\r\n\r\nI think this simple solution should not impact any
features.","sha":"4af36fece290263c4fd86f0e06d3e12bdb05f81b","branchLabelMapping":{"^v8.13.0$":"main","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["bug","blocker","release_note:fix","impact:critical","Team:ResponseOps","Team:Detection
Alerts","v8.12.0","v8.13.0","v8.11.4"],"title":"[SECURITY_SOLUTIONS]
Only query security alerts with current
user","number":174216,"url":"https://github.com/elastic/kibana/pull/174216","mergeCommit":{"message":"[SECURITY_SOLUTIONS]
Only query security alerts with current user (#174216)\n\n##
Summary\r\n\r\nWe just got
an\r\n[SDH#814](elastic/sdh-security-team#814)
that\r\ntell us that some feature like `KPIs` and `grouping` are not
acting as\r\nthey should be.\r\n\r\n@PhilippeOberti is doing an
investigation to check which feature has\r\nbeen impacted by this bug.
This bug has been introduced in
this\r\nhttps://github.com//pull/112113 since
8.0.0\r\n\r\nI think this simple solution should not impact any
features.","sha":"4af36fece290263c4fd86f0e06d3e12bdb05f81b"}},"sourceBranch":"main","suggestedTargetBranches":["8.12","8.11"],"targetPullRequestStates":[{"branch":"8.12","label":"v8.12.0","branchLabelMappingKey":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"state":"NOT_CREATED"},{"branch":"main","label":"v8.13.0","branchLabelMappingKey":"^v8.13.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/174216","number":174216,"mergeCommit":{"message":"[SECURITY_SOLUTIONS]
Only query security alerts with current user (#174216)\n\n##
Summary\r\n\r\nWe just got
an\r\n[SDH#814](elastic/sdh-security-team#814)
that\r\ntell us that some feature like `KPIs` and `grouping` are not
acting as\r\nthey should be.\r\n\r\n@PhilippeOberti is doing an
investigation to check which feature has\r\nbeen impacted by this bug.
This bug has been introduced in
this\r\nhttps://github.com//pull/112113 since
8.0.0\r\n\r\nI think this simple solution should not impact any
features.","sha":"4af36fece290263c4fd86f0e06d3e12bdb05f81b"}},{"branch":"8.11","label":"v8.11.4","branchLabelMappingKey":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"state":"NOT_CREATED"}]}]
BACKPORT-->

---------

Co-authored-by: Kibana Machine <[email protected]>
  • Loading branch information
XavierM and kibanamachine authored Jan 5, 2024
1 parent 88e3bc9 commit 4273961
Show file tree
Hide file tree
Showing 6 changed files with 9,345 additions and 12 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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<typeof serverMock.create>;
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);
});
Expand All @@ -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(),
})
Expand All @@ -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',
})
);
});
Expand All @@ -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 })
);
});
Expand All @@ -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(),
Expand All @@ -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)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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 }
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,5 +30,6 @@ export default ({ loadTestFile }: FtrProviderContext): void => {
loadTestFile(require.resolve('./find_rules'));
loadTestFile(require.resolve('./find_rule_exception_references'));
loadTestFile(require.resolve('./get_rule_management_filters'));
loadTestFile(require.resolve('./query_alerts'));
});
};
Original file line number Diff line number Diff line change
@@ -0,0 +1,154 @@
/*
* 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 '../../common/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: '[email protected]',
roles: [roleToAccessSecuritySolution.name],
};
const userAllSecWithDls = {
username: 'user_all_sec_with_dls',
password: 'user_all_sec_with_dls',
full_name: 'userAllSecWithDls',
email: '[email protected]',
roles: [roleToAccessSecuritySolutionWithDls.name],
};

// eslint-disable-next-line import/no-default-export
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);
});
});
};
Loading

0 comments on commit 4273961

Please sign in to comment.