-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
Changes from all commits
1eb95af
6539b60
b79dd8a
b77f56a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -7,6 +7,7 @@ | |
/* eslint-disable complexity */ | ||
|
||
import { Logger, KibanaRequest } from 'src/core/server'; | ||
import { partition } from 'lodash'; | ||
|
||
import { | ||
SIGNALS_ID, | ||
|
@@ -41,6 +42,7 @@ import { | |
createSearchAfterReturnType, | ||
mergeReturns, | ||
createSearchAfterReturnTypeFromResponse, | ||
checkPrivileges, | ||
} from './utils'; | ||
import { signalParamsSchema } from './signal_params_schema'; | ||
import { siemRuleActionGroups } from './siem_rule_action_groups'; | ||
|
@@ -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 | ||
// 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]; | ||
|
@@ -600,7 +645,7 @@ export const signalRulesAlertType = ({ | |
`[+] Finished indexing ${result.createdSignalsCount} signals into ${outputIndex}` | ||
) | ||
); | ||
if (!hasError) { | ||
if (!hasError && !wroteStatus) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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, | ||
|
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.