Re: [RFC/PATCH] send-email: die if CA path doesn't exist

From: Jeff King <peff@peff.net>
Date: Nov 24, 2015, 19:58 UTC
Message-ID: <20151124195842.GA7174@sigill.intra.peff.net>
In-Reply-To: <20151120194651.GC21488@serenity.lan>

On Fri, Nov 20, 2015 at 07:46:51PM +0000, John Keeping wrote:

Show 16 quoted lines

> > For people who know their systems are broken and want to proceed anyway,
> > what is the appropriate work-around? Obviously it involves disabling
> > peer verification, but would we want to include instructions for doing
> > so (either in the error message, or perhaps mentioning it in the commit
> > message)?
> 
> The documentation already says:
> 
> 	Set it to an empty string to disable certificate verification.
> 
> It's a bit lost in the middle of a paragraph but I think that is the
> best place for the detail of how to disable verification.
> 
> Having revisted the patch, I do think the message might be a bit terse,
> but I can't think of a reasonably concise way to point at the
> --smtp-ssl-cert-path argument as being the culprit.

Hrm. I was thinking that somebody might not have any clue that --smtp-ssl-cert-path exists, and with this patch their setup would suddenly go from working (well, insecure but passing mail) to broken. They need to know where to look to find that documentation.

But it looks like this code path only triggers if you have set smtp-ssl-cert-path to something bogus. So anybody who does so at least knows about the option.

Which makes me wonder what happens when the cert path isn't defined by Git. The code says:

        if (!defined $smtp_ssl_cert_path) {
                # use the OpenSSL defaults
                return (SSL_verify_mode => SSL_VERIFY_PEER());
        }

What does OpenSSL do when there is no cert? Hopefully it reports a verification failure (and in that sense your patch is making our code consistent with that, which is a good thing).

> Maybe we shouldn't worry too much about that, but should instead put the
> invalid path into the error message:
> 
> 	die "CA path \"$smtp_ssl_cert_path\" does not exist.";

Given what I wrote above, yeah, I'd agree that is sufficient (and I do think mentioning the path is helpful).

-Peff