Re: [git-multimail] smtplib, check certificate

Simon P [off-list ref] 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.

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/

On 04/22/2016 08:05 AM, Matthieu Moy wrote:

Simon P [off-list ref] writes:

quoted

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

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.

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.

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.

Le 22/04/2016 08:41, Michael Haggerty a écrit :

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.