[actions] for simplistic email servers, set rejectUnauthorized to false #91760
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
We should see if the typing has improved for nodemailer; older versions didn't have typing that was useful to us, so we ended up resorting to using |
resolves elastic#91686 The poor email action has not had great success in setting TLS options correctly. Prior to 7.11, it was basically always setting `rejectUnauthorized` to false, so was never validating certificates. Starting in 7.11.0, it started respecting TLS certificates, but there are some simple/test servers in use that use self-signed certificates. The real fix for this will be the resolution of issue elastic#80120 , but until then, this PR does a special-case check if the `secure` option is off (so the email client connects with a plain socket and then upgrades to TLS via STARTTLS) and both the user and password for the server are not set, then it will use `rejectUnauthorized: false`. Otherwise, it uses the global configured value of this setting. This also changes some other cases, where `secure: true` often did not set any `rejectUnauthorized` property at all, and so did not get verified. Now in all cases, `rejectUnauthorized` will be set, and the value will correspond to the globally configured value, except for the special case checked here, and when a proxy is in use (that logic did not change). So it is possible this would break customers, who were using insecure servers and email action worked, but with this fix the connections will be rejected. They should have been rejected all this time though. The work-around for this problem, if we don't implement a fix like this, is that customers will need to set the global `rejectUnauthorized` to `false`, which means NONE of their TLS connections for any actions will be verified. Which seems extreme.
pmuellr
force-pushed
the
actions/email-no-tls
branch
from
4b90633
to
7690ddd
Compare
pmuellr
added
Feature:Actions
Team:ResponseOps
|
Pinging @elastic/kibana-alerting-services (Team:Alerting Services) |
ymao1
approved these changes
Feb 19, 2021
|
@elasticmachine merge upstream |
pmuellr
added
release_note:breaking
and removed
release_note:skip
|
@elasticmachine merge upstream |
💚 Build SucceededMetrics [docs]Async chunks
Page load bundle
History
To update your PR or re-run it, just comment with: |
9 tasks
pmuellr
added a commit
to pmuellr/kibana
that referenced
this pull request
Mar 1, 2021
…se (elastic#91760) resolves elastic#91686 The poor email action has not had great success in setting TLS options correctly. Prior to 7.11, it was basically always setting `rejectUnauthorized` to false, so was never validating certificates. Starting in 7.11.0, it started respecting TLS certificates, but there are some simple/test servers in use that use self-signed certificates. The real fix for this will be the resolution of issue elastic#80120 , but until then, this PR does a special-case check if the `secure` option is off (so the email client connects with a plain socket and then upgrades to TLS via STARTTLS) and both the user and password for the server are not set, then it will use `rejectUnauthorized: false`. Otherwise, it uses the global configured value of this setting. This also changes some other cases, where `secure: true` often did not set any `rejectUnauthorized` property at all, and so did not get verified. Now in all cases, `rejectUnauthorized` will be set, and the value will correspond to the globally configured value, except for the special case checked here, and when a proxy is in use (that logic did not change). So it is possible this would break customers, who were using insecure servers and email action worked, but with this fix the connections will be rejected. They should have been rejected all this time though. The work-around for this problem, if we don't implement a fix like this, is that customers will need to set the global `rejectUnauthorized` to `false`, which means NONE of their TLS connections for any actions will be verified. Which seems extreme.
pmuellr
added a commit
to pmuellr/kibana
that referenced
this pull request
Mar 1, 2021
…se (elastic#91760) resolves elastic#91686 The poor email action has not had great success in setting TLS options correctly. Prior to 7.11, it was basically always setting `rejectUnauthorized` to false, so was never validating certificates. Starting in 7.11.0, it started respecting TLS certificates, but there are some simple/test servers in use that use self-signed certificates. The real fix for this will be the resolution of issue elastic#80120 , but until then, this PR does a special-case check if the `secure` option is off (so the email client connects with a plain socket and then upgrades to TLS via STARTTLS) and both the user and password for the server are not set, then it will use `rejectUnauthorized: false`. Otherwise, it uses the global configured value of this setting. This also changes some other cases, where `secure: true` often did not set any `rejectUnauthorized` property at all, and so did not get verified. Now in all cases, `rejectUnauthorized` will be set, and the value will correspond to the globally configured value, except for the special case checked here, and when a proxy is in use (that logic did not change). So it is possible this would break customers, who were using insecure servers and email action worked, but with this fix the connections will be rejected. They should have been rejected all this time though. The work-around for this problem, if we don't implement a fix like this, is that customers will need to set the global `rejectUnauthorized` to `false`, which means NONE of their TLS connections for any actions will be verified. Which seems extreme.
pmuellr
added a commit
to pmuellr/kibana
that referenced
this pull request
Mar 1, 2021
…se (elastic#91760) resolves elastic#91686 The poor email action has not had great success in setting TLS options correctly. Prior to 7.11, it was basically always setting `rejectUnauthorized` to false, so was never validating certificates. Starting in 7.11.0, it started respecting TLS certificates, but there are some simple/test servers in use that use self-signed certificates. The real fix for this will be the resolution of issue elastic#80120 , but until then, this PR does a special-case check if the `secure` option is off (so the email client connects with a plain socket and then upgrades to TLS via STARTTLS) and both the user and password for the server are not set, then it will use `rejectUnauthorized: false`. Otherwise, it uses the global configured value of this setting. This also changes some other cases, where `secure: true` often did not set any `rejectUnauthorized` property at all, and so did not get verified. Now in all cases, `rejectUnauthorized` will be set, and the value will correspond to the globally configured value, except for the special case checked here, and when a proxy is in use (that logic did not change). So it is possible this would break customers, who were using insecure servers and email action worked, but with this fix the connections will be rejected. They should have been rejected all this time though. The work-around for this problem, if we don't implement a fix like this, is that customers will need to set the global `rejectUnauthorized` to `false`, which means NONE of their TLS connections for any actions will be verified. Which seems extreme.
pmuellr
added a commit
that referenced
this pull request
Mar 1, 2021
…se (#91760) (#93133) resolves #91686 The poor email action has not had great success in setting TLS options correctly. Prior to 7.11, it was basically always setting `rejectUnauthorized` to false, so was never validating certificates. Starting in 7.11.0, it started respecting TLS certificates, but there are some simple/test servers in use that use self-signed certificates. The real fix for this will be the resolution of issue #80120 , but until then, this PR does a special-case check if the `secure` option is off (so the email client connects with a plain socket and then upgrades to TLS via STARTTLS) and both the user and password for the server are not set, then it will use `rejectUnauthorized: false`. Otherwise, it uses the global configured value of this setting. This also changes some other cases, where `secure: true` often did not set any `rejectUnauthorized` property at all, and so did not get verified. Now in all cases, `rejectUnauthorized` will be set, and the value will correspond to the globally configured value, except for the special case checked here, and when a proxy is in use (that logic did not change). So it is possible this would break customers, who were using insecure servers and email action worked, but with this fix the connections will be rejected. They should have been rejected all this time though. The work-around for this problem, if we don't implement a fix like this, is that customers will need to set the global `rejectUnauthorized` to `false`, which means NONE of their TLS connections for any actions will be verified. Which seems extreme.
pmuellr
added a commit
that referenced
this pull request
Mar 1, 2021
…se (#91760) (#93131) resolves #91686 The poor email action has not had great success in setting TLS options correctly. Prior to 7.11, it was basically always setting `rejectUnauthorized` to false, so was never validating certificates. Starting in 7.11.0, it started respecting TLS certificates, but there are some simple/test servers in use that use self-signed certificates. The real fix for this will be the resolution of issue #80120 , but until then, this PR does a special-case check if the `secure` option is off (so the email client connects with a plain socket and then upgrades to TLS via STARTTLS) and both the user and password for the server are not set, then it will use `rejectUnauthorized: false`. Otherwise, it uses the global configured value of this setting. This also changes some other cases, where `secure: true` often did not set any `rejectUnauthorized` property at all, and so did not get verified. Now in all cases, `rejectUnauthorized` will be set, and the value will correspond to the globally configured value, except for the special case checked here, and when a proxy is in use (that logic did not change). So it is possible this would break customers, who were using insecure servers and email action worked, but with this fix the connections will be rejected. They should have been rejected all this time though. The work-around for this problem, if we don't implement a fix like this, is that customers will need to set the global `rejectUnauthorized` to `false`, which means NONE of their TLS connections for any actions will be verified. Which seems extreme.
pmuellr
added a commit
that referenced
this pull request
Mar 1, 2021
…se (#91760) (#93132) resolves #91686 The poor email action has not had great success in setting TLS options correctly. Prior to 7.11, it was basically always setting `rejectUnauthorized` to false, so was never validating certificates. Starting in 7.11.0, it started respecting TLS certificates, but there are some simple/test servers in use that use self-signed certificates. The real fix for this will be the resolution of issue #80120 , but until then, this PR does a special-case check if the `secure` option is off (so the email client connects with a plain socket and then upgrades to TLS via STARTTLS) and both the user and password for the server are not set, then it will use `rejectUnauthorized: false`. Otherwise, it uses the global configured value of this setting. This also changes some other cases, where `secure: true` often did not set any `rejectUnauthorized` property at all, and so did not get verified. Now in all cases, `rejectUnauthorized` will be set, and the value will correspond to the globally configured value, except for the special case checked here, and when a proxy is in use (that logic did not change). So it is possible this would break customers, who were using insecure servers and email action worked, but with this fix the connections will be rejected. They should have been rejected all this time though. The work-around for this problem, if we don't implement a fix like this, is that customers will need to set the global `rejectUnauthorized` to `false`, which means NONE of their TLS connections for any actions will be verified. Which seems extreme.
gmmorris
added a commit
to gmmorris/kibana
that referenced
this pull request
Mar 2, 2021
* master: (42 commits) [Lens] Introduces new chart switcher (elastic#91844) [Lens] fix selection when dragging (elastic#93034) Converts usage collection README to .mdx (elastic#92982) Fix expanding document when using saved search data grid (elastic#92999) [SECURITY SOLUTIONS] Bug case connector (elastic#93104) [Security Solution] [Timeline] Bugfix to include unmapped fields in the timeline event details JSON (elastic#92025) [Alerting][Docs] Changed alerting documentation to point to a single source of explaining the configurations. (elastic#92942) [APM] Fix hidden search bar in error pages while loading (elastic#84476) (elastic#93139) [DOCS] Fixes links for machine learning alerts (elastic#92744) [Security Solution][Detections] -Fixes rule edit flow bug with max_signals (elastic#92748) [SecuritySolution][Case] Disable cases on detections in read-only mode (elastic#93010) [Security Solution][Case][Bug] Prevent closing collection when pushing (elastic#93095) [Security Solution][Detections][7.12] Critical Threshold Rule Fixes (elastic#92667) Bump ems landing page to 7.12 (elastic#93065) [App Search] Implement various Relevance Tuning states and form actions (elastic#92644) [actions] for simplistic email servers, set rejectUnauthorized to false (elastic#91760) [Security Solution][Case] Migrate category & subcategory fields of ServiceNow ITSM connector (elastic#93092) Hide instances latency distribution chart (elastic#92869) [Maps] fix MapboxDraw import from pointing to dist just pointing to folder (elastic#93087) [Maps] fix results trimmed tooltip message doubles feature count for line and polygon features (elastic#92932) ...
v1v
added a commit
to shahzad31/kibana
that referenced
this pull request
Mar 2, 2021
… playwright-ftr-e2e * 'playwright-ftr-e2e' of github.com:shahzad31/kibana: (38 commits) [chore] Enable core's eslint rule: `@ts-expect-error` (elastic#93086) [Lens] Introduces new chart switcher (elastic#91844) [Lens] fix selection when dragging (elastic#93034) Converts usage collection README to .mdx (elastic#92982) Fix expanding document when using saved search data grid (elastic#92999) [SECURITY SOLUTIONS] Bug case connector (elastic#93104) [Security Solution] [Timeline] Bugfix to include unmapped fields in the timeline event details JSON (elastic#92025) [Alerting][Docs] Changed alerting documentation to point to a single source of explaining the configurations. (elastic#92942) [APM] Fix hidden search bar in error pages while loading (elastic#84476) (elastic#93139) [DOCS] Fixes links for machine learning alerts (elastic#92744) [Security Solution][Detections] -Fixes rule edit flow bug with max_signals (elastic#92748) [SecuritySolution][Case] Disable cases on detections in read-only mode (elastic#93010) [Security Solution][Case][Bug] Prevent closing collection when pushing (elastic#93095) [Security Solution][Detections][7.12] Critical Threshold Rule Fixes (elastic#92667) Bump ems landing page to 7.12 (elastic#93065) [App Search] Implement various Relevance Tuning states and form actions (elastic#92644) [actions] for simplistic email servers, set rejectUnauthorized to false (elastic#91760) [Security Solution][Case] Migrate category & subcategory fields of ServiceNow ITSM connector (elastic#93092) Hide instances latency distribution chart (elastic#92869) [Maps] fix MapboxDraw import from pointing to dist just pointing to folder (elastic#93087) ...
jloleysens
added a commit
that referenced
this pull request
Mar 3, 2021
… ilm/rollup-v2-action * 'ilm/rollup-v2-action' of github.com:elastic/kibana: (30 commits) Fix expanding document when using saved search data grid (#92999) [SECURITY SOLUTIONS] Bug case connector (#93104) [Security Solution] [Timeline] Bugfix to include unmapped fields in the timeline event details JSON (#92025) [Alerting][Docs] Changed alerting documentation to point to a single source of explaining the configurations. (#92942) [APM] Fix hidden search bar in error pages while loading (#84476) (#93139) [DOCS] Fixes links for machine learning alerts (#92744) [Security Solution][Detections] -Fixes rule edit flow bug with max_signals (#92748) [SecuritySolution][Case] Disable cases on detections in read-only mode (#93010) [Security Solution][Case][Bug] Prevent closing collection when pushing (#93095) [Security Solution][Detections][7.12] Critical Threshold Rule Fixes (#92667) Bump ems landing page to 7.12 (#93065) [App Search] Implement various Relevance Tuning states and form actions (#92644) [actions] for simplistic email servers, set rejectUnauthorized to false (#91760) [Security Solution][Case] Migrate category & subcategory fields of ServiceNow ITSM connector (#93092) Hide instances latency distribution chart (#92869) [Maps] fix MapboxDraw import from pointing to dist just pointing to folder (#93087) [Maps] fix results trimmed tooltip message doubles feature count for line and polygon features (#92932) [Security Solution][Detecttions] Indicator enrichment tweaks (#92989) [Maps] fix fit to data on heatmap not working (#92697) [Security Solution][Endpoint][Admin] Fixes policy sticky footer save test (#92919) ...
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
resolves #91686
The poor email action has not had great success in setting TLS options
correctly. Prior to 7.11, it was basically always setting
rejectUnauthorizedto false, so was never validating certificates. Starting in 7.11.0, it
started respecting TLS certificates, but there are some simple/test servers
in use that use self-signed certificates. The default action config of
rejecting unauthorized certificates now means you can no longer
connect to these servers.
The real fix for this will be the resolution of issue
#80120 , but until then, this PR
does a special-case check if the
secureoption is off (so the email clientconnects with a plain socket and then upgrades to TLS via STARTTLS) and both
the user and password for the server are not set, then it will use
rejectUnauthorized: false. Otherwise, it uses the global configured valueof this setting.
This also changes some other cases, where
secure: trueoften did notset any
rejectUnauthorizedproperty at all, and so did not get verified.Now in all cases,
rejectUnauthorizedwill be set, and the value willcorrespond to the globally configured value, except for the special case
checked here, and when a proxy is in use (that logic did not change).
So it is possible this would break customers, who were using insecure servers
and email action worked, but with this fix the connections will be rejected.
They should have been rejected all this time though.
The work-around for this problem, if we don't implement a fix like this, is
that customers will need to set the global
rejectUnauthorizedtofalse,which means NONE of their TLS connections for any actions will be verified.
Which seems extreme.
Checklist
Delete any items that are not applicable to this PR.
Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n supportDocumentation was added for features that require explanation or tutorialsAny UI touched in this PR is usable by keyboard only (learn more about keyboard accessibility)Any UI touched in this PR does not create any new axe failures (run axe in browser: FF, Chrome)If a plugin configuration key changed, check if it needs to be allowlisted in the cloud and added to the docker listThis renders correctly on smaller devices using a responsive layout. (You can test this in your browser)This was checked for cross-browser compatibilityFor maintainers
Release note
Email connector TLS processing has changed so that unless overridden by the global
xpack.actions.rejectUnauthorizedconfig setting, all TLS connections require verified certificates. There is one exception, for email connectors which both use thesecurefalse setting and do not use authentication - these connectors no longer require a verified certificate, to support usage with email test servers.