[PATCH] smtp tls: fix for server doesn't support STARTTLS extension

[Zhigang Wang]

doc/hgrc.5.txt | 3 +++
mercurial/mail.py | 17 +++++++++++------
2 files changed, 14 insertions(+), 6 deletions(-)


# HG changeset patch
# User Zhigang Wang <***@oracle.com>
# Date 1291967692 -28800
# Node ID d44d0475a776eaffb8d8b34107e357dd43cfbbd3
# Parent 5dac0d04b838d599acdc5db9ee0b7de51de15537
smtp tls: fix for server doesn't support STARTTLS extension.

Currently we only support enabling TLS by using SMTP STARTTLS extension. But
not all the servers support it.

With this patch, user can choose which way to use to enable TLS:

* Default:

tls = false
starttls = false
port = 25

* To use tls:

tls = true
port = 465

* To use starttls:

starttls = true
port = 465

Signed-off-by: Zhigang Wang <***@gmail.com>

diff -r 5dac0d04b838 -r d44d0475a776 doc/hgrc.5.txt
--- a/doc/hgrc.5.txt Wed Dec 08 13:12:12 2010 -0600
+++ b/doc/hgrc.5.txt Fri Dec 10 15:54:52 2010 +0800
@@ -711,6 +711,9 @@
``tls``
Optional. Whether to connect to mail server using TLS. True or
False. Default: False.
+``starttls``
+ Optional. Whether to enable TLS using STARTTLS extension. True or
+ False. Default: False.
``username``
Optional. User name for authenticating with the SMTP server.
Default: none.
diff -r 5dac0d04b838 -r d44d0475a776 mercurial/mail.py
--- a/mercurial/mail.py Wed Dec 08 13:12:12 2010 -0600
+++ b/mercurial/mail.py Fri Dec 10 15:54:52 2010 +0800
@@ -33,7 +33,15 @@
def _smtp(ui):
'''build an smtp connection and return a function to send mail'''
local_hostname = ui.config('smtp', 'local_hostname')
- s = smtplib.SMTP(local_hostname=local_hostname)
+ if ui.configbool('smtp', 'tls') or ui.configbool('smtp', 'starttls'):
+ if not hasattr(socket, 'ssl'):
+ raise util.Abort(_("can't use TLS: Python SSL support "
+ "not installed"))
+ if ui.configbool('smtp', 'tls') and not ui.configbool('smtp', 'starttls'):
+ ui.note(_('(using tls)\n'))
+ s = smtplib.SMTP_SSL(local_hostname=local_hostname)
+ else:
+ s = smtplib.SMTP(local_hostname=local_hostname)
mailhost = ui.config('smtp', 'host')
if not mailhost:
raise util.Abort(_('smtp.host not configured - cannot send mail'))
@@ -41,11 +49,8 @@
ui.note(_('sending mail: smtp host %s, port %s\n') %
(mailhost, mailport))
s.connect(host=mailhost, port=mailport)
- if ui.configbool('smtp', 'tls'):
- if not hasattr(socket, 'ssl'):
- raise util.Abort(_("can't use TLS: Python SSL support "
- "not installed"))
- ui.note(_('(using tls)\n'))
+ if ui.configbool('smtp', 'starttls'):
+ ui.note(_('(using starttls)\n'))
s.ehlo()
s.starttls()
s.ehlo()

[Mads Kiilerich]

Post by Zhigang Wang
doc/hgrc.5.txt | 3 +++
mercurial/mail.py | 17 +++++++++++------
2 files changed, 14 insertions(+), 6 deletions(-)
# HG changeset patch
# Date 1291967692 -28800
# Node ID d44d0475a776eaffb8d8b34107e357dd43cfbbd3
# Parent 5dac0d04b838d599acdc5db9ee0b7de51de15537
smtp tls: fix for server doesn't support STARTTLS extension.

It seems like this is a repost of your patch from
http://selenic.com/pipermail/mercurial/2010-August/thread.html#34033 ,
and the issues raised there has apparently not been addressed?

(And see http://mercurial.selenic.com/wiki/ContributingChanges - patches
are more welcome on the mercurial-devel list.)

/Mads

[Zhigang Wang]

Post by Mads Kiilerich

 doc/hgrc.5.txt    |   3 +++
 mercurial/mail.py |  17 +++++++++++------
 2 files changed, 14 insertions(+), 6 deletions(-)
# HG changeset patch
# Date 1291967692 -28800
# Node ID d44d0475a776eaffb8d8b34107e357dd43cfbbd3
# Parent  5dac0d04b838d599acdc5db9ee0b7de51de15537
smtp tls: fix for server doesn't support STARTTLS extension.

It seems like this is a repost of your patch from
http://selenic.com/pipermail/mercurial/2010-August/thread.html#34033 , and
the issues raised there has apparently not been addressed?

Yes. It's just a repost. I find this feature is not in hg yet after
months. I don't know what is the best way to do it. I still think this
is the right way to solve it. Let's cooperate to get this feature in.

The problem of this patch is: it changed what 'tls' means. We can
change the logic this way to keep backward compatible: if tls = true,
startls = true by default; if user manually set starttls = false,
we'll use smtplib.SMTP_SSL instead of starttls. Is that OK?

Post by Mads Kiilerich
(And see http://mercurial.selenic.com/wiki/ContributingChanges - patches are
more welcome on the mercurial-devel list.)

Thanks, I'll send to that mailing list in the future.

Zhigang

Post by Mads Kiilerich
/Mads

[Mads Kiilerich]

Post by Zhigang Wang

Post by Mads Kiilerich

Post by Zhigang Wang
doc/hgrc.5.txt | 3 +++
mercurial/mail.py | 17 +++++++++++------
2 files changed, 14 insertions(+), 6 deletions(-)
# HG changeset patch
# Date 1291967692 -28800
# Node ID d44d0475a776eaffb8d8b34107e357dd43cfbbd3
# Parent 5dac0d04b838d599acdc5db9ee0b7de51de15537
smtp tls: fix for server doesn't support STARTTLS extension.

It seems like this is a repost of your patch from
http://selenic.com/pipermail/mercurial/2010-August/thread.html#34033 , and
the issues raised there has apparently not been addressed?

Yes. It's just a repost. I find this feature is not in hg yet after
months. I don't know what is the best way to do it. I still think this
is the right way to solve it. Let's cooperate to get this feature in.

Yes, it would be a nice feature, so let us cooperate on getting it in.

Ignoring the feedback and posting exactly the same patch again is
however not efficient cooperation ;-)

Post by Zhigang Wang
The problem of this patch is: it changed what 'tls' means. We can
change the logic this way to keep backward compatible: if tls = true,
startls = true by default; if user manually set starttls = false,
we'll use smtplib.SMTP_SSL instead of starttls. Is that OK?

That could work, but it seems a bit odd to have a feature that is
unnamed and defined by disabling another feature.

The feature you are introducing is AFAIK not standardized, but it is
what Postfix calls TLS wrapper mode or smtps, and (a non-standard)
/etc/services calls it smtps and "SMTP over SSL (TLS)". (I have seen and
proposed ssmtp, but that seems less common.) How about enabling your
feature with smtp.smtps=true or smtp.tlswrapper=true instead?

As mentioned last time it could make sense to deprecate smtp.tls (in
another patch) and start using smtp.starttls - just to avoid one kind of
confusion (and introduce another).

Finally: I guess someone should start working on verifying the
certificates of these TLS connections ;-)

/Mads

[Zhigang Wang]

Post by Mads Kiilerich

Post by Zhigang Wang

Post by Mads Kiilerich

 doc/hgrc.5.txt    |   3 +++
 mercurial/mail.py |  17 +++++++++++------
 2 files changed, 14 insertions(+), 6 deletions(-)
# HG changeset patch
# Date 1291967692 -28800
# Node ID d44d0475a776eaffb8d8b34107e357dd43cfbbd3
# Parent  5dac0d04b838d599acdc5db9ee0b7de51de15537
smtp tls: fix for server doesn't support STARTTLS extension.

It seems like this is a repost of your patch from
http://selenic.com/pipermail/mercurial/2010-August/thread.html#34033 , and
the issues raised there has apparently not been addressed?

Yes. It's just a repost. I find this feature is not in hg yet after
months. I don't know what is the best way to do it. I still think this
is the right way to solve it. Let's cooperate to get this feature in.

Yes, it would be a nice feature, so let us cooperate on getting it in.
Ignoring the feedback and posting exactly the same patch again is however
not efficient cooperation ;-)

Post by Zhigang Wang
The problem of this patch is: it changed what 'tls' means. We can
change the logic this way to keep backward compatible: if tls = true,
startls = true by default; if user manually set starttls = false,
we'll use smtplib.SMTP_SSL instead of starttls. Is that OK?

That could work, but it seems a bit odd to have a feature that is unnamed
and defined by disabling another feature.
The feature you are introducing is AFAIK not standardized, but it is what
Postfix calls TLS wrapper mode or smtps, and (a non-standard) /etc/services
calls it smtps and "SMTP over SSL (TLS)". (I have seen and proposed ssmtp,
but that seems less common.) How about enabling your feature with
smtp.smtps=true or smtp.tlswrapper=true instead?
As mentioned last time it could make sense to deprecate smtp.tls (in another
patch) and start using smtp.starttls - just to avoid one kind of confusion
(and introduce another).
Finally: I guess someone should start working on verifying the certificates
of these TLS connections ;-)
/Mads

This is the new patch.

The undetermined issue is: when both tls and smtps = true. Currently
will fail for my mail server due to:

File "/home/zhigang/lib64/python2.7/site-packages/mercurial/mail.py",
line 55, in _smtp
s.starttls()
File "/usr/lib64/python2.7/smtplib.py", line 614, in starttls
raise SMTPException("STARTTLS extension not supported by server.")
smtplib.SMTPException: STARTTLS extension not supported by server.

Should we enforce that users cannot set both tls and smtps = true?

Thanks,

Zhigang

[Mads Kiilerich]

Post by Zhigang Wang
This is the new patch.

Thank you.

Please see http://mercurial.selenic.com/wiki/ContributingChanges :
"We prefer patches in the message body so we can review them (no
attachments or URLs!)"

That will make it easier to review and reply in mails.

Post by Zhigang Wang
The undetermined issue is: when both tls and smtps = true. Currently
File "/home/zhigang/lib64/python2.7/site-packages/mercurial/mail.py",
line 55, in _smtp
s.starttls()
File "/usr/lib64/python2.7/smtplib.py", line 614, in starttls
raise SMTPException("STARTTLS extension not supported by server.")
smtplib.SMTPException: STARTTLS extension not supported by server.
Should we enforce that users cannot set both tls and smtps = true?

Yes, I think so.

Another idea that could make it more obvious that the options are
mutually exclusive - and at the same time let us remain backward compatible:

Keep using smtp.tls as before, with the default False and where boolean
true implies starttls. To that we add handling of the values 'starttls'
and 'smtps'. The boolean values could perhaps be deprecated - but kept
for compatibility.

(See the pager option in hgext/pager.py.)

/Mads