[CTI] Adds Threat Intel dashboard links to Overview page #100423
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ecezalp
force-pushed
the
security-team-1098
branch
5 times, most recently
from
9ae851f to
1a04bdb
Compare
ecezalp
changed the title
ecezalp
added
auto-backport
ecezalp
force-pushed
the
security-team-1098
branch
from
1a04bdb to
a94a04d
Compare
ecezalp
force-pushed
the
security-team-1098
branch
from
a94a04d to
32afd68
Compare
|
Pinging @elastic/security-solution (Team: SecuritySolution) |
rylnd
reviewed
May 28, 2021
There was a problem hiding this comment.
This is looking great! I had a few nits but nothing major.
At the feature level, I had a few questions that we should bring up as a group:
-
No card appears if dashboards are not loaded. While dashboards are expected and those links are useful here, I would expect that if a user had not loaded the dashboards that we would still display the counts of each module without links.
-
Only displaying TI in the given timerange: while this is most consistent with the other cards, not having any recent indicators for e.g. abuseurl means that I can't quickly navigate to that dashboard. Perhaps we could display links for all dashboards unconditionally, but only display counts for the time range?
x-pack/plugins/security_solution/public/overview/components/overview_cti_links/index.test.tsx
Outdated
Show resolved
Hide resolved
x-pack/plugins/security_solution/public/overview/components/overview_cti_links/index.test.tsx
Outdated
Show resolved
Hide resolved
|
|
||
| const returnVal = { | ||
| eventCounts: data.reduce((acc, item) => { | ||
| if (item.y && item.g?.match('threatintel.')) { |
There was a problem hiding this comment.
Instead of passing an empty query and checking the format of the dataset value (which may not always conform to this structure), I think it'll be more accurate and simpler to query instead for event.kind: 'indicator', which should guarantee that we're only dealing with indicator data.
| const returnVal = { | ||
| eventCounts: data.reduce((acc, item) => { | ||
| if (item.y && item.g?.match('threatintel.')) { | ||
| const id = item.g.replace('threatintel.', ''); |
There was a problem hiding this comment.
This is unfortunate 😦 ; fileset.name contains the unscoped version of this value but that's very filebeat specific.
@P1llus is there a strong reason for prefixing event.dataset values with threatintel.? That seems somewhat redundant with event.module: 'threatintel'. If not, could we perhaps update that for this 7.14?
There was a problem hiding this comment.
This topic is still in discussion on multiple places, so I don't know, as one would be removed in packages. So we would have to put this on hold at least for another few days.
.../plugins/security_solution/public/overview/containers/overview_cti_links/get_event_counts.ts
Outdated
Show resolved
Hide resolved
x-pack/plugins/security_solution/public/overview/containers/overview_cti_links/index.tsx
Outdated
Show resolved
Hide resolved
x-pack/plugins/security_solution/public/overview/containers/overview_cti_links/index.tsx
Outdated
Show resolved
Hide resolved
rylnd
reviewed
Jun 3, 2021
| @@ -49,6 +50,7 @@ export interface SetupPlugins { | |||
| export interface StartPlugins { | |||
| cases: CasesUiStart; | |||
| data: DataPublicPluginStart; | |||
| dashboard: DashboardStart; | |||
There was a problem hiding this comment.
We've declared dashboard as an optional dependency, but this is saying it will be always available.
Suggested change
| dashboard: DashboardStart; | |
| dashboard?: DashboardStart; |
We should consider what to do in the case where this plugin is disabled, but I assume it's the same as if dashboards are not installed.
| filterQuery: convertToBuildEsQuery({ | ||
| config: esQuery.getEsQueryConfig(uiSettings), | ||
| indexPattern, | ||
| queries: [{ query: `event.kind = "enrichment"`, language: 'kql' }], |
There was a problem hiding this comment.
A few notes here:
- Is this actually KQL? I would expect the syntax to be
event.kind: "enrichment" - I don't see this filter reflected in the network request, and I don't think it's being applied, so we're receiving ALL events in these indices, and then filtering down in memory to those whose datasets start with
threatintel. - We're doing a lot of unnecessary work with this query:
- Using an interval of 12h is unnecessary and just costs extra time/bandwidth/summing logic on the client; we just want total counts over the entire interval
- We're also returning
hitsbut not using those; you can specifysize: 0on the outer query to prevent these from being returned.
There was a problem hiding this comment.
Also: I think event.type: "indicator" is more accurate than event.kind: "enrichment"; I frankly don't know what the latter is trying to convey and I'd suggest we focus on the former.
|
jenkins, test this (restarting due to jenkins upgrade) |
|
@elasticmachine merge upstream |
|
@elasticmachine merge upstream |
rylnd
approved these changes
Jun 24, 2021
There was a problem hiding this comment.
LGTM! Lots of comments, but nothing that should block this from merging!
The dashboard SO parsing, and manipulation of indicator fields are of most concern; we should think about how to minimize brittleness there.
We definitely need a cypress test or two, here, as well, but that can come in a later PR.
| store = createStore(myState, SUB_PLUGINS_REDUCER, kibanaObservable, storage); | ||
| }); | ||
|
|
||
| it('renders danger inner panel', () => { |
There was a problem hiding this comment.
What is a "danger inner panel?" It might be clearer to describe this from the user perspective.
There was a problem hiding this comment.
updated description to renders splitPanel with "danger" variant
| </Provider> | ||
| ); | ||
|
|
||
| expect(wrapper.find('[data-test-subj="cti-with-events"]').length).toEqual(1); |
There was a problem hiding this comment.
Nit: I think this form reads a little better:
Suggested change
| expect(wrapper.find('[data-test-subj="cti-with-events"]').length).toEqual(1); | |
| expect(wrapper.exists('[data-test-subj="cti-with-events"]')).toBe(true); |
| body={i18n.DANGER_BODY} | ||
| button={ | ||
| <EuiFlexItem> | ||
| <EuiButton href={threatIntelDocLink} color={'danger'} style={{ maxWidth: 150 }} fill> |
There was a problem hiding this comment.
Is there significance to the 150 here? Just trying to keep magic numbers/strings to a minimum.
There was a problem hiding this comment.
updated CtiInnerPanel to use Eui more efficiently to limit button growth
| padding: 0; | ||
| margin-top: ${({ theme }) => theme.eui.paddingSizes.m}; | ||
| margin-left: 12px; | ||
| transform: scale(${({ color }) => (color === 'primary' ? 1.4 : 1)}); |
There was a problem hiding this comment.
Is this to make the (i) icon closer in size to the other icons?
|
|
||
| CtiNoEventsComponent.displayName = 'CtiNoEvents'; | ||
|
|
||
| export const CtiNoEvents = React.memo(CtiNoEventsComponent); |
There was a problem hiding this comment.
How are you deciding when to use React.memo for these components? Asking for a friend 😉
There was a problem hiding this comment.
this unofficial guide seems to have some good information - I see React.memo being used in the codebase liberally, and I haven't hesitated to add it to a component with no props as that seems to be very safe
| if (DashboardsSO?.savedObjects?.length) { | ||
| const dashboardUrls = await Promise.all( | ||
| DashboardsSO.savedObjects.map((SO) => | ||
| createDashboardUrl!({ |
There was a problem hiding this comment.
Can we try to refactor this to get rid of the !? I realize that the isDashboardPluginDisabled guard above ensures that this'll be here, but it would be nice to get typescript to infer that. Otherwise this ! has the potential to mask legitimate type errors in the future.
There was a problem hiding this comment.
updated to remove !
| export const isOverviewItem = (item: { path?: string; title?: string }) => | ||
| item.title === OVERVIEW_DASHBOARD_LINK_TITLE; | ||
|
|
||
| export const createLinkFromDashboardSO = ( |
There was a problem hiding this comment.
This logic seems brittle to dashboard changes. Have you thought about ways to make this integration more robust? Do we have tests that will fail if e.g. the dashboard titles are changed?
There was a problem hiding this comment.
great question. I got confirmation from Andrew Pease regarding the relationship between dashboard names and the corresponding threatintel. field names, that we can consistently assume that removal of spaces and lowercasing for the title will correspond to the field name. I definitely agree that this is a suboptimal approach, but as of this moment there is no other information on the dashboard object that enables us to infer which SO corresponds to which source. we should update this logic in the close feature, created #103365 to keep track of the change
|
|
||
| export const TAG_REQUEST_BODY = { | ||
| type: 'tag', | ||
| search: 'threat intel', |
There was a problem hiding this comment.
This should probably be called out as its own constant.
| const { to, from } = useMemo( | ||
| () => ({ | ||
| to: new Date().toISOString(), | ||
| from: new Date('01 January 1970 00:00 UTC').toISOString(), |
There was a problem hiding this comment.
Nit:
Suggested change
| from: new Date('01 January 1970 00:00 UTC').toISOString(), | |
| from: new Date(0).toISOString(), |
| filters: [], | ||
| }), | ||
| histogramType: MatrixHistogramType.events, | ||
| indexNames: ['filebeat-*'], |
There was a problem hiding this comment.
Similar to investigation enrichment, we should discuss whether to hardcode this to filebeat or to respect the user's configured datasources on the overview page. An argument against hardcoding is that it discourages users from defining their own indicator indexes (outside of the filebeat-* pattern, at least).
There was a problem hiding this comment.
cool, since the discussion is already taking place at the linked thread I am not creating a new issue - I will assign this to a constant for now in the CTI constants file for better consistency.
rylnd
reviewed
Jun 24, 2021
| @@ -25,11 +25,9 @@ export const CtiDisabledModuleComponent = () => { | |||
| title={i18n.DANGER_TITLE} | |||
| body={i18n.DANGER_BODY} | |||
| button={ | |||
| <EuiFlexItem> | |||
|
@elasticmachine merge upstream |
kibanamachine
pushed a commit
to kibanamachine/kibana
that referenced
this pull request
Jun 28, 2021
💚 Backport successful
This backport PR will be merged automatically after passing CI. |
💔 Build Failed
Failed CI Steps
Test FailuresKibana Pipeline / general / task-queue-process-10 / X-Pack Endpoint API Integration Tests.x-pack/test/security_solution_endpoint_api_int/apis/metadata·ts.Endpoint plugin test metadata api POST /api/endpoint/metadata when index is not empty metadata api should return one entry for each host with default pagingStandard OutStack TraceKibana Pipeline / general / Chrome UI Functional Tests.test/functional/apps/visualize/_vega_chart·ts.visualize app visualize ciGroup12 vega chart in visualize app vega chart initial render should have view and control containersStandard OutStack TraceKibana Pipeline / general / X-Pack Accessibility Tests.x-pack/test/accessibility/apps/spaces·ts.Kibana spaces page meets a11y validations a11y test for space selection pageStandard OutStack Traceand 2 more failures, only showing the first 3. Metrics [docs]Module Count
Async chunks
History
To update your PR or re-run it, just comment with: cc @ecezalp |
3 tasks
3 tasks
1 task
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
This change adds Threat Intel dashboard links to the Overview page.
screenshot
notes
currently the link panel does not take the user to the Dashboard with the selected date range on the Overview page.Dashboard links now carry over the time range from the securitySolution to the dashboard app. To enable this feature, the dashboard plugin had to be added to the security solution.To add dashboards with data
cd beats && git checkout master && git fetch && git pull && cd x-pack/filebeatbeats/x-pack/filebeat/filebeat.ymloutput.elasticsearchsetup.kibanaexample
mage buildto build filebeat./filebeat modules enable threatinteland editmodules.d/threatintel.ymlto ensure that all values forenabledaretrue./filebeat setup -E setup.dashboards.directory=build/kibanato setup dashboards (while kibana is running)./filebeat -eto start filebeatChecklist
Delete any items that are not applicable to this PR.
Additional States
threat intel module disabled
dashboard module disabled and there are events for the selected time period
dashboards are disabled and there are no events for selected time period
modules are enabled but there are no events for selected time period
modules are enabled and there are events
