Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Conversation

YulNaumenko
Copy link
Contributor

@YulNaumenko YulNaumenko commented May 14, 2021

Resolves #97635

Current PR adds new action TLS configuration options, to better match with Kibana security, which uses a node.js agnostic configuration of verificationMode:

  1. Deprecated xpack.actions.rejectUnauthorized in favor of the new option xpack.actions.tls.verificationMode
  2. Deprecated xpack.actions.proxyRejectUnauthorizedCertificates in favor of the new option xpack.actions.tls.proxyVerificationMode.
  3. Deprecated xpack.actions.customHostSettings.tls.rejectUnauthorized in favor of the new option xpack.actions.customHostSettings.tls.verificationMode.

Checklist

Opened follow up issue to remove deprecated configuration option in 8.x version.

@YulNaumenko YulNaumenko self-assigned this May 14, 2021
@YulNaumenko YulNaumenko added v7.14.0 v8.0.0 Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) Feature:Actions release_note:skip Skip the PR/issue when compiling release notes labels May 18, 2021
@YulNaumenko YulNaumenko marked this pull request as ready for review May 18, 2021 19:56
@YulNaumenko YulNaumenko requested review from a team as code owners May 18, 2021 19:56
@elasticmachine
Copy link
Contributor

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

Copy link
Contributor

@tylersmalley tylersmalley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Docker config changes LGTM

@YulNaumenko YulNaumenko requested review from legrego and a team May 18, 2021 19:57
@YulNaumenko YulNaumenko requested review from gchaps and jportner May 18, 2021 19:57
@YulNaumenko
Copy link
Contributor Author

@jportner and @legrego I've added you as a reviewers, because need your thoughts about the naming conventions for the new actions configuration TLS options. (previous comment)

docs/settings/alert-action-settings.asciidoc Outdated Show resolved Hide resolved
docs/settings/alert-action-settings.asciidoc Outdated Show resolved Hide resolved
docs/settings/alert-action-settings.asciidoc Outdated Show resolved Hide resolved
docs/settings/alert-action-settings.asciidoc Outdated Show resolved Hide resolved
docs/settings/alert-action-settings.asciidoc Outdated Show resolved Hide resolved
docs/settings/alert-action-settings.asciidoc Outdated Show resolved Hide resolved
docs/settings/alert-action-settings.asciidoc Outdated Show resolved Hide resolved
docs/settings/alert-action-settings.asciidoc Outdated Show resolved Hide resolved
docs/settings/alert-action-settings.asciidoc Outdated Show resolved Hide resolved
docs/settings/alert-action-settings.asciidoc Outdated Show resolved Hide resolved
Copy link
Contributor

@jportner jportner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just did a quick high-level review, looking good so far.

In addition to the comments below, you may want to consider logging out deprecation warnings when the old settings are detected, so you can let users know they should use the new ones instead!

Edit:

@jportner and @legrego I've added you as a reviewers, because need your thoughts about the naming conventions for the new actions configuration TLS options. (previous comment)

I think the new naming conventions for the config options are great 👍

x-pack/plugins/actions/server/config.ts Outdated Show resolved Hide resolved
x-pack/plugins/actions/server/config.ts Outdated Show resolved Hide resolved
Comment on lines 89 to +92
rejectUnauthorized: schema.boolean({ defaultValue: true }),
tls: schema.maybe(
schema.object({
verificationMode: schema.maybe(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If users can set rejectUnauthorized and verificationMode at the same time, it may lead to confusion because verificationMode take precedence. It may be a better idea to prevent users from setting these both at the same time.

GitHub isn't letting me suggest changes for all of the lines, but I've added a very ugly diff below to explain what I mean!

Expand diff
diff --git a/x-pack/plugins/actions/server/config.ts b/x-pack/plugins/actions/server/config.ts
index dbd0fd18a55..ff03224b7b1 100644
--- a/x-pack/plugins/actions/server/config.ts
+++ b/x-pack/plugins/actions/server/config.ts
@@ -32,87 +32,108 @@ const customHostSettingsSchema = schema.object({
     })
   ),
   tls: schema.maybe(
-    schema.object({
-      /**
-       * @deprecated in favor of `verificationMode`
-       **/
-      rejectUnauthorized: schema.maybe(schema.boolean()),
-      verificationMode: schema.maybe(
-        schema.oneOf(
-          [schema.literal('none'), schema.literal('certificate'), schema.literal('full')],
-          { defaultValue: 'full' }
-        )
-      ),
-      certificateAuthoritiesFiles: schema.maybe(
-        schema.oneOf([
-          schema.string({ minLength: 1 }),
-          schema.arrayOf(schema.string({ minLength: 1 }), { minSize: 1 }),
-        ])
-      ),
-      certificateAuthoritiesData: schema.maybe(schema.string({ minLength: 1 })),
-    })
+    schema.object(
+      {
+        /**
+         * @deprecated in favor of `verificationMode`
+         **/
+        rejectUnauthorized: schema.maybe(schema.boolean()),
+        verificationMode: schema.maybe(
+          schema.oneOf(
+            [schema.literal('none'), schema.literal('certificate'), schema.literal('full')],
+            { defaultValue: 'full' }
+          )
+        ),
+        certificateAuthoritiesFiles: schema.maybe(
+          schema.oneOf([
+            schema.string({ minLength: 1 }),
+            schema.arrayOf(schema.string({ minLength: 1 }), { minSize: 1 }),
+          ])
+        ),
+        certificateAuthoritiesData: schema.maybe(schema.string({ minLength: 1 })),
+      },
+      {
+        validate: ({ rejectUnauthorized, verificationMode }) => {
+          if (rejectUnauthorized !== undefined && verificationMode) {
+            return 'cannot use [rejectUnauthorized] when [verificationMode] is specified; instead, you should only use [verificationMode]';
+          }
+        },
+      }
+    )
   ),
 });
 
 export type CustomHostSettings = TypeOf<typeof customHostSettingsSchema>;
 
-export const configSchema = schema.object({
-  enabled: schema.boolean({ defaultValue: true }),
-  allowedHosts: schema.arrayOf(
-    schema.oneOf([schema.string({ hostname: true }), schema.literal(AllowedHosts.Any)]),
-    {
-      defaultValue: [AllowedHosts.Any],
-    }
-  ),
-  enabledActionTypes: schema.arrayOf(
-    schema.oneOf([schema.string(), schema.literal(EnabledActionTypes.Any)]),
-    {
-      defaultValue: [AllowedHosts.Any],
-    }
-  ),
-  preconfiguredAlertHistoryEsIndex: schema.boolean({ defaultValue: false }),
-  preconfigured: schema.recordOf(schema.string(), preconfiguredActionSchema, {
-    defaultValue: {},
-    validate: validatePreconfigured,
-  }),
-  proxyUrl: schema.maybe(schema.string()),
-  proxyHeaders: schema.maybe(schema.recordOf(schema.string(), schema.string())),
-  /**
-   * @deprecated in favor of `ssl.proxyVerificationMode`
-   **/
-  proxyRejectUnauthorizedCertificates: schema.boolean({ defaultValue: true }),
-  proxyBypassHosts: schema.maybe(schema.arrayOf(schema.string({ hostname: true }))),
-  proxyOnlyHosts: schema.maybe(schema.arrayOf(schema.string({ hostname: true }))),
-  /**
-   * @deprecatedin favor of `ssl.verificationMode`
-   **/
-  rejectUnauthorized: schema.boolean({ defaultValue: true }),
-  tls: schema.maybe(
-    schema.object({
-      verificationMode: schema.maybe(
-        schema.oneOf(
-          [schema.literal('none'), schema.literal('certificate'), schema.literal('full')],
-          { defaultValue: 'full' }
-        )
-      ),
-      proxyVerificationMode: schema.maybe(
-        schema.oneOf(
-          [schema.literal('none'), schema.literal('certificate'), schema.literal('full')],
-          { defaultValue: 'full' }
-        )
-      ),
-    })
-  ),
-  maxResponseContentLength: schema.byteSize({ defaultValue: '1mb' }),
-  responseTimeout: schema.duration({ defaultValue: '60s' }),
-  customHostSettings: schema.maybe(schema.arrayOf(customHostSettingsSchema)),
-  cleanupFailedExecutionsTask: schema.object({
+export const configSchema = schema.object(
+  {
     enabled: schema.boolean({ defaultValue: true }),
-    cleanupInterval: schema.duration({ defaultValue: '5m' }),
-    idleInterval: schema.duration({ defaultValue: '1h' }),
-    pageSize: schema.number({ defaultValue: 100 }),
-  }),
-});
+    allowedHosts: schema.arrayOf(
+      schema.oneOf([schema.string({ hostname: true }), schema.literal(AllowedHosts.Any)]),
+      {
+        defaultValue: [AllowedHosts.Any],
+      }
+    ),
+    enabledActionTypes: schema.arrayOf(
+      schema.oneOf([schema.string(), schema.literal(EnabledActionTypes.Any)]),
+      {
+        defaultValue: [AllowedHosts.Any],
+      }
+    ),
+    preconfiguredAlertHistoryEsIndex: schema.boolean({ defaultValue: false }),
+    preconfigured: schema.recordOf(schema.string(), preconfiguredActionSchema, {
+      defaultValue: {},
+      validate: validatePreconfigured,
+    }),
+    proxyUrl: schema.maybe(schema.string()),
+    proxyHeaders: schema.maybe(schema.recordOf(schema.string(), schema.string())),
+    /**
+     * @deprecated in favor of `ssl.proxyVerificationMode`
+     **/
+    proxyRejectUnauthorizedCertificates: schema.boolean({ defaultValue: true }),
+    proxyBypassHosts: schema.maybe(schema.arrayOf(schema.string({ hostname: true }))),
+    proxyOnlyHosts: schema.maybe(schema.arrayOf(schema.string({ hostname: true }))),
+    /**
+     * @deprecatedin favor of `ssl.verificationMode`
+     **/
+    rejectUnauthorized: schema.boolean({ defaultValue: true }),
+    tls: schema.maybe(
+      schema.object({
+        verificationMode: schema.maybe(
+          schema.oneOf(
+            [schema.literal('none'), schema.literal('certificate'), schema.literal('full')],
+            { defaultValue: 'full' }
+          )
+        ),
+        proxyVerificationMode: schema.maybe(
+          schema.oneOf(
+            [schema.literal('none'), schema.literal('certificate'), schema.literal('full')],
+            { defaultValue: 'full' }
+          )
+        ),
+      })
+    ),
+    maxResponseContentLength: schema.byteSize({ defaultValue: '1mb' }),
+    responseTimeout: schema.duration({ defaultValue: '60s' }),
+    customHostSettings: schema.maybe(schema.arrayOf(customHostSettingsSchema)),
+    cleanupFailedExecutionsTask: schema.object({
+      enabled: schema.boolean({ defaultValue: true }),
+      cleanupInterval: schema.duration({ defaultValue: '5m' }),
+      idleInterval: schema.duration({ defaultValue: '1h' }),
+      pageSize: schema.number({ defaultValue: 100 }),
+    }),
+  },
+  {
+    validate: ({ rejectUnauthorized, proxyRejectUnauthorizedCertificates, tls }) => {
+      if (rejectUnauthorized !== undefined && tls?.verificationMode) {
+        return 'cannot use [rejectUnauthorized] when [tls.verificationMode] is specified; instead, you should only use [tls.verificationMode]';
+      }
+      if (proxyRejectUnauthorizedCertificates !== undefined && tls?.proxyVerificationMode) {
+        return 'cannot use [proxyRejectUnauthorizedCertificates] when [tls.proxyVerificationMode] is specified; instead, you should only use [tls.proxyVerificationMode]';
+      }
+    },
+  }
+);
 
 export type ActionsConfig = TypeOf<typeof configSchema>;
 

Alternatively, you could log a warning like the one used below:

if (proxyBypassHosts && proxyOnlyHosts) {
logger.warn(
'The confgurations xpack.actions.proxyBypassHosts and xpack.actions.proxyOnlyHosts can not be used at the same time. The configuration xpack.actions.proxyOnlyHosts will be ignored.'
);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think, what Larry suggested below, could help to solve this issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jportner do you think we still need this validation with the changes I've provided in the commit?

Copy link
Contributor

@jportner jportner May 20, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, using a deprecation may be a better idea than a regular warning log. But the deprecation warning that's been added mentions that the config option is unused -- that's not quite true, and it also doesn't mention that verificationMode effectively overrides rejectUnauthorized.

That's why I didn't suggest adding a deprecation originally as there isn't a 1:1 mapping so you can't use renameFromRoot() for it, but unused() feels wrong too. So maybe a custom deprecation for each setting is the best course of action. Up to you, really! Thanks for addressing my other feedback!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, that existing deprecation options renameFromRoot() and unused() don't fit well enough for the changes I've done. I will replace it with the custom deprecation function with the proper message.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jportner I've added the commit where switched from unsed() deprecation mode to a custom function with the specific message. Do you think it will enough to explain this to the users?

x-pack/plugins/actions/server/actions_config.ts Outdated Show resolved Hide resolved
@YulNaumenko
Copy link
Contributor Author

@elasticmachine merge upstream

@YulNaumenko YulNaumenko requested a review from jportner May 24, 2021 18:59
Copy link
Contributor

@jportner jportner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great stuff!

Copy link
Member

@pmuellr pmuellr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good! Question about throwing an error for invalid verificationMode value, and made some notes about additional tests we should add.

renameFromRoot('xpack.actions.whitelistedHosts', 'xpack.actions.allowedHosts'),
(settings, fromPath, addDeprecation) => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we have any way of testing these?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also, we already have some functional tests that test at least a bit of this stuff - can't test all the combinations, but I think what we have now is good enough; see #96630 for those tests. We can either add more variants with the new options, or change some of the existing ones to use the new options, or both ...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remembering there are some slightly odd "jest tests" that got added in #96630 which actually start up TLS servers to make sure the configs will actually work with axios. They are more like "function tests" in that they actually do make real requests, but they are heavily mocked, and allow us to run a lot more combinations of options that we can with the actual function tests. In the file x-pack/plugins/actions/server/builtin_action_types/lib/axios_utils_connection.test.ts. We should add some more tests there as well, with the new options ...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pmuellr I've changed integration tests to use the new verificationMode option and added a tests for legacy rejectUnauthorized global configuration. Do you think we need something in addition?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this will be good enough, thx!

@legrego legrego removed their request for review May 25, 2021 17:27
@YulNaumenko YulNaumenko requested a review from pmuellr May 26, 2021 04:47
@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Unknown metric groups

References to deprecated APIs

id before after diff
canvas 29 25 -4
crossClusterReplication 8 6 -2
fleet 22 20 -2
globalSearch 4 2 -2
indexManagement 12 7 -5
infra 261 149 -112
lens 67 45 -22
licensing 18 15 -3
lists 239 236 -3
maps 286 208 -78
ml 121 115 -6
monitoring 109 56 -53
securitySolution 386 342 -44
stackAlerts 101 95 -6
total -342

History

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

cc @YulNaumenko

Copy link
Member

@pmuellr pmuellr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes since my initial review LGTM!

renameFromRoot('xpack.actions.whitelistedHosts', 'xpack.actions.allowedHosts'),
(settings, fromPath, addDeprecation) => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this will be good enough, thx!

@YulNaumenko YulNaumenko merged commit 134a3de into elastic:master May 27, 2021
YulNaumenko added a commit to YulNaumenko/kibana that referenced this pull request May 27, 2021
…onMode`. (elastic#100179)

* [Actions] Converted `rejectUnauthorized` config usages to `verificationMode`.

* added new verificationMode config options for tls, proxy tls and custom hosts

* added unit tests

* added unit tests

* added kibana docker

* Apply suggestions from code review

Co-authored-by: gchaps <[email protected]>

* Update alert-action-settings.asciidoc

* Apply suggestions from code review

Co-authored-by: Joe Portner <[email protected]>

* removed legacyRegectUnauthorized logic from getNodeTLSOptions

* added deprecations

* fixed doc links

* fixed docs

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

Co-authored-by: Joe Portner <[email protected]>

* [DOCS] Fixes build error

* fixed deprecations to set custom message

* fixed doc

* changed to not throw exception on non existing verification mode

* added tests

* fixed tests

* fixed tests

* added integration tests for legacy rejectUnauthorized fale

* fixed tests

Co-authored-by: gchaps <[email protected]>
Co-authored-by: Joe Portner <[email protected]>
Co-authored-by: lcawl <[email protected]>
Co-authored-by: Kibana Machine <[email protected]>
YulNaumenko added a commit that referenced this pull request May 27, 2021
…onMode`. (#100179) (#100830)

* [Actions] Converted `rejectUnauthorized` config usages to `verificationMode`.

* added new verificationMode config options for tls, proxy tls and custom hosts

* added unit tests

* added unit tests

* added kibana docker

* Apply suggestions from code review

Co-authored-by: gchaps <[email protected]>

* Update alert-action-settings.asciidoc

* Apply suggestions from code review

Co-authored-by: Joe Portner <[email protected]>

* removed legacyRegectUnauthorized logic from getNodeTLSOptions

* added deprecations

* fixed doc links

* fixed docs

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

Co-authored-by: Joe Portner <[email protected]>

* [DOCS] Fixes build error

* fixed deprecations to set custom message

* fixed doc

* changed to not throw exception on non existing verification mode

* added tests

* fixed tests

* fixed tests

* added integration tests for legacy rejectUnauthorized fale

* fixed tests

Co-authored-by: gchaps <[email protected]>
Co-authored-by: Joe Portner <[email protected]>
Co-authored-by: lcawl <[email protected]>
Co-authored-by: Kibana Machine <[email protected]>

Co-authored-by: gchaps <[email protected]>
Co-authored-by: Joe Portner <[email protected]>
Co-authored-by: lcawl <[email protected]>
Co-authored-by: Kibana Machine <[email protected]>
Copy link
Member

@legrego legrego left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@YulNaumenko sorry for the post-merge review, but can we change the .tls. config key portion to .ssl.? So xpack.actions.customHostSettings.tls.verificationMode for example would become xpack.actions.customHostSettings.ssl.verificationMode.

I know that tls is perhaps more accurate, but ssl is the standard config key across all of our stack components, and I think consistency would be preferred over technical correctness in this specific case.

@YulNaumenko
Copy link
Contributor Author

@YulNaumenko sorry for the post-merge review, but can we change the .tls. config key portion to .ssl.? So xpack.actions.customHostSettings.tls.verificationMode for example would become xpack.actions.customHostSettings.ssl.verificationMode.

I know that tls is perhaps more accurate, but ssl is the standard config key across all of our stack components, and I think consistency would be preferred over technical correctness in this specific case.

@legrego I opened a follow up issue on you comment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Actions release_note:skip Skip the PR/issue when compiling release notes Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) v7.14.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[actions] convert rejectUnauthorized config usages to verificationMode
9 participants