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.

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.