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

[TIP] add field existence check to the painless script #144344

Merged
merged 1 commit into from Nov 8, 2022
Merged

[TIP] add field existence check to the painless script #144344

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
31 changes: 21 additions & 10 deletions x-pack/plugins/threat_intelligence/common/types/indicator.ts
View file
Open in desktop
Expand Up @@ -32,13 +32,12 @@ export enum RawIndicatorFieldId {
FileSha3512 = 'threat.indicator.file.hash.sha3-512',
FileSha512224 = 'threat.indicator.file.hash.sha512/224',
FileSha512256 = 'threat.indicator.file.hash.sha512/256',
FileSSDeep = 'threat.indicator.file.ssdeep',
FileTlsh = 'threat.indicator.file.tlsh',
FileImpfuzzy = 'threat.indicator.file.impfuzzy',
FileImphash = 'threat.indicator.file.imphash',
FilePehash = 'threat.indicator.file.pehash',
FileVhash = 'threat.indicator.file.vhash',
FileTelfhash = 'threat.indicator.file.elf.telfhash',
FileSSDeep = 'threat.indicator.file.hash.ssdeep',
FileTlsh = 'threat.indicator.file.hash.tlsh',
FileImpfuzzy = 'threat.indicator.file.hash.impfuzzy',
FileImphash = 'threat.indicator.file.hash.imphash',
FilePehash = 'threat.indicator.file.hash.pehash',
FileVhash = 'threat.indicator.file.hash.vhash',
Comment on lines +35 to +40
Copy link
Contributor

Choose a reason for hiding this comment

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

modified these using this source of truth

X509Serial = 'threat.indicator.x509.serial_number',
WindowsRegistryKey = 'threat.indicator.registry.key',
WindowsRegistryPath = 'threat.indicator.registry.path',
Expand All @@ -56,12 +55,24 @@ export enum RawIndicatorFieldId {
* (reverse of https://github.com/elastic/kibana/blob/main/x-pack/plugins/security_solution/common/cti/constants.ts#L35)
*/
export const IndicatorFieldEventEnrichmentMap: { [id: string]: string[] } = {
[RawIndicatorFieldId.FileSha256]: ['file.hash.sha256'],
Copy link
Contributor

Choose a reason for hiding this comment

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

this fixes an unrelated bug: happens when the grid is trying to display the investigate in timeline button, the data grid crashes.
This bug was hidden behind the one the PR is trying to fix.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Are we sure that this fixes it for good? What if there is no threat.indicator.file.hash.* field on the document. See for example https://github.com/elastic/seceng/issues/4768#issuecomment-1303165947 , their documents have the prefix threatintel.* instead of threat.* therefore neither of these mappings will work. But I believe we should still handle this gracefully and don't render the Investigate In Timeline action if it is not supported

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 also think this is a good candidate for 8.5 backporting

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it all depends on how we handle the original bug: if we show the value in the grid with a - for example (as you suggested earlier) then this might break. If we don't show the entry in the grid this should be good, as this list matches the list we use in the painless script.

Once we figure out the original bug, I can update this piece of code as well

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 added a bit more safety to the use_investigate_in_timeline hook to avoid issues when a new hash is added to one list and not added to another. Ideally, we let the type checker do that and do not rely on syncing two lists manually

[RawIndicatorFieldId.FileMd5]: ['file.hash.md5'],
[RawIndicatorFieldId.FileSha1]: ['file.hash.sha1'],
[RawIndicatorFieldId.FileSha256]: ['file.hash.sha256'],
[RawIndicatorFieldId.FileImphash]: ['file.pe.imphash'],
[RawIndicatorFieldId.FileTelfhash]: ['file.elf.telfhash'],
[RawIndicatorFieldId.FileSha224]: ['file.hash.sha224'],
[RawIndicatorFieldId.FileSha3224]: ['file.hash.sha3-224'],
[RawIndicatorFieldId.FileSha3256]: ['file.hash.sha3-256'],
[RawIndicatorFieldId.FileSha384]: ['file.hash.sha384'],
[RawIndicatorFieldId.FileSha3384]: ['file.hash.sha3-384'],
[RawIndicatorFieldId.FileSha512]: ['file.hash.sha512'],
[RawIndicatorFieldId.FileSha3512]: ['file.hash.sha3-512'],
[RawIndicatorFieldId.FileSha512224]: ['file.hash.sha512/224'],
[RawIndicatorFieldId.FileSha512256]: ['file.hash.sha512/256'],
[RawIndicatorFieldId.FileSSDeep]: ['file.hash.ssdeep'],
Copy link
Contributor

Choose a reason for hiding this comment

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

same here, modifying following this

[RawIndicatorFieldId.FileTlsh]: ['file.hash.tlsh'],
[RawIndicatorFieldId.FileImpfuzzy]: ['file.hash.impfuzzy'],
[RawIndicatorFieldId.FileImphash]: ['file.hash.imphash'],
[RawIndicatorFieldId.FilePehash]: ['file.hash.pehash'],
[RawIndicatorFieldId.FileVhash]: ['file.hash.vhash'],
[RawIndicatorFieldId.Ip]: ['source.ip', 'destination.ip'],
[RawIndicatorFieldId.UrlFull]: ['url.full'],
[RawIndicatorFieldId.WindowsRegistryPath]: ['registry.path'],
Expand Down
84 changes: 82 additions & 2 deletions x-pack/plugins/threat_intelligence/cypress/e2e/indicators.cy.ts
View file
Open in desktop
Expand Up @@ -26,9 +26,13 @@ import {
INDICATORS_TABLE,
INDICATORS_TABLE_FEED_NAME_COLUMN_HEADER,
INDICATORS_TABLE_FIRST_SEEN_COLUMN_HEADER,
INDICATORS_TABLE_INDICATOR_NAME_CELL,
INDICATORS_TABLE_INDICATOR_NAME_COLUMN_HEADER,
INDICATORS_TABLE_INDICATOR_TYPE_CELL,
INDICATORS_TABLE_INDICATOR_TYPE_COLUMN_HEADER,
INDICATORS_TABLE_INVESTIGATE_IN_TIMELINE_BUTTON_ICON,
INDICATORS_TABLE_LAST_SEEN_COLUMN_HEADER,
INDICATORS_TABLE_ROW_CELL,
INSPECTOR_BUTTON,
INSPECTOR_PANEL,
LEADING_BREADCRUMB,
Expand All @@ -50,12 +54,88 @@ const THREAT_INTELLIGENCE = '/app/security/threat_intelligence/indicators';
const URL_WITH_CONTRADICTORY_FILTERS =
'/app/security/threat_intelligence/indicators?indicators=(filterQuery:(language:kuery,query:%27%27),filters:!((%27$state%27:(store:appState),meta:(alias:!n,disabled:!f,index:%27%27,key:threat.indicator.type,negate:!f,params:(query:file),type:phrase),query:(match_phrase:(threat.indicator.type:file))),(%27$state%27:(store:appState),meta:(alias:!n,disabled:!f,index:%27%27,key:threat.indicator.type,negate:!f,params:(query:url),type:phrase),query:(match_phrase:(threat.indicator.type:url)))),timeRange:(from:now/d,to:now/d))';

describe('Invalid Indicators', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

this section tests that the table shows up even with missing fields. The following logic is applied:

  • test with all fields present
  • test with missing sha256
  • test with missing sha256 and md5
  • continue removing fields until there are none left

The UI Indicator name column should display the proper value for the present fields.
If no fields are present, the Indicator name column shows -. and the investigate in timeline button is not rendered

Copy link
Contributor

Choose a reason for hiding this comment

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

added more tests for the following:

  • missing threat field entirely
  • missing type field
  • missing indicator field

I think this is overkill but we should be safe

describe('verify the grid loads even with missing fields', () => {
before(() => {
esArchiverLoad('threat_intelligence/invalid_indicators_data');

cy.visit(THREAT_INTELLIGENCE);
selectRange();
});
after(() => {
esArchiverUnload('threat_intelligence/invalid_indicators_data');
});

it('should display data grid despite the missing fields', () => {
cy.get(INDICATORS_TABLE).should('exist');

// there are 19 documents in the x-pack/test/threat_intelligence_cypress/es_archives/threat_intelligence/invalid_indicators_data/data.json
const documentsNumber = 22;
cy.get(INDICATORS_TABLE_ROW_CELL).should('have.length.gte', documentsNumber);

// the last 3 documents have no hash so the investigate in timeline button isn't rendered
cy.get(INDICATORS_TABLE_INVESTIGATE_IN_TIMELINE_BUTTON_ICON).should(
'have.length',
documentsNumber - 4
);

// we should have 21 documents plus the header
cy.get(INDICATORS_TABLE_INDICATOR_NAME_CELL).should('have.length', documentsNumber + 1);

// this entry has no hash to we show - in the Indicator Name column
cy.get(INDICATORS_TABLE_INDICATOR_NAME_CELL)
.eq(documentsNumber - 3)
.should('contain.text', '-');

// this entry is missing the file key entirely
cy.get(INDICATORS_TABLE_INDICATOR_NAME_CELL)
.eq(documentsNumber - 2)
.should('contain.text', '-');

// this entry is missing the type field
cy.get(INDICATORS_TABLE_INDICATOR_NAME_CELL)
.eq(documentsNumber - 1)
.should('contain.text', '-');
cy.get(INDICATORS_TABLE_INDICATOR_TYPE_CELL)
.eq(documentsNumber - 1)
.should('contain.text', '-');

// this entry is missing the type field
cy.get(INDICATORS_TABLE_INDICATOR_NAME_CELL).last().should('contain.text', '-');
cy.get(INDICATORS_TABLE_INDICATOR_TYPE_CELL).last().should('contain.text', '-');
});
});

describe('verify the grid loads even with missing mappings and missing fields', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

same tests as above but this time with a missing mappings.json file. Not sure these tests are really useful though...

before(() => {
esArchiverLoad('threat_intelligence/missing_mappings_indicators_data');

cy.visit(THREAT_INTELLIGENCE);
selectRange();
});
after(() => {
esArchiverUnload('threat_intelligence/missing_mappings_indicators_data');
});

it('should display data grid despite the missing mappings and missing fields', () => {
cy.get(INDICATORS_TABLE).should('exist');

// there are 2 documents in the x-pack/test/threat_intelligence_cypress/es_archives/threat_intelligence/missing_mappings_indicators_data/data.json
const documentsNumber = 2;
cy.get(INDICATORS_TABLE_ROW_CELL).should('have.length.gte', documentsNumber);

// we should have 2 documents plus the header
cy.get(INDICATORS_TABLE_INDICATOR_NAME_CELL).should('have.length', documentsNumber + 1);
});
});
});

describe('Indicators', () => {
before(() => {
esArchiverLoad('threat_intelligence');
esArchiverLoad('threat_intelligence/indicators_data');
});
after(() => {
esArchiverUnload('threat_intelligence');
esArchiverUnload('threat_intelligence/indicators_data');
});

describe('Indicators page loading', () => {
Expand Down
4 changes: 2 additions & 2 deletions x-pack/plugins/threat_intelligence/cypress/e2e/query_bar.cy.ts
View file
Open in desktop
Expand Up @@ -36,10 +36,10 @@ const THREAT_INTELLIGENCE = '/app/security/threat_intelligence/indicators';

describe('Indicators', () => {
before(() => {
esArchiverLoad('threat_intelligence');
esArchiverLoad('threat_intelligence/indicators_data');
});
after(() => {
esArchiverUnload('threat_intelligence');
esArchiverUnload('threat_intelligence/indicators_data');
});

describe('Indicators query bar interaction', () => {
Expand Down
4 changes: 2 additions & 2 deletions x-pack/plugins/threat_intelligence/cypress/e2e/timeline.cy.ts
View file
Open in desktop
Expand Up @@ -34,10 +34,10 @@ before(() => {

describe('Indicators', () => {
before(() => {
esArchiverLoad('threat_intelligence');
esArchiverLoad('threat_intelligence/indicators_data');
});
after(() => {
esArchiverUnload('threat_intelligence');
esArchiverUnload('threat_intelligence/indicators_data');
});

describe('Indicators timeline interactions', () => {
Expand Down
6 changes: 5 additions & 1 deletion x-pack/plugins/threat_intelligence/cypress/screens/indicators.ts
View file
Open in desktop
Expand Up @@ -21,7 +21,11 @@ export const DEFAULT_LAYOUT_TITLE = `[data-test-subj="tiDefaultPageLayoutTitle"]

export const INDICATORS_TABLE = `[data-test-subj="tiIndicatorsTable"]`;

export const INDICATORS_TABLE_TIMESTAMP_COLUMN_HEADER = `[data-test-subj="dataGridHeaderCell-@timestamp"]`;
export const INDICATORS_TABLE_ROW_CELL = `[data-test-subj="dataGridRowCell"]`;

export const INDICATORS_TABLE_INDICATOR_NAME_CELL = `[data-gridcell-column-id="threat.indicator.name"]`;

export const INDICATORS_TABLE_INDICATOR_TYPE_CELL = `[data-gridcell-column-id="threat.indicator.type"]`;

export const INDICATORS_TABLE_INDICATOR_NAME_COLUMN_HEADER = `[data-test-subj="dataGridHeaderCell-threat.indicator.name"]`;

Expand Down
22 changes: 22 additions & 0 deletions ...ns/threat_intelligence/public/modules/timeline/hooks/use_investigate_in_timeline.test.tsx
View file
Open in desktop
Expand Up @@ -27,6 +27,28 @@ describe('useInvestigateInTimeline()', () => {
expect(hookResult.result.current).toEqual({});
});

it('should return empty object if name_origin value is missing on the mapping investigate in timeline mapping', () => {
const indicator: Indicator = generateMockUrlIndicator();
indicator.fields['threat.indicator.name_origin'] = ['threat.indicator.url.missing'];

hookResult = renderHook(() => useInvestigateInTimeline({ indicator }), {
wrapper: TestProvidersComponent,
});

expect(hookResult.result.current).toEqual({});
});

it('should return empty object if @timestamp is missing', () => {
const indicator: Indicator = generateMockUrlIndicator();
indicator.fields['@timestamp'] = undefined;

hookResult = renderHook(() => useInvestigateInTimeline({ indicator }), {
wrapper: TestProvidersComponent,
});

expect(hookResult.result.current).toEqual({});
});

it('should return investigateInTimelineFn', () => {
const indicator: Indicator = generateMockUrlIndicator();

Expand Down
12 changes: 9 additions & 3 deletions .../plugins/threat_intelligence/public/modules/timeline/hooks/use_investigate_in_timeline.ts
View file
Open in desktop
Expand Up @@ -42,17 +42,23 @@ export const useInvestigateInTimeline = ({
const securitySolutionContext = useContext(SecuritySolutionContext);

const { key, value } = getIndicatorFieldAndValue(indicator, RawIndicatorFieldId.Name);
if (!fieldAndValueValid(key, value)) {
const sourceEventField = IndicatorFieldEventEnrichmentMap[key];

if (!fieldAndValueValid(key, value) || !sourceEventField) {
return {} as unknown as UseInvestigateInTimelineValue;
}

const dataProviders: DataProvider[] = [...IndicatorFieldEventEnrichmentMap[key], key].map(
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 believe this bug shows that we should be more strict with the types as we rely on a very specific set of fields for this feature

(e: string) => generateDataProvider(e, value as string)
const dataProviders: DataProvider[] = [...sourceEventField, key].map((e: string) =>
generateDataProvider(e, value as string)
);

const to = unwrapValue(indicator, RawIndicatorFieldId.TimeStamp) as string;
const from = moment(to).subtract(10, 'm').toISOString();

if (!to || !from) {
return {} as unknown as UseInvestigateInTimelineValue;
}

const investigateInTimelineFn = securitySolutionContext?.getUseInvestigateInTimeline({
dataProviders,
from,
Expand Down