Skip to content

Conversation

@mikecote
Copy link
Contributor

@mikecote mikecote commented Jan 19, 2021

Resolves #87047

In this PR, I'm doing a few changes to fix the problem. The general purpose of each commit is as follows:

  1. 78b7e2e: I'm removing the TS type ActionsConfigType because it is a duplicate / subset of ActionsConfig that wasn't needed. This allowed me to add more functionality to the configurationUtilities in following commits.
  2. 08ebe63: This commit fixes the problem by passing configurationUtilities down to the getProxyAgents function from every possible path. The configurationUtilities has a new function isRejectUnauthorizedCertificatesEnabled .
  3. 1a17f8d: Since passing proxySettings was happening at the same places that configurationUtilities was, I moved the access to proxySettings to configurationUtilities.
  4. a84503d: I realized checking error.isAxiosError didn't always work (ex: when it mentions missing kbn-xsrf header) so I changed it to error.code.

@mikecote mikecote added v8.0.0 release_note:skip Skip the PR/issue when compiling release notes Feature:Actions Team:ResponseOps Platform ResponseOps team (formerly the Cases and Alerting teams) t// v7.12.0 labels Jan 19, 2021
@mikecote mikecote self-assigned this Jan 19, 2021
@mikecote mikecote marked this pull request as ready for review January 20, 2021 13:10
@mikecote mikecote requested a review from a team as a code owner January 20, 2021 13:10
@elasticmachine
Copy link
Contributor

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

Copy link
Contributor

@YulNaumenko YulNaumenko left a comment

Choose a reason for hiding this comment

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

LGTM

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.

LGTM

Seems like we could build a function test for this - create a new https server with a newly created self-signed cert (with a loooong lifetime), that just has one endpoint that we can test with webhook. Then execute the webhook, with and without the rejectUnauthorized setting - should get one to work and the other to fail. Obviously set it up to use the proxy we're testing with, in security_and_spaces . :-)

@mikecote
Copy link
Contributor Author

@pmuellr I went ahead and added a functional test in this commit (af638a3). I made it re-use the dev certs so it's one less thing to expire 🙂 I added it to the spaces_only suite because the fix is for non-proxy scenarios (the other suite uses a proxy for everything). Let me know what you think of the approach? 🙏

@mikecote
Copy link
Contributor Author

@elasticmachine merge upstream

@mikecote
Copy link
Contributor Author

@elasticmachine merge upstream

@mikecote mikecote added release_note:fix and removed release_note:skip Skip the PR/issue when compiling release notes labels Jan 27, 2021
@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

✅ unchanged

History

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

@mikecote mikecote merged commit da8ce37 into elastic:master Jan 28, 2021
mikecote added a commit to mikecote/kibana that referenced this pull request Jan 28, 2021
* Remove ActionsConfigType due to being a duplicate

* Fix rejectUnauthorized not being configured

* Move proxySettings to configurationUtilities

* Fix isAxiosError check to code

* Add functional test

* Remove comment

* Close webhook server

Co-authored-by: Kibana Machine <[email protected]>
mikecote added a commit to mikecote/kibana that referenced this pull request Jan 28, 2021
* Remove ActionsConfigType due to being a duplicate

* Fix rejectUnauthorized not being configured

* Move proxySettings to configurationUtilities

* Fix isAxiosError check to code

* Add functional test

* Remove comment

* Close webhook server

Co-authored-by: Kibana Machine <[email protected]>
mikecote added a commit that referenced this pull request Jan 29, 2021
* Remove ActionsConfigType due to being a duplicate

* Fix rejectUnauthorized not being configured

* Move proxySettings to configurationUtilities

* Fix isAxiosError check to code

* Add functional test

* Remove comment

* Close webhook server

Co-authored-by: Kibana Machine <[email protected]>

Co-authored-by: Kibana Machine <[email protected]>
mikecote added a commit that referenced this pull request Jan 29, 2021
* Remove ActionsConfigType due to being a duplicate

* Fix rejectUnauthorized not being configured

* Move proxySettings to configurationUtilities

* Fix isAxiosError check to code

* Add functional test

* Remove comment

* Close webhook server

Co-authored-by: Kibana Machine <[email protected]>

Co-authored-by: Kibana Machine <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Feature:Actions release_note:fix Team:ResponseOps Platform ResponseOps team (formerly the Cases and Alerting teams) t// v7.11.0 v7.12.0 v8.0.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

xpack.actions.rejectUnauthorized setting does not work

5 participants