Re: [PATCH v2 2/2] send-email: introduce sendemail.smtpsslcertpath

From: Junio C Hamano <gitster@pobox.com>
Date: Jul 6, 2013, 06:25 UTC
Message-ID: <7v1u7c6w7z.fsf@alter.siamese.dyndns.org>
In-Reply-To: <20130705184333.GN9161@serenity.lan>

John Keeping <john@keeping.me.uk> writes:

> I'd rather have '$smtp_ssl_cert_path ne ""' in the first if condition
> (instead of the '-d $smtp_ssl_cert_path') ...

I agree. The signal for "no certs" should be an explicit "nonsense" value like an empty string, not just a string that does not name an expected filesystem object. Otherwise people can misspell paths and disable the validation by accident.

> Perhaps a complete solution could allow CA files as well.

Yes, that would be a good idea. Care to roll into a "fixup!" patch against [2/2]?

Show 13 quoted lines

> 	if (defined $smtp_ssl_cert_path) {
> 		if ($smtp_ssl_cert_path eq "") {
> 			return (SSL_verify_mode => SSL_VERIFY_NONE);
> 		} elsif (-f $smtp_ssl_cert_path) {
> 			return (SSL_verify_mode => SSL_VERIFY_PEER,
> 				SSL_ca_file => $smtp_ssl_cert_path);
> 		} else {
> 			return (SSL_verify_mode => SSL_VERIFY_PEER,
> 				SSL_ca_path => $smtp_ssl_cert_path);
> 		}
> 	} else {
> 		return (SSL_verify_mode => SSL_VERIFY_PEER);
> 	}

Two things that worry me a bit are:

 (1) At the end user UI level, it may look nice to accept some form
     of --no-option-name to say "I have been using SSL against my
     server with handrolled cert, and I want to keep using the
     verify-none option"; "--ssl-cert-path=" looks somewhat ugly.
     The same goes for [sendemail] ssl_cert_path = "" config.

 (2) How loudly does the new code barf when no configuration is done
     (i.e. we just pass SSL_VERIFY_PEER and let the system default
     CA path to take effect) and the server cert does not validate?
     The warning that triggered this thread, if we had the
     configuration mechanism we have been designing together, would
     have been a good reminder for the user to use it, but would we
     give a similar (less noisy is fine, as long as it is clear)
     diagnostic message?