[RAM] Add support for self-signed SSL certificate auth in webhook connector by Zacqary · Pull Request #161894 · elastic/kibana

Zacqary · 2023-07-13T17:16:34Z

Summary

Adds UI for CA and client-side SSL certificate auth to webhook connector, and adds these properties to the webhook's HttpsAgent on execution:

When Editing

Also creates a Field helper for EuiFilePicker

Checklist

Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
Unit or functional tests were updated or added to match the most common scenarios

ghost · 2023-07-13T17:16:52Z

🤖 GitHub comments

Expand to view the GitHub comments

Just comment with:

/oblt-deploy : Deploy a Kibana instance using the Observability test environments.
run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

…signed

…fix'

…signed # Conflicts: # x-pack/test/detection_engine_api_integration/security_and_spaces/group1/export_rules.ts

…y/kibana into 160812-webhook-selfsigned

…-ref HEAD~1..HEAD --fix'

cnasikas · 2023-07-26T08:23:12Z

Unfortunately no. HTML File inputs cannot be set to anything manually, it's a browser security issue. We might be able to modify the EUI File input element to display some kind of a fake placeholder file but that's a deeper EUI level change and out of scope for this issue.

Make sense. A solution would be to hide the file picker if there is a CA and show our own custom component (unrelated to file inputs) where the user can see a fake file name and an "X" icon to remove the CA. Pressing the "X" will show the file picker. Nevertheless, I don't think is ideal and it may be more confusing. We can leave it as it is and enhance it later if needed.

I allowed this so that the user is able to select Verification Mode: none without adding a CA file, but I can require the CA file if the verification mode is set to certificate or full

I found it confusing to press the Add certificate authority and then set the Verification mode to None. Does having the Add certificate authority set to off equals not uploading a CA file + setting the Verification mode to None? If yes, then I think we should not allow users to choose None and require a CA file.

Zacqary · 2023-07-26T16:20:46Z

Does having the Add certificate authority set to off equals not uploading a CA file + setting the Verification mode to None?

No it defaults to Full

…signed # Conflicts: # x-pack/test/tsconfig.json

…y/kibana into 160812-webhook-selfsigned

pmuellr

Not finished reviewing, but I hit a few things early that I think we'll need to get resolved.

undefined-able values in config/secrets
hasAuth: false vs authType: None

We also need to get tests added, similar to what is currently in https://github.com/elastic/kibana/blob/main/x-pack/plugins/actions/server/integration_tests/axios_utils_connection.test.ts - would be fine to re-org that if it's getting a bit big - perhaps even just have tests for the new combinations of ssl-ish options in a separate module.

pmuellr · 2023-07-13T17:41:03Z

 const secretSchemaProps = {
-  user: schema.nullable(schema.string()),
-  password: schema.nullable(schema.string()),
+  user: schema.maybe(schema.string()),


I don't think we can use maybe() here - unless we finally fixed the partial update issue, but I'd guess not. Anything in config / schema can never be undefined. schema.nullable() accepts an undefined value (or value not preset for the property), and return null.

See: https://docs.elastic.dev/reops/developer-guide/best-practices-connectors#undefined-config--secrets--params-properties

pmuellr · 2023-07-25T19:14:25Z

-  const agentSSLOptions = getNodeSSLOptions(logger, generalSSLSettings.verificationMode);
+  const agentSSLOptions = getNodeSSLOptions(
+    logger,
+    sslOverrides?.verificationMode ?? generalSSLSettings.verificationMode,


Yes, the webhook settings should override kibana.yml settings.

I'm wondering if there's cases where somehow they would be mixed - would that make sense? Is it even possible? I'd think we wouldn't want that, as it might be difficult to figure out what settings would actually be used.

pmuellr · 2023-07-25T20:08:36Z

@@ -54,6 +55,25 @@ const configSchemaProps = {
  }),
  headers: nullableType(HeadersSchema),
  hasAuth: schema.boolean({ defaultValue: true }),


What is the intention of having both hasAuth - which was the way we determined whether to require a user/pass - and authType: NONE? I assume they "mean" the same thing. But if we leave it like this, there are lots of potential combinations we'll have to verify; hasAuth: false and authType: Basic should be an error, right?

Would it be better to not have authType: NONE at all? Cuts down on some of the invalid combinations. So the validation would be !hasAuth && !authType or hasAuth && authType, or whatever.

hasAuth seems to be a property common to several connector types, attempting to remove it broke a lot of tests at a deeper level.

It would also be a major breaking change for existing connectors, I'm not quite sure how to handle that.

It looks like hasAuth used by email and cases, but should just be a config, so independent of any other connectors.

But I'm not suggesting we remove it - though that IS a possibility - change 2-state hasAuth to 3-state authType. That would require a migration, so ... not a happy path for me :-)

My suggestion is to remove the NONE choice of authType. Since that's already covered by hasAuth. So you could only set authType if hasAuth is true, otherwise it would need to be null.

Had some difficulty getting the form to correctly load saved connectors with hasAuth: false but it should be working now. Connector API will now throw an error if hasAuth is false and authType is anything but null or undefined.

spong

Security Solution changes LGTM! Thanks for the added auth support here @Zacqary 🙂 🎉

…icationmode none selection

…signed # Conflicts: # x-pack/test/tsconfig.json

… for pfx

…signed

pmuellr

I'm no expert on client-side certs, but seems like the integration_tests that got added need to be changed to be able to start the server in a mode where it requires the client-side certs.

pmuellr · 2023-08-02T20:42:31Z

+    });
+
+    test('it works with cert, key, and ca in SSL overrides', async () => {
+      const { url, server } = await createServer({ useHttps: true });


I think this isn't going to test the client-side certificate stuff. We'll need to add another option to createServer() to have it pass requestCert in the TLS options when creating the server. Otherwise, the server is just going to ignore those.

To make sure this is working, we should have a negative test as well. Make a request to a client-side enabled server WITHOUT any of the client-side stuff, and it should fail. If we had that test in now, presumably the request would succeed, so the test would fail.

Adding requestCert and getting socket hang up errors. This seems like what Node does when you're sending it an invalid client certificate but I'm not sure, docs are very unclear. I'm still investigating.

Figured it out, server wasn't being configured with the Kibana CA by default. Tests are pushed and working now.

kibana-ci · 2023-08-03T12:51:46Z

💚 Build Succeeded

Buildkite Build
Commit: bd16912

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`esUiShared`	230	232	+2
`stackConnectors`	203	204	+1
total			+3

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id	before	after	diff
`stackConnectors`	439.3KB	447.6KB	+8.2KB

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id	before	after	diff
`esUiShared`	154.8KB	156.4KB	+1.6KB

History

💛 Build #146892 was flaky 42c9de5
💔 Build #146874 failed f7a17c1
💚 Build #146611 succeeded 095b295
💔 Build #146550 failed 5e1b5f9
💚 Build #145007 succeeded 5657773
💚 Build #144786 succeeded a2f2adf

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

pmuellr

LGTM

Realized we should probably have a function test for this as well, in https://github.com/elastic/kibana/blob/main/x-pack/test/alerting_api_integration/spaces_only/tests/actions/connector_types/stack/webhook.ts - we don't have to test all the possibilities, but it would be good to test all the way through with at least one set of good parameters - and probably a negative test as well for the client-side cert (don't send one on the request).

Fine to do this is as a follow-on PR before feature freeze ...

Zacqary Xeper added 4 commits July 10, 2023 13:53

[RAM] Add UI for webhook self-signed certificates

82c52c3

Add self-signed cert to webhook connector SSL options

619ead3

Fix verificationMode

a17d982

Update consts and jest tests

80c6999

Zacqary added Team:ResponseOps Platform ResponseOps team (formerly the Cases and Alerting teams) t// release_note:feature Makes this part of the condensed release notes Feature:Alerting/RulesManagement Issues related to the Rules Management UX v8.10.0 labels Jul 13, 2023

Zacqary Xeper and others added 21 commits July 13, 2023 13:10

Merge remote-tracking branch 'upstream/main' into 160812-webhook-self…

1af51a0

…signed

Use sslOverrides instead of reassigning getSSLSettings

3d399bc

Add CA upload

417cb62

Convert authentication UI to card

18853c7

Update webhook without hasAuth

4f3ae01

Fix jest

6644db5

Fix typecheck

6c579b1

Reenable hasAuth, add deprecated comment

5879cb7

Add hasAuth back as hidden field

c681a2f

Fix FTR

a0645b5

[CI] Auto-commit changed files from 'node scripts/lint_ts_projects --…

22dc435

…fix'

Merge remote-tracking branch 'upstream/main' into 160812-webhook-self…

242b764

…signed # Conflicts: # x-pack/test/detection_engine_api_integration/security_and_spaces/group1/export_rules.ts

Merge branch '160812-webhook-selfsigned' of https://github.com/Zacqar…

41db08c

…y/kibana into 160812-webhook-selfsigned

Fix typecheck

1e5918c

Fix jest

e199175

Fix FTR

d617749

Fix bulk actions test

ba4de0a

Restore required hasAuth

5cfcd39

Fix jest 14

7970bca

Fix jest

be54a55

Add CA to webhook validation test

9aa5381

Zacqary Xeper and others added 3 commits July 25, 2023 12:27

Fix FTR 20

80a1352

Fix FTR 15

e0eaf0f

[CI] Auto-commit changed files from 'node scripts/precommit_hook.js -…

a2f2adf

…-ref HEAD~1..HEAD --fix'

Zacqary Xeper added 2 commits July 26, 2023 11:21

Merge remote-tracking branch 'upstream/main' into 160812-webhook-self…

2e87a42

…signed # Conflicts: # x-pack/test/tsconfig.json

Merge branch '160812-webhook-selfsigned' of https://github.com/Zacqar…

5657773

…y/kibana into 160812-webhook-selfsigned

pmuellr requested changes Jul 26, 2023

View reviewed changes

spong approved these changes Jul 28, 2023

View reviewed changes

Zacqary Xeper added 9 commits August 1, 2023 11:38

Remove webhooReplace webhook-authentication-none with null, fix verif…

0648780

…icationmode none selection

Merge remote-tracking branch 'upstream/main' into 160812-webhook-self…

1c56050

…signed # Conflicts: # x-pack/test/tsconfig.json

Validate empty authType when hasAuth is false

9dba031

Add comment on basicAuth

51445e6

Clarify ternary on basicAuth

5e1b5f9

Fix typecheck

724c3d9

Fix tests

095b295

Add axios integration tests for SSL overrides, allow .p12 file format…

f7a17c1

… for pfx

Merge remote-tracking branch 'upstream/main' into 160812-webhook-self…

42c9de5

…signed

pmuellr requested changes Aug 2, 2023

View reviewed changes

Zacqary Xeper added 3 commits August 3, 2023 07:38

Add integration tests for SSL client auth

be9b446

Add test for empty cert

99bdd06

Clarify test description

bd16912

pmuellr approved these changes Aug 3, 2023

View reviewed changes

Zacqary merged commit 7e2ba48 into elastic:main Aug 3, 2023

kibanamachine added the backport:skip This PR does not require backporting label Aug 3, 2023

Zacqary mentioned this pull request Aug 3, 2023

[RAM] Add functional test for SSL webhook authentication #163112

Open

shanisagiv1 mentioned this pull request Oct 18, 2023

OIDC authentication support for Kibana actions #88081

Open

shanisagiv1 mentioned this pull request Apr 8, 2024

Support self-signed SSL certificate auth in Case webhook connector #180255

Closed

jcger mentioned this pull request Oct 7, 2026

[Actions] Global certificate authority setting for connectors (usable on ECE / Elastic Cloud) #295848

Open

Conversation

Zacqary commented Jul 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

When Editing

Checklist

Uh oh!

ghost commented Jul 13, 2023

🤖 GitHub comments

Uh oh!

cnasikas commented Jul 26, 2023

Uh oh!

Zacqary commented Jul 26, 2023

Uh oh!

pmuellr left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

spong left a comment

Choose a reason for hiding this comment

Uh oh!

pmuellr left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kibana-ci commented Aug 3, 2023

💚 Build Succeeded

Metrics [docs]

Module Count

Async chunks

Page load bundle

History

Uh oh!

pmuellr left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

Zacqary commented Jul 13, 2023 •

edited

Loading