-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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
Changes from all commits
010f093
c4edc7d
f943313
5f11d31
979ad9a
3148569
3e08e73
58f7464
1788952
9377e0c
df9b010
bad2321
b837f27
94f9dee
28722b1
185f4ba
bf89a05
30ac118
3c6f7b7
3c26be4
f1f5ddc
58d0e01
e8f464c
3054481
d7bc0da
0910131
3827188
0cd9b83
5973dde
ded440e
99d83ee
8e7e00e
c033517
fc475fd
5828090
4275da7
0214b61
498f9c4
6bcb6ae
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 | ||
---|---|---|---|---|
|
@@ -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; | ||||
const ALERT_CONSUMERS = `${ALERT_NAMESPACE}.consumers` as const; | ||||
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. Are these going to be shared? There are a bunch of fields in kibana/x-pack/plugins/security_solution/public/common/components/drag_and_drop/helpers.ts Line 113 in b5e8db2
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. Yeah the fields here are expected to be used by all solutions that use alerts as data. The 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. Shouldn't it be 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. I'm not sure. For now I just made it similar to the existing 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. I'm just looking at the spec and I can't find I think it's 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; | ||||
|
@@ -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; | ||||
|
@@ -99,6 +101,7 @@ const fields = { | |||
ALERT_EVALUATION_VALUE, | ||||
ALERT_ID, | ||||
ALERT_OWNER, | ||||
ALERT_CONSUMERS, | ||||
ALERT_PRODUCER, | ||||
ALERT_REASON, | ||||
ALERT_RISK_SCORE, | ||||
|
@@ -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, | ||||
|
@@ -151,6 +155,7 @@ export { | |||
ALERT_NAMESPACE, | ||||
ALERT_RULE_NAMESPACE, | ||||
ALERT_OWNER, | ||||
ALERT_CONSUMERS, | ||||
ALERT_PRODUCER, | ||||
ALERT_REASON, | ||||
ALERT_RISK_SCORE, | ||||
|
@@ -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, | ||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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
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. When can we get 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.
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. 👍 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); | ||
|
@@ -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
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. Confused a bit, why do we need to check this? If 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. In some scenarios we could get an |
||
}); | ||
|
||
const concreteIndexName = `${alias}-000001`; | ||
|
||
if (!aliasExists) { | ||
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. I removed this check so that we can add the 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. 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, | ||
|
@@ -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, | ||
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. If the index already exists, will we ever get here? 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. 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
in this case both calls race to attempt to make the index and often one hits a 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. True. But if the lack of a write index was the cause for this function being called we'd just fail silently because 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. 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 |
||
// 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( | ||
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. Nit: 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. as of ES5 they're supposed to be equivalent, right? https://es5.github.io/#x15.11.1 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. 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 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 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( | ||
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. Nit: |
||
`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 |
---|---|---|
|
@@ -58,7 +58,7 @@ describe('Alert details with unmapped fields', () => { | |
|
||
it('Displays the unmapped field on the table', () => { | ||
const expectedUnmmappedField = { | ||
row: 55, | ||
row: 88, | ||
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. ? |
||
field: 'unmapped', | ||
text: 'This is the unmapped field', | ||
}; | ||
|
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 my understanding correct that
kibana.alert.owner
is going to get removed in favour ofkibana.consumers
?