-
-
Notifications
You must be signed in to change notification settings - Fork 4.6k
#41935 smtp: allow self-signed certificates on localhost #46957
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
base: master
Are you sure you want to change the base?
Conversation
|
||
/** @psalm-suppress InternalMethod */ | ||
$stream->setStreamOptions($currentStreamingOptions); | ||
} else if ($mailSmtphost === 'localhost' || $mailSmtphost === '127.0.0.1') { |
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.
In theory we could even do this if no valid host was provided, e.g. by using such a filter: https://github.com/nextcloud/all-in-one/blob/c9b97220d0175914c3ee8c7199949376f7bfe3b0/php/src/Data/ConfigurationManager.php#L274-L295
However this sounds like magic and not sure if we want to actually do this automatically.
WDYT @nextcloud/server-backend @miaulalala
|
I will adapt the test as soon as it is clear whether this change has a chance of being accepted or not. Personally I'm totally fine if the PR is being rejected. #41935 was just an idea to reduce confusion for users using a local mail server, resulting in many false bug reports. |
} else if ($mailSmtphost === 'localhost' || $mailSmtphost === '127.0.0.1') { | ||
$currentStreamingOptions = $stream->getStreamOptions(); | ||
$currentStreamingOptions = array_merge_recursive($currentStreamingOptions, array( | ||
'ssl' => array( | ||
'allow_self_signed' => true, | ||
'verify_peer' => false, | ||
'verify_peer_name' => false | ||
) | ||
)); |
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'm of two minds here - one is that it indeed makes localhosts work without any setup, which, as you stated has confused many users due to how the Symfony Mailer works. OTOH I'm not entirely happy with overwriting config options per default.
I think there's some middle ground here: check if the ssl
option is set and only overwrite it if a) the host is localhost || 127.0.0.1
and b) if the option doesn't already exist in the $currentStreamingOptions
I also think it doesn't need to be an elseif but can be an if condition on it's own. Something like:
} else if ($mailSmtphost === 'localhost' || $mailSmtphost === '127.0.0.1') { | |
$currentStreamingOptions = $stream->getStreamOptions(); | |
$currentStreamingOptions = array_merge_recursive($currentStreamingOptions, array( | |
'ssl' => array( | |
'allow_self_signed' => true, | |
'verify_peer' => false, | |
'verify_peer_name' => false | |
) | |
)); | |
} | |
$local = ['localhost', '127.0.0.1']; // You could make this a constant | |
if (in_array($mailSmtphost, $local) && !isset($currentStreamingOptions['ssl'])) { | |
$currentStreamingOptions = $stream->getStreamOptions(); | |
$currentStreamingOptions = array_merge_recursive($currentStreamingOptions, array( | |
'ssl' => array( | |
'allow_self_signed' => true, | |
'verify_peer' => false, | |
'verify_peer_name' => false | |
) | |
)); |
symfony/symfony#53621 (but it will take a while until we are on mailer 7.1) |
Hello there, We hope that the review process is going smooth and is helpful for you. We want to ensure your pull request is reviewed to your satisfaction. If you have a moment, our community management team would very much appreciate your feedback on your experience with this PR review process. Your feedback is valuable to us as we continuously strive to improve our community developer experience. Please take a moment to complete our short survey by clicking on the following link: https://cloud.nextcloud.com/apps/forms/s/i9Ago4EQRZ7TWxjfmeEpPkf6 Thank you for contributing to Nextcloud and we hope to hear from you soon! (If you believe you should not receive this message, you can add yourself to the blocklist.) |
allow_self_signed=true
ifmail_smtphost = localhost
#41935Summary
A mailserver listening on localhost can never have a valid TLS certificate and it does not require one. This patch enables
allow_self_signed
and disablesverify_peer
andverify_peer_name
whenmail_smtphost
equalslocalhost
or127.0.0.1
.Pull request as suggested by joshtrichards in #41935