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.

Sign up for GitHub

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

Jump to bottom

Make xpack.actions.rejectUnauthorized setting work #88690

Merged
merged 15 commits into from
Jan 28, 2021
Merged

Make xpack.actions.rejectUnauthorized setting work #88690

merged 15 commits into from
Jan 28, 2021

Conversation

mikecote
Copy link
Contributor

@mikecote mikecote commented Jan 19, 2021 •
edited
Loading

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 Label for the ResponseOps team (formerly the Cases and Alerting teams) 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)

YulNaumenko
YulNaumenko approved these changes Jan 20, 2021
View reviewed changes
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

pmuellr
pmuellr approved these changes Jan 20, 2021
View reviewed changes
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 mikecote mentioned this pull request Jan 28, 2021
Merged
mikecote added a commit to mikecote/kibana that referenced this pull request Jan 28, 2021
@mikecote @kibanamachine
Make xpack.actions.rejectUnauthorized setting work (elastic#88690)
b8fee64 
* 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 <42973632+kibanamachine@users.noreply.github.com>
mikecote added a commit to mikecote/kibana that referenced this pull request Jan 28, 2021
@mikecote @kibanamachine
Make xpack.actions.rejectUnauthorized setting work (elastic#88690)
6ff3bc2 
* 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 <42973632+kibanamachine@users.noreply.github.com>
@mikecote mikecote mentioned this pull request Jan 28, 2021
Merged
mikecote added a commit that referenced this pull request Jan 29, 2021
@mikecote @kibanamachine
Make xpack.actions.rejectUnauthorized setting work (#88690) (#89618)
9647898 
* 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 <42973632+kibanamachine@users.noreply.github.com>

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
mikecote added a commit that referenced this pull request Jan 29, 2021
@mikecote @kibanamachine
Make xpack.actions.rejectUnauthorized setting work (#88690) (#89619)
3ea5c2d 
* 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 <42973632+kibanamachine@users.noreply.github.com>

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
@mikecote mikecote mentioned this pull request Jan 30, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pmuellr pmuellr pmuellr approved these changes

@YulNaumenko YulNaumenko YulNaumenko approved these changes

Assignees

@mikecote mikecote

Labels
Feature:Actions release_note:fix Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) v7.11.0 v7.12.0 v8.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

xpack.actions.rejectUnauthorized setting does not work
5 participants
@mikecote @elasticmachine @kibanamachine @pmuellr @YulNaumenko