Re: [git-multimail] smtplib, check certificate

From: Simon Pontié <simon@le-huit.fr>
Date: Apr 24, 2016, 18:20 UTC
Message-ID: <571D0E83.8010400@le-huit.fr>
In-Reply-To: <vpqoa92rxew.fsf@anie.imag.fr>

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.

Show 9 quoted lines

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

Show 5 quoted lines

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