Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Security Solution] [Detections] Add "read index" privilege check on rule execution #83134

Merged
merged 4 commits into from
Dec 22, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ import {
getListsClient,
getExceptions,
sortExceptionItems,
checkPrivileges,
} from './utils';
import { parseScheduleDates } from '../../../../common/detection_engine/parse_schedule_dates';
import { RuleExecutorOptions, SearchAfterAndBulkCreateReturnType } from './types';
Expand All @@ -42,6 +43,7 @@ jest.mock('./utils', () => {
getListsClient: jest.fn(),
getExceptions: jest.fn(),
sortExceptionItems: jest.fn(),
checkPrivileges: jest.fn(),
};
});
jest.mock('../notifications/schedule_notification_actions');
Expand Down Expand Up @@ -105,6 +107,7 @@ describe('rules_notification_alert_type', () => {
find: jest.fn(),
goingToRun: jest.fn(),
error: jest.fn(),
partialFailure: jest.fn(),
};
(ruleStatusServiceFactory as jest.Mock).mockReturnValue(ruleStatusService);
(getGapBetweenRuns as jest.Mock).mockReturnValue(moment.duration(0));
Expand All @@ -124,6 +127,21 @@ describe('rules_notification_alert_type', () => {
searchAfterTimes: [],
createdSignalsCount: 10,
});
(checkPrivileges as jest.Mock).mockImplementation((_, indices) => {
return {
index: indices.reduce(
(acc: { index: { [x: string]: { read: boolean } } }, index: string) => {
return {
[index]: {
read: true,
},
...acc,
};
},
{}
),
};
});
alertServices.callCluster.mockResolvedValue({
hits: {
total: { value: 10 },
Expand Down Expand Up @@ -170,6 +188,52 @@ describe('rules_notification_alert_type', () => {
});
});

it('should set a partial failure for when rules cannot read ALL provided indices', async () => {
(checkPrivileges as jest.Mock).mockResolvedValueOnce({
username: 'elastic',
has_all_requested: false,
cluster: {},
index: {
'myfa*': {
read: true,
},
'some*': {
read: false,
},
},
application: {},
});
payload.params.index = ['some*', 'myfa*'];
await alert.executor(payload);
expect(ruleStatusService.partialFailure).toHaveBeenCalled();
expect(ruleStatusService.partialFailure.mock.calls[0][0]).toContain(
'Missing required read permissions on indexes: ["some*"]'
);
});

it('should set a failure status for when rules cannot read ANY provided indices', async () => {
(checkPrivileges as jest.Mock).mockResolvedValueOnce({
username: 'elastic',
has_all_requested: false,
cluster: {},
index: {
'myfa*': {
read: false,
},
'some*': {
read: false,
},
},
application: {},
});
payload.params.index = ['some*', 'myfa*'];
await alert.executor(payload);
expect(ruleStatusService.error).toHaveBeenCalled();
expect(ruleStatusService.error.mock.calls[0][0]).toContain(
'The rule does not have read privileges to any of the following indices: ["myfa*","some*"]'
);
});

it('should NOT warn about the gap between runs if gap small', async () => {
(getGapBetweenRuns as jest.Mock).mockReturnValue(moment.duration(1, 'm'));
(getGapMaxCatchupRatio as jest.Mock).mockReturnValue({
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
/* eslint-disable complexity */

import { Logger, KibanaRequest } from 'src/core/server';
import { partition } from 'lodash';

import {
SIGNALS_ID,
Expand Down Expand Up @@ -41,6 +42,7 @@ import {
createSearchAfterReturnType,
mergeReturns,
createSearchAfterReturnTypeFromResponse,
checkPrivileges,
} from './utils';
import { signalParamsSchema } from './signal_params_schema';
import { siemRuleActionGroups } from './siem_rule_action_groups';
Expand Down Expand Up @@ -171,8 +173,51 @@ export const signalRulesAlertType = ({

logger.debug(buildRuleMessage('[+] Starting Signal Rule execution'));
logger.debug(buildRuleMessage(`interval: ${interval}`));
let wroteStatus = false;
await ruleStatusService.goingToRun();

// check if rule has permissions to access given index pattern
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this a TO DO? Would definitely be great to move out for testing too.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had written a function in the old multiple timestamps PR (which was closed) that would have been the home for this privilege check as well. Maybe in 7.12 I can bring that function back. I think once we add more types of pre-execution checks we can merge them into a single function to be executed at the top of the rule executor like this privilege check is now.

// move this collection of lines into a function in utils
// so that we can use it in create rules route, bulk, etc.
try {
const inputIndex = await getInputIndex(services, version, index);
const privileges = await checkPrivileges(services, inputIndex);

const indexNames = Object.keys(privileges.index);
const [indexesWithReadPrivileges, indexesWithNoReadPrivileges] = partition(
indexNames,
(indexName) => privileges.index[indexName].read
);

if (
indexesWithReadPrivileges.length > 0 &&
indexesWithNoReadPrivileges.length >= indexesWithReadPrivileges.length
) {
// some indices have read privileges others do not.
// set a partial failure status
const errorString = `Missing required read permissions on indexes: ${JSON.stringify(
indexesWithNoReadPrivileges
)}`;
logger.debug(buildRuleMessage(errorString));
await ruleStatusService.partialFailure(errorString);
wroteStatus = true;
} else if (
indexesWithReadPrivileges.length === 0 &&
indexesWithNoReadPrivileges.length === indexNames.length
) {
// none of the indices had read privileges so set the status to failed
// since we can't search on any indices we do not have read privileges on
const errorString = `The rule does not have read privileges to any of the following indices: ${JSON.stringify(
indexesWithNoReadPrivileges
)}`;
logger.debug(buildRuleMessage(errorString));
await ruleStatusService.error(errorString);
wroteStatus = true;
}
} catch (exc) {
logger.error(buildRuleMessage(`Check privileges failed to execute ${exc}`));
}

const gap = getGapBetweenRuns({ previousStartedAt, interval, from, to });
if (gap != null && gap.asMilliseconds() > 0) {
const fromUnit = from[from.length - 1];
Expand Down Expand Up @@ -600,7 +645,7 @@ export const signalRulesAlertType = ({
`[+] Finished indexing ${result.createdSignalsCount} signals into ${outputIndex}`
)
);
if (!hasError) {
if (!hasError && !wroteStatus) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We'll only be reporting success if it checked all indices? Wondering if we should start better documenting/defining what "successful" rule run means. Like in this case if we successfully searched the indices we could, I might expect a success with a warning?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, my thinking was if the rule can't search every index pattern provided because it's missing read privileges we would want to say "this partially failed" because we couldn't do everything the rule was written to accomplish https://github.com/elastic/kibana/pull/83134/files/b79dd8abf743a5451c3346f7dd004735b7beb732#diff-bd205efcb13acaf5f9552bd573f21422d37d60c6f2aa03ae7f98d18b95fbc6e4R194. We could re-work the partial failure status to just be a "warning" status maybe? Maybe that would clear things up. What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea, that makes sense too, if we weren't able to fulfill what the rule is asking. No need to change anything, maybe just an interesting convo to have at a team meeting about how to qualify different states.

await ruleStatusService.success('succeeded', {
bulkCreateTimeDurations: result.bulkCreateTimes,
searchAfterTimeDurations: result.searchAfterTimes,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,20 @@ export const shorthandMap = {
},
};

export const checkPrivileges = async (services: AlertServices, indices: string[]) =>
services.callCluster('transport.request', {
path: '/_security/user/_has_privileges',
method: 'POST',
body: {
index: [
{
names: indices ?? [],
privileges: ['read'],
},
],
},
});

export const getGapMaxCatchupRatio = ({
logger,
previousStartedAt,
Expand Down