[actions] adds config allowing per-host networking options #96630

pmuellr · 2021-04-08T18:45:42Z

resolves #80120

doc preview: https://kibana_96630.docs-preview.app.elstc.co/guide/en/kibana/master/alert-action-settings-kb.html

Summary

Adds a new Kibana configuration key xpack.actions.customHostSettings which allows per-host configuration of connection settings for https and smtp for alerting actions. Initially this is just for TLS settings, expandable to other settings in the future.

The purpose of these is to allow customers to provide server certificates for servers accessed by actions, whose certificate authority is not available publicly. Alternatively, a per-server rejectUnauthorized: false configuration may be used to bypass the verification step for specific servers, but require it for other servers that do not have per-host customization.

Support was also added to allow per-host customization of ignoreTLS and requireTLS flags for use with the email action.

Checklist

Delete any items that are not applicable to this PR.

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
~~Any UI touched in this PR is usable by keyboard only (learn more about keyboard accessibility)~~
~~Any UI touched in this PR does not create any new axe failures (run axe in browser: FF, Chrome)~~
If a plugin configuration key changed, check if it needs to be allowlisted in the cloud and added to the docker list
~~This renders correctly on smaller devices using a responsive layout. (You can test this in your browser)~~
~~This was checked for cross-browser compatibility~~

For maintainers

~~This was checked for breaking API changes and was labeled appropriately~~

Remaining To Do's

Release note

Adds a new Kibana configuration key xpack.actions.customHostSettings which allows
per-host configuration of connection settings for https and smtp for alerting
actions.

elasticmachine · 2021-04-15T18:46:27Z

Pinging @elastic/kibana-alerting-services (Team:Alerting Services)

pmuellr · 2021-04-15T18:52:46Z

This PR is not quite done, but I think ready for review, especially considering 7.13 feature freeze is next week - hopefully we can squeeze this in.

todo's:

fix the doc - there's some bad formatting due to the excessive length of the config keys
add function tests - there's not a ton we can add here, given that we only have two configs we can actually test with, so I'm not expecting to have to spend a lot of time on these
add the cloud doc and config validation, in the cloud repo

Note on the function tests - I've actually added some jest tests that do "kind of" end-to-end testing with various configs, using actual config properties, real https servers and our axios client. I'm surprised this ended up working out so well! I'm going to open an issue to add some similar sorts of tests for proxy and mail (using maildev).

pmuellr · 2021-04-15T18:56:19Z

@gchaps will need a review of the docs here (link at the top), and advice on how to avoid the super narrow right-hand column starting 1/3 of the way down, after the example block, due to the excessive length of the config keys

jbudz

kibana-docker LGTM

resolves: elastic#80120 Adds a new Kibana configuration key xpack.actions.customHostSettings which allows per-host configuration of connection settings for https and smtp for alerting actions. Initially this is just for TLS settings, expandable to other settings in the future. The purpose of these is to allow customers to provide server certificates for servers accessed by actions, whose certificate authority is not available publicly. Alternatively, a per-server rejectUnauthorized: false configuration may be used to bypass the verification step for specific servers, but require it for other servers that do not have per-host customization. Support was also added to allow per-host customization of ignoreTLS and requireTLS flags for use with the email action.

ymao1

This looks awesome! Doesn't have to be done as part of this PR, but I think it would be really helpful to expand the docs section with more examples of how this config can be used in conjunction with various example connector configs. Maybe a troubleshooting section for connector configs that give examples for how to update the config in response to the most commonly seen error messages when executing actions?

pmuellr · 2021-04-19T14:28:44Z

I think it would be really helpful to expand the docs section with more examples of how this config can be used in conjunction with various example connector configs. Maybe a troubleshooting section for connector configs that give examples for how to update the config in response to the most commonly seen error messages when executing actions?

Ya, an extended example would be good here, especially since the cloud version will require inlining the ca PEM data - it's actually not that bad in YAML, but an example will clarify the exact syntax for customers.

YulNaumenko

LGTM

pmuellr · 2021-04-19T14:42:57Z

@elasticmachine merge upstream

pmuellr · 2021-04-19T15:36:08Z

@legrego @jportner woops - should have given you a heads up on this last week. This PR is the "config-only" version of the per-host TLS configuration options, that I had you review previously in PR #94807

I did originally intend to use verificationMode instead of rejectUnauthorized, per comment in the previous PR - but we were already using rejectUnauthorized in two other config keys, so it seemed more appropriate to leave it that way, rather than to reshape that as the same shape as verificationMode.

I just opened issue #97635 to track this.

jportner

Made a quick pass, mostly LGTM but see a few nits/suggestions in my comments below!

@legrego @jportner woops - should have given you a heads up on this last week. This PR is the "config-only" version of the per-host TLS configuration options, that I had you review previously in PR #94807

I did originally intend to use verificationMode instead of rejectUnauthorized, per comment in the previous PR - but we were already using rejectUnauthorized in two other config keys, so it seemed more appropriate to leave it that way, rather than to reshape that as the same shape as verificationMode.

Makes sense, thanks for following up 👍

Note: this just means that users won't be able to use rejectUnauthorized: true with hostname verification disabled (that is what verificationMode: certificate does). We don't want to encourage disabling hostname verification anyway, so that's fine by me.

jportner · 2021-04-19T15:40:20Z

docs/settings/alert-action-settings.asciidoc

+  values.
+
+| `xpack.actions.customHostSettings[n].smtp.ignoreTLS` {ess-icon}
+  | A boolean value indicatting that TLS must not be used for this connection.


Nit

Suggested change

| A boolean value indicatting that TLS must not be used for this connection.

| A boolean value indicating that TLS must not be used for this connection.

jportner · 2021-04-19T15:40:32Z

docs/settings/alert-action-settings.asciidoc

+  | A boolean value indicatting that TLS must not be used for this connection.
+
+| `xpack.actions.customHostSettings[n].smtp.requireTLS` {ess-icon}
+  | A boolean value indicatting that TLS must be used for this connection.


Nit

Suggested change

| A boolean value indicatting that TLS must be used for this connection.

| A boolean value indicating that TLS must be used for this connection.

jportner · 2021-04-19T15:49:05Z

docs/settings/alert-action-settings.asciidoc

+| `xpack.actions.customHostSettings[n].tls.certificateAuthoritiesFiles` {ess-icon}
+  | A file name or list of file names of PEM-encoded certificate files which
+  should be used to validate the server.


Note: this (and a few other cells) make the left column width stretch so much that the docs are difficult to read:

I see above for xpack.encryptedSavedObjects .encryptionKey the workaround was to insert a space to cause the line break, but that means a user can't copy/paste the config property into their YML file without encountering a parsing error.

I ran into the same situation in #97158, I'll paste a relevant snippet of the PR description below:

the asciidoc hacks that I needed to make for the settings table in bb8e2aa are kind of awful, but it seems to be the only solution that will work. The key names for the new config options are too long, and they stretch the left column of the table out to the maximum size as a result. Regular columns in AsciiDoc will not preserve leading whitespace characters, so I had to utilize an AsciiDoc block element column style with the a operator.

So, for the short term, you could potentially change this to make it a bit more readable:

Suggested change

| `xpack.actions.customHostSettings[n].tls.certificateAuthoritiesFiles` {ess-icon}

| A file name or list of file names of PEM-encoded certificate files which

should be used to validate the server.

a|

----

xpack.actions.customHostSettings:

- tls.certificateAuthoritiesFiles: {ess-icon}

----

| A file name or list of file names of PEM-encoded certificate files which

should be used to validate the server.

Kaarina suggested a better long term solution would be to change the layout to use a definition list instead of a table: #97158 (comment)

Problem with this "style", is that I can't actually use the {ess-icon} here. It shows up literally :-)

I ended up doing it the same as the "broken" xpack.encryptedSavedObjects .encryptionKey style already in use here, but I think this is ok since it's unlikely customers would use the "single key/value" approach to setting these values; I'd guess they will use a complete "object" like the example provided in the doc.

jportner · 2021-04-19T16:01:46Z

x-pack/plugins/actions/server/builtin_action_types/lib/get_custom_agents.ts

+    // see: src/core/server/elasticsearch/legacy/elasticsearch_client_config.ts
+    if (tlsSettings.rejectUnauthorized !== undefined) {
+      agentOptions.rejectUnauthorized = tlsSettings.rejectUnauthorized;
+    }


Nit: you mentioned it in the asciidocs, but it's not abundantly clear here that this will inherit from xpack.actions.rejectUnauthorized (which defaults to true)

I would suggest just adding a comment:

Suggested change

// see: src/core/server/elasticsearch/legacy/elasticsearch_client_config.ts

if (tlsSettings.rejectUnauthorized !== undefined) {

agentOptions.rejectUnauthorized = tlsSettings.rejectUnauthorized;

}

// Note: if custom TLS settings are defined but rejectUnauthorized is not set, fall back to the config for `xpack.actions.rejectUnauthorized` (default: true)

// See also: src/core/server/elasticsearch/legacy/elasticsearch_client_config.ts

if (tlsSettings.rejectUnauthorized !== undefined) {

agentOptions.rejectUnauthorized = tlsSettings.rejectUnauthorized;

}

Added a number of comments in this file to hopefully clarify things.

jportner · 2021-04-19T16:05:07Z

x-pack/plugins/actions/server/builtin_action_types/lib/get_custom_agents.test.ts

+
+    expect(httpsAgent?.options.ca).toBe('ca data here');
+    expect(httpsAgent?.options.rejectUnauthorized).toBe(false);
+  });
 });


Follow-on from #96630 (comment):

It would be good to see unit test cases for the different permutations of xpack.actions.rejectUnauthorized and xpack.actions.customHostSettings[n].tls.rejectUnauthorized.

Added additional test cases.

jportner · 2021-04-19T16:05:55Z

x-pack/plugins/actions/server/builtin_action_types/lib/send_email.ts

+      if (tlsSettings?.rejectUnauthorized !== undefined) {
+        tlsConfig.rejectUnauthorized = tlsSettings?.rejectUnauthorized;
+      }


Nit: same as #96630 (comment)

jportner · 2021-04-19T16:11:06Z

x-pack/plugins/actions/server/builtin_action_types/lib/send_email.test.ts

+        },
+      ]
+    `);
+  });
 });


Same as #96630 (comment)

pmuellr · 2021-04-19T16:29:28Z

Note: this just means that users won't be able to use rejectUnauthorized: true with hostname verification disabled (that is what verificationMode: certificate does). We don't want to encourage disabling hostname verification anyway, so that's fine by me.

I think when I looked at some security code, it wasn't actually doing anything with the hostname verification bit - it appeared that perhaps node didn't support that at all, or easily. Or perhaps it ended up getting set up before hand.

I think we probably need to look at changing rejectUnauthorized usage to verificationMode, in the 8.0 time frame. I'll open an issue ...

gchaps · 2021-04-19T18:55:59Z

docs/settings/alert-action-settings.asciidoc

@@ -48,6 +48,67 @@ You can configure the following settings in the `kibana.yml` file.
  +
  Note that hosts associated with built-in actions, such as Slack and PagerDuty, are not automatically added to allowed hosts. If you are not using the default `[*]` setting, you must ensure that the corresponding endpoints are added to the allowed hosts as well.

+| `xpack.actions.customHostSettings` {ess-icon} 
+  | A list of custom host settings to override existing global settings. It
+  defaults to an empty list. In the example below, a custom host setting for a


Suggested change

defaults to an empty list. In the example below, a custom host setting for a

defaults to an empty list. In the following example, a custom host setting for a

gchaps · 2021-04-19T18:56:12Z

docs/settings/alert-action-settings.asciidoc

@@ -48,6 +48,67 @@ You can configure the following settings in the `kibana.yml` file.
  +
  Note that hosts associated with built-in actions, such as Slack and PagerDuty, are not automatically added to allowed hosts. If you are not using the default `[*]` setting, you must ensure that the corresponding endpoints are added to the allowed hosts as well.

+| `xpack.actions.customHostSettings` {ess-icon} 
+  | A list of custom host settings to override existing global settings. It
+  defaults to an empty list. In the example below, a custom host setting for a


Suggested change

defaults to an empty list. In the example below, a custom host setting for a

Defaults to an empty list. In the example below, a custom host setting for a

gchaps · 2021-04-19T18:58:57Z

docs/settings/alert-action-settings.asciidoc

+[cols="2*<"]
+|===
+
+| `xpack.actions.customHostSettings[n].url` {ess-icon}


Try breaking this in two lines:

|xpack.action
.customHostSettings[n].url {ess-icon}

gchaps · 2021-04-19T18:59:31Z

docs/settings/alert-action-settings.asciidoc

+|===
+
+| `xpack.actions.customHostSettings[n].url` {ess-icon}
+  | A URL associated with this custom host setting.  Should be in form


Suggested change

| A URL associated with this custom host setting. Should be in form

| A URL associated with this custom host setting. Should be in the form of

gchaps · 2021-04-19T19:00:04Z

docs/settings/alert-action-settings.asciidoc

+| `xpack.actions.customHostSettings[n].url` {ess-icon}
+  | A URL associated with this custom host setting.  Should be in form
+  `protocol://hostname:port`, where `protocol` is `https` or `smtp`.  If the
+  port is not provided, 443 will be used for `https` and 25 will be used for


Suggested change

port is not provided, 443 will be used for `https` and 25 will be used for

port is not provided, 443 is used for `https` and 25 is used for

gchaps · 2021-04-19T19:00:36Z

docs/settings/alert-action-settings.asciidoc

+  | A URL associated with this custom host setting.  Should be in form
+  `protocol://hostname:port`, where `protocol` is `https` or `smtp`.  If the
+  port is not provided, 443 will be used for `https` and 25 will be used for
+  `smtp`.  The `smtp` URLs will be used for the Email actions which use this


Suggested change

`smtp`. The `smtp` URLs will be used for the Email actions which use this

`smtp`. The `smtp` URLs are used for the Email actions that use this

gchaps · 2021-04-19T19:01:59Z

docs/settings/alert-action-settings.asciidoc

+
+  Note that no other URL values should be part of this URL, including paths,
+  query strings, and authentication information.  When an http or smtp request
+  is being made as part of executing an action, only the protocol, hostname and


Suggested change

is being made as part of executing an action, only the protocol, hostname and

is made as part of executing an action, only the protocol, hostname, and

gchaps · 2021-04-19T19:03:45Z

docs/settings/alert-action-settings.asciidoc

+  port of the URL for that request are used to look up these configuration
+  values.
+
+| `xpack.actions.customHostSettings[n].smtp.ignoreTLS` {ess-icon}


xpack.actions
.customHostSettings[n]
.stmp.ignoreTLS {ess-icon}

gchaps · 2021-04-19T19:14:12Z

docs/settings/alert-action-settings.asciidoc

+  | A boolean value indicatting that TLS must be used for this connection.
+
+| `xpack.actions.customHostSettings[n].tls.rejectUnauthorized` {ess-icon}
+  | A boolean value indicating whether to bypass to certificate validation


Can this be 2 sentences:

A boolean value indicating whether to bypass to certificate validation. Overrides the general xpack.actions.rejectUnauthorized configuration for requests made for this hostname/port.

gchaps · 2021-04-19T19:16:24Z

docs/settings/alert-action-settings.asciidoc

+
+| `xpack.actions.customHostSettings[n].tls.certificateAuthoritiesFiles` {ess-icon}
+  | A file name or list of file names of PEM-encoded certificate files which
+  should be used to validate the server.


Suggest this text:

A file name or list of file names of PEM-encoded certificate files to use to validate the server.

gchaps · 2021-04-19T19:17:06Z

docs/settings/alert-action-settings.asciidoc

+| `xpack.actions.customHostSettings[n].tls.certificateAuthoritiesData` {ess-icon}
+  | The contents of a PEM-encoded certificate file, or multiple files appended
+  into a single string.  This configuration can be used for environments where
+  the files themselves cannot be made available.  


Suggested change

the files themselves cannot be made available.

the files cannot be made available.

Looks like there is an error with the format of the description for this setting:

xpack.actions.responseTimeout

I wonder if something changed? Can no longer use | within back-tick'd blocks? I ended up changing the | to ,, which looks fine, and did a little formatting to get the back-ticked section to not split across a line.

pmuellr · 2021-04-27T20:37:18Z

Doesn't have to be done as part of this PR, but I think it would be really helpful to expand the docs section with more examples of how this config can be used in conjunction with various example connector configs. Maybe a troubleshooting section for connector configs that give examples for how to update the config in response to the most commonly seen error messages when executing actions?

I elaborated on the example a bit, wasn't sure about the best way to show in concert with example connector configs, since that presumably would involve screenshots, at least for this PR.

Also added a section in troubleshooting, basically just pointing to the action config page, referencing TLS errors.

pmuellr · 2021-04-28T14:32:31Z

@elasticmachine merge upstream

kibanamachine · 2021-04-28T16:28:56Z

💚 Build Succeeded

Metrics [docs]

✅ unchanged

History

💚 Build #122823 succeeded 4e9b67c
💚 Build #122535 succeeded 64aa6b7
💔 Build #122419 failed 0eb6e1a
💔 Build #122371 failed 9ad88d7
💔 Build #121102 failed 1f77180

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

…6630) resolves: elastic#80120 Adds a new Kibana configuration key xpack.actions.customHostSettings which allows per-host configuration of connection settings for https and smtp for alerting actions. Initially this is just for TLS settings, expandable to other settings in the future. The purpose of these is to allow customers to provide server certificates for servers accessed by actions, whose certificate authority is not available publicly. Alternatively, a per-server rejectUnauthorized: false configuration may be used to bypass the verification step for specific servers, but require it for other servers that do not have per-host customization. Support was also added to allow per-host customization of ignoreTLS and requireTLS flags for use with the email action.

…98674) resolves: #80120 Adds a new Kibana configuration key xpack.actions.customHostSettings which allows per-host configuration of connection settings for https and smtp for alerting actions. Initially this is just for TLS settings, expandable to other settings in the future. The purpose of these is to allow customers to provide server certificates for servers accessed by actions, whose certificate authority is not available publicly. Alternatively, a per-server rejectUnauthorized: false configuration may be used to bypass the verification step for specific servers, but require it for other servers that do not have per-host customization. Support was also added to allow per-host customization of ignoreTLS and requireTLS flags for use with the email action.

mikecote · 2021-04-30T12:02:55Z

@pmuellr it looks like the docs aren't rendering properly? https://www.elastic.co/guide/en/kibana/current/alert-action-settings-kb.html

…6630) resolves: elastic#80120 Adds a new Kibana configuration key xpack.actions.customHostSettings which allows per-host configuration of connection settings for https and smtp for alerting actions. Initially this is just for TLS settings, expandable to other settings in the future. The purpose of these is to allow customers to provide server certificates for servers accessed by actions, whose certificate authority is not available publicly. Alternatively, a per-server rejectUnauthorized: false configuration may be used to bypass the verification step for specific servers, but require it for other servers that do not have per-host customization. Support was also added to allow per-host customization of ignoreTLS and requireTLS flags for use with the email action.

pmuellr force-pushed the actions/ssl-per-host-config branch 4 times, most recently from 5e68b33 to a74afd4 Compare April 15, 2021 18:44

pmuellr added Feature:Alerting Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) v7.13.0 v8.0.0 labels Apr 15, 2021

pmuellr marked this pull request as ready for review April 15, 2021 18:46

pmuellr requested review from a team as code owners April 15, 2021 18:46

jbudz approved these changes Apr 15, 2021

View reviewed changes

pmuellr force-pushed the actions/ssl-per-host-config branch from a74afd4 to c2bb7d6 Compare April 15, 2021 19:04

pmuellr added the release_note:enhancement label Apr 15, 2021

ymao1 approved these changes Apr 19, 2021

View reviewed changes

YulNaumenko approved these changes Apr 19, 2021

View reviewed changes

Merge branch 'master' into actions/ssl-per-host-config

58dd01e

pmuellr mentioned this pull request Apr 19, 2021

[actions] adds a connectors_networking plugin #94807

Closed

9 tasks

jportner self-requested a review April 19, 2021 16:12

jportner reviewed Apr 19, 2021

View reviewed changes

gchaps reviewed Apr 19, 2021

View reviewed changes

pmuellr added 3 commits April 20, 2021 01:13

add function tests

c430595

fix ts lint error

dcb5b16

change FT config so preconfigured connectors don't have spaces in name

1f77180

spalger added v7.14.0 and removed v7.13.0 labels Apr 21, 2021

pmuellr added 4 commits April 26, 2021 11:11

merge master with resolved merge conflicts

9ad88d7

fix ts errors

0eb6e1a

fix function tests

64aa6b7

address PR comments

4e9b67c

Merge branch 'master' into actions/ssl-per-host-config

91b1691

pmuellr merged commit b31f4a1 into elastic:master Apr 28, 2021

pmuellr mentioned this pull request Apr 28, 2021

[7.x] [actions] adds config allowing per-host networking options (#96630) #98674

Merged

pmuellr added the backported label Apr 28, 2021

pmuellr mentioned this pull request Apr 30, 2021

[Docs] Broken content in Kibana docs #98933

Closed

pmuellr mentioned this pull request May 24, 2021

[Actions] Converted rejectUnauthorized config usages to verificationMode. #100179

Merged

2 tasks

ymao1 mentioned this pull request Jun 22, 2021

[Actions] Rename tls.* configs to ssl.* #102902

Merged

2 tasks

pmuellr mentioned this pull request Jun 22, 2021

[actions] add extensive proxy + email jest tests #102935

Open

	\| A boolean value indicatting that TLS must not be used for this connection.
	\| A boolean value indicating that TLS must not be used for this connection.

	defaults to an empty list. In the example below, a custom host setting for a
	defaults to an empty list. In the following example, a custom host setting for a

	\| A URL associated with this custom host setting. Should be in form
	\| A URL associated with this custom host setting. Should be in the form of

	port is not provided, 443 will be used for `https` and 25 will be used for
	port is not provided, 443 is used for `https` and 25 is used for

	`smtp`. The `smtp` URLs will be used for the Email actions which use this
	`smtp`. The `smtp` URLs are used for the Email actions that use this

	is being made as part of executing an action, only the protocol, hostname and
	is made as part of executing an action, only the protocol, hostname, and

	the files themselves cannot be made available.
	the files cannot be made available.

[actions] adds config allowing per-host networking options #96630

[actions] adds config allowing per-host networking options #96630

Conversation

pmuellr commented Apr 8, 2021 • edited Loading

Summary

Checklist

For maintainers

Remaining To Do's

Release note

elasticmachine commented Apr 15, 2021

pmuellr commented Apr 15, 2021

pmuellr commented Apr 15, 2021

jbudz left a comment

Choose a reason for hiding this comment

ymao1 left a comment

Choose a reason for hiding this comment

pmuellr commented Apr 19, 2021

YulNaumenko left a comment

Choose a reason for hiding this comment

pmuellr commented Apr 19, 2021

pmuellr commented Apr 19, 2021 • edited Loading

jportner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jportner Apr 19, 2021 • 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

jportner Apr 19, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jportner Apr 19, 2021 • edited Loading

Choose a reason for hiding this comment

jportner Apr 19, 2021 • edited Loading

Choose a reason for hiding this comment

pmuellr commented Apr 19, 2021

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pmuellr commented Apr 27, 2021

pmuellr commented Apr 28, 2021

kibanamachine commented Apr 28, 2021

💚 Build Succeeded

Metrics [docs]

History

mikecote commented Apr 30, 2021

pmuellr commented Apr 8, 2021 •

edited

Loading

pmuellr commented Apr 19, 2021 •

edited

Loading

jportner Apr 19, 2021 •

edited

Loading

jportner Apr 19, 2021 •

edited

Loading

jportner Apr 19, 2021 •

edited

Loading

jportner Apr 19, 2021 •

edited

Loading