Re: [git-multimail] smtplib, check certificate

[Matthieu Moy <Matthieu.Moy@grenoble-inp.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.

> @@ -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 ,.

> +                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/