[Security Solution][Detection Engine] Only check read privileges of extant indices during rule execution (#177658)

## Summary

This PR modifies the privilege-checking behavior during rule execution,
restricting the indices against which we verify `read` access to only
those that exist.

### Outstanding questions

- [x] Are there any backwards-compatibility/semver concerns with
changing this behavior?
* We discussed in which situations a user might reasonably be using the
existing behavior, and determined those to be borderline. If we end up
receiving feedback to the contrary, we can add back the old behavior as
configuration.
- [x] Is the `IndexPatternsFetcher` an appropriate implementation to use
for the existence checking?


### Steps to Review
1. Create a rule with a pattern including a non-existent index, e.g.
`auditbeat-*,does-not-exist`
2. Enable the rule, and observe no warning about e.g. missing read
privileges for `does-not-exist`
3. (optional) Remove read access to `auditbeat-*`, or extend the pattern
to include an existing index that the rule author cannot read
4. (optional) Observe a warning for the non-readable index


### Checklist

Delete any items that are not applicable to this PR.

- [ ]
[Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html)
was added for features that require explanation or tutorials
- [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

### For maintainers

- [ ] This was checked for breaking API changes and was [labeled
appropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)

---------

Co-authored-by: Yara Tercero <yctercero@users.noreply.github.com>
This commit is contained in:
Ryland Herrick 2025-01-21 13:11:56 -06:00 committed by GitHub
parent 3719be0144
commit b955f332ec
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
4 changed files with 134 additions and 13 deletions

View file

@ -9,6 +9,7 @@ import { isEmpty, partition } from 'lodash';
import agent from 'elastic-apm-node';
import type * as estypes from '@elastic/elasticsearch/lib/api/typesWithBodyKey';
import { IndexPatternsFetcher } from '@kbn/data-plugin/server';
import { TIMESTAMP } from '@kbn/rule-data-utils';
import { createPersistenceRuleTypeWrapper } from '@kbn/rule-registry-plugin/server';
import { buildExceptionFilter } from '@kbn/lists-plugin/server/services/exception_lists';
@ -257,16 +258,20 @@ export const createSecurityRuleTypeWrapper: CreateSecurityRuleTypeWrapper =
let skipExecution: boolean = false;
try {
if (!isMachineLearningParams(params)) {
const privileges = await checkPrivilegesFromEsClient(esClient, inputIndex);
const indexPatterns = new IndexPatternsFetcher(scopedClusterClient.asInternalUser);
const existingIndices = await indexPatterns.getExistingIndices(inputIndex);
const readIndexWarningMessage = await hasReadIndexPrivileges({
privileges,
ruleExecutionLogger,
uiSettingsClient,
});
if (existingIndices.length > 0) {
const privileges = await checkPrivilegesFromEsClient(esClient, existingIndices);
const readIndexWarningMessage = await hasReadIndexPrivileges({
privileges,
ruleExecutionLogger,
uiSettingsClient,
});
if (readIndexWarningMessage != null) {
wrapperWarnings.push(readIndexWarningMessage);
if (readIndexWarningMessage != null) {
wrapperWarnings.push(readIndexWarningMessage);
}
}
const timestampFieldCaps = await withSecuritySpan('fieldCaps', () =>

View file

@ -21,6 +21,7 @@ import { QUERY_RULE_TYPE_ID } from '@kbn/securitysolution-rules';
import { hasTimestampFields } from '../utils/utils';
import { RuleExecutionStatusEnum } from '../../../../../common/api/detection_engine';
const actualHasTimestampFields = jest.requireActual('../utils/utils').hasTimestampFields;
jest.mock('../utils/utils', () => ({
...jest.requireActual('../utils/utils'),
getExceptions: () => [],
@ -114,7 +115,8 @@ describe('Custom Query Alerts', () => {
expect(eventsTelemetry.queueTelemetryEvents).not.toHaveBeenCalled();
});
it('sends an alert when events are found', async () => {
it('short-circuits and writes a warning if no indices are found', async () => {
(hasTimestampFields as jest.Mock).mockImplementationOnce(actualHasTimestampFields); // default behavior will produce a 'no indices found' result from this helper
const queryAlertType = securityRuleTypeWrapper(
createQueryAlertType({
eventsTelemetry,
@ -156,6 +158,77 @@ describe('Custom Query Alerts', () => {
await executor({ params });
expect((await ruleDataClient.getWriter()).bulk).not.toHaveBeenCalled();
expect(eventsTelemetry.sendAsync).not.toHaveBeenCalled();
expect(mockedStatusLogger.logStatusChange).toHaveBeenCalledWith(
expect.objectContaining({
newStatus: RuleExecutionStatusEnum['partial failure'],
message:
'This rule is attempting to query data from Elasticsearch indices listed in the "Index patterns" section of the rule definition, however no index matching: ["auditbeat-*","filebeat-*","packetbeat-*","winlogbeat-*"] was found. This warning will continue to appear until a matching index is created or this rule is disabled.',
})
);
});
it('sends an alert when events are found', async () => {
const queryAlertType = securityRuleTypeWrapper(
createQueryAlertType({
eventsTelemetry,
licensing,
scheduleNotificationResponseActionsService: () => null,
experimentalFeatures: allowedExperimentalValues,
logger,
version: '1.0.0',
id: QUERY_RULE_TYPE_ID,
name: 'Custom Query Rule',
})
);
alerting.registerType(queryAlertType);
const params = getQueryRuleParams();
// mock field caps so as not to short-circuit on "no indices found"
services.scopedClusterClient.asInternalUser.fieldCaps.mockResolvedValueOnce({
// @ts-expect-error our fieldCaps mock only seems to use the last value of the overloaded FieldCapsApi
body: {
indices: params.index!,
fields: {
_id: {
_id: {
type: '_id',
metadata_field: true,
searchable: true,
aggregatable: false,
},
},
},
},
});
services.scopedClusterClient.asCurrentUser.search.mockReturnValue(
elasticsearchClientMock.createSuccessTransportRequestPromise({
hits: {
hits: [sampleDocNoSortId()],
sequences: [],
events: [],
total: {
relation: 'eq',
value: 1,
},
},
took: 0,
timed_out: false,
_shards: {
failed: 0,
skipped: 0,
successful: 1,
total: 1,
},
})
);
await executor({ params });
expect((await ruleDataClient.getWriter()).bulk).toHaveBeenCalled();
expect(eventsTelemetry.sendAsync).toHaveBeenCalled();
});

View file

@ -52,15 +52,16 @@ export default ({ getService }: FtrProviderContext) => {
await deleteAllRules(supertest, log);
});
describe('should set status to partial failure when user has no access', () => {
context('when all indices exist but user cannot read host_alias', () => {
const indexTestCases = [
['host_alias'],
['host_alias', 'auditbeat-8.0.0'],
['host_alias*'],
['host_alias*', 'auditbeat-*'],
];
indexTestCases.forEach((index) => {
it(`for KQL rule with index param: ${index}`, async () => {
it(`sets rule status to partial failure for KQL rule with index param: ${index}`, async () => {
const rule = {
...getRuleForAlertTesting(index),
query: 'process.executable: "/usr/bin/sudo"',
@ -90,13 +91,55 @@ export default ({ getService }: FtrProviderContext) => {
await deleteUserAndRole(getService, ROLES.detections_admin);
});
});
});
context('when some specified indices do not exist, but user can read all others', () => {});
describe('when no specified indices exist', () => {
describe('for a query rule', () => {
it('sets rule status to partial failure and does not execute', async () => {
const rule = getRuleForAlertTesting(['non-existent-index']);
await createUserAndRole(getService, ROLES.detections_admin);
const { id } = await createRuleWithAuth(supertestWithoutAuth, rule, {
user: ROLES.detections_admin,
pass: 'changeme',
});
await waitForRulePartialFailure({
supertest,
log,
id,
});
const { body } = await supertest
.get(DETECTION_ENGINE_RULES_URL)
.set('kbn-xsrf', 'true')
.set('elastic-api-version', '2023-10-31')
.query({ id })
.expect(200);
// TODO: https://github.com/elastic/kibana/pull/121644 clean up, make type-safe
const lastExecution = body?.execution_summary?.last_execution;
expect(lastExecution.message).to.contain(
'This rule is attempting to query data from Elasticsearch indices listed in the "Index patterns" section of the rule definition, however no index matching: ["non-existent-index"] was found. This warning will continue to appear until a matching index is created or this rule is disabled.'
);
// no metrics == no work performed, presumably
expect(lastExecution.metrics).to.eql({});
await deleteUserAndRole(getService, ROLES.detections_admin);
});
});
});
context('for threshold rules', () => {
const thresholdIndexTestCases = [
['host_alias', 'auditbeat-8.0.0'],
['host_alias*', 'auditbeat-*'],
];
thresholdIndexTestCases.forEach((index) => {
it(`for threshold rule with index param: ${index}`, async () => {
it(`with index param: ${index}`, async () => {
const rule: ThresholdRuleCreateProps = {
...getThresholdRuleForAlertTesting(index),
threshold: {

View file

@ -162,7 +162,7 @@ export default ({ getService }: FtrProviderContext) => {
const rule = await fetchRule(supertest, { id });
expect(rule?.execution_summary?.last_execution.status).toBe('partial failure');
expect(rule?.execution_summary?.last_execution.message).toBe(
expect(rule?.execution_summary?.last_execution.message).toContain(
'This rule is attempting to query data from Elasticsearch indices listed in the "Index patterns" section of the rule definition, however no index matching: ["does-not-exist-*"] was found. This warning will continue to appear until a matching index is created or this rule is disabled.'
);
});