-
Notifications
You must be signed in to change notification settings - Fork 8.1k
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
Change all connectors to use the basic auth header instead of the auth
property of axios
#183162
Conversation
auth
property of axios
}; | ||
|
||
export const getBasicAuthHeader = ({ username, password }: GetBasicAuthHeaderArgs) => { | ||
const header = `Basic ${Buffer.from(`${username}:${password}`).toString('base64')}`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we encode the username or the password before passing it to Buffer.from
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think so. There aren't many options that seem like they would make sense anyway: ascii
, utf8
, utf16le
, ucs2
, latin1
, and binary
. It uses utf8
by default, which seems right since that's how we store the data ...
/ci |
Pinging @elastic/response-ops (Team:ResponseOps) |
@elasticmachine merge upstream |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, noticed one test name that I think needs to be changed.
Also not sure whether this PR should include the changes for axios / follow-redirects. I figured that would be done separately, later, but I'm not sure. I'm curious if these versions were needed for the PR, or maybe you were just testing with them.
}; | ||
|
||
export const getBasicAuthHeader = ({ username, password }: GetBasicAuthHeaderArgs) => { | ||
const header = `Basic ${Buffer.from(`${username}:${password}`).toString('base64')}`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think so. There aren't many options that seem like they would make sense anyway: ascii
, utf8
, utf16le
, ucs2
, latin1
, and binary
. It uses utf8
by default, which seems right since that's how we store the data ...
}); | ||
}); | ||
|
||
it('overrides the auth header if provided', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it('overrides the auth header if provided', () => { | |
it('does not override the auth header if provided', () => { |
I'll leave that up to @jeramysoucy ... since Fleet also uses axios, I'm guessing we will want to do some coordination ... |
It looks like the only
I do, however, see a use case in the CI stats performance metrics APM client. But I don't think this would be subject to a redirect. @cnasikas I don't necessarily want to hold up merging of this PR. So if you are complete with all of your testing, I can follow up with fleet and ops separately. Otherwise I am happy to ping them here if we can get a quick answer and merge everything at the same time. What do you think? |
Ty @jeramysoucy! We are not ready with the testing yet. I would say to ping them if you don't mind and merge everything at the same time. |
I confirmed with ops that we do not need to consider their use case:
|
Also, confirmed this with Fleet.
So, I think we are all set to apply the upgrade in this PR along with your changes. |
@jeramysoucy Amazing! Thanks! |
@elasticmachine merge upstream |
All connectors were tested successfully. Details: #182391 (comment) |
💚 Build Succeeded
Metrics [docs]Public APIs missing comments
Async chunks
Canvas Sharable Runtime
History
To update your PR or re-run it, just comment with: cc @cnasikas |
@cnasikas What is the risk/effort to backport this to 8.14 and 7.17? I assume 8.14 wouldn't be too bad, but 7.17 could be considerably more risky. |
I do not see any risk for 8.14 and the effort should be low. For 7.17, the effort should be big because a lot has changed to all of these connectors. We may need to do it all over again. About the risk, I am not sure tbh. Let me give it a try for both and I will let you know. |
💚 All backports created successfully
Note: Successful backport PRs will be merged automatically after passing CI. Questions ?Please refer to the Backport tool documentation |
@jeramysoucy PR for 8.14 #184091. I tried for 7.17 but the conflicts are too many. We would have to do it manually again. Is it something you would like us to do? |
…th` property of `axios` (elastic#183162) ## Summary Fixes: elastic#182391 ## Framework changes - Utils to construct basic header from username and password: [`fad6bde` (elastic#183162)](elastic@fad6bde), [`b10d103` (elastic#183162)](elastic@b10d103) - Automatically convert `auth` to basic auth header in the sub-actions framework: [`ee27353` (elastic#183162)](elastic@ee27353) - Automatically convert `auth` to basic auth header in axios utils: [`94753a7` (elastic#183162)](elastic@94753a7) ## Jira Commit: [`c366163` (elastic#183162)](elastic@c366163) ## All ServiceNow connectors Commit: [`4324d93` (elastic#183162)](elastic@4324d93) ## IBM Resilient IBM Resilient already uses the basic auth headers. PR elastic#180561 added this functionality. The connector was manually tested when reviewing the PR. In [`7d9edab` (elastic#183162)](elastic@7d9edab) I updated the connector to use the new util function. ## Webhook Commit: [`1a62c77` (elastic#183162)](elastic@1a62c77) ## Cases webhook Commit: [`104f881` (elastic#183162)](elastic@104f881) ## xMatters Commit: [`ea7be2b` (elastic#183162)](elastic@ea7be2b) ## Connectors that do not use the `axios` `auth` property - D3Security - Email - Microsoft Teams - OpenAI - Opsgenie - PagerDuty - Sentinel One - Slack - Slack API - Swimlane - Tines - Torq ### Checklist Delete any items that are not applicable to this PR. - [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios - [ ] [Flaky Test Runner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1) was used on any tests changed ### Risk Matrix Delete this section if it is not applicable to this PR. Before closing this PR, invite QA, stakeholders, and other developers to identify risks that should be tested prior to the change/feature release. When forming the risk matrix, consider some of the following examples and how they may potentially impact the change: | Risk | Probability | Severity | Mitigation/Notes | |---------------------------|-------------|----------|-------------------------| | Connectors not working correctly | Low | High | Unit test and manual testing of all connectors affected | ### For maintainers - [x] This was checked for breaking API changes and was [labeled appropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process) --------- Co-authored-by: “jeramysoucy” <jeramy.soucy@elastic.co> Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> (cherry picked from commit 4b7d014) # Conflicts: # x-pack/plugins/stack_connectors/server/connector_types/resilient/resilient.ts
…the `auth` property of `axios` (#183162) (#184091) # Backport This will backport the following commits from `main` to `8.14`: - [Change all connectors to use the basic auth header instead of the `auth` property of `axios` (#183162)](#183162) <!--- Backport version: 8.9.8 --> ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport) <!--BACKPORT [{"author":{"name":"Christos Nasikas","email":"christos.nasikas@elastic.co"},"sourceCommit":{"committedDate":"2024-05-17T11:18:01Z","message":"Change all connectors to use the basic auth header instead of the `auth` property of `axios` (#183162)\n\n## Summary\r\n\r\nFixes: #182391 Framework changes\r\n\r\n- Utils to construct basic header from username and password: [`fad6bde`\r\n(#183162)](https://github.com/elastic/kibana/pull/183162/commits/fad6bde6afb1302c8629da47173abcdf41a1a602),\r\n[`b10d103`\r\n(#183162)](https://github.com/elastic/kibana/pull/183162/commits/b10d103bd92fd17c77b97a6e014832ac1c6a9bdc)\r\n- Automatically convert `auth` to basic auth header in the sub-actions\r\nframework: [`ee27353`\r\n(#183162)](https://github.com/elastic/kibana/pull/183162/commits/ee2735305142cbb84a930e3dc36e7a6d9116bab6)\r\n- Automatically convert `auth` to basic auth header in axios utils:\r\n[`94753a7`\r\n(#183162)](https://github.com/elastic/kibana/pull/183162/commits/94753a7342595c0e2ee33c8f6620661d73526990)\r\n\r\n## Jira\r\n\r\nCommit: [`c366163`\r\n(#183162)](https://github.com/elastic/kibana/pull/183162/commits/c366163486a516f1d2d62b63d3bde171b6643e19)\r\n\r\n## All ServiceNow connectors\r\n\r\nCommit: [`4324d93`\r\n(#183162)](https://github.com/elastic/kibana/pull/183162/commits/4324d931f7bcacfd8b2d7b4eaa9c40562dc22c52)\r\n\r\n## IBM Resilient\r\n\r\nIBM Resilient already uses the basic auth headers. PR\r\nhttps://github.com//pull/180561 added this functionality.\r\nThe connector was manually tested when reviewing the PR.\r\n\r\nIn [`7d9edab`\r\n(#183162)](https://github.com/elastic/kibana/pull/183162/commits/7d9edabd6e8454ebc2190bfc7f565c8c345f47f5)\r\nI updated the connector to use the new util function.\r\n\r\n## Webhook\r\n\r\nCommit: [`1a62c77`\r\n(#183162)](https://github.com/elastic/kibana/pull/183162/commits/1a62c77d46dd40e9529ae102a6db3fc1513c494e)\r\n\r\n## Cases webhook\r\n\r\nCommit: [`104f881`\r\n(#183162)](https://github.com/elastic/kibana/pull/183162/commits/104f8812515f5944bc103fa0142e55a0b0350e84)\r\n\r\n## xMatters\r\n\r\nCommit: [`ea7be2b`\r\n(#183162)](https://github.com/elastic/kibana/pull/183162/commits/ea7be2bbee89b71c3855769fc480a013e4020732)\r\n\r\n## Connectors that do not use the `axios` `auth` property\r\n\r\n- D3Security\r\n- Email\r\n- Microsoft Teams\r\n- OpenAI\r\n- Opsgenie\r\n- PagerDuty\r\n- Sentinel One\r\n- Slack\r\n- Slack API\r\n- Swimlane\r\n- Tines\r\n- Torq\r\n\r\n### Checklist\r\n\r\nDelete any items that are not applicable to this PR.\r\n\r\n- [x] [Unit or functional\r\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\r\nwere updated or added to match the most common scenarios\r\n- [ ] [Flaky Test\r\nRunner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1) was\r\nused on any tests changed\r\n\r\n### Risk Matrix\r\n\r\nDelete this section if it is not applicable to this PR.\r\n\r\nBefore closing this PR, invite QA, stakeholders, and other developers to\r\nidentify risks that should be tested prior to the change/feature\r\nrelease.\r\n\r\nWhen forming the risk matrix, consider some of the following examples\r\nand how they may potentially impact the change:\r\n\r\n| Risk | Probability | Severity | Mitigation/Notes |\r\n\r\n|---------------------------|-------------|----------|-------------------------|\r\n| Connectors not working correctly | Low | High | Unit test and manual\r\ntesting of all connectors affected |\r\n\r\n\r\n### For maintainers\r\n\r\n- [x] This was checked for breaking API changes and was [labeled\r\nappropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)\r\n\r\n---------\r\n\r\nCo-authored-by: “jeramysoucy” <jeramy.soucy@elastic.co>\r\nCo-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>","sha":"4b7d0145827925a297d1e1728e4447ecc4673554","branchLabelMapping":{"^v8.15.0$":"main","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["bug","release_note:skip","backport:skip","Team:ResponseOps","Feature:Actions/ConnectorTypes","ci:build-cloud-image","ci:build-serverless-image","v8.15.0"],"number":183162,"url":"#183162 all connectors to use the basic auth header instead of the `auth` property of `axios` (#183162)\n\n## Summary\r\n\r\nFixes: #182391 Framework changes\r\n\r\n- Utils to construct basic header from username and password: [`fad6bde`\r\n(#183162)](https://github.com/elastic/kibana/pull/183162/commits/fad6bde6afb1302c8629da47173abcdf41a1a602),\r\n[`b10d103`\r\n(#183162)](https://github.com/elastic/kibana/pull/183162/commits/b10d103bd92fd17c77b97a6e014832ac1c6a9bdc)\r\n- Automatically convert `auth` to basic auth header in the sub-actions\r\nframework: [`ee27353`\r\n(#183162)](https://github.com/elastic/kibana/pull/183162/commits/ee2735305142cbb84a930e3dc36e7a6d9116bab6)\r\n- Automatically convert `auth` to basic auth header in axios utils:\r\n[`94753a7`\r\n(#183162)](https://github.com/elastic/kibana/pull/183162/commits/94753a7342595c0e2ee33c8f6620661d73526990)\r\n\r\n## Jira\r\n\r\nCommit: [`c366163`\r\n(#183162)](https://github.com/elastic/kibana/pull/183162/commits/c366163486a516f1d2d62b63d3bde171b6643e19)\r\n\r\n## All ServiceNow connectors\r\n\r\nCommit: [`4324d93`\r\n(#183162)](https://github.com/elastic/kibana/pull/183162/commits/4324d931f7bcacfd8b2d7b4eaa9c40562dc22c52)\r\n\r\n## IBM Resilient\r\n\r\nIBM Resilient already uses the basic auth headers. PR\r\nhttps://github.com//pull/180561 added this functionality.\r\nThe connector was manually tested when reviewing the PR.\r\n\r\nIn [`7d9edab`\r\n(#183162)](https://github.com/elastic/kibana/pull/183162/commits/7d9edabd6e8454ebc2190bfc7f565c8c345f47f5)\r\nI updated the connector to use the new util function.\r\n\r\n## Webhook\r\n\r\nCommit: [`1a62c77`\r\n(#183162)](https://github.com/elastic/kibana/pull/183162/commits/1a62c77d46dd40e9529ae102a6db3fc1513c494e)\r\n\r\n## Cases webhook\r\n\r\nCommit: [`104f881`\r\n(#183162)](https://github.com/elastic/kibana/pull/183162/commits/104f8812515f5944bc103fa0142e55a0b0350e84)\r\n\r\n## xMatters\r\n\r\nCommit: [`ea7be2b`\r\n(#183162)](https://github.com/elastic/kibana/pull/183162/commits/ea7be2bbee89b71c3855769fc480a013e4020732)\r\n\r\n## Connectors that do not use the `axios` `auth` property\r\n\r\n- D3Security\r\n- Email\r\n- Microsoft Teams\r\n- OpenAI\r\n- Opsgenie\r\n- PagerDuty\r\n- Sentinel One\r\n- Slack\r\n- Slack API\r\n- Swimlane\r\n- Tines\r\n- Torq\r\n\r\n### Checklist\r\n\r\nDelete any items that are not applicable to this PR.\r\n\r\n- [x] [Unit or functional\r\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\r\nwere updated or added to match the most common scenarios\r\n- [ ] [Flaky Test\r\nRunner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1) was\r\nused on any tests changed\r\n\r\n### Risk Matrix\r\n\r\nDelete this section if it is not applicable to this PR.\r\n\r\nBefore closing this PR, invite QA, stakeholders, and other developers to\r\nidentify risks that should be tested prior to the change/feature\r\nrelease.\r\n\r\nWhen forming the risk matrix, consider some of the following examples\r\nand how they may potentially impact the change:\r\n\r\n| Risk | Probability | Severity | Mitigation/Notes |\r\n\r\n|---------------------------|-------------|----------|-------------------------|\r\n| Connectors not working correctly | Low | High | Unit test and manual\r\ntesting of all connectors affected |\r\n\r\n\r\n### For maintainers\r\n\r\n- [x] This was checked for breaking API changes and was [labeled\r\nappropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)\r\n\r\n---------\r\n\r\nCo-authored-by: “jeramysoucy” <jeramy.soucy@elastic.co>\r\nCo-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>","sha":"4b7d0145827925a297d1e1728e4447ecc4673554"}},"sourceBranch":"main","suggestedTargetBranches":[],"targetPullRequestStates":[{"branch":"main","label":"v8.15.0","labelRegex":"^v8.15.0$","isSourceBranch":true,"state":"MERGED","url":"#183162 all connectors to use the basic auth header instead of the `auth` property of `axios` (#183162)\n\n## Summary\r\n\r\nFixes: #182391 Framework changes\r\n\r\n- Utils to construct basic header from username and password: [`fad6bde`\r\n(#183162)](https://github.com/elastic/kibana/pull/183162/commits/fad6bde6afb1302c8629da47173abcdf41a1a602),\r\n[`b10d103`\r\n(#183162)](https://github.com/elastic/kibana/pull/183162/commits/b10d103bd92fd17c77b97a6e014832ac1c6a9bdc)\r\n- Automatically convert `auth` to basic auth header in the sub-actions\r\nframework: [`ee27353`\r\n(#183162)](https://github.com/elastic/kibana/pull/183162/commits/ee2735305142cbb84a930e3dc36e7a6d9116bab6)\r\n- Automatically convert `auth` to basic auth header in axios utils:\r\n[`94753a7`\r\n(#183162)](https://github.com/elastic/kibana/pull/183162/commits/94753a7342595c0e2ee33c8f6620661d73526990)\r\n\r\n## Jira\r\n\r\nCommit: [`c366163`\r\n(#183162)](https://github.com/elastic/kibana/pull/183162/commits/c366163486a516f1d2d62b63d3bde171b6643e19)\r\n\r\n## All ServiceNow connectors\r\n\r\nCommit: [`4324d93`\r\n(#183162)](https://github.com/elastic/kibana/pull/183162/commits/4324d931f7bcacfd8b2d7b4eaa9c40562dc22c52)\r\n\r\n## IBM Resilient\r\n\r\nIBM Resilient already uses the basic auth headers. PR\r\nhttps://github.com//pull/180561 added this functionality.\r\nThe connector was manually tested when reviewing the PR.\r\n\r\nIn [`7d9edab`\r\n(#183162)](https://github.com/elastic/kibana/pull/183162/commits/7d9edabd6e8454ebc2190bfc7f565c8c345f47f5)\r\nI updated the connector to use the new util function.\r\n\r\n## Webhook\r\n\r\nCommit: [`1a62c77`\r\n(#183162)](https://github.com/elastic/kibana/pull/183162/commits/1a62c77d46dd40e9529ae102a6db3fc1513c494e)\r\n\r\n## Cases webhook\r\n\r\nCommit: [`104f881`\r\n(#183162)](https://github.com/elastic/kibana/pull/183162/commits/104f8812515f5944bc103fa0142e55a0b0350e84)\r\n\r\n## xMatters\r\n\r\nCommit: [`ea7be2b`\r\n(#183162)](https://github.com/elastic/kibana/pull/183162/commits/ea7be2bbee89b71c3855769fc480a013e4020732)\r\n\r\n## Connectors that do not use the `axios` `auth` property\r\n\r\n- D3Security\r\n- Email\r\n- Microsoft Teams\r\n- OpenAI\r\n- Opsgenie\r\n- PagerDuty\r\n- Sentinel One\r\n- Slack\r\n- Slack API\r\n- Swimlane\r\n- Tines\r\n- Torq\r\n\r\n### Checklist\r\n\r\nDelete any items that are not applicable to this PR.\r\n\r\n- [x] [Unit or functional\r\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\r\nwere updated or added to match the most common scenarios\r\n- [ ] [Flaky Test\r\nRunner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1) was\r\nused on any tests changed\r\n\r\n### Risk Matrix\r\n\r\nDelete this section if it is not applicable to this PR.\r\n\r\nBefore closing this PR, invite QA, stakeholders, and other developers to\r\nidentify risks that should be tested prior to the change/feature\r\nrelease.\r\n\r\nWhen forming the risk matrix, consider some of the following examples\r\nand how they may potentially impact the change:\r\n\r\n| Risk | Probability | Severity | Mitigation/Notes |\r\n\r\n|---------------------------|-------------|----------|-------------------------|\r\n| Connectors not working correctly | Low | High | Unit test and manual\r\ntesting of all connectors affected |\r\n\r\n\r\n### For maintainers\r\n\r\n- [x] This was checked for breaking API changes and was [labeled\r\nappropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)\r\n\r\n---------\r\n\r\nCo-authored-by: “jeramysoucy” <jeramy.soucy@elastic.co>\r\nCo-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>","sha":"4b7d0145827925a297d1e1728e4447ecc4673554"}}]}] BACKPORT-->
Summary
Fixes: #182391
Framework changes
fad6bde
(#183162),b10d103
(#183162)auth
to basic auth header in the sub-actions framework:ee27353
(#183162)auth
to basic auth header in axios utils:94753a7
(#183162)Jira
Commit:
c366163
(#183162)All ServiceNow connectors
Commit:
4324d93
(#183162)IBM Resilient
IBM Resilient already uses the basic auth headers. PR #180561 added this functionality. The connector was manually tested when reviewing the PR.
In
7d9edab
(#183162) I updated the connector to use the new util function.Webhook
Commit:
1a62c77
(#183162)Cases webhook
Commit:
104f881
(#183162)xMatters
Commit:
ea7be2b
(#183162)Connectors that do not use the
axios
auth
propertyChecklist
Delete any items that are not applicable to this PR.
Risk Matrix
Delete this section if it is not applicable to this PR.
Before closing this PR, invite QA, stakeholders, and other developers to identify risks that should be tested prior to the change/feature release.
When forming the risk matrix, consider some of the following examples and how they may potentially impact the change:
For maintainers