feat: Reworking the SMTP uri to use smtps scheme for implicit TLS and smtp for explicit StartTLS/cleartext #1770

StarAurryon · 2021-09-21T19:25:40Z

This changes the previous behaviour of the smtp uri from:
smtps://server:xxx/?skip_ssl_verify=true&legacy_ssl=true

To:
smtp://server:xxx/?skip_starttls=true&skip_ssl_verify=true (for StartTLS/cleartext)
smtps://server:xxx/?skip_ssl_verify=true (for Implicit TLS)

Related issue(s)

Closes #1769

Checklist

I have read the contributing guidelines.
I am following the
contributing code guidelines.
I have read the security policy.
I confirm that this pull request does not address a security
vulnerability. If this pull request addresses a security. vulnerability, I
confirm that I got green light (please contact
security@ory.sh) from the maintainers to push
the changes.
I have added tests that prove my fix is effective or that my feature
works.
I have added or changed the documentation.

Further Comments

I'm just having issues with MySQL persistence tests that seem unrelated. Could you confirm?

CLAassistant · 2021-09-21T19:25:45Z

All committers have signed the CLA.

codecov · 2021-09-21T22:26:24Z

Codecov Report

Merging #1770 (a957605) into master (c6aa6b5) will decrease coverage by 0.04%.
The diff coverage is 90.00%.

@@            Coverage Diff             @@
##           master    #1770      +/-   ##
==========================================
- Coverage   74.07%   74.03%   -0.05%     
==========================================
  Files         260      260              
  Lines       12770    12773       +3     
==========================================
- Hits         9460     9456       -4     
- Misses       2680     2684       +4     
- Partials      630      633       +3

Impacted Files	Coverage Δ
selfservice/strategy/oidc/provider_microsoft.go	`0.00% <ø> (ø)`
x/mailhog.go	`6.97% <0.00%> (ø)`
courier/courier.go	`59.15% <100.00%> (-9.20%)`	⬇️
cmd/courier/watch.go	`72.41% <0.00%> (+12.06%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 164a90d...a957605. Read the comment docs.

zepatrik

I did not look too deep into the actual code change, this is only regarding documentation.

docs/docs/reference/configuration.md

driver/config/.schema/config.schema.json

StarAurryon · 2021-09-27T17:11:13Z

I just rebased against the latest master.

aeneasr · 2021-09-29T11:00:49Z

Could you please add a breaking change note in form of a comment here that helps users upgrade their existing SMTP URLs to the new format? Thank you!

Example:

BREAKING CHANGE: This changes x to y please do z.

Examples:

- x -> y
- z -> x

aeneasr

Thank you! This is looking good! Could you please also add documentation to this page: https://www.ory.sh/kratos/docs/concepts/email-sms/#sending-e-mails-via-smtp

courier/courier.go

StarAurryon · 2021-09-29T11:26:25Z

Could you please add a breaking change note in form of a comment here that helps users upgrade their existing SMTP URLs to the new format? Thank you!

Example:
BREAKING CHANGE: This changes x to y please do z.

Examples:

- x -> y
- z -> x

Would you like me to do it only on the first commit? And should I merge the 3 into one?

aeneasr · 2021-09-30T13:16:57Z

You can add it as a comment! I will squash merge them anyways :)

… smtp for explicit StartTLS/cleartext BREAKING CHANGE: The smtps scheme used in courier config url with cleartext/StartTLS/TLS SMTP connection types is now only supporting implicit TLS. For StartTLS and cleartext SMTP, please use the smtp scheme instead. Example: - SMTP Cleartext: smtp://foo:bar@my-mailserver:1234/?disable_starttls=true - SMTP with StartTLS: smtps://foo:bar@my-mailserver:1234/ -> smtp://foo:bar@my-mailserver:1234/ - SMTP with implicit TLS: smtps://foo:bar@my-mailserver:1234/?legacy_ssl=true -> smtps://foo:bar@my-mailserver:1234/

Fixing drivers/config/.schema/config.schema.json Co-authored-by: Patrik <zepatrik@users.noreply.github.com>

…ema)

…ring

StarAurryon · 2021-10-05T15:45:00Z

Wow! The error in circleci is weird. Any idea?

aeneasr · 2021-10-06T08:03:20Z

looks like a flake - restarting the job :)

aeneasr · 2021-10-12T05:58:08Z

It passed! :) I will resolve the merge conflict and it's good to go!

…artTLS, and cleartext SMTP (#1831) BREAKING CHANGE: The SMTPS scheme used in courier config url with cleartext/StartTLS/TLS SMTP connection types is now only supporting implicit TLS. For StartTLS and cleartext SMTP, please use the smtp scheme instead. Example: - SMTP Cleartext: `smtp://foo:bar@my-mailserver:1234/?disable_starttls=true` - SMTP with StartTLS: `smtps://foo:bar@my-mailserver:1234/` -> `smtp://foo:bar@my-mailserver:1234/` - SMTP with implicit TLS: `smtps://foo:bar@my-mailserver:1234/?legacy_ssl=true` -> `smtps://foo:bar@my-mailserver:1234/` Closes #1770 Closes #1769

StarAurryon requested review from aeneasr and zepatrik as code owners September 21, 2021 19:25

StarAurryon force-pushed the smtp-scheme-rework branch from cd7b8f6 to 566ae17 Compare September 21, 2021 19:29

StarAurryon changed the title ~~Reworking the SMTP uri to use smtps scheme for implicit TLS and smtp …~~ BREAKING CHANGE: Reworking the SMTP uri to use smtps scheme for implicit TLS and smtp for explicit StartTLS/cleartext Sep 21, 2021

StarAurryon changed the title ~~BREAKING CHANGE: Reworking the SMTP uri to use smtps scheme for implicit TLS and smtp for explicit StartTLS/cleartext~~ feat: Reworking the SMTP uri to use smtps scheme for implicit TLS and smtp for explicit StartTLS/cleartext Sep 21, 2021

StarAurryon force-pushed the smtp-scheme-rework branch 3 times, most recently from 9d06f24 to aac7d9a Compare September 21, 2021 21:52

zepatrik requested changes Sep 22, 2021

View reviewed changes

docs/docs/reference/configuration.md Outdated Show resolved Hide resolved

driver/config/.schema/config.schema.json Outdated Show resolved Hide resolved

StarAurryon force-pushed the smtp-scheme-rework branch from 4af82ed to 788db14 Compare September 27, 2021 17:03

aeneasr requested changes Sep 29, 2021

View reviewed changes

courier/courier.go Outdated Show resolved Hide resolved

StarAurryon and others added 5 commits October 5, 2021 16:17

fix: Reworking the SMTP uri (Updating the config schema)

c03a20d

Fixing drivers/config/.schema/config.schema.json Co-authored-by: Patrik <zepatrik@users.noreply.github.com>

fix: Reworking the SMTP uri (Regenerating the doc from the config sch…

6cdddbb

…ema)

fix: Renaming 'skip_starttls' to 'disable_starttls' in smtp config st…

d8e5b41

…ring

fix: Adding missing doc to email-sms.md

a957605

StarAurryon force-pushed the smtp-scheme-rework branch 2 times, most recently from 4762890 to a957605 Compare October 5, 2021 14:39

aeneasr approved these changes Oct 12, 2021

View reviewed changes

aeneasr mentioned this pull request Oct 12, 2021

refactor(courier): support SMTP schemes for implicit TLS, explicit StartTLS, and cleartext SMTP #1831

Merged

aeneasr closed this in #1831 Oct 12, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Reworking the SMTP uri to use smtps scheme for implicit TLS and smtp for explicit StartTLS/cleartext #1770

feat: Reworking the SMTP uri to use smtps scheme for implicit TLS and smtp for explicit StartTLS/cleartext #1770

StarAurryon commented Sep 21, 2021 •

edited

CLAassistant commented Sep 21, 2021 •

edited

codecov bot commented Sep 21, 2021 •

edited

zepatrik left a comment

StarAurryon commented Sep 27, 2021

aeneasr commented Sep 29, 2021

aeneasr left a comment

StarAurryon commented Sep 29, 2021

aeneasr commented Sep 30, 2021

StarAurryon commented Oct 5, 2021

aeneasr commented Oct 6, 2021

aeneasr commented Oct 12, 2021

feat: Reworking the SMTP uri to use smtps scheme for implicit TLS and smtp for explicit StartTLS/cleartext #1770

feat: Reworking the SMTP uri to use smtps scheme for implicit TLS and smtp for explicit StartTLS/cleartext #1770

Conversation

StarAurryon commented Sep 21, 2021 • edited

Related issue(s)

Checklist

Further Comments

CLAassistant commented Sep 21, 2021 • edited

codecov bot commented Sep 21, 2021 • edited

Codecov Report

zepatrik left a comment

Choose a reason for hiding this comment

StarAurryon commented Sep 27, 2021

aeneasr commented Sep 29, 2021

aeneasr left a comment

Choose a reason for hiding this comment

StarAurryon commented Sep 29, 2021

aeneasr commented Sep 30, 2021

StarAurryon commented Oct 5, 2021

aeneasr commented Oct 6, 2021

aeneasr commented Oct 12, 2021

StarAurryon commented Sep 21, 2021 •

edited

CLAassistant commented Sep 21, 2021 •

edited

codecov bot commented Sep 21, 2021 •

edited