[alerting] Adds whitelisting configuration for the builtin webhooks action #44242

gmmorris · 2019-08-28T12:41:10Z

Summary

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

…ook actions

elasticmachine · 2019-08-28T13:53:19Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

…Kiabana configs making them an action specific concern

pmuellr

I was imagining something much simpler - single config whitelist property, and some library function an action can call to check to see if a particular hostname/url/whatever-we-whitelist-on is "valid", probably returning a response object {status, message} that the action can just return (single translated message in the lib function).

Not sure if we really need per-action whitelists; maybe I missed a discussion on that.

pmuellr · 2019-08-28T18:57:23Z

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

@@ -8,6 +8,7 @@ jest.mock('./lib/send_email', () => ({
  sendEmail: jest.fn(),
 }));

+import { none } from 'fp-ts/lib/Option';


Just happened to think; email takes either a host/port or "service name", where a service name is a nodemailer alias for a well-known host/port, eg, 'gmail'.

I believe we could probably check the ultimate host/port if a service name is provided (get it from nodemailer), so we could add a whitelisting check here. Understand if you don't want to tackle that in this PR, and if not, create a project card to do it later.

Likewise, pagerduty and slack have default but overridable http scheme/host/port values we should be able to whitelist.

Yeah, I was definitely focusing on Webhooks here.
I definitely think we need to take into account what email/slack need when designing the solution, but the implementation should probably be a follow up PR.

It's a good point that in webhooks you might want to "whitelist hostnames and paths", while in email "hostnames and nodemailer aliases", this feels like an argument for separate whitelists, as the needs are different.
No? 🤔

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

pmuellr · 2019-08-28T19:06:13Z

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

+  return configObject => {
+    const { url }: ActionTypeConfigType = configObject;
+
+    const anyUrlAllowed = kibanaConfig


It looks like the config takes a list of urls (things parseable as a Url), but only checks the hostname. If that's the case, I think the whitelist should just contain the hostname, otherwise it's a bit confusing; it looks like you could whitelist https://bob.com, and then hope that http://bob.com wouldn't pass whitelist validation, but it would.

We also have one non-http action - email - so using an url for that would be slightly odd.

Yeah, I was wondering about that and figured I'd just push it a a draft and then have a conversation.

The question I had in the back of my ind was - might we want to whitelist a specific path and not a whole hostname?

pmuellr · 2019-08-28T19:18:34Z

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

@@ -59,6 +59,10 @@ export function createTestConfig(name: string, options: CreateTestConfigOptions)
          `--plugin-path=${path.join(__dirname, 'fixtures', 'plugins', 'alerts')}`,
          `--plugin-path=${path.join(__dirname, 'fixtures', 'plugins', 'actions')}`,
          `--server.xsrf.whitelist=${JSON.stringify(getAllExternalServiceSimulatorPaths())}`,
+          `--xpack.actions.webhook.whitelistedEndpoints=${JSON.stringify([


I'm not sure that we really need or want action-specific config values here; I was imagining a single list used across all actions. That wouldn't allow you to allow a url for one action, and not for another, but I'm not sure there's a real use case there. Seems like it could simplify the code.

If we do have action-specific whitelists, should we check if they specify a config for an action that doesn't exist, like ... they mispelled it? "warning: action whitelist config specified for non-existant action foo-bob"

Added comments on this here: https://github.com/elastic/kibana-team/issues/136

Felt like the conversation was being split too much between Slack/PR/Issue :)

pmuellr · 2019-08-28T19:30:43Z

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

  url: schema.string(),
  method: schema.oneOf([schema.literal(WebhookMethods.POST), schema.literal(WebhookMethods.PUT)], {
    defaultValue: WebhookMethods.POST,
  }),
  headers: nullableType(HeadersSchema),
+};
+export type ActionTypeConfigType = TypeOf<typeof UnvalidatedConfigSchema>;


there's something about not having these type and schema's name "match" that feels not right :-)

Hmm fair.
The reason for "ActionTypeConfigType" is to align with the other actions.
The UnvalidatedConfigSchema is because... it doesn't have validation attached, the reason being that we can't yet attach validation because we don't have the configuration at the time of parsing the module yet.

I was hoping to find a way to "attach" validation to a Schema and so we wouldn't need two versions but it doesn't look possible.

I'll rename it so it aligns, because I know what you mean... but... it also feel wrong to not be explicit about it not having the expected validation... :/

pmuellr · 2019-08-28T19:35:01Z

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

@@ -4,31 +4,103 @@
 * you may not use this file except in compliance with the Elastic License.
 */
 import { i18n } from '@kbn/i18n';
+import { URL } from 'url';
+import Boom from 'boom';


just checked and we currently aren't using Boom in the other actions, so this is maybe a smell. Basically, actions never throw errors :-) . I think in the Boom usage below, you can just return a {status, message} object instead of throwing.

I thought we do when a schema is validated:

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

Line 51 in 91cc23b

throw Boom.badRequest(`error validating ${name}: ${err.message}`);

I used Boom for that reason - to align with the other validations.

pmuellr · 2019-08-28T19:52:51Z

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

@@ -4,6 +4,7 @@
 * you may not use this file except in compliance with the Elastic License.
 */

+import { Option } from 'fp-ts/lib/Option';


I think Option<T> is just T | undefined - oh, looking at it, it's ... a thing!

Not sure we want to expose this in an API that a customer may have to code against - they'll need to pull in fp-ts as well in their action code?

Well, the point of an Option is as a guard against null and undefined.

Ideally you don't want developers to have to think about null/undefined and anything that has the possibility of not being there is exposed as an Option. Once you get used to an Option based code base, it's hard to go back to the numerous null checks that usually fill JS codebases 🤷‍♂️

I was a little worried that I might be jumping into the deep end with this as people who aren't used to it will find it weird, but having completely moved away from nulls (and null pointer exceptions) over the past few years... it's very attractive to use Optionals to step up our use of the typesystem and null safety.

I guess this is a debate we need to have about whether we'd like to try and provide a "null safe" approach to interacting with our code using Optionals, or leave that to developers.

The issue of "customer may have to code against... fp-ts" mioght be an issue, I'm just not familiar with how we expect these customers to add their own code.

Will that be within Kibana or as external code?

elasticmachine · 2019-08-29T07:55:02Z

Pinging @elastic/kibana-stack-services

elasticmachine · 2019-08-29T09:18:32Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

…ng align a little better

elasticmachine · 2019-08-29T10:52:16Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-08-29T11:18:39Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

…cluded protocols breaking the tests

elasticmachine · 2019-08-29T12:53:35Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-08-29T14:06:33Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

gmmorris · 2019-08-30T10:57:48Z

Closing as a new PR will include a deeper refactoring that'll allow this to be done better.

gmmorris added 2 commits August 27, 2019 15:17

fix(action-webhook-whitelisting): enabled webhook action

efefa36

feat(webhook-whitelisting): added whitelist in kibana config for webh…

d2773f7

…ook actions

gmmorris added v8.0.0 v7.5.0 labels Aug 28, 2019

gmmorris self-assigned this Aug 28, 2019

Merge branch 'master' into actions/webhook-whitelisting

5be9aa4

feat(webhook-whitelisting): extracted the schema for action specific …

91cc23b

…Kiabana configs making them an action specific concern

pmuellr reviewed Aug 28, 2019

View reviewed changes

gmmorris added Feature:Alerting Team:Stack Services labels Aug 29, 2019

gmmorris added 2 commits August 29, 2019 10:30

fix(webhook-whitelisting): fixed typo

da1a502

refactor(webhook-whitelisting): renamed Config types to make the nami…

ee41f02

…ng align a little better

gmmorris added 2 commits August 29, 2019 12:36

fix(webhook-whitelisting): whitelisting has to be hostnames and not urls

f13da9f

fix(webhook-whitelisting): whitelisting baked into the test server in…

67a2cda

…cluded protocols breaking the tests

gmmorris closed this Aug 30, 2019

gmmorris added the backported label Sep 9, 2019

gmmorris deleted the actions/webhook-whitelisting branch November 21, 2019 14:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[alerting] Adds whitelisting configuration for the builtin webhooks action #44242

[alerting] Adds whitelisting configuration for the builtin webhooks action #44242

gmmorris commented Aug 28, 2019 •

edited

Loading

elasticmachine commented Aug 28, 2019

pmuellr left a comment

pmuellr Aug 28, 2019

pmuellr Aug 28, 2019

gmmorris Aug 29, 2019

pmuellr Aug 28, 2019

gmmorris Aug 29, 2019 •

edited

Loading

pmuellr Aug 28, 2019

gmmorris Aug 29, 2019

pmuellr Aug 28, 2019

gmmorris Aug 29, 2019

pmuellr Aug 28, 2019

gmmorris Aug 29, 2019

pmuellr Aug 28, 2019

gmmorris Aug 29, 2019

gmmorris Aug 29, 2019

elasticmachine commented Aug 29, 2019

elasticmachine commented Aug 29, 2019

elasticmachine commented Aug 29, 2019

elasticmachine commented Aug 29, 2019

elasticmachine commented Aug 29, 2019

elasticmachine commented Aug 29, 2019

gmmorris commented Aug 30, 2019

[alerting] Adds whitelisting configuration for the builtin webhooks action #44242

[alerting] Adds whitelisting configuration for the builtin webhooks action #44242

Conversation

gmmorris commented Aug 28, 2019 • edited Loading

Summary

Checklist

For maintainers

elasticmachine commented Aug 28, 2019

💔 Build Failed

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

gmmorris Aug 29, 2019 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elasticmachine commented Aug 29, 2019

elasticmachine commented Aug 29, 2019

💚 Build Succeeded

elasticmachine commented Aug 29, 2019

💚 Build Succeeded

elasticmachine commented Aug 29, 2019

💚 Build Succeeded

elasticmachine commented Aug 29, 2019

💔 Build Failed

elasticmachine commented Aug 29, 2019

💚 Build Succeeded

gmmorris commented Aug 30, 2019

gmmorris commented Aug 28, 2019 •

edited

Loading

gmmorris Aug 29, 2019 •

edited

Loading