-
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][Investigations] - Add kibana.alert.url #155069
[Security Solution][Investigations] - Add kibana.alert.url #155069
Conversation
b234ce5
to
85ae638
Compare
...k/plugins/security_solution/server/lib/detection_engine/routes/index/get_signals_template.ts
Outdated
Show resolved
Hide resolved
@@ -143,11 +148,20 @@ export const buildAlertRoot = ( | |||
alertTimestampOverride | |||
); | |||
const alertId = generateAlertId(doc); | |||
const alertUrl = getAlertDetailsUrl({ |
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.
Should the individual building block alerts for EQL sequences have alert URLs as well?
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.
Yep, added them
lists, | ||
logger, | ||
config, | ||
publicBaseUrl, |
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.
Does the use of publicBaseUrl
require customers to configure the core.http.basePath.publicBaseUrl
setting in order for alert URLs to work?
Rules created through the UI have a
meta: {from: "1m", kibana_siem_app_url: "http://localhost:5601/jde/app/security"}
object where the kibana_siem_app_url
is populated from the browser to provide results_link
functionality. Should we use that param in conjunction with the publicBaseUrl?
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.
Note from discussing on zoom: kibana_siem_app_url
does not get populated for prebuilt rules at the moment. We may want to update the prebuilt rule installation to populate that field for prebuilt rules. cc @banderror
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.
We may want to update the prebuilt rule installation to populate that field for prebuilt rules
I'm missing the context @marshallmain -- could you please explain what this field is used for? I don't know what the "results_link functionality" is.
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.
It's added to the actions context to provide users of 3rd party systems with a link back to the alerts table. However, since the Kibana server does not always know the public facing URL of Kibana we rely on this kibana_siem_app_url
rule parameter that is set by the UI when custom rules are created.
@@ -49,6 +49,9 @@ const ALERT_LAST_DETECTED = `${ALERT_NAMESPACE}.last_detected` as const; | |||
// kibana.alert.reason - human readable reason that this alert is active | |||
const ALERT_REASON = `${ALERT_NAMESPACE}.reason` as const; | |||
|
|||
// kibana.alert.url - url which will take the user directly to a view of the alert | |||
const ALERT_URL = `${ALERT_NAMESPACE}.url` as const; |
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.
Could you add ALERT_URL
to an alert schema for 8.8, similar to how we updated the BaseFields for 8.4 (x-pack/plugins/security_solution/common/detection_engine/schemas/alerts/8.4.0/index.ts) when adding ALERT_RULE_INDICES
?
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 am doing it another PR.
5b42f67
to
18fd4b5
Compare
@@ -21,6 +21,7 @@ const { | |||
applyDeltaToColumnWidth, | |||
changeViewMode, | |||
removeColumn, | |||
toggleDetailPanel, |
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.
This was added as a bugfix. The flyout kept re-opening on refresh of the page after being closed. This was due to the fact that the opening and closing of the flyout wasn't tracked for updating the localstorage config.
@@ -31,7 +31,7 @@ const AlertsContainerComponent: React.FC = () => { | |||
<Switch> | |||
<Route path={ALERTS_PATH} exact component={AlertsRoute} /> | |||
{/* Redirect to the alerts page filtered for the given alert id */} | |||
<Route path={`${ALERTS_PATH}/:alertId`} component={AlertDetailsRedirect} /> | |||
<Route path={`${ALERTS_PATH}/redirect/:alertId`} component={AlertDetailsRedirect} /> |
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.
Added the redirect
subpath, to be explicit about the path functionality, but also preserve any future paths we may want to use such as /alert
or /:id
, etc...
> | ||
{i18n.SHARE_ALERT} | ||
</EuiButtonEmpty> | ||
<EuiFlexItem grow={false}> |
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.
This was a visual bug fix for the position of the Share Alert Details
button here: #155030
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.
Not for this PR, but while desk testing, we noted that:
- URLs generated by the
kibana.alert.url
feature in this PR do NOT encode the:
characters in the×tamp=2023-04-24T18:10:36.225Z
URL parameter, per the example below:
http://localhost:5601/s/space-madness/app/security/alerts/redirect/aa7c1d26c66a7a549233c000bc8c240979a273abad6b50ada5b19f7dec86553a?index=.alerts-security.alerts-space-madness×tamp=2023-04-24T18:10:36.225Z
Above: A URL generated with the kibana.alert.url
feature in this PR
However, URLs generated by the Share Alert Deatils
button encode the :
characters in the ×tamp=2023-04-24T18%3A10%3A36.225Z
URL parameter, per the example below:
http://localhost:5601/s/space-madness/app/security/alerts/redirect/aa7c1d26c66a7a549233c000bc8c240979a273abad6b50ada5b19f7dec86553a?index=.alerts-security.alerts-space-madness×tamp=2023-04-24T18%3A10%3A36.225Z
Above: A URL for the same alert, generated with the Share Alert Details
button
Since it doesn't feel necessary to encode the :
characters, the differences in behavior may be worth investigating / normalizing.
}); | ||
if (isPreviewAlert) return null; | ||
const url = getAppUrl({ path: alertDetailPath }); | ||
// We use window.location.origin instead of http.basePath as the http.basePath has to be configured in config dev yml |
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.
While we expect publicBaseUrl
to be configured by most of our users, it's not guaranteed to always be configured. Since we have guaranteed access to the location.origin on the UI, we use that in place of core.http.publicBaseUrl
,
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 desk tested this with a specific alert, in a non-default space, and verified that:
✅ when Kibana is run without a base path, a valid link (with no base path) is copied:
http://localhost:5601/s/space-madness/app/security/alerts/redirect/8c6f83aeefa05a130769c3bf2419d6923714554916ce4b990966c7b9d1459261?index=.alerts-security.alerts-space-madness×tamp=2023-04-24T20%3A54%3A25.941Z
✅ when Kibana is run with a base path, a valid link for the same alert (that includes the base path) is copied:
http://localhost:5601/jgy/s/space-madness/app/security/alerts/redirect/8c6f83aeefa05a130769c3bf2419d6923714554916ce4b990966c7b9d1459261?index=.alerts-security.alerts-space-madness×tamp=2023-04-24T20%3A54%3A25.941
}) => { | ||
const alertDetailPath = buildAlertDetailPath({ alertId, index, timestamp }); | ||
const alertDetailPathWithAppPath = `${APP_PATH}${alertDetailPath}`; | ||
return basePath |
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.
core.http.publicBaseUrl
isn't guaranteed to be set, though it's highly likely that it will be. Given that condition, we will not produce a url if the basePath
is not set
6b7fffd
to
4bd1ec8
Compare
Pinging @elastic/security-threat-hunting (Team:Threat Hunting) |
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.
ResponseOps changes (one comment in one file) LGTM :-)
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.
AO changes LGTM
@@ -94,7 +94,7 @@ const ALERT_RULE_TAGS = `${ALERT_RULE_NAMESPACE}.tags` as const; | |||
// kibana.alert.rule_type_id - rule type id for rule that generated this alert | |||
const ALERT_RULE_TYPE_ID = `${ALERT_RULE_NAMESPACE}.rule_type_id` as const; | |||
|
|||
// kibana.alert.url - allow our user to go back to the details url in kibana | |||
// kibana.alert.url - url which will redirect users to the alert page filtered for the given alert |
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.
the alert page filtered for the given alert
nit: Since the AO team is working on introducing a dedicated page for each alert, this comment will not be accurate in the future. How about changing it to something like:
url which will redirect users to a page related to the given alert
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.
Yep, will do, thanks @maryam-saeidi . Are there any tickets for the planned work @maryam-saeidi ?
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.
Yes, we have different epics for different rule types, for example:
NewTermsFields840, | ||
} from '../8.4.0'; | ||
|
||
/* DO NOT MODIFY THIS SCHEMA TO ADD NEW FIELDS. These types represent the alerts that shipped in 8.6.0. |
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.
consider replacing 8.6.0
with 8.8.0
throughout this comment
*/ | ||
|
||
export type GenericAlert880 = AlertWithCommonFields800< | ||
BaseFields840 & { [ALERT_URL]: string | undefined } |
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.
Since ALERT_URL
is common to all the alert types, can you define it in a new interface BaseFields880
in this file and have the rule-specific alert interfaces extend BaseFields880
instead of the 8.4 versions of those interfaces?
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.
Also, originally when adding these schemas I was torn between whether we should extend prior versions when adding new fields (avoiding duplication but adding some complexity) or simply copy the old version and add the new fields. I think with the way the schemas have evolved it might be easier to read if we copy the previous version and accept the duplication of existing fields in the new schema.
I imagine labeling each field with when it was added could help with the duplication, e.g.
export interface BaseFields880 {
[TIMESTAMP]: string; // ADDED IN: 8.0.0
[SPACE_IDS]: string[]; // ADDED IN: 8.0.0
[EVENT_KIND]: 'signal'; // ADDED IN: 8.0.0
...
[ALERT_RULE_VERSION]: number; // ADDED IN: 8.0.0
'kibana.alert.rule.risk_score': number; // ADDED IN: 8.0.0
'kibana.alert.rule.severity': string; // ADDED IN: 8.0.0
'kibana.alert.rule.building_block_type': string | undefined; // ADDED IN: 8.0.0
[ALERT_RULE_INDICES]: string[]; // ADDED IN: 8.4.0
[ALERT_URL]: string | undefined; // ADDED IN: 8.8.0
[key: string]: SearchTypes; // ADDED IN: 8.0.0
}
For this PR it's completely fine to continue extending old versions of the interfaces, but I wanted to note that we don't have to stick with that design going forward and if it makes more sense to you to copy all the fields over then that's a reasonable approach too (and one we may switch to in the future). Of course interfaces within a particular version will still extend each other, e.g. EqlShellFields880 extends BaseFields880
.
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.
Yea, I wanted to do that, but since it doesn't look like the UUID is generated at that point, it made it a bit harder since the url depends on that. Since I didn't have access to the ALERT_UUID
in build_alert.ts
, I just followed the same pattern as how ALERT_UUID
was set in the 8.4.0 file.
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.
ruleExecutionLogger | ||
); | ||
|
||
const alertUrl = getAlertDetailsUrl({ |
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.
It looks like we call getAlertDetailsUrl
every time we call buildBulkBody
, can we move the details URL logic inside buildBulkBody
? That change will line up nicely with adding the ALERT_URL
to BaseFieldsLatest
since buildBulkBody
returns BaseFieldsLatest
.
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.
Yea, I wanted to do that, but it also relied on having the alert id, which for eql alerts was generated outside of buildBulkBody
. I can migrate that logic too into buildBulkBody
, but wasn't sure if there was a reason it was done separately, so I just left them all externally similar to how the kibana.alert.uuid
is set
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.
As we discussed on slack, I've updated the code so that the build_alert.ts
has the logic for creating the url, and also sets the id. As suggested, I've updated all of the types and added a placeholder id for the eql alerts that's later overridden: c916500
Perhaps not for this PR, but the following steps / video demonstrates an issue where, when the Alerts table grouping feature is enabled on the Alerts page, the flyout doesn't appear in a newly-loaded tab until table grouping is disabled (in the new tab). To reproduce (with a local Elasticsearch started with
If you run kibana with
or If you run kibana with
cd x-pack/plugins/security_solution && yarn test:generate && cd -
Expected result
Expected results
Expected results
Actual results
Expected results
Actual results
repro.mov |
}) => { | ||
const alertDetailPath = buildAlertDetailPath({ alertId, index, timestamp }); | ||
const alertDetailPathWithAppPath = `${APP_PATH}${alertDetailPath}`; | ||
return basePath |
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.
Consider updating the PR description to note that, as a result of this bathPath
check, the kibana.alert.url
feature in this PR must be enabled by adding one of the following configurations to config/kibana.dev.yml
:
If you run kibana with yarn start --no-base-path
:
server.publicBaseUrl: 'http://localhost:5601'
or
If you run kibana with yarn start
(and a base path):
server.basePath: '/foo'
server.publicBaseUrl: 'http://localhost:5601/foo'
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.
Added already 😄
4382b38
to
6056b08
Compare
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.
Thanks @michaelolo24 for this feature! 🙏
I successfully desk tested it with various combinations of:
✅ server.publicBaseUrl
configured without a base path (in kibana.dev.yml
), e.g.
server.publicBaseUrl: 'http://localhost:5601'
✅ server.publicBaseUrl
configured with a base path:
server.basePath: '/foo'
server.publicBaseUrl: 'http://localhost:5601/foo'
✅ the default
space
✅ a non-default space
In all cases, the redirect URL behaved as-expected apart from one observation that may be addressed in a follow-up PR.
LGTM 🚀
Thanks for this note. We were able to discuss a bit over zoom, but currently the flyout logic is embedded within the |
20b2783
to
c916500
Compare
💛 Build succeeded, but was flaky
Failed CI StepsMetrics [docs]Module Count
Async chunks
Page load bundle
Unknown metric groupsESLint disabled line counts
Total ESLint disabled count
History
To update your PR or re-run it, just comment with: |
|
Hey @logeekal thanks for bringing this up
|
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.
Looks great, thanks!
Summary
This PR introduces the field
kibana.alert.url
to the alerts generated by al alert rule types. Functionality was added in this PR for 8.8 to allow users to link directly to the alert flyout. To be able to provide users with this field via our connectors, we are adding the url under the fieldkibana.alert.url
.To test, create an alert of any type and you should see this field set in the alert flyout:

The url provided is a redirect path that contains the necessary information (space, id, index, and timestamp) to be able to redirect the user to a filtered alert page for the given alert and the detail flyout opened. This allows us to retain flexibility in the future for any changes that may occur with the alert flyout or an alert page. More on that can be found in the earlier pr: #148800
Testing
kibana.alert.url
field makes use of thepublicBaseUrl
configuration which must be set in your kibana.dev.yml for this field to be generated. Add the following to your yaml file. Note that if you use abasePath
, it will have to be appended to the end of yourpublicBaseUrl
path.with basePath:
kibana.alert.url
in the table.***Caveat - when grouping is enabled, the details flyout will not open as the table that it is attached to is not actually loaded at that point in time. When the table is loaded by either disabling grouping or opening the group, the details flyout will open