Re: [git-multimail] smtplib, check certificate

From: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
Date: Apr 22, 2016, 06:05 UTC
Message-ID: <vpqoa92rxew.fsf@anie.imag.fr>
In-Reply-To: <571949D2.10507@le-huit.fr>

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/