Open Bug 1182445 Opened 5 years ago Updated 1 year ago

Add support for SMTP TLS

Categories

(Bugzilla :: Email Notifications, enhancement)

enhancement
Not set

Tracking

()

Bugzilla 5.0

People

(Reporter: damicogiuse, Assigned: dylanAtHome)

References

Details

Attachments

(4 files)

Attached file bugzilla5_smtp_tls.zip
User Agent: Mozilla/5.0 (X11; Linux i686) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/43.0.2357.132 Safari/537.36

Steps to reproduce:

I upgraded my bugzilla from version 4.4.9 to 5.0 (downloading the tar archive of bugzilla 5.0 and installing it from scratch)


Actual results:

Almost everything went fine. I had only a problem in making the SMTP TLS e-mail notifications working properly under bugzilla 5.0. As far as I remember I had similar issues also to make the same protocol working under bugzilla version 4.x as it seems STMP TLS is not officially supported. Anyway this time the fix was quite easy and I was wandering if it would be possible to include officially SMTP TLS e-mail notifications in one of the next bugzilla releases. 
According to my tests this is what is needed to make SMTP TLS working under bugzilla 5.0:
1) Installation of Perl module Email::Sender::Transport::SMTP::TLS
2) editing of  Bugzilla/Mailer.pm file (basically I changed the perl transport module from  Email::Sender::Transport::SMTP::Persistent to Email::Sender::Transport::SMTP::TLS)
3) editing of Bugzilla.pm file (commenting the line 669 $smtp->disconnect if $smtp;)
I have attached the corresponding diff files.  


Expected results:

Of course the modifications above allow the use of SMTP TLS but not the other SMTP protocols. It would be fine to have both these options implemented in the next bugzilla release.
Looks like Email::Sender::Transport::SMTPS should be used instead of Email::Sender::Transport::SMTP::TLS, per the maintainer of ::SMTP::TLS himself. But ::SMTPS is behind the original Email::Sender::Transport::SMTP code from a bug fix point of view, so I don't think we should use ::SMTPS either. Maybe it's enough to subclass _smtp_client() and _net_smtp_args(); I don't know.
Severity: normal → enhancement
Status: UNCONFIRMED → NEW
Ever confirmed: true
Summary: Support for sending e-mail notifications using SMTP TLS not working → Add support for SMTP TLS
Thanks for considering that! It is also my feeling that a couple of subclass could easily make possible the implementation of a native support for SMTP TLS.
Using the Bugzilla version 5.0.2 from git, I was missing the SMTP TLS option. Not aware of this bug, I fixed it for my install and created the diff. It worked for me, so I decided to share the diff.

* Added Email::Send::SMTP::TLS library to Mailer.pm
* Added the additional choice SMTP::TLS (displayed in admin panel)
* Added description for the SMTP::TLS choice
* Added an appropriate section in Mailer.pm for 
   if ($method == "SMTP::TLS")

Please feel free to modify the code. Hope to see TLS Mailing available in the next version.

Best regards,
   Lars
Duplicate of this bug: 1244289
We are successfully using Email::Send::SMTP::TLS on a Bugzilla 4.4.4 server, but couldn't get it to work on an updated Bugzilla 5.0.2 server.

After making the changes that Lars proposed, whenever Bugzilla attempts to send an email we get an error about localhost 25.  So the smtpserver, etc. parameters aren't getting sent properly to the email library.

Just using a "push" in the Mailer.pm code for SMTP:TLS is the part that confuses me, since the Mailer.pm code for SMTP uses something quite different:
$transport = Bugzilla->request_cache->{smtp} //= Email::Sender::Transport::SMTP::Persistent->new({...});

Perl is not my thing though, so it may make perfectly good sense.  Anyway, we couldn't get it to work.
Flags: needinfo?(mail)
@Steve: A pitty it did not work at your side as I hoped this could save time for others. I just compared my patch as it is running here. 
I read "about localhost 25" in your post. Using TLS now, I had to put example.org:587 in the settings of bugzilla relating the smtp account.
Go to: <your_site+path_to_bugzilla>/editparams.cgi?section=mta and find 'smtpserver'
Insert :587 (this should/could be the port for TLS - please check with your configuration or ask your admin)

Good luck!
   Lars
Flags: needinfo?(mail)
Thanks Lars.  We're using it with smtpserver set to smtp.office365.com:587.

However, we just found an alternative that did work for us:

use Email::Sender::Transport::SMTP::TLS;

if ($method eq “SMTP::TLS”) {
	my ($host, $port) = split(/:/, Bugzilla->params->{‘smtpserver’}, 2);
	$transport = Bugzilla->request_cache->{smtp} //= Email::Sender::Transport::SMTP::TLS->new({
		host => $host,
		defined($port) ? (port => $port) : (),
		username => Bugzilla->params->{‘smtp_username’},
		password => Bugzilla->params->{‘smtp_password’},
		helo => $hostname
	});
}

Since Bugzilla normally uses Email::Sender::Transport::SMTP::Persistent, I thought using Email::Sender::Transport::SMTP::TLS in the same way might work.  And it did!

I hope this is helpful for others.

Thanks.
Hi Steve,

great it worked out for you at the end and hope I could help out in the beginning. I am looking forward to test your proposal next weekend. I like the $transport = ..... looks cleaner. I would add the ssl and debug lines to get it complete.
If it works out on my side, I may offer a new version of the patch. Thanks for sharing your experience and proposal!

Lars
We have noticed one issue so far with it.

Sometimes we get a warning after bugzilla sends emails:
Can't locate object method "disconnect" via package "Email::Sender::Transport::SMTP::TLS" at Bugzilla.pm line 685.
END failed--call queue aborted.

I assume this is because Email::Sender::Transport::SMTP::Persistent has a disconnect method, but Email::Sender::Transport::SMTP::TLS does not.

It still seems to be sending emails. We haven't had a chance to dig into it further to find a workaround though.
Hi Steve,

I think your can get rid of that warning message just commenting out the line 685 in Bugzilla.pm file (as reported in my first post). In my case it worked fine!

I hope this helps!
I seem to have gotten rid of the warning message by changing $transport = Bugzilla->request_cache->{smtp} to $transport = Bugzilla->request_cache->{smtptls}

This prevents it from executing the Bugzilla.pm code
    my $smtp = $cache->{smtp};
    $smtp->disconnect if $smtp;
    clear_request_cache();
when we're using SMTP::TLS.  It still applies for SMTP though, as intended.
Attachment #8702144 - Flags: review?(dkl)
Assignee: email-notifications → dylan
Target Milestone: --- → Bugzilla 5.0
Assignee: dylan → dylan
Attachment #8702144 - Flags: review?(dkl) → review?(jfearn)
Comment on attachment 8702144 [details] [diff] [review]
Bug_1182445_bugzilla_5-0-2_SMTP+TLS_patch.diff

Review of attachment 8702144 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me.
Attachment #8702144 - Flags: review?(jfearn) → review+
Attached patch bz-5.0.3-tls.txtSplinter Review
Patch to fix email sending using STARTTLS for SSL transport against Bugzilla 5.0.3.
Comment on attachment 8702144 [details] [diff] [review]
Bug_1182445_bugzilla_5-0-2_SMTP+TLS_patch.diff

Review of attachment 8702144 [details] [diff] [review]:
-----------------------------------------------------------------

Does it really make sense to treat SMTP::TLS as an option like SMTP, if there's an additional checkbox to enable SSL support for SMTP? In the provided patch the flag to enable SSL support is simply used as well for SMTP::TLS now, because the code seems to have been copied, but what does that actually mean? TLS with STARTSSL or is the SSL flag ignored or what is the combination of both? Shouldn't it still be SMTP only and the admin needs to additionally decide between SSL vs. TLS vs. whatever using come check or radio box? If SMTP::TLS make sense, why doesn't it make sense to provide SMTP::SSL or SMTP::STARTSSL or whatever as well? This looks a bit confusing to me, SMTP exists to distinguish between Sendmail, Testfile etc., it doesn't exist because it means unencrypted transport only.

OTOH, the SSL checkbox doesn't apply to the options "Sendmail", "Test" etc., so it might be a good idea to remove that checkbox and use SMTP::* options only.

Should this be an additional bug, cleaning up those options in the UI?
Comment on attachment 8702144 [details] [diff] [review]
Bug_1182445_bugzilla_5-0-2_SMTP+TLS_patch.diff

Review of attachment 8702144 [details] [diff] [review]:
-----------------------------------------------------------------

Is that patch really working? I don't see where an instance of "Email::Send::SMTP::TLS" is created and a user on the user mailing list applied it and reported that it doesn't work. Which would make sense without any instance of "...::TLS". The new code in MessageToMTA seems to only push args to a private array without ever using them to create a transport instance. The newer patch instead does seem to create an instance of "...::TLS".

https://groups.google.com/d/msg/mozilla.support.bugzilla/fLSwshCY76k/RJonMh6zBwAJ

Since SSL/TLS are options of Email::Sender::Transport::SMTP::Persistent I believe that the smtp_ssl setting should be extended to include the different options in order to allow TLS. According to the documentation the ssl parameter accepts the following values: if 'starttls', use STARTTLS; if 'ssl' (or 1), connect securely; otherwise, no security. The patch I created changes the following:

  1. Config/MTA.pm - smtp_ssl is changed to a drop down.
  2. Config/Common.pm - check_smtp_ssl is changed to look for the new values.
  3. Mailer.pm - passing the ssl parameter is changed to correctly reflect the new values.
Attachment #9042446 - Flags: review?(dkl)
You need to log in before you can comment on or make changes to this bug.