threads / discuss / 42101

[git-multimail] smtplib, check certificate

Subject: [git-multimail] smtplib, check certificate

## tl;dr

5 messages between Apr 21, 2016 and Apr 24, 2016.

replies: 4people: 4as markdown or json

Simon P· Apr 21, 2016, 21:44 UTC · lore
Hi,

It seems that smtplib doesn't check if a certificate is valid (signed by a trusted CA).

For my personal usage, I patched the starttls code in git-multimail: only for starttls with smtplib.

This patch is inspired from
https://github.com/graingert/secure-smtplib/blob/master/src/secure_smtplib/__init__.py

It could be easy to add support cert check in for smtps (see secure_smtplib).

This patch was tested only on git-multimail (v1.2)
It introduces two new options:
  - multimailhook.smtpcheckcert (default false)
  - multimailhook.smtpcacerts (default
                               /etc/ssl/certs/ca-certificates.crt)

Best regards, Simon P.

diff --git a/git-multimail/git_multimail.py b/git-multimail/git_multimail.py
index fae5c91..b49ed9d 100755
--- a/git-multimail/git_multimail.py
+++ b/git-multimail/git_multimail.py
@@ -57,6 +57,7 @@ import subprocess
 import shlex
 import optparse
 import smtplib
+import ssl
 import time
 import cgi
 
@@ -1945,6 +1946,7 @@ class SMTPMailer(Mailer):
                  smtpservertimeout=10.0, smtpserverdebuglevel=0,
                  smtpencryption='none',
                  smtpuser='', smtppass='',
+                 smtpcacerts='/etc/ssl/certs/ca-certificates.crt',smtpcheckcert=False
                  ):
         if not envelopesender:
             sys.stderr.write(
@@ -1974,13 +1976,43 @@ class SMTPMailer(Mailer):
             if self.security == 'none':
                 self.smtp = call(smtplib.SMTP, self.smtpserver, timeout=self.smtpservertimeout)
             elif self.security == 'ssl':
+                if smtpcheckcert:
+                    msg = "Checking certificate is not supported for ssl, prefer starttls"
+                    raise smtplib.SMTPException(msg)
                 self.smtp = call(smtplib.SMTP_SSL, self.smtpserver, timeout=self.smtpservertimeout)
             elif self.security == 'tls':
                 if ':' not in self.smtpserver:
                     self.smtpserver += ':587'  # default port for TLS
                 self.smtp = call(smtplib.SMTP, self.smtpserver, timeout=self.smtpservertimeout)
-                self.smtp.ehlo()
-                self.smtp.starttls()
+                if smtpcheckcert:
+                    # inspired form:
+                    #   https://github.com/graingert/secure-smtplib/blob/master/src/secure_smtplib/__init__.py
+                    # but add the path to trusted ca, and force ceritficate verification.
+                    self.smtp.ehlo_or_helo_if_needed()
+                    if not self.smtp.has_extn("starttls"):
+                        msg = "STARTTLS extension not supported by server"
+                        raise smtplib.SMTPException(msg)
+                    (resp, reply) = self.smtp.docmd("STARTTLS")
+                    if resp == 220:
+                        self.smtp.sock = ssl.wrap_socket(
+                            self.smtp.sock,
+                            ca_certs=smtpcacerts,
+                            cert_reqs=ssl.CERT_REQUIRED
+                        )
+                        if not hasattr(self.smtp.sock, "read"):
+                            # using httplib.FakeSocket with Python 2.5.x or earlier
+                            self.smtp.sock.read = self.smtp.sock.recv
+                        self.smtp.file = smtplib.SSLFakeFile(self.smtp.sock)
+                        self.smtp.helo_resp = None
+                        self.smtp.ehlo_resp = None
+                        self.smtp.esmtp_features = {}
+                        self.smtp.does_esmtp = 0
+                    else:
+                        msg = "Wrong answer to the STARTTLS command"
+                        raise smtplib.SMTPException(msg)
+                else:
+                    self.smtp.ehlo()
+                    self.smtp.starttls()
                 self.smtp.ehlo()
             else:
                 sys.stdout.write('*** Error: Control reached an invalid option. ***')
@@ -3500,6 +3532,8 @@ def choose_mailer(config, environment):
         smtpencryption = config.get('smtpencryption', default='none')
         smtpuser = config.get('smtpuser', default='')
         smtppass = config.get('smtppass', default='')
+        smtpcacerts = config.get('smtpcacerts', default='/etc/ssl/certs/ca-certificates.crt')
+        smtpcheckcert = config.get_bool('smtpcheckcert', default='false')
         mailer = SMTPMailer(
             envelopesender=(environment.get_sender() or environment.get_fromaddr()),
             smtpserver=smtpserver, smtpservertimeout=smtpservertimeout,
@@ -3507,6 +3541,8 @@ def choose_mailer(config, environment):
             smtpencryption=smtpencryption,
             smtpuser=smtpuser,
             smtppass=smtppass,
+            smtpcacerts=smtpcacerts,
+            smtpcheckcert=smtpcheckcert
             )
     elif mailer == 'sendmail':
         command = config.get('sendmailcommand')
Matthieu Moy· Apr 22, 2016, 06:05 UTC · re: Simon P · lore

Re: [git-multimail] smtplib, check certificate

Simon P <simon.git@le-huit.fr> writes:
> Hi,
Hi, and thanks for the patch.

Please, add your sign-off and a proper commit message to your patch, see:

https://github.com/git-multimail/git-multimail/blob/master/CONTRIBUTING.rst

I'm OK with patches by email, but you may prefer using a pull-request (among other things, creating a pull-request triggers a Travis-CI build and would have noticed the absence of sign-off and a minor PEP8 issue in your code.

The patch obviously lacks documentation, and some way to test it. Actually, the testsuite will fail if you document the configuration variable and they don't appear somewhere in the testsuite. A fully automatic test would be hard to write, but I have a semi-automated testsuite for smtp: some configurations in t/*.config.in, and a script test-email-config to run a test with each of the configurations (then I check my mailbox). There should be one configuration with a valid certificate and another with a buggy one so that we can check that the certificate is actually checked.

Show 5 quoted lines
> @@ -1945,6 +1946,7 @@ class SMTPMailer(Mailer):
>                   smtpservertimeout=10.0, smtpserverdebuglevel=0,
>                   smtpencryption='none',
>                   smtpuser='', smtppass='',
> +                 smtpcacerts='/etc/ssl/certs/ca-certificates.crt',smtpcheckcert=False

Do you need a default for smtpcheckcert if you already have one in config.get(smtpcheckcert)? In any case, I'd rather avoid having two hardcoded path in the code. If you need '/etc/ssl/certs/ca-certificates.crt' in two places, please define a constant elsewhere in the code and use it here.

Missing space after ,.
Show 9 quoted lines
> +                if smtpcheckcert:
> +                    # inspired form:
> +                    #   https://github.com/graingert/secure-smtplib/blob/master/src/secure_smtplib/__init__.py
> +                    # but add the path to trusted ca, and force ceritficate verification.
> +                    self.smtp.ehlo_or_helo_if_needed()
> +                    if not self.smtp.has_extn("starttls"):
> +                        msg = "STARTTLS extension not supported by server"
> +                        raise smtplib.SMTPException(msg)
> +                    (resp, reply) = self.smtp.docmd("STARTTLS")
Parenthesis around (resp, reply) are not needed, I prefer to omit them.
Thanks,
-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/
Michael Haggerty· Apr 22, 2016, 06:41 UTC · re: Matthieu Moy · lore

Re: [git-multimail] smtplib, check certificate

On 04/22/2016 08:05 AM, Matthieu Moy wrote:
Show 9 quoted lines
> Simon P <simon.git@le-huit.fr> writes:
> > This patch is inspired from
> > 
> > https://github.com/graingert/secure-smtplib/blob/master/src/secure_smtplib/__init__.py
> 
> Please, add your sign-off and a proper commit message to your patch,
> see:
> 
> https://github.com/git-multimail/git-multimail/blob/master/CONTRIBUTING.rst

I hate that we even have to worry about this stuff, but graingert/secure-smtplib looks to be GPLv3, whereas git-multimail is GPLv2 (like the Git project and Linux); *not* "GPLv2 or later". So if "inspired" means "incorporated copyrightable content" then this patch might be problematic.

Michael
Simon Pontié· Apr 24, 2016, 19:14 UTC · re: Michael Haggerty · lore

Re: [git-multimail] smtplib, check certificate

Le 22/04/2016 08:41, Michael Haggerty a écrit :
Show 5 quoted lines
> I hate that we even have to worry about this stuff, but
> graingert/secure-smtplib looks to be GPLv3, whereas git-multimail is
> GPLv2 (like the Git project and Linux); *not* "GPLv2 or later". So if
> "inspired" means "incorporated copyrightable content" then this patch
> might be problematic.
https://github.com/git-multimail/git-multimail/pull/150#issuecomment-214020193

There is a GPLv3 on the secure-smtplib implementation: https://github.com/graingert/secure-smtplib but I also found an older code equivalent to my patch under MIT license here: https://github.com/cybercase/django-smtp-starttls (code in django_smtp_starttls.py, licence in setup.py)

I think there is not problem to re-use MIT code.
Simon.
Simon Pontié· Apr 24, 2016, 18:20 UTC · re: Matthieu Moy · lore

Re: [git-multimail] smtplib, check certificate

Le 22/04/2016 08:05, Matthieu Moy a écrit :
> Hi, and thanks for the patch.
Hi.
Thanks for your tool, it is very useful!
> Please, add your sign-off and a proper commit message to your patch,
> see:
Done, I also signed my commit via PGP.
> I'm OK with patches by email, but you may prefer using a pull-request
> (among other things, creating a pull-request triggers a Travis-CI build
> and would have noticed the absence of sign-off and a minor PEP8 issue in
> your code.
I don't like github, but I understand your requirement. I submitted a
pull-request of a modified version of the patch:
  https://github.com/git-multimail/git-multimail/pull/150
I am not a python developer and I am not a Travis-CI user, so I cannot
understand failure messages at:
  https://travis-ci.org/git-multimail/git-multimail/builds/125406555
> The patch obviously lacks documentation
I have added a description in the README file.
Show 9 quoted lines
> and some way to test it.
> Actually, the testsuite will fail if you document the configuration
> variable and they don't appear somewhere in the testsuite. A fully
> automatic test would be hard to write, but I have a semi-automated
> testsuite for smtp: some configurations in t/*.config.in
> test-email-config to run a test with each of the configurations (then I
> check my mailbox). There should be one configuration with a valid
> certificate and another with a buggy one so that we can check that the
> certificate is actually checked.
I have added some test: firstly, I renamed the file `smtp-tls.config.in`
to `smtp-tls-nocheckcert.config.in` because this configuration do not
check the server certificate. I also added to test files:
  - `smtp-tls-checkcert-unverifiedcert.config.in`
  - `smtp-tls-checkcert-verifiedcert.config.in`

The first one (unverifiedcert) uses a fake trusted CA list to check the unverified server certificate detection (that can be tested with the gmail server for example).

The second one (verifiedcert), assumes that your system have a file `/etc/ssl/certs/ca-certificates.crt` with a list of all CA trusted by your system (this file exist in Debian systems). It should succeed with the gmail server.

Show 5 quoted lines
> Do you need a default for smtpcheckcert if you already have one in
> config.get(smtpcheckcert)? In any case, I'd rather avoid having two
> hardcoded path in the code. If you need
> '/etc/ssl/certs/ca-certificates.crt' in two places, please define a
> constant elsewhere in the code and use it here.

I have modified the configuration, there is now only one configuration var: smtpCACerts. If it is empty (default), the server certificate is not verified (like before the patch) but a warning is emitted. If the var is set, the targeted file is used to verify the server certificate.

For now, only the tls configuration is supported.
Simon P.

← back to recent threads