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][Platform] - Add connectors to import/export API #148703

Merged
Merged
Show file tree
Hide file tree
Changes from 47 commits
Commits
Show all changes
54 commits
Select commit Hold shift + click to select a range
ea2319e
use getImporter and getExporter from Saved Object
WafaaNasr Jan 10, 2023
dddba87
[CI] Auto-commit changed files from 'node scripts/ts_project_linter -…
kibanamachine Jan 11, 2023
1d5af44
Merge branch 'main' of https:/elastic/kibana into 118774-…
WafaaNasr Jan 11, 2023
2f5dcb4
resolve conflicts
WafaaNasr Jan 11, 2023
605ba89
fix tests
WafaaNasr Jan 11, 2023
c9a2656
fix get export by id
WafaaNasr Jan 11, 2023
9b2cb0e
[CI] Auto-commit changed files from 'node scripts/lint_ts_projects --…
kibanamachine Jan 11, 2023
f5a199b
comment warnings until decide if needed
WafaaNasr Jan 11, 2023
eb98ea3
Merge branch '118774-import-export-connectors-with-rules' of https://…
WafaaNasr Jan 11, 2023
0a00326
fix tests
WafaaNasr Jan 11, 2023
eca3cf3
enable Rule importing even if connectors are missing secrets
WafaaNasr Jan 17, 2023
3f56c94
merge with latest
WafaaNasr Jan 17, 2023
912b6c9
test: Import rules response schema
WafaaNasr Jan 17, 2023
d898d3b
rename validateRuleActionConnectors to skipActionConnectorsValidation…
WafaaNasr Jan 17, 2023
f69890b
remove unused var
WafaaNasr Jan 17, 2023
b9f4fb3
optional skipActionConnectorsValidations
WafaaNasr Jan 17, 2023
3b6106b
fix /rules/import_rules/route.test.ts
WafaaNasr Jan 17, 2023
2d7c903
fix condition
WafaaNasr Jan 17, 2023
67e8006
refactor closing Modal
WafaaNasr Jan 18, 2023
75c3c7f
fixing export e2e test
WafaaNasr Jan 18, 2023
a354bab
fix api integrations
WafaaNasr Jan 18, 2023
16795bd
fix import rules
WafaaNasr Jan 18, 2023
5e20363
implement overwriting of connectors
WafaaNasr Jan 19, 2023
f0b24d1
Merge branch 'main' of https:/elastic/kibana into 118774-…
WafaaNasr Jan 19, 2023
0d0b61e
fix tests
WafaaNasr Jan 19, 2023
f3e6161
adding error handling
WafaaNasr Jan 24, 2023
c177d1b
Merge branch 'main' of https:/elastic/kibana into 118774-…
WafaaNasr Jan 24, 2023
42e9ae8
apply ux feedback
WafaaNasr Jan 25, 2023
c2c9ef7
add tests for overwrite checkbox
WafaaNasr Jan 25, 2023
503aeb0
add export test
WafaaNasr Jan 26, 2023
e58dfe3
Merge branch 'main' of https:/elastic/kibana into 118774-…
WafaaNasr Jan 26, 2023
fb1a282
fixing read only access
WafaaNasr Jan 27, 2023
9d2aa0b
handle migration errors, overwrite rules, handle not importing connec…
WafaaNasr Jan 30, 2023
490ffb0
Merge branch 'main' into 118774-import-export-connectors-with-rules
kibanamachine Jan 30, 2023
b299057
fix test
WafaaNasr Jan 30, 2023
efbda93
add tests to import connectors and utils
WafaaNasr Jan 31, 2023
87b604d
add test for skipActionConnectorsValidations in update rules
WafaaNasr Jan 31, 2023
5d04232
refactor importing connectors
WafaaNasr Jan 31, 2023
2adf31d
apply comment and add tests
WafaaNasr Feb 1, 2023
816cfe5
[CI] Auto-commit changed files from 'node scripts/eslint --no-cache -…
kibanamachine Feb 1, 2023
31f5963
using warning message from the warning array and update the tests
WafaaNasr Feb 1, 2023
2f4d171
Merge branch '118774-import-export-connectors-with-rules' of https://…
WafaaNasr Feb 1, 2023
41bfb58
update tests description
WafaaNasr Feb 1, 2023
a417457
update overwrite message and snapshots
WafaaNasr Feb 1, 2023
67b4b2f
show success messages when importing successfully
WafaaNasr Feb 1, 2023
f192914
apply team repos
WafaaNasr Feb 1, 2023
7b2c13e
add tests for export by id
WafaaNasr Feb 1, 2023
680530f
apply docs team changes
WafaaNasr Feb 2, 2023
31bcd73
apply comments
WafaaNasr Feb 2, 2023
879ea94
Merge branch 'main' into 118774-import-export-connectors-with-rules
kibanamachine Feb 6, 2023
5098552
apply same ux as in the connectors page
WafaaNasr Feb 6, 2023
2da1121
merge with latest
WafaaNasr Feb 6, 2023
eb6dfe5
remove todos already addressed
WafaaNasr Feb 6, 2023
6dac771
Merge branch 'main' into 118774-import-export-connectors-with-rules
kibanamachine Feb 6, 2023
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 @@ -16,6 +16,7 @@ describe('importQuerySchema', () => {
as_new_list: false,
overwrite: true,
overwrite_exceptions: true,
overwrite_action_connectors: true,
};
const decoded = importQuerySchema.decode(payload);
const checked = exactCheck(payload, decoded);
Expand All @@ -30,6 +31,7 @@ describe('importQuerySchema', () => {
as_new_list: false,
overwrite: 'wrong',
overwrite_exceptions: true,
overwrite_action_connectors: true,
};
const decoded = importQuerySchema.decode(payload);
const checked = exactCheck(payload, decoded);
Expand All @@ -48,6 +50,7 @@ describe('importQuerySchema', () => {
as_new_list: false,
overwrite: true,
overwrite_exceptions: 'wrong',
overwrite_action_connectors: true,
};
const decoded = importQuerySchema.decode(payload);
const checked = exactCheck(payload, decoded);
Expand All @@ -58,6 +61,24 @@ describe('importQuerySchema', () => {
]);
expect(message.schema).toEqual({});
});
test('it should NOT validate a non boolean value for "overwrite_action_connectors"', () => {
const payload: Omit<ImportQuerySchema, 'overwrite_action_connectors'> & {
overwrite_action_connectors: string;
} = {
as_new_list: false,
overwrite: true,
overwrite_exceptions: true,
overwrite_action_connectors: 'wrong',
};
const decoded = importQuerySchema.decode(payload);
const checked = exactCheck(payload, decoded);
const message = foldLeftRight(checked);

expect(getPaths(left(message.errors))).toEqual([
'Invalid value "wrong" supplied to "overwrite_action_connectors"',
]);
expect(message.schema).toEqual({});
});

test('it should NOT allow an extra key to be sent in', () => {
const payload: ImportQuerySchema & {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,16 +14,18 @@ export const importQuerySchema = t.exact(
t.partial({
overwrite: DefaultStringBooleanFalse,
overwrite_exceptions: DefaultStringBooleanFalse,
overwrite_action_connectors: DefaultStringBooleanFalse,
as_new_list: DefaultStringBooleanFalse,
})
);

export type ImportQuerySchema = t.TypeOf<typeof importQuerySchema>;
export type ImportQuerySchemaDecoded = Omit<
ImportQuerySchema,
'overwrite' | 'overwrite_exceptions' | 'as_new_list'
'overwrite' | 'overwrite_exceptions' | 'as_new_list' | 'overwrite_action_connectors'
> & {
overwrite: boolean;
overwrite_exceptions: boolean;
overwrite_action_connectors: boolean;
as_new_list: boolean;
};
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,10 @@ import { parseDuration } from '../../lib';
export async function validateActions(
context: RulesClientContext,
alertType: UntypedNormalizedRuleType,
data: Pick<RawRule, 'notifyWhen' | 'throttle' | 'schedule'> & { actions: NormalizedAlertAction[] }
data: Pick<RawRule, 'notifyWhen' | 'throttle' | 'schedule'> & {
actions: NormalizedAlertAction[];
},
skipMissingSecretsValidation?: boolean
): Promise<void> {
const { actions, notifyWhen, throttle } = data;
const hasRuleLevelNotifyWhen = typeof notifyWhen !== 'undefined';
Expand All @@ -28,27 +31,28 @@ export async function validateActions(

const errors = [];

// check for actions using connectors with missing secrets
const actionsClient = await context.getActionsClient();
const actionIds = [...new Set(actions.map((action) => action.id))];
const actionResults = (await actionsClient.getBulk(actionIds)) || [];
const actionsUsingConnectorsWithMissingSecrets = actionResults.filter(
(result) => result.isMissingSecrets
);

if (actionsUsingConnectorsWithMissingSecrets.length) {
errors.push(
i18n.translate('xpack.alerting.rulesClient.validateActions.misconfiguredConnector', {
defaultMessage: 'Invalid connectors: {groups}',
values: {
groups: actionsUsingConnectorsWithMissingSecrets
.map((connector) => connector.name)
.join(', '),
},
})
if (!skipMissingSecretsValidation) {
// check for actions using connectors with missing secrets
const actionsClient = await context.getActionsClient();
const actionIds = [...new Set(actions.map((action) => action.id))];
const actionResults = (await actionsClient.getBulk(actionIds)) || [];
const actionsUsingConnectorsWithMissingSecrets = actionResults.filter(
(result) => result.isMissingSecrets
);
}

if (actionsUsingConnectorsWithMissingSecrets.length) {
errors.push(
i18n.translate('xpack.alerting.rulesClient.validateActions.misconfiguredConnector', {
defaultMessage: 'Invalid connectors: {groups}',
values: {
groups: actionsUsingConnectorsWithMissingSecrets
.map((connector) => connector.name)
.join(', '),
},
})
);
}
}
// check for actions with invalid action groups
const { actionGroups: alertTypeActionGroups } = alertType;
const usedAlertActionGroups = actions.map((action) => action.group);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -46,11 +46,12 @@ export interface CreateOptions<Params extends RuleTypeParams> {
| 'nextRun'
> & { actions: NormalizedAlertAction[] };
options?: SavedObjectOptions;
skipMissingSecretsValidation?: boolean;
}

export async function create<Params extends RuleTypeParams = never>(
context: RulesClientContext,
{ data, options }: CreateOptions<Params>
{ data, options, skipMissingSecretsValidation }: CreateOptions<Params>
): Promise<SanitizedRule<Params>> {
Copy link
Contributor

Choose a reason for hiding this comment

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

There were some changes merged in #145637 that I believe makes this skipActionConnectorsValidations unnecessary. We are now checking whether the rule type to create is a SIEM rule and stripping action level frequencies. Can you verify that you shouldn't need this skip param?

cc @Zacqary

Copy link
Contributor Author

@WafaaNasr WafaaNasr Jan 31, 2023

Choose a reason for hiding this comment

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

  • skipActionConnectorsValidations is used only when the user import rule(s), and is used because it skips the validation of the actions as it has been done already in the import API before calling the create fn.
  • We don't need the validation in the create method as well because we deal differently with the missing secrets warning message, we don't throw an exception, instead we continue the importing process by just showing a warning message in the UI

If you have another suggestion please let me know :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the explanation @WafaaNasr! Instead of bypassing the action validation altogether what do you think about a flag called allowMissingConnectorSecrets that is passed into the validateActions function. Inside that function, there is a check for connectors with missing secrets that will throw the validation error. Passing allowMissingConnectorSecrets=true to validateActions will log a warning but not throw the error? That way the rest of the action body can be correctly validated.

cc @XavierM to see if he has any suggestions for this.

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, @ymao1! Sure I see your point!
Maybe as you mentioned we can pass the allowMissingConnectorSecrets to the validateActions function
then add an if condition around line 31=> 50, check if allowMissingConnectorSecrets was true
to avoid getting the actions again, and loop over them to check for missing secrets.
https:/elastic/kibana/blob/main/x-pack/plugins/alerting/server/rules_client/lib/validate_actions.ts#L32

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ymao1 could you please validate if this f192914 will be a valid solution?

Copy link
Contributor

@XavierM XavierM Feb 2, 2023

Choose a reason for hiding this comment

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

I agree with @ymao1 and also like better the name allowMissingConnectorSecrets. We should log these errors because if we have SDH, it will be easy to understand why it happens and give some guidance to our users.
I will do something like below, really similar to what you did ;)

export async function validateActions(
 context: RulesClientContext,
 alertType: UntypedNormalizedRuleType,
 data: Pick<RawRule, 'notifyWhen' | 'throttle' | 'schedule'> & {
   actions: NormalizedAlertAction[];
 },
 logger: Logger,
 allowMissingConnectorSecrets?: boolean
): Promise<void> {
 const { actions, notifyWhen, throttle } = data;
 const hasRuleLevelNotifyWhen = typeof notifyWhen !== 'undefined';
 const hasRuleLevelThrottle = Boolean(throttle);
 if (actions.length === 0) {
   return;
 }

 const errors = [];

 // check for actions using connectors with missing secrets
 const actionsClient = await context.getActionsClient();
 const actionIds = [...new Set(actions.map((action) => action.id))];
 const actionResults = (await actionsClient.getBulk(actionIds)) || [];
 const actionsUsingConnectorsWithMissingSecrets = actionResults.filter(
   (result) => result.isMissingSecrets
 );

 if (actionsUsingConnectorsWithMissingSecrets.length) {
   if (allowMissingConnectorSecrets) {
     logger.error(
       `Invalid connectors with "allowMissingConnectorSecrets": ${actionsUsingConnectorsWithMissingSecrets
         .map((connector) => connector.name)
         .join(', ')}`
     );
   } else {
     errors.push(
       i18n.translate('xpack.alerting.rulesClient.validateActions.misconfiguredConnector', {
         defaultMessage: 'Invalid connectors: {groups}',
         values: {
           groups: actionsUsingConnectorsWithMissingSecrets
             .map((connector) => connector.name)
             .join(', '),
         },
       })
     );
   }
 }
 ...
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Awesome, thanks @XavierM thanks for the explanation and the suggested code,
Could you please validate

const id = options?.id || SavedObjectsUtils.generateId();

Expand Down Expand Up @@ -104,10 +105,11 @@ export async function create<Params extends RuleTypeParams = never>(
}
}

await validateActions(context, ruleType, data);
await validateActions(context, ruleType, data, skipMissingSecretsValidation);
await withSpan({ name: 'validateActions', type: 'rules' }, () =>
validateActions(context, ruleType, data)
validateActions(context, ruleType, data, skipMissingSecretsValidation)
);

// Throw error if schedule interval is less than the minimum and we are enforcing it
const intervalInMs = parseDuration(data.schedule.interval);
if (
Expand Down
17 changes: 11 additions & 6 deletions x-pack/plugins/alerting/server/rules_client/methods/update.ts
Original file line number Diff line number Diff line change
Expand Up @@ -38,22 +38,23 @@ export interface UpdateOptions<Params extends RuleTypeParams> {
throttle?: string | null;
notifyWhen?: RuleNotifyWhenType | null;
};
skipMissingSecretsValidation?: boolean;
}

export async function update<Params extends RuleTypeParams = never>(
context: RulesClientContext,
{ id, data }: UpdateOptions<Params>
{ id, data, skipMissingSecretsValidation }: UpdateOptions<Params>
): Promise<PartialRule<Params>> {
return await retryIfConflicts(
context.logger,
`rulesClient.update('${id}')`,
async () => await updateWithOCC<Params>(context, { id, data })
async () => await updateWithOCC<Params>(context, { id, data, skipMissingSecretsValidation })
);
}

async function updateWithOCC<Params extends RuleTypeParams>(
context: RulesClientContext,
{ id, data }: UpdateOptions<Params>
{ id, data, skipMissingSecretsValidation }: UpdateOptions<Params>
): Promise<PartialRule<Params>> {
let alertSavedObject: SavedObject<RawRule>;

Expand Down Expand Up @@ -99,7 +100,11 @@ async function updateWithOCC<Params extends RuleTypeParams>(

context.ruleTypeRegistry.ensureRuleTypeEnabled(alertSavedObject.attributes.alertTypeId);

const updateResult = await updateAlert<Params>(context, { id, data }, alertSavedObject);
const updateResult = await updateAlert<Params>(
context,
{ id, data, skipMissingSecretsValidation },
alertSavedObject
);

await Promise.all([
alertSavedObject.attributes.apiKey
Expand Down Expand Up @@ -138,7 +143,7 @@ async function updateWithOCC<Params extends RuleTypeParams>(

async function updateAlert<Params extends RuleTypeParams>(
context: RulesClientContext,
{ id, data }: UpdateOptions<Params>,
{ id, data, skipMissingSecretsValidation }: UpdateOptions<Params>,
{ attributes, version }: SavedObject<RawRule>
): Promise<PartialRule<Params>> {
const ruleType = context.ruleTypeRegistry.get(attributes.alertTypeId);
Expand All @@ -156,7 +161,7 @@ async function updateAlert<Params extends RuleTypeParams>(

// Validate
const validatedAlertTypeParams = validateRuleTypeParams(data.params, ruleType.validate?.params);
await validateActions(context, ruleType, data);
await validateActions(context, ruleType, data, skipMissingSecretsValidation);

// Throw error if schedule interval is less than the minimum and we are enforcing it
const intervalInMs = parseDuration(data.schedule.interval);
Expand Down
116 changes: 116 additions & 0 deletions x-pack/plugins/alerting/server/rules_client/tests/create.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3047,4 +3047,120 @@ describe('create()', () => {
expect(unsecuredSavedObjectsClient.create).not.toHaveBeenCalled();
expect(taskManager.schedule).not.toHaveBeenCalled();
});
test('should create a rule even if action is missing secret when skipMissingSecretsValidation is true', async () => {
const data = getMockData({
actions: [
{
group: 'default',
id: '1',
params: {
foo: true,
},
},
{
group: 'default',
id: '1',
params: {
foo: true,
},
},
{
group: 'default',
id: '2',
params: {
foo: true,
},
},
],
});
actionsClient.getBulk.mockReset();
actionsClient.getBulk.mockResolvedValue([
{
id: '1',
actionTypeId: '.slack',
config: {},
isMissingSecrets: true,
name: 'Slack connector',
isPreconfigured: false,
isDeprecated: false,
},
]);
unsecuredSavedObjectsClient.create.mockResolvedValueOnce({
id: '1',
type: 'alert',
attributes: {
alertTypeId: '123',
schedule: { interval: '1m' },
params: {
bar: true,
},
createdAt: new Date().toISOString(),
updatedAt: new Date().toISOString(),
notifyWhen: null,
actions: [
{
group: 'default',
actionRef: 'action_0',
actionTypeId: '.slack',
params: {
foo: true,
},
},
],
},
references: [
{
name: 'action_0',
type: 'action',
id: '1',
},
{
name: 'action_1',
type: 'action',
id: '1',
},
{
name: 'action_2',
type: 'action',
id: '2',
},
],
});
unsecuredSavedObjectsClient.create.mockResolvedValueOnce({
id: '1',
type: 'alert',
attributes: {
actions: [],
scheduledTaskId: 'task-123',
},
references: [],
});
const result = await rulesClient.create({ data, skipMissingSecretsValidation: true });
expect(result).toMatchInlineSnapshot(`
Object {
"actions": Array [
Object {
"actionTypeId": ".slack",
"group": "default",
"id": "1",
"params": Object {
"foo": true,
},
},
],
"alertTypeId": "123",
"createdAt": 2019-02-12T21:01:22.479Z,
"id": "1",
"notifyWhen": null,
"params": Object {
"bar": true,
},
"schedule": Object {
"interval": "1m",
},
"scheduledTaskId": "task-123",
"updatedAt": 2019-02-12T21:01:22.479Z,
}
`);
});
});
Loading