{"thread":{"id":"42101","subject":"[git-multimail] smtplib, check certificate","startedAt":"2016-04-21T21:44:50Z","lastAt":"2016-04-24T19:14:43Z","messageCount":5,"participants":["Simon P","Matthieu Moy","Michael Haggerty","Simon Pontié"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"284077","messageId":"571949D2.10507@le-huit.fr","threadId":"42101","inReplyTo":null,"subject":"[git-multimail] smtplib, check certificate","fromName":"Simon P","fromEmail":"simon.git@le-huit.fr","sentAt":"2016-04-21T21:44:50Z","receivedAt":"2016-04-21T21:44:50Z","isPatch":false,"sender":{"key":"simon.git@le-huit.fr","avatar":null},"body":"Hi,\n\nIt seems that smtplib doesn't check if a certificate is valid (signed by\na trusted CA).\n\nFor my personal usage, I patched the starttls code in git-multimail:\nonly for starttls with smtplib.\n\nThis patch is inspired from\n\nhttps://github.com/graingert/secure-smtplib/blob/master/src/secure_smtplib/__init__.py\n\nIt could be easy to add support cert check in for smtps (see\nsecure_smtplib).\n\nThis patch was tested only on git-multimail (v1.2)\n\nIt introduces two new options:\n - multimailhook.smtpcheckcert (default false)\n - multimailhook.smtpcacerts (default\n /etc/ssl/certs/ca-certificates.crt)\n\nBest regards,\nSimon P.\n\n\ndiff --git a/git-multimail/git_multimail.py b/git-multimail/git_multimail.py\nindex fae5c91..b49ed9d 100755\n--- a/git-multimail/git_multimail.py\n+++ b/git-multimail/git_multimail.py\n@@ -57,6 +57,7 @@ import subprocess\n import shlex\n import optparse\n import smtplib\n+import ssl\n import time\n import cgi\n \n@@ -1945,6 +1946,7 @@ class SMTPMailer(Mailer):\n smtpservertimeout=10.0, smtpserverdebuglevel=0,\n smtpencryption='none',\n smtpuser='', smtppass='',\n+ smtpcacerts='/etc/ssl/certs/ca-certificates.crt',smtpcheckcert=False\n ):\n if not envelopesender:\n sys.stderr.write(\n@@ -1974,13 +1976,43 @@ class SMTPMailer(Mailer):\n if self.security == 'none':\n self.smtp = call(smtplib.SMTP, self.smtpserver, timeout=self.smtpservertimeout)\n elif self.security == 'ssl':\n+ if smtpcheckcert:\n+ msg = \"Checking certificate is not supported for ssl, prefer starttls\"\n+ raise smtplib.SMTPException(msg)\n self.smtp = call(smtplib.SMTP_SSL, self.smtpserver, timeout=self.smtpservertimeout)\n elif self.security == 'tls':\n if ':' not in self.smtpserver:\n self.smtpserver += ':587' # default port for TLS\n self.smtp = call(smtplib.SMTP, self.smtpserver, timeout=self.smtpservertimeout)\n- self.smtp.ehlo()\n- self.smtp.starttls()\n+ if smtpcheckcert:\n+ # inspired form:\n+ # https://github.com/graingert/secure-smtplib/blob/master/src/secure_smtplib/__init__.py\n+ # but add the path to trusted ca, and force ceritficate verification.\n+ self.smtp.ehlo_or_helo_if_needed()\n+ if not self.smtp.has_extn(\"starttls\"):\n+ msg = \"STARTTLS extension not supported by server\"\n+ raise smtplib.SMTPException(msg)\n+ (resp, reply) = self.smtp.docmd(\"STARTTLS\")\n+ if resp == 220:\n+ self.smtp.sock = ssl.wrap_socket(\n+ self.smtp.sock,\n+ ca_certs=smtpcacerts,\n+ cert_reqs=ssl.CERT_REQUIRED\n+ )\n+ if not hasattr(self.smtp.sock, \"read\"):\n+ # using httplib.FakeSocket with Python 2.5.x or earlier\n+ self.smtp.sock.read = self.smtp.sock.recv\n+ self.smtp.file = smtplib.SSLFakeFile(self.smtp.sock)\n+ self.smtp.helo_resp = None\n+ self.smtp.ehlo_resp = None\n+ self.smtp.esmtp_features = {}\n+ self.smtp.does_esmtp = 0\n+ else:\n+ msg = \"Wrong answer to the STARTTLS command\"\n+ raise smtplib.SMTPException(msg)\n+ else:\n+ self.smtp.ehlo()\n+ self.smtp.starttls()\n self.smtp.ehlo()\n else:\n sys.stdout.write('*** Error: Control reached an invalid option. ***')\n@@ -3500,6 +3532,8 @@ def choose_mailer(config, environment):\n smtpencryption = config.get('smtpencryption', default='none')\n smtpuser = config.get('smtpuser', default='')\n smtppass = config.get('smtppass', default='')\n+ smtpcacerts = config.get('smtpcacerts', default='/etc/ssl/certs/ca-certificates.crt')\n+ smtpcheckcert = config.get_bool('smtpcheckcert', default='false')\n mailer = SMTPMailer(\n envelopesender=(environment.get_sender() or environment.get_fromaddr()),\n smtpserver=smtpserver, smtpservertimeout=smtpservertimeout,\n@@ -3507,6 +3541,8 @@ def choose_mailer(config, environment):\n smtpencryption=smtpencryption,\n smtpuser=smtpuser,\n smtppass=smtppass,\n+ smtpcacerts=smtpcacerts,\n+ smtpcheckcert=smtpcheckcert\n )\n elif mailer == 'sendmail':\n command = config.get('sendmailcommand')\n"},{"id":"284096","messageId":"vpqoa92rxew.fsf@anie.imag.fr","threadId":"42101","inReplyTo":"571949D2.10507@le-huit.fr","subject":"Re: [git-multimail] smtplib, check certificate","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2016-04-22T06:05:11Z","receivedAt":"2016-04-22T06:05:11Z","isPatch":false,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Simon P writes:\n\n> Hi,\n\nHi, and thanks for the patch.\n\nPlease, add your sign-off and a proper commit message to your patch,\nsee:\n\nhttps://github.com/git-multimail/git-multimail/blob/master/CONTRIBUTING.rst\n\nI'm OK with patches by email, but you may prefer using a pull-request\n(among other things, creating a pull-request triggers a Travis-CI build\nand would have noticed the absence of sign-off and a minor PEP8 issue in\nyour code.\n\nThe patch obviously lacks documentation, and some way to test it.\nActually, the testsuite will fail if you document the configuration\nvariable and they don't appear somewhere in the testsuite. A fully\nautomatic test would be hard to write, but I have a semi-automated\ntestsuite for smtp: some configurations in t/*.config.in, and a script\ntest-email-config to run a test with each of the configurations (then I\ncheck my mailbox). There should be one configuration with a valid\ncertificate and another with a buggy one so that we can check that the\ncertificate is actually checked.\n\n> @@ -1945,6 +1946,7 @@ class SMTPMailer(Mailer):\n> smtpservertimeout=10.0, smtpserverdebuglevel=0,\n> smtpencryption='none',\n> smtpuser='', smtppass='',\n> + smtpcacerts='/etc/ssl/certs/ca-certificates.crt',smtpcheckcert=False\n\nDo you need a default for smtpcheckcert if you already have one in\nconfig.get(smtpcheckcert)? In any case, I'd rather avoid having two\nhardcoded path in the code. If you need\n'/etc/ssl/certs/ca-certificates.crt' in two places, please define a\nconstant elsewhere in the code and use it here.\n\nMissing space after ,.\n\n> + if smtpcheckcert:\n> + # inspired form:\n> + # https://github.com/graingert/secure-smtplib/blob/master/src/secure_smtplib/__init__.py\n> + # but add the path to trusted ca, and force ceritficate verification.\n> + self.smtp.ehlo_or_helo_if_needed()\n> + if not self.smtp.has_extn(\"starttls\"):\n> + msg = \"STARTTLS extension not supported by server\"\n> + raise smtplib.SMTPException(msg)\n> + (resp, reply) = self.smtp.docmd(\"STARTTLS\")\n\nParenthesis around (resp, reply) are not needed, I prefer to omit them.\n\nThanks,\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"284099","messageId":"5719C7A0.50403@alum.mit.edu","threadId":"42101","inReplyTo":"vpqoa92rxew.fsf@anie.imag.fr","subject":"Re: [git-multimail] smtplib, check certificate","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2016-04-22T06:41:36Z","receivedAt":"2016-04-22T06:41:36Z","isPatch":false,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 04/22/2016 08:05 AM, Matthieu Moy wrote:\n> Simon P writes:\n> > This patch is inspired from\n> > \n> > https://github.com/graingert/secure-smtplib/blob/master/src/secure_smtplib/__init__.py\n> \n> Please, add your sign-off and a proper commit message to your patch,\n> see:\n> \n> https://github.com/git-multimail/git-multimail/blob/master/CONTRIBUTING.rst\n\nI hate that we even have to worry about this stuff, but\ngraingert/secure-smtplib looks to be GPLv3, whereas git-multimail is\nGPLv2 (like the Git project and Linux); *not* \"GPLv2 or later\". So if\n\"inspired\" means \"incorporated copyrightable content\" then this patch\nmight be problematic.\n\nMichael\n"},{"id":"284347","messageId":"571D0E83.8010400@le-huit.fr","threadId":"42101","inReplyTo":"vpqoa92rxew.fsf@anie.imag.fr","subject":"Re: [git-multimail] smtplib, check certificate","fromName":"Simon Pontié","fromEmail":"simon@le-huit.fr","sentAt":"2016-04-24T18:20:51Z","receivedAt":"2016-04-24T18:20:51Z","isPatch":false,"sender":{"key":"simon@le-huit.fr","avatar":null},"body":"Le 22/04/2016 08:05, Matthieu Moy a écrit :\n> Hi, and thanks for the patch.\n\nHi.\n\nThanks for your tool, it is very useful!\n\n\n> Please, add your sign-off and a proper commit message to your patch,\n> see:\n\nDone, I also signed my commit via PGP.\n\n> I'm OK with patches by email, but you may prefer using a pull-request\n> (among other things, creating a pull-request triggers a Travis-CI build\n> and would have noticed the absence of sign-off and a minor PEP8 issue in\n> your code.\n\nI don't like github, but I understand your requirement. I submitted a\npull-request of a modified version of the patch:\n https://github.com/git-multimail/git-multimail/pull/150\nI am not a python developer and I am not a Travis-CI user, so I cannot\nunderstand failure messages at:\n https://travis-ci.org/git-multimail/git-multimail/builds/125406555\n\n> The patch obviously lacks documentation\n\nI have added a description in the README file.\n\n> and some way to test it.\n> Actually, the testsuite will fail if you document the configuration\n> variable and they don't appear somewhere in the testsuite. A fully\n> automatic test would be hard to write, but I have a semi-automated\n> testsuite for smtp: some configurations in t/*.config.in\n> test-email-config to run a test with each of the configurations (then I\n> check my mailbox). There should be one configuration with a valid\n> certificate and another with a buggy one so that we can check that the\n> certificate is actually checked.\n\nI have added some test: firstly, I renamed the file `smtp-tls.config.in`\nto `smtp-tls-nocheckcert.config.in` because this configuration do not\ncheck the server certificate. I also added to test files:\n - `smtp-tls-checkcert-unverifiedcert.config.in`\n - `smtp-tls-checkcert-verifiedcert.config.in`\n\nThe first one (unverifiedcert) uses a fake trusted CA list to check the\nunverified server certificate detection (that can be tested with the\ngmail server for example).\n\nThe second one (verifiedcert), assumes that your system have a file\n`/etc/ssl/certs/ca-certificates.crt` with a list of all CA trusted by\nyour system (this file exist in Debian systems). It should succeed with\nthe gmail server.\n\n> Do you need a default for smtpcheckcert if you already have one in\n> config.get(smtpcheckcert)? In any case, I'd rather avoid having two\n> hardcoded path in the code. If you need\n> '/etc/ssl/certs/ca-certificates.crt' in two places, please define a\n> constant elsewhere in the code and use it here.\n\nI have modified the configuration, there is now only one configuration\nvar: smtpCACerts. If it is empty (default), the server certificate is\nnot verified (like before the patch) but a warning is emitted. If the\nvar is set, the targeted file is used to verify the server certificate.\n\nFor now, only the tls configuration is supported.\n\nSimon P.\n"},{"id":"284353","messageId":"571D1B23.7090806@le-huit.fr","threadId":"42101","inReplyTo":"5719C7A0.50403@alum.mit.edu","subject":"Re: [git-multimail] smtplib, check certificate","fromName":"Simon Pontié","fromEmail":"simon@le-huit.fr","sentAt":"2016-04-24T19:14:43Z","receivedAt":"2016-04-24T19:14:43Z","isPatch":false,"sender":{"key":"simon@le-huit.fr","avatar":null},"body":"\nLe 22/04/2016 08:41, Michael Haggerty a écrit :\n> I hate that we even have to worry about this stuff, but\n> graingert/secure-smtplib looks to be GPLv3, whereas git-multimail is\n> GPLv2 (like the Git project and Linux); *not* \"GPLv2 or later\". So if\n> \"inspired\" means \"incorporated copyrightable content\" then this patch\n> might be problematic.\n\nhttps://github.com/git-multimail/git-multimail/pull/150#issuecomment-214020193\n\nThere is a GPLv3 on the secure-smtplib implementation:\nhttps://github.com/graingert/secure-smtplib\nbut I also found an older code equivalent to my patch under MIT license\nhere: https://github.com/cybercase/django-smtp-starttls (code in\ndjango_smtp_starttls.py, licence in setup.py)\n\nI think there is not problem to re-use MIT code.\n\nSimon.\n"}]}