From f7a4801c8850e95fa09cc3aa052cab64b125b3e6 Mon Sep 17 00:00:00 2001 From: Patrick Mueller Date: Tue, 5 May 2020 19:06:54 -0400 Subject: [PATCH 1/2] [Alerting] changes preconfigured actions config from array to object resolves https://github.com/elastic/kibana/issues/63171 Previously, preconfigured actions were specified as an array of action properties. This ended up being problematic when using the kibana keystore for secrets, as you'd have to reference specific actions via index. This changes preconfigured actions to be specified as an object, where the property key is the id, and the body is the remainder of the action properties. As access to preconfigured actions has leaked across the code base, it's probably time to consider changing the internal representation from an array to a Map, to provide easier access by action id. For a future PR. --- x-pack/plugins/actions/README.md | 2 +- x-pack/plugins/actions/server/config.test.ts | 53 ++++++++++++++--- x-pack/plugins/actions/server/config.ts | 39 +++++++++---- x-pack/plugins/actions/server/plugin.test.ts | 57 +++++++++---------- x-pack/plugins/actions/server/plugin.ts | 14 +++-- .../alerting_api_integration/common/config.ts | 16 ++---- x-pack/test/functional_with_es_ssl/config.ts | 10 ++-- 7 files changed, 118 insertions(+), 73 deletions(-) diff --git a/x-pack/plugins/actions/README.md b/x-pack/plugins/actions/README.md index 4c8cc3aa503e6..54624b94e0de3 100644 --- a/x-pack/plugins/actions/README.md +++ b/x-pack/plugins/actions/README.md @@ -98,7 +98,7 @@ Built-In-Actions are configured using the _xpack.actions_ namespoace under _kiba | _xpack.actions._**enabled** | Feature toggle which enabled Actions in Kibana. | boolean | | _xpack.actions._**whitelistedHosts** | Which _hostnames_ are whitelisted for the Built-In-Action? This list should contain hostnames of every external service you wish to interact with using Webhooks, Email or any other built in Action. Note that you may use the string "\*" in place of a specific hostname to enable Kibana to target any URL, but keep in mind the potential use of such a feature to execute [SSRF](https://www.owasp.org/index.php/Server_Side_Request_Forgery) attacks from your server. | Array | | _xpack.actions._**enabledActionTypes** | A list of _actionTypes_ id's that are enabled. A "\*" may be used as an element to indicate all registered actionTypes should be enabled. The actionTypes registered for Kibana are `.server-log`, `.slack`, `.email`, `.index`, `.pagerduty`, `.webhook`. Default: `["*"]` | Array | -| _xpack.actions._**preconfigured** | A list of preconfigured actions. Default: `[]` | Array | +| _xpack.actions._**preconfigured** | A object of action id / preconfigured actions. Default: `{}` | Array | #### Whitelisting Built-in Action Types diff --git a/x-pack/plugins/actions/server/config.test.ts b/x-pack/plugins/actions/server/config.test.ts index 161a6c31d4e59..e86f2d7832828 100644 --- a/x-pack/plugins/actions/server/config.test.ts +++ b/x-pack/plugins/actions/server/config.test.ts @@ -14,7 +14,7 @@ describe('config validation', () => { "enabledActionTypes": Array [ "*", ], - "preconfigured": Array [], + "preconfigured": Object {}, "whitelistedHosts": Array [ "*", ], @@ -24,16 +24,15 @@ describe('config validation', () => { test('action with preconfigured actions', () => { const config: Record = { - preconfigured: [ - { - id: 'my-slack1', + preconfigured: { + mySlack1: { actionTypeId: '.slack', name: 'Slack #xyz', config: { webhookUrl: 'https://hooks.slack.com/services/abcd/efgh/ijklmnopqrstuvwxyz', }, }, - ], + }, }; expect(configSchema.validate(config)).toMatchInlineSnapshot(` Object { @@ -41,21 +40,57 @@ describe('config validation', () => { "enabledActionTypes": Array [ "*", ], - "preconfigured": Array [ - Object { + "preconfigured": Object { + "mySlack1": Object { "actionTypeId": ".slack", "config": Object { "webhookUrl": "https://hooks.slack.com/services/abcd/efgh/ijklmnopqrstuvwxyz", }, - "id": "my-slack1", "name": "Slack #xyz", "secrets": Object {}, }, - ], + }, "whitelistedHosts": Array [ "*", ], } `); }); + + test('validates preconfigured action ids', () => { + expect(() => + configSchema.validate(preConfiguredActionConfig('')) + ).toThrowErrorMatchingInlineSnapshot( + `"[preconfigured]: invalid preconfigured action id \\"\\""` + ); + + expect(() => + configSchema.validate(preConfiguredActionConfig('constructor')) + ).toThrowErrorMatchingInlineSnapshot( + `"[preconfigured]: invalid preconfigured action id \\"constructor\\""` + ); + + expect(() => + configSchema.validate(preConfiguredActionConfig('__proto__')) + ).toThrowErrorMatchingInlineSnapshot( + `"[preconfigured]: invalid preconfigured action id \\"__proto__\\""` + ); + }); }); + +// object creator that ensures we can create a property named __proto__ on an +// object, via JSON.parse() +function preConfiguredActionConfig(id: string) { + return JSON.parse(`{ + "preconfigured": { + ${JSON.stringify(id)}: { + "actionTypeId": ".server-log", + "name": "server log 1" + }, + "serverLog": { + "actionTypeId": ".server-log", + "name": "server log 2" + } + } + }`); +} diff --git a/x-pack/plugins/actions/server/config.ts b/x-pack/plugins/actions/server/config.ts index 1f04efd1941b4..b2f3fa2680a9c 100644 --- a/x-pack/plugins/actions/server/config.ts +++ b/x-pack/plugins/actions/server/config.ts @@ -7,6 +7,13 @@ import { schema, TypeOf } from '@kbn/config-schema'; import { WhitelistedHosts, EnabledActionTypes } from './actions_config'; +const preconfiguredActionSchema = schema.object({ + name: schema.string({ minLength: 1 }), + actionTypeId: schema.string({ minLength: 1 }), + config: schema.recordOf(schema.string(), schema.any(), { defaultValue: {} }), + secrets: schema.recordOf(schema.string(), schema.any(), { defaultValue: {} }), +}); + export const configSchema = schema.object({ enabled: schema.boolean({ defaultValue: true }), whitelistedHosts: schema.arrayOf( @@ -21,18 +28,26 @@ export const configSchema = schema.object({ defaultValue: [WhitelistedHosts.Any], } ), - preconfigured: schema.arrayOf( - schema.object({ - id: schema.string({ minLength: 1 }), - name: schema.string(), - actionTypeId: schema.string({ minLength: 1 }), - config: schema.recordOf(schema.string(), schema.any(), { defaultValue: {} }), - secrets: schema.recordOf(schema.string(), schema.any(), { defaultValue: {} }), - }), - { - defaultValue: [], - } - ), + preconfigured: schema.recordOf(schema.string(), preconfiguredActionSchema, { + defaultValue: {}, + validate: validatePreconfigured, + }), }); export type ActionsConfig = TypeOf; + +const invalidActionIds = new Set(['', '__proto__', 'constructor']); + +function validatePreconfigured(preconfigured: Record): string | undefined { + // check for ids that should not be used + for (const id of Object.keys(preconfigured)) { + if (invalidActionIds.has(id)) { + return `invalid preconfigured action id "${id}"`; + } + } + + // in case __proto__ was used as a preconfigured action id ... + if (Object.getPrototypeOf(preconfigured) !== Object.getPrototypeOf({})) { + return `invalid preconfigured action id "__proto__"`; + } +} diff --git a/x-pack/plugins/actions/server/plugin.test.ts b/x-pack/plugins/actions/server/plugin.test.ts index 2b334953063d1..8673d992ada98 100644 --- a/x-pack/plugins/actions/server/plugin.test.ts +++ b/x-pack/plugins/actions/server/plugin.test.ts @@ -12,6 +12,7 @@ import { taskManagerMock } from '../../task_manager/server/mocks'; import { eventLogMock } from '../../event_log/server/mocks'; import { UsageCollectionSetup } from 'src/plugins/usage_collection/server'; import { ActionType } from './types'; +import { ActionsConfig } from './config'; import { ActionsPlugin, ActionsPluginsSetup, @@ -31,33 +32,11 @@ describe('Actions Plugin', () => { let pluginsSetup: jest.Mocked; beforeEach(() => { - context = coreMock.createPluginInitializerContext({ - preconfigured: [ - { - id: 'my-slack1', - actionTypeId: '.slack', - name: 'Slack #xyz', - description: 'Send a message to the #xyz channel', - config: { - webhookUrl: 'https://hooks.slack.com/services/abcd/efgh/ijklmnopqrstuvwxyz', - }, - }, - { - id: 'custom-system-abc-connector', - actionTypeId: 'system-abc-action-type', - description: 'Send a notification to system ABC', - name: 'System ABC', - config: { - xyzConfig1: 'value1', - xyzConfig2: 'value2', - listOfThings: ['a', 'b', 'c', 'd'], - }, - secrets: { - xyzSecret1: 'credential1', - xyzSecret2: 'credential2', - }, - }, - ], + context = coreMock.createPluginInitializerContext({ + enabled: true, + enabledActionTypes: ['*'], + whitelistedHosts: ['*'], + preconfigured: {}, }); plugin = new ActionsPlugin(context); coreSetup = coreMock.createSetup(); @@ -192,6 +171,7 @@ describe('Actions Plugin', () => { }); }); }); + describe('start()', () => { let plugin: ActionsPlugin; let coreSetup: ReturnType; @@ -200,8 +180,18 @@ describe('Actions Plugin', () => { let pluginsStart: jest.Mocked; beforeEach(() => { - const context = coreMock.createPluginInitializerContext({ - preconfigured: [], + const context = coreMock.createPluginInitializerContext({ + enabled: true, + enabledActionTypes: ['*'], + whitelistedHosts: ['*'], + preconfigured: { + preconfiguredServerLog: { + actionTypeId: '.server-log', + name: 'preconfigured-server-log', + config: {}, + secrets: {}, + }, + }, }); plugin = new ActionsPlugin(context); coreSetup = coreMock.createSetup(); @@ -220,6 +210,15 @@ describe('Actions Plugin', () => { }); describe('getActionsClientWithRequest()', () => { + it('should handle preconfigured actions', async () => { + // coreMock.createSetup doesn't support Plugin generics + // eslint-disable-next-line @typescript-eslint/no-explicit-any + await plugin.setup(coreSetup as any, pluginsSetup); + const pluginStart = plugin.start(coreStart, pluginsStart); + + expect(pluginStart.isActionExecutable('preconfiguredServerLog', '.server-log')).toBe(true); + }); + it('should not throw error when ESO plugin not using a generated key', async () => { // coreMock.createSetup doesn't support Plugin generics // eslint-disable-next-line @typescript-eslint/no-explicit-any diff --git a/x-pack/plugins/actions/server/plugin.ts b/x-pack/plugins/actions/server/plugin.ts index f14df794bbf47..bc7440c8bee4d 100644 --- a/x-pack/plugins/actions/server/plugin.ts +++ b/x-pack/plugins/actions/server/plugin.ts @@ -150,12 +150,14 @@ export class ActionsPlugin implements Plugin, Plugi const actionsConfig = (await this.config) as ActionsConfig; const actionsConfigUtils = getActionsConfigurationUtilities(actionsConfig); - this.preconfiguredActions.push( - ...actionsConfig.preconfigured.map( - preconfiguredAction => - ({ ...preconfiguredAction, isPreconfigured: true } as PreConfiguredAction) - ) - ); + for (const preconfiguredId of Object.keys(actionsConfig.preconfigured)) { + this.preconfiguredActions.push({ + ...actionsConfig.preconfigured[preconfiguredId], + id: preconfiguredId, + isPreconfigured: true, + }); + } + const actionTypeRegistry = new ActionTypeRegistry({ taskRunnerFactory, taskManager: plugins.taskManager, diff --git a/x-pack/test/alerting_api_integration/common/config.ts b/x-pack/test/alerting_api_integration/common/config.ts index d6b5b40d99d99..5fa87446e0f43 100644 --- a/x-pack/test/alerting_api_integration/common/config.ts +++ b/x-pack/test/alerting_api_integration/common/config.ts @@ -83,17 +83,15 @@ export function createTestConfig(name: string, options: CreateTestConfigOptions) ])}`, `--xpack.actions.enabledActionTypes=${JSON.stringify(enabledActionTypes)}`, '--xpack.eventLog.logEntries=true', - `--xpack.actions.preconfigured=${JSON.stringify([ - { - id: 'my-slack1', + `--xpack.actions.preconfigured=${JSON.stringify({ + 'my-slack1': { actionTypeId: '.slack', name: 'Slack#xyz', config: { webhookUrl: 'https://hooks.slack.com/services/abcd/efgh/ijklmnopqrstuvwxyz', }, }, - { - id: 'custom-system-abc-connector', + 'custom-system-abc-connector': { actionTypeId: 'system-abc-action-type', name: 'SystemABC', config: { @@ -106,8 +104,7 @@ export function createTestConfig(name: string, options: CreateTestConfigOptions) xyzSecret2: 'credential2', }, }, - { - id: 'preconfigured-es-index-action', + 'preconfigured-es-index-action': { actionTypeId: '.index', name: 'preconfigured_es_index_action', config: { @@ -116,8 +113,7 @@ export function createTestConfig(name: string, options: CreateTestConfigOptions) executionTimeField: 'timestamp', }, }, - { - id: 'preconfigured.test.index-record', + 'preconfigured.test.index-record': { actionTypeId: 'test.index-record', name: 'Test:_Preconfigured_Index_Record', config: { @@ -127,7 +123,7 @@ export function createTestConfig(name: string, options: CreateTestConfigOptions) encrypted: 'this-is-also-ignored-and-also-required', }, }, - ])}`, + })}`, ...disabledPlugins.map(key => `--xpack.${key}.enabled=false`), ...plugins.map( pluginDir => diff --git a/x-pack/test/functional_with_es_ssl/config.ts b/x-pack/test/functional_with_es_ssl/config.ts index ef2270fb97745..50de76d67e06b 100644 --- a/x-pack/test/functional_with_es_ssl/config.ts +++ b/x-pack/test/functional_with_es_ssl/config.ts @@ -66,21 +66,19 @@ export default async function({ readConfigFile }: FtrConfigProviderContext) { `--elasticsearch.ssl.certificateAuthorities=${CA_CERT_PATH}`, `--plugin-path=${join(__dirname, 'fixtures', 'plugins', 'alerts')}`, `--xpack.actions.enabledActionTypes=${JSON.stringify(enabledActionTypes)}`, - `--xpack.actions.preconfigured=${JSON.stringify([ - { - id: 'my-slack1', + `--xpack.actions.preconfigured=${JSON.stringify({ + 'my-slack1': { actionTypeId: '.slack', name: 'Slack#xyztest', config: { webhookUrl: 'https://hooks.slack.com/services/abcd/efgh/ijklmnopqrstuvwxyz', }, }, - { - id: 'my-server-log', + 'my-server-log': { actionTypeId: '.server-log', name: 'Serverlog#xyz', }, - ])}`, + })}`, ], }, }; From 7134f4f70ba9562e4f750e22f32abe7ace3e9930 Mon Sep 17 00:00:00 2001 From: Patrick Mueller Date: Thu, 7 May 2020 11:17:12 -0400 Subject: [PATCH 2/2] update docs --- .../user/alerting/action-types/email.asciidoc | 22 +++++++++---------- .../user/alerting/action-types/index.asciidoc | 14 ++++++------ .../alerting/action-types/pagerduty.asciidoc | 14 ++++++------ .../alerting/action-types/server-log.asciidoc | 6 ++--- .../user/alerting/action-types/slack.asciidoc | 10 ++++----- .../alerting/action-types/webhook.asciidoc | 22 +++++++++---------- .../pre-configured-connectors.asciidoc | 8 +++---- 7 files changed, 48 insertions(+), 48 deletions(-) diff --git a/docs/user/alerting/action-types/email.asciidoc b/docs/user/alerting/action-types/email.asciidoc index 689d870d9cadc..81b4e210961f6 100644 --- a/docs/user/alerting/action-types/email.asciidoc +++ b/docs/user/alerting/action-types/email.asciidoc @@ -24,17 +24,17 @@ Password:: password for 'login' type authentication. [source,text] -- - id: 'my-email' - name: preconfigured-email-action-type - actionTypeId: .email - config: - from: testsender@test.com <1.1> - host: validhostname <1.2> - port: 8080 <1.3> - secure: false <1.4> - secrets: - user: testuser <2.1> - password: passwordkeystorevalue <2.2> + my-email: + name: preconfigured-email-action-type + actionTypeId: .email + config: + from: testsender@test.com <1.1> + host: validhostname <1.2> + port: 8080 <1.3> + secure: false <1.4> + secrets: + user: testuser <2.1> + password: passwordkeystorevalue <2.2> -- `config` defines the action type specific to the configuration and contains the following properties: diff --git a/docs/user/alerting/action-types/index.asciidoc b/docs/user/alerting/action-types/index.asciidoc index 4f5254e3311d8..c71412210c535 100644 --- a/docs/user/alerting/action-types/index.asciidoc +++ b/docs/user/alerting/action-types/index.asciidoc @@ -21,13 +21,13 @@ Execution time field:: This field will be automatically set to the time the ale [source,text] -- - id: 'my-index' - name: action-type-index - actionTypeId: .index - config: - index: .kibana <1> - refresh: true <2> - executionTimeField: somedate <3> + my-index: + name: action-type-index + actionTypeId: .index + config: + index: .kibana <1> + refresh: true <2> + executionTimeField: somedate <3> -- `config` defines the action type specific to the configuration and contains the following properties: diff --git a/docs/user/alerting/action-types/pagerduty.asciidoc b/docs/user/alerting/action-types/pagerduty.asciidoc index 957c035b028f6..cd51ec2e3301e 100644 --- a/docs/user/alerting/action-types/pagerduty.asciidoc +++ b/docs/user/alerting/action-types/pagerduty.asciidoc @@ -141,13 +141,13 @@ Integration Key:: A 32 character PagerDuty Integration Key for an integration [source,text] -- - id: 'my-pagerduty' - name: preconfigured-pagerduty-action-type - actionTypeId: .pagerduty - config: - apiUrl: https://test.host <1.1> - secrets: - routingKey: testroutingkey <2.1> + my-pagerduty: + name: preconfigured-pagerduty-action-type + actionTypeId: .pagerduty + config: + apiUrl: https://test.host <1.1> + secrets: + routingKey: testroutingkey <2.1> -- `config` defines the action type specific to the configuration and contains the following properties: diff --git a/docs/user/alerting/action-types/server-log.asciidoc b/docs/user/alerting/action-types/server-log.asciidoc index f08dbe5542f0f..eadca229bc19c 100644 --- a/docs/user/alerting/action-types/server-log.asciidoc +++ b/docs/user/alerting/action-types/server-log.asciidoc @@ -18,9 +18,9 @@ Name:: The name of the connector. The name is used to identify a connector [source,text] -- - id: 'my-server-log' - name: test - actionTypeId: .server-log + my-server-log: + name: test + actionTypeId: .server-log -- [float] diff --git a/docs/user/alerting/action-types/slack.asciidoc b/docs/user/alerting/action-types/slack.asciidoc index 195093536bc04..afa616ba77b3a 100644 --- a/docs/user/alerting/action-types/slack.asciidoc +++ b/docs/user/alerting/action-types/slack.asciidoc @@ -19,11 +19,11 @@ Webhook URL:: The URL of the incoming webhook. See https://api.slack.com/messa [source,text] -- - id: 'my-slack' - name: preconfigured-slack-action-type - actionTypeId: .slack - config: - webhookUrl: 'https://hooks.slack.com/services/abcd/efgh/ijklmnopqrstuvwxyz' <1> + my-slack: + name: preconfigured-slack-action-type + actionTypeId: .slack + config: + webhookUrl: 'https://hooks.slack.com/services/abcd/efgh/ijklmnopqrstuvwxyz' <1> -- `config` defines the action type specific to the configuration and contains the following properties: diff --git a/docs/user/alerting/action-types/webhook.asciidoc b/docs/user/alerting/action-types/webhook.asciidoc index f4c108426642d..27609652288b5 100644 --- a/docs/user/alerting/action-types/webhook.asciidoc +++ b/docs/user/alerting/action-types/webhook.asciidoc @@ -23,17 +23,17 @@ Password:: An optional password. If set, HTTP basic authentication is used. Cur [source,text] -- - id: 'my-webhook' - name: preconfigured-webhook-action-type - actionTypeId: .webhook - config: - url: https://test.host <1.1> - method: POST <1.2> - headers: <1.3> - testheader: testvalue - secrets: - user: testuser <2.1> - password: passwordkeystorevalue <2.2> + my-webhook: + name: preconfigured-webhook-action-type + actionTypeId: .webhook + config: + url: https://test.host <1.1> + method: POST <1.2> + headers: <1.3> + testheader: testvalue + secrets: + user: testuser <2.1> + password: passwordkeystorevalue <2.2> -- `config` defines the action type specific to the configuration and contains the following properties: diff --git a/docs/user/alerting/pre-configured-connectors.asciidoc b/docs/user/alerting/pre-configured-connectors.asciidoc index 5ff4ea15df561..d5c20d1853d42 100644 --- a/docs/user/alerting/pre-configured-connectors.asciidoc +++ b/docs/user/alerting/pre-configured-connectors.asciidoc @@ -25,12 +25,12 @@ The following example shows a valid configuration of two out-of-the box connecto ```js xpack.actions.preconfigured: - - id: 'my-slack1' <1> + my-slack1: <1> actionTypeId: .slack <2> name: 'Slack #xyz' <3> config: <4> webhookUrl: 'https://hooks.slack.com/services/abcd/efgh/ijklmnopqrstuvwxyz' - - id: 'webhook-service' + webhook-service: actionTypeId: .webhook name: 'Email service' config: @@ -44,7 +44,7 @@ The following example shows a valid configuration of two out-of-the box connecto password: changeme ``` -<1> `id` is the action connector identifier. +<1> the key is the action connector identifier, eg `my-slack1` in this example. <2> `actionTypeId` is the action type identifier. <3> `name` is the name of the preconfigured connector. <4> `config` is the action type specific to the configuration. @@ -69,7 +69,7 @@ The following example shows a valid configuration of preconfigured action type w ```js xpack.actions.enabledActionTypes: ['.slack', '.email', '.index'] <1> xpack.actions.preconfigured: <2> - - id: 'my-server-log' + my-server-log: actionTypeId: .server-log name: 'Server log #xyz' ```