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

[Security Solution] Allow users to edit max_signals field for custom rules #179680

Merged
merged 62 commits into from
May 3, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
62 commits
Select commit Hold shift + click to select a range
15db4e4
exposes alerting config setting and creates form compoenent
dplumlee Mar 29, 2024
af60e77
Merge remote-tracking branch 'upstream/main' into max-signals-field-f…
dplumlee Apr 2, 2024
bb5a1d2
changes user-facing language to max alerts
dplumlee Apr 2, 2024
8015b84
adds tests
dplumlee Apr 2, 2024
264cdb6
updates language
dplumlee Apr 2, 2024
69dc936
adds param to one million mock constructors
dplumlee Apr 2, 2024
ca8b462
updates types
dplumlee Apr 2, 2024
d396a12
updates tests and mocks
dplumlee Apr 3, 2024
07927d1
adds type
dplumlee Apr 3, 2024
3522cb7
updates tests
dplumlee Apr 3, 2024
1a1c121
Merge remote-tracking branch 'upstream/main' into max-signals-field-f…
dplumlee Apr 9, 2024
0319c94
changes logic for max validations
dplumlee Apr 9, 2024
1749631
updates tests
dplumlee Apr 9, 2024
19ebcf0
adds warning state
dplumlee Apr 10, 2024
813c807
reset config value
dplumlee Apr 10, 2024
00e942a
Merge remote-tracking branch 'upstream/main' into max-signals-field-f…
dplumlee Apr 11, 2024
cd4eb47
adds max_signals rule execution logic
dplumlee Apr 12, 2024
5ad1b65
updates tests and types
dplumlee Apr 12, 2024
ff4b232
adds cypress tests
dplumlee Apr 12, 2024
2053068
Merge remote-tracking branch 'upstream/main' into max-signals-field-f…
dplumlee Apr 15, 2024
bec6bf7
updates test attributes
dplumlee Apr 15, 2024
aadde40
Merge remote-tracking branch 'upstream/main' into max-signals-field-f…
dplumlee Apr 15, 2024
34588b2
updates warning design
dplumlee Apr 16, 2024
61fbdc1
Merge remote-tracking branch 'upstream/main' into max-signals-field-f…
dplumlee Apr 16, 2024
7aaa62a
Merge remote-tracking branch 'upstream/main' into max-signals-field-f…
dplumlee Apr 16, 2024
3e749f6
updates language
dplumlee Apr 16, 2024
bd01d55
updates attribute
dplumlee Apr 16, 2024
aa2c565
Merge remote-tracking branch 'upstream/main' into max-signals-field-f…
dplumlee Apr 17, 2024
975490e
Merge remote-tracking branch 'upstream/main' into max-signals-field-f…
dplumlee Apr 22, 2024
25c05e3
addresses comments
dplumlee Apr 22, 2024
b47deff
Merge remote-tracking branch 'upstream/main' into max-signals-field-f…
dplumlee Apr 22, 2024
e709722
fixes execution logic
dplumlee Apr 22, 2024
9886913
strips out no longer needed rulesClient param
dplumlee Apr 22, 2024
89bd1e7
adds defaultable import export tests
dplumlee Apr 23, 2024
304db6f
Merge remote-tracking branch 'upstream/main' into max-signals-field-f…
dplumlee Apr 24, 2024
db88782
addresses comments
dplumlee Apr 24, 2024
dd0a87e
updates tests
dplumlee Apr 24, 2024
bedbd7d
Merge remote-tracking branch 'upstream/main' into max-signals-field-f…
dplumlee Apr 25, 2024
f400f99
changes defaulting logic one last time
dplumlee Apr 25, 2024
8add109
updates integration tests to match unified method
dplumlee Apr 25, 2024
e4e8af5
Merge remote-tracking branch 'upstream/main' into max-signals-field-f…
dplumlee Apr 26, 2024
54b8041
fixes spelling mistakes
dplumlee Apr 26, 2024
ad64d19
addresses response ops changes
dplumlee Apr 26, 2024
1cb02a2
fixes test
dplumlee Apr 27, 2024
259d52e
Merge remote-tracking branch 'upstream/main' into max-signals-field-f…
dplumlee Apr 29, 2024
bcca901
addresses comments
dplumlee Apr 30, 2024
346e10e
Merge remote-tracking branch 'upstream/main' into max-signals-field-f…
dplumlee Apr 30, 2024
ce2f06e
adds test
dplumlee Apr 30, 2024
940f925
Merge remote-tracking branch 'upstream/main' into max-signals-field-f…
dplumlee May 1, 2024
92d3ed2
Merge remote-tracking branch 'upstream/main' into max-signals-field-f…
dplumlee May 1, 2024
b1ce138
Merge remote-tracking branch 'upstream/main' into max-signals-field-f…
dplumlee May 2, 2024
995462b
Merge remote-tracking branch 'upstream/main' into max-signals-field-f…
dplumlee May 2, 2024
69f4b87
Merge remote-tracking branch 'upstream/main' into max-signals-field-f…
dplumlee May 2, 2024
dad1f8d
Merge remote-tracking branch 'upstream/main' into max-signals-field-f…
dplumlee May 2, 2024
9244392
Merge remote-tracking branch 'upstream/main' into max-signals-field-f…
dplumlee May 2, 2024
8d02476
Merge remote-tracking branch 'upstream/main' into max-signals-field-f…
dplumlee May 2, 2024
d0e0432
Merge remote-tracking branch 'upstream/main' into max-signals-field-f…
dplumlee May 2, 2024
18a724f
Merge branch 'main' into max-signals-field-form-component
jpdjere May 3, 2024
0b71b77
Fix handling of 0 in form
jpdjere May 3, 2024
07d81e6
Added tests for form validation
jpdjere May 3, 2024
c850892
Remove empty line
jpdjere May 3, 2024
3cbb10f
Merge branch 'main' into max-signals-field-form-component
jpdjere May 3, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -330,6 +330,7 @@ export default function ({ getService }: PluginFunctionalProviderContext) {
'xpack.stack_connectors.enableExperimental (array)',
'xpack.trigger_actions_ui.enableExperimental (array)',
'xpack.trigger_actions_ui.enableGeoTrackingThresholdAlert (boolean)',
'xpack.alerting.rules.run.alerts.max (number)',
'xpack.upgrade_assistant.featureSet.migrateSystemIndices (boolean)',
'xpack.upgrade_assistant.featureSet.mlSnapshots (boolean)',
'xpack.upgrade_assistant.featureSet.reindexCorrectiveActions (boolean)',
Expand Down
1 change: 1 addition & 0 deletions x-pack/plugins/alerting/public/mocks.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ const createSetupContract = (): Setup => ({

const createStartContract = (): Start => ({
getNavigation: jest.fn(),
getMaxAlertsPerRun: jest.fn(),
});

export const alertingPluginMock = {
Expand Down
14 changes: 12 additions & 2 deletions x-pack/plugins/alerting/public/plugin.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
* 2.0.
*/

import { AlertingPublicPlugin } from './plugin';
import { AlertingPublicPlugin, AlertingUIConfig } from './plugin';
import { coreMock } from '@kbn/core/public/mocks';
import {
createManagementSectionMock,
Expand All @@ -17,7 +17,17 @@ jest.mock('./services/rule_api', () => ({
loadRuleType: jest.fn(),
}));

const mockInitializerContext = coreMock.createPluginInitializerContext();
const mockAlertingUIConfig: AlertingUIConfig = {
rules: {
run: {
alerts: {
max: 1000,
},
},
},
};

const mockInitializerContext = coreMock.createPluginInitializerContext(mockAlertingUIConfig);
const management = managementPluginMock.createSetupContract();
const mockSection = createManagementSectionMock();

Expand Down
21 changes: 20 additions & 1 deletion x-pack/plugins/alerting/public/plugin.ts
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@ export interface PluginSetupContract {
}
export interface PluginStartContract {
getNavigation: (ruleId: Rule['id']) => Promise<string | undefined>;
getMaxAlertsPerRun: () => number;
}
export interface AlertingPluginSetup {
management: ManagementSetup;
Expand All @@ -69,13 +70,28 @@ export interface AlertingPluginStart {
data: DataPublicPluginStart;
}

export interface AlertingUIConfig {
rules: {
run: {
alerts: {
max: number;
};
};
};
}

export class AlertingPublicPlugin
implements
Plugin<PluginSetupContract, PluginStartContract, AlertingPluginSetup, AlertingPluginStart>
{
private alertNavigationRegistry?: AlertNavigationRegistry;
private config: AlertingUIConfig;
readonly maxAlertsPerRun: number;

constructor(private readonly initContext: PluginInitializerContext) {}
constructor(private readonly initContext: PluginInitializerContext) {
this.config = this.initContext.config.get<AlertingUIConfig>();
this.maxAlertsPerRun = this.config.rules.run.alerts.max;
}

public setup(core: CoreSetup, plugins: AlertingPluginSetup) {
this.alertNavigationRegistry = new AlertNavigationRegistry();
Expand Down Expand Up @@ -150,6 +166,9 @@ export class AlertingPublicPlugin
return rule.viewInAppRelativeUrl;
}
},
getMaxAlertsPerRun: () => {
return this.maxAlertsPerRun;
},
};
}
}
2 changes: 1 addition & 1 deletion x-pack/plugins/alerting/server/config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,7 @@ export type AlertingConfig = TypeOf<typeof configSchema>;
export type RulesConfig = TypeOf<typeof rulesSchema>;
export type AlertingRulesConfig = Pick<
AlertingConfig['rules'],
'minimumScheduleInterval' | 'maxScheduledPerMinute'
'minimumScheduleInterval' | 'maxScheduledPerMinute' | 'run'
> & {
isUsingSecurity: boolean;
};
Expand Down
8 changes: 5 additions & 3 deletions x-pack/plugins/alerting/server/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,7 @@
import type { PublicMethodsOf } from '@kbn/utility-types';
import { PluginConfigDescriptor, PluginInitializerContext } from '@kbn/core/server';
import { RulesClient as RulesClientClass } from './rules_client';
import { configSchema } from './config';
import { AlertsConfigType } from './types';
import { AlertingConfig, configSchema } from './config';

export type RulesClient = PublicMethodsOf<RulesClientClass>;

Expand Down Expand Up @@ -79,8 +78,11 @@ export const plugin = async (initContext: PluginInitializerContext) => {
return new AlertingPlugin(initContext);
};

export const config: PluginConfigDescriptor<AlertsConfigType> = {
export const config: PluginConfigDescriptor<AlertingConfig> = {
schema: configSchema,
exposeToBrowser: {
Copy link
Member

Choose a reason for hiding this comment

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

If we use exposeToBrowser, then in theory we don't need this added to the triggers_actions_ui config route. Though exposeToBrowser only makes it available to the browsers alerting plugin, not t_a_ui - though obviously we could make it accessible.

But I don't think we need both, so we should pick one and not do the other. We don't need two ways to do the same thing ...

Or perhaps I missed something ...

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'm not sure if I understand, the only triggers_actions_ui code that has been modified here was some test files to align with the mock alerting plugin. But to your larger point, I agree - the exposeToBrowser has been implemented here and other plugins can use them if they so choose. That's how we're utilizing it in security solution

Copy link
Member

Choose a reason for hiding this comment

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

The way this is structured we are also changing the output of the HTTP endpoint /internal/triggers_actions_ui/_config - to return ALL the values under run ,not just the alerts.max value -
which is something we need to consider re: backwards compatibility, documentation, and security (should we be exposing these via API). Do we really need to change the output of this endpoint?

$ curl $KB_URL/internal/triggers_actions_ui/_config
{"minimumScheduleInterval":{"value":"1s","enforce":false},"maxScheduledPerMinute":10000,"run":{"actions":{"max":100000},"alerts":{"max":1000}},"isUsingSecurity":true}

I believe it's also the case that the values returned by this endpoint are or can be calculated, so using the value from the config wouldn't work. Which is why we have this endpoint. Static values (I assume these are static) can just use the exposeToBrowser path.

Copy link
Member

Choose a reason for hiding this comment

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

In the end, I would like to NOT change the output of the _config route (that's adding the run props) - not so much because it's a security concern now, but feels like a slippery slope to having a problem later, if someone follows this pattern, and we do leak something we shouldn't.

Feels like we need to remove run from AlertingRulesConfig, or change the _config route in t_a_ui to pick the fields it should be returning from the bigger config object (the existing ones, but not run).

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 think the second way is probably the more preferable method, just so we can use that getConfig function elsewhere in other plugins without changing the _config route output. Right now we're using the getConfig function to compare on the server side in security solution as well which is why the run props were added in the first place. If y'all are ok with exposing the config values under run to that internal getConfig method from the plugin setup object and modifying the triggers_actions_ui config route so that we're locked into the existing values, I can change that over.

Copy link
Member

Choose a reason for hiding this comment

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

I don't have a problem exposting this at the plugin level - my concern is at the HTTP response level.

Filtering IN just the props we want returned from that _config endpoint would be perfect, as it means we won't have to worry about accidently leaking things later. So, in theory, x-pack/plugins/triggers_actions_ui/server/routes/config.test.ts won't have any changes, but presumably it's pair config.ts will.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok that all sounds good, I think we're on the same page 👍

rules: { run: { alerts: { max: true } } },
},
deprecations: ({ renameFromRoot, deprecate }) => [
renameFromRoot('xpack.alerts.healthCheck', 'xpack.alerting.healthCheck', { level: 'warning' }),
renameFromRoot(
Expand Down
1 change: 1 addition & 0 deletions x-pack/plugins/alerting/server/plugin.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -163,6 +163,7 @@ describe('Alerting Plugin', () => {
maxScheduledPerMinute: 10000,
isUsingSecurity: false,
minimumScheduleInterval: { value: '1m', enforce: false },
run: { alerts: { max: 1000 }, actions: { max: 1000 } },
});

expect(setupContract.frameworkAlerts.enabled()).toEqual(false);
Expand Down
2 changes: 1 addition & 1 deletion x-pack/plugins/alerting/server/plugin.ts
Original file line number Diff line number Diff line change
Expand Up @@ -458,7 +458,7 @@ export class AlertingPlugin {
},
getConfig: () => {
return {
...pick(this.config.rules, ['minimumScheduleInterval', 'maxScheduledPerMinute']),
...pick(this.config.rules, ['minimumScheduleInterval', 'maxScheduledPerMinute', 'run']),
isUsingSecurity: this.licenseState ? !!this.licenseState.getIsSecurityEnabled() : false,
};
},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -123,7 +123,6 @@ components:
references:
$ref: './common_attributes.schema.yaml#/components/schemas/RuleReferenceArray'

# maxSignals not used in ML rules but probably should be used
max_signals:
$ref: './common_attributes.schema.yaml#/components/schemas/MaxSignals'
threat:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,7 @@ import { fieldFormatsMock } from '@kbn/field-formats-plugin/common/mocks';
import { indexPatternFieldEditorPluginMock } from '@kbn/data-view-field-editor-plugin/public/mocks';
import { UpsellingService } from '@kbn/security-solution-upselling/service';
import { calculateBounds } from '@kbn/data-plugin/common';
import { alertingPluginMock } from '@kbn/alerting-plugin/public/mocks';

const mockUiSettings: Record<string, unknown> = {
[DEFAULT_TIME_RANGE]: { from: 'now-15m', to: 'now', mode: 'quick' },
Expand Down Expand Up @@ -128,6 +129,7 @@ export const createStartServicesMock = (
const cloud = cloudMock.createStart();
const mockSetHeaderActionMenu = jest.fn();
const mockTimelineFilterManager = createFilterManagerMock();
const alerting = alertingPluginMock.createStartContract();

/*
* Below mocks are needed by unified field list
Expand Down Expand Up @@ -250,6 +252,7 @@ export const createStartServicesMock = (
dataViewFieldEditor: indexPatternFieldEditorPluginMock.createStartContract(),
upselling: new UpsellingService(),
timelineFilterManager: mockTimelineFilterManager,
alerting,
} as unknown as StartServices;
};

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -263,7 +263,7 @@ describe('description_step', () => {
mockLicenseService
);

expect(result.length).toEqual(13);
expect(result.length).toEqual(14);
});
});

Expand Down Expand Up @@ -768,6 +768,33 @@ describe('description_step', () => {
});
});
});

describe('maxSignals', () => {
test('returns default "max signals" description', () => {
const result: ListItems[] = getDescriptionItem(
'maxSignals',
'Max alerts per run',
mockAboutStep,
mockFilterManager,
mockLicenseService
);

expect(result[0].title).toEqual('Max alerts per run');
expect(result[0].description).toEqual(100);
});

test('returns empty array when "value" is a undefined', () => {
const result: ListItems[] = getDescriptionItem(
'maxSignals',
'Max alerts per run',
{ ...mockAboutStep, maxSignals: undefined },
mockFilterManager,
mockLicenseService
);

expect(result.length).toEqual(0);
});
});
});
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -342,6 +342,9 @@ export const getDescriptionItem = (
return get('isBuildingBlock', data)
? [{ title: i18n.BUILDING_BLOCK_LABEL, description: i18n.BUILDING_BLOCK_DESCRIPTION }]
: [];
} else if (field === 'maxSignals') {
const value: number | undefined = get(field, data);
return value ? [{ title: label, description: value }] : [];
}

const description: string = get(field, data);
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,99 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License
* 2.0; you may not use this file except in compliance with the Elastic License
* 2.0.
*/

import React, { useMemo, useCallback } from 'react';
import type { EuiFieldNumberProps } from '@elastic/eui';
import { EuiTextColor, EuiFormRow, EuiFieldNumber, EuiIcon } from '@elastic/eui';
import type { FieldHook } from '@kbn/es-ui-shared-plugin/static/forms/hook_form_lib';
import { css } from '@emotion/css';
import { DEFAULT_MAX_SIGNALS } from '../../../../../common/constants';
import * as i18n from './translations';
import { useKibana } from '../../../../common/lib/kibana';

interface MaxSignalsFieldProps {
dataTestSubj: string;
field: FieldHook<number | ''>;
idAria: string;
isDisabled: boolean;
placeholder?: string;
}

const MAX_SIGNALS_FIELD_WIDTH = 200;

export const MaxSignals: React.FC<MaxSignalsFieldProps> = ({
dataTestSubj,
field,
idAria,
isDisabled,
placeholder,
}): JSX.Element => {
const { setValue, value } = field;
const { alerting } = useKibana().services;
const maxAlertsPerRun = alerting.getMaxAlertsPerRun();

const [isInvalid, error] = useMemo(() => {
if (typeof value === 'number' && !isNaN(value) && value <= 0) {
return [true, i18n.GREATER_THAN_ERROR];
}
return [false];
}, [value]);

const hasWarning = useMemo(
() => typeof value === 'number' && !isNaN(value) && value > maxAlertsPerRun,
[maxAlertsPerRun, value]
);

const handleMaxSignalsChange: EuiFieldNumberProps['onChange'] = useCallback(
(e) => {
const maxSignalsValue = (e.target as HTMLInputElement).value;
// Has to handle an empty string as the field is optional
setValue(maxSignalsValue !== '' ? Number(maxSignalsValue.trim()) : '');
},
[setValue]
);

const helpText = useMemo(() => {
const textToRender = [];
if (hasWarning) {
textToRender.push(
<EuiTextColor color="warning">{i18n.LESS_THAN_WARNING(maxAlertsPerRun)}</EuiTextColor>
);
}
textToRender.push(i18n.MAX_SIGNALS_HELP_TEXT(DEFAULT_MAX_SIGNALS));
return textToRender;
}, [hasWarning, maxAlertsPerRun]);

return (
<EuiFormRow
css={css`
.euiFormControlLayout {
width: ${MAX_SIGNALS_FIELD_WIDTH}px;
}
`}
describedByIds={idAria ? [idAria] : undefined}
fullWidth
helpText={helpText}
label={field.label}
labelAppend={field.labelAppend}
isInvalid={isInvalid}
error={error}
>
<EuiFieldNumber
isInvalid={isInvalid}
value={value as EuiFieldNumberProps['value']}
onChange={handleMaxSignalsChange}
isLoading={field.isValidating}
placeholder={placeholder}
data-test-subj={dataTestSubj}
disabled={isDisabled}
append={hasWarning ? <EuiIcon size="s" type="warning" color="warning" /> : undefined}
/>
</EuiFormRow>
);
};

MaxSignals.displayName = 'MaxSignals';
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License
* 2.0; you may not use this file except in compliance with the Elastic License
* 2.0.
*/

import { i18n } from '@kbn/i18n';

export const GREATER_THAN_ERROR = i18n.translate(
'xpack.securitySolution.detectionEngine.createRule.stepAboutRule.maxAlertsFieldGreaterThanError',
{
defaultMessage: 'Max alerts must be greater than 0.',
}
);

export const LESS_THAN_WARNING = (maxNumber: number) =>
i18n.translate(
'xpack.securitySolution.detectionEngine.createRule.stepAboutRule.maxAlertsFieldLessThanWarning',
{
values: { maxNumber },
defaultMessage:
'Kibana only allows a maximum of {maxNumber} {maxNumber, plural, =1 {alert} other {alerts}} per rule run.',
}
);

export const MAX_SIGNALS_HELP_TEXT = (defaultNumber: number) =>
i18n.translate(
'xpack.securitySolution.detectionEngine.createRule.stepAboutRule.fieldMaxAlertsHelpText',
{
values: { defaultNumber },
defaultMessage:
'The maximum number of alerts the rule will create each time it runs. Default is {defaultNumber}.',
}
);
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
* 2.0.
*/

import { DEFAULT_MAX_SIGNALS } from '../../../../../common/constants';
import type { AboutStepRule } from '../../../../detections/pages/detection_engine/rules/types';
import { fillEmptySeverityMappings } from '../../../../detections/pages/detection_engine/rules/helpers';

Expand Down Expand Up @@ -33,5 +34,6 @@ export const stepAboutDefaultValue: AboutStepRule = {
timestampOverride: '',
threat: threatDefault,
note: '',
maxSignals: DEFAULT_MAX_SIGNALS,
setup: '',
};
Loading