[alerting] Adds Action Type configuration support and whitelisting #44483

gmmorris · 2019-08-30T10:26:05Z

Summary

Refactor Action Types registration.
Adds whitelisting configuration for the builtin webhooks action.
Refers to: https://github.com/elastic/kibana-team/issues/136

Checklist

Use ~~strikethroughs~~ to remove checklist items you don't feel are applicable to this PR.

~~This was checked for cross-browser compatibility, including a check against IE11~~
Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
Documentation was added for features that require explanation or tutorials
Unit or functional tests were updated or added to match the most common scenarios
~~This was checked for keyboard-only and screenreader accessibility~~

For maintainers

This was checked for breaking API changes and was labeled appropriately
This includes a feature addition or change that requires a release note and was labeled appropriately

…e instanciation

elasticmachine · 2019-08-30T11:04:02Z

Pinging @elastic/kibana-stack-services

gmmorris · 2019-08-30T11:08:29Z

I'll we adding documentation for:

The API change around ActionType usage
The additional configuration needed for whitelisting.\

Just waiting to confirm the team is comfortable with the changes....

elasticmachine · 2019-08-30T12:45:45Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-08-30T14:32:56Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

pmuellr

This is looking great! I'll re-review when you feel like it's done.

x-pack/legacy/plugins/actions/index.ts

x-pack/legacy/plugins/actions/server/actions_config.ts

x-pack/legacy/plugins/actions/server/builtin_action_types/slack.ts

x-pack/legacy/plugins/actions/server/builtin_action_types/webhook.ts

x-pack/test/alerting_api_integration/common/config.ts

… an empty array and Any is now an asterix

elasticmachine · 2019-09-02T09:02:15Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-09-02T10:10:51Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

…easier to read

elasticmachine · 2019-09-02T12:27:24Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

…ion configuration options

…n the whitelist config array

…config

elasticmachine · 2019-09-04T10:38:31Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-09-04T11:26:12Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

bmcconaghy

The implementation of isWhitelistedHost seems convoluted to me. I expected it to either return a boolean (based on * || Array.includes) or to throw if those conditions are not met. Returning an error seems like an odd choice to me, definitely don't expect errors to get passed around, only thrown when they are created.

bmcconaghy · 2019-09-04T13:56:30Z

x-pack/legacy/plugins/actions/server/actions_config.ts

+}
+
+export interface ActionsConfigurationUtilities {
+  isWhitelistedHostname: (hostname: string) => string | NotWhitelistedError;


I find the name and semantics of this to be strange. I expect is... methods to return booleans.

Fair, I could rename it to ensureIsWhitelisted... or something like that.

bmcconaghy · 2019-09-04T13:59:54Z

x-pack/legacy/plugins/actions/server/actions_config.ts

+      return tryCatch(() => new URL(uri))
+        .map(url => url.hostname)
+        .map(hostname => {
+          const result = isWhitelistedHostname(hostname);


why doesn't this just throw instead of returning an error and then throwing based on instanceof? Seems overcomplicated to me.

You can't typecheck thrown errors in Typescript, so the general recommendation there seems to be to return errors.
I agree it's over complicated- that's why the Result type is a much more elegant solution, but there were concerns about Result type being used on external APIs.

gmmorris · 2019-09-04T14:09:28Z

The implementation of isWhitelistedHost seems convoluted to me. I expected it to either return a boolean (based on * || Array.includes) or to throw if those conditions are not met. Returning an error seems like an odd choice to me, definitely don't expect errors to get passed around, only thrown when they are created.

It did originally return a boolean, but following feedback from Patrick we decided the function would return the messaging that made it . clear what the source of the problem was (value not being in the whitelist) so that you don't have top repeat that in each action that uses the utility.

I also agree, returning Errors is weird in JS land, which is why the Result type is a nice abstraction for this, but as long as there's unease about using TS types on APIs like this, we're low on options here.

We can either:

Throw, which means you can't type check that it's handled by the developer.
Only return a value if it is an error (like validators) which is hard to compose, is implicit, and is error prone.
Return boolean, and handle the messaging independently, which repeats code.

To me, the ideal is option #4, where we use TS on our APIs, but sounds like we're not quite ready for that yet.

bmcconaghy · 2019-09-04T14:21:17Z

I'm OK with the approach here so long as we change the name of the method to check....

gmmorris · 2019-09-04T14:36:38Z

I'm OK with the approach here so long as we change the name of the method to check....

Cool, thanks, I'll rename to make it clearer.

pmuellr

Made a bunch of comments; most concerned with the validator function shapes here - eg, isWhitelistedHostname: (hostname: string) => string | NotWhitelistedError - it doesn't seem very idiomatic to me. I suppose I could be convinced, but ... it will be tough heh.

x-pack/legacy/plugins/actions/README.md

x-pack/legacy/plugins/actions/server/actions_config.ts

pmuellr · 2019-09-04T13:48:37Z

x-pack/legacy/plugins/actions/server/actions_config.ts

+}
+
+const whitelistingErrorMessage = 'target url not in whitelist';
+const doesValueWhitelistAnyHostname = (whitelistedHostname: string): boolean =>


this starts to read as a valuable piece of function, but there's no verbs, or something. Would something like isHostnameWildcard() work instead? Or is this some kind of functional pattern I'm not familiar with?

I also noticed you used const <functionName> = <lambda> as is the style for a lot of code these days, but I think we've generally just used functions in this area of Kibana at least; ie, function doesValueWhitelistAnyHostname(...) {...} instead.

Regarding doesValueWhitelistAnyHostname it's not about FP, it's about communicating intent.
Implementation is easier to communicate (they read the code) but intent is much harder (read the code, and the surrounding code, and how they fit together). I find it helpful to communicate intent in the name of functions rather than implementation.

Hope that helps

x-pack/legacy/plugins/actions/server/builtin_action_types/email.test.ts

pmuellr · 2019-09-04T14:07:54Z

x-pack/legacy/plugins/actions/server/builtin_action_types/webhook.ts

  const log = (level: string, msg: string) =>
    execOptions.services.log([level, 'actions', 'webhook'], msg);

  const id = execOptions.id;
  const { method, url, headers = {} } = execOptions.config as ActionTypeConfigType;
  const { user: username, password } = execOptions.secrets as ActionTypeSecretsType;
  const { body: data } = execOptions.params as ActionParamsType;
+  const whitelistValidation = configurationUtilities.isWhitelistedUri(url);


I thought we re-ran the validators (all of them?) before invoking the executor? Yeah, I think so, so this explicit check shouldn't be needed, and I think you'd never get into this method, and have the subsequent code run.

kibana/x-pack/legacy/plugins/actions/server/lib/execute.ts

Lines 50 to 56 in 49fdd24

try {

validatedParams = validateParams(actionType, params);

validatedConfig = validateConfig(actionType, config);

validatedSecrets = validateSecrets(actionType, secrets);

} catch (err) {

return { status: 'error', message: err.message, retry: false };

}

Yup, I've double checked and this is true, but somewhat implicit as it requires prior knowledge of the wiring.
I've removed the redundant validation, but feel this is something worth revisiting in the future... perhaps there's a better way to model this so it's clearer.

x-pack/legacy/plugins/actions/server/builtin_action_types/webhook.ts

x-pack/test/alerting_api_integration/security_and_spaces/tests/actions/index.ts

elasticmachine · 2019-09-05T08:38:41Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

…ts, either throw or boolean

…r as config is rechecked before the executor is called

elasticmachine · 2019-09-05T10:07:54Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-09-05T10:49:19Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

bmcconaghy

Code LGTM

pmuellr

Fantastic!

I made a few comments for things I think need to be fixed, but can be in a subsequent PR; let's get this sucker merged!

pmuellr · 2019-09-05T18:18:16Z

x-pack/legacy/plugins/actions/index.ts

@@ -27,6 +27,13 @@ export function actions(kibana: any) {
      return Joi.object()
        .keys({
          enabled: Joi.boolean().default(false),
+          whitelistedHosts: Joi.alternatives()


I think alternatives was here when the property was string | string[]? So, I think we can remove the alternatives() and try() wrappers, just make it a Joi.array()? Or maybe there's some joi sneakiness I don't yet understand :-)

It clearly works now, so fine with shipping this way, create an issue / project card if it should be fixed later.

Yeah, that's not needed anymore, I'll fix in a small PR next

pmuellr · 2019-09-05T18:23:11Z

x-pack/legacy/plugins/actions/server/builtin_action_types/es_index.test.ts

@@ -19,6 +20,12 @@ import { ActionParamsType, ActionTypeConfigType } from './es_index';

 const ACTION_TYPE_ID = '.index';
 const NO_OP_FN = () => {};
+const MOCK_KIBANA_CONFIG_UTILS: ActionsConfigurationUtilities = {


fyi: more bits to add to the theoretical "testing rig for actions" :-)

pmuellr · 2019-09-05T18:30:06Z

x-pack/legacy/plugins/actions/server/builtin_action_types/webhook.ts

+    configurationUtilities.ensureWhitelistedUri(configObject.url);
+  } catch (whitelistError) {
+    return i18n.translate('xpack.actions.builtin.webhook.webhookConfigurationError', {
+      defaultMessage: 'error configuring webhook action: {message}',


I think we still need the actionId in here - unless it's somehow included in some other way (not obvious to me that it is).

If we need to get it added, can be done in a subsequent PR; create an issue | project card for it.

Yeah, we don't have access to the ID at this point (in create it happens before an ID is generated, and in update/execute it's not provided), so this will have to be a separate PR.

…lastic#44483) A wide refactor of Action Types and their configuration: - Action Types are now initialised with a their associated configuration based on `kibana.yml` - You may now whitelist certain hostnames to allow built-in actions, such as _Webhooks_, to communicate with the outside world - The _Webhooks_ Built-in action is now available, if you have `alerting` and `actions` enabled

…44483) (#44969) A wide refactor of Action Types and their configuration: - Action Types are now initialised with a their associated configuration based on `kibana.yml` - You may now whitelist certain hostnames to allow built-in actions, such as _Webhooks_, to communicate with the outside world - The _Webhooks_ Built-in action is now available, if you have `alerting` and `actions` enabled

…ete-for-distance_feature * 'master' of github.com:elastic/kibana: (89 commits) Replace TSVB timeseries charts with elastic-charts (elastic#33558) [TSVB][Top N aggregation] Unable to deal with negative values (elastic#43581) [alerting] Adds Action Type configuration support and whitelisting (elastic#44483) FTR: fix WebDriver Actions calls (elastic#44605) [Code] add NodeRepositoriesService to watch new repositories on local node (elastic#44677) [skip-ci][Maps] Improve Maps intro page (elastic#44721) [Maps] Update titles and descriptions for data sources (elastic#44833) Types + Extract Integration Util (elastic#44433) Downgrade log level from info to debug for cases when we cannot handle authentication attempt. (elastic#44933) [Reporting] Remove Chome stdout/stderr observables, Add Browser Logger observable (elastic#44359) Update Jest script to output coverage (elastic#44447) [ftr] support --kibana-install-dir flag (elastic#44552) [WATCHER] Allow user to set a threshold value of 0 (elastic#44810) Remove injectI18n in dashboard plugin. (elastic#44580) [Graph] Save modal (elastic#44261) Use external script for the OIDC Implicit flow handler page. (elastic#44866) disable router prefixing with pluginId (elastic#44855) [SIEM] Fix bug on url + inspect functionality on hosts/hostDetails page (elastic#44671) [ML] File data viz limiting uploaded doc chunk size (elastic#44768) [code] Append go env variable 'GOCACHE' to go lsp spawn command. (elastic#44864) ...

…plate * 'master' of github.com:elastic/kibana: (91 commits) [APM] Make number of x ticks responsive to the plot width (elastic#44870) [ML] Single metric viewer: Fix top nav refresh behaviour. (elastic#44860) Replace TSVB timeseries charts with elastic-charts (elastic#33558) [TSVB][Top N aggregation] Unable to deal with negative values (elastic#43581) [alerting] Adds Action Type configuration support and whitelisting (elastic#44483) FTR: fix WebDriver Actions calls (elastic#44605) [Code] add NodeRepositoriesService to watch new repositories on local node (elastic#44677) [skip-ci][Maps] Improve Maps intro page (elastic#44721) [Maps] Update titles and descriptions for data sources (elastic#44833) Types + Extract Integration Util (elastic#44433) Downgrade log level from info to debug for cases when we cannot handle authentication attempt. (elastic#44933) [Reporting] Remove Chome stdout/stderr observables, Add Browser Logger observable (elastic#44359) Update Jest script to output coverage (elastic#44447) [ftr] support --kibana-install-dir flag (elastic#44552) [WATCHER] Allow user to set a threshold value of 0 (elastic#44810) Remove injectI18n in dashboard plugin. (elastic#44580) [Graph] Save modal (elastic#44261) Use external script for the OIDC Implicit flow handler page. (elastic#44866) disable router prefixing with pluginId (elastic#44855) [SIEM] Fix bug on url + inspect functionality on hosts/hostDetails page (elastic#44671) ...

gmmorris added 3 commits August 30, 2019 11:23

feat(action-types-refactor): pass kibana config to builtin action typ…

6c48bc1

…e instanciation

Merge branch 'master' into refactor-action-types

4113bd2

fix(webhook-whitelisting): whitelist support for webhook execution

c03104c

gmmorris added Feature:Alerting Team:Stack Services v7.5.0 v8.0.0 labels Aug 30, 2019

gmmorris changed the title ~~[alerting] refactor action types~~ [alerting] Action Type configuration and whitelisting Aug 30, 2019

gmmorris changed the title ~~[alerting] Action Type configuration and whitelisting~~ [alerting] Adds Action Type configuration support and whitelisting Aug 30, 2019

fix(webhook-whitelisting): fixed missing type sig

e46f616

pmuellr reviewed Aug 30, 2019

View reviewed changes

gmmorris added 2 commits September 2, 2019 08:28

Merge branch 'master' into actions/refactor-action-types

a12f7b9

refactor(webhook-whitelisting): removed the None option as we can use…

ea4580f

… an empty array and Any is now an asterix

gmmorris added 4 commits September 2, 2019 11:17

refactor(webhook-whitelisting): unified whitelisting error message

f169c0a

refactor(webhook-whitelisting): curry valdiaiton to make it a little …

4855bb0

…easier to read

fix(webhook-whitelisting): removed unused import

cd87387

fix(webhook-whitelisting): cleaned up messaging around webhook errors

057a6ec

gmmorris added 2 commits September 2, 2019 13:35

fix(webhook-whitelisting): fixed typing of mocks

fad46ab

readme(webhook-whitelisting): added documentation of the Built-in-Act…

7d8ed08

…ion configuration options

gmmorris marked this pull request as ready for review September 2, 2019 13:17

gmmorris requested a review from a team September 2, 2019 13:17

Merge branch 'master' into actions/refactor-action-types

a93f2f0

gmmorris added the release_note:skip Skip the PR/issue when compiling release notes label Sep 2, 2019

gmmorris added 3 commits September 4, 2019 10:07

doc(webhook-whitelisting): Updated documentation for whitelisting

ac21af9

refactor(webhook-whitelisting): Whitelist Any url by specifying a * i…

b277445

…n the whitelist config array

doc(webhook-whitelisting): Updated documentation for actions Enabled …

2699dd3

…config

bmcconaghy reviewed Sep 4, 2019

View reviewed changes

Merge branch 'master' into actions/refactor-action-types

f73fb00

pmuellr requested changes Sep 4, 2019

View reviewed changes

Merge branch 'master' into actions/refactor-action-types

481da21

gmmorris added 3 commits September 5, 2019 09:50

refactor(webhook-whitelisting): Provide two ways of checking whitelis…

9a84b8b

…ts, either throw or boolean

refactor(webhook-whitelisting): Removed whitelisting check in executo…

8a9ddc9

…r as config is rechecked before the executor is called

Merge branch 'master' into actions/refactor-action-types

72f70b4

bmcconaghy approved these changes Sep 5, 2019

View reviewed changes

pmuellr approved these changes Sep 5, 2019

View reviewed changes

gmmorris merged commit 0d1f3cf into elastic:master Sep 6, 2019

gmmorris mentioned this pull request Sep 6, 2019

[7.x] [alerting] Adds Action Type configuration support and whitelisting (#44483) #44969

Merged

gmmorris added the backported label Sep 9, 2019

gmmorris deleted the actions/refactor-action-types branch November 21, 2019 14:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[alerting] Adds Action Type configuration support and whitelisting #44483

[alerting] Adds Action Type configuration support and whitelisting #44483

gmmorris commented Aug 30, 2019 •

edited

Loading

elasticmachine commented Aug 30, 2019

gmmorris commented Aug 30, 2019 •

edited

Loading

elasticmachine commented Aug 30, 2019

elasticmachine commented Aug 30, 2019

pmuellr left a comment

elasticmachine commented Sep 2, 2019

elasticmachine commented Sep 2, 2019

elasticmachine commented Sep 2, 2019

elasticmachine commented Sep 4, 2019

elasticmachine commented Sep 4, 2019

bmcconaghy left a comment

bmcconaghy Sep 4, 2019

gmmorris Sep 4, 2019

bmcconaghy Sep 4, 2019

gmmorris Sep 4, 2019

gmmorris commented Sep 4, 2019 •

edited

Loading

bmcconaghy commented Sep 4, 2019

gmmorris commented Sep 4, 2019

pmuellr left a comment

pmuellr Sep 4, 2019

gmmorris Sep 5, 2019

pmuellr Sep 4, 2019

gmmorris Sep 5, 2019

elasticmachine commented Sep 5, 2019

elasticmachine commented Sep 5, 2019

elasticmachine commented Sep 5, 2019

bmcconaghy left a comment

pmuellr left a comment

pmuellr Sep 5, 2019

gmmorris Sep 6, 2019

pmuellr Sep 5, 2019

gmmorris Sep 6, 2019

pmuellr Sep 5, 2019

gmmorris Sep 6, 2019

	try {
	validatedParams = validateParams(actionType, params);
	validatedConfig = validateConfig(actionType, config);
	validatedSecrets = validateSecrets(actionType, secrets);
	} catch (err) {
	return { status: 'error', message: err.message, retry: false };
	}

[alerting] Adds Action Type configuration support and whitelisting #44483

[alerting] Adds Action Type configuration support and whitelisting #44483

Conversation

gmmorris commented Aug 30, 2019 • edited Loading

Summary

Checklist

For maintainers

elasticmachine commented Aug 30, 2019

gmmorris commented Aug 30, 2019 • edited Loading

elasticmachine commented Aug 30, 2019

💚 Build Succeeded

elasticmachine commented Aug 30, 2019

💚 Build Succeeded

pmuellr left a comment

Choose a reason for hiding this comment

elasticmachine commented Sep 2, 2019

💚 Build Succeeded

elasticmachine commented Sep 2, 2019

💚 Build Succeeded

elasticmachine commented Sep 2, 2019

💔 Build Failed

elasticmachine commented Sep 4, 2019

💚 Build Succeeded

elasticmachine commented Sep 4, 2019

💚 Build Succeeded

bmcconaghy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gmmorris commented Sep 4, 2019 • edited Loading

bmcconaghy commented Sep 4, 2019

gmmorris commented Sep 4, 2019

pmuellr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elasticmachine commented Sep 5, 2019

💚 Build Succeeded

elasticmachine commented Sep 5, 2019

💚 Build Succeeded

elasticmachine commented Sep 5, 2019

💚 Build Succeeded

bmcconaghy left a comment

Choose a reason for hiding this comment

pmuellr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gmmorris commented Aug 30, 2019 •

edited

Loading

gmmorris commented Aug 30, 2019 •

edited

Loading

gmmorris commented Sep 4, 2019 •

edited

Loading