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

Deal with opportunistic TLS change in PHPMailer 5.2.10 #9528

Merged
merged 6 commits into from Mar 22, 2016

Conversation

Projects
None yet
7 participants
@roland-d
Contributor

roland-d commented Mar 22, 2016

Pull Request for Issue #9373

Summary of Changes

PHPMailer 5.2.10 introduced a new feature called Opportunistic TLS, this will see if a server advertises TLS encryption. If it does, a secure connection will be established. This is only possible if the server has been setup correctly and uses correct certificates. If the server has invalid certificates, the mail sending will fail.

Testing Instructions

  1. Install Joomla 3.5.0 on a server which advertises TLS and has invalid certificates setup (or just a server where the mail sending / test mail button no longer works after installing 3.5.0 but worked with 3.4.8)
  2. Check if mail sending fails
  3. Apply the patch
  4. Check if the mail sending succeeds now

@roland-d roland-d added this to the Joomla 3.5.1 milestone Mar 22, 2016

@mbabker

This comment has been minimized.

Show comment
Hide comment
@mbabker

mbabker Mar 22, 2016

Member

I don't agree with this approach as it's an all or nothing change. You're disabling it for everyone even if they do have a valid configuration and things work correctly if TLS security isn't explicitly set up otherwise. It's on par with saying that since CentOS 5 servers won't work with the current update system CDN when they disable support for older protocols that nobody can have secure connections to the update server.

Member

mbabker commented Mar 22, 2016

I don't agree with this approach as it's an all or nothing change. You're disabling it for everyone even if they do have a valid configuration and things work correctly if TLS security isn't explicitly set up otherwise. It's on par with saying that since CentOS 5 servers won't work with the current update system CDN when they disable support for older protocols that nobody can have secure connections to the update server.

@brianteeman

This comment has been minimized.

Show comment
Hide comment
@brianteeman

brianteeman Mar 22, 2016

Contributor

Maybe i misunderstood but this doesnt disable TLS if it is explicitly set in the config. It only disables the guesswork

Contributor

brianteeman commented Mar 22, 2016

Maybe i misunderstood but this doesnt disable TLS if it is explicitly set in the config. It only disables the guesswork

@Bakual

This comment has been minimized.

Show comment
Hide comment
@Bakual

Bakual Mar 22, 2016

Contributor

I think disabling it is fine. It's kind of a strange feature to be honest. It creates issues which are very hard to track down. If I set up my system to not use TLS, I expect it to not use it and not magically use it neverthless.
We already have a setting to enable TLS or SSL. I think that's all we need.
If you really want, add an option "auto" there which enables opportunistic TLS if selected, but if it's set to "none", it should not use any security.

Contributor

Bakual commented Mar 22, 2016

I think disabling it is fine. It's kind of a strange feature to be honest. It creates issues which are very hard to track down. If I set up my system to not use TLS, I expect it to not use it and not magically use it neverthless.
We already have a setting to enable TLS or SSL. I think that's all we need.
If you really want, add an option "auto" there which enables opportunistic TLS if selected, but if it's set to "none", it should not use any security.

@mbabker

This comment has been minimized.

Show comment
Hide comment
@mbabker

mbabker Mar 22, 2016

Member

Right, it disables enhancing to TLS if the server reports that it is able to support it. PHPMailer probably shouldn't bump up though if it requires extra configuration that isn't set in the class that would be set otherwise if you explicitly enable the TLS security option.

We too often take a stance that it's Joomla's responsibility to mask server configuration issues or bugs in third party libraries. This is another PR in that direction. If a server broadcasts that it supports TLS then fails when you try to use it that's a server configuration error, not a Joomla error. If the auto-TLS upgrade fails because PHPMailer needs more info to use it that's a PHPMailer issue, not a Joomla issue.

But, go ahead and merge this. It sounds like the decision is made already.

Member

mbabker commented Mar 22, 2016

Right, it disables enhancing to TLS if the server reports that it is able to support it. PHPMailer probably shouldn't bump up though if it requires extra configuration that isn't set in the class that would be set otherwise if you explicitly enable the TLS security option.

We too often take a stance that it's Joomla's responsibility to mask server configuration issues or bugs in third party libraries. This is another PR in that direction. If a server broadcasts that it supports TLS then fails when you try to use it that's a server configuration error, not a Joomla error. If the auto-TLS upgrade fails because PHPMailer needs more info to use it that's a PHPMailer issue, not a Joomla issue.

But, go ahead and merge this. It sounds like the decision is made already.

@brianteeman

This comment has been minimized.

Show comment
Hide comment
@brianteeman

brianteeman Mar 22, 2016

Contributor
Contributor

brianteeman commented Mar 22, 2016

@mbabker

This comment has been minimized.

Show comment
Hide comment
@mbabker

mbabker Mar 22, 2016

Member

They should pull it or default it to off then if it's really THAT buggy.

Member

mbabker commented Mar 22, 2016

They should pull it or default it to off then if it's really THAT buggy.

@brianteeman

This comment has been minimized.

Show comment
Hide comment
@brianteeman

brianteeman Mar 22, 2016

Contributor
Contributor

brianteeman commented Mar 22, 2016

@roland-d

This comment has been minimized.

Show comment
Hide comment
@roland-d

roland-d Mar 22, 2016

Contributor

Updated the PR to try with TLS, if it fails, it will try sending it without TLS. Zero configuration :)

Contributor

roland-d commented Mar 22, 2016

Updated the PR to try with TLS, if it fails, it will try sending it without TLS. Zero configuration :)

@roland-d roland-d changed the title from Turn off opportunistic TLS to Deal with opportunistic TLS change in PHPMailer 5.2.10 Mar 22, 2016

@mbabker

This comment has been minimized.

Show comment
Hide comment
@mbabker

mbabker Mar 22, 2016

Member

In general, a better idea. Still tricky because they only throw one Exception type for all errors. I'd say check if $this->SMTPAutoTLS is already false before trying the second send (someone in an extension could customize this stuff, you never know) and it'd be fine.

As for other improvements, see #9530

Member

mbabker commented Mar 22, 2016

In general, a better idea. Still tricky because they only throw one Exception type for all errors. I'd say check if $this->SMTPAutoTLS is already false before trying the second send (someone in an extension could customize this stuff, you never know) and it'd be fine.

As for other improvements, see #9530

@roland-d

This comment has been minimized.

Show comment
Hide comment
@roland-d

roland-d Mar 22, 2016

Contributor

@mbabker Good point on adding the check. I have added it in. Thanks.

Contributor

roland-d commented Mar 22, 2016

@mbabker Good point on adding the check. I have added it in. Thanks.

wilsonge added a commit that referenced this pull request Mar 22, 2016

Merge pull request #9528 from roland-d/phpmailer-opportunistic-tls
Deal with opportunistic TLS change in PHPMailer 5.2.10

@wilsonge wilsonge merged commit c43e410 into joomla:staging Mar 22, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@roland-d roland-d deleted the roland-d:phpmailer-opportunistic-tls branch Apr 13, 2016

@cjsdfw

This comment has been minimized.

Show comment
Hide comment
@cjsdfw

cjsdfw Apr 14, 2016

Hi,
For what is worth...
This change is not working for me in WAMP 3 environment (Windows 10) but adding the following fixes the issue. Please note I am a newbie so please check it, I am not sure what the change implies but it does work and without it the mail will fail:

Line 120 $this->SMTPAutoTLS = false;
// Added lines
$this->SMTPOptions = array(
'ssl' => array(
'verify_peer' => false,
'verify_peer_name' => false,
'allow_self_signed' => true
)
);
// End of added lines

cjsdfw commented Apr 14, 2016

Hi,
For what is worth...
This change is not working for me in WAMP 3 environment (Windows 10) but adding the following fixes the issue. Please note I am a newbie so please check it, I am not sure what the change implies but it does work and without it the mail will fail:

Line 120 $this->SMTPAutoTLS = false;
// Added lines
$this->SMTPOptions = array(
'ssl' => array(
'verify_peer' => false,
'verify_peer_name' => false,
'allow_self_signed' => true
)
);
// End of added lines

@mbabker

This comment has been minimized.

Show comment
Hide comment
@mbabker

mbabker Apr 14, 2016

Member

That change disables SSL verification in full. PHP 5.6+ by default
verifies certificates if you've entered a configuration that communicates
on a secure port or protocol.

On Thursday, April 14, 2016, cjsdfw notifications@github.com wrote:

Hi,
For what is worth...
This change is not working for me in WAMP 3 environment (Windows 10) but
adding the following fixes the issue. Please note I am a newbie so please
check it, I am not sure what the change implies but it does work and
without it the mail will fail:

Line 120 $this->SMTPAutoTLS = false;
// Added lines
$this->SMTPOptions = array(
'ssl' => array(
'verify_peer' => false,
'verify_peer_name' => false,
'allow_self_signed' => true
)
);
// End of added lines


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#9528 (comment)

Member

mbabker commented Apr 14, 2016

That change disables SSL verification in full. PHP 5.6+ by default
verifies certificates if you've entered a configuration that communicates
on a secure port or protocol.

On Thursday, April 14, 2016, cjsdfw notifications@github.com wrote:

Hi,
For what is worth...
This change is not working for me in WAMP 3 environment (Windows 10) but
adding the following fixes the issue. Please note I am a newbie so please
check it, I am not sure what the change implies but it does work and
without it the mail will fail:

Line 120 $this->SMTPAutoTLS = false;
// Added lines
$this->SMTPOptions = array(
'ssl' => array(
'verify_peer' => false,
'verify_peer_name' => false,
'allow_self_signed' => true
)
);
// End of added lines


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#9528 (comment)

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