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

Merged
merged 39 commits into from
Aug 5, 2021
Merged
Show file tree
Hide file tree
Changes from all 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.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
6 changes: 6 additions & 0 deletions packages/kbn-rule-data-utils/src/technical_field_names.ts
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ const ALERT_EVALUATION_THRESHOLD = `${ALERT_NAMESPACE}.evaluation.threshold` as
const ALERT_EVALUATION_VALUE = `${ALERT_NAMESPACE}.evaluation.value` as const;
const ALERT_ID = `${ALERT_NAMESPACE}.id` as const;
const ALERT_OWNER = `${ALERT_NAMESPACE}.owner` as const;
Copy link
Contributor

Choose a reason for hiding this comment

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

Is my understanding correct that kibana.alert.owner is going to get removed in favour of kibana.consumers?

const ALERT_CONSUMERS = `${ALERT_NAMESPACE}.consumers` as const;
Copy link
Contributor

Choose a reason for hiding this comment

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

Are these going to be shared? There are a bunch of fields in security_solution, not in here and I'm just trying to figure out whether they should be added here or just kept within security_solution i.e. all the original_event fields here:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah the fields here are expected to be used by all solutions that use alerts as data. The original_event fields are security-specific because other solutions are not attempting to preserve the whole source documents (so they don't need to preserve the original event.* fields when they populate fields like event.kind: signal)

Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't it be kibana.consumers? Here it's kibana.alert.consumers.

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'm not sure. For now I just made it similar to the existing ALERT_PRODUCER. I think as long as we're using the constants from this package, follow up naming discussions can be had to change the field names as needed.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm just looking at the spec and I can't find kibana.alert.consumers there, but:

I think it's kibana.consumers because the concept of shareability between features might be more broad than the alerting domain.

Not something that this PR should be responsible for, so I think we can merge it as is, but I'd double-check this naming at the next alerts-as-data meeting.

const ALERT_PRODUCER = `${ALERT_NAMESPACE}.producer` as const;
const ALERT_REASON = `${ALERT_NAMESPACE}.reason` as const;
const ALERT_RISK_SCORE = `${ALERT_NAMESPACE}.risk_score` as const;
Expand Down Expand Up @@ -70,6 +71,7 @@ const ALERT_RULE_SEVERITY_MAPPING = `${ALERT_RULE_NAMESPACE}.severity_mapping` a
const ALERT_RULE_TAGS = `${ALERT_RULE_NAMESPACE}.tags` as const;
const ALERT_RULE_TO = `${ALERT_RULE_NAMESPACE}.to` as const;
const ALERT_RULE_TYPE = `${ALERT_RULE_NAMESPACE}.type` as const;
const ALERT_RULE_TYPE_ID = `${ALERT_RULE_NAMESPACE}.rule_type_id` as const;
const ALERT_RULE_UPDATED_AT = `${ALERT_RULE_NAMESPACE}.updated_at` as const;
const ALERT_RULE_UPDATED_BY = `${ALERT_RULE_NAMESPACE}.updated_by` as const;
const ALERT_RULE_VERSION = `${ALERT_RULE_NAMESPACE}.version` as const;
Expand Down Expand Up @@ -99,6 +101,7 @@ const fields = {
ALERT_EVALUATION_VALUE,
ALERT_ID,
ALERT_OWNER,
ALERT_CONSUMERS,
ALERT_PRODUCER,
ALERT_REASON,
ALERT_RISK_SCORE,
Expand All @@ -124,6 +127,7 @@ const fields = {
ALERT_RULE_TAGS,
ALERT_RULE_TO,
ALERT_RULE_TYPE,
ALERT_RULE_TYPE_ID,
ALERT_RULE_UPDATED_AT,
ALERT_RULE_UPDATED_BY,
ALERT_RULE_VERSION,
Expand Down Expand Up @@ -151,6 +155,7 @@ export {
ALERT_NAMESPACE,
ALERT_RULE_NAMESPACE,
ALERT_OWNER,
ALERT_CONSUMERS,
ALERT_PRODUCER,
ALERT_REASON,
ALERT_RISK_SCORE,
Expand Down Expand Up @@ -179,6 +184,7 @@ export {
ALERT_RULE_TAGS,
ALERT_RULE_TO,
ALERT_RULE_TYPE,
ALERT_RULE_TYPE_ID,
ALERT_RULE_UPDATED_AT,
ALERT_RULE_UPDATED_BY,
ALERT_RULE_VERSION,
Expand Down
2 changes: 1 addition & 1 deletion x-pack/plugins/rule_registry/server/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,11 +11,11 @@ import { RuleRegistryPlugin } from './plugin';
export * from './config';
export type { RuleRegistryPluginSetupContract, RuleRegistryPluginStartContract } from './plugin';
export type { RacRequestHandlerContext, RacApiRequestHandlerContext } from './types';
export { RuleDataPluginService } from './rule_data_plugin_service';
export { RuleDataClient } from './rule_data_client';
export { IRuleDataClient } from './rule_data_client/types';
export { getRuleData, RuleExecutorData } from './utils/get_rule_executor_data';
export { createLifecycleRuleTypeFactory } from './utils/create_lifecycle_rule_type_factory';
export { RuleDataPluginService } from './rule_data_plugin_service';
export {
LifecycleRuleExecutor,
LifecycleAlertService,
Expand Down
51 changes: 44 additions & 7 deletions x-pack/plugins/rule_registry/server/rule_data_client/index.ts
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

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}`
);
}
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ describe('Alert details with unmapped fields', () => {

it('Displays the unmapped field on the table', () => {
const expectedUnmmappedField = {
row: 55,
row: 88,
Copy link
Contributor

Choose a reason for hiding this comment

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

?

field: 'unmapped',
text: 'This is the unmapped field',
};
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ describe('Alert details with unmapped fields', () => {

it('Displays the unmapped field on the table', () => {
const expectedUnmmappedField = {
row: 55,
row: 88,
field: 'unmapped',
text: 'This is the unmapped field',
};
Expand Down
7 changes: 5 additions & 2 deletions x-pack/plugins/security_solution/server/client/client.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,12 +9,15 @@ import { ConfigType } from '../config';

export class AppClient {
private readonly signalsIndex: string;
private readonly spaceId: string;

constructor(private spaceId: string, private config: ConfigType) {
constructor(_spaceId: string, private config: ConfigType) {
const configuredSignalsIndex = this.config.signalsIndex;

this.signalsIndex = `${configuredSignalsIndex}-${this.spaceId}`;
this.signalsIndex = `${configuredSignalsIndex}-${_spaceId}`;
this.spaceId = _spaceId;
}

public getSignalsIndex = (): string => this.signalsIndex;
public getSpaceId = (): string => this.spaceId;
}