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.

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

[Mailer] add ability to disable the TLS peer verification via DSN #35262

Merged
merged 1 commit into from
Jan 29, 2020

Conversation

Livda
Copy link
Contributor

@Livda Livda commented Jan 8, 2020

Q A
Branch? 4.4
Bug fix? no
New feature? yes
Deprecations? no
Tickets Fix
License MIT
Doc PR symfony/symfony-docs/pull/12997

Add the ability to disable the peer TLS verification with the DNS when using EsmtpTransport like this :

MAILER_DSN=smtp://foo@default?verify_peer=false

By default the verification is enabled

@stof
Copy link
Member

stof commented Jan 8, 2020

Do we actually want to allow that ? Disabling peer verification means that your TLS connection is insecure (anyone can do a MitM attack without any issue).

@stof
Copy link
Member

stof commented Jan 8, 2020

What is the use case to add such feature ?

@nicolas-grekas nicolas-grekas added this to the next milestone Jan 8, 2020
@Livda
Copy link
Contributor Author

Livda commented Jan 9, 2020

In case of internal enterprise SMTP with self signed certificate, it's currently impossible to send mail without this. I'm aware that's an insecure way to do things, but there is no way to force no use of TLS connection to work around this issue. I think it's a good compromise in term of security between no encryption at all and fully authenticated TLS connection.

@derrabus
Copy link
Member

internal enterprise SMTP with self signed certificate

The way to go here would be to trust your internal CA that signed your certificate. TLS without certificate verification is a big red flag.

@Livda
Copy link
Contributor Author

Livda commented Jan 14, 2020

The way to go here would be to trust your internal CA that signed your certificate. TLS without certificate verification is a big red flag.

The issue here is I cannot trust any CA because it's a self signed certificate. By design there is no CA on a self signed certificate. I do know it's a bad way to do things, but I have no control over the SMTP server which use this certificate.
Currently I'm using Switfmailer without any encryption at all because I can't use this lib. There is no way to not use TLS with this lib, if the server respond with STARTTLS capability, so I've made this PR in order to use this mailer in such case. But I can add a flag to prevent TLS even if the server said it support TLS after the EHLO request.

Copy link
Member

@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

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

Thanks, here are some suggestions.

@xfifix
Copy link
Contributor

xfifix commented Jan 22, 2020

i have the same problem !

nicolas-grekas
nicolas-grekas previously approved these changes Jan 27, 2020
Copy link
Member

@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

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

Looks good thanks, here are some comments to finish the PR.
Can you add a test case maybe?
Please also add a line in the CHANGELOG of the component and update the description of the PR to keep it in sync.

@nicolas-grekas
Copy link
Member

(please add a line in the changelog of the component)

src/Symfony/Component/Mailer/CHANGELOG.md Outdated Show resolved Hide resolved
@fabpot fabpot force-pushed the mailer-transport-verify-peer branch from ea3de33 to 4b854da Compare January 29, 2020 07:52
@fabpot
Copy link
Member

fabpot commented Jan 29, 2020

Thank you @Livda.

@fabpot fabpot closed this in f0748f8 Jan 29, 2020
@fabpot fabpot merged commit 4b854da into symfony:master Jan 29, 2020
javiereguiluz added a commit to symfony/symfony-docs that referenced this pull request Jan 30, 2020
…n via DSN (Aurélien Fontaine)

This PR was squashed before being merged into the master branch (closes #12997).

Discussion
----------

[Mailer] add ability to disable the TLS peer verification via DSN

Documentation for the PR symfony/symfony#35262

Commits
-------

28a391a [Mailer] add ability to disable the TLS peer verification via DSN
@hectorprats
Copy link

#mailer.yaml

parameters:
env(verify): 'false'

framework:
mailer:
dsn: '%env(MAILER_DSN)%?verify_peer=%env(bool:verify)%'

@pschirch
Copy link

pschirch commented Apr 1, 2020

Is there a chance to make this functionality available in Symfony 4.4 as well?

@xfifix
Copy link
Contributor

xfifix commented Apr 1, 2020

@pschirch i need it too !

@nicolas-grekas nicolas-grekas modified the milestones: next, 5.1 May 4, 2020
@temp
Copy link

temp commented May 5, 2020

Regarding the post from @hectorprats , is this the recommended way? No way to set it only with the MAILER_DSN? I tried it with MAILER_DSN="my-host?verify_peer=false", but this results in a string "false" for verify_peer.
Edit: Ok, works with MAILER_DSN="my-host?verify_peer=0"

@hectorprats
Copy link

It works, but 0 !== boolean.

it's better to keep the semantic.

@roublarstar
Copy link

Thanks @Livda !
But i want to use it with LTS v4.4, because i have an SSL error :(.
I need to create issue on v4.4 ?
@xfifix @pschirch

@temp
Copy link

temp commented May 5, 2020

@hectorprats Sorry, but this is unnecessary. Your version also ends in verify_peer= and verify_peer=1. And for the semantics, you also have a string "true", and not a boolean.
So in my examples MAILER_DSN="my-host?verify_peer=" will result in disabled peer verification. And you don't have to modify the supplied mailer.yaml.

@hectorprats
Copy link

null is not the same as false.

And.. read better my version. You can see the transformation.

@temp
Copy link

temp commented May 5, 2020

It results in the same string inside MAILER_DSN, without the need for modifying a framework-config-file. Both versions work - everyone can choose his version ;-)

@fabpot fabpot mentioned this pull request May 5, 2020
@ivoba
Copy link
Contributor

ivoba commented May 12, 2020

@pschirch @roublarstar I described on SO on how to get this working in 4.4 by overriding EsmtpTransportFactory:
https://stackoverflow.com/a/61662214/541949

Still it would be great if this can be ported to 4.4 :)

@silvioq
Copy link

silvioq commented Oct 11, 2020

It seems that this patch is not on the 4.4 branch ... Could you put in version 4.4x?

@nacholibre
Copy link

For all those who are on 4.4 and verify_peer options is not implemented. You can specify the port in the DSN and achieve the same thing. Example:

MAILER_DSN=smtp://127.0.0.1:25

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.