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.

Sign up for GitHub

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] Siem signals -> alerts as data field and index aliases #106049

Merged
merged 39 commits into from
Aug 5, 2021
Merged

[Security Solution] Siem signals -> alerts as data field and index aliases #106049

Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
39 commits
Select commit Hold shift + click to select a range
010f093
Add aliases mapping signal fields to alerts as data fields
marshallmain Jul 16, 2021
c4edc7d
Add aliases mapping alerts as data fields to signal fields
marshallmain Jul 16, 2021
f943313
Replace siem signals templates per space and add AAD index aliases to…
marshallmain Jul 17, 2021
5f11d31
Remove first version of new mapping json file
marshallmain Jul 19, 2021
979ad9a
Merge branch 'master' into signal-aad-aliases
marshallmain Jul 19, 2021
3148569
Convert existing legacy siem-signals templates to new ES templates
marshallmain Jul 26, 2021
3e08e73
Catch 404 if siem signals templates were already updated
marshallmain Jul 26, 2021
58f7464
Enhance error message when index exists but is not write index for alias
marshallmain Jul 26, 2021
1788952
Merge branch 'master' into signal-aad-aliases
marshallmain Jul 26, 2021
9377e0c
Check if alias write index exists before creating new write index
marshallmain Jul 26, 2021
df9b010
More robust write target creation logic
marshallmain Jul 26, 2021
bad2321
Add RBAC required fields for AAD to siem signals indices
marshallmain Jul 27, 2021
b837f27
Fix index name in index mapping update
marshallmain Jul 28, 2021
94f9dee
Merge branch 'master' into signal-aad-aliases
marshallmain Jul 28, 2021
28722b1
Throw errors if bulk retry fails or existing indices are not writeable
marshallmain Jul 28, 2021
185f4ba
Add new template to routes even without experimental rule registry fl…
marshallmain Jul 30, 2021
bf89a05
Merge branch 'master' into signal-aad-aliases
marshallmain Jul 30, 2021
30ac118
Check template version before updating template
marshallmain Jul 30, 2021
3c6f7b7
First pass at modifying routes to handle inserting field aliases
marshallmain Jul 30, 2021
3c26be4
Always insert field aliases when create_index_route is called
marshallmain Jul 30, 2021
f1f5ddc
Update snapshot test
marshallmain Jul 30, 2021
58d0e01
Remove template update logic from plugin setup
marshallmain Jul 30, 2021
e8f464c
Use aliases_version field to detect if aliases need update
marshallmain Aug 2, 2021
3054481
Fix bugs
marshallmain Aug 2, 2021
d7bc0da
Merge branch 'master' into signal-aad-aliases
kibanamachine Aug 2, 2021
0910131
oops update snapshot
marshallmain Aug 2, 2021
3827188
Use internal user for PUT alias to fix perms issue
marshallmain Aug 2, 2021
0cd9b83
Update comment
marshallmain Aug 2, 2021
5973dde
Disable new resource creation if ruleRegistryEnabled
marshallmain Aug 3, 2021
ded440e
Only attempt to add aliases if siem-signals index already exists
marshallmain Aug 3, 2021
99d83ee
Merge branch 'master' into signal-aad-aliases
marshallmain Aug 3, 2021
8e7e00e
Fix types, add aliases to aad indices, use package field names
marshallmain Aug 3, 2021
c033517
Undo adding aliases to AAD indices
marshallmain Aug 3, 2021
fc475fd
Remove unused import
marshallmain Aug 3, 2021
5828090
Update test and snapshot oops
marshallmain Aug 3, 2021
4275da7
Filter out kibana.* fields from generated signals
marshallmain Aug 4, 2021
0214b61
Update cypress test to account for new fields in table
marshallmain Aug 4, 2021
498f9c4
Properly handle space ids with dashes in them
marshallmain Aug 4, 2021
6bcb6ae
Merge branch 'master' into signal-aad-aliases
kibanamachine Aug 5, 2021
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
25 changes: 19 additions & 6 deletions x-pack/plugins/rule_registry/server/rule_data_client/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,7 +96,8 @@ export class RuleDataClient implements IRuleDataClient {
if (response.body.errors) {
if (
response.body.items.length > 0 &&
response.body.items?.[0]?.index?.error?.type === 'index_not_found_exception'
(response.body.items?.[0]?.index?.error?.type === 'index_not_found_exception' ||
response.body.items?.[0]?.index?.error?.type === 'illegal_argument_exception')
Copy link
Contributor Author

Choose a reason for hiding this comment

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

illegal_argument_exception is returned if the alias exists but has no write_index.

Copy link
Member

Choose a reason for hiding this comment

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

Unfortunately illegal_argument_exception can be returned for many reasons other than the lack of a write index. I can imagine two ways to reduce the risk of misinterpretation:

  1. Parse the error message (has other risks).
  2. Query the alias for is_write_alias.

Do you think it's safe enough to just assume the reason?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a good point, I'll add logic similar to the old "alias exists" check in createWriteTargetIfNeeded but instead checking to ensure the alias has no write target before attempting to create a write target for it. You're right that otherwise we could end up with illegal_argument_exceptions that cause us to try creating a new write target and that would cause problems if the alias already has a write target.

Copy link
Contributor Author

@marshallmain marshallmain Jul 27, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I replaced the aliasExists check with a more general "index exists" check on the AAD indices. This makes the createWriteTargetIfNeeded function safer to call optimistically when we encounter index_not_found or illegal_argument exceptions while still allowing the security solution to add AAD aliases to our .siem-signals indices. Now, if we do get an illegal_argument_exception for a reason other than "no concrete AAD indices exist", then createWriteTargetIfNeeded will return without making new indices and potentially making things worse.

) {
return this.createWriteTargetIfNeeded({ namespace }).then(() => {
return clusterClient.bulk(requestWithDefaultParameters);
Expand All @@ -116,13 +117,14 @@ export class RuleDataClient implements IRuleDataClient {

const clusterClient = await this.getClusterClient();

const { body: aliasExists } = await clusterClient.indices.existsAlias({
name: alias,
const { body: indicesExist } = await clusterClient.indices.exists({
index: `${alias}-*`,
allow_no_indices: false,
Comment on lines +129 to +131
Copy link
Contributor

Choose a reason for hiding this comment

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

Confused a bit, why do we need to check this? If createWriteTargetIfNeeded is called when we get index_not_found_exception, shouldn't it be enough to go and try to create the concrete index right away?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In some scenarios we could get an illegal_argument_exception for a reason other than the "alias exists but has no write index". In that case, we'd call createWriteTargetIfNeeded but if an index exists for the alias already then we don't want to try creating a new index. Essentially, the check here is to make it safe to call createWriteTargetIfNeeded optimistically and know that it won't create unnecessary new resources or make things worse when attempting to rectify errors.

});

const concreteIndexName = `${alias}-000001`;

if (!aliasExists) {
Copy link
Contributor Author

@marshallmain marshallmain Jul 20, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I removed this check so that we can add the .alerts-security.alerts alias to the .siem-signals indices before the .alerts-security.alerts concrete indices are actually created. Otherwise, we would have to wait until the first new alert was written to .alerts before we could add the alias to .siem-signals. This way we can always treat all existing alerts as though they are in .alerts and don't have to worry about handling old and new alerts separately.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This check has now been replaced with a more specific check - querying to find concrete indices rather than the alias.

if (!indicesExist) {
try {
await clusterClient.indices.create({
index: concreteIndexName,
Expand All @@ -135,8 +137,19 @@ export class RuleDataClient implements IRuleDataClient {
},
});
} catch (err) {
// something might have created the index already, that sounds OK
if (err?.meta?.body?.error?.type !== 'resource_already_exists_exception') {
// If the index already exists and it's the write index for the alias,
Copy link
Member

Choose a reason for hiding this comment

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

If the index already exists, will we ever get here? indicesExist will have been true in line 127. The error is silent in that case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's still possible to get here if multiple code paths attempt to create the index at the same time. It can be triggered in testing if the index doesn't exist yet with a construct like

ruleDataClient.getWriter().bulk(request);
ruleDataClient.getWriter().bulk(request);

in this case both calls race to attempt to make the index and often one hits a resource_already_exists_exception. It's also theoretically possible that some other source (e.g. user, backup script) could make the index in between the indices exist check and the index creation, so by fetching the index after getting the exception we can check that it was created correctly.

Copy link
Member

Choose a reason for hiding this comment

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

True. But if the lack of a write index was the cause for this function being called we'd just fail silently because indicesExist === true. Would raising an exception in an else branch for the top level check do the trick?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah I see. I was counting on the bulk retry to fail again in that case and throw the error, but I didn't notice that it wasn't checking for errors on the retry. I added handling for both in 28722b1 - the retry bulk call now throws errors if it encounters any, and if createWriteTargetIfNeeded finds existing indices then it checks to make sure one of them is the write index and throws an error otherwise. I think the bulk retry error handling alone would be sufficient to avoid silently failing, but checking the alias specifically inside createWriteTargetIfNeeded allows us to have a more specific error message for that case so I added both.

// something else created it so suppress the error. If it's not the write
// index, that's bad, throw an error.
if (err?.meta?.body?.error?.type === 'resource_already_exists_exception') {
const { body: existingIndices } = await clusterClient.indices.get({
index: concreteIndexName,
});
if (!existingIndices[concreteIndexName]?.aliases?.[alias]?.is_write_index) {
throw Error(
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: throw new (although seems like there's no difference for the built-in Error constructor, it may or may not work with custom exceptions)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

as of ES5 they're supposed to be equivalent, right? https://es5.github.io/#x15.11.1

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, but for a custom exception this might not work.

Example:

class CustomError extends Error {
  constructor(value) {
    super('Hello this is my message');
    this.value = value;
  }
}

let e = CustomError(42);
console.log(e.value); // ?

Actually, I just checked it in Chrome, and calling CustomError(42) without new just throws TypeError: Class constructor CustomError cannot be invoked without 'new', seems like V8 has a special handling for class constructors.

However, this won't work the same way for any constructor defined as a function. In general, something like that would be needed:

function User(name) {
  if (!new.target) { // if you run me without new
    return new User(name); // ...I will add new for you
  }

  this.name = name;
}

let john = User("John"); // redirects call to new User
alert(john.name); // John

See https://javascript.info/constructor-new#constructor-mode-test-new-target

Maybe I was overthinking when trying to explain it, but for me it's quite simple - let's use new everywhere and in all cases when calling constructors for the sake of safety and consistency. It's like ===.

Sorry, I'm 🚲 🏠 🎨 'ing

`Attempted to create index: ${concreteIndexName} as the write index for alias: ${alias}, but the index already exists and is not the write index for the alias`
);
}
} else {
throw err;
}
}
Expand Down
8 changes: 7 additions & 1 deletion .../plugins/security_solution/server/lib/detection_engine/routes/index/create_index_route.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,8 +26,10 @@ import signalsPolicy from './signals_policy.json';
import { templateNeedsUpdate } from './check_template_version';
import { getIndexVersion } from './get_index_version';
import { isOutdated } from '../../migrations/helpers';
import { parseExperimentalConfigValue } from '../../../../../common/experimental_features';
import { ConfigType } from '../../../../config';

export const createIndexRoute = (router: SecuritySolutionPluginRouter) => {
export const createIndexRoute = (router: SecuritySolutionPluginRouter, config: ConfigType) => {
router.post(
{
path: DETECTION_ENGINE_INDEX_URL,
Expand All @@ -37,6 +39,10 @@ export const createIndexRoute = (router: SecuritySolutionPluginRouter) => {
},
},
async (context, request, response) => {
const { ruleRegistryEnabled } = parseExperimentalConfigValue(config.enableExperimental);
if (ruleRegistryEnabled) {
return response.ok({ body: { acknowledged: true } });
}
const siemResponse = buildSiemResponse(response);

try {
Expand Down
47 changes: 47 additions & 0 deletions ...lugins/security_solution/server/lib/detection_engine/routes/index/get_signals_template.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,3 +69,50 @@ export const getSignalsTemplate = (index: string) => {
};
return template;
};

export const getNewSignalsTemplate = (index: string) => {
const template = {
index_patterns: [`${index}-*`],
template: {
settings: {
index: {
lifecycle: {
name: index,
rollover_alias: index,
},
},
mapping: {
total_fields: {
limit: 10000,
},
},
},
mappings: {
dynamic: false,
properties: {
...ecsMapping.mappings.properties,
...otherMapping.mappings.properties,
signal: signalsMapping.mappings.properties.signal,
threat: {
...ecsMapping.mappings.properties.threat,
properties: {
...ecsMapping.mappings.properties.threat.properties,
indicator: {
...otherMapping.mappings.properties.threat.properties.indicator,
properties: {
...otherMapping.mappings.properties.threat.properties.indicator.properties,
event: ecsMapping.mappings.properties.event,
},
},
},
},
},
_meta: {
version: SIGNALS_TEMPLATE_VERSION,
},
},
},
version: SIGNALS_TEMPLATE_VERSION,
};
return template;
};
93 changes: 93 additions & 0 deletions ...lugins/security_solution/server/lib/detection_engine/routes/index/signal_aad_mapping.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,93 @@
{
"signal.ancestors.depth": "kibana.alert.ancestors.depth",
"signal.ancestors.id": "kibana.alert.ancestors.id",
"signal.ancestors.index": "kibana.alert.ancestors.index",
"signal.ancestors.type": "kibana.alert.ancestors.type",
"signal.depth": "kibana.alert.depth",
"signal.original_event.action": "kibana.alert.original_event.action",
"signal.original_event.category": "kibana.alert.original_event.category",
"signal.original_event.code": "kibana.alert.original_event.code",
"signal.original_event.created": "kibana.alert.original_event.created",
"signal.original_event.dataset": "kibana.alert.original_event.dataset",
"signal.original_event.duration": "kibana.alert.original_event.duration",
"signal.original_event.end": "kibana.alert.original_event.end",
"signal.original_event.hash": "kibana.alert.original_event.hash",
"signal.original_event.id": "kibana.alert.original_event.id",
"signal.original_event.kind": "kibana.alert.original_event.kind",
"signal.original_event.module": "kibana.alert.original_event.module",
"signal.original_event.outcome": "kibana.alert.original_event.outcome",
"signal.original_event.provider": "kibana.alert.original_event.provider",
"signal.original_event.risk_score": "kibana.alert.original_event.risk_score",
"signal.original_event.risk_score_norm": "kibana.alert.original_event.risk_score_norm",
"signal.original_event.sequence": "kibana.alert.original_event.sequence",
"signal.original_event.severity": "kibana.alert.original_event.severity",
"signal.original_event.start": "kibana.alert.original_event.start",
"signal.original_event.timezone": "kibana.alert.original_event.timezone",
"signal.original_event.type": "kibana.alert.original_event.type",
"signal.original_time": "kibana.alert.original_time",
"signal.rule.author": "kibana.alert.rule.author",
"signal.rule.building_block_type": "kibana.alert.rule.building_block_type",
"signal.rule.created_at": "kibana.alert.rule.created_at",
"signal.rule.created_by": "kibana.alert.rule.created_by",
"signal.rule.description": "kibana.alert.rule.description",
"signal.rule.enabled": "kibana.alert.rule.enabled",
"signal.rule.false_positives": "kibana.alert.rule.false_positives",
"signal.rule.from": "kibana.alert.rule.from",
"signal.rule.id": "kibana.alert.rule.id",
"signal.rule.immutable": "kibana.alert.rule.immutable",
"signal.rule.index": "kibana.alert.rule.index",
"signal.rule.interval": "kibana.alert.rule.interval",
"signal.rule.language": "kibana.alert.rule.language",
"signal.rule.license": "kibana.alert.rule.license",
"signal.rule.max_signals": "kibana.alert.rule.max_signals",
"signal.rule.name": "kibana.alert.rule.name",
"signal.rule.note": "kibana.alert.rule.note",
"signal.rule.query": "kibana.alert.rule.query",
"signal.rule.references": "kibana.alert.rule.references",
"signal.rule.risk_score": "kibana.alert.risk_score",
"signal.rule.risk_score_mapping.field": "kibana.alert.rule.risk_score_mapping.field",
"signal.rule.risk_score_mapping.operator": "kibana.alert.rule.risk_score_mapping.operator",
"signal.rule.risk_score_mapping.value": "kibana.alert.rule.risk_score_mapping.value",
"signal.rule.rule_id": "kibana.alert.rule.rule_id",
"signal.rule.rule_name_override": "kibana.alert.rule.rule_name_override",
"signal.rule.saved_id": "kibana.alert.rule.saved_id",
"signal.rule.severity": "kibana.alert.severity",
"signal.rule.severity_mapping.field": "kibana.alert.rule.severity_mapping.field",
"signal.rule.severity_mapping.operator": "kibana.alert.rule.severity_mapping.operator",
"signal.rule.severity_mapping.value": "kibana.alert.rule.severity_mapping.value",
"signal.rule.severity_mapping.severity": "kibana.alert.rule.severity_mapping.severity",
"signal.rule.tags": "kibana.alert.rule.tags",
"signal.rule.threat.framework": "kibana.alert.rule.threat.framework",
"signal.rule.threat.tactic.id": "kibana.alert.rule.threat.tactic.id",
"signal.rule.threat.tactic.name": "kibana.alert.rule.threat.tactic.name",
"signal.rule.threat.tactic.reference": "kibana.alert.rule.threat.tactic.reference",
"signal.rule.threat.technique.id": "kibana.alert.rule.threat.technique.id",
"signal.rule.threat.technique.name": "kibana.alert.rule.threat.technique.name",
"signal.rule.threat.technique.reference": "kibana.alert.rule.threat.technique.reference",
"signal.rule.threat.technique.subtechnique.id": "kibana.alert.rule.threat.technique.subtechnique.id",
"signal.rule.threat.technique.subtechnique.name": "kibana.alert.rule.threat.technique.subtechnique.name",
"signal.rule.threat.technique.subtechnique.reference": "kibana.alert.rule.threat.technique.subtechnique.reference",
"signal.rule.threat_index": "kibana.alert.rule.threat_index",
"signal.rule.threat_indicator_path": "kibana.alert.rule.threat_indicator_path",
"signal.rule.threat_language": "kibana.alert.rule.threat_language",
"signal.rule.threat_mapping.entries.field": "kibana.alert.rule.threat_mapping.entries.field",
"signal.rule.threat_mapping.entries.value": "kibana.alert.rule.threat_mapping.entries.value",
"signal.rule.threat_mapping.entries.type": "kibana.alert.rule.threat_mapping.entries.type",
"signal.rule.threat_query": "kibana.alert.rule.threat_query",
"signal.rule.threshold.field": "kibana.alert.rule.threshold.field",
"signal.rule.threshold.value": "kibana.alert.rule.threshold.value",
"signal.rule.timeline_id": "kibana.alert.rule.timeline_id",
"signal.rule.timeline_title": "kibana.alert.rule.timeline_title",
"signal.rule.to": "kibana.alert.rule.to",
"signal.rule.type": "kibana.alert.rule.type",
"signal.rule.updated_at": "kibana.alert.rule.updated_at",
"signal.rule.updated_by": "kibana.alert.rule.updated_by",
"signal.rule.version": "kibana.alert.rule.version",
"signal.status": "kibana.alert.workflow_status",
"signal.threshold_result.from": "kibana.alert.threshold_result.from",
"signal.threshold_result.terms.field": "kibana.alert.threshold_result.terms.field",
"signal.threshold_result.terms.value": "kibana.alert.threshold_result.terms.value",
"signal.threshold_result.cardinality.field": "kibana.alert.threshold_result.cardinality.field",
"signal.threshold_result.cardinality.value": "kibana.alert.threshold_result.cardinality.value",
"signal.threshold_result.count": "kibana.alert.threshold_result.count"
}
Loading