Skip to content

Commit

Permalink
[SECURITY_SOLUTIONS] Only query security alerts with current user (el…
Browse files Browse the repository at this point in the history
…astic#174216)

## Summary

We just got an
[SDH#814](elastic/sdh-security-team#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
elastic#112113 since 8.0.0

I think this simple solution should not impact any features.
  • Loading branch information
XavierM authored Jan 4, 2024
1 parent 1330168 commit 4af36fe
Show file tree
Hide file tree
Showing 4 changed files with 176 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 @@ -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'));
});
}
Original file line number Diff line number Diff line change
@@ -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: '[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],
};

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);
});
});
};

0 comments on commit 4af36fe

Please sign in to comment.