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

[Detection Engine] ML Rule Alert Suppression - Followup #188267

Merged
merged 11 commits into from
Jul 23, 2024
Original file line number Diff line number Diff line change
Expand Up @@ -477,77 +477,90 @@ const StepDefineRuleComponent: FC<StepDefineRuleProps> = ({
isEqlSequenceQuery(queryBar?.query?.query as string) &&
groupByFields.length === 0;

const isSuppressionGroupByDisabled =
/**
* If we don't have ML field information, users can't meaningfully interact with these fields */
const areSuppressionFieldsDisabledByMlFields =
isMlRule(ruleType) && (noMlJobsStarted || mlFieldsLoading || !mlSuppressionFields.length);
Copy link
Contributor

Choose a reason for hiding this comment

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

Screenshot 2024-07-17 at 12 05 43

I added two jobs (one running, one not). Selected field, configured suppression and then removed running job.
So, suppression config is disabled, but field actually was from a running job.

If I save rule, with "disabled" suppression and start job - suppression would work, but the field stored there would be from another job.

I can't clear this suppression on form , because when I removed job, it is still disabled
Screenshot 2024-07-17 at 12 13 15

So, I have to add the same running job to clear it.

Similar can happen when user changes index, without adjusting suppression fields. But in this case, suppression controls are not disabled and user can easily change those fields
Here, after removing running job, controls become disabled. It requires user to add this job again to change fields or start another.
I am not sure, how critical it can be - just an observation on behaviour.
What do you think about it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for calling this out. I agree that the UX wasn't great, so I fixed it in 8b6c460.


const areSuppressionFieldsDisabled =
!isAlertSuppressionLicenseValid ||
areSuppressionFieldsDisabledBySequence ||
isEsqlSuppressionLoading ||
(isMlRule(ruleType) && (noMlJobsStarted || mlFieldsLoading || !mlSuppressionFields.length));
areSuppressionFieldsDisabledByMlFields;

const suppressionGroupByDisabledText = areSuppressionFieldsDisabledBySequence
? i18n.EQL_SEQUENCE_SUPPRESSION_DISABLE_TOOLTIP
: isMlRule(ruleType) && noMlJobsStarted
? i18n.MACHINE_LEARNING_SUPPRESSION_DISABLED_LABEL
: alertSuppressionUpsellingMessage;
const isSuppressionGroupByDisabled = areSuppressionFieldsDisabled || isEsqlSuppressionLoading;

const suppressionGroupByFields = isEsqlRule(ruleType)
? esqlSuppressionFields
: isMlRule(ruleType)
? mlSuppressionFields
: termsAggregationFields;
const suppressionGroupByDisabledText = useMemo(() => {
if (areSuppressionFieldsDisabledBySequence) {
return i18n.EQL_SEQUENCE_SUPPRESSION_DISABLE_TOOLTIP;
} else if (isMlRule(ruleType) && noMlJobsStarted) {
return i18n.MACHINE_LEARNING_SUPPRESSION_DISABLED_LABEL;
} else {
return alertSuppressionUpsellingMessage;
}
}, [
alertSuppressionUpsellingMessage,
areSuppressionFieldsDisabledBySequence,
noMlJobsStarted,
ruleType,
]);

const suppressionGroupByFields = useMemo(() => {
if (isEsqlRule(ruleType)) {
return esqlSuppressionFields;
} else if (isMlRule(ruleType)) {
return mlSuppressionFields;
} else {
return termsAggregationFields;
}
}, [esqlSuppressionFields, mlSuppressionFields, ruleType, termsAggregationFields]);

/**
* Component that allows selection of suppression intervals disabled:
* - if suppression license is not valid(i.e. less than platinum)
* - or for not threshold rule - when groupBy fields not selected
* - Eql sequence is used
* - ML Field information is not available
*/
const isGroupByChildrenDisabled =
areSuppressionFieldsDisabledBySequence || !isAlertSuppressionLicenseValid || isThresholdRule
? false
: !groupByFields?.length;
areSuppressionFieldsDisabled || (isThresholdRule ? false : !groupByFields?.length);

/**
* Per rule execution radio option is disabled
* - if suppression license is not valid(i.e. less than platinum)
* - always disabled for threshold rule
* - Eql sequence is used and suppression fields are in the default state
* - ML Field information is not available
*/
const isPerRuleExecutionDisabled =
areSuppressionFieldsDisabledBySequence || !isAlertSuppressionLicenseValid || isThresholdRule;
const isPerRuleExecutionDisabled = areSuppressionFieldsDisabled || isThresholdRule;

/**
* Per time period execution radio option is disabled
* - if suppression license is not valid(i.e. less than platinum)
* - disabled for threshold rule when enabled suppression is not checked
* - Eql sequence is used and suppression fields are in the default state
* - ML Field information is not available
*/
const isPerTimePeriodDisabled =
areSuppressionFieldsDisabledBySequence ||
!isAlertSuppressionLicenseValid ||
(isThresholdRule && !enableThresholdSuppression);
areSuppressionFieldsDisabled || (isThresholdRule && !enableThresholdSuppression);

/**
* Suppression duration is disabled when
* - if suppression license is not valid(i.e. less than platinum)
* - when suppression by rule execution is selected in radio button
* - when threshold suppression is not enabled and no group by fields selected
* - Eql sequence is used and suppression fields are in the default state
* - ML Field information is not available
* */
const isDurationDisabled =
vitaliidm marked this conversation as resolved.
Show resolved Hide resolved
areSuppressionFieldsDisabledBySequence ||
!isAlertSuppressionLicenseValid ||
(!enableThresholdSuppression && groupByFields?.length === 0);
areSuppressionFieldsDisabled || (!enableThresholdSuppression && groupByFields?.length === 0);

/**
* Suppression missing fields is disabled when
* - if suppression license is not valid(i.e. less than platinum)
* - when no group by fields selected
* - Eql sequence is used and suppression fields are in the default state
* */
const isMissingFieldsDisabled =
areSuppressionFieldsDisabledBySequence ||
!isAlertSuppressionLicenseValid ||
!groupByFields.length;
const isMissingFieldsDisabled = areSuppressionFieldsDisabled || !groupByFields.length;

Copy link
Contributor

@vitaliidm vitaliidm Jul 19, 2024

Choose a reason for hiding this comment

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

Screen.Recording.2024-07-19.at.09.54.54.mov

I see some confusing behaviour here.

a1 job is started
Spike in logon job is not installed

When only one selected everything seemed to work as expected.

When both of them - behaviour becomes unpredictable. When I removed a1 job, selected field still there, dropdown shows list of available fields(presumably from removed a1 job). No warnings that some of the job not started
When I deselect suppression fields - control becomes disabled with tooltip message

Copy link
Contributor

Choose a reason for hiding this comment

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

Spike in logon events is not installed, when selected with running job, no warning is displayed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@vitaliidm did you notice that this behavior was specific to custom ML jobs? I can't reproduce that with just prebuilt ones.

  1. Can you provide the steps you took to create that a1 job?
  2. Can you reproduce this behavior in main, or just on this branch?

Copy link
Contributor

@vitaliidm vitaliidm Jul 19, 2024

Choose a reason for hiding this comment

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

@rylnd

  1. Job was created through wizard
Screen.Recording.2024-07-19.at.17.02.40.mov
  1. This is behaviour in main
Screen.Recording.2024-07-19.at.16.59.43.mov

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@vitaliidm this is certainly an interesting one that I hadn't encountered. I'm still not able to reproduce; after creating a custom job (and getting it in a persistent "started" state), I see the expected behavior:

Screen.Recording.2024-07-19.at.4.51.00.PM.mov

And you can see in the following screenshot that the custom job shows up with the rest of the jobs and is otherwise treated no differently (which was my previous theory on this behavior):

Screenshot 2024-07-19 at 4 48 39 PM

So perhaps there's something in your job's state that's different than what I'm seeing, or our environments are otherwise different? Let's pair on Monday and figure it out.

const GroupByChildren = useCallback(
({ groupByRadioSelection, groupByDurationUnit, groupByDurationValue }) => (
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,10 @@ import {
TIMESTAMP,
} from '@kbn/rule-data-utils';
import { ALERT_ORIGINAL_TIME } from '@kbn/security-solution-plugin/common/field_maps/field_names';
import { DETECTION_ENGINE_SIGNALS_STATUS_URL as DETECTION_ENGINE_ALERTS_STATUS_URL } from '@kbn/security-solution-plugin/common/constants';
import {
DETECTION_ENGINE_SIGNALS_STATUS_URL as DETECTION_ENGINE_ALERTS_STATUS_URL,
ENABLE_ASSET_CRITICALITY_SETTING,
} from '@kbn/security-solution-plugin/common/constants';
import { EsArchivePathBuilder } from '../../../../../../es_archive_path_builder';
import { FtrProviderContext } from '../../../../../../ftr_provider_context';
import {
Expand Down Expand Up @@ -1102,6 +1105,63 @@ export default ({ getService }: FtrProviderContext) => {
});
});
});

describe('with enrichments', () => {
const kibanaServer = getService('kibanaServer');

before(async () => {
await esArchiver.load('x-pack/test/functional/es_archives/entity/risks');
await esArchiver.load('x-pack/test/functional/es_archives/asset_criticality');
await kibanaServer.uiSettings.update({
[ENABLE_ASSET_CRITICALITY_SETTING]: true,
});
});

after(async () => {
await esArchiver.unload('x-pack/test/functional/es_archives/entity/risks');
await esArchiver.unload('x-pack/test/functional/es_archives/asset_criticality');
});

beforeEach(async () => {
const timestamp = new Date().toISOString();
const anomalyWithKnownEntities = {
...baseAnomaly,
timestamp,
user: { name: 'root' },
host: { name: 'zeek-newyork-sha-aa8df15' },
};
await indexListOfDocuments([anomalyWithKnownEntities]);

ruleProps = {
...baseRuleProps,
from: timestamp,
alert_suppression: {
group_by: ['host.name'],
missing_fields_strategy: 'suppress',
},
};
});

it('should be enriched with host risk score', async () => {
const { previewId } = await previewRule({ supertest, rule: ruleProps });
const previewAlerts = await getPreviewAlerts({ es, previewId });
expect(previewAlerts).toHaveLength(1);
const alertSource = previewAlerts[0]._source;

expect(alertSource?.host?.risk?.calculated_level).toBe('Low');
expect(alertSource?.host?.risk?.calculated_score_norm).toBe(23);
});

it('should be enriched alert with criticality_level', async () => {
const { previewId } = await previewRule({ supertest, rule: ruleProps });
const previewAlerts = await getPreviewAlerts({ es, previewId });
expect(previewAlerts).toHaveLength(1);
const fullAlert = previewAlerts[0]._source;

expect(fullAlert?.['host.asset.criticality']).toBe('medium_impact');
expect(fullAlert?.['user.asset.criticality']).toBe('extreme_impact');
});
});
});
});
};