[Alerting] changes preconfigured actions config from array to object (#…

…65397) (#65756) resolves #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.
elastic · May 7, 2020 · ddb3e6d · ddb3e6d
1 parent 159d49e
commit ddb3e6d
Show file tree

Hide file tree

Showing 14 changed files with 166 additions and 121 deletions.
diff --git 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: [email protected] <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: [email protected] <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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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'
 ```

diff --git 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<String> |
 | _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<String> |
-| _xpack.actions._**preconfigured**      | A list of preconfigured actions. Default: `[]`                                                                                                                                                                                                                                                                                                                                                                                                                                | Array<Object> |
+| _xpack.actions._**preconfigured**      | A object of action id / preconfigured actions. Default: `{}`                                                                                                                                                                                                                                                                                                                                                                                                                                | Array<Object> |
 
 #### 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
@@ -14,7 +14,7 @@ describe('config validation', () => {
         "enabledActionTypes": Array [
           "*",
         ],
-        "preconfigured": Array [],
+        "preconfigured": Object {},
         "whitelistedHosts": Array [
           "*",
         ],
@@ -24,38 +24,73 @@ describe('config validation', () => {
 
   test('action with preconfigured actions', () => {
     const config: Record<string, unknown> = {
-      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 {
         "enabled": true,
         "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
@@ -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<typeof configSchema>;
+
+const invalidActionIds = new Set(['', '__proto__', 'constructor']);
+
+function validatePreconfigured(preconfigured: Record<string, unknown>): 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__"`;
+  }
+}