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

Jump to bottom

[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 15 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
51 changes: 44 additions & 7 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,10 +96,20 @@ 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.every(
(item) => item.index?.error?.type === 'index_not_found_exception'
) ||
response.body.items.every(
(item) => item.index?.error?.type === 'illegal_argument_exception'
))
Comment on lines +99 to +104
Copy link
Contributor

Choose a reason for hiding this comment

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

When can we get illegal_argument_exception and why?

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 can be returned when trying to write to an alias that exists but has no write index

Copy link
Contributor

Choose a reason for hiding this comment

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

👍 thank you

) {
return this.createWriteTargetIfNeeded({ namespace }).then(() => {
return clusterClient.bulk(requestWithDefaultParameters);
return clusterClient.bulk(requestWithDefaultParameters).then((retryResponse) => {
if (retryResponse.body.errors) {
throw new ResponseError(retryResponse);
}
return retryResponse;
});
});
}
const error = new ResponseError(response);
Expand All @@ -116,13 +126,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,11 +146,37 @@ 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;
}
}
} else {
// If we find indices matching the pattern, then we expect one of them to be the write index for the alias.
// Throw an error if none of them are the write index.
const { body: aliasesResponse } = await clusterClient.indices.getAlias({
index: `${alias}-*`,
});
if (
!Object.entries(aliasesResponse).some(
([_, aliasesObject]) => aliasesObject.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

`Indices matching pattern ${alias}-* exist but none are set as the write index for alias ${alias}`
);
}
}
}
}
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
95 changes: 95 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 @@ -8,6 +8,7 @@
import signalsMapping from './signals_mapping.json';
import ecsMapping from './ecs_mapping.json';
import otherMapping from './other_mappings.json';
import aadFieldConversion from './signal_aad_mapping.json';

/**
@constant
Expand Down Expand Up @@ -69,3 +70,97 @@ export const getSignalsTemplate = (index: string) => {
};
return template;
};

export const createSignalsFieldAliases = () => {
const fieldAliases: Record<string, unknown> = {};
Object.entries(aadFieldConversion).forEach(([key, value]) => {
fieldAliases[value] = {
type: 'alias',
path: key,
};
});
return fieldAliases;
};

export const getRbacRequiredFields = (spaceId: string) => {
return {
'kibana.space_ids': {
type: 'constant_keyword',
value: spaceId,
},
'kibana.consumers': {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we use the const in the package to ensure we're all using the right field names?

Copy link
Contributor Author

@marshallmain marshallmain Aug 2, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

👍 I updated the space IDs field to use the const names from the package, but the other fields are changing in #105096 so to avoid conflicts I just put the strings in as placeholders (with the expected string value based on the spec). I can follow up and update these to use the const field names.

type: 'constant_keyword',
value: 'siem',
},
'kibana.producer': {
type: 'constant_keyword',
value: 'siem',
},
// TODO: discuss naming of this field and what the value will be for legacy signals.
// Can we leave it as 'siem.signals' or do we need a runtime field that will map signal.rule.type
// to the new ruleTypeId?
'kibana.alert.rule.rule_type_id': {
type: 'constant_keyword',
value: 'siem.signals',
},
};
};

export const getNewSignalsTemplate = (
index: string,
spaceId: string,
aadIndexAliasName: string
) => {
const fieldAliases = createSignalsFieldAliases();
const template = {
index_patterns: [`${index}-*`],
template: {
aliases: {
[aadIndexAliasName]: {
is_write_index: false,
},
},
settings: {
index: {
lifecycle: {
name: index,
rollover_alias: index,
},
},
mapping: {
total_fields: {
limit: 10000,
},
},
},
mappings: {
dynamic: false,
properties: {
...ecsMapping.mappings.properties,
...otherMapping.mappings.properties,
...fieldAliases,
...getRbacRequiredFields(spaceId),
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